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

Handle decreasing l1 block number #84

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

jbearer
Copy link
Member

@jbearer jbearer commented Oct 6, 2023

No description provided.

In particular, this removes a call to GetBlockByNumber, where the
resulting block was used only to get the block number! This should
reduce Infura usage by one call per zkevm node per HotShot block.
This allows us to use the L1 head from HotShot as the L1 block
number when reading batches from NewBlocks events in regular mode,
without messing up the rest of the synchronizer which assumes the
L1 block number is the current L1 block we're synchronizing. This
brings the regular node into consistency with the preconfirmations
node.
@jbearer jbearer requested a review from nomaxg October 6, 2023 12:10
This should make the node more robust if this case ever does
happen. It shouldn't happen, and if it does we'll need to debug
and fix it. But at least we have a chance that the node will keep
running.
}
log.Infof("L1 block %d does not already exist, adding to database", blockNumber)
err = s.state.AddBlock(s.ctx, &b, dbTx)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the behavior if the contract event handler is concurrently calling AddBlock? Is that a race condition we need to address here? If the contract handler adds the block between lines 619 and here and 645 errors out, we wouldn't need to bubble up an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should not be possible because we hold a lock on the DB here (everything done with dbTx should be atomic).

Also, AddBlock is idempotent, so even if this race did happen, 645 should succeed and everything should be ok.

Copy link
Contributor

@nomaxg nomaxg left a comment

Choose a reason for hiding this comment

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

LGTM just one question about a potential race condition

@jbearer jbearer merged commit 008843e into hotshot-integration Oct 6, 2023
2 checks passed
@jbearer jbearer deleted the fix/decreasing-l1-block-number branch October 6, 2023 14:49
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.

2 participants