Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Clone trait for validator verifier #14847

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

vusirikala
Copy link
Contributor

@vusirikala vusirikala commented Oct 2, 2024

Description

For optimistic signature verification feature, signature verification step adds the list of malicious authors (authors who submitted invalid signatures) to validator verifier. Future signature verification for this author will happen pessimistically. For this to work, we need to make sure the same view of validator verifier is held across the entire system. To ensure this, this PR removes "Clone" trait from ValidatorVerifier. Wherever a validator verifier is to be cloned, we use Arc<ValidatorVerifier> instead.

How Has This Been Tested?

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Oct 2, 2024

⏱️ 9h 46m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 4h 38m
execution-performance / single-node-performance 2h 24m 🟩🟩🟥🟩🟩 (+1 more)
execution-performance / test-target-determinator 24m 🟩🟩🟩🟩🟩
test-target-determinator 18m 🟩🟩🟩🟩
check 15m 🟩🟩🟩🟩
forge-framework-upgrade-test / forge 10m 🟥
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 9m 🟩
rust-cargo-deny 9m 🟩🟩🟩🟩🟩
check-dynamic-deps 8m 🟩🟩🟩🟩🟩 (+1 more)
fetch-last-released-docker-image-tag 6m 🟩🟩🟩🟩
rust-doc-tests 5m 🟩

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

Job Duration vs 7d avg Delta
check-dynamic-deps 2m 1m +105%
execution-performance / test-target-determinator 8m 4m +79%

settingsfeedbackdocs ⋅ learn more about trunk.io

@vusirikala vusirikala requested review from danielxiangzl and removed request for bchocho, lightmark, msmouse, grao1991, JoshLind, sasha8 and gelash October 2, 2024 17:59
@vusirikala vusirikala added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Oct 2, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

// Note: The "Clone" trait has been removed intentionally for ValidatorVerifier to ensure that the same
// view of ValidatorVerifier is used across the system. In can case a ValidatorVerifier needs to be cloned,
// please use Arc<ValidatorVerifier> instead.
#[derive(Debug, Derivative, Serialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove the Arc inside the struct now since the whole struct can only be shared with Arc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -130,7 +129,10 @@ impl TryFrom<ValidatorConsensusInfoMoveStruct> for ValidatorConsensusInfo {
/// Supports validation of signatures for known authors with individual voting powers. This struct
/// can be used for all signature verification operations including block and network signature
/// verification, respectively.
#[derive(Clone, Debug, Derivative, Serialize)]
// Note: The "Clone" trait has been removed intentionally for ValidatorVerifier to ensure that the same
// view of ValidatorVerifier is used across the system. In can case a ValidatorVerifier needs to be cloned,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// view of ValidatorVerifier is used across the system. In can case a ValidatorVerifier needs to be cloned,
// view of ValidatorVerifier is used across the system. In case a ValidatorVerifier needs to be cloned,

This comment has been minimized.

This comment has been minimized.

@vusirikala vusirikala enabled auto-merge (squash) October 3, 2024 02:09

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.

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.

Copy link
Contributor

github-actions bot commented Oct 3, 2024

✅ Forge suite compat success on e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8

Compatibility test results for e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8 (PR)
1. Check liveness of validators at old version: e8a6faf60a98afca8982e0582f929401294bbd33
compatibility::simple-validator-upgrade::liveness-check : committed: 12299.46 txn/s, latency: 2752.93 ms, (p50: 1900 ms, p70: 2200, p90: 5600 ms, p99: 15700 ms), latency samples: 399340
2. Upgrading first Validator to new version: fb0bec49ec5bd0fe07378ea660ba206db3e84bc8
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6605.06 txn/s, latency: 4309.51 ms, (p50: 4900 ms, p70: 5200, p90: 5400 ms, p99: 5600 ms), latency samples: 120260
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6690.21 txn/s, latency: 4854.37 ms, (p50: 5100 ms, p70: 5600, p90: 6400 ms, p99: 6900 ms), latency samples: 228760
3. Upgrading rest of first batch to new version: fb0bec49ec5bd0fe07378ea660ba206db3e84bc8
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7331.88 txn/s, latency: 3815.22 ms, (p50: 4200 ms, p70: 4400, p90: 4800 ms, p99: 4900 ms), latency samples: 134980
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7056.04 txn/s, latency: 4559.21 ms, (p50: 4700 ms, p70: 5000, p90: 6200 ms, p99: 6500 ms), latency samples: 240940
4. upgrading second batch to new version: fb0bec49ec5bd0fe07378ea660ba206db3e84bc8
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 9499.35 txn/s, latency: 2784.49 ms, (p50: 2800 ms, p70: 3400, p90: 4000 ms, p99: 4300 ms), latency samples: 177700
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10104.83 txn/s, latency: 3067.87 ms, (p50: 2800 ms, p70: 3500, p90: 4100 ms, p99: 4400 ms), latency samples: 336520
5. check swarm health
Compatibility test for e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8 passed
Test Ok

Copy link
Contributor

github-actions bot commented Oct 3, 2024

✅ Forge suite realistic_env_max_load success on fb0bec49ec5bd0fe07378ea660ba206db3e84bc8

two traffics test: inner traffic : committed: 12551.10 txn/s, latency: 3179.43 ms, (p50: 2700 ms, p70: 3000, p90: 3900 ms, p99: 9600 ms), latency samples: 4772180
two traffics test : committed: 99.95 txn/s, latency: 3444.75 ms, (p50: 2400 ms, p70: 2800, p90: 6900 ms, p99: 9800 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.242, avg: 0.221", "QsPosToProposal: max: 0.276, avg: 0.192", "ConsensusProposalToOrdered: max: 0.331, avg: 0.300", "ConsensusOrderedToCommit: max: 0.442, avg: 0.415", "ConsensusProposalToCommit: max: 0.747, avg: 0.716"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.17s no progress at version 1943149 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 7.61s no progress at version 1943147 (avg 6.15s) [limit 15].
Test Ok

Copy link
Contributor

github-actions bot commented Oct 3, 2024

✅ Forge suite framework_upgrade success on e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8

Compatibility test results for e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8 (PR)
Upgrade the nodes to version: fb0bec49ec5bd0fe07378ea660ba206db3e84bc8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1218.33 txn/s, submitted: 1221.23 txn/s, failed submission: 2.91 txn/s, expired: 2.91 txn/s, latency: 2557.80 ms, (p50: 2300 ms, p70: 2400, p90: 4200 ms, p99: 6100 ms), latency samples: 108980
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1073.16 txn/s, submitted: 1075.39 txn/s, failed submission: 2.22 txn/s, expired: 2.22 txn/s, latency: 2785.61 ms, (p50: 2600 ms, p70: 3000, p90: 4400 ms, p99: 6200 ms), latency samples: 96500
5. check swarm health
Compatibility test for e8a6faf60a98afca8982e0582f929401294bbd33 ==> fb0bec49ec5bd0fe07378ea660ba206db3e84bc8 passed
Upgrade the remaining nodes to version: fb0bec49ec5bd0fe07378ea660ba206db3e84bc8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1247.94 txn/s, submitted: 1249.49 txn/s, failed submission: 1.55 txn/s, expired: 1.55 txn/s, latency: 2652.01 ms, (p50: 2400 ms, p70: 2700, p90: 4200 ms, p99: 7200 ms), latency samples: 112680
Test Ok

@vusirikala vusirikala merged commit 6938caa into main Oct 3, 2024
49 checks passed
@vusirikala vusirikala deleted the satya/remove_clone_validator_verifier branch October 3, 2024 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants