Optimistic signature verification for votes and order votes #14642

vusirikala · 2024-09-15T01:17:01Z

Description

This PR implements optimistic signature verification to reduce the time required to verify proposal votes and order votes.
When the optimistic signature verification feature flag is enabled, we will not verify these messages up front. We will accumulate the unverified messages, and when the accumulated voting power is higher than a threshold, we will aggregate all the signatures and verify the aggregated signature.
If the verification fails, we need to verify each individual signature. The ValidatorVerifier stores the list of authors that submitted bad messages, and will disable the optimistic signature verification for these malicious voters.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-09-15T01:17:04Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	3h 27m	🟥 🟩 🟩 🟩 🟩 (+4 more)
forge-compat-test / forge	1h 19m	🟩 🟩 ⬜ 🟩 🟩
forge-e2e-test / forge	58m	🟩 🟩 ⬜ 🟩 🟩
test-target-determinator	45m	🟩 🟩 🟩 🟩 🟩 (+3 more)
execution-performance / test-target-determinator	39m	🟩 🟩 🟩 🟩 🟩 (+3 more)
check	29m	🟩 🟩 🟩 🟩 🟩 (+3 more)
general-lints	15m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-cargo-deny	15m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
execution-performance / test-target-determinator	8m	5m
test-target-determinator	8m	5m

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

danielxiangzl · 2024-10-03T21:57:59Z

