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.

Sign up for GitHub

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

Jump to bottom

Verify consensus message author matches with the sender #15386

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Verify consensus message author matches with the sender #15386

wants to merge 5 commits into from
+70 −34

Conversation

vusirikala
Copy link
Contributor

@vusirikala vusirikala commented Nov 24, 2024 •
edited
Loading

Description

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

@trunk-io Trunk.io
Copy link

trunk-io bot commented Nov 24, 2024 •
edited
Loading

⏱️ 3h 27m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 20m 🟩 🟥 🟥 🟥 
execution-performance / test-target-determinator 17m 🟩 🟩 🟩 🟩 
test-target-determinator 17m 🟩 🟩 🟩 🟩 
forge-compat-test / forge 16m 🟩 
forge-e2e-test / forge 14m 🟩 
check 14m 🟩 🟩 🟩 🟩 
rust-move-tests 13m 🟩 
rust-move-tests 12m 🟩 
rust-move-tests 12m 🟩 
rust-move-tests 12m 🟩 
rust-cargo-deny 10m 🟩 🟩 🟩 ⬜ 🟩 (+2 more)
check-dynamic-deps 8m 🟩 🟩 🟩 🟩 🟩 (+2 more)
fetch-last-released-docker-image-tag 6m 🟩 🟩 🟩 🟩 
rust-doc-tests 5m 🟩 
rust-doc-tests 5m 🟩 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@vusirikala vusirikala requested review from manudhundi and danielxiangzl and removed request for sasha8 November 24, 2024 23:50
@vusirikala vusirikala enabled auto-merge (squash) November 24, 2024 23:51
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@vusirikala vusirikala changed the title Verify consensus message author matches with the sender [Draft] Verify consensus message author matches with the sender Nov 25, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
manudhundi
manudhundi requested changes Nov 25, 2024
View reviewed changes
consensus/src/epoch_manager.rs Outdated
@@ -1512,6 +1513,46 @@ impl<P: OnChainConfigProvider> EpochManager<P> {
Ok(())
}

fn check_author(&mut self, peer_id: AccountAddress, msg: &ConsensusMsg) -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this a bug fix, how about a test that verifies the bug is actually fixed ?

zekun000
zekun000 reviewed Nov 25, 2024
View reviewed changes
consensus/src/epoch_manager.rs Outdated Show resolved Hide resolved
consensus/src/epoch_manager.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
ibalajiarun
ibalajiarun reviewed Nov 25, 2024
View reviewed changes
consensus/src/epoch_manager.rs Outdated
@@ -1446,6 +1446,7 @@ impl<P: OnChainConfigProvider> EpochManager<P> {
BlockStage::EPOCH_MANAGER_RECEIVED,
);
}
self.check_author(peer_id, &consensus_msg)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this should be part of the UnverifiedEvent.verify check to keep them all in one place @zekun000 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the sender verification checks to individual verify functions.

