cluster: fix dead deposit amounts validation in unmarshalers - #4636
Merged
Conversation
Validate the parsed deposit amounts instead of the zero-valued named return in the v1.8, v1.9 and v1.10-11 definition unmarshalers. The checks called VerifyDepositAmounts on the empty named return value, so they always passed and definitions with invalid deposit amounts unmarshaled without error. The v1.10-11 unmarshaler now also passes the parsed compounding flag. category: bug ticket: none Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4636 +/- ##
==========================================
+ Coverage 58.22% 58.27% +0.04%
==========================================
Files 247 247
Lines 34082 34082
==========================================
+ Hits 19843 19860 +17
+ Misses 11768 11754 -14
+ Partials 2471 2468 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
KaloyanTanev
approved these changes
Aug 12, 2026
…ts-validation-fix # Conflicts: # cluster/cluster_test.go
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing bug in cluster definition unmarshaling where deposit amount validation was inadvertently performed against the zero-valued named return (def) instead of the parsed JSON struct (defJSON), causing invalid deposit_amounts to be accepted for v1.8, v1.9, and v1.10–v1.11 definitions.
Changes:
- Validate
defJSON.DepositAmounts(and parseddefJSON.Compoundingfor v1.10–v1.11) instead ofdef.DepositAmounts/def.Compoundingduring unmarshaling. - Force
compounding=falsefor v1.8 and v1.9 definitions (since those versions don’t support compounding). - Add a regression test covering invalid/valid deposit amount cases across v1.8–v1.11, including the compounding-specific upper bound.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
cluster/definition.go |
Fixes dead deposit amount validation by checking parsed JSON fields (and correct compounding behavior by version). |
cluster/cluster_test.go |
Adds regression tests ensuring invalid deposit amounts fail unmarshaling and compounding rules are enforced. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pinebit
enabled auto-merge (squash)
August 12, 2026 11:36
KaloyanTanev
added a commit
that referenced
this pull request
Aug 19, 2026
* app/log: fix slog handler panic on named types (#4640) * app/log: fix slog handler panic on named types Stringify all slog values via fmt.Sprint instead of using zapcore.ReflectType, which panics in the logfmt encoder on named types like protocol.ID. Add a recover guard in Handle so future encoding panics drop the log line instead of crashing the process. * app/log: log slog handler panics through charon logger Route the recover output through Error() instead of raw stderr so it appears in Loki and structured log output. Also fix test comment accuracy and add bool assertion. * app/log: add nested recover for slog panic logging Wrap the Error() call in the recover handler with its own defer/recover so that if the structured logger itself panics we fall back to stderr instead of crashing. * core/validatorapi: preserve sync selections response order (#4641) * core/validatorapi: preserve SyncCommitteeSelections response order Build the response by iterating the original request slice instead of the internal Go map, so response[i] corresponds to request[i]. Prysm matches aggregated selection proofs to requests by array index; random map iteration attached proofs to wrong subcommittees, causing "signature not verified" 500s on submit_contribution_and_proofs. * core/validatorapi: clone ValidatorSetA in ordering test Avoid mutating shared package-level map state. * dkg: validate cluster definition threshold (#4634) * dkg: validate cluster definition threshold Reject cluster definitions with a threshold below 2 or above the number of operators, and log a warning when the threshold differs from the recommended ceil(2n/3) value. Previously charon dkg ran the ceremony silently with any threshold, unlike charon create dkg which validates and warns. category: bug ticket: none Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * app/log: make ForT log initializers safe and restorable Wrap the test write syncer with zapcore.Lock and restore the previous global logger on test cleanup. Previously Init*ForT replaced the global logger permanently, so tests running afterwards in the same package wrote to the test buffer, racing on unsynchronized writers when logging concurrently (caught by CI in dkg TestFrostDKG after TestCheckThreshold). * cluster: dedup definition peers by peer ID (#4635) Reject cluster definitions containing operators whose ENRs encode the same public key. Peers previously deduplicated operators by ENR string only, so distinct ENRs sharing a key (and thus a peer ID) passed verification and collapsed the peer index map built during DKG setup, causing an index out-of-range panic in newFrostP2P. category: bug ticket: none * cluster: fix dead deposit amounts validation in unmarshalers (#4636) Validate the parsed deposit amounts instead of the zero-valued named return in the v1.8, v1.9 and v1.10-11 definition unmarshalers. The checks called VerifyDepositAmounts on the empty named return value, so they always passed and definitions with invalid deposit amounts unmarshaled without error. The v1.10-11 unmarshaler now also passes the parsed compounding flag. category: bug ticket: none * dkg/bcast: bind broadcast signatures to cluster session (#4638) * dkg/bcast: bind broadcast signatures to cluster session Bind reliable-broadcast signatures to the cluster session and message ID. Previously the signed hash covered only the protobuf type URL and value, so signatures remained valid across DKG sessions and message IDs, allowing replay of captured messages into other ceremonies. * dkg/bcast: propagate hash write errors * core/priority: gate duties received from peers (#4643) The priority protocol handler used the duty slot straight off the wire. A cluster peer could retain a deadliner entry and a request buffer per distinct slot, neither of which is released until the (attacker chosen) deadline expires. Gate received duties with core.DutyGaterFunc before allocating any per-duty state, as parsigex and the consensus components already do. Duties initiated locally stay ungated, they come from the scheduler. category: bug ticket: none Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * p2p: bound relay address query responses (#4642) Relay address resolution read the HTTP response body with io.ReadAll and no size limit, using a zero-value http.Client with no timeout, in a loop that runs for the process lifetime. A malicious or compromised configured relay could stream an endless response and grow the heap until the node was OOM killed. Limit the response to 64KB, which is well above a valid ENR string or multiaddr array, set a 10s per-attempt client timeout, and close the response body on the non-2xx retry path where it was leaked. category: bug ticket: none * p2p: remove noisy QUIC happy-path debug logs (#4645) * build(deps): Bump google.golang.org/protobuf from 1.36.11 to 1.36.12 in the go-dependencies group (#4644) * build(deps): Bump google.golang.org/protobuf Bumps the go-dependencies group with 1 update: google.golang.org/protobuf. Updates `google.golang.org/protobuf` from 1.36.11 to 1.36.12 --- updated-dependencies: - dependency-name: google.golang.org/protobuf dependency-version: 1.36.12 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> * *: regenerate protobuf files for v1.36.12 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: kalo <24719519+KaloyanTanev@users.noreply.github.com> * dkg: improved reshare logging (#4650) * dkg: improved reshare logging * Logging progression for normal DKG as well * build(deps): Bump the go-dependencies group across 1 directory with 5 updates (#4648) Bumps the go-dependencies group with 4 updates in the / directory: [github.com/stretchr/testify](https://github.com/stretchr/testify), [golang.org/x/crypto](https://github.com/golang/crypto), [golang.org/x/net](https://github.com/golang/net) and [golang.org/x/tools](https://github.com/golang/tools). Updates `github.com/stretchr/testify` from 1.11.1 to 1.12.0 - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.11.1...v1.12.0) Updates `golang.org/x/crypto` from 0.54.0 to 0.55.0 - [Commits](golang/crypto@v0.54.0...v0.55.0) Updates `golang.org/x/net` from 0.57.0 to 0.58.0 - [Commits](golang/net@v0.57.0...v0.58.0) Updates `golang.org/x/text` from 0.40.0 to 0.41.0 - [Release notes](https://github.com/golang/text/releases) - [Commits](golang/text@v0.40.0...v0.41.0) Updates `golang.org/x/tools` from 0.48.0 to 0.49.0 - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.48.0...v0.49.0) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-version: 1.12.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: golang.org/x/crypto dependency-version: 0.55.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: golang.org/x/net dependency-version: 0.58.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: golang.org/x/text dependency-version: 0.41.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: golang.org/x/tools dependency-version: 0.49.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Bump the docker-dependencies group across 2 directories with 1 update (#4649) Bumps the docker-dependencies group with 1 update in the / directory: golang. Bumps the docker-dependencies group with 1 update in the /testutil/promrated directory: golang. Updates `golang` from 1.26.5-trixie to 1.26.6-trixie Updates `golang` from 1.26.5-trixie to 1.26.6-trixie Updates `golang` from 1.26.5-alpine to 1.26.6-alpine Updates `golang` from 1.26.5-alpine to 1.26.6-alpine --- updated-dependencies: - dependency-name: golang dependency-version: 1.26.6-trixie dependency-type: direct:production update-type: version-update:semver-patch dependency-group: docker-dependencies - dependency-name: golang dependency-version: 1.26.6-trixie dependency-type: direct:production update-type: version-update:semver-patch dependency-group: docker-dependencies - dependency-name: golang dependency-version: 1.26.6-alpine dependency-type: direct:production update-type: version-update:semver-patch dependency-group: docker-dependencies - dependency-name: golang dependency-version: 1.26.6-alpine dependency-type: direct:production update-type: version-update:semver-patch dependency-group: docker-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Andrei Smirnov <andrei@obol.tech> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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
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.
Validate the parsed deposit amounts instead of the zero-valued named return in the v1.8, v1.9 and v1.10-11 definition unmarshalers.
The checks called
deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding)wheredefis the named return value, still zero at that point — the parsed data lives indefJSON.VerifyDepositAmounts(nil, false)returns nil via its empty-slice early return, so the checks always passed and definitions with invalid deposit amounts (below the 1ETH minimum, above the maximum, or summing to less than 32ETH) unmarshaled without error. The bug dates back to the introduction of partial deposits in v1.8 and was copied into the v1.9 and v1.10-11 unmarshalers.The v1.8 and v1.9 unmarshalers now pass
compounding=falsesince those versions don't support compounding; the v1.10-11 unmarshaler passes the parsed compounding flag so large amounts are only accepted for compounding validators.Note this is defense in depth: the DKG path already re-validates amounts after loading (
dkg/disk.go), but other consumers of definition/lock unmarshaling relied on the dead check.category: bug
ticket: none