types/src/ledger_info.rs

    }

    pub fn verified_voters(&self) -> impl Iterator<Item = &AccountAddress> {
-        self.verified_signatures.signatures().keys()
+        self.signatures.iter().filter_map(|(voter, signature)| {


nit: can shorten as

self.signatures.iter() .filter(|(_, sig)| sig.is_verified()) .map(|(voter, _)| voter)

types/src/ledger_info.rs

zekun000 · 2024-10-03T22:42:23Z

consensus/src/pending_votes.rs

+                VoteStatus::NotEnoughVotes(li) => {
+                    write!(
+                        f,
+                        "LI {} has {} verified votes {:?}, {} unverified votes {:?}",


please don't use debug format, it spams the log

zekun000 · 2024-10-03T22:46:22Z

types/src/ledger_info.rs

            Ok(_) => {
-                self.merge_signatures(&epoch_state.verifier, false);
+                self.filter_invalid_signatures(verifier, false);


in some sense we don't really need this since the aggregation already succeeds and returns?

Yeah, we don't. But it still makes the data structure complete, and makes it easy in test cases to check that after aggregate_and_verify function is called, we have x number of verified signatures.

Alin was telling me that even if the aggregation succeeds, it doesn't necessarily mean all individual signatures are valid, some validators can collude to produce two invalid signatures that can be aggregated to be valid. in that sense, for the sake of sanity, I think we probably shouldn't mark them as "valid"

zekun000 · 2024-10-03T22:51:24Z

consensus/src/pending_order_votes.rs

+            OrderVoteReceptionResult::VoteAdded(1)
+        );
+
+        vote_0.set_verified();


what are we trying to do by setting it to verified?

Just wanted to make sure that the data structure works when it receives a mix of verified and unverified signatures.

zekun000 · 2024-10-03T22:53:15Z

consensus/src/pending_order_votes.rs

+            pending_order_votes.insert_order_vote(&vote_2, &verifier, None),
+            OrderVoteReceptionResult::VoteAdded(2)
+        );
+        assert_eq!(verifier.pessimistic_verify_set().len(), 1);


instead, here we should check if valid signatures are set to verified?

Added the check.

zekun000 · 2024-10-03T22:54:39Z

consensus/src/pending_votes.rs

+        partial_sigs.add_signature(signers[0].author(), vote_0.signature().clone());
+
+        // same author voting for the same thing -> DuplicateVote
+        vote_0.set_verified();


same q here

Just wanted to make sure that the data structure works when it receives a mix of verified and unverified signatures.

zekun000 · 2024-10-03T22:57:57Z

testsuite/smoke-test/src/consensus/consensus_fault_tolerance.rs

@@ -213,6 +213,54 @@ async fn test_no_failures() {
    .unwrap();
 }

+#[tokio::test]
+async fn test_faulty_votes() {


how did you verify this works as expected? we don't have any logs showing pessimistic validators being added?

I have added some println! states in validator_verifier and checked that the authors that are added to pessimistic verify set are being verified pessimistically from next time.

github-actions · 2024-10-07T23:16:05Z

✅ Forge suite `realistic_env_max_load` success on `9cf9e3af36ec803f0798eaff294bef8f0b309794`

two traffics test: inner traffic : committed: 12878.40 txn/s, latency: 3093.44 ms, (p50: 2800 ms, p70: 3000, p90: 3600 ms, p99: 6300 ms), latency samples: 4896640
two traffics test : committed: 99.93 txn/s, latency: 3049.06 ms, (p50: 2600 ms, p70: 3000, p90: 4700 ms, p99: 7000 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.265, avg: 0.226", "QsPosToProposal: max: 0.228, avg: 0.181", "ConsensusProposalToOrdered: max: 0.329, avg: 0.299", "ConsensusOrderedToCommit: max: 0.453, avg: 0.427", "ConsensusProposalToCommit: max: 0.753, avg: 0.727"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.91s no progress at version 37499 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 7.60s no progress at version 2653032 (avg 7.60s) [limit 15].
Test Ok

github-actions · 2024-10-07T23:17:55Z

✅ Forge suite `compat` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `9cf9e3af36ec803f0798eaff294bef8f0b309794`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> 9cf9e3af36ec803f0798eaff294bef8f0b309794 (PR)
1. Check liveness of validators at old version: 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775
compatibility::simple-validator-upgrade::liveness-check : committed: 13100.53 txn/s, latency: 2633.72 ms, (p50: 2100 ms, p70: 2200, p90: 6600 ms, p99: 9400 ms), latency samples: 487540
2. Upgrading first Validator to new version: 9cf9e3af36ec803f0798eaff294bef8f0b309794
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7566.80 txn/s, latency: 3617.25 ms, (p50: 3600 ms, p70: 4400, p90: 4900 ms, p99: 5100 ms), latency samples: 139640
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7118.85 txn/s, latency: 4477.87 ms, (p50: 4600 ms, p70: 4800, p90: 6500 ms, p99: 6700 ms), latency samples: 242120
3. Upgrading rest of first batch to new version: 9cf9e3af36ec803f0798eaff294bef8f0b309794
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6375.48 txn/s, latency: 4467.00 ms, (p50: 4900 ms, p70: 5200, p90: 5900 ms, p99: 6200 ms), latency samples: 129360
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7201.80 txn/s, latency: 4483.95 ms, (p50: 4700 ms, p70: 4800, p90: 6400 ms, p99: 6800 ms), latency samples: 245280
4. upgrading second batch to new version: 9cf9e3af36ec803f0798eaff294bef8f0b309794
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11283.26 txn/s, latency: 2399.45 ms, (p50: 2600 ms, p70: 2700, p90: 3000 ms, p99: 3200 ms), latency samples: 200100
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10845.77 txn/s, latency: 2909.91 ms, (p50: 2800 ms, p70: 3000, p90: 3600 ms, p99: 4600 ms), latency samples: 354080
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> 9cf9e3af36ec803f0798eaff294bef8f0b309794 passed
Test Ok

github-actions · 2024-10-07T23:20:42Z

✅ Forge suite `framework_upgrade` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `9cf9e3af36ec803f0798eaff294bef8f0b309794`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> 9cf9e3af36ec803f0798eaff294bef8f0b309794 (PR)
Upgrade the nodes to version: 9cf9e3af36ec803f0798eaff294bef8f0b309794
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1048.78 txn/s, submitted: 1051.51 txn/s, failed submission: 2.73 txn/s, expired: 2.73 txn/s, latency: 2875.44 ms, (p50: 2700 ms, p70: 3000, p90: 4600 ms, p99: 6300 ms), latency samples: 92340
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1056.07 txn/s, submitted: 1057.84 txn/s, failed submission: 1.77 txn/s, expired: 1.77 txn/s, latency: 2866.53 ms, (p50: 2700 ms, p70: 3000, p90: 4500 ms, p99: 6300 ms), latency samples: 95660
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> 9cf9e3af36ec803f0798eaff294bef8f0b309794 passed
Upgrade the remaining nodes to version: 9cf9e3af36ec803f0798eaff294bef8f0b309794
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1068.75 txn/s, submitted: 1070.61 txn/s, failed submission: 1.86 txn/s, expired: 1.86 txn/s, latency: 2878.85 ms, (p50: 2700 ms, p70: 3000, p90: 4800 ms, p99: 6900 ms), latency samples: 92120
Test Ok

ibalajiarun · 2024-10-07T23:22:26Z

config/src/config/consensus_config.rs

@@ -78,6 +78,7 @@ pub struct ConsensusConfig {
    // must match one of the CHAIN_HEALTH_WINDOW_SIZES values.
    pub window_for_chain_health: usize,
    pub chain_health_backoff: Vec<ChainHealthBackoffValues>,
+    // Deprecated
    pub qc_aggregator_type: QcAggregatorType,


Since it's local config, why not remove it?

vusirikala added 4 commits September 14, 2024 17:12

Deprecate delayed QC aggregate msg

c906718

Ledger info with mixed signatures

c4c2cdd

Minor change

08f45c9

Optimistic signature verification for votes and order votes

1e85a83

vusirikala requested review from zekun000, sasha8, ibalajiarun, JoshLind and gregnazario as code owners September 15, 2024 01:17

vusirikala changed the base branch from main to satya/ledger_info_with_mixed_signatures September 15, 2024 01:17

vusirikala requested review from sitalkedia, danielxiangzl and igor-aptos and removed request for gregnazario, JoshLind and sasha8 September 15, 2024 01:17

vusirikala added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 15, 2024