Sequencer catchup #1151

sveitser · 2024-02-26T19:13:14Z

Add function to fetch forgotten leaves of the fee merkle tree.
Add function to fetch merkle tree frontier (not hooked up yet, might change).

TODO

Get a the right view number for querying catchup API.
Tests.
Many TODOs in code.

jbearer

Definitely on the right track. Let's ask the consensus team about where to get the view number

jbearer · 2024-02-26T19:25:07Z

sequencer/src/catchup.rs

+                }
+            }
+            tracing::warn!("Could not fetch account from any peer, retrying");
+            // TODO: backoff? for testing a function that doesn't loop forever is probably more convenient.


IMO:

Make backoff a parameter to set in the constructor. We can set it really short for tests that are expected to fail a few times before eventually succeeding (testing the backoff case)

Make a separate function that returns an error, for easy unit testing. Then the main function just call that function in a loop.

sequencer/src/catchup.rs

tbro · 2024-02-26T19:56:37Z

sequencer/src/state.rs

+                Some(balance.cloned().unwrap_or_default().add(amount))
+            })
+            .expect("update_with succeeds");
+        // TODO: can this really not fail?


I don't think so, b/c both Some or None cases are handled in the closure. I feel that this can be written more succinctly, though, by remembering the account balance in the Ok(NotInMemory) case of the match on update_with (as you've done ~line 143).

sequencer/src/catchup.rs

jbearer · 2024-03-06T15:54:18Z

sequencer/src/header.rs

+            .await;
+
+        // Insert missing fee state entries
+        let mut state = parent_state.clone();


Since we're cloning state here we could pass it into from_info by value and avoid a second clone (although clone should be fairly cheap now that stuff is Arced)

sveitser · 2024-03-07T13:34:39Z

Edit: the issue sees to be the impl Into Iterator in the function and the wrong error message from the compiler mentioned in the issue below. So I'm unblocked again.

@jbearer I tried to use box dyn trait in the last commit 053d95f, this fails to compile with the error message

error[E0391]: cycle detected when computing type of opaque `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new::{opaque#0}`
   --> sequencer/src/header.rs:256:5

full error message at the end.

