[dag] Integrate dag fetcher with RB node handler #9585

ibalajiarun · 2023-08-09T22:08:25Z

Description

This PR integrates DAG fetcher to the RB node handler. Whenever the node is missing parents, the fetcher is requested to fetch those missing parents and their ancestors. To make sure the GCed DAG is not refetched, it changes the lowest_round in dag_store to provide the information.

Test Plan

Existing tests.

ibalajiarun · 2023-08-09T22:08:43Z

Current dependencies on/for this PR:

main
- PR [dag] Handle certified node in DAG Driver #9312
  - PR [dag] Integrate Order Rule with dag driver #9452
    - PR [dag] Integrate Dag Fetcher with Dag Driver #9453
      - PR [dag] rename reliable_broadcast.rs to rb_handler.rs #9454
        
        PR [dag] bootstrap logic #9455
        
        PR [dag] network sender implementation #9456
        
        PR [dag] e2e integration test #9457
        
        PR [dag] Integrate dag fetcher with RB node handler #9585 👈
        
        PR [dag][bugfix] fix bitmask for incomplete first round #9723
        PR [dag] preliminary state sync implementation #9724

This comment was auto-generated by Graphite.

sasha8 · 2023-08-10T06:56:34Z

consensus/src/dag/rb_handler.rs

+                error!("request to fetch failed: {}", err);
+            }
+            bail!(NodeBroadcastHandleError::MissingParents);
+        }


Why there are missing parents in this case?

I merged this check with the below one. This one will miss the case when a node has all invalid parents.

zekun000 · 2023-08-11T19:06:12Z

