[optqs] bug fixes and perf improvements #15452

ibalajiarun · 2024-12-02T23:19:26Z

Description

This PR includes the following fixes

[optqs] support fetching batches from QC signers
In optQS, batches can only be fetched from consensus and batch proposer until the block gets a QC. after QC, batches should be fetched from QC signers as well to guarantee progress.
[optqs] ability to update responders on inflight fetch
Builds on top of previous commit to update responders in flight without waiting for previous fetch to complete.
[optqs] Ignore stale proposals due to fetch lag
If a validators fetches in critical path of proposal and then votes, it is possible that it might already have a SyncInfo for that round because other validators moved on. In such a case, the validator should ignore the proposal instead of voting. This issue came up in logs because some validators couldn't verify the Vote message because the vote round and sync info QC round were same.
Enables OptQS on forge

trunk-io · 2024-12-02T23:19:30Z

⏱️ 9h 40m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
test-target-determinator	1h 29m	🟩 🟩 🟩 🟩 🟩 (+16 more)
execution-performance / single-node-performance	1h 8m	🟩 🟩 🟩
forge-realistic-env-graceful-overload / forge	56m	🟥 🟥
rust-cargo-deny	38m	🟩 🟩 🟩 🟩 🟩 (+17 more)
check-dynamic-deps	24m	🟩 🟩 🟩 🟩 🟩 (+18 more)
rust-move-tests	14m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	⬜
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

bchocho · 2024-12-04T18:17:56Z

consensus/src/quorum_store/batch_store.rs

+                Self::gc_previous_epoch_batches_from_db(db_clone, epoch);
+            });
+        } else {
+            Self::gc_expired_batches_from_db(


Should all gc be spawn_blocking? An epoch nearing completion could also have a lot of batches to gc? I see this call eventually calls spawn_blocking on delete_batches so might as well make things consistent and guard against the get_all_batches being expensive?

may be the name is misleading. this call also populates the batch store in memory cache and until we populate this cache we cannot return. so this cannot be spawned async.

yeah, it seems more consistent to both spawn_blocking, this branch is for restart right?

Ah makes sense. Maybe some renaming/moving code around will make this more clear :) Not a blocker, more a nit

oh I didn't see the response lol

zekun000

we should split the optqs changes out from the pr, it's too big probably shouldn't rush into release

zekun000 · 2024-12-04T18:26:07Z

consensus/consensus-types/src/pipelined_block.rs

@@ -286,6 +288,10 @@ impl PipelinedBlock {
            .take()
            .expect("pre_commit_result_rx missing.")
    }
+
+    pub fn set_qc(&self, qc: Option<Arc<QuorumCert>>) {


why this takes an Option?

Removed the option and made an assertion when calling set_qc

zekun000 · 2024-12-04T18:26:45Z

consensus/consensus-types/src/proposal_msg.rs

-        self.proposal.epoch()
+    /// Returns the epoch associated with the proposal after verifying that the
+    /// payload data is also associated with the same epoch
+    pub fn verified_epoch(&self) -> Result<u64> {


it seems better to verify the payload epoch in verify_well_formed function and keep here to just return u64?

this is how we did for BatchMsg seems like, copied that. moved it to verify_well_formed.

zekun000 · 2024-12-04T18:28:20Z

consensus/src/block_storage/block_store.rs

+        for block in &blocks_to_commit {
+            // Given the block is ready to commit, then it certainly must have a QC.
+            // However, we keep it an Option to be safe.
+            block.set_qc(self.get_quorum_cert_for_block(block.id()));


instead of this, we can just do a reverse iteration since every block carries the parent's qc, we still need to get the qc for the last block though

do you think the call is expensive? the change looks more verbose... I also want to Arc the QC so I can avoid the cloning the QC.

blocks_to_commit .last() .expect("at least one block is required") .set_qc( self .get_quorum_cert_for_block(block.id()) .expect("QC must be present"), ); for [parent, child] in blocks_to_commit.windows(2).rev() { parent.set_qc(Arc::new(child.block().quorum_cert().clone())); }

nah I just didn't like the Option there

Okay, i will just expect a QC then and still use the function call.

zekun000 · 2024-12-04T18:36:51Z

consensus/src/payload_manager.rs

+
+    let result = {
+        // If the future is completed then use the result.
+        if let Some(result) = fut.clone().now_or_never() {


is it worth having this early check? just appending the responders doesn't seem too bad?

yeah, makes sense. we compute it anyhow. I will remove it.

zekun000 · 2024-12-04T18:38:43Z

consensus/src/payload_manager.rs

@@ -604,11 +643,13 @@ async fn process_payload_helper<T: TDataInfo>(
                .batch_summary
                .iter()
                .map(|proof| {


this is not a proof but a data ptr right?

zekun000 · 2024-12-04T18:39:09Z

consensus/src/payload_manager.rs

@@ -604,11 +643,13 @@ async fn process_payload_helper<T: TDataInfo>(
                .batch_summary
                .iter()
                .map(|proof| {
-                    let mut signers = proof.signers(ordered_authors);
+                    let mut signers = signers.clone();
+                    signers.append(&mut proof.signers(ordered_authors));


we add the qc signers to proof responders too?

it's only passed for opt batches not proofs. proof is a data ptr. will rename.

zekun000 · 2024-12-04T18:41:10Z

consensus/src/quorum_store/batch_store.rs

+                Self::gc_previous_epoch_batches_from_db(db_clone, epoch);
+            });
+        } else {
+            Self::gc_expired_batches_from_db(


yeah, it seems more consistent to both spawn_blocking, this branch is for restart right?

github-actions · 2024-12-05T01:20:50Z

✅ Forge suite `realistic_env_max_load` success on `1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a`

two traffics test: inner traffic : committed: 14890.32 txn/s, latency: 2664.43 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5661660
two traffics test : committed: 99.94 txn/s, latency: 1318.32 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 1600 ms), latency samples: 1760
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.501, avg: 1.444", "ConsensusProposalToOrdered: max: 0.330, avg: 0.291", "ConsensusOrderedToCommit: max: 0.378, avg: 0.369", "ConsensusProposalToCommit: max: 0.667, avg: 0.660"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.80s no progress at version 25178 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.74s no progress at version 2777402 (avg 0.74s) [limit 16].
Test Ok

github-actions · 2024-12-05T01:21:55Z

✅ Forge suite `compat` success on `3527aa2e299553b759c515d9843586bad48c802c` ==> `1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a`

Compatibility test results for 3527aa2e299553b759c515d9843586bad48c802c ==> 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a (PR)
1. Check liveness of validators at old version: 3527aa2e299553b759c515d9843586bad48c802c
compatibility::simple-validator-upgrade::liveness-check : committed: 13828.85 txn/s, latency: 2443.98 ms, (p50: 1800 ms, p70: 2000, p90: 4500 ms, p99: 12700 ms), latency samples: 451020
2. Upgrading first Validator to new version: 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6437.33 txn/s, latency: 4473.62 ms, (p50: 5100 ms, p70: 5400, p90: 5600 ms, p99: 5700 ms), latency samples: 116740
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6299.62 txn/s, latency: 5255.41 ms, (p50: 5500 ms, p70: 5600, p90: 6000 ms, p99: 7300 ms), latency samples: 209760
3. Upgrading rest of first batch to new version: 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7106.64 txn/s, latency: 3977.71 ms, (p50: 4400 ms, p70: 4500, p90: 4800 ms, p99: 5100 ms), latency samples: 143620
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7268.22 txn/s, latency: 4495.55 ms, (p50: 4700 ms, p70: 4800, p90: 5800 ms, p99: 6400 ms), latency samples: 240720
4. upgrading second batch to new version: 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11594.44 txn/s, latency: 2407.06 ms, (p50: 2600 ms, p70: 2800, p90: 3100 ms, p99: 3400 ms), latency samples: 200120
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12001.63 txn/s, latency: 2667.59 ms, (p50: 2800 ms, p70: 2900, p90: 3100 ms, p99: 3300 ms), latency samples: 387740
5. check swarm health
Compatibility test for 3527aa2e299553b759c515d9843586bad48c802c ==> 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a passed
Test Ok

zekun000 · 2024-12-06T21:57:43Z

consensus/consensus-types/src/common.rs

+                }
+            },
+            Payload::QuorumStoreInlineHybrid(inline_batches, proof_with_data, _) => {
+                if proof_with_data.proofs.iter().any(|p| p.epoch() != epoch) {


nit: I think it's slightly better to have this?

ensure!(iter.all(..), "...");

zekun000 · 2024-12-06T22:22:52Z

consensus/consensus-types/src/proposal_msg.rs

@@ -77,6 +77,9 @@ impl ProposalMsg {
            "Proposal {} does not define an author",
            self.proposal
        );
+        self.proposal
+            .payload()


it feels it should go inside the proposal.verify_well_formed?

zekun000 · 2024-12-06T22:25:41Z

consensus/src/payload_manager.rs

@@ -56,6 +56,7 @@ pub trait TPayloadManager: Send + Sync {
    async fn get_transactions(
        &self,
        block: &Block,
+        block_signers: Option<BitVec>,


hmm, this is not vector of signers because qc only has the bitvec?

zekun000 · 2024-12-06T22:38:14Z

consensus/src/payload_manager.rs

@@ -453,13 +461,15 @@ impl TPayloadManager for QuorumStorePayloadManager {
                    self.batch_reader.clone(),
                    block,
                    &self.ordered_authors,
+                    block_signers.as_ref(),
                )
                .await?;
                let proof_batch_txns = process_payload_helper(


we should rename "process_payload_helper` and "process_payload", they're confusing lol. maybe "process_optqs_payload" and "process_qs_payload" etc

zekun000 · 2024-12-07T02:32:31Z

consensus/src/pipeline/pipeline_builder.rs

        let _tracker = Tracker::new("prepare", &block);
        // the loop can only be abort by the caller
        let input_txns = loop {
-            match preparer.prepare_block(&block).await {
+            match preparer.prepare_block(&block, qc.clone()).await {


this probably does't work, since the future is built when we receive the block, we need to pass the Arc of Mutex here and check if qc is set on every loop run

zekun000 · 2024-12-07T02:33:25Z

consensus/src/round_manager.rs

@@ -1135,6 +1135,20 @@ impl RoundManager {

    pub async fn process_verified_proposal(&mut self, proposal: Block) -> anyhow::Result<()> {
        let proposal_round = proposal.round();
+        let sync_info = self.block_store.sync_info();
+
+        if proposal_round <= sync_info.highest_round() {


I thought inserting existing block is no-op (and will not create a vote)

ibalajiarun force-pushed the balaji/optqs-batch branch 2 times, most recently from 4f486f1 to b55b5f0 Compare December 2, 2024 23:22

ibalajiarun added the CICD:run-forge-e2e-perf Run the e2e perf forge only label Dec 2, 2024

This comment has been minimized.

Sign in to view

ibalajiarun changed the title ~~Balaji/optqs batch~~ [optqs] support fetching batches from QC signers Dec 2, 2024

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/optqs-batch branch from b55b5f0 to 2b3e3ac Compare December 3, 2024 00:14

This comment has been minimized.

Sign in to view

ibalajiarun marked this pull request as ready for review December 3, 2024 01:45

ibalajiarun requested review from zekun000, sasha8, JoshLind and gregnazario as code owners December 3, 2024 01:45

This comment has been minimized.

Sign in to view

ibalajiarun requested review from bchocho and gelash as code owners December 3, 2024 05:34

ibalajiarun force-pushed the balaji/optqs-batch branch from bc884d4 to af611f0 Compare December 3, 2024 05:37

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/optqs-batch branch from 9db3836 to 8ebd336 Compare December 3, 2024 18:22

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/optqs-batch branch 2 times, most recently from 3dab484 to be1a1b2 Compare December 3, 2024 19:12

This comment has been minimized.

Sign in to view

bchocho reviewed Dec 4, 2024

View reviewed changes

ibalajiarun added CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR and removed CICD:run-forge-e2e-perf Run the e2e perf forge only labels Dec 4, 2024

This comment has been minimized.

Sign in to view

zekun000 reviewed Dec 4, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/optqs-batch branch from 2c8c283 to a86ca72 Compare December 5, 2024 00:06

ibalajiarun changed the title ~~[optqs][qs] bug fixes and perf improvements~~ [optqs] bug fixes and perf improvements Dec 5, 2024

ibalajiarun added 6 commits December 4, 2024 16:10

[optqs] support fetching batches from QC signers

fa94d7c

[optqs] ability to update responders on inflight fetch

a631c7c

[optqs] Ignore stale proposals due to fetch lag

5e982a9

[consensus] Refuse proposals with payloads from different epochs

eb43c48

unused lint fixes

be9243c

enable optqs on forge

3739e25

ibalajiarun force-pushed the balaji/optqs-batch branch from a86ca72 to 3739e25 Compare December 5, 2024 00:11

This comment has been minimized.

Sign in to view

address feedback

1f2e658

ibalajiarun requested a review from zekun000 December 5, 2024 00:56

This comment has been minimized.

Sign in to view

zekun000 reviewed Dec 7, 2024

View reviewed changes

[optqs] bug fixes and perf improvements #15452

Are you sure you want to change the base?

[optqs] bug fixes and perf improvements #15452

Conversation

ibalajiarun commented Dec 2, 2024 • edited Loading

Description

trunk-io bot commented Dec 2, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

zekun000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 5, 2024

✅ Forge suite realistic_env_max_load success on 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a

github-actions bot commented Dec 5, 2024

✅ Forge suite compat success on 3527aa2e299553b759c515d9843586bad48c802c ==> 1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibalajiarun commented Dec 2, 2024 •

edited

Loading

trunk-io bot commented Dec 2, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a`

✅ Forge suite `compat` success on `3527aa2e299553b759c515d9843586bad48c802c` ==> `1f2e6581370ea6ef5e96ffbf290b35fd97eddd0a`