Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink "message context" #1151

Closed
Stebalien opened this issue Nov 27, 2022 · 7 comments
Closed

Rethink "message context" #1151

Stebalien opened this issue Nov 27, 2022 · 7 comments
Labels
P2 P2: Should be resolved
Milestone

Comments

@Stebalien
Copy link
Member

We currently have a syscalls to lookup a message context and a network context where:

  1. The "message" context includes "message" related things (from, to, etc.).
  2. The "network" context includes "network" related things (epoch, timestamp, basefee, etc.).

However, the "message" context is a bit funky because it includes the origin, gas premium, and gas limit. Really, its a mix of message specific information and "invocation context" related things.

We should consider either splitting it up into multiple parts, or renaming it back to InvocationContext.

@Stebalien Stebalien added the P2 P2: Should be resolved label Nov 27, 2022
@Stebalien Stebalien added this to the M2.2 milestone Nov 27, 2022
@mriise
Copy link
Contributor

mriise commented Nov 28, 2022

I would personally prefer to split into multiple parts.

@Stebalien
Copy link
Member Author

Alternatively, find some way to embed this contextual information in the actor when instantiating it.

@Stebalien
Copy link
Member Author

Basically, think about forward compatibility and performance.

@Stebalien
Copy link
Member Author

Alternative: split these contexts back up into individual functions (e.g., get_epoch, etc.).

We moved away from this approach for performance reasons (crossing the syscall boundary was expensive) but we may be able to avoid all of this without crossing the syscall boundary if we:

  1. Use multiple memories (i.e., a second memory contextual information).
  2. Use multiple wasm modules and/or add a few wasm functions to the actor that read data out of this second memory.

The benefit of this approach is that:

  1. It has no syscall overhead.
  2. We can statically assert that actors cannot directly access this second memory.
  3. We can expose additional information to actors without having to change existing datastructures. I.e., it makes API evolution easier.
  4. We reduce our reliance (slightly) on "C ABI layout". We still have a few "C ABI" structures, but we'd have fewer.

@Stebalien
Copy link
Member Author

Ok, so, we'd likely want to bundle these all into a single wasm module which means removing all the namespacing we have on the current APIs. I.e., we currently have methods like network::context. We'd change them to fvm::network_context (or fvm1::network_context if we wanted to version the API).

@Stebalien
Copy link
Member Author

Ah HA! Ok, we don't even need to do this at runtime. We can:

  1. Expose globals at runtime.
  2. Link wasm "shims" at compile time. LLD (https://lld.llvm.org/WebAssembly.html) will do this, so this shouldn't be complex or anything.

This lets us reduce the API surface to just "import these wasm globals".

@Stebalien
Copy link
Member Author

While exploring this issue, we decided it likely wasn't the right approach. See #1908 for the current thinking.

@Stebalien Stebalien closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 P2: Should be resolved
Projects
None yet
Development

No branches or pull requests

2 participants