chore: refactor crates audit [skip-line-limit]#1554
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdd workspace crates e3-committee-hash and e3-slashing; move committee-hash and slashing logic into new crates; re-export canonical implementations in consumers; refactor CiphernodeBuilder and networking surface; move contract-deployments codegen to CLI build script; add events CommitmentLink API and slashing actors/extensions; update tests and Dockerfile. ChangesPrimary Refactor & Feature Additions Workspace & manifests
Committee hash crate
Slashing crate (core actors & extensions)
zk-prover actor shims & dkg bundle
Events primitives
CiphernodeBuilder & Networking
Config, CLI, and Build Script
Misc, Consumers, and Packaging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
crates/committee-hash/src/lib.rs (1)
61-90: ⚡ Quick winUse a golden vector here instead of self-referential assertions.
The first test only proves the output is non-zero, and the second rebuilds the expected limbs with the same byte slicing the implementation already uses. A packing or endianness regression could still pass both tests. For a cross-language hash contract, I'd pin one known committee to exact hash/hi/lo hex values from the Solidity side.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/committee-hash/src/lib.rs` around lines 61 - 90, Replace the self-referential tests with golden-value checks: in tests encode_packed_matches_solidity_layout and split_limbs_match_solidity_bytes32_layout, use a fixed, known committee vector (the same addresses used in the Solidity contract) and assert the produced hash and split_committee_hash limbs exactly equal hard-coded hex constants (expected hash, expected_hi, expected_lo) taken from the Solidity implementation; specifically, set nodes to the pinned addresses, compute hash = hash_committee_addresses(&nodes) and assert_eq!(hash.0, <expected_hash_bytes>), then call split_committee_hash(hash) and assert_eq!(limbs.hi.0, <expected_hi_bytes>) and assert_eq!(limbs.lo.0, <expected_lo_bytes>) instead of reconstructing expected bytes from hash. Ensure the hex constants match the Solidity outputs (copy them into the test as byte arrays).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 741-753: The AccusationManagerExtension is being initialized using
resolve_slashing_manager(), which currently falls back to self.chains.first()
and can pick a disabled chain; update the builder so that
create_aggregate_config aggregates config from all configured chains (not just
enabled ones) and change resolve_slashing_manager() to select the slashing
manager from that aggregated config (or otherwise explicitly iterate all
configured chains) instead of using self.chains.first(); additionally make
setup_evm_system() only iterate and process chains that are enabled so EIP‑712
verifyingContract and accusation votes are generated against enabled chains only
(adjust functions: create_aggregate_config, resolve_slashing_manager, and
setup_evm_system, and references where AccusationManagerExtension::create is
called).
- Around line 566-573: The current Forked bus wiring in
CiphernodeBuilder::resolve_bus creates a new local_bus (via
Self::create_local_bus()) and only calls EventBus::pipe(bus, &local_bus), which
forwards events from the provided source bus into the local bus but not the
other way; update the behavior to match intent by either adjusting the
with_forked_bus docstring to state that the fork is one-way (source → local) or
make the wiring bidirectional by adding the reverse pipe call
EventBus::pipe(&local_bus, bus) so events published on local_bus are also
forwarded to the original bus; locate BusMode::Forked handling in resolve_bus
and modify wiring or docs accordingly.
In `@crates/cli/build.rs`:
- Around line 87-103: The current loop quietly skips missing "sepolia" or
contracts lacking address/blockNumber, producing a partial CONTRACT_DEPLOYMENTS;
modify the logic so the build fails fast instead: check that json.as_object()
contains "sepolia" (networks.get("sepolia")) and if absent panic! with a clear
message, and inside the for loop require that contract_data["address"].as_str()
and contract_data["blockNumber"].as_u64() are present (use expect or explicit
panic! referencing contract_name) instead of skipping, so missing keys cause the
build to error rather than emitting a partial contract_info used by
contract_info.push_str.
In `@crates/config/src/program_config.rs`:
- Around line 32-33: The serde deserialization default for the field
risc0_dev_mode (#[serde(default)]) doesn't match the struct's Default
implementation (which sets it to 1), causing inconsistent behavior; fix by
making the serde default explicit and consistent with the struct default—add a
helper fn like fn default_risc0_dev_mode() -> u8 { 1 } and change the field
attribute to #[serde(default = "default_risc0_dev_mode")], and similarly update
any other fields in the same block (lines with the same mismatch) to use
explicit serde default functions that return the values used by the
ProgramConfig Default impl.
In `@crates/events/src/commitment_link.rs`:
- Around line 52-54: The trait currently provides default implementations that
call unimplemented! for check_signals (and similarly for check_consistency),
which creates a runtime panic if implementors forget to override; remove the
default bodies so these methods are abstract in the trait (i.e., declare fn
check_signals(...) -> bool; and fn check_consistency(...) -> bool; without
calling unimplemented!), forcing compile-time implementation by consumers
(referencing the check_signals and check_consistency method signatures in
commitment_link.rs).
In `@crates/slashing/src/accusation_manager_ext.rs`:
- Around line 72-80: The code only calls AccusationManager::setup(...) from the
EnclaveEventData::CommitteeFinalized branch inside on_event, so after node
restart already-finalized E3s never get an AccusationManager registered and
their events are dropped; to fix, ensure hydrate (or startup code) explicitly
registers accusation recipients for existing finalized committees by iterating
finalized E3s and calling AccusationManager::setup(...) for each one (or,
alternatively, change on_event to not early-return when
ctx.get_event_recipient("accusation_manager") is checked globally — perform
per-E3 recipient checks instead); update the logic referencing on_event,
EnclaveEventData::CommitteeFinalized, hydrate, ctx.get_event_recipient and
AccusationManager::setup so restarted nodes re-register AccusationManager
recipients for already-finalized E3s.
In `@crates/slashing/src/accusation_manager.rs`:
- Around line 671-730: After inserting the dedupe key into self.accused_proofs
you must undo that insert on every early-return path so retries are not
permanently suppressed; locate the insert and the variable key along with the
failure branches around sign_accusation_digest(...) and bus.publish(...), and
call self.accused_proofs.remove(&key) immediately before each return on those
error paths (e.g., before returning after a publish error and any other
post-insert early returns) so the dedupe bit is rolled back whenever accusation
creation fails.
- Around line 345-364: The accusation_id function currently only hashes
(chain_id, e3_id, accused, proof_type) which causes different accusations that
differ by data_hash, deadline, or the forwarded C3 payload to collide; update
accusation_id (used with ProofFailureAccusation) to include those unique fields
(e.g., accusation.data_hash, accusation.deadline, and the forwarded C3
payload/field used for C3a/C3b) in the packed ABI encoding so each distinct
accusation produces a distinct keccak256 ID and no valid forwarded proof is lost
when deduplicating pending entries.
- Around line 875-913: The forwarded-payload branch currently only checks
signature and e3_id but does not ensure the forwarded proof actually matches the
accusation; to fix, inside the branch that handles accusation.signed_payload
(after forwarded_valid or as part of its validation), verify
forwarded.payload.proof_type == accusation.proof_type and that
Self::compute_payload_hash(forwarded) == accusation.data_hash (or compare the
computed data_hash variable to accusation.data_hash) before proceeding to create
evidence and dispatch re-verification; reference functions/fields:
accusation.signed_payload, forwarded.recover_address(),
forwarded.payload.proof_type, Self::compute_payload_hash(forwarded),
accusation.proof_type, and accusation.data_hash.
In `@crates/slashing/src/commitment_consistency_checker_ext.rs`:
- Around line 44-72: hydrate currently does not recreate the
CommitmentConsistencyChecker for already-finalized E3s, so after restart the
"commitment_consistency_checker" recipient remains unset; update hydrate(&self,
ctx: &mut E3Context, snapshot: &E3ContextSnapshot) to detect finalized E3(s)
from the snapshot or META (use META_KEY / meta.params_preset), recreate links
via (self.links_factory)(meta.params_preset) and call
CommitmentConsistencyChecker::setup(&self.bus, e3_id, links) for each active
finalized E3, then call
ctx.set_event_recipient("commitment_consistency_checker", Some(addr.into()))
exactly as on_event does so the checker is registered on restart.
---
Nitpick comments:
In `@crates/committee-hash/src/lib.rs`:
- Around line 61-90: Replace the self-referential tests with golden-value
checks: in tests encode_packed_matches_solidity_layout and
split_limbs_match_solidity_bytes32_layout, use a fixed, known committee vector
(the same addresses used in the Solidity contract) and assert the produced hash
and split_committee_hash limbs exactly equal hard-coded hex constants (expected
hash, expected_hi, expected_lo) taken from the Solidity implementation;
specifically, set nodes to the pinned addresses, compute hash =
hash_committee_addresses(&nodes) and assert_eq!(hash.0, <expected_hash_bytes>),
then call split_committee_hash(hash) and assert_eq!(limbs.hi.0,
<expected_hi_bytes>) and assert_eq!(limbs.lo.0, <expected_lo_bytes>) instead of
reconstructing expected bytes from hash. Ensure the hex constants match the
Solidity outputs (copy them into the test as byte arrays).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2100ca5c-bc88-4d83-8e3a-04db1ea25919
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
Cargo.tomlcrates/aggregator/Cargo.tomlcrates/aggregator/src/committee.rscrates/aggregator/src/committee_hash.rscrates/aggregator/src/publickey_aggregator.rscrates/ciphernode-builder/Cargo.tomlcrates/ciphernode-builder/src/ciphernode.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/cli/Cargo.tomlcrates/cli/build.rscrates/cli/src/ciphernode/setup.rscrates/cli/src/config_setup.rscrates/cli/src/main.rscrates/committee-hash/Cargo.tomlcrates/committee-hash/src/lib.rscrates/config/src/app_config.rscrates/config/src/lib.rscrates/config/src/program_config.rscrates/entrypoint/build.rscrates/entrypoint/src/config/mod.rscrates/events/src/bus_handle.rscrates/events/src/commitment_link.rscrates/events/src/lib.rscrates/evm/src/lib.rscrates/slashing/Cargo.tomlcrates/slashing/src/accusation_manager.rscrates/slashing/src/accusation_manager_ext.rscrates/slashing/src/commitment_consistency_checker.rscrates/slashing/src/commitment_consistency_checker_ext.rscrates/slashing/src/lib.rscrates/tests/tests/integration.rscrates/utils/Cargo.tomlcrates/utils/src/committee_hash.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/actors/accusation_manager_ext.rscrates/zk-prover/src/actors/commitment_consistency_checker.rscrates/zk-prover/src/actors/commitment_consistency_checker_ext.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/dkg_attestation_bundle.rscrates/zk-prover/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/evm/src/lib.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/slashing/src/accusation_manager.rs (1)
349-361:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind
accusation_idtodeadlineas well.
deadlineis part of the signed accusation/vote contract, so two accusations with the same(chain_id, e3_id, accused, proof_type, data_hash)but different deadlines are not interchangeable. With the current ID, the first pending accusation suppresses later fresh deadlines and those votes then get rejected as deadline mismatches.Suggested fix
- /// `keccak256(abi.encodePacked(chainId, e3Id, accused, proofType, dataHash))` + /// `keccak256(abi.encodePacked(chainId, e3Id, accused, proofType, dataHash, deadline))` fn accusation_id(accusation: &ProofFailureAccusation) -> [u8; 32] { @@ accusation.accused, U256::from(accusation.proof_type as u8), FixedBytes::from(accusation.data_hash), + U256::from(accusation.deadline), ) .abi_encode_packed();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/slashing/src/accusation_manager.rs` around lines 349 - 361, The accusation_id currently hashes (chainId, e3Id, accused, proofType, dataHash) but omits the signed deadline, causing distinct accusations with different deadlines to collide; update the accusation_id function to include accusation.deadline (convert to the same hashed type, e.g., U256::from(accusation.deadline) or other canonical representation) in the tuple used to compute the keccak256 so the resulting 32-byte ID binds the deadline into the identity of a ProofFailureAccusation.crates/events/src/commitment_link.rs (1)
52-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the consistency hook mandatory.
Returning
falsehere turns a missing implementation into a silent "inconsistent" result, which can cascade into bogus commitment violations instead of failing fast. Prefer makingcheck_signalsrequired and keepingcheck_consistencyas the optional delegating helper.Suggested fix
- /// Default returns `false`. Implementations should override either this - /// method or [`check_consistency`]. - fn check_signals(&self, _source_values: &[FieldValue], _target_public_signals: &[u8]) -> bool { - false - } + /// Implementations must define the proof-only consistency check, or + /// override [`check_consistency`] if party context is required. + fn check_signals(&self, source_values: &[FieldValue], target_public_signals: &[u8]) -> bool;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/events/src/commitment_link.rs` around lines 52 - 56, The trait currently provides a default implementation for check_signals that returns false, which hides missing implementations and causes silent "inconsistent" results; change the trait so check_signals is a required method (remove its default body) and make check_consistency the optional helper that delegates to check_signals by default (i.e., implement check_consistency to call check_signals), updating any implementors that relied on the old default to provide an explicit check_signals implementation; reference the trait methods check_signals and check_consistency in commitment_link.rs and ensure compilation by adding explicit implementations where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/events/src/commitment_link.rs`:
- Around line 52-56: The trait currently provides a default implementation for
check_signals that returns false, which hides missing implementations and causes
silent "inconsistent" results; change the trait so check_signals is a required
method (remove its default body) and make check_consistency the optional helper
that delegates to check_signals by default (i.e., implement check_consistency to
call check_signals), updating any implementors that relied on the old default to
provide an explicit check_signals implementation; reference the trait methods
check_signals and check_consistency in commitment_link.rs and ensure compilation
by adding explicit implementations where needed.
In `@crates/slashing/src/accusation_manager.rs`:
- Around line 349-361: The accusation_id currently hashes (chainId, e3Id,
accused, proofType, dataHash) but omits the signed deadline, causing distinct
accusations with different deadlines to collide; update the accusation_id
function to include accusation.deadline (convert to the same hashed type, e.g.,
U256::from(accusation.deadline) or other canonical representation) in the tuple
used to compute the keccak256 so the resulting 32-byte ID binds the deadline
into the identity of a ProofFailureAccusation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c16d46ed-06d9-4e23-aa97-5625de5fa025
📒 Files selected for processing (3)
crates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/commitment_link.rscrates/slashing/src/accusation_manager.rs
ctrlc03
reviewed
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Chores