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

Rust relayer cannot build metadata for an aggregationIsm(trustedRelayerIsm) #4675

Open
tkporter opened this issue Oct 14, 2024 · 1 comment
Labels

Comments

@tkporter
Copy link
Collaborator

tkporter commented Oct 14, 2024

Problem

Context:

  • The trusted relayer ISM looks at state in the Mailbox to determine who the msg.sender was that originally called the process function there.
  • The Rust relayer's logic for building aggregation ISM metadata involves eth_estimateGas calls for each of the inner ISM verify functions so that we can figure out which of the inner ISMs would use the least amount of gas

The problem is that calling a trusted relayer ISM's verify function directly in that eth_estimateGas results in the Mailbox state never being updated to indicate who called Mailbox.process (because Mailbox.process is never even called!)

Solution

Not totally sure, but options i see:

  • Nam and I sorta recently discussed on Discord that this aggregation ISM logic may be overengineered, and we could just rely on some kind of heuristics to figure out which inner ISMs to prioritize (we'd wanna continue to prioritize message ID ISMs over merkle proof ones, for example). So there may be room to just rm this estimate gas in favor of some heuristics
  • eth_estimateGas lets you specify state overrides (docs). I don't think this is a great path though as this empirically has been outside the "core RPC spec" I've seen chains adhere to. I think we're likely to find chains that don't support state overrides. This would also have the relayer start to be aware of Mailbox contract
    internals (to find which storage variable to override), whereas so far the relayer has just used Mailbox APIs and internals have been abstracted away. This could be important if we get ourselves in a scenario where some Mailbox deployments have different storage layouts than other previously deployed ones (which I could totally see -- afaik the contracts team hasn't made a point of retaining the storage layout when new changes are made)
  • others ?
@tkporter tkporter added the agent label Oct 14, 2024
@yorhodes
Copy link
Member

  • Another option is to rely on the configured ordering of the submodules in the aggregation ISM. This puts the onus of gas optimization on the configuration rather than the relayer (although we could probably do this intelligently in the SDK deployers). Then we can omit the individual verify calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

2 participants