Optimistic signature verification for commit votes #14643

vusirikala · 2024-09-15T01:26:02Z

Description

This PR implements optimistic signature verification to reduce the time required to verify commit 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:26:06Z

⏱️ 3h 59m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 6m	🟩 🟩 🟩
forge-compat-test / forge	33m	🟩 🟩
forge-e2e-test / forge	28m	🟩 🟩
test-target-determinator	13m	🟩 🟩 🟩
execution-performance / test-target-determinator	13m	🟩 🟩 🟩
check	11m	🟩 🟩 🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	9m	🟩
general-lints	7m	🟩 🟩 🟩 🟩
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	3m	🟥

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

danielxiangzl · 2024-09-16T21:20:21Z

consensus/src/pipeline/buffer_manager.rs

                        match commit_msg.req.verify(&epoch_state_clone.verifier) {
                            Ok(_) => {
-                                let _ = tx.unbounded_send(commit_msg);
+                                let _ = tx.unbounded_send((commit_msg, true));


nit: Instead of true and false, it reads better if you can create an enum for verified and unverified and pass them.

…_votes_and_order_votes

zekun000 · 2024-10-08T20:26:53Z

consensus/src/pipeline/buffer_item.rs

+    for vote in unverified_votes {
+        let author = vote.author();
+        let sig = vote.signature_with_status();
+        if vote.ledger_info() == commit_ledger_info {


I don't understand what does this condition mean, if the ledger info doesn't match we should just filter out those signatures?

zekun000 · 2024-10-08T20:30:12Z

consensus/src/pipeline/buffer_item.rs

@@ -95,7 +74,7 @@ fn aggregate_commit_proof(
 // we differentiate buffer items at different stages
 // for better code readability
 pub struct OrderedItem {
-    pub unverified_signatures: PartialSignatures,
+    pub unverified_votes: Vec<CommitVote>,


using vector here loses the guarantee that one author only has one vote in partial signatures, why this is not a ledger info with unverified signatures?

zekun000 · 2024-10-08T20:33:03Z

consensus/src/pipeline/buffer_manager.rs

@@ -704,6 +698,17 @@ impl BufferManager {
        }
    }

+    fn get_commit_message(commit_vote: CommitVote) -> CommitMessage {


nit: this type of function typically prefix with gen|generate not get

zekun000 · 2024-10-09T21:43:48Z

consensus/src/pipeline/buffer_item.rs

+    for vote in unverified_votes.values() {
+        let sig = vote.signature_with_status();
+        if vote.ledger_info() == commit_ledger_info {
+            li_with_sig.add_signature(vote.author(), sig);


nit: this function signature looks awkward, it should take a ownership of sig instead of a reference and clone inside

zekun000 · 2024-10-09T21:44:03Z

config/src/config/consensus_config.rs

@@ -321,7 +321,7 @@ impl Default for ConsensusConfig {
            num_bounded_executor_tasks: 16,
            enable_pre_commit: true,
            max_pending_rounds_in_commit_vote_cache: 100,
-            optimistic_sig_verification: false,
+            optimistic_sig_verification: true,


disable before landing

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

✅ Forge suite `realistic_env_max_load` success on `a3ba6dc1a71de655aec773a105feeca5552c937f`

two traffics test: inner traffic : committed: 13655.62 txn/s, latency: 2916.16 ms, (p50: 2700 ms, p70: 3000, p90: 3000 ms, p99: 6000 ms), latency samples: 5192200
two traffics test : committed: 100.00 txn/s, latency: 2484.54 ms, (p50: 2400 ms, p70: 2500, p90: 2800 ms, p99: 5800 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.251, avg: 0.222", "QsPosToProposal: max: 0.269, avg: 0.247", "ConsensusProposalToOrdered: max: 0.332, avg: 0.301", "ConsensusOrderedToCommit: max: 0.478, avg: 0.455", "ConsensusProposalToCommit: max: 0.775, avg: 0.756"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.01s no progress at version 32781 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.23s no progress at version 2050010 (avg 6.64s) [limit 15].
Test Ok

github-actions · 2024-10-09T23:08:58Z

✅ Forge suite `framework_upgrade` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `a3ba6dc1a71de655aec773a105feeca5552c937f`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f (PR)
Upgrade the nodes to version: a3ba6dc1a71de655aec773a105feeca5552c937f
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1155.63 txn/s, submitted: 1159.15 txn/s, failed submission: 3.51 txn/s, expired: 3.51 txn/s, latency: 2558.38 ms, (p50: 2100 ms, p70: 2600, p90: 4200 ms, p99: 6600 ms), latency samples: 105240
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1129.51 txn/s, submitted: 1131.74 txn/s, failed submission: 2.23 txn/s, expired: 2.23 txn/s, latency: 2654.13 ms, (p50: 2400 ms, p70: 3000, p90: 3900 ms, p99: 5700 ms), latency samples: 101320
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f passed
Upgrade the remaining nodes to version: a3ba6dc1a71de655aec773a105feeca5552c937f
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 814.19 txn/s, submitted: 815.51 txn/s, failed submission: 1.32 txn/s, expired: 1.32 txn/s, latency: 3591.20 ms, (p50: 2400 ms, p70: 4500, p90: 6500 ms, p99: 15300 ms), latency samples: 73880
Test Ok

github-actions · 2024-10-09T23:14:52Z

✅ Forge suite `compat` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `a3ba6dc1a71de655aec773a105feeca5552c937f`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f (PR)
1. Check liveness of validators at old version: 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775
compatibility::simple-validator-upgrade::liveness-check : committed: 11266.66 txn/s, latency: 2499.51 ms, (p50: 1900 ms, p70: 2000, p90: 2400 ms, p99: 29000 ms), latency samples: 464220
2. Upgrading first Validator to new version: a3ba6dc1a71de655aec773a105feeca5552c937f
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7034.57 txn/s, latency: 3989.63 ms, (p50: 4300 ms, p70: 4700, p90: 4900 ms, p99: 5000 ms), latency samples: 130920
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6910.85 txn/s, latency: 4707.04 ms, (p50: 4900 ms, p70: 5100, p90: 6100 ms, p99: 6500 ms), latency samples: 237660
3. Upgrading rest of first batch to new version: a3ba6dc1a71de655aec773a105feeca5552c937f
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6733.24 txn/s, latency: 4128.30 ms, (p50: 4600 ms, p70: 5000, p90: 5200 ms, p99: 5400 ms), latency samples: 125640
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6932.23 txn/s, latency: 4649.81 ms, (p50: 4800 ms, p70: 5200, p90: 6300 ms, p99: 6800 ms), latency samples: 233680
4. upgrading second batch to new version: a3ba6dc1a71de655aec773a105feeca5552c937f
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11060.83 txn/s, latency: 2497.95 ms, (p50: 2700 ms, p70: 2900, p90: 3000 ms, p99: 3200 ms), latency samples: 192480
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11452.23 txn/s, latency: 2742.18 ms, (p50: 2600 ms, p70: 2800, p90: 3300 ms, p99: 4700 ms), latency samples: 370580
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f passed
Test Ok

vusirikala added 5 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

Optimistic signature verification for commit votes

02d34f3

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

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

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

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

This comment has been minimized.

Sign in to view

vusirikala added 3 commits September 15, 2024 12:05

Resetting minor changes

8d197db

Resetting minor changes

9a43029

Minor change

2237342

danielxiangzl approved these changes Sep 16, 2024

View reviewed changes

vusirikala added 3 commits September 16, 2024 17:44

Addressing PR comments

32d5633

Addressing PR comments

1df442b

Merge branch 'satya/ledger_info_with_mixed_signatures' into satya/osv…

6fce605

…_votes_and_order_votes

This comment has been minimized.

Sign in to view

Base automatically changed from satya/osv_votes_and_order_votes to main October 7, 2024 23:20

Merge branch 'main' into satya/osv_commit_votes

d0a056d

This comment has been minimized.

Sign in to view

zekun000 reviewed Oct 8, 2024

View reviewed changes

Minor fix

592c51d

This comment has been minimized.

Sign in to view

Add a unit test

5a8269d

zekun000 approved these changes Oct 9, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Added another unit test

a3ba6dc

vusirikala enabled auto-merge (squash) October 9, 2024 22:36

This comment has been minimized.

Sign in to view

vusirikala merged commit 0efb4fc into main Oct 9, 2024
72 of 94 checks passed

vusirikala deleted the satya/osv_commit_votes branch October 9, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimistic signature verification for commit votes #14643

Optimistic signature verification for commit votes #14643

vusirikala commented Sep 15, 2024 •

edited

Loading

trunk-io bot commented Sep 15, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

danielxiangzl Sep 16, 2024

vusirikala Sep 17, 2024

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.

zekun000 Oct 8, 2024

zekun000 Oct 8, 2024

zekun000 Oct 8, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zekun000 Oct 9, 2024

zekun000 Oct 9, 2024

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 Oct 9, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

Optimistic signature verification for commit votes #14643

Optimistic signature verification for commit votes #14643

Conversation

vusirikala commented Sep 15, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Sep 15, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

danielxiangzl Sep 16, 2024

Choose a reason for hiding this comment

vusirikala Sep 17, 2024

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.

zekun000 Oct 8, 2024

Choose a reason for hiding this comment

zekun000 Oct 8, 2024

Choose a reason for hiding this comment

zekun000 Oct 8, 2024

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.

zekun000 Oct 9, 2024

Choose a reason for hiding this comment

zekun000 Oct 9, 2024

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.

github-actions bot commented Oct 9, 2024

✅ Forge suite realistic_env_max_load success on a3ba6dc1a71de655aec773a105feeca5552c937f

github-actions bot commented Oct 9, 2024

✅ Forge suite framework_upgrade success on 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f

github-actions bot commented Oct 9, 2024

✅ Forge suite compat success on 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> a3ba6dc1a71de655aec773a105feeca5552c937f

vusirikala commented Sep 15, 2024 •

edited

Loading

trunk-io bot commented Sep 15, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `a3ba6dc1a71de655aec773a105feeca5552c937f`

✅ Forge suite `framework_upgrade` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `a3ba6dc1a71de655aec773a105feeca5552c937f`

✅ Forge suite `compat` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `a3ba6dc1a71de655aec773a105feeca5552c937f`