refactor(starknet_batcher): validate flow refactor

[dafnamatsry]

No description provided.

[reviewable-StarkWare]

This change is 

[dafnamatsry]

• refactor(starknet_batcher): add tests and refactor batcher_test #2640
• refactor(starknet_batcher): delete the proposal manager #2618
• refactor(starknet_batcher): move around some types and utils to a common utils file #2622
• refactor(starknet_batcher): validate flow refactor #2441 👈 (View in Graphite)
• refactor(starknet_batcher): rename GetProposalResultError and remove the ProposalNotFound variant #2483
• refactor(starknet_batcher): add getters to the proposal manger #2480
• main

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

[codecov]

Codecov Report

Attention: Patch coverage is 72.88136% with 16 lines in your changes missing coverage. Please review.

Project coverage is 6.41%. Comparing base (e3165c4) to head (9fff59e).
Report is 833 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_batcher/src/batcher.rs	72.88%	12 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #2441       +/-   ##
==========================================
- Coverage   40.10%   6.41%   -33.69%     
==========================================
  Files          26     145      +119     
  Lines        1895   17384    +15489     
  Branches     1895   17384    +15489     
==========================================
+ Hits          760    1116      +356     
- Misses       1100   16191    +15091     
- Partials       35      77       +42     

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

[Yael-Starkware]

This is a bit overwhelming, can you split to smaller PRs?
if not, can you please give an overview of the changes introduced here?

Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @alonh5)

[dafnamatsry]

Done (see additional 2 PRs in the stack).
This is still a big PR, but it's a bit harder to split it (mostly because of how it affects the tests).
What is done here:

• Delete the get_proposal_status and await_proposal_commitment from the proposal_manager.
• Refactor send_proposal_content accordingly.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @alonh5)

[Yael-Starkware]

Reviewed 2 of 3 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

crates/starknet_batcher/src/batcher_test.rs line 70 at r4 (raw file):

fn proposal_commitment() -> ProposalCommitment {
    ProposalOutput::from(BlockExecutionArtifacts::create_for_testing()).commitment

The intention was not to give the ProposalCommitment a default value in order to verify that the batcher indeed passed the correct value from the block_builder to the consensus.

Code quote:

ProposalOutput::from(BlockExecutionArtifacts::create_for_testing()).commitment

---

crates/starknet_batcher/src/batcher_test.rs line 208 at r4 (raw file):

        &mut proposal_manager,
        PROPOSAL_ID,
        Ok(ProposalOutput::from(BlockExecutionArtifacts::create_for_testing())),

same comment for the default value,
in other tests as well

Code quote:

ProposalOutput::from(BlockExecutionArtifacts::create_for_testing())

---

crates/starknet_batcher/src/batcher_test.rs line 413 at r4 (raw file):

        &mut proposal_manager,
        PROPOSAL_ID,
        Ok(ProposalOutput::from(BlockExecutionArtifacts::create_for_testing())),

same comment for the default value

Code quote:

ProposalOutput::from(BlockExecutionArtifacts::create_for_testing()

---

crates/starknet_batcher/src/batcher.rs line 322 at r4 (raw file):

        // TODO: Consider removing the proposal from the proposal manager and keep it in the batcher
        // for decision reached.
        self.propose_tx_streams.remove(&proposal_id);

what happens if there are no new transactions in the block - so the stream is empty , but the block is not closed yet
(no timeout and no block full)?

Code quote:

self.propose_tx_streams.remove(&proposal_id);

---

crates/starknet_batcher/src/batcher.rs line 374 at r4 (raw file):

        let completed_proposals = self.proposal_manager.get_completed_proposals().await;
        let guard = completed_proposals.lock().await;
        let proposal_result = guard.get(&proposal_id).expect("Proposal should exist");

Doesn't seem right to get a field from the proposal manager and change it here in the batcher.
I assume this is just a step towards the refactor and eventually completed_proposals will be a part of the batcher. right?

Code quote:

        let completed_proposals = self.proposal_manager.get_completed_proposals().await;
        let guard = completed_proposals.lock().await;
        let proposal_result = guard.get(&proposal_id).expect("Proposal should exist");

---

crates/starknet_batcher/src/batcher.rs line 374 at r4 (raw file):

        let completed_proposals = self.proposal_manager.get_completed_proposals().await;
        let guard = completed_proposals.lock().await;
        let proposal_result = guard.get(&proposal_id).expect("Proposal should exist");

I'm not sure it's ideal to hide the panic inside the function (even though it is well-documented). It might be more straightforward and easier to understand if it were part of the calling function instead.

Code quote:

.expect("Proposal should exist");

[dafnamatsry]

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 322 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

what happens if there are no new transactions in the block - so the stream is empty , but the block is not closed yet
(no timeout and no block full)?

It wouldn't reach this point.
recv_many will block until either there are some txs in the channel or if the channel is closed (in which case the block is closed).

---

crates/starknet_batcher/src/batcher.rs line 374 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Doesn't seem right to get a field from the proposal manager and change it here in the batcher.
I assume this is just a step towards the refactor and eventually completed_proposals will be a part of the batcher. right?

Yes, the proposal manger will be deleted in a following PR.
Note that the batcher only reads the field, and doesn't change it.

---

crates/starknet_batcher/src/batcher.rs line 374 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I'm not sure it's ideal to hide the panic inside the function (even though it is well-documented). It might be more straightforward and easier to understand if it were part of the calling function instead.

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 70 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

The intention was not to give the ProposalCommitment a default value in order to verify that the batcher indeed passed the correct value from the block_builder to the consensus.

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 208 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same comment for the default value,
in other tests as well

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 413 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same comment for the default value

Done.

[Yael-Starkware]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

crates/starknet_batcher/src/batcher.rs line 322 at r4 (raw file):

Previously, dafnamatsry wrote…

It wouldn't reach this point.
recv_many will block until either there are some txs in the channel or if the channel is closed (in which case the block is closed).

right, thanks!

---

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

fn proposal_commitment() -> ProposalCommitment {
    ProposalCommitment {
        state_diff_commitment: StateDiffCommitment(PoseidonHash(felt!(u128::try_from(7).unwrap()))),

wdyt about having the BlockExecutionArtifacts::create_for_testing() return a non-zero value?
Then we won't need this messy line.

Code quote:

state_diff_commitment: StateDiffCommitment(PoseidonHash(felt!(u128::try_from(7).unwrap()))),

---

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

}

fn mock_validate_block(proposal_manager: &mut MockProposalManagerTraitWrapper) {

rename to fit both validate and propose

Code quote:

mock_validate_block

---

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

) {
    proposal_manager.expect_wrap_get_completed_proposals().times(1).return_once(move || {
        async move { Arc::new(tokio::sync::Mutex::new(HashMap::from([(proposal_id, proposal_result)]))) }.boxed()

use tokio::sync::Mutex;

Suggestion:

Mutex

---

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

        deadline: deadline(),
        retrospective_block_hash: None,
        block_info: BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() },

Suggestion:

initial_block_info()

---

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

    let invalid_proposal_result = Err(ProposalError::BlockBuilderError(Arc::new(
        BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull),
    )));

I think this can be a const, already being used by two functions.

Code quote:

    let invalid_proposal_result = Err(ProposalError::BlockBuilderError(Arc::new(
        BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull),
    )));

---

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

        .expect_wrap_spawn_proposal()
        .times(1)
        .return_once(|_, _, _| { async move { Ok(()) } }.boxed());

Suggestion:

mock_validate_block(&mut proposal_manager);

[Yael-Starkware]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

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

        block_info: initial_block_info(),
    };
    batcher.validate_block(validate_block_input).await.unwrap();

I think this code can be shared with batcher_with_validated_proposal() funciton

Code quote:

    let mut batcher = create_batcher(MockDependencies {
        proposal_manager,
        block_builder_factory,
        ..Default::default()
    });

    batcher.start_height(StartHeightInput { height: INITIAL_HEIGHT }).await.unwrap();

    let validate_block_input = ValidateBlockInput {
        proposal_id: PROPOSAL_ID,
        deadline: deadline(),
        retrospective_block_hash: None,
        block_info: initial_block_info(),
    };
    batcher.validate_block(validate_block_input).await.unwrap();

[Yael-Starkware]

Reviewable status: 1 of 4 files reviewed, 9 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

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

) {
    proposal_manager.expect_wrap_get_completed_proposals().times(1).return_once(move || {
        async move { Arc::new(tokio::sync::Mutex::new(HashMap::from([(proposal_id, proposal_result)]))) }.boxed()

Suggestion:

    proposal_result: ProposalResult<ProposalOutput>,
) {
    proposal_manager.expect_wrap_get_completed_proposals().times(1).return_once(move || {
        async move { Arc::new(tokio::sync::Mutex::new(HashMap::from([(PROPOSAL_ID, proposal_result)]))) }.boxed()

[Yael-Starkware]

Reviewed 1 of 3 files at r5.
Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

[Yael-Starkware]

Reviewable status: 2 of 4 files reviewed, 10 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

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

    // Returns a completed proposal result, either its commitment or an error if the proposal
    // failed.
    // Panics if the proposal doesn't exist.

[dafnamatsry]

Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

wdyt about having the BlockExecutionArtifacts::create_for_testing() return a non-zero value?
Then we won't need this messy line.

I reverted my change, so this is the line that was before.
But, what you suggested is exactly what I'm working on now as part of completely deleting the proposal manager.
So it will be in following PRs.

---

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

Previously, Yael-Starkware (YaelD) wrote…

rename to fit both validate and propose

Done.

---

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

Previously, Yael-Starkware (YaelD) wrote…

use tokio::sync::Mutex;

Done.

---

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

Previously, Yael-Starkware (YaelD) wrote…

I think this code can be shared with batcher_with_validated_proposal() funciton

Could be. I prefer to do this kind of refactoring as a last step.
Note that all of the proposal manger mock code here will be deleted.

---

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

Previously, Yael-Starkware (YaelD) wrote…

I think this can be a const, already being used by two functions.

A bit problematic because of the Arc.
I can put only the BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull) behind a const but IMO it won't make the test more readable.

---

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

) {
    proposal_manager.expect_wrap_get_completed_proposals().times(1).return_once(move || {
        async move { Arc::new(tokio::sync::Mutex::new(HashMap::from([(proposal_id, proposal_result)]))) }.boxed()

Done.

---

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

        deadline: deadline(),
        retrospective_block_hash: None,
        block_info: BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() },

Done.

---

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

        .expect_wrap_spawn_proposal()
        .times(1)
        .return_once(|_, _, _| { async move { Ok(()) } }.boxed());

Done.

[Yael-Starkware]

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

---

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

Previously, dafnamatsry wrote…

A bit problematic because of the Arc.
I can put only the BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull) behind a const but IMO it won't make the test more readable.

maybe lazy_static?
or a separate function?

[dafnamatsry]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

maybe lazy_static?
or a separate function?

Done.

[Yael-Starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dafnamatsry)

---

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

    // Returns a completed proposal result, either its commitment or an error if the proposal
    // failed.
    // Panics if the proposal doesn't exist.

in case this wasn't clear, I was asking to remove the comment about the panic.

[dafnamatsry]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

in case this wasn't clear, I was asking to remove the comment about the panic.

Oops. Missed it.
Done.

[Yael-Starkware]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

[alonh5]

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

---

crates/starknet_batcher/src/batcher_test.rs line 202 at r8 (raw file):

        block_info: initial_block_info(),
    };
    batcher.validate_block(validate_block_input).await.unwrap();

(After rebase)

Suggestion:

    batcher.validate_block(validate_block_input()).await.unwrap();

---

crates/starknet_batcher/src/batcher.rs line 238 at r8 (raw file):

                self.proposal_manager.abort_proposal(proposal_id).await;
                self.close_input_transaction_stream(proposal_id)?;
                Ok(SendProposalContentResponse { response: ProposalStatus::Aborted })

Extract to function? For symmetry

Code quote:

                self.proposal_manager.abort_proposal(proposal_id).await;
                self.close_input_transaction_stream(proposal_id)?;
                Ok(SendProposalContentResponse { response: ProposalStatus::Aborted })

[alonh5]

Reviewed 1 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry)

[dafnamatsry]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 238 at r8 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Extract to function? For symmetry

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 202 at r8 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

(After rebase)

Done.

[alonh5]

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