[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

ibalajiarun · 2023-09-07T21:48:37Z

Description

This PR enables broadcasting latest ledger info with the CertifiedNode for state syncing. It obtains the ledger info from the storage adapter.

Test Plan

Fixed unit tests.

ibalajiarun · 2023-09-07T21:48:49Z

Current dependencies on/for this PR:

main
- PR [dag] split fetcher into service and helper struct #9830
  - PR [dag][bugfix] reverse nodes before adding to dag store #9831
    - PR [dag] preliminary state sync implementation #9724
      - PR [dag] broadcast CertifiedNodeMsg with LedgerInfo #9968 👈
        
        PR [dag] dag rebootstrap #9967
        
        PR [consensus] introduce CommitSignerProvider trait #10006
        
        PR [consensus] epoch manager refactoring; dag is coming #9994
        
        PR [dag] commit signer #10040
        
        PR [dag] store parent block info in adapter #10041
        PR [dag] simplify DAGNetworkSender impl #10042
        PR [dag] async message handler support #10055
        PR [consensus][dag] dag integration helpers #10056
        PR [dag] epoch manager integration; dag is here #10007
      - PR [dag] state sync good case unit test #9832

This comment was auto-generated by Graphite.

zekun000 · 2023-09-08T21:37:41Z

consensus/src/dag/dag_driver.rs

@@ -151,12 +155,19 @@ impl DagDriver {
        let signature_builder =
            SignatureBuilder::new(node.metadata().clone(), self.epoch_state.clone());
        let cert_ack_set = CertificateAckState::new(self.epoch_state.verifier.len());
+        let latest_ledger_info = self


carrying a db here is a bit ugly, I was thinking about caching one in the notifier adapter and rely on callback to update it, wdyt

i think we anyhow need the StorageAdapter here right? we can just use that instead. we can cache within the storage adapter if necessary?

sounds better, just expose a function on the DAGStorage?

sasha8

LGTM

sasha8 · 2023-09-08T22:12:13Z

consensus/src/dag/dag_handler.rs

@@ -92,7 +92,7 @@ impl NetworkHandler {
                .map(|r| r.into()),
            DAGMessage::CertifiedNodeMsg(node) => node
                .verify(&self.epoch_state.verifier)
-                .and_then(|_| self.dag_driver.process(node))
+                .and_then(|_| self.dag_driver.process(node.certified_node()))


It is a bit confusing that the node has certified_node that has a node...
Maybe change the name of the variable?

github-actions · 2023-09-11T20:06:30Z

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`

Compatibility test results for aptos-node-v1.6.2 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4557 txn/s, latency: 7225 ms, (p50: 7100 ms, p90: 10200 ms, p99: 12900 ms), latency samples: 173200
2. Upgrading first Validator to new version: 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1639 txn/s, latency: 16465 ms, (p50: 17500 ms, p90: 22200 ms, p99: 43100 ms), latency samples: 86900
3. Upgrading rest of first batch to new version: 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1381 txn/s, latency: 16665 ms, (p50: 18800 ms, p90: 22300 ms, p99: 38700 ms), latency samples: 92540
4. upgrading second batch to new version: 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3443 txn/s, latency: 9140 ms, (p50: 10200 ms, p90: 12600 ms, p99: 13300 ms), latency samples: 137740
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a passed
Test Ok

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

github-actions · 2023-09-11T20:10:22Z

✅ Forge suite `realistic_env_max_load` success on `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`

two traffics test: inner traffic : committed: 6359 txn/s, latency: 6123 ms, (p50: 6000 ms, p90: 7500 ms, p99: 11400 ms), latency samples: 2785260
two traffics test : committed: 100 txn/s, latency: 3224 ms, (p50: 3100 ms, p90: 4000 ms, p99: 6000 ms), latency samples: 1840
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.277, avg: 0.218", "QsPosToProposal: max: 0.155, avg: 0.147", "ConsensusProposalToOrdered: max: 0.610, avg: 0.573", "ConsensusOrderedToCommit: max: 0.509, avg: 0.492", "ConsensusProposalToCommit: max: 1.099, avg: 1.066"]
Max round gap was 1 [limit 4] at version 820659. Max no progress secs was 3.576528 [limit 10] at version 2824386.
Test Ok

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

github-actions · 2023-09-11T20:22:12Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`

Compatibility test results for aptos-node-v1.5.1 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a (PR)
Upgrade the nodes to version: 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 5132 txn/s, latency: 6290 ms, (p50: 6600 ms, p90: 9600 ms, p99: 11700 ms), latency samples: 189920
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a passed
Test Ok

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

ibalajiarun force-pushed the balaji/dag-state-sync branch 2 times, most recently from fab0ad8 to 6dd6796 Compare September 7, 2023 23:41

Base automatically changed from balaji/dag-state-sync to main September 8, 2023 14:45

ibalajiarun force-pushed the balaji/bcast-certified-node-msg branch from a5d3321 to 89c4dec Compare September 8, 2023 16:08

ibalajiarun marked this pull request as ready for review September 8, 2023 16:10

ibalajiarun requested review from zekun000 and sasha8 as code owners September 8, 2023 16:10

zekun000 reviewed Sep 8, 2023

View reviewed changes

sasha8 approved these changes Sep 8, 2023

View reviewed changes

[dag] broadcast CertifiedNodeMsg with LedgerInfo

73c0bd6

ibalajiarun force-pushed the balaji/bcast-certified-node-msg branch from 89c4dec to 1db0e3f Compare September 11, 2023 15:30

ibalajiarun mentioned this pull request Sep 11, 2023

[consensus] epoch manager refactoring; dag is coming #9994

Merged

address feedback

a2a16b5

ibalajiarun force-pushed the balaji/bcast-certified-node-msg branch from 1db0e3f to a2a16b5 Compare September 11, 2023 15:32

ibalajiarun requested a review from zekun000 September 11, 2023 15:32

zekun000 approved these changes Sep 11, 2023

View reviewed changes

lint

4ec231e

ibalajiarun enabled auto-merge (squash) September 11, 2023 19:01

This comment has been minimized.

Sign in to view

ibalajiarun disabled auto-merge September 11, 2023 19:33

This comment has been minimized.

Sign in to view

address feedback

1b28c10

ibalajiarun enabled auto-merge (squash) September 11, 2023 19:35

This comment has been minimized.

Sign in to view

ibalajiarun merged commit 0c0aa7f into main Sep 11, 2023

ibalajiarun deleted the balaji/bcast-certified-node-msg branch September 11, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

ibalajiarun commented Sep 7, 2023 •

edited

Loading

ibalajiarun commented Sep 7, 2023 •

edited

Loading

zekun000 Sep 8, 2023

ibalajiarun Sep 9, 2023

zekun000 Sep 11, 2023

sasha8 left a comment

sasha8 Sep 8, 2023 •

edited

Loading

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.

This comment has been minimized.

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

Conversation

ibalajiarun commented Sep 7, 2023 • edited Loading

Description

Test Plan

ibalajiarun commented Sep 7, 2023 • edited Loading

zekun000 Sep 8, 2023

Choose a reason for hiding this comment

ibalajiarun Sep 9, 2023

Choose a reason for hiding this comment

zekun000 Sep 11, 2023

Choose a reason for hiding this comment

sasha8 left a comment

Choose a reason for hiding this comment

sasha8 Sep 8, 2023 • edited Loading

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.

This comment has been minimized.

github-actions bot commented Sep 11, 2023

✅ Forge suite compat success on aptos-node-v1.6.2 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a

github-actions bot commented Sep 11, 2023

✅ Forge suite realistic_env_max_load success on 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a

github-actions bot commented Sep 11, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a

ibalajiarun commented Sep 7, 2023 •

edited

Loading

ibalajiarun commented Sep 7, 2023 •

edited

Loading

sasha8 Sep 8, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`

✅ Forge suite `realistic_env_max_load` success on `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `1b28c10ad0fdcebc681546ea1e2dbbbfa29c5c2a`