[dag] node and certified node verification checks #8971

ibalajiarun · 2023-07-05T23:29:53Z

Description

This PR adds self-contained verification checks and validation checks to Node and CertifiedNode handlers.

Test Plan

Unit tests

zekun000 · 2023-07-06T00:55:24Z

consensus/src/dag/types.rs

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

+        if current_round == 0 {


ah I think we need to decide on whether it starts at 0 or 1

sasha8 · 2023-07-06T17:56:34Z

consensus/src/dag/types.rs

@@ -197,8 +239,15 @@ impl Node {

 impl TDAGMessage for Node {
    fn verify(&self, verifier: &ValidatorVerifier) -> anyhow::Result<()> {
+        ensure!(self.digest() == self.calculate_digest(), "invalid digest");


Does it make sense to move this check after we validate it?
To prevent DDoS. If I already signed a node for you in this round I do not need to verify and compute expensive digest.

sasha8 · 2023-07-06T17:57:34Z

consensus/src/dag/types.rs

@@ -281,6 +332,8 @@ impl Deref for CertifiedNode {

 impl TDAGMessage for CertifiedNode {
    fn verify(&self, verifier: &ValidatorVerifier) -> anyhow::Result<()> {
+        ensure!(self.digest() == self.calculate_digest(), "invalid digest");
+


Same comment. If we already have this certified node, why compute digests?

We need digest to lookup the certifiednode.

I think Zekun is changing it. Lookup by metadata.

sasha8

Please add a future TODO regarding my comments.

github-actions · 2023-07-07T14:55:36Z

✅ Forge suite `compat` success on `aptos-node-v1.4.4` ==> `d02611f72303465153e0bc2c96d7c3701d51965c`

Compatibility test results for aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c (PR)
1. Check liveness of validators at old version: aptos-node-v1.4.4
compatibility::simple-validator-upgrade::liveness-check : committed: 7084 txn/s, latency: 4717 ms, (p50: 4800 ms, p90: 6600 ms, p99: 8600 ms), latency samples: 240880
2. Upgrading first Validator to new version: d02611f72303465153e0bc2c96d7c3701d51965c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4214 txn/s, latency: 7535 ms, (p50: 7900 ms, p90: 9100 ms, p99: 9600 ms), latency samples: 164360
3. Upgrading rest of first batch to new version: d02611f72303465153e0bc2c96d7c3701d51965c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3710 txn/s, latency: 8634 ms, (p50: 9400 ms, p90: 11800 ms, p99: 12500 ms), latency samples: 141000
4. upgrading second batch to new version: d02611f72303465153e0bc2c96d7c3701d51965c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6441 txn/s, latency: 5012 ms, (p50: 5200 ms, p90: 7200 ms, p99: 7900 ms), latency samples: 231900
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c passed
Test Ok

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

github-actions · 2023-07-07T14:58:09Z

✅ Forge suite `realistic_env_max_load` success on `d02611f72303465153e0bc2c96d7c3701d51965c`

two traffics test: inner traffic : committed: 5903 txn/s, latency: 6625 ms, (p50: 6300 ms, p90: 8100 ms, p99: 14100 ms), latency samples: 2556320
two traffics test : committed: 100 txn/s, latency: 2530 ms, (p50: 2400 ms, p90: 2900 ms, p99: 7400 ms), latency samples: 1820
Max round gap was 1 [limit 4] at version 1191643. Max no progress secs was 3.994787 [limit 10] at version 1191643.
Test Ok

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

github-actions · 2023-07-07T15:04:06Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.4.4` ==> `d02611f72303465153e0bc2c96d7c3701d51965c`

Compatibility test results for aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c (PR)
Upgrade the nodes to version: d02611f72303465153e0bc2c96d7c3701d51965c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4559 txn/s, latency: 7127 ms, (p50: 7200 ms, p90: 9400 ms, p99: 12800 ms), latency samples: 168700
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c passed
Test Ok

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

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch 5 times, most recently from 3f34e94 to 3b1b268 Compare July 6, 2023 00:10

ibalajiarun requested a review from sasha8 July 6, 2023 00:14

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch from 3b1b268 to b3fe5e0 Compare July 6, 2023 00:17

ibalajiarun marked this pull request as ready for review July 6, 2023 00:19

ibalajiarun requested a review from zekun000 as a code owner July 6, 2023 00:19

ibalajiarun marked this pull request as draft July 6, 2023 00:26

ibalajiarun marked this pull request as ready for review July 6, 2023 00:47

zekun000 approved these changes Jul 6, 2023

View reviewed changes

ibalajiarun mentioned this pull request Jul 6, 2023

[dag] certified node receiver handler #8919

Merged

ibalajiarun force-pushed the balaji/dag-node-handler branch 2 times, most recently from feadb53 to 762ba93 Compare July 6, 2023 04:07

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch 2 times, most recently from b9f6339 to b301c8b Compare July 6, 2023 04:32

ibalajiarun force-pushed the balaji/dag-node-handler branch from 762ba93 to 8f40b25 Compare July 6, 2023 04:38

Base automatically changed from balaji/dag-node-handler to main July 6, 2023 05:16

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch from b301c8b to 7d13095 Compare July 6, 2023 05:22

sasha8 reviewed Jul 6, 2023

View reviewed changes

sasha8 approved these changes Jul 6, 2023

View reviewed changes

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch from 7d13095 to d2a5fde Compare July 6, 2023 23:37

ibalajiarun enabled auto-merge (squash) July 6, 2023 23:37

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch 2 times, most recently from 92f84dd to 544bea6 Compare July 7, 2023 03:34

[dag] node and certified node verification checks

d02611f

ibalajiarun force-pushed the balaji/dag-broadcast-fix branch from 544bea6 to d02611f Compare July 7, 2023 03:46

This comment has been minimized.

# to view

ibalajiarun merged commit e412242 into main Jul 7, 2023

ibalajiarun deleted the balaji/dag-broadcast-fix branch July 7, 2023 15:04

banool pushed a commit that referenced this pull request Jul 19, 2023

[dag] node and certified node verification checks (#8971)

df9a713

gedigi pushed a commit that referenced this pull request Aug 2, 2023

[dag] node and certified node verification checks (#8971)

a9ef69d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dag] node and certified node verification checks #8971

[dag] node and certified node verification checks #8971

ibalajiarun commented Jul 5, 2023 •

edited

Loading

zekun000 Jul 6, 2023

sasha8 Jul 6, 2023

sasha8 Jul 6, 2023

ibalajiarun Jul 6, 2023

sasha8 Jul 6, 2023

sasha8 left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

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

		if current_round == 0 {

[dag] node and certified node verification checks #8971

[dag] node and certified node verification checks #8971

Conversation

ibalajiarun commented Jul 5, 2023 • edited Loading

Description

Test Plan

zekun000 Jul 6, 2023

Choose a reason for hiding this comment

sasha8 Jul 6, 2023

Choose a reason for hiding this comment

sasha8 Jul 6, 2023

Choose a reason for hiding this comment

ibalajiarun Jul 6, 2023

Choose a reason for hiding this comment

sasha8 Jul 6, 2023

Choose a reason for hiding this comment

sasha8 left a 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 Jul 7, 2023

✅ Forge suite compat success on aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c

github-actions bot commented Jul 7, 2023

✅ Forge suite realistic_env_max_load success on d02611f72303465153e0bc2c96d7c3701d51965c

github-actions bot commented Jul 7, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.4.4 ==> d02611f72303465153e0bc2c96d7c3701d51965c

ibalajiarun commented Jul 5, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.4.4` ==> `d02611f72303465153e0bc2c96d7c3701d51965c`

✅ Forge suite `realistic_env_max_load` success on `d02611f72303465153e0bc2c96d7c3701d51965c`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.4.4` ==> `d02611f72303465153e0bc2c96d7c3701d51965c`