Skip to content

feat: consensus hash for handshakes#1129

Merged
antouhou merged 13 commits into
masterfrom
feat-consensus-hash
Feb 10, 2026
Merged

feat: consensus hash for handshakes#1129
antouhou merged 13 commits into
masterfrom
feat-consensus-hash

Conversation

@antouhou

@antouhou antouhou commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Describe the changes
Add a consensus params hash to the handshake request, so we can detect when we're connecting to a node with a different consensus config. This doesn't prevent the connection, but prints a big error message. Also added a helper function to generate raw json payloads for the handshake test, as changing it manually becomes very annoying at some point

Related Issue(s)

Checklist

  • Tests have been added/updated for the changes.
  • Documentation has been updated for the changes (if applicable).
  • The code follows Rust's style guidelines.

Additional Context

Summary by CodeRabbit

  • New Features

    • V2 handshakes include a consensus_config_hash and propagate peer IDs in V2 responses; peers log mismatches to highlight config inconsistencies.
    • ConsensusConfig exposes a deterministic keccak256-style hash used throughout the protocol.
  • Refactor

    • Handshake handling split into V1/V2 flows; V1 payloads are translated for V2 compatibility.
    • Various config and pricing types now derive Hash to enable stable hashing/comparisons.
  • Tests

    • Added tests validating deterministic consensus config hashing and V2 handshake behavior.

@coderabbitai

coderabbitai Bot commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Adds deterministic Keccak-256 hashing for ConsensusConfig and related structs, extends V2 handshake payloads/responses to include consensus_config_hash, propagates and checks that hash across p2p/gossip flows, and updates tests to use the hash and a deterministic signer.

Changes

Cohort / File(s) Summary
Consensus hashing & derives
crates/types/src/config/consensus.rs
Derive Hash on many config structs; add Keccak256Hasher and ConsensusConfig::keccak256_hash(); manual Hash impl for IrysRethConfig; add deterministic-hash tests.
Handshake types & encoding
crates/types/src/version.rs
Add consensus_config_hash: H256 to HandshakeRequestV2; introduce HandshakeResponseV2; keep/rename V1 types and alias HandshakeResponse to V2; update encoding/signing, defaults, and tests.
P2P handshake logic
crates/p2p/src/server.rs, crates/p2p/src/peer_network_service.rs, crates/p2p/src/gossip_client.rs
Populate consensus_config_hash in V2 requests/responses; split V1/V2 handling; on V2 acceptance compare peer hash to local hash and log mismatches; convert V1 responses into V2-compatible internal form.
Gossip handler & service init
crates/p2p/src/gossip_data_handler.rs, crates/p2p/src/gossip_service.rs
Add consensus_config_hash field to GossipDataHandler and pass config.consensus.keccak256_hash() into its construction.
Tests & test utils
crates/chain/tests/multi_node/peer_discovery.rs, crates/chain/tests/utils.rs, crates/p2p/src/tests/util.rs
Use deterministic ECDSA signer in a test; include consensus_config_hash when building V2 handshake requests; inject hash into test GossipDataHandler setups.
Minor derive additions
crates/types/src/hardfork_config.rs, crates/types/src/storage_pricing.rs
Add Hash derive to several hardfork and phantom/storage-pricing types (trait additions only).

Sequence Diagram(s)

(Skipped — changes are payload extension, version branching, and logging; control flow remains simple.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • DanMacDonald
  • JesseTheRobot
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.42% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: consensus hash for handshakes' accurately captures the main change: adding consensus configuration hashing to peer handshake protocols for detecting configuration mismatches.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-consensus-hash

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@antouhou antouhou marked this pull request as ready for review February 10, 2026 00:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/types/src/version.rs (1)

396-405: ⚠️ Potential issue | 🟡 Minor

HandshakeResponse.consensus_config_hash is sent unsigned over plain HTTP — note the trust model.

Unlike HandshakeRequestV2 (which is signed), the response has no signature and travels over unencrypted HTTP. A network-level attacker can intercept and modify consensus_config_hash, causing a false mismatch or suppressing a real one, undermining the feature's purpose.

Also applies to: 407-421

🤖 Fix all issues with AI agents
In `@crates/chain/tests/multi_node/peer_discovery.rs`:
- Around line 162-163: Replace the hardcoded "consensus_config_hash" and its
paired "signature" literal in the raw JSON fixture with a generated handshake
JSON using the project's raw-handshake helper (or a local
RawHandshake/RawHandshakeBuilder) so the hash and signature are computed
together; specifically, change the test in peer_discovery.rs to call the
raw-handshake JSON helper (or invoke the builder and serialize it) to produce
the JSON that includes the computed consensus_config_hash and signature instead
of embedding fixed strings.

