Cache commit votes received for future rounds in the buffer manager #14570

vusirikala · 2024-09-09T20:47:49Z

Description

When the buffer manager receives a commit vote, but the block is not stored in the buffer items, we cache the commit votes and use those votes when the block is added to the buffer manager.

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-09T20:47:53Z

⏱️ 6h 55m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
forge-compat-test / forge	1h 26m	🟩 🟩 🟩 🟩 🟩 (+1 more)
forge-e2e-test / forge	1h 24m	🟩 🟩 🟩 🟩 🟩 (+1 more)
test-target-determinator	37m	🟩 🟩 🟩 🟩 🟩 (+3 more)
execution-performance / test-target-determinator	36m	🟩 🟩 🟩 🟩 🟩 (+3 more)
check	29m	🟩 🟩 🟩 🟩 🟩 (+3 more)
execution-performance / single-node-performance	26m	🟩 🟩 🟩 🟩 🟩 (+3 more)
general-lints	14m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-cargo-deny	14m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-tests	10m	🟩
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	9m	🟩 🟥 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	9m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-tests	2m	🟩
rust-move-tests	2m	🟩
rust-move-tests	2m	🟩
rust-move-tests	2m	🟩
rust-move-tests	2m	🟩
rust-move-tests	2m	🟩
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+3 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-tests	1m	🟩
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	29s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	26s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	22s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	19s	🟩 🟩 🟩 🟩 🟩 (+3 more)
determine-docker-build-metadata	16s	🟩 🟩 🟩 🟩 🟩 (+3 more)
Backport PR	5s	🟥
permission-check	2s	🟩
rust-move-tests	1s	⬜

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

Job	Duration	vs 7d avg	Delta
execution-performance / single-node-performance	25m	18m

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

zekun000 · 2024-09-09T21:18:01Z

consensus/src/pipeline/buffer_manager.rs

+
+    // If the buffer manager receives a commit vote for a block that is not in buffer items, then
+    // the vote will be cached. We can cache upto 30 blocks, and upto 150 commit votes per block.
+    pending_commit_votes: LruCache<HashValue, Vec<CommitVote>>,


this is not ideal, it's not ddos safe. a single malicious validator can flush all useful pending votes. we should maintain something similar to the PendingVote structure that only allows one vote per author.

I don't think this works as expected, a single malicious author can still send CommitVote for 30 fake blocks to occupy the whole cache

Good point. Implemented a different eviction mechanism now.
If the number of blocks in pending_commit_votes exceeds 50, then out of all the blocks that were last accessed more than 100ms ago, I remove one block with least voting power.
What do you think?

the invariant we want to maintain is very simple - every author can only have one vote per round and we don't cache rounds that are too far. we just need to maintain a map from round -> pending votes, and each round we only allow author to have one vote. it's basically a subset of the existing PendingVotes structure.

Yes. The current code allows only one vote per author per block.

I don't see how the current code prevents one author to have multiple fake blocks per round that push every other blocks out

For each block, we store HashMap<AccountAddress, CommitVote>. So, we can store only one commit vote per author.
In order to protect against malicious authors, we won't evict any block if its last accessed timestamp is within 100ms. When there is a need for eviction, out of all the blocks that were last accessed more than 100ms ago, we evict the blocks with least voting power. So, most likely the malicious authors' blocks will be evicted.

this doesn't prevent a malicious author to submit millions of blocks to OOM the node since there's no limit of how many blocks it can vote for. as I said above, I think the simple way is to maintain a map of round -> pending votes that only allows one vote per author.

danielxiangzl

LGTM except one small comment

config/src/config/consensus_config.rs

consensus/src/pipeline/buffer_manager.rs

zekun000 · 2024-09-18T01:03:09Z

consensus/src/pipeline/buffer_manager.rs

+                    .epoch_state
+                    .verifier
+                    .get_voting_power(&vote.author())
+                    .is_some()


I don't think we need this given that the signature was verified (it can't verify without being in the validator set)?

The commit vote verification checks that the address is in validator set, but doesn't check it has non zero voting power. But checking for non zero voting power just seems a minor check that can be removed.
With optimistic signature verification, the commit votes won't be verified. So, we will have to put check again to avoid OOM attacks.

what does optimistic verification have to do with this? we only allow one vote per author per round regardless of signature?

But what if a malicious party submits many votes under different author names who either don't exist in ValidatorVerifier or has a zero voting power? That will be an OOM attack vector too?

good point, we should have the check that the sender matches the declared author field, the network link is encrypted with noise handshake and the key is on-chain. so after we move to optimistic verification, we can add the sender check here

aptos-core/consensus/src/network.rs

Line 720 in 6a71f18

