You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR ports charon/eth2util/signing/signing.go to Rust, adds message_root() helpers to versioned types, and refactors signeddata.rs to delegate to those helpers. The implementation is generally correct and well-tested.
Issues
1. Performance: Double genesis API call in fetch_domain — extensions.rs:323-342
fetch_domain sequentially calls both fetch_genesis_fork_version() and fetch_genesis_validators_root(), each of which independently calls fetch_genesis_data() → /eth/v1/beacon/genesis. That's two network round-trips to the same endpoint per domain lookup.
// Both of these independently call fetch_genesis_data() → /eth/v1/beacon/genesislet genesis_fork_version = self.fetch_genesis_fork_version().await?;let genesis_validators_root = self.fetch_genesis_validators_root().await?;
Fix: call fetch_genesis_data() once and parse both fields.
2. Minor divergence from Go: zero-signature check ordering — signing.rs:142-146
Go's Verify fetches the domain first, then checks for a zero signature. The Rust version checks for zero signature before the async get_data_root call. This is actually an improvement (avoids an unnecessary network call), but it's a behavioral divergence worth documenting if callers rely on specific error precedence.
// Rust: checks zero sig first (faster, but differs from Go order)if*signature == [0;96]{returnErr(SigningError::ZeroSignature);}let signing_root = get_data_root(client, domain_name, epoch, message_root).await?;
Not a bug — but if exact parity with Go is required, the order should match. Consider adding a comment explaining the intentional divergence.
This is called from deposit/types.rs and registration.rs both within pluto_eth2util, so pub(crate) is correct. However, get_data_root and get_domain are pub — if consumers ever need the low-level primitive (e.g., for deposit/registration use cases where there's no async beacon client), they can't access it. Low priority, but worth considering making it pub.
The test suite covers valid sig, zero sig, and wrong pubkey. There is no test for a valid signature produced over a different message root but with the correct pubkey. This is a meaningful negative case.
// Suggested test: sign message_root_a, then call verify with message_root_b// Should return Err(SigningError::Verification(_))
Functional parity: get_domain, get_data_root, verify, and verify_aggregate_and_proof_selection all match the Go implementations correctly, including the EIP-7044 voluntary-exit Capella pinning and the genesis-domain special-case for DomainApplicationBuilder.
Good refactoring in signeddata.rs: Delegating to message_root() methods on versioned types removes duplicated SSZ hashing logic and is cleaner.
Test vectors: The compute_signing_root_matches_registration_vector and get_data_root_matches_registration_vector tests pin against known Teku test data, providing strong confidence in correctness.
SyncContributionAndProof message root: selection_proof_message_root() correctly hashes SyncAggregatorSelectionData { slot, subcommittee_index } — consistent with the spec and the old inline code it replaces.
Summary
The implementation is correct and well-structured. The main actionable items are: (1) eliminating the redundant genesis API call in fetch_domain, (2) optionally documenting the zero-sig check divergence from Go, and (3) adding a test for the wrong-message-root case.
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
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.
Fix: #168