jwk #4: jwk update quorum certification #11857

zjma · 2024-02-01T15:33:44Z

Description

Test Plan

trunk-io · 2024-02-01T15:33:47Z

⏱️ 39h 4m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-unit-tests	14h 39m	⬜ 🟩 ⬜ ⬜ 🟩 (+12 more)
windows-build	6h 1m	🟩 🟩 🟩 🟩 🟩 (+13 more)
rust-unit-coverage	4h 38m	🟩 🟥
rust-smoke-coverage	3h 28m	🟩 🟥
execution-performance / single-node-performance	2h 37m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-smoke-tests	1h 12m	⬜ 🟩 ⬜ ⬜ 🟩 (+3 more)
rust-lints	1h 9m	⬜ 🟩 ⬜ ⬜ 🟩 (+12 more)
run-tests-main-branch	1h 8m	🟩 🟩 ⬜ 🟩 🟩 (+13 more)
check	52m	⬜ 🟩 ⬜ ⬜ 🟩 (+13 more)
rust-images / rust-all	35m	⬜ 🟩 ⬜ ⬜ 🟩 (+3 more)
general-lints	34m	🟩 🟩 ⬜ 🟩 🟩 (+12 more)
check-dynamic-deps	33m	🟩 🟩 🟩 🟩 🟩 (+13 more)
forge-e2e-test / forge	28m	🟩 🟩
forge-compat-test / forge	27m	🟩 🟩
cli-e2e-tests / run-cli-tests	16m	🟩 🟩
semgrep/ci	6m	🟩 🟩 🟩 🟩 🟩 (+13 more)
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	3m	🟩 🟩
file_change_determinator	3m	🟩 🟩 🟩 🟩 🟩 (+13 more)
file_change_determinator	3m	🟩 🟩 🟩 🟩 🟩 (+13 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
execution-performance / file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	55s	🟩 🟩 🟩 🟩 🟩 (+13 more)
permission-check	49s	🟩 🟩 🟩 🟩 🟩 (+13 more)
permission-check	48s	🟩 🟩 🟩 🟩 🟩 (+13 more)
permission-check	47s	🟩 🟩 🟩 🟩 🟩 (+13 more)
permission-check	24s	🟩 🟩 🟩 🟩 🟩 (+3 more)
determine-docker-build-metadata	18s	🟩 🟩 🟩 🟩 🟩 (+3 more)
upload-to-codecov	16s	🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-lints	12m	8m
windows-build	27m	18m

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

codecov · 2024-02-01T20:52:07Z

Codecov Report

Attention: 379 lines in your changes are missing coverage. Please review.

Comparison is base (793563f) 70.1% compared to head (b019753) 69.9%.
Report is 2 commits behind head on main.

❗ Current head b019753 differs from pull request most recent head dd03efb. Consider uploading reports for the commit dd03efb to get more accurate results

Files	Patch %	Lines
aptos-move/aptos-vm/src/validator_txns/jwk.rs	0.0%	93 Missing ⚠️
crates/aptos-jwk-consensus/src/network.rs	0.0%	86 Missing ⚠️
types/src/jwks/mod.rs	19.6%	41 Missing ⚠️
...tos-jwk-consensus/src/certified_update_producer.rs	0.0%	27 Missing ⚠️
types/src/jwks/jwk/mod.rs	28.5%	20 Missing ⚠️
types/src/jwks/rsa/mod.rs	9.5%	19 Missing ⚠️
types/src/jwks/unsupported/mod.rs	55.5%	16 Missing ⚠️
...rates/aptos-jwk-consensus/src/network_interface.rs	0.0%	15 Missing ⚠️
crates/aptos-jwk-consensus/src/types.rs	0.0%	14 Missing ⚠️
consensus/safety-rules/src/safety_rules_manager.rs	0.0%	10 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #11857     +/-   ##
=========================================
- Coverage    70.1%    69.9%   -0.3%     
=========================================
  Files        2182     2187      +5     
  Lines      414855   413937    -918     
=========================================
- Hits       291207   289676   -1531     
- Misses     123648   124261    +613

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zekun000 · 2024-02-02T20:00:54Z

crates/aptos-jwk-consensus/src/certified_update_producer.rs

+    ) -> AbortHandle;
+}
+
+pub struct RealCertifiedUpdateProducer {


nit: update the naming convention, trait starts with T, and struct removes Real

zekun000 · 2024-02-02T20:04:48Z

crates/aptos-jwk-consensus/src/certified_update_producer.rs

+/// Once invoked by `JWKConsensusManager` to `start_produce`,
+/// it starts producing a `QuorumCertifiedUpdate` and returns an abort handle.
+/// Once an `QuorumCertifiedUpdate` is available, it is sent back via a channel given earlier.
+pub trait CertifiedUpdateProducer: Send + Sync {


I'd just call it UpdateCertifier

zekun000 · 2024-02-02T20:05:06Z

crates/aptos-jwk-consensus/src/certified_update_producer.rs

+        &self,
+        epoch_state: Arc<EpochState>,
+        payload: ProviderJWKs,
+        qc_update_tx: Option<aptos_channel::Sender<(), QuorumCertifiedUpdate>>,


why this is Option?

doesn't have to, fixed

zekun000 · 2024-02-02T20:05:59Z

crates/aptos-jwk-consensus/src/observation_aggregation/mod.rs

+            "adding peer observation failed with mismatched view"
+        );
+
+        // Verify the quorum-cert.


this is to verify the signature not the quorum cert?

i thought they are the same thing..

quorum cert is a 2f+1 aggregate signature, here's a single signature?

ah that's true.

zekun000 · 2024-02-02T20:07:04Z

crates/aptos-jwk-consensus/src/observation_aggregation/mod.rs

+
+        // All checks passed. Aggregating.
+        aggregator.contributors.insert(sender);
+        let new_multi_sig = if let Some(existing) = aggregator.multi_sig.take() {


this looks weird, we have a PartialSignatures for building it

danielxiangzl · 2024-02-02T21:19:43Z

crates/aptos-jwk-consensus/src/observation_aggregation/mod.rs

+    type Message = ObservedUpdateRequest;
+    type Response = ObservedUpdateResponse;
+
+    fn add(


I think the reliable broadcast may panic, if a validator has a different observation than anyone else? If the rb receives all response but does not aggregate it will panic.

For fixes, either rb keeps fetching if the response is different than mine, or allow aggregation to fail and return None. In the later case the validator needs to retry rb to fetch again. Let's discuss what is better.

NVM, I saw you already did the first one.

zekun000 · 2024-02-05T06:14:53Z

crates/aptos-jwk-consensus/src/observation_aggregation/mod.rs

+        {
+            return Ok(None);
+        }
+        let multi_sig = Signature::aggregate(


there's a verifier.aggregate_signature function

github-actions · 2024-02-05T14:22:54Z

✅ Forge suite `realistic_env_max_load` success on `dd03efbe8045e70482cde6c72fc94ada72f1efd7`

two traffics test: inner traffic : committed: 7076 txn/s, latency: 5392 ms, (p50: 4800 ms, p90: 7200 ms, p99: 14400 ms), latency samples: 3064180
two traffics test : committed: 100 txn/s, latency: 2345 ms, (p50: 2100 ms, p90: 2500 ms, p99: 9400 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.276, avg: 0.207", "QsPosToProposal: max: 0.166, avg: 0.145", "ConsensusProposalToOrdered: max: 0.611, avg: 0.548", "ConsensusOrderedToCommit: max: 0.463, avg: 0.442", "ConsensusProposalToCommit: max: 1.023, avg: 0.990"]
Max round gap was 1 [limit 4] at version 1360489. Max no progress secs was 7.546461 [limit 15] at version 1360489.
Test Ok

github-actions · 2024-02-05T14:23:13Z

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

Compatibility test results for aptos-node-v1.8.3 ==> dd03efbe8045e70482cde6c72fc94ada72f1efd7 (PR)
1. Check liveness of validators at old version: aptos-node-v1.8.3
compatibility::simple-validator-upgrade::liveness-check : committed: 4790 txn/s, latency: 6682 ms, (p50: 6500 ms, p90: 9500 ms, p99: 16800 ms), latency samples: 182020
2. Upgrading first Validator to new version: dd03efbe8045e70482cde6c72fc94ada72f1efd7
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1869 txn/s, latency: 15650 ms, (p50: 19500 ms, p90: 22200 ms, p99: 22500 ms), latency samples: 91620
3. Upgrading rest of first batch to new version: dd03efbe8045e70482cde6c72fc94ada72f1efd7
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1770 txn/s, latency: 16563 ms, (p50: 19000 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92080
4. upgrading second batch to new version: dd03efbe8045e70482cde6c72fc94ada72f1efd7
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3167 txn/s, latency: 9842 ms, (p50: 9900 ms, p90: 20200 ms, p99: 20600 ms), latency samples: 126680
5. check swarm health
Compatibility test for aptos-node-v1.8.3 ==> dd03efbe8045e70482cde6c72fc94ada72f1efd7 passed
Test Ok

zjma added 13 commits February 1, 2024 01:48

jwk types update

68e2f24

update

777450d

update

ef680a9

jwk txn and execution

d0957db

consensus ensure jwk txns are expected

3a46733

update

6452075

jwk consensus network type defs

873ab98

update cargo.toml

54c5c9a

update

f7ef043

update

20ccbc6

update

27b7d80

lint

99cc957

jwk update quorum certification

b019753

zjma force-pushed the zjma/jwk_update_qc branch from 1c7a9b9 to b019753 Compare February 1, 2024 15:37

zjma changed the title ~~jwk update quorum certification~~ jwk #4: jwk update quorum certification Feb 1, 2024

zjma added the JWK label Feb 1, 2024

zekun000 reviewed Feb 2, 2024

View reviewed changes

danielxiangzl reviewed Feb 2, 2024

View reviewed changes

danielxiangzl approved these changes Feb 2, 2024

View reviewed changes

zjma added 4 commits February 4, 2024 10:55

update

51f750c

update

bd675c0

update

652b9f4

update

a0d1c56

Base automatically changed from zjma/jwk_network_msg_defs to main February 5, 2024 04:05

Merge remote-tracking branch 'origin/main' into zjma/jwk_update_qc

744ae24

zjma marked this pull request as ready for review February 5, 2024 04:11

zjma added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Feb 5, 2024

heliuchuan approved these changes Feb 5, 2024

View reviewed changes

zjma added 5 commits February 4, 2024 20:19

update

2f5556b

update

9817a00

update

0def8a9

update

21e47cd

update

99de43b

This comment has been minimized.

Sign in to view

zekun000 approved these changes Feb 5, 2024

View reviewed changes

zjma added 2 commits February 5, 2024 05:50

update

b27aabd

update

dd03efb

zjma enabled auto-merge (squash) February 5, 2024 13:56

This comment has been minimized.

Sign in to view

zjma merged commit c2a3453 into main Feb 5, 2024
69 of 83 checks passed

zjma deleted the zjma/jwk_update_qc branch February 5, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jwk #4: jwk update quorum certification #11857

jwk #4: jwk update quorum certification #11857

zjma commented Feb 1, 2024

trunk-io bot commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

zekun000 Feb 2, 2024

zjma Feb 4, 2024

zekun000 Feb 2, 2024

zjma Feb 4, 2024

zekun000 Feb 2, 2024

zjma Feb 4, 2024

zekun000 Feb 2, 2024

zjma Feb 2, 2024

zekun000 Feb 2, 2024

zjma Feb 2, 2024

zjma Feb 4, 2024

zekun000 Feb 2, 2024

zjma Feb 4, 2024

danielxiangzl Feb 2, 2024

danielxiangzl Feb 2, 2024 •

edited

Loading

danielxiangzl Feb 2, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zekun000 Feb 5, 2024

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

jwk #4: jwk update quorum certification #11857

jwk #4: jwk update quorum certification #11857

Conversation

zjma commented Feb 1, 2024

Description

Test Plan

trunk-io bot commented Feb 1, 2024 • edited Loading

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

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

danielxiangzl Feb 2, 2024 • edited Loading

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.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 5, 2024

✅ Forge suite realistic_env_max_load success on dd03efbe8045e70482cde6c72fc94ada72f1efd7

github-actions bot commented Feb 5, 2024

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

trunk-io bot commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

danielxiangzl Feb 2, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `dd03efbe8045e70482cde6c72fc94ada72f1efd7`

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