consensus/src/dag/rb_handler.rs

        let current_round = node.metadata().round();

        // round 0 is a special case and does not require any parents
        if current_round == 0 {
-            return Ok(());
+            bail!(NodeBroadcastHandleError::InvalidRound);


nit: we should start to carry more debug information into the errors, like exact round, missing parents id, why it's invalid etc

Fixed this now. I will do a separate pass for improving the errors overall and start adding metric counters.

zekun000 · 2023-08-11T19:13:11Z

consensus/src/dag/rb_handler.rs

-            NodeBroadcastHandleError::MissingParents
-        );
+        if prev_round >= dag_reader.lowest_round() {
+            if let Err(err) = self.fetch_requester.request_for_node(node) {


I think we should at least unify this condition with the one below, it also doesn't look correct to me, maybe what you meant to check is prev_round > highest_round?

previously the check is to filter out node that below lowest round, the error was misleading

Good catch. I did not account for check inversion - I was doing ensure! before. But, I removed this condition and just use the one with missing parents check below. Previously I was not checking for signatures if none of the parents existed locally, but I should have.

zekun000 · 2023-08-22T00:48:30Z

consensus/src/dag/rb_handler.rs


        let dag_reader = self.dag.read();
-        // check if the parent round is missing in the DAG
-        ensure!(


we still need to filter out the lower round right? otherwise we'd fetch all those nodes from gc'ed rounds

That is a good point. I can send you a node from a very old round and force you to fetch if it is below the GC level. We should ignore nodes from rounds below the GC level.

Yes, I needed the DAG store to support this minimum round to fix this.

It's becoming hard to write unit tests without a verifier trait. I will follow-up to introduce that trait and then we can have more unit tests for this case. Added TODO for now.

sasha8 · 2023-08-22T17:56:59Z

consensus/src/dag/rb_handler.rs

@@ -92,18 +88,21 @@ impl NodeBroadcastHandler {
            .cloned()
            .collect();
        if !missing_parents.is_empty() {
-            // For each missing parent, verify their signatures and voting power
+            // For each missing parent, verify their signatures and voting power. Otherwise, a malicious node can send 
+            // bad nodes with fake parents and cause this peer to issue unnecessary fetch requests.
            ensure!(
                missing_parents
                    .iter()
                    .all(|parent| { parent.verify(&self.epoch_state.verifier).is_ok() }),


Should this be in node.verify()?

This is to only verify the missing parents. Ideally, we won't miss any parents in the normal case and don't have to verify at all.

github-actions · 2023-08-23T23:33:43Z

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `72cf56d05ce9326379f9eee77b2a88054f45d704`

Compatibility test results for aptos-node-v1.6.2 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704 (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4152 txn/s, latency: 7016 ms, (p50: 6600 ms, p90: 9900 ms, p99: 14200 ms), latency samples: 174420
2. Upgrading first Validator to new version: 72cf56d05ce9326379f9eee77b2a88054f45d704
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1775 txn/s, latency: 15952 ms, (p50: 19300 ms, p90: 22000 ms, p99: 22600 ms), latency samples: 92340
3. Upgrading rest of first batch to new version: 72cf56d05ce9326379f9eee77b2a88054f45d704
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1796 txn/s, latency: 16445 ms, (p50: 19200 ms, p90: 22300 ms, p99: 22500 ms), latency samples: 93440
4. upgrading second batch to new version: 72cf56d05ce9326379f9eee77b2a88054f45d704
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3388 txn/s, latency: 9332 ms, (p50: 10200 ms, p90: 12700 ms, p99: 13300 ms), latency samples: 135520
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704 passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2023-08-23T23:36:37Z

✅ Forge suite `realistic_env_max_load` success on `72cf56d05ce9326379f9eee77b2a88054f45d704`

two traffics test: inner traffic : committed: 6027 txn/s, latency: 6495 ms, (p50: 6300 ms, p90: 8100 ms, p99: 16300 ms), latency samples: 2615860
two traffics test : committed: 100 txn/s, latency: 3184 ms, (p50: 2900 ms, p90: 3900 ms, p99: 10000 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.224, avg: 0.213", "QsPosToProposal: max: 0.408, avg: 0.197", "ConsensusProposalToOrdered: max: 0.711, avg: 0.651", "ConsensusOrderedToCommit: max: 0.551, avg: 0.522", "ConsensusProposalToCommit: max: 1.223, avg: 1.173"]
Max round gap was 1 [limit 4] at version 1340981. Max no progress secs was 4.017658 [limit 10] at version 1342809.
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2023-08-23T23:47:09Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `72cf56d05ce9326379f9eee77b2a88054f45d704`

Compatibility test results for aptos-node-v1.5.1 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704 (PR)
Upgrade the nodes to version: 72cf56d05ce9326379f9eee77b2a88054f45d704
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4568 txn/s, latency: 7152 ms, (p50: 7800 ms, p90: 10200 ms, p99: 10800 ms), latency samples: 169040
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704 passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

ibalajiarun requested a review from zekun000 as a code owner August 9, 2023 22:08

sasha8 self-assigned this Aug 10, 2023

sasha8 self-requested a review August 10, 2023 06:42

sasha8 reviewed Aug 10, 2023

View reviewed changes

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from 956bfdb to d6fab23 Compare August 10, 2023 23:05

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 716d377 to 59ba104 Compare August 10, 2023 23:05

zekun000 requested changes Aug 11, 2023

View reviewed changes

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from d6fab23 to 1fbee40 Compare August 11, 2023 19:41

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 59ba104 to 32fcb06 Compare August 11, 2023 19:41

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from 1fbee40 to a444858 Compare August 11, 2023 19:45

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 32fcb06 to 8b0a5d4 Compare August 11, 2023 19:45

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from a444858 to b6f8a6a Compare August 11, 2023 19:59

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 8b0a5d4 to 243f6be Compare August 11, 2023 19:59

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from b6f8a6a to 609eae5 Compare August 11, 2023 20:57

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 243f6be to 5c9dd14 Compare August 11, 2023 20:57

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from 609eae5 to e5b998b Compare August 11, 2023 21:51

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 5c9dd14 to ea63e78 Compare August 11, 2023 21:51

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from e5b998b to dd10cc2 Compare August 14, 2023 19:09

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from ea63e78 to cbd8f53 Compare August 14, 2023 19:09

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from dd10cc2 to 7a0a66f Compare August 14, 2023 19:25

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from cbd8f53 to ec7a202 Compare August 14, 2023 19:25

ibalajiarun requested a review from a team as a code owner August 14, 2023 20:27

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from 7567419 to 39527bf Compare August 21, 2023 19:51

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from acbbad8 to 37d61fd Compare August 21, 2023 19:51

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from 39527bf to bc49d3c Compare August 21, 2023 21:18

ibalajiarun force-pushed the balaji/dag-rb-fetch branch 2 times, most recently from 38e8f4a to c596a63 Compare August 21, 2023 23:51

ibalajiarun requested review from sasha8 and zekun000 August 21, 2023 23:54

zekun000 reviewed Aug 22, 2023

View reviewed changes

sasha8 reviewed Aug 22, 2023

View reviewed changes

ibalajiarun force-pushed the balaji/dag-e2e-6 branch from bc49d3c to eb5f074 Compare August 22, 2023 20:52

ibalajiarun force-pushed the balaji/dag-rb-fetch branch 2 times, most recently from 94171db to 781eec9 Compare August 22, 2023 20:57

ibalajiarun requested review from zekun000 and sasha8 August 22, 2023 21:00

sasha8 approved these changes Aug 22, 2023

View reviewed changes

Base automatically changed from balaji/dag-e2e-6 to main August 22, 2023 21:36

[dag] Integrate dag fetcher with RB node handler

72cf56d

ibalajiarun force-pushed the balaji/dag-rb-fetch branch from 781eec9 to 72cf56d Compare August 22, 2023 23:53

This was referenced Aug 22, 2023

[dag][bugfix] fix bitmask for incomplete first round #9723

Merged

[dag] preliminary state sync implementation #9724

Merged

zekun000 enabled auto-merge (rebase) August 23, 2023 23:02

This comment has been minimized.

Sign in to view

zekun000 approved these changes Aug 24, 2023

View reviewed changes

zekun000 merged commit cfb7c43 into main Aug 24, 2023
73 checks passed

zekun000 deleted the balaji/dag-rb-fetch branch August 24, 2023 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dag] Integrate dag fetcher with RB node handler #9585

[dag] Integrate dag fetcher with RB node handler #9585

ibalajiarun commented Aug 9, 2023 •

edited

Loading

ibalajiarun commented Aug 9, 2023 •

edited

Loading

sasha8 Aug 10, 2023

ibalajiarun Aug 21, 2023

zekun000 Aug 11, 2023

ibalajiarun Aug 21, 2023 •

edited

Loading

zekun000 Aug 11, 2023

ibalajiarun Aug 21, 2023

zekun000 Aug 22, 2023

sasha8 Aug 22, 2023

ibalajiarun Aug 22, 2023

ibalajiarun Aug 22, 2023

sasha8 Aug 22, 2023

ibalajiarun Aug 22, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

[dag] Integrate dag fetcher with RB node handler #9585

[dag] Integrate dag fetcher with RB node handler #9585

Conversation

ibalajiarun commented Aug 9, 2023 • edited Loading

Description

Test Plan

ibalajiarun commented Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibalajiarun Aug 21, 2023 • 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

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 Aug 23, 2023

✅ Forge suite compat success on aptos-node-v1.6.2 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704

github-actions bot commented Aug 23, 2023

✅ Forge suite realistic_env_max_load success on 72cf56d05ce9326379f9eee77b2a88054f45d704

github-actions bot commented Aug 23, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 72cf56d05ce9326379f9eee77b2a88054f45d704

ibalajiarun commented Aug 9, 2023 •

edited

Loading

ibalajiarun commented Aug 9, 2023 •

edited

Loading

ibalajiarun Aug 21, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `72cf56d05ce9326379f9eee77b2a88054f45d704`

✅ Forge suite `realistic_env_max_load` success on `72cf56d05ce9326379f9eee77b2a88054f45d704`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `72cf56d05ce9326379f9eee77b2a88054f45d704`