zekun000
zekun000 reviewed Nov 25, 2024
View reviewed changes
consensus/src/epoch_manager.rs Outdated
ConsensusMsg::CommitVoteMsg(commit_vote) => Some(commit_vote.author()),
ConsensusMsg::BatchMsg(batch) => batch.author(),
ConsensusMsg::RoundTimeoutMsg(round_timeout) => Some(round_timeout.author()),
ConsensusMsg::BatchResponse(batch_response) => Some(batch_response.author()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this works too, even this doesn't go through this path (it goes through rpc), this is the batch author which is not necessarily the responser.

consensus/src/epoch_manager.rs Outdated
ConsensusMsg::RoundTimeoutMsg(round_timeout) => Some(round_timeout.author()),
ConsensusMsg::BatchResponse(batch_response) => Some(batch_response.author()),
ConsensusMsg::BatchRequestMsg(batch_request) => Some(batch_request.source()),
ConsensusMsg::SignedBatchInfo(sign_batch_info) => sign_batch_info.author(),
Copy link
Contributor

Choose a reason for hiding this comment

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

it also feels awkward to only check the first author, probably better to have this check to individual verify function instead of here

Copy link
Contributor Author

@vusirikala vusirikala Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. The verify function already checks the author here for each individual signed batch info. So, I originally ignored the author check here by setting author to None. Should I revert it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean instead of having this giant check_author function, check the author inside each individual message's verify function or the UnverifiedEvent::verify function as Balaji mentioned above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the sender verification checks to individual verify functions.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@vusirikala vusirikala enabled auto-merge (squash) November 26, 2024 00:13
@vusirikala vusirikala changed the title [Draft] Verify consensus message author matches with the sender Verify consensus message author matches with the sender Nov 26, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733

Compatibility test results for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733 (PR)
Upgrade the nodes to version: 042a2818382675f19f69e68cec14924f0fd25733
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1329.30 txn/s, submitted: 1333.59 txn/s, failed submission: 4.29 txn/s, expired: 4.29 txn/s, latency: 2215.68 ms, (p50: 1800 ms, p70: 2200, p90: 3600 ms, p99: 4800 ms), latency samples: 117800
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1348.62 txn/s, submitted: 1352.18 txn/s, failed submission: 3.56 txn/s, expired: 3.56 txn/s, latency: 2158.54 ms, (p50: 1800 ms, p70: 2400, p90: 3300 ms, p99: 4800 ms), latency samples: 121120
5. check swarm health
Compatibility test for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733 passed
Upgrade the remaining nodes to version: 042a2818382675f19f69e68cec14924f0fd25733
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1284.41 txn/s, submitted: 1288.18 txn/s, failed submission: 3.77 txn/s, expired: 3.77 txn/s, latency: 2437.07 ms, (p50: 2100 ms, p70: 2400, p90: 4700 ms, p99: 6600 ms), latency samples: 115980
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733

Compatibility test results for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733 (PR)
1. Check liveness of validators at old version: f436adbe4384d6c5fd296addbb7f52d4be77231b
compatibility::simple-validator-upgrade::liveness-check : committed: 14722.12 txn/s, latency: 2326.49 ms, (p50: 2100 ms, p70: 2100, p90: 2300 ms, p99: 6900 ms), latency samples: 483800
2. Upgrading first Validator to new version: 042a2818382675f19f69e68cec14924f0fd25733
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7487.08 txn/s, latency: 3652.53 ms, (p50: 3900 ms, p70: 4300, p90: 5200 ms, p99: 5400 ms), latency samples: 134320
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7796.03 txn/s, latency: 4109.89 ms, (p50: 4200 ms, p70: 4400, p90: 5800 ms, p99: 6400 ms), latency samples: 259900
3. Upgrading rest of first batch to new version: 042a2818382675f19f69e68cec14924f0fd25733
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7380.22 txn/s, latency: 3797.02 ms, (p50: 4100 ms, p70: 4300, p90: 4500 ms, p99: 4600 ms), latency samples: 141420
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7771.86 txn/s, latency: 4117.30 ms, (p50: 4400 ms, p70: 4500, p90: 5700 ms, p99: 5900 ms), latency samples: 258840
4. upgrading second batch to new version: 042a2818382675f19f69e68cec14924f0fd25733
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11086.98 txn/s, latency: 2441.10 ms, (p50: 2400 ms, p70: 2600, p90: 3800 ms, p99: 4000 ms), latency samples: 189920
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5161.76 txn/s, submitted: 5161.91 txn/s, expired: 0.16 txn/s, latency: 2824.50 ms, (p50: 2600 ms, p70: 2700, p90: 5100 ms, p99: 6400 ms), latency samples: 357189
5. check swarm health
Compatibility test for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 042a2818382675f19f69e68cec14924f0fd25733 passed
Test Ok

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 042a2818382675f19f69e68cec14924f0fd25733

two traffics test: inner traffic : committed: 14190.58 txn/s, latency: 2859.52 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 17800 ms), latency samples: 5395560
two traffics test : committed: 99.99 txn/s, latency: 2173.67 ms, (p50: 1500 ms, p70: 2000, p90: 2400 ms, p99: 13500 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.990, avg: 1.304", "ConsensusProposalToOrdered: max: 0.329, avg: 0.294", "ConsensusOrderedToCommit: max: 0.379, avg: 0.366", "ConsensusProposalToCommit: max: 0.669, avg: 0.660"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.90s no progress at version 1559986 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.82s no progress at version 5831416 (avg 14.83s) [limit 16].
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibalajiarun ibalajiarun ibalajiarun left review comments

@manudhundi manudhundi manudhundi requested changes

@zekun000 zekun000 zekun000 approved these changes

@danielxiangzl danielxiangzl Awaiting requested review from danielxiangzl

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vusirikala @ibalajiarun @manudhundi @zekun000