[dag] preliminary state sync implementation #9724

ibalajiarun · 2023-08-22T23:53:45Z

Description

This PR provides the preliminary implementation of state sync for the DAG. The integration part will be in another PR.

Test Plan

Unit test in following PR

ibalajiarun · 2023-08-22T23:53:57Z

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-08-30T23:05:45Z

consensus/src/dag/types.rs

@@ -418,6 +419,33 @@ impl TDAGMessage for CertifiedNode {
    }
 }

+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
+pub struct CertifiedNodeWithLedgerInfo {


the convention we have today is just to call it CertifiedNodeMessage

zekun000 · 2023-08-30T23:08:00Z

consensus/src/dag/dag_network.rs

+
+    async fn send_epoch_change(&self, proof: EpochChangeProof);
+
+    async fn send_commit_proof(&self, ledger_info: LedgerInfoWithSignatures);


I don't want to couple this two with network sender, this is more like self notifier. maybe another adapter trait?

zekun000 · 2023-08-30T23:09:54Z

consensus/src/dag/dag_state_sync.rs

+        // Note: ledger info round <= highest ordered round
+        if dag_reader.highest_committed_round().unwrap_or_default()
+            < ledger_info.commit_info().round()
+            && dag_reader.exists_by_round_digest(


do we need this existence check? if the round is between commit and order, it should always exist if we don't violate safety?

zekun000 · 2023-08-30T23:18:54Z

consensus/src/dag/dag_state_sync.rs

+        let commit_li = node.ledger_info();
+
+        // TODO: there is a case where DAG fetches missing nodes in window and a crash happens and when we restart,
+        // we end up with a gap between the DAG and we need to be smart enough to clean up the DAG before the gap.


ideally we pass the ledger info in constructor and do the cleanup

zekun000 · 2023-08-30T23:20:14Z

consensus/src/dag/dag_state_sync.rs

+        // Create a new DAG store and Fetch blocks
+        let target_round = node.round();
+        let start_round = commit_li.commit_info().round().saturating_sub(DAG_WINDOW);
+        let sync_dag_store = Arc::new(RwLock::new(Dag::new(


we probably need a different constructor for this, this'll re-construct the current dag from storage

zekun000 · 2023-08-30T23:25:38Z

consensus/src/dag/dag_state_sync.rs

+        {
+            let mut dag_writer = sync_dag_store.write();
+            dag_writer.prune();
+            if let Some(node_status) = dag_writer.get_node_ref_mut_by_round_digest(


this is not enough, we need recursively marking all previous committed anchors in the window and all causal history

discussed with @sasha8 on this, I think the best way is probably to read the events from block metadata that contains previous anchor information

Also, it is not enough to mark only the anchor, we should mark all the causal history.

Also, we are not persisting it. Won't this be a problem in recovery? Should we persist before calling state sync?

We don't persist in normal path as well. I think db will give us the latest ledger info from which we can always get this info back even after recovery.

But how will we know what nodes were already committed after recovery?

zekun000 · 2023-08-30T23:26:07Z

consensus/src/dag/dag_state_sync.rs

+            }
+        }
+
+        if commit_li.ledger_info().ends_epoch() {


I think we can move this up and skip the syncing

zekun000 · 2023-08-30T23:51:18Z

consensus/src/dag/dag_state_sync.rs

+use itertools::Itertools;
+use std::sync::Arc;
+
+pub const DAG_WINDOW: u64 = 10;


we need to move this probably to order rule, since it affects whether a node is included in a commit

I was thinking this should be onchain config.

zekun000 · 2023-08-30T23:53:40Z

consensus/src/dag/dag_state_sync.rs

+        {
+            let mut dag_writer = sync_dag_store.write();
+            dag_writer.prune();
+            if let Some(node_status) = dag_writer.get_node_ref_mut_by_round_digest(


discussed with @sasha8 on this, I think the best way is probably to read the events from block metadata that contains previous anchor information

github-actions · 2023-09-08T00:09:42Z

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `6dd67967a3c9f46769b8f50a4acbb582b7449617`

Compatibility test results for aptos-node-v1.6.2 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617 (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4252 txn/s, latency: 7318 ms, (p50: 6300 ms, p90: 11100 ms, p99: 22600 ms), latency samples: 170080
2. Upgrading first Validator to new version: 6dd67967a3c9f46769b8f50a4acbb582b7449617
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1779 txn/s, latency: 16369 ms, (p50: 19000 ms, p90: 22300 ms, p99: 22400 ms), latency samples: 92540
3. Upgrading rest of first batch to new version: 6dd67967a3c9f46769b8f50a4acbb582b7449617
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1435 txn/s, latency: 17093 ms, (p50: 19000 ms, p90: 22900 ms, p99: 38400 ms), latency samples: 91840
4. upgrading second batch to new version: 6dd67967a3c9f46769b8f50a4acbb582b7449617
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3478 txn/s, latency: 9173 ms, (p50: 9500 ms, p90: 12900 ms, p99: 13200 ms), latency samples: 146080
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617 passed
Test Ok

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

github-actions · 2023-09-08T00:14:53Z

✅ Forge suite `realistic_env_max_load` success on `6dd67967a3c9f46769b8f50a4acbb582b7449617`

two traffics test: inner traffic : committed: 6548 txn/s, latency: 5987 ms, (p50: 5700 ms, p90: 7700 ms, p99: 11600 ms), latency samples: 2829080
two traffics test : committed: 100 txn/s, latency: 3097 ms, (p50: 3000 ms, p90: 3800 ms, p99: 4800 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.220, avg: 0.211", "QsPosToProposal: max: 0.166, avg: 0.152", "ConsensusProposalToOrdered: max: 0.623, avg: 0.589", "ConsensusOrderedToCommit: max: 0.554, avg: 0.525", "ConsensusProposalToCommit: max: 1.160, avg: 1.115"]
Max round gap was 1 [limit 4] at version 1363788. Max no progress secs was 3.631506 [limit 10] at version 1363788.
Test Ok

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

github-actions · 2023-09-08T00:25:40Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `6dd67967a3c9f46769b8f50a4acbb582b7449617`

Compatibility test results for aptos-node-v1.5.1 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617 (PR)
Upgrade the nodes to version: 6dd67967a3c9f46769b8f50a4acbb582b7449617
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 5143 txn/s, latency: 6282 ms, (p50: 6500 ms, p90: 9600 ms, p99: 12400 ms), latency samples: 190320
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617 passed
Test Ok

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

ibalajiarun force-pushed the balaji/dag-state-sync branch from 85dd42d to 918b442 Compare August 22, 2023 23:53

ibalajiarun mentioned this pull request Aug 22, 2023

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

Merged

ibalajiarun force-pushed the balaji/dag-bitmask-fix branch from 984b7a8 to 178b9f5 Compare August 24, 2023 20:43

ibalajiarun force-pushed the balaji/dag-state-sync branch from 918b442 to ac67202 Compare August 24, 2023 20:43

ibalajiarun force-pushed the balaji/dag-bitmask-fix branch from 178b9f5 to 2bcdbd4 Compare August 24, 2023 20:54

ibalajiarun force-pushed the balaji/dag-state-sync branch from ac67202 to d4409ca Compare August 24, 2023 20:54

ibalajiarun force-pushed the balaji/dag-bitmask-fix branch from 2bcdbd4 to bc4d9ee Compare August 24, 2023 21:01

Base automatically changed from balaji/dag-bitmask-fix to main August 24, 2023 21:34

ibalajiarun force-pushed the balaji/dag-state-sync branch 2 times, most recently from abf8a81 to 7071576 Compare August 30, 2023 04:39

ibalajiarun changed the base branch from main to balaji/dag-fixes August 30, 2023 05:14

ibalajiarun force-pushed the balaji/dag-state-sync branch from 7071576 to 023b879 Compare August 30, 2023 05:14

This was referenced Aug 30, 2023

[dag] split fetcher into service and helper struct #9830

Merged

[dag][bugfix] reverse nodes before adding to dag store #9831

Merged

[dag] state sync good case unit test #9832

Merged

ibalajiarun marked this pull request as ready for review August 30, 2023 05:15

ibalajiarun requested a review from zekun000 as a code owner August 30, 2023 05:15

ibalajiarun changed the title ~~[dag][wip] state sync~~ [dag] preliminary state sync implementation Aug 30, 2023

ibalajiarun requested a review from sasha8 August 30, 2023 05:17

zekun000 reviewed Aug 30, 2023

View reviewed changes

ibalajiarun mentioned this pull request Sep 7, 2023

[dag] dag rebootstrap #9967

Merged

ibalajiarun force-pushed the balaji/dag-state-sync branch from 7e41139 to 3932158 Compare September 7, 2023 21:48

ibalajiarun mentioned this pull request Sep 7, 2023

[dag] broadcast CertifiedNodeMsg with LedgerInfo #9968

Merged

ibalajiarun force-pushed the balaji/dag-state-sync branch from 3932158 to fab0ad8 Compare September 7, 2023 23:25

ibalajiarun enabled auto-merge (squash) September 7, 2023 23:26

ibalajiarun added 7 commits September 7, 2023 16:26

[dag] populate anchor digest in LI for state sync

8dc5e3c

[dag] add epoch change and commit proof fn to DAGNetworkSender

e7e46ce

[dag] preliminary state sync logic

e2b004a

address feedback

d5ea560

remove committed node status enum

ab3e895

unify notifier and upstreamnotifier trait

1f3d306

address feedback

6dd6796

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

This comment has been minimized.

Sign in to view

ibalajiarun mentioned this pull request Sep 8, 2023

[dag] support commit event for history anchors #9955

Merged

ibalajiarun merged commit e6d05d5 into main Sep 8, 2023

ibalajiarun deleted the balaji/dag-state-sync branch September 8, 2023 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dag] preliminary state sync implementation #9724

[dag] preliminary state sync implementation #9724

ibalajiarun commented Aug 22, 2023 •

edited

Loading

ibalajiarun commented Aug 22, 2023 •

edited

Loading

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

sasha8 Sep 1, 2023

sasha8 Sep 1, 2023

ibalajiarun Sep 5, 2023

sasha8 Sep 5, 2023

zekun000 Aug 30, 2023

zekun000 Aug 30, 2023

ibalajiarun Aug 31, 2023

zekun000 Aug 30, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023


		async fn send_epoch_change(&self, proof: EpochChangeProof);

		async fn send_commit_proof(&self, ledger_info: LedgerInfoWithSignatures);

[dag] preliminary state sync implementation #9724

[dag] preliminary state sync implementation #9724

Conversation

ibalajiarun commented Aug 22, 2023 • edited Loading

Description

Test Plan

ibalajiarun commented Aug 22, 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

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 Sep 8, 2023

✅ Forge suite compat success on aptos-node-v1.6.2 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617

github-actions bot commented Sep 8, 2023

✅ Forge suite realistic_env_max_load success on 6dd67967a3c9f46769b8f50a4acbb582b7449617

github-actions bot commented Sep 8, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 6dd67967a3c9f46769b8f50a4acbb582b7449617

ibalajiarun commented Aug 22, 2023 •

edited

Loading

ibalajiarun commented Aug 22, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `6dd67967a3c9f46769b8f50a4acbb582b7449617`

✅ Forge suite `realistic_env_max_load` success on `6dd67967a3c9f46769b8f50a4acbb582b7449617`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `6dd67967a3c9f46769b8f50a4acbb582b7449617`