feat: consensus hash for handshakes#1129
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 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
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
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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_hashis 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 modifyconsensus_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
JesseTheRobot
left a comment
There was a problem hiding this comment.
LGTM, other than the private key encoding issue the rabbit raised
There was a problem hiding this comment.
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.
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
Additional Context
Summary by CodeRabbit
New Features
Refactor
Tests