feat(starknet_batcher): implement sync_block

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• feat(starknet_batcher): implement sync_block #2604 👈 (View in Graphite)
• refactor(starknet_batcher): create method clear previous proposals for code dedup #2670
• refactor(starknet_batcher): create a function to get_height from storage for code dedup #2665
• feat(starknet_batcher): communication for sync block request #2603
• main

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

[codecov]

Codecov Report

Attention: Patch coverage is 41.17647% with 10 lines in your changes missing coverage. Please review.

Please upload report for BASE (arni/sync_block/batcher_side_communications@390db80). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/starknet_batcher/src/batcher.rs	41.17%	10 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                              @@
##             arni/sync_block/batcher_side_communications   #2604   +/-   ##
=============================================================================
  Coverage                                               ?   6.39%           
=============================================================================
  Files                                                  ?     141           
  Lines                                                  ?   17233           
  Branches                                               ?   17233           
=============================================================================
  Hits                                                   ?    1102           
  Misses                                                 ?   16055           
  Partials                                               ?      76           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alonh5]

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

---

crates/starknet_batcher/src/batcher.rs line 353 at r1 (raw file):

    }

    pub async fn add_sync_block(&mut self, sync_block: SyncBlock) -> BatcherResult<()> {

We should also abort the active height if there is one, right?

---

crates/starknet_batcher/src/batcher.rs line 353 at r1 (raw file):

    }

    pub async fn add_sync_block(&mut self, sync_block: SyncBlock) -> BatcherResult<()> {

Test?

---

crates/starknet_batcher/src/batcher.rs line 377 at r1 (raw file):

            proposal_output;
        // TODO(Arni): Move the height and the `info` level log into the function
        // `commit_proposal_and_block`.

Why not do it here?

Code quote:

        // TODO(Arni): Move the height and the `info` level log into the function
        // `commit_proposal_and_block`.

[ArniStarkware]

crates/starknet_batcher/src/batcher.rs line 357 at r2 (raw file):

        let address_to_nonce = state_diff.nonces.iter().map(|(k, v)| (*k, *v)).collect();
        let tx_hashes = transaction_hashes.into_iter().collect();
        // TODO: Keep the height from start_height or get it from the input.

Is this the correct height to be synced to?
Isn't it supposed to be some other height?
The height is read from storage. Is it the intention that the given state_diff be committed to the same height?

Code quote:

        // TODO: Keep the height from start_height or get it from the input.

[ArniStarkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

crates/starknet_batcher/src/batcher.rs line 353 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

We should also abort the active height if there is one, right?

Done.

---

crates/starknet_batcher/src/batcher.rs line 353 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Test?

Done.

---

crates/starknet_batcher/src/batcher.rs line 377 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why not do it here?

Because it is incorrect!
We talked on DMs. The height for decision_reached should be the current height.
The height for add_sync_block should be current height + 1.

[ArniStarkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

crates/starknet_batcher/src/batcher.rs line 377 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Because it is incorrect!
We talked on DMs. The height for decision_reached should be the current height.
The height for add_sync_block should be current height + 1.

1. Reverted the changes to the height marker.
2. Followed through with the original TODO.

[alonh5]

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_batcher/src/batcher.rs line 369 at r5 (raw file):

        let info_callback = |height: BlockNumber| {
            info!("Syncing block at height {} and notifying mempool of the block.", height);
        };

This callback is cool, but can't we just have the same message for both?
Also add #[instrument(skip(self), err)] at the top and that way we'll be able to distinct the two.

Code quote:

        let info_callback = |height: BlockNumber| {
            info!("Syncing block at height {} and notifying mempool of the block.", height);
        };

---

crates/starknet_batcher/src/batcher_test.rs line 499 at r5 (raw file):

            tx_hashes: test_tx_hashes(),
        }))
        .returning(|_| Ok(()));

add times(1) so we know they're called.

Code quote:

    mock_dependencies
        .storage_writer
        .expect_commit_proposal()
        .with(eq(INITIAL_HEIGHT), eq(test_state_diff()))
        .returning(|_, _| Ok(()));

    mock_dependencies
        .mempool_client
        .expect_commit_block()
        .with(eq(CommitBlockArgs {
            address_to_nonce: test_contract_nonces(),
            tx_hashes: test_tx_hashes(),
        }))
        .returning(|_| Ok(()));

---

crates/starknet_batcher/src/batcher_test.rs line 510 at r5 (raw file):

    let get_height_response = batcher.get_height().await.unwrap();
    assert_eq!(get_height_response.height, INITIAL_HEIGHT);

This should be the next height after syncing one block, no?
But the storage reader is mocked anyway, so there's no need for this check.

Suggestion:

INITIAL_HEIGHT.next()

[ArniStarkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

crates/starknet_batcher/src/batcher.rs line 369 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This callback is cool, but can't we just have the same message for both?
Also add #[instrument(skip(self), err)] at the top and that way we'll be able to distinct the two.

Done. The log changed slightly. For decision_reached, the info will no longer contain the proposal_id - we count on the fact that the proposal_id appears on the call to decision_reached.

---

crates/starknet_batcher/src/batcher_test.rs line 499 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

add times(1) so we know they're called.

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 510 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This should be the next height after syncing one block, no?
But the storage reader is mocked anyway, so there's no need for this check.

Removed.

[alonh5]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ArniStarkware]

crates/starknet_batcher/src/batcher.rs line 369 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done. The log changed slightly. For decision_reached, the info will no longer contain the proposal_id - we count on the fact that the proposal_id appears on the call to decision_reached.

2024-12-16T13:04:58.220108Z  INFO decision_reached: starknet_batcher::batcher: Committing block at height 3 and notifying mempool of the block. input=DecisionReachedInput { proposal_id: ProposalId(0) }