I wonder if the actual problem is object safety due to using async in the trait and the error message is just wrong (rust-lang/rust#119502).

I saw here here that using async_trait might help but the error message is the same.

    Checking sequencer v0.1.0 (/Users/lulu/r/EspressoSystems/espresso-sequencer/sequencer)
error[E0391]: cycle detected when checking effective visibilities
    |
note: ...which requires computing type of `state::<impl at sequencer/src/state.rs:254:1: 254:47>::validate_and_apply_header::{opaque#0}`...
   --> sequencer/src/state.rs:263:5
    |
263 | /     async fn validate_and_apply_header(
264 | |         &self,
265 | |         instance: &Self::Instance,
266 | |         parent_leaf: &Leaf,
267 | |         proposed_header: &Header,
268 | |     ) -> Result<Self, Self::Error> {
    | |__________________________________^
note: ...which requires computing type of opaque `state::<impl at sequencer/src/state.rs:254:1: 254:47>::validate_and_apply_header::{opaque#0}`...
   --> sequencer/src/state.rs:263:5
    |
263 | /     async fn validate_and_apply_header(
264 | |         &self,
265 | |         instance: &Self::Instance,
266 | |         parent_leaf: &Leaf,
267 | |         proposed_header: &Header,
268 | |     ) -> Result<Self, Self::Error> {
    | |__________________________________^
note: ...which requires type-checking `state::<impl at sequencer/src/state.rs:254:1: 254:47>::validate_and_apply_header`...
   --> sequencer/src/state.rs:263:5
    |
263 | /     async fn validate_and_apply_header(
264 | |         &self,
265 | |         instance: &Self::Instance,
266 | |         parent_leaf: &Leaf,
267 | |         proposed_header: &Header,
268 | |     ) -> Result<Self, Self::Error> {
    | |__________________________________^
    = note: ...which again requires checking effective visibilities, completing the cycle
note: cycle used when type-checking `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new`
   --> sequencer/src/header.rs:256:5
    |
256 | /     async fn new(
257 | |         parent_state: &ValidatedState,
258 | |         instance_state: &NodeState,
259 | |         parent_leaf: &Leaf,
260 | |         payload_commitment: VidCommitment,
261 | |         metadata: <<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
262 | |     ) -> Self {
    | |_____________^
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

error[E0391]: cycle detected when computing type of opaque `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new::{opaque#0}`
   --> sequencer/src/header.rs:256:5
    |
256 | /     async fn new(
257 | |         parent_state: &ValidatedState,
258 | |         instance_state: &NodeState,
259 | |         parent_leaf: &Leaf,
260 | |         payload_commitment: VidCommitment,
261 | |         metadata: <<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
262 | |     ) -> Self {
    | |_____________^
    |
note: ...which requires type-checking `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new`...
   --> sequencer/src/header.rs:256:5
    |
256 | /     async fn new(
257 | |         parent_state: &ValidatedState,
258 | |         instance_state: &NodeState,
259 | |         parent_leaf: &Leaf,
260 | |         payload_commitment: VidCommitment,
261 | |         metadata: <<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
262 | |     ) -> Self {
    | |_____________^
    = note: ...which requires checking effective visibilities...
note: ...which requires computing type of `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new::{opaque#0}`...
   --> sequencer/src/header.rs:256:5
    |
256 | /     async fn new(
257 | |         parent_state: &ValidatedState,
258 | |         instance_state: &NodeState,
259 | |         parent_leaf: &Leaf,
260 | |         payload_commitment: VidCommitment,
261 | |         metadata: <<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
262 | |     ) -> Self {
    | |_____________^
    = note: ...which again requires computing type of opaque `header::<impl at sequencer/src/header.rs:255:1: 255:38>::new::{opaque#0}`, completing the cycle
note: cycle used when comparing an impl and trait method signature, inferring any hidden `impl Trait` types in the process
   --> sequencer/src/header.rs:256:5
    |
256 | /     async fn new(
257 | |         parent_state: &ValidatedState,
258 | |         instance_state: &NodeState,
259 | |         parent_leaf: &Leaf,
260 | |         payload_commitment: VidCommitment,
261 | |         metadata: <<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
262 | |     ) -> Self {
    | |_____________^
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

For more information about this error, try `rustc --explain E0391`.
error: could not compile `sequencer` (lib) due to 2 previous errors

sequencer/src/state.rs

- Add functions to fetch from peers. [squashed version of #1151]

jbearer · 2024-03-08T16:39:41Z

sequencer/src/header.rs

+                .chain(l1_deposits.iter().map(|info| info.account())),
+        );
+
+        // Fetch missing fee state entries


We also need to fetch the blocks frontier if missing

sveitser · 2024-03-08T18:22:14Z

sequencer/src/catchup.rs

            .expect_ok()
            .unwrap();
-        mt.remember(view.get_u64(), elem, proof.clone())


This is currently where the test panics. The element returned by lookup doesn't match the one passed in to the function.

sveitser · 2024-03-11T13:40:16Z

@jbearer @tbro I don't know if anyone reviewed my changes in detail. I looked at the work that @jbearer added and it looks great.

tbro · 2024-03-11T22:11:38Z

sequencer/src/state.rs

@@ -338,6 +477,12 @@ impl FeeAmount {
    }
 }

+impl From<u64> for FeeAmount {


I think we had this before, but we removed it. Why are we putting it back again?

I think it's currently only used in one place, when deleting it I get:

error[E0277]: the trait bound `state::FeeAmount: From<{integer}>` is not satisfied --> sequencer/src/api.rs:920:56 | 920 | state.prefund_account(Default::default(), 1000.into()); | ^^^^ the trait `From<{integer}>` is not implemented for `state::FeeAmount` | = help: the trait `From<ethers::types::U256>` is implemented for `state::FeeAmount` = help: for that trait implementation, expected `ethers::types::U256`, found `{integer}` = note: required for `{integer}` to implement `Into<state::FeeAmount>`

I don't really mind keeping it, it's simple and will probably be used more once we add more tests.

I think it's just something that makes sense since FeeAmount is supposed to act like a number. E.g. U256 also implements this

Yea I don't think it hurts anything either but you can just do: U256::from(1000).into().

jbearer

@sveitser I did review your changes so if you've reviewed my changes I think we're good

- Add functions to fetch from peers. - Re-organize block building 1. Check what's missing 2. Fetch missing data from peers 3. Build block - Restore merkle tree frontier - dd backoff to state peers - Make state peers configurable - Add nightly dev shell - Initial mock implementation of StateCatchup - Address review comments

…nt builder IDs

This commit adds catchup to the restartability test, so that it should in theory pass now. However, it is still failing due to an error from HotShot: "Couldn't find parent view in state map". I will look into this more next week. For now, the test remains ignored.

sveitser requested a review from tbro February 26, 2024 19:13

jbearer reviewed Feb 26, 2024

View reviewed changes

tbro reviewed Feb 26, 2024

View reviewed changes

tbro force-pushed the instance-state-add-l1-client branch 4 times, most recently from 8ac034a to d652382 Compare February 29, 2024 18:11

tbro mentioned this pull request Feb 22, 2024

Set up "instance state" #966

Closed

2 tasks

tbro force-pushed the instance-state-add-l1-client branch from 5834477 to a01aad7 Compare February 29, 2024 23:50

Base automatically changed from instance-state-add-l1-client to main March 1, 2024 16:27

sveitser force-pushed the ma/sequencer-catchup branch from 525719e to 0586141 Compare March 4, 2024 13:36

jbearer reviewed Mar 6, 2024

View reviewed changes

tbro reviewed Mar 7, 2024

View reviewed changes

sequencer/src/state.rs Outdated Show resolved Hide resolved

tbro reviewed Mar 7, 2024

View reviewed changes

sequencer/src/state.rs Outdated Show resolved Hide resolved

tbro mentioned this pull request Mar 7, 2024

Add get_pending_deposits and test #1188

Merged

tbro pushed a commit that referenced this pull request Mar 7, 2024

WIP: sequencer catchup

0763d56

- Add functions to fetch from peers. [squashed version of #1151]

tbro mentioned this pull request Mar 7, 2024

Replace calls to fetch_fee_receipts with L1Client.get_finalized_deposits #1211

Merged

jbearer reviewed Mar 8, 2024

View reviewed changes

sveitser commented Mar 8, 2024

View reviewed changes

sveitser marked this pull request as ready for review March 11, 2024 14:42

sveitser requested review from nomaxg, philippecamacho and ImJeremyHe as code owners March 11, 2024 14:42

tbro reviewed Mar 11, 2024

View reviewed changes

jbearer approved these changes Mar 12, 2024

View reviewed changes

tbro approved these changes Mar 12, 2024

View reviewed changes

sveitser and others added 2 commits March 12, 2024 16:39

Fix blocks catchup; feature-flag test code

05df1aa

jbearer added 2 commits March 12, 2024 16:39

Add end-to-end catchup test, make things work when nodes have differe…

1ec5e6b

…nt builder IDs

Update test_restart

f1cee9b

This commit adds catchup to the restartability test, so that it should in theory pass now. However, it is still failing due to an error from HotShot: "Couldn't find parent view in state map". I will look into this more next week. For now, the test remains ignored.

sveitser force-pushed the ma/sequencer-catchup branch from 7cbf7d5 to f1cee9b Compare March 12, 2024 15:43

sveitser merged commit 97d28e5 into main Mar 12, 2024
11 checks passed

sveitser deleted the ma/sequencer-catchup branch March 12, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sequencer catchup #1151

Sequencer catchup #1151

sveitser commented Feb 26, 2024 •

edited

Loading

jbearer left a comment

jbearer Feb 26, 2024

tbro Feb 26, 2024

jbearer Mar 6, 2024

sveitser commented Mar 7, 2024 •

edited

Loading

jbearer Mar 8, 2024

sveitser Mar 8, 2024 •

edited

Loading

sveitser commented Mar 11, 2024

tbro Mar 11, 2024

sveitser Mar 12, 2024 •

edited

Loading

jbearer Mar 12, 2024

tbro Mar 12, 2024

jbearer left a comment

Sequencer catchup #1151

Sequencer catchup #1151

Conversation

sveitser commented Feb 26, 2024 • edited Loading

TODO

jbearer left a comment

Choose a reason for hiding this comment

jbearer Feb 26, 2024

Choose a reason for hiding this comment

tbro Feb 26, 2024

Choose a reason for hiding this comment

jbearer Mar 6, 2024

Choose a reason for hiding this comment

sveitser commented Mar 7, 2024 • edited Loading

jbearer Mar 8, 2024

Choose a reason for hiding this comment

sveitser Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

sveitser commented Mar 11, 2024

tbro Mar 11, 2024

Choose a reason for hiding this comment

sveitser Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

jbearer Mar 12, 2024

Choose a reason for hiding this comment

tbro Mar 12, 2024

Choose a reason for hiding this comment

jbearer left a comment

Choose a reason for hiding this comment

sveitser commented Feb 26, 2024 •

edited

Loading

sveitser commented Mar 7, 2024 •

edited

Loading

sveitser Mar 8, 2024 •

edited

Loading

sveitser Mar 12, 2024 •

edited

Loading