[dag] cache votes without re-counting #11808

ibalajiarun · 2024-01-28T22:16:41Z

Description

Instead of counting votes each time, this PR adds caching functionality for votes.

Also, checks for epoch when handling a RB node.

Test Plan

Fixed existing tests.

trunk-io · 2024-01-28T22:16:44Z

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

Job	Cumulative Duration	Recent Runs
rust-unit-tests	1h 15m	🟩 🟩
windows-build	1h 9m	🟩 🟩 🟩
rust-smoke-tests	29m	🟩
rust-lints	11m	🟥 🟥
run-tests-main-branch	7m	🟥 🟥
check-dynamic-deps	6m	🟩 🟩 🟩
general-lints	5m	🟩 🟩
semgrep/ci	53s	🟩 🟩 🟩
check	38s	🟥 ⬜ 🟥
file_change_determinator	21s	🟩 ⬜ 🟩
file_change_determinator	17s	🟩 🟩
permission-check	9s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩
permission-check	5s	🟩 ⬜ 🟩
permission-check	5s	🟩 ⬜ 🟩

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

Job	Duration	vs 7d avg	Delta
windows-build	26m	18m

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

ibalajiarun · 2024-01-28T22:16:58Z

Current dependencies on/for this PR:

main
- PR [dag] refactor/improve dag health monitoring #11362
  - PR [dag] stop voting based on pipeline latency #11407
    - PR [dag] concurrent dag store #11505
      - PR [dag] parallel message handler implementation #11685
        
        PR [dag] support parallel rb voting #11743
        
        PR [dag] broadcast nodes within window till all validators ack #11751
        
        PR [dag] cache votes without re-counting #11808 👈
        
        PR [dag] decouple payload 2/n #11918
        
        PR Balaji/dag forge #11433

This stack of pull requests is managed by Graphite.

sasha8 · 2024-02-01T22:07:40Z

consensus/src/dag/dag_store.rs