ConsensusMsg::CommitVoteMsg(commit_vote) => {

Removed the check and simplified the if else statement.

zekun000 · 2024-09-18T01:04:22Z

consensus/src/pipeline/buffer_manager.rs

+            } else {
+                let mut votes = HashMap::new();
+                votes.insert(vote.author(), vote);
+                self.pending_commit_votes.insert(round, votes);


this whole block can be just self.pending_commit_votes.entry(round).or_insert(..).insert(author, vote)

github-actions · 2024-09-18T17:04:08Z

✅ Forge suite `realistic_env_max_load` success on `017832589c2b180835802ba9ee3245e54215a42a`

two traffics test: inner traffic : committed: 14271.74 txn/s, submitted: 14273.69 txn/s, failed submission: 1.89 txn/s, expired: 1.95 txn/s, latency: 2782.43 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3800 ms), latency samples: 5426440
two traffics test : committed: 98.17 txn/s, submitted: 100.07 txn/s, failed submission: 1.90 txn/s, expired: 1.90 txn/s, latency: 2199.66 ms, (p50: 2300 ms, p70: 2400, p90: 2500 ms, p99: 5000 ms), latency samples: 1920
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.250, avg: 0.219", "QsPosToProposal: max: 0.391, avg: 0.338", "ConsensusProposalToOrdered: max: 0.321, avg: 0.295", "ConsensusOrderedToCommit: max: 0.430, avg: 0.407", "ConsensusProposalToCommit: max: 0.725, avg: 0.702"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.01s no progress at version 2703369 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.55s no progress at version 2703367 (avg 4.64s) [limit 15].
Test Ok

github-actions · 2024-09-18T17:09:35Z

✅ Forge suite `compat` success on `70806ff543496aa6de7807feff49e7e1370efd20` ==> `017832589c2b180835802ba9ee3245e54215a42a`

Compatibility test results for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 017832589c2b180835802ba9ee3245e54215a42a (PR)
1. Check liveness of validators at old version: 70806ff543496aa6de7807feff49e7e1370efd20
compatibility::simple-validator-upgrade::liveness-check : committed: 14506.12 txn/s, latency: 2224.41 ms, (p50: 1800 ms, p70: 1900, p90: 3800 ms, p99: 13300 ms), latency samples: 498320
2. Upgrading first Validator to new version: 017832589c2b180835802ba9ee3245e54215a42a
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7682.08 txn/s, latency: 3707.31 ms, (p50: 4100 ms, p70: 4300, p90: 4400 ms, p99: 4600 ms), latency samples: 140980
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6968.78 txn/s, latency: 4559.19 ms, (p50: 4500 ms, p70: 4500, p90: 7100 ms, p99: 7400 ms), latency samples: 263320
3. Upgrading rest of first batch to new version: 017832589c2b180835802ba9ee3245e54215a42a
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7787.20 txn/s, latency: 3635.62 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4500 ms), latency samples: 143540
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7741.74 txn/s, latency: 4125.37 ms, (p50: 4400 ms, p70: 4400, p90: 5100 ms, p99: 5500 ms), latency samples: 256160
4. upgrading second batch to new version: 017832589c2b180835802ba9ee3245e54215a42a
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10770.26 txn/s, latency: 2530.46 ms, (p50: 2700 ms, p70: 2900, p90: 3000 ms, p99: 3200 ms), latency samples: 192140
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10345.72 txn/s, latency: 2947.01 ms, (p50: 2700 ms, p70: 3000, p90: 5100 ms, p99: 6500 ms), latency samples: 337640
5. check swarm health
Compatibility test for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 017832589c2b180835802ba9ee3245e54215a42a passed
Test Ok

github-actions · 2024-09-18T17:57:22Z

✅ Forge suite `framework_upgrade` success on `70806ff543496aa6de7807feff49e7e1370efd20` ==> `017832589c2b180835802ba9ee3245e54215a42a`

Compatibility test results for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 017832589c2b180835802ba9ee3245e54215a42a (PR)
Upgrade the nodes to version: 017832589c2b180835802ba9ee3245e54215a42a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1247.50 txn/s, submitted: 1250.39 txn/s, failed submission: 2.89 txn/s, expired: 2.89 txn/s, latency: 2467.01 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 6000 ms), latency samples: 112280
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1244.82 txn/s, submitted: 1246.83 txn/s, failed submission: 2.01 txn/s, expired: 2.01 txn/s, latency: 2441.45 ms, (p50: 2100 ms, p70: 2700, p90: 3900 ms, p99: 5700 ms), latency samples: 111220
5. check swarm health
Compatibility test for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 017832589c2b180835802ba9ee3245e54215a42a passed
Upgrade the remaining nodes to version: 017832589c2b180835802ba9ee3245e54215a42a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1176.49 txn/s, submitted: 1180.22 txn/s, failed submission: 3.73 txn/s, expired: 3.73 txn/s, latency: 2488.70 ms, (p50: 2400 ms, p70: 2700, p90: 3400 ms, p99: 5400 ms), latency samples: 107140
Test Ok

Caching pending commit votes

bbe06ff

vusirikala requested review from zekun000, sasha8 and ibalajiarun as code owners September 9, 2024 20:47

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

vusirikala marked this pull request as draft September 9, 2024 20:48

vusirikala requested review from danielxiangzl and msmouse and removed request for sasha8 September 9, 2024 20:48

vusirikala marked this pull request as ready for review September 9, 2024 20:52

vusirikala changed the title ~~[Draft] Cache commit votes received for future rounds in the buffer manager~~ Cache commit votes received for future rounds in the buffer manager Sep 9, 2024