Skip to content

*: harden feerecipient sign and fetch#4571

Open
pinebit wants to merge 2 commits into
mainfrom
pinebit/feerecipient-fix
Open

*: harden feerecipient sign and fetch#4571
pinebit wants to merge 2 commits into
mainfrom
pinebit/feerecipient-fix

Conversation

@pinebit

@pinebit pinebit commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Hardening feerecipient behavior

Docs PR: ObolNetwork/obol-gitbook#159

Summary

Fetch

  • feerecipient fetch merges newly fetched, quorum-aggregated builder registrations into the existing overrides file instead of replacing it, keeping the entry with the latest timestamp per validator pubkey. Existing entries win ties, and a warning is logged when a fetched registration is discarded as not newer.
  • Merge and verification logic moved into app.MergeBuilderRegistrationOverrides, shared by the CLI fetch and the runtime --fetch-feerecipient-updates paths so their semantics cannot drift. verifyRegistrationSignature stays unexported.
  • The existing overrides file is loaded leniently during fetch: a corrupt or unreadable file is logged and rebuilt, and entries failing signature verification are skipped instead of aborting the whole merge. Fetched registrations failing verification are likewise skipped and logged.
  • The overrides file is written atomically (temp file + rename), so an interrupted fetch cannot leave a truncated file that would block charon run at startup.

Signature verification

  • Builder registration signatures are now always verified. Mainnet's genesis fork version (0x00000000) equals the zero fork version value, so the previous "verify only when the fork version is non-zero" gate silently disabled all verification for mainnet clusters, in LoadBuilderRegistrationOverrides, the background fetch, and the CLI fetch.

Sign

  • The fee recipient address must not be the zero address, and a mixed-case address must match its EIP-55 checksum, preventing typos that would irrecoverably send rewards to the wrong address.
  • A new registration whose timestamp is not later than the current quorum registration on the remote API is rejected, since it would be signed but never applied.
  • When joining an in-progress registration, its timestamp and gas limit are adopted as before, but a warning is now logged when that overrides explicit --timestamp or --gas-limit flags.
  • The default gas limit is resolved from the most recent of the cluster lock, the local overrides file, and the current quorum registration on the remote API, keeping operators with divergent local files signing the same message.

Test plan

  • go test -race ./cmd/... ./app/...
  • pre-commit run --from-ref origin/main --to-ref HEAD — all hooks pass
  • New merge tests: same-pubkey newer/older/equal timestamps, disjoint merge, corrupt file rebuild, invalid existing entry pruning, invalid fetched skip, sorted output
  • New e2e tests: re-fetch updates the same validator on disk and is idempotent, corrupt overrides file self-heals, batch with invalid signature merges the valid one
  • New sign tests: zero address and EIP-55 validation, stale timestamp rejected against quorum, in-progress timestamp adopted despite conflicting flags

category: refactor
ticket: none

@pinebit pinebit changed the title *: merge fetched builder registrations instead of overwriting *: merge fetched builder registrations Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.07692% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.27%. Comparing base (107b026) to head (becd79b).

Files with missing lines Patch % Lines
cmd/feerecipientsign.go 73.77% 12 Missing and 4 partials ⚠️
cmd/feerecipientfetch.go 36.84% 7 Missing and 5 partials ⚠️
app/builderregistration.go 86.00% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4571      +/-   ##
==========================================
+ Coverage   57.22%   57.27%   +0.05%     
==========================================
  Files         245      245              
  Lines       33449    33531      +82     
==========================================
+ Hits        19140    19205      +65     
- Misses      11885    11887       +2     
- Partials     2424     2439      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pinebit pinebit changed the title *: merge fetched builder registrations *: harden feerecipient sign and fetch Jul 2, 2026
pinebit and others added 2 commits July 2, 2026 18:06
Always verify builder registration signatures: mainnet's genesis fork
version equals the zero value, so the previous non-zero gate silently
disabled verification for mainnet clusters.

Move merge logic into app.MergeBuilderRegistrationOverrides shared by
the CLI fetch and runtime paths. Load existing overrides leniently in
fetch (corrupt files are rebuilt, invalid entries skipped) and warn
when a fetched registration is discarded as not newer. Write the
overrides file atomically via temp file and rename.

Reject the zero fee recipient address and mixed-case addresses failing
EIP-55 checksum in sign. Reject new registrations whose timestamp is
not later than the current quorum registration. Resolve the default
gas limit from the most recent of lock, overrides and remote quorum,
and warn when explicit --timestamp/--gas-limit flags are overridden by
adopted in-progress values.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pinebit pinebit force-pushed the pinebit/feerecipient-fix branch from dc499ea to becd79b Compare July 2, 2026 15:10
@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

@pinebit pinebit requested review from KaloyanTanev and OisinKyne July 2, 2026 15:11
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.

2 participants