+        }
+
+        for parent in node.parents_metadata() {
+            let voting_power = self


you can move this outside, no?

zekun000 · 2024-02-04T00:22:56Z

consensus/src/dag/dag_store.rs

+                .expect("must exist");
+            match node_status {
+                Some(NodeStatus::Unordered {
+                    votes_culumative_voting_power,


I'd call it weak_vote and strong_vote correspondingly

zekun000 · 2024-02-04T00:23:47Z

consensus/src/dag/dag_store.rs

+                .expect("must exist");
+            let node_status = self
+                .get_node_ref_mut(parent.round(), parent.author())
+                .expect("must exist");


just add another expect here instead of unreachable below?

we need a mutable reference to node_status. If i expect, then I wont get a reference to the Option in BTreeMap.

zekun000 · 2024-02-04T00:29:12Z

consensus/src/dag/types.rs

+            _ = tx.take().expect("must exist").send(certificate);
+        }
+
+        if partial_signatures.signatures().len() == self.epoch_state.verifier.len() {


add some comments here to explain why the aggregation is through a channel? I assume it's because we want to keep the rb alive even after cert?

github-actions · 2024-02-09T16:11:55Z

✅ Forge suite `compat` success on `aptos-node-v1.8.3` ==> `e09ec1fbf02cfa539b4c274104cf122f4e77eb23`

Compatibility test results for aptos-node-v1.8.3 ==> e09ec1fbf02cfa539b4c274104cf122f4e77eb23 (PR)
1. Check liveness of validators at old version: aptos-node-v1.8.3
compatibility::simple-validator-upgrade::liveness-check : committed: 4737 txn/s, latency: 6749 ms, (p50: 6600 ms, p90: 9600 ms, p99: 17200 ms), latency samples: 180040
2. Upgrading first Validator to new version: e09ec1fbf02cfa539b4c274104cf122f4e77eb23
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1711 txn/s, latency: 16558 ms, (p50: 19200 ms, p90: 25000 ms, p99: 26200 ms), latency samples: 92440
3. Upgrading rest of first batch to new version: e09ec1fbf02cfa539b4c274104cf122f4e77eb23
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1839 txn/s, latency: 15653 ms, (p50: 19600 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 91980
4. upgrading second batch to new version: e09ec1fbf02cfa539b4c274104cf122f4e77eb23
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3170 txn/s, latency: 9487 ms, (p50: 9900 ms, p90: 12200 ms, p99: 12900 ms), latency samples: 136340
5. check swarm health
Compatibility test for aptos-node-v1.8.3 ==> e09ec1fbf02cfa539b4c274104cf122f4e77eb23 passed
Test Ok

github-actions · 2024-02-09T16:13:58Z

✅ Forge suite `realistic_env_max_load` success on `e09ec1fbf02cfa539b4c274104cf122f4e77eb23`

two traffics test: inner traffic : committed: 7435 txn/s, latency: 5112 ms, (p50: 4600 ms, p90: 6400 ms, p99: 13300 ms), latency samples: 3212220
two traffics test : committed: 100 txn/s, latency: 2452 ms, (p50: 2100 ms, p90: 2500 ms, p99: 14500 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.274, avg: 0.210", "QsPosToProposal: max: 0.199, avg: 0.169", "ConsensusProposalToOrdered: max: 0.596, avg: 0.562", "ConsensusOrderedToCommit: max: 0.500, avg: 0.469", "ConsensusProposalToCommit: max: 1.069, avg: 1.031"]
Max round gap was 1 [limit 4] at version 1544694. Max no progress secs was 9.913752 [limit 15] at version 1544694.
Test Ok

This was referenced Jan 28, 2024

[dag] refactor/improve dag health monitoring #11362

Merged

[dag] stop voting based on pipeline latency #11407

Merged

Balaji/dag forge #11433

Closed

[dag] concurrent dag store #11505

Merged

[dag] parallel message handler implementation #11685

Merged

This was referenced Jan 28, 2024

[dag] support parallel rb voting #11743

Merged

[dag] broadcast nodes within window till all validators ack #11751

Merged

ibalajiarun marked this pull request as ready for review January 28, 2024 22:17

ibalajiarun requested review from zekun000 and sasha8 as code owners January 28, 2024 22:17

ibalajiarun force-pushed the balaji/dag-count-votes-opt branch from 9604403 to a184dfa Compare January 31, 2024 18:53

sasha8 approved these changes Feb 1, 2024

View reviewed changes

zekun000 approved these changes Feb 4, 2024

View reviewed changes

ibalajiarun mentioned this pull request Feb 6, 2024

[dag] decouple payload 2/n #11918

Closed

ibalajiarun force-pushed the balaji/extended-rb branch from ba071cb to 1deb748 Compare February 8, 2024 23:31

ibalajiarun requested review from JoshLind and gregnazario as code owners February 8, 2024 23:31

ibalajiarun force-pushed the balaji/dag-count-votes-opt branch from a184dfa to ae6a329 Compare February 8, 2024 23:31

ibalajiarun force-pushed the balaji/extended-rb branch from 1deb748 to 012db41 Compare February 9, 2024 05:01

Base automatically changed from balaji/extended-rb to main February 9, 2024 06:41

ibalajiarun added 4 commits February 9, 2024 07:17

[dag] cache votes without re-counting

9e83960

[dag] keep broadcasting node until all ack

1ede79b

split vote counting

a57e595

lint

9a52fb1

ibalajiarun force-pushed the balaji/dag-count-votes-opt branch from ae6a329 to 9a52fb1 Compare February 9, 2024 15:28

lint

e09ec1f

ibalajiarun enabled auto-merge (squash) February 9, 2024 15:46

This comment has been minimized.

Sign in to view

ibalajiarun merged commit a83b5f7 into main Feb 9, 2024
77 of 80 checks passed

ibalajiarun deleted the balaji/dag-count-votes-opt branch February 9, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dag] cache votes without re-counting #11808

[dag] cache votes without re-counting #11808

ibalajiarun commented Jan 28, 2024 •

edited

Loading

trunk-io bot commented Jan 28, 2024 •

edited

Loading

ibalajiarun commented Jan 28, 2024 •

edited

Loading

sasha8 Feb 1, 2024

zekun000 Feb 4, 2024

zekun000 Feb 4, 2024

ibalajiarun Feb 9, 2024

zekun000 Feb 4, 2024

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 9, 2024

github-actions bot commented Feb 9, 2024

[dag] cache votes without re-counting #11808

[dag] cache votes without re-counting #11808

Conversation

ibalajiarun commented Jan 28, 2024 • edited Loading

Description

Test Plan

trunk-io bot commented Jan 28, 2024 • edited Loading

ibalajiarun commented Jan 28, 2024 • edited Loading

sasha8 Feb 1, 2024

Choose a reason for hiding this comment

zekun000 Feb 4, 2024

Choose a reason for hiding this comment

zekun000 Feb 4, 2024

Choose a reason for hiding this comment

ibalajiarun Feb 9, 2024

Choose a reason for hiding this comment

zekun000 Feb 4, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 9, 2024

✅ Forge suite compat success on aptos-node-v1.8.3 ==> e09ec1fbf02cfa539b4c274104cf122f4e77eb23

github-actions bot commented Feb 9, 2024

✅ Forge suite realistic_env_max_load success on e09ec1fbf02cfa539b4c274104cf122f4e77eb23

ibalajiarun commented Jan 28, 2024 •

edited

Loading

trunk-io bot commented Jan 28, 2024 •

edited

Loading

ibalajiarun commented Jan 28, 2024 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.8.3` ==> `e09ec1fbf02cfa539b4c274104cf122f4e77eb23`

✅ Forge suite `realistic_env_max_load` success on `e09ec1fbf02cfa539b4c274104cf122f4e77eb23`