Skip to content

fix: finish eth2registration method in cluster/distvalidator#298

Merged
varex83 merged 5 commits intomainfrom
bohdan/distvalidator-eth2registration
Mar 26, 2026
Merged

fix: finish eth2registration method in cluster/distvalidator#298
varex83 merged 5 commits intomainfrom
bohdan/distvalidator-eth2registration

Conversation

@varex83
Copy link
Copy Markdown
Collaborator

@varex83 varex83 commented Mar 25, 2026

Small PR, was done by testing codex with claude skills (I asked what we can do small but needed -> asked to implement it)

@varex83 varex83 force-pushed the bohdan/distvalidator-eth2registration branch from eaeb1c9 to 65143a6 Compare March 25, 2026 16:07
@iamquang95
Copy link
Copy Markdown
Collaborator

@claude

@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

Claude finished @iamquang95's task in 1m 56s —— View job


PR Review

  • Gather context
  • Read changed files
  • Check Go reference
  • Provide review feedback

Summary

This PR implements eth2_registration() in DistValidator, converting the stored BuilderRegistration into a VersionedSignedValidatorRegistration. The implementation is mostly correct but contains one notable correctness issue with the validation logic.


Findings

[High] Length checks on fixed-size arrays are dead code — validation never fires

crates/cluster/src/distvalidator.rs:112-114

if reg.signature.len() != BLS_SIGNATURE_LEN          // always false
    || reg.message.pub_key.len() != BLS_PUBKEY_LEN    // always false
    || reg.message.fee_recipient.len() != ADDRESS_LEN // always false

Looking at crates/cluster/src/registration.rs:

  • signature is phase0::BLSSignature = [u8; 96].len() is always 96
  • pub_key is phase0::BLSPubKey = [u8; 48].len() is always 48
  • fee_recipient is [u8; 20].len() is always 20

In Rust, fixed-size arrays have a compile-time constant length, so these three conditions are always false. The InvalidRegistration error can only ever be triggered by the gas_limit == 0 or timestamp == 0 checks. The length checks should be replaced with zero-value checks (e.g., reg.signature == [0u8; 96]) or call the existing zero_registration() helper — though note zero_registration() has the same bug with .is_empty() on fixed-size arrays (a pre-existing issue not introduced here).

The idiomatic fix would be:

if reg.signature == [0u8; BLS_SIGNATURE_LEN]
    || reg.message.pub_key == [0u8; BLS_PUBKEY_LEN]
    || reg.message.fee_recipient == [0u8; ADDRESS_LEN]
    || reg.message.gas_limit == 0
    || reg.message.timestamp.timestamp() == 0
{
    return Err(DistValidatorError::InvalidRegistration);
}

Fix this →

[Low] No tests added

The method has no corresponding test. A unit test covering at least the happy path and the zero-registration error path would ensure functional correctness and serve as a regression guard. Ideally a test vector derived from the Go reference (Go charon at v1.7.1) should be included per the project's testing guidelines.

[Low] Pre-existing: zero_registration() is also broken

crates/cluster/src/distvalidator.rs:98-104

The existing zero_registration() method uses .is_empty() on fixed-size arrays, which always returns false. This means zero_registration() always returns false, and callers relying on it are silently broken. This is not introduced by this PR, but is worth fixing alongside this change.


Parity Matrix

Component Go ref (v1.7.1) Rust Match Notes
Return type eth2api.VersionedSignedValidatorRegistration VersionedSignedValidatorRegistration yes
Zero-registration check validates non-empty fields length checks (always pass) no Fixed-size array lengths are constant
Timestamp conversion int64 → direct i64.try_into::<u64>() yes Negative timestamps map to InvalidRegistration
Field mapping direct copy direct copy yes

@iamquang95
Copy link
Copy Markdown
Collaborator

The finding is valid, but the zero_registration has bug also please fix the zero_registration as well, it's call is_empty on a fixed size array

Copy link
Copy Markdown
Collaborator

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

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

Agree with the comments from @iamquang95.

Comment thread crates/cluster/src/distvalidator.rs Outdated
@varex83 varex83 merged commit 1c8bbc3 into main Mar 26, 2026
8 checks passed
@varex83 varex83 deleted the bohdan/distvalidator-eth2registration branch March 26, 2026 14:49
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.

3 participants