In `@crates/p2p/src/peer_network_service.rs`:
- Around line 1021-1028: The consensus-hash mismatch log fires for V1 peers
because we compare hashes even when the peer's negotiated protocol/version
implies no consensus hash; update the check around
inner.state.lock().await.config.consensus.keccak256_hash() vs
accepted_peers.consensus_config_hash to first ensure the negotiated protocol
supports a consensus hash (e.g., check accepted_peers.protocol_version or that
the peer hash and our_hash are non-zero/default) before logging; specifically,
gate the error branch using the negotiated protocol/version field on
accepted_peers (or by verifying our_hash != Default::default() &&
accepted_peers.consensus_config_hash != Default::default()) and only then call
error! with gossip_address, our_hash and accepted_peers.consensus_config_hash.

In `@crates/types/src/config/consensus.rs`:
- Around line 886-909: Add a third unit test in the tests module to ensure two
independently constructed ConsensusConfig instances (created via
ConsensusConfig::testing() or equivalent builder, not cloned) produce identical
keccak256_hash() outputs; this verifies there is no hidden non-determinism or
pointer-based state. Refer to the existing tests and add a test function (e.g.,
test_consensus_hash_independent_instances) that constructs config_a =
ConsensusConfig::testing() and config_b = ConsensusConfig::testing() and asserts
keccak256_hash() equality. Ensure the test uses keccak256_hash() and H256
comparisons consistent with the other tests.
- Around line 427-460: The current Keccak256Hasher + std::hash::Hash approach is
non-canonical and platform/compiler-dependent; replace
ConsensusConfig::keccak256_hash to serialize the config to a deterministic byte
representation (e.g., canonical JSON via serde_json::to_vec with stable key
ordering or an RLP/CBOR canonical encoder) and compute
alloy_primitives::keccak256 over that byte buffer instead of calling
self.hash(&mut hasher); remove or stop using Keccak256Hasher and its finalize()
pattern and ensure the new implementation references
ConsensusConfig::keccak256_hash (and not std::hash::Hash) so all nodes compute a
portable, deterministic digest.

Comment thread crates/chain/tests/multi_node/peer_discovery.rs Outdated
Comment thread crates/p2p/src/peer_network_service.rs Outdated
Comment thread crates/types/src/config/consensus.rs Outdated
Comment thread crates/types/src/config/consensus.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@crates/p2p/src/server.rs`:
- Around line 1166-1176: The code recomputes ConsensusConfig::keccak256_hash()
on every handshake; instead compute and store the hash once (e.g., add a
consensus_config_hash field to the Server or DataHandler during initialization
from the immutable ConsensusConfig) and reference that stored value here rather
than calling keccak256_hash() again. Update the use-site
(the_node_consensus_config_hash) to read the precomputed field (preserve the
same type used by version_request.consensus_config_hash) and remove the runtime
keccak256_hash() call so handshakes use the cached hash.

Comment thread crates/p2p/src/server.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@crates/types/src/config/consensus.rs`:
- Around line 279-293: The consensus hash implementation for IrysRethConfig
currently includes account.private_key, which must be omitted; update the impl
std::hash::Hash for IrysRethConfig::hash method to stop hashing the private_key
field for each (addr, account) in self.alloc—only hash addr, account.nonce,
account.balance, account.code, and account.storage so the private key (a local
secret) is excluded from consensus hashing.

Comment thread crates/types/src/config/consensus.rs Outdated

@JesseTheRobot JesseTheRobot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, other than the private key encoding issue the rabbit raised

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@crates/types/src/config/consensus.rs`:
- Around line 29-35: Update the doc comment that currently references `Hash`
trait stability to instead state that deterministic consensus hashing is
produced by `keccak256_hash()` and depends on the stability of the canonical
JSON serialization used there; remove any implication that `ConsensusConfig`
derives or relies on Rust's `Hash` trait and explicitly mention that nodes must
use matching canonical JSON formats and compatible dependency/serialization
versions (and/or the same `keccak256_hash()` implementation) to achieve
identical consensus hashes.

Comment thread crates/types/src/config/consensus.rs Outdated
@antouhou antouhou merged commit 55b63b9 into master Feb 10, 2026
18 checks passed
@antouhou antouhou deleted the feat-consensus-hash branch February 10, 2026 19:15
@coderabbitai coderabbitai Bot mentioned this pull request Feb 17, 2026
3 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Apr 8, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants