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

No more cached checkpoints in agents #550

Merged
merged 23 commits into from
Jun 8, 2022

Conversation

tkporter
Copy link
Collaborator

@tkporter tkporter commented Jun 7, 2022

Once again I'm apologizing for a big PR 😅

For motivation and what the high level plan was going into this PR, see this comment: #529 (comment)

The meat of this PR includes:

  • Following the removal of cached checkpoints from the Inbox, there is no longer anything to index in AbacusCommon (this is the stale name for Mailbox.sol). So I removed AbacusCommonIndexer and friends.
  • We previously had some non-functional code in ContractSync to fetch cached checkpoints. We were never using this, and especially now that we will not care about cached checkpoints on the Inbox, I decided to remove it. It was just taking up space and not useful in its current form, imo.
  • I moved the fns latest_cached_root and latest_cached_checkpoint out of AbacusCommon and into Outbox, because they no longer live in Mailbox.sol.
  • The fn latest_checkpoint was added on the Outbox, which is the view function that validators now rely upon to learn about new checkpoints to sign. The old latest_cached_checkpoint isn't relevant for validators anymore and the finality of it doesn't really matter, so latest_checkpoint is now the one in which a block lag can be provided to in order to ensure that the latest checkpoint a validator needs to sign has reached finality.
  • The process fn on the Inbox in the agents is now removed. On the contract side, this is only callable by the InboxValidatorManager contract, so the agents will never need to call Inbox.process themselves. Instead, the process function now lives in InboxValidatorManager within the agent codebase, and it takes in the MultisigSignedCheckpoint and the message & proof.
  • As a reminder, the relayer up until this PR had two main tasks spawned per inbox: the CheckpointRelayer and the MessageProcessor. The original purpose was that the CheckpointRelayer needed to only relay checkpoints to inboxes that had messages destined for them. Since cached checkpoints on the Inboxes are no longer a thing, this isn't necessary. So this PR renames CheckpointRelayer -> CheckpointFetcher, and no longer spawns a new CheckpointFetcher per inbox, instead spawning a single CheckpointFetcher.
  • The CheckpointFetcher/CheckpointRelayer used to have an option to immediately process messages. This is removed, and all message processing occurs in the MessageProcessor now.
  • The CheckpointFetcher is responsible for continually polling for new signed checkpoints. Upon finding a signed checkpoint at an index higher than the last known signed checkpoint, it sends the signed checkpoint via a channel to all MessageProcessors. Before making CheckpointFetcher a singleton, I had originally used mpsc channels, but now that CheckpointFetcher is a singleton a watch channel is used.

Some drive-by changes:

  • Removed abacus-cli
  • Fixed the deploy tooling that was setting the domain (& therefore the domain hash which is required for correctly verifying signatures) on the InboxValidatorManager to the inbox domain instead of correctly using the outbox domain. Can't wait for e2e tests!
  • Some of the logging around unsuccessfully processing messages was on the warn/error level, when this is (mostly) out of our control in cases where a message's handler just reverts. I reduced them to info
  • Changed some relayer settings names to what I felt was easier to understand without context
  • Removed some destination_and_nonce related code that is no longer relevant

Tested using ./run-locally.sh

Fixes #529

@tkporter tkporter requested review from webbhorn and mattiekat and removed request for webbhorn June 7, 2022 16:38
Copy link
Contributor

@mattiekat mattiekat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I understood everything going on here, but if it is working during tests I have no issues with it, just a few minor things, some of which are more for going forward than this PR.

rust/agents/relayer/src/checkpoint_fetcher.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/relayer.rs Show resolved Hide resolved
rust/agents/relayer/src/relayer.rs Show resolved Hide resolved
rust/agents/relayer/src/relayer.rs Show resolved Hide resolved
@tkporter tkporter merged commit d120a76 into main Jun 8, 2022
@tkporter tkporter deleted the trevor/agents-no-cached-checkpoints branch June 8, 2022 11:00
tkporter added a commit that referenced this pull request Jun 8, 2022
* Update ABIs, compiles. Removed Inbox indexer and cached checkpoints from the Inbox

* Rm checkpoint indexing test

* Update InboxValidatorManager, just need to connect the pieces now

* Add channels

* So close

* Fix InboxValidatorManager deploy

* Fix bug where prover_sync wasn't in line with latest_signed_checkpoint

* Rm immediate message processing, clean up

* rm abacus-cli

* more cleanup

* rename / rm some settings

* TS renames / rms

* Lower some processing failure logs to info

* Checkpoint fetcher doesn't need the CommittedMessages

* Hardcode kathy dispatching

* Move to watch channel

* I'm sorry clippy

* nits

* rm some nonce related stuff

* PR comments
tkporter added a commit that referenced this pull request Jun 8, 2022
…asPaymaster ABI (#562)

* Make InterchainGasPaymaster optional

* No more cached checkpoints in agents (#550)

* Update ABIs, compiles. Removed Inbox indexer and cached checkpoints from the Inbox

* Rm checkpoint indexing test

* Update InboxValidatorManager, just need to connect the pieces now

* Add channels

* So close

* Fix InboxValidatorManager deploy

* Fix bug where prover_sync wasn't in line with latest_signed_checkpoint

* Rm immediate message processing, clean up

* rm abacus-cli

* more cleanup

* rename / rm some settings

* TS renames / rms

* Lower some processing failure logs to info

* Checkpoint fetcher doesn't need the CommittedMessages

* Hardcode kathy dispatching

* Move to watch channel

* I'm sorry clippy

* nits

* rm some nonce related stuff

* PR comments

* Update InterchainGasPaymaster ABI

* update configs with correct addresses

* rm index from

* make clippy happy
yorhodes pushed a commit that referenced this pull request Jun 8, 2022
* Update ABIs, compiles. Removed Inbox indexer and cached checkpoints from the Inbox

* Rm checkpoint indexing test

* Update InboxValidatorManager, just need to connect the pieces now

* Add channels

* So close

* Fix InboxValidatorManager deploy

* Fix bug where prover_sync wasn't in line with latest_signed_checkpoint

* Rm immediate message processing, clean up

* rm abacus-cli

* more cleanup

* rename / rm some settings

* TS renames / rms

* Lower some processing failure logs to info

* Checkpoint fetcher doesn't need the CommittedMessages

* Hardcode kathy dispatching

* Move to watch channel

* I'm sorry clippy

* nits

* rm some nonce related stuff

* PR comments
@asaj
Copy link
Contributor

asaj commented Jun 8, 2022

I believe this closes #472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relayer should use InboxValidatorManager.process and not cache checkpoints on the Inbox
4 participants