fix(starknet_l1_provider): catch_up_height from sync client

[giladchase]

Most of this is boilerplate, major changes are:

1. catch up height is now lazily initialized by the spawned sync task
   which continously asks the sync state for its height. This can be
   None if the sync is starting up.
2. Until catch up height is ready, the rest of the startup flow is
   unchanged, with the addition that before the spawned task starts
   generating commit-blocks to sync up the provider, it'll first update
   the catch up height, which is synced with the bootstrapper.

[reviewable-StarkWare]

This change is 

[giladchase]

• fix(starknet_l1_provider): catch_up_height from sync client #4967 👈 (View in Graphite)
• fix(starknet_l1_provider): init from LogStateUpdate height #4898
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[alonh5]

Reviewed 8 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r2 (raw file):

    pub fn is_caught_up(&self, current_provider_height: BlockNumber) -> bool {
        self.catch_up_height()
            .is_some_and(|catch_up_height| catch_up_height <= current_provider_height)

This is the logical direction, makes it easier to read.
Also what is the current provider height defined as? is t the last retrieved block or the first unretrieved block?
Please document this and adjust here accordingly

Suggestion:

current_provider_height >= catch_up_height 

---

crates/starknet_l1_provider/src/bootstrapper.rs line 137 at r2 (raw file):

    let catch_up_height = *catch_up_height.get().expect("Initialized above");

    while current_height <= catch_up_height {

can you use is_caught_up()?

Code quote:

current_height <= catch_up_height

---

crates/starknet_l1_provider/src/l1_provider.rs line 215 at r2 (raw file):

    sync_client: SharedStateSyncClient,
    scraper_synced_startup_height: BlockNumber,
) -> L1ProviderResult<L1Provider> {

What can fail here?

---

crates/starknet_l1_provider/src/l1_provider.rs line 236 at r2 (raw file):

        .map(|catch_up_height_override| {
            warn!(
                "Initializing L1Provider with OVERRIDDEN catch-up height: \

Will this warn happen only upon override?

[ArniStarkware]

crates/starknet_sequencer_node/src/components.rs line 264 at r4 (raw file):

                    provider_startup_height,
                )
                .unwrap(),

Why should this fail?
Can this be an expect? Every other fail in this function is very well documented.

Code quote:

unwrap()

[alonh5]

Reviewed 5 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @giladchase)

[giladchase]

Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is the logical direction, makes it easier to read.
Also what is the current provider height defined as? is t the last retrieved block or the first unretrieved block?
Please document this and adjust here accordingly

Done and Done

---

crates/starknet_l1_provider/src/bootstrapper.rs line 137 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

can you use is_caught_up()?

Not quite, is_caught_up also has error handling for uninitialized catch-up, which is not possible here (and using it will imply that it can be uninitialized).
Also it's a method of the bootstrapper, and this function's running as a spawned task without access to self.

It's possible to refactor by extracting the comparison itself into a free function and reusing it, but it might become a bit forced 🤔

---

crates/starknet_l1_provider/src/l1_provider.rs line 215 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

What can fail here?

Nothing 😱
Tnx

---

crates/starknet_l1_provider/src/l1_provider.rs line 236 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Will this warn happen only upon override?

Yep, cause of the Option::map. If the override is None then it'll skip the block.

---

crates/starknet_sequencer_node/src/components.rs line 264 at r4 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why should this fail?
Can this be an expect? Every other fail in this function is very well documented.

Never fails, AlonH also noticed, i'm stupid 😬

[alonh5]

Reviewed 2 of 3 files at r6, 1 of 1 files at r7.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 137 at r2 (raw file):

Previously, giladchase wrote…

Not quite, is_caught_up also has error handling for uninitialized catch-up, which is not possible here (and using it will imply that it can be uninitialized).
Also it's a method of the bootstrapper, and this function's running as a spawned task without access to self.

It's possible to refactor by extracting the comparison itself into a free function and reusing it, but it might become a bit forced 🤔

Right (the second part). It's fine like this

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r6 (raw file):

    pub fn is_caught_up(&self, current_provider_height: BlockNumber) -> bool {
        self.catch_up_height()
            .is_some_and(|catch_up_height| current_provider_height >= catch_up_height)

In this case it must be strictly larger, no?

Suggestion:

current_provider_height > catch_up_height

---

crates/starknet_l1_provider/src/bootstrapper.rs line 136 at r6 (raw file):

    let catch_up_height = *catch_up_height.get().expect("Initialized above");

    while catch_up_height >= current_height {

Actually this direction was right before. It depends what the logical statement is, in this case you want to say: while I haven't reached the catch up height keep catching up. In the previous example the statement is I am caught up if my I passes the catch up height.

Suggestion:

current_height <= catch_up_height

[giladchase]

Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

In this case it must be strictly larger, no?

Most of the time ya, but you can have cases where the catch-up height is the same as the startup height. For example, when starting out a fresh node.

---

crates/starknet_l1_provider/src/bootstrapper.rs line 136 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Actually this direction was right before. It depends what the logical statement is, in this case you want to say: while I haven't reached the catch up height keep catching up. In the previous example the statement is I am caught up if my I passes the catch up height.

Done.

[alonh5]

Reviewed 1 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArniStarkware and @giladchase)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r6 (raw file):

Previously, giladchase wrote…

Most of the time ya, but you can have cases where the catch-up height is the same as the startup height. For example, when starting out a fresh node.

How come? it should be consistent

---

crates/starknet_l1_provider/src/bootstrapper.rs line 136 at r6 (raw file):

Previously, giladchase wrote…

Done.

Didn't push?

---

crates/starknet_l1_provider/tests/startup_flows.rs line 149 at r6 (raw file):

    // Make the mocked sync client try removing from a hashmap as a response to get block.
    let mut sync_client = MockStateSyncClient::default();
    sync_client.expect_get_block().returning(|_| panic!("Should not have been called!"));

This is not needed, it fails anyways if you don't set an expectation.

Code quote:

sync_client.expect_get_block().returning(|_| panic!("Should not have been called!"));

---

crates/starknet_l1_provider/tests/startup_flows.rs line 150 at r6 (raw file):

    let mut sync_client = MockStateSyncClient::default();
    sync_client.expect_get_block().returning(|_| panic!("Should not have been called!"));
    let sync_height_response = Arc::new(Mutex::new(None));

Is the Arc just so you can change it during the test? In that case you can use .times().returning(). Look at the batcher tests for reference.

---

crates/starknet_l1_provider/tests/startup_flows.rs line 178 at r6 (raw file):

    let no_txs_committed = []; // Not testing txs in this test.
    l1_provider.commit_block(&no_txs_committed, startup_height).unwrap();
    tokio::time::sleep(config.startup_sync_sleep_retry_interval).await;

Why do you need all the sleeps?

---

crates/starknet_l1_provider/tests/startup_flows.rs line 189 at r6 (raw file):

    assert!(l1_provider.state.is_bootstrapping());

    *sync_height_response.lock().unwrap() = Some(BlockNumber(2));

Is this the startup height on purpose? Can it be the height you reached?

Suggestion:

start_height_plus_2

[graphite-app]

Merge activity

• Mar 18, 10:02 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.

[giladchase]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/starknet_l1_provider/src/bootstrapper.rs line 50 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

How come? it should be consistent

Offline discussions, yr right probably, will check async.

---

crates/starknet_l1_provider/src/bootstrapper.rs line 136 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Didn't push?

Done.

---

crates/starknet_l1_provider/tests/startup_flows.rs line 149 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is not needed, it fails anyways if you don't set an expectation.

Done.

---

crates/starknet_l1_provider/tests/startup_flows.rs line 150 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Is the Arc just so you can change it during the test? In that case you can use .times().returning(). Look at the batcher tests for reference.

Talked offline, added a comment

---

crates/starknet_l1_provider/tests/startup_flows.rs line 178 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do you need all the sleeps?

Just to be safe, the sync task is spawned and running in the background every interval.

---

crates/starknet_l1_provider/tests/startup_flows.rs line 189 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Is this the startup height on purpose? Can it be the height you reached?

Yep on purpose, spoke offline

[alonh5]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)