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

Sequencer better DA fork handling & minor improvements #1718

Open
wants to merge 23 commits into
base: nightly
Choose a base branch
from

Conversation

yaziciahmet
Copy link
Contributor

@yaziciahmet yaziciahmet commented Jan 16, 2025

Description

Panic if we are on the wrong side of the DA fork. It is better to not continue with an invalid state and manually recover, which we will have to do until we do a very smart handling of DA forks.

To ensure no forks happened while sequencer restart, we check that the head soft confirmation's l1 height still corresponds to the same l1 hash we stored.

To ensure no forks happened while sequencer is running, in da block monitor, if the queried l1 block height is same as the previously queried l1 block, we ensure that their hashes match as well, if 1 block generated, we ensure that the prev hash matches with the previously queried l1 block, if more than 1 block generated, we re-query the previously queried l1 block and ensure it's hash is still the same.

Also made some minor improvements to sequencer.

Linked Issues

Testing

Describe how these changes were tested. If you've added new features, have you added unit tests?

Docs

Describe where this code is documented. If it changes a documented interface, have the docs been updated?

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 89.55224% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.2%. Comparing base (3ce2289) to head (c499156).

Files with missing lines Patch % Lines
crates/sequencer/src/runner.rs 89.5% 7 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/sequencer/src/runner.rs 88.8% <89.5%> (+0.5%) ⬆️

... and 5 files with indirect coverage changes

@yaziciahmet yaziciahmet marked this pull request as ready for review January 20, 2025 13:54
@auto-assign auto-assign bot requested a review from jfldde January 20, 2025 13:54
@yaziciahmet yaziciahmet changed the title Sequencer better l1 block tracking & minor improvements Sequencer better DA fork handling & minor improvements Jan 20, 2025

Ok((last_finalized_block, l1_fee_rate))
}

async fn ensure_no_da_forks<Da: DaService>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to implement this for all other nodes? because they also follow the wrong fork, at which point, being at the wrong DA block might lead them to misbehave.

I am not totally sure, but this is something to check for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to me like there is nothing to do for other nodes for this PR specifically:

  • light client prover: light client proof execution will fail due to header chain verification (prev_hash will not be equal), hence it wont continue
  • batch prover: there won't be new seq comms since sequencer will be stopped so no new batch proofs. we will have to manually rollback to the start of the fork block anyways, hence, it doesn't matter if batch prover is up without producing any proof.
  • full node: similar logic with batch prover, no new seq comms, and we will have to rollback manually anyways.

Remaining work seems to me to be blocked by rollback feat for our nodes. Once we have proper rollback functionality, we can detect forks in all types of nodes, rollback to the beginning of the fork, and continue happily ever after from that point on, and we won't need manual intervention at all.

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.

Sequencer smarter L1 block increase
2 participants