Skip to content

feat(eth2util): implement signing#340

Open
iamquang95 wants to merge 10 commits intomainfrom
iamquang95/implement-eth2util-signing
Open

feat(eth2util): implement signing#340
iamquang95 wants to merge 10 commits intomainfrom
iamquang95/implement-eth2util-signing

Conversation

@iamquang95
Copy link
Copy Markdown
Collaborator

Fix: #168

@iamquang95
Copy link
Copy Markdown
Collaborator Author

@claude

@claude
Copy link
Copy Markdown

claude bot commented Apr 20, 2026

Claude finished @iamquang95's task in 2m 33s —— View job


Code Review: feat(eth2util): implement signing

  • Read changed files and understand PR scope
  • Review Go source for functional equivalence
  • Analyze new signing.rs implementation
  • Review changes to eth2api and other crates
  • Post findings

Overview

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_domainextensions.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/genesis
let 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.

Fix this →


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] {
    return Err(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.


3. Minor: compute_signing_root visibility — signing.rs:95

pub(crate) fn compute_signing_root(message_root: Root, domain: Domain) -> Root {

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.


4. Missing test: verify rejects wrong message root — signing.rs:tests

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(_))

Fix this →


Positive Observations

  • 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.

@iamquang95 iamquang95 marked this pull request as ready for review April 20, 2026 15:30
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.

Implement eth2util/signing

1 participant