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

feat(sequencing): cache proposals from bigger heights #2325

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

guy-starkware
Copy link
Contributor

If a proposal is received by Consensus that has height bigger than the current height, cache it into a hash map that saves it for later.

When starting a new height, will first look to see if any cached proposals exist before listening to messages from the network.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented Nov 28, 2024

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 18.16%. Comparing base (e3165c4) to head (14c5f8e).
Report is 701 commits behind head on main.

Files with missing lines Patch % Lines
crates/sequencing/papyrus_consensus/src/manager.rs 80.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2325       +/-   ##
===========================================
- Coverage   40.10%   18.16%   -21.94%     
===========================================
  Files          26      119       +93     
  Lines        1895    14129    +12234     
  Branches     1895    14129    +12234     
===========================================
+ Hits          760     2567     +1807     
- Misses       1100    11291    +10191     
- Partials       35      271      +236     

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

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 17 files at r1, 1 of 11 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 19 files reviewed, 3 unresolved discussions (waiting on @guy-starkware)


crates/sequencing/papyrus_consensus/src/manager.rs line 155 at r4 (raw file):

                    shc_events.push(task.run());
                }
            }

If you don't think a decision can happen let's explicitly panic or just log. I don't want to risk missing it.

Suggestion:

            match shc_return {
                ShcReturn::Decision(decision) => return Ok(decision),
                ShcReturn::Tasks(tasks) => {
                    for task in tasks {
                        shc_events.push(task.run());
                    }
                }
            }

crates/sequencing/papyrus_consensus/src/manager.rs line 163 at r4 (raw file):

                message = next_message(&mut current_height_messages, broadcast_channels) => {
                    self.handle_message(context, height, &mut shc, message?).await?
                },

Please add a TODO to remove the continue_propagation/report_peer from this. They make this function non cancel safe, since it's possible for broadcasted_messages_receiver to return a message, but when we call to one of those functions a different branch completes causing us to drop it without returning.

This is very important so please make sure I handle this as soon as your code is submitted

Code quote:

                message = next_message(&mut current_height_messages, broadcast_channels) => {
                    self.handle_message(context, height, &mut shc, message?).await?
                },

crates/sequencing/papyrus_consensus/src/manager_test.rs line 84 at r4 (raw file):

}

#[ignore] // TODO(guyn): return this once caching proposals is implemented.

remove? (and below)

Code quote:

#[ignore] // TODO(guyn): return this once caching proposals is implemented.

@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from 31603d7 to d5190a6 Compare December 1, 2024 14:11
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 19 files reviewed, 3 unresolved discussions (waiting on @matan-starkware)


crates/sequencing/papyrus_consensus/src/manager.rs line 155 at r4 (raw file):

Previously, matan-starkware wrote…

If you don't think a decision can happen let's explicitly panic or just log. I don't want to risk missing it.

Done.


crates/sequencing/papyrus_consensus/src/manager.rs line 163 at r4 (raw file):

Previously, matan-starkware wrote…

Please add a TODO to remove the continue_propagation/report_peer from this. They make this function non cancel safe, since it's possible for broadcasted_messages_receiver to return a message, but when we call to one of those functions a different branch completes causing us to drop it without returning.

This is very important so please make sure I handle this as soon as your code is submitted

Done.


crates/sequencing/papyrus_consensus/src/manager_test.rs line 84 at r4 (raw file):

Previously, matan-starkware wrote…

remove? (and below)

Yes!

@guy-starkware guy-starkware changed the title feat(consensus): cache proposals from bigger heights feat(sequencing): cache proposals from bigger heights Dec 2, 2024
@matan-starkware matan-starkware self-requested a review December 3, 2024 07:45
@guy-starkware guy-starkware changed the base branch from main to guyn/streams/validate_streams December 3, 2024 10:54
@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from d5190a6 to bc1c858 Compare December 3, 2024 12:34
@guy-starkware guy-starkware changed the base branch from guyn/streams/validate_streams to main December 3, 2024 12:34
@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from bc1c858 to 74003f4 Compare December 3, 2024 13:10
Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r6, 3 of 19 files at r7, 1 of 1 files at r8, all commit messages.
Dismissed @guy-starkware from 3 discussions.
Reviewable status: 3 of 19 files reviewed, 2 unresolved discussions (waiting on @guy-starkware)


crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

    #[async_trait]
    impl ConsensusContext for TestContext {

Now that we have removed ProposalWrapper I think we should be able to remove this whole mock scope and use the one from test_util. Can you please look into that in another PR?

Code quote:

    #[async_trait]
    impl ConsensusContext for TestContext {

crates/sequencing/papyrus_consensus/src/manager_test.rs line 151 at r7 (raw file):

                .unwrap();
            block_receiver
        })

I feel like this pattern repeats a lot. WDYT of adding this helper fn?

This can be done in another PR, and also check other tests in this crate (if so put the fn def in test_util.rs)

I actually see that we don't have any examples here with us being the builder.

Suggestion:

fn validate_result(
    built_id: Felt,
    received_id: Felt,
) -> oneshot::Receiver<(ProposalContentId, ProposalFin)> {
    let (block_sender, block_receiver) = oneshot::channel();
    block_sender
        .send((
            ProposalContentId(built_id),
            ProposalFin { proposal_content_id: BlockHash(received_id) },
        ))
        .unwrap();
    block_receiver
}
       
        .return_once(move |_, _, _, _| {
            validate_result(Felt::TWO, Felt::TWO)
        })

@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from 74003f4 to d53c1c5 Compare December 4, 2024 12:28
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 19 files reviewed, 2 unresolved discussions (waiting on @matan-starkware)


crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, matan-starkware wrote…

Now that we have removed ProposalWrapper I think we should be able to remove this whole mock scope and use the one from test_util. Can you please look into that in another PR?

We can remove it because we can put the real SequencerConsensusContext instead?


crates/sequencing/papyrus_consensus/src/manager_test.rs line 151 at r7 (raw file):

Previously, matan-starkware wrote…

I feel like this pattern repeats a lot. WDYT of adding this helper fn?

This can be done in another PR, and also check other tests in this crate (if so put the fn def in test_util.rs)

I actually see that we don't have any examples here with us being the builder.

Done.

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 19 files at r7, 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)


crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, guy-starkware wrote…

We can remove it because we can put the real SequencerConsensusContext instead?

No, a given implementation of the Context trait cannot have any impact here since we are generic. I believe that we can redefine the test_util struct to have ProposalPart = TestProposalPart with:

enum TestProposalPart { ProposalInit, ProposalFin }.

This is another PR though so we can discuss when you get to it.


crates/sequencing/papyrus_consensus/src/manager_test.rs line 105 at r9 (raw file):

}

fn context_validates_hashes(context: &mut MockTestContext, block_hash: Felt) {

Suggestion:

fn expect_validate_proposal(context: &mut MockTestContext, block_hash: Felt) {

@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from d53c1c5 to 4237007 Compare December 4, 2024 21:13
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matan-starkware)


crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, matan-starkware wrote…

No, a given implementation of the Context trait cannot have any impact here since we are generic. I believe that we can redefine the test_util struct to have ProposalPart = TestProposalPart with:

enum TestProposalPart { ProposalInit, ProposalFin }.

This is another PR though so we can discuss when you get to it.

Ok, I'm putting it on my list.


crates/sequencing/papyrus_consensus/src/manager_test.rs line 105 at r9 (raw file):

}

fn context_validates_hashes(context: &mut MockTestContext, block_hash: Felt) {

Done.

@matan-starkware matan-starkware self-requested a review December 5, 2024 07:36
Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from 4237007 to ef40e92 Compare December 5, 2024 08:25
@matan-starkware matan-starkware self-requested a review December 5, 2024 08:28
@guy-starkware guy-starkware force-pushed the guyn/streams/cache_proposals branch from ef40e92 to 14c5f8e Compare December 5, 2024 09:01
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 19 files at r7, 12 of 13 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

@guy-starkware guy-starkware merged commit cbf967d into main Dec 5, 2024
19 checks passed
FrancoGiachetta added a commit to lambdaclass/sequencer that referenced this pull request Dec 6, 2024
* chore(starknet_batcher): delete obsolete todo (starkware-libs#2389)

* chore(blockifier): add global max_sierra_gas to versioned constants (starkware-libs#2330)

* feat(starknet_api): checked mul for gas vector (starkware-libs#2300)

* feat(consensus): proposer rotates across validators (starkware-libs#2405)

* feat(sequencing): validate streamed proposals (starkware-libs#2305)

* feat(ci): deny rust warnings in all workflows (starkware-libs#2388)

Signed-off-by: Dori Medini <[email protected]>

* feat(blockifier): compute allocation cost (starkware-libs#2301)

* chore(starknet_sequencer_infra): add dynamic logging util fn

commit-id:9ffe9fbe

* chore(starknet_sequencer_infra): add tracing test

commit-id:76d16e9a

* chore(starknet_sequencer_infra): add run_until utility fn

commit-id:194a4b6c

* chore(infra_utils): change run_until to support async functions

commit-id:92e1f8a3

* chore(starknet_integration_tests): use run_until to await for block creation

commit-id:667e001c

* chore(infra_utils): rename logger struct

commit-id:6520ae54

* chore(blockifier): explicit creation of AccountTransaction (starkware-libs#2331)

* test(starknet_integration_tests): test commit blocks by running multiple heights

* chore(starknet_batcher): set temp gas prices in propose block input (starkware-libs#2341)

* chore(starknet_batcher): set use_kzg_da flag in build block input (starkware-libs#2345)

* feat(ci): inherit the rust toolchain toml in moonrepo action (starkware-libs#2423)

Signed-off-by: Dori Medini <[email protected]>

* chore(blockifier): enforce_fee() impl by api::executable_transaction::AccountTransaction (starkware-libs#2377)

* chore(starknet_integration_tests): inherit sequencer node's stdout (starkware-libs#2427)

* chore(blockifier): invoke() declare() deploy_account() change ret val to api_tx (starkware-libs#2412)

* chore(starknet_consensus_manager): set proposer address in propose block input (starkware-libs#2346)

* feat(consensus): add observer mode

* feat(consensus): sequencer context broadcasts votes (starkware-libs#2422)

* chore(deployment): support unified deployment config (starkware-libs#2378)

* feat(starknet_api): add sierra version to class info (starkware-libs#2313)

* refactor(starknet_api): change default sierra contract class to valid one (starkware-libs#2439)

* feat(starknet_l1_provider): add uninitiailized state and make it the default (starkware-libs#2434)

This is to comply with upcoming integration with infra, which separates
instantiation with initialization. In particular, `Pending` state should
be already post-syncing with L1, whereas `Uninitialized` is unsynced and
unusable.

Co-Authored-By: Gilad Chase <[email protected]>

* feat(papyrus_storage)!: bump storage version for version 13.4 (starkware-libs#2333)

* feat(native_blockifier): allow deserialization of  python l1_data_gas (starkware-libs#2447)

* refactor(blockifier): split FC to groups base on their tags (starkware-libs#2236)

* test(consensus): remove warning on into mock propsal part (starkware-libs#2448)

* chore(blockifier): use test_utils::invoke_tx() instead of trans::test_utils::account_invoke_tx() (starkware-libs#2428)

* chore(blockifier): save sierra to Feature contracts (starkware-libs#2370)

* feat(blockifier): don't count Sierra gas in CairoSteps mode (starkware-libs#2440)

* chore(blockifier): convert Sierra gas to L1 gas if in L1 gas mode (starkware-libs#2451)

* feat(blockifier): add comprehensive state diff versioned constant (starkware-libs#2407)

* chore(starknet_consensus_manager): add chain_id to config

* refactor(papyrus_p2p_sync): add random_header utility function (starkware-libs#2381)

* chore(starknet_batcher): pass block info from consensus to batcher (starkware-libs#2238)

* test(starknet_mempool): tx added to mempool are forwarded to propagator client (starkware-libs#2288)

* fix: fix CR comments

* test(starknet_mempool): tx added to mempool are forwarded to propagator client

* feat(sequencing): cache proposals from bigger heights(starkware-libs#2325)

* fix: change to latest ubuntu version in feature combo CI (starkware-libs#2414)

* chore(blockifier): replace entry_point_gas_cost with initial_budget (starkware-libs#2247)

* test(starknet_gateway): handle todo in test_get_block_info (starkware-libs#2267)

* chore(starknet_api): revert use get_packaget_dir instead of env var

This reverts commit c45f5cc.

commit-id:a48736e7

* chore(starknet_api): rely on env::current_dir() instead of CARGO_MANIFEST_DIR

commit-id:301ed4eb

* chore(blockifier): move env var from run time to compile time

commit-id:80a7265d

* chore(starknet_sierra_compile): move env var from run time to compile time

commit-id:6e7f2a75

* chore: remove the use of zero as a validator id (starkware-libs#2411)

* refactor(papyrus_p2p_sync): add_test receives query size instead of constant (starkware-libs#2379)

* fix(blockifier): merge state diff with squash (starkware-libs#2310)

* feat(blockifier): get revert receipt only in case of revert (starkware-libs#2471)

* chore(starknet_integration_tests): create chain info once (starkware-libs#2482)

* chore(starknet_sierra_compile): split build utils

commit-id:0f504fd7

* chore(starknet_sierra_compile): set runtime-accessible out_dir env var

commit-id:539f16db

* chore(starknet_sierra_compile): avoid using OUT_DIR in run time

commit-id:cd6fba29

* refactor(starknet_api): use const in sierra version (starkware-libs#2477)

* chore: cleanups of OUT_DIR env var (starkware-libs#2484)

commit-id:18d61b1d

* chore(starknet_api): shorten executable_transaction  usage path

* fix(sequencing): remove old proposal pipes from consensus (starkware-libs#2452)

* test(starknet_integration_tests): match sequencer address with default validator id (starkware-libs#2486)

* fix(ci): move specific versioned deps to root toml (starkware-libs#2487)

* fix(ci): move specific versioned deps to root toml

Signed-off-by: Dori Medini <[email protected]>

* fix(starknet_sierra_compile): fix build.rs

Signed-off-by: Dori Medini <[email protected]>

* chore(starknet_batcher): in block builder use the consensus suplied sequncer address (starkware-libs#2409)

* chore: cleanups of CARGO_MANIFEST_DIR env var

commit-id:c96f2d88

* fix(starknet_sierra_compile): missing import in feature (starkware-libs#2495)

commit-id:abd0a286

* chore(blockifier): add keccak_builtin_gas_cost (starkware-libs#2327)

* chore(blockifier): add struct ExecutionFlags to AccTx & use instead of transaction::ExecutionFlags (starkware-libs#2429)

* chore(blockifier): add new constructor to AccountTransaction (starkware-libs#2431)

* chore(blockifier): remove only_qury from IvokeTxArgs (starkware-libs#2437)

* chore(blockifier): remove struct ExecutionFlags and replace w concurrency_mode bool (starkware-libs#2470)

* chore(blockifier): remove declare.rs deploy_account.rs invoke.rs from blockifier (starkware-libs#2478)

---------

Signed-off-by: Dori Medini <[email protected]>
Co-authored-by: YaelD <[email protected]>
Co-authored-by: aner-starkware <[email protected]>
Co-authored-by: yoavGrs <[email protected]>
Co-authored-by: matan-starkware <[email protected]>
Co-authored-by: guy-starkware <[email protected]>
Co-authored-by: dorimedini-starkware <[email protected]>
Co-authored-by: Itay Tsabary <[email protected]>
Co-authored-by: avivg-starkware <[email protected]>
Co-authored-by: Yair Bakalchuk <[email protected]>
Co-authored-by: Arnon Hod <[email protected]>
Co-authored-by: Alon Haramati <[email protected]>
Co-authored-by: Asmaa Magdoub <[email protected]>
Co-authored-by: alon-dotan-starkware <[email protected]>
Co-authored-by: AvivYossef-starkware <[email protected]>
Co-authored-by: giladchase <[email protected]>
Co-authored-by: Gilad Chase <[email protected]>
Co-authored-by: DvirYo-starkware <[email protected]>
Co-authored-by: nimrod-starkware <[email protected]>
Co-authored-by: Meshi Peled <[email protected]>
Co-authored-by: Yoni <[email protected]>
Co-authored-by: Yael Doweck <[email protected]>
Co-authored-by: ShahakShama <[email protected]>
Co-authored-by: Alon-Lukatch-Starkware <[email protected]>
Co-authored-by: Yonatan-Starkware <[email protected]>
Co-authored-by: Yair <[email protected]>
Co-authored-by: Itay-Tsabary-Starkware <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants