[consensus] split round timeout msg out of vote msg #14433

ibalajiarun · 2024-08-27T15:46:25Z

Description

This PR splits out the timeout data from the Vote struct into new RoundTimeout struct. This means validators do not need to form and vote on a nil block when timing out a round. Also, the purpose of the nil block is satisfied by the order vote mechanism, which ensures the parent gets a QC even if the next round proposal fails/timeout.

The change is behind a local config, which can be flipped after a full binary rollout.

Test Plan

Compat test flipping config flag false to true: https://github.com/aptos-labs/aptos-core/actions/runs/10603758720

This PR adds a Smoke test to flip the flag as well

trunk-io · 2024-08-27T15:46:29Z

⏱️ 15h 19m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	4h 1m	🟩 🟩 🟩 🟩 🟩 (+6 more)
forge-compat-test / forge	1h 22m	⬜ 🟥 ⬜ 🟩 🟩 (+2 more)
forge-e2e-test / forge	1h 21m	⬜ 🟥 ⬜ 🟩 🟩 (+3 more)
test-target-determinator	1h 1m	🟩 🟩 ⬜ 🟩 🟩 (+8 more)
execution-performance / test-target-determinator	54m	🟩 🟩 🟩 🟩 🟩 (+6 more)
general-lints	43m	🟩 🟩 🟩 🟩 🟩 (+20 more)
rust-cargo-deny	42m	🟩 🟩 🟩 🟩 🟩 (+20 more)
check-dynamic-deps	33m	🟩 🟩 🟩 🟩 🟩 (+20 more)
semgrep/ci	12m	🟩 🟩 🟩 🟩 🟩 (+20 more)
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	11m	🟩 🟩 ⬜ 🟩 🟩 (+2 more)
check	10m	🟥 🟥 🟥 🟩 🟩 (+6 more)
rust-move-tests	10m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
forge-compat-test / forge	22m	15m

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

zekun000 · 2024-08-27T20:44:55Z

consensus/consensus-types/src/round_timeout.rs

+    timeout: TwoChainTimeout,
+    author: Author,
+    /// LedgerInfo of a block that is going to be committed in case this vote gathers QC.
+    ledger_info: LedgerInfo,


hmm, why the timeout carries a ledger info?

zekun000 · 2024-08-27T20:47:01Z

consensus/src/network.rs

+                                    response_sender: callback,
+                                })
+                            },
+                            ConsensusMsg::RandGenMessage(req) => {


hmm, is this block just formatting change?

I added RoundTimeoutMsg handling, and then the formatter took over.

zekun000 · 2024-08-27T20:47:51Z

consensus/src/pending_votes.rs

        }
    }

+    /// Insert a vote and if the vote is valid, return a QuorumCertificate preferentially over a


the comment seems out-of-date, this function only aggregates tc?

zekun000 · 2024-08-27T20:53:22Z

consensus/src/round_manager.rs

+            VoteReceptionResult::NewQuorumCertificate(qc) => {
+                unreachable!()
+            },
+            VoteReceptionResult::New2ChainTimeoutCertificate(tc) => {


we probably can split the vote from timeout in the pending_vote too, so the result can be separate (after we disable the current vote msg)

Will add a TODO to do this after disabling the current vote handling.

zekun000 · 2024-08-27T20:53:49Z

consensus/src/round_manager.rs

@@ -1545,6 +1671,9 @@ impl RoundManager {
                        VerifiedEvent::VoteMsg(vote_msg) => {
                            monitor!("process_vote", self.process_vote_msg(*vote_msg).await)
                        }
+                        VerifiedEvent::RoundTimeoutV2Msg(vote_msg) => {
+                            monitor!("process_vote", self.process_round_timeout_msg(*vote_msg).await)


s/process_vote/process_timeout

zekun000 · 2024-08-27T20:55:25Z

types/src/on_chain_config/consensus_config.rs

+        main: ConsensusConfigV1,
+        quorum_store_enabled: bool,
+        order_vote_enabled: bool,
+        round_timeout_msg_enabled: bool,


any reason this needs to be on-chain config? we should be able to support both modes and transition in two releases?

zekun000 · 2024-09-26T22:19:07Z

consensus/src/liveness/round_state.rs

@@ -225,7 +228,7 @@ impl RoundState {

    /// Return if already voted for timeout
    pub fn is_vote_timeout(&self) -> bool {


the name is a bit confusing, this should be already_vote_timeout or something?

zekun000 · 2024-09-26T22:19:53Z

consensus/src/network.rs

@@ -405,6 +406,12 @@ impl NetworkSender {
        self.broadcast(msg).await
    }

+    pub async fn broadcast_round_timeout(&self, round_timeout: RoundTimeoutMsg) {
+        fail_point!("consensus::send::vote", |_| ());


s/vote/timeout

I reused the name to preserve any existing usages but I don't see any usages for these failpoints.

zekun000 · 2024-09-26T22:29:31Z

consensus/src/pending_votes.rs

@@ -99,6 +100,83 @@ impl PendingVotes {
        }
    }

+    /// Insert a RoundTimeout and return a TimeoutCertificate if it can be formed
+    pub fn insert_round_timeout(


we should re-use the code for insert_vote as well? at least majority piece

you mean checking if there was an existing timeout vote?

no I mean code wise it's duplicate of the piece in insert_vote?

The return result changes between the two implementations. In the old code, we return vote voting power if we don't have enough to aggregate regardless, but in new code, we return the tc voting power. If we want to keep existing logic then we have to keep them separate.

zekun000 · 2024-09-26T22:32:17Z

consensus/src/round_manager.rs

@@ -1197,6 +1253,9 @@ impl RoundManager {
                state = vote.vote_data().proposed().executed_state_id(),
                is_timeout = vote.is_timeout(),
            );
+            if self.local_config.enable_round_timeout_msg {
+                error!("Received Vote with Timeout with Round Timeout Message enabled.");


I don't think this holds, during rollout, some nodes will have the flag enabled while others don't. the flag can only control sender side but not receiver

This is meant to be a debugging statement, but since we don't send debug logs to humio, I made them errors. 🤦🏽. I just record the log and continue.

but it'll happen all the time during rollout (if there's timeout)

Yes, that is what my expectation is. If it can be useful to debug, why not? Don't you think it's useful? I can downgrade to warn as well.

we probably don't want noise during the rollout, this doesn't give much information as well since it's expected during the release

Removed it. Done.

zekun000 · 2024-09-26T22:33:29Z

consensus/src/round_manager.rs

+            epoch = timeout.epoch(),
+            round = timeout.round(),
+        );
+        if !self.local_config.enable_round_timeout_msg {


this too, the first release needs to have nodes enabling receiving, then the following release enabling the flag for sending

same as above. just logging for debugging and send to humio. I plan to ignore it from alerts. I could also make this a warn instead.

sasha8 · 2024-10-01T22:55:27Z

consensus/consensus-types/src/round_timeout.rs

+        );
+        ensure!(
+            self.round_timeout.two_chain_timeout().hqc_round()
+                <= self.sync_info.highest_certified_round(),


Why do we need <= here?

github-actions · 2024-10-02T00:33:22Z

✅ Forge suite `realistic_env_max_load` success on `d065858957566d652115306ab821c5d04904baeb`

two traffics test: inner traffic : committed: 14388.90 txn/s, latency: 2761.04 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3200 ms), latency samples: 5471000
two traffics test : committed: 99.91 txn/s, latency: 1659.25 ms, (p50: 1400 ms, p70: 1500, p90: 1600 ms, p99: 10600 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.244, avg: 0.228", "QsPosToProposal: max: 1.146, avg: 1.121", "ConsensusProposalToOrdered: max: 0.325, avg: 0.292", "ConsensusOrderedToCommit: max: 0.412, avg: 0.399", "ConsensusProposalToCommit: max: 0.703, avg: 0.691"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.62s no progress at version 2489658 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.66s no progress at version 2489656 (avg 8.66s) [limit 15].
Test Ok

github-actions · 2024-10-02T00:33:50Z

✅ Forge suite `compat` success on `7ef01a26f8d8a38610e3d364b722df517c970749` ==> `d065858957566d652115306ab821c5d04904baeb`

Compatibility test results for 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb (PR)
1. Check liveness of validators at old version: 7ef01a26f8d8a38610e3d364b722df517c970749
compatibility::simple-validator-upgrade::liveness-check : committed: 13545.06 txn/s, latency: 2500.60 ms, (p50: 2100 ms, p70: 2200, p90: 2800 ms, p99: 7800 ms), latency samples: 444960
2. Upgrading first Validator to new version: d065858957566d652115306ab821c5d04904baeb
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7371.33 txn/s, latency: 3871.65 ms, (p50: 4400 ms, p70: 4600, p90: 4700 ms, p99: 4800 ms), latency samples: 139020
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6849.07 txn/s, latency: 4635.00 ms, (p50: 4700 ms, p70: 4800, p90: 6800 ms, p99: 7100 ms), latency samples: 227080
3. Upgrading rest of first batch to new version: d065858957566d652115306ab821c5d04904baeb
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7468.29 txn/s, latency: 3827.11 ms, (p50: 4200 ms, p70: 4600, p90: 4600 ms, p99: 4800 ms), latency samples: 139420
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7411.63 txn/s, latency: 4325.40 ms, (p50: 4700 ms, p70: 4700, p90: 5600 ms, p99: 6100 ms), latency samples: 250140
4. upgrading second batch to new version: d065858957566d652115306ab821c5d04904baeb
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11118.45 txn/s, latency: 2502.88 ms, (p50: 2800 ms, p70: 2900, p90: 3000 ms, p99: 3200 ms), latency samples: 194620
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10002.48 txn/s, latency: 3130.41 ms, (p50: 2900 ms, p70: 3000, p90: 5700 ms, p99: 7100 ms), latency samples: 322780
5. check swarm health
Compatibility test for 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb passed
Test Ok

github-actions · 2024-10-02T00:37:24Z

✅ Forge suite `framework_upgrade` success on `7ef01a26f8d8a38610e3d364b722df517c970749` ==> `d065858957566d652115306ab821c5d04904baeb`

Compatibility test results for 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb (PR)
Upgrade the nodes to version: d065858957566d652115306ab821c5d04904baeb
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1350.66 txn/s, submitted: 1354.47 txn/s, failed submission: 3.81 txn/s, expired: 3.81 txn/s, latency: 2361.04 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 5400 ms), latency samples: 120580
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1150.05 txn/s, submitted: 1152.08 txn/s, failed submission: 2.02 txn/s, expired: 2.02 txn/s, latency: 2629.73 ms, (p50: 2400 ms, p70: 2700, p90: 3900 ms, p99: 5700 ms), latency samples: 102260
5. check swarm health
Compatibility test for 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb passed
Upgrade the remaining nodes to version: d065858957566d652115306ab821c5d04904baeb
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1214.90 txn/s, submitted: 1216.28 txn/s, failed submission: 1.38 txn/s, expired: 1.38 txn/s, latency: 2643.68 ms, (p50: 2700 ms, p70: 3000, p90: 3600 ms, p99: 5700 ms), latency samples: 105280
Test Ok

ibalajiarun force-pushed the balaji/vote-v2 branch 2 times, most recently from 1bccc3d to 141cab9 Compare August 27, 2024 20:53

zekun000 reviewed Aug 27, 2024

View reviewed changes

ibalajiarun force-pushed the balaji/vote-v2 branch 6 times, most recently from 5672524 to c37d436 Compare August 28, 2024 00:03

ibalajiarun changed the title ~~Balaji/vote v2~~ [consensus] split round timeout msg out of vote msg Aug 28, 2024

ibalajiarun force-pushed the balaji/vote-v2 branch from c37d436 to 1daf468 Compare August 28, 2024 00:23

ibalajiarun added the CICD:run-forge-e2e-perf Run the e2e perf forge only label Aug 28, 2024

ibalajiarun force-pushed the balaji/vote-v2 branch from 1daf468 to 94ec984 Compare August 28, 2024 02:58

This comment has been minimized.

Sign in to view

ibalajiarun added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Aug 28, 2024

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/vote-v2 branch from 94ec984 to bb52454 Compare August 28, 2024 17:18

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/vote-v2 branch from a594bc8 to 9948aac Compare August 28, 2024 19:59

This comment has been minimized.

Sign in to view

ibalajiarun force-pushed the balaji/vote-v2 branch from 3218147 to a1c10ef Compare August 29, 2024 14:56

ibalajiarun requested a review from zekun000 August 29, 2024 14:58

ibalajiarun force-pushed the balaji/vote-v2 branch from a1c10ef to 4e1780e Compare August 29, 2024 16:59

ibalajiarun force-pushed the balaji/vote-v2 branch 2 times, most recently from 810b8ea to 0f4ff74 Compare September 6, 2024 15:38

zekun000 reviewed Sep 26, 2024

View reviewed changes

ibalajiarun force-pushed the balaji/vote-v2 branch 2 times, most recently from 8d78e14 to 410c341 Compare September 30, 2024 22:29

ibalajiarun added 2 commits September 30, 2024 15:34

[consensus] split round timeout msg out of vote msg

443cb2c

Smoke test for round timeout msg rollout

26d5580

ibalajiarun force-pushed the balaji/vote-v2 branch from 410c341 to 26d5580 Compare September 30, 2024 22:35

fixup! [consensus] split round timeout msg out of vote msg

51518aa

ibalajiarun requested a review from zekun000 October 1, 2024 19:32

zekun000 approved these changes Oct 1, 2024

View reviewed changes

sasha8 approved these changes Oct 1, 2024

View reviewed changes

update goldenfiles

d065858

ibalajiarun enabled auto-merge (squash) October 2, 2024 00:05

This comment has been minimized.

Sign in to view

ibalajiarun merged commit 628e88b into main Oct 2, 2024
82 of 90 checks passed

ibalajiarun deleted the balaji/vote-v2 branch October 2, 2024 14:16

dependabot bot mentioned this pull request Dec 4, 2024

Bump axios, aptos and wagmi jagadish-chauhan/blockchain#13

Open

		@@ -225,7 +228,7 @@ impl RoundState {

		/// Return if already voted for timeout
		pub fn is_vote_timeout(&self) -> bool {

[consensus] split round timeout msg out of vote msg #14433

[consensus] split round timeout msg out of vote msg #14433

Conversation

ibalajiarun commented Aug 27, 2024 • edited Loading

Description

Test Plan

trunk-io bot commented Aug 27, 2024 • edited Loading

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.

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

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.

github-actions bot commented Oct 2, 2024

✅ Forge suite realistic_env_max_load success on d065858957566d652115306ab821c5d04904baeb

github-actions bot commented Oct 2, 2024

✅ Forge suite compat success on 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb

github-actions bot commented Oct 2, 2024

✅ Forge suite framework_upgrade success on 7ef01a26f8d8a38610e3d364b722df517c970749 ==> d065858957566d652115306ab821c5d04904baeb

ibalajiarun commented Aug 27, 2024 •

edited

Loading

trunk-io bot commented Aug 27, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `d065858957566d652115306ab821c5d04904baeb`

✅ Forge suite `compat` success on `7ef01a26f8d8a38610e3d364b722df517c970749` ==> `d065858957566d652115306ab821c5d04904baeb`

✅ Forge suite `framework_upgrade` success on `7ef01a26f8d8a38610e3d364b722df517c970749` ==> `d065858957566d652115306ab821c5d04904baeb`