-
Notifications
You must be signed in to change notification settings - Fork 431
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… trevor/agents-no-cached-checkpoints
… trevor/agents-no-cached-checkpoints
tkporter
commented
Jun 7, 2022
webbhorn
reviewed
Jun 7, 2022
webbhorn
reviewed
Jun 7, 2022
webbhorn
reviewed
Jun 7, 2022
webbhorn
reviewed
Jun 7, 2022
webbhorn
reviewed
Jun 7, 2022
webbhorn
approved these changes
Jun 7, 2022
webbhorn
approved these changes
Jun 7, 2022
mattiekat
approved these changes
Jun 7, 2022
There was a problem hiding this 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.
… trevor/agents-no-cached-checkpoints
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
I believe this closes #472 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
AbacusCommonIndexer
and friends.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.latest_cached_root
andlatest_cached_checkpoint
out of AbacusCommon and into Outbox, because they no longer live in Mailbox.sol.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 oldlatest_cached_checkpoint
isn't relevant for validators anymore and the finality of it doesn't really matter, solatest_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.process
fn on theInbox
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, theprocess
function now lives inInboxValidatorManager
within the agent codebase, and it takes in the MultisigSignedCheckpoint and the message & proof.CheckpointRelayer
and theMessageProcessor
. The original purpose was that theCheckpointRelayer
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 renamesCheckpointRelayer
->CheckpointFetcher
, and no longer spawns a newCheckpointFetcher
per inbox, instead spawning a singleCheckpointFetcher
.CheckpointFetcher
/CheckpointRelayer
used to have an option to immediately process messages. This is removed, and all message processing occurs in theMessageProcessor
now.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 allMessageProcessors
. Before makingCheckpointFetcher
a singleton, I had originally used mpsc channels, but now thatCheckpointFetcher
is a singleton a watch channel is used.Some drive-by changes:
abacus-cli
InboxValidatorManager
to the inbox domain instead of correctly using the outbox domain. Can't wait for e2e tests!info
destination_and_nonce
related code that is no longer relevantTested using
./run-locally.sh
Fixes #529