chore(release): 1.7.0 - #1409
Merged
Merged
Conversation
…1374) Bumps [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) from 9.2.1 to 9.3.0. - [Release notes](https://github.com/golangci/golangci-lint-action/releases) - [Commits](golangci/golangci-lint-action@v9.2.1...v9.3.0) --- updated-dependencies: - dependency-name: golangci/golangci-lint-action dependency-version: 9.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 6.3.0 to 6.5.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v6.3.0...v6.5.0) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 6.5.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.81.1 to 1.82.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.81.1...v1.82.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.82.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Merge tag 'v1.6.0' into v1.6-dev
govulncheck fails on v1.6-dev and every PR branch with GO-2026-5856, a vulnerability in the Go standard library's crypto/tls at go1.26.4. It is reached from dash/core/client.go, rpc/jsonrpc/server/http_server.go, rpc/jsonrpc/client/http_json_client.go and internal/libs/autofile/group.go. It is fixed in go1.26.5, so bump every pin: - go.mod - DOCKER/Dockerfile, test/docker/Dockerfile, test/e2e/docker/Dockerfile - go-version in the build, check-generated, e2e, govulncheck, lint, release and tests workflows - go-version in .github/actions/bls, which runs its own setup-go after the caller's. Leaving it at 1.26.4 put the older toolchain back on the path, so every job that installs BLS failed with "go.mod requires go >= 1.26.5 (running go 1.26.4)". - the versions quoted in README.md, test/fuzz/README.md and the two Go tutorials Verified against golang:1.26.5-alpine3.23: the tree builds and govulncheck reports no vulnerabilities. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Bumps [docker/login-action](https://github.com/docker/login-action) from 4.2.0 to 4.4.0. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@v4.2.0...v4.4.0) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 4.4.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 4.1.0 to 4.2.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@v4.1.0...v4.2.0) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 7.2.0 to 7.3.0. - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](docker/build-push-action@v7.2.0...v7.3.0) --- updated-dependencies: - dependency-name: docker/build-push-action dependency-version: 7.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.82.0 to 1.82.1. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.82.0...v1.82.1) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.82.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.21.0 to 0.22.0. - [Commits](golang/sync@v0.21.0...v0.22.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-version: 0.22.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/adlio/schema](https://github.com/adlio/schema) from 1.5.0 to 1.5.1. - [Release notes](https://github.com/adlio/schema/releases) - [Changelog](https://github.com/adlio/schema/blob/main/CHANGELOG.md) - [Commits](adlio/schema@v1.5.0...v1.5.1) --- updated-dependencies: - dependency-name: github.com/adlio/schema dependency-version: 1.5.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.46.0 to 0.47.0. - [Commits](golang/sys@v0.46.0...v0.47.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-version: 0.47.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
….2 (#1388) Bumps [github.com/creachadair/atomicfile](https://github.com/creachadair/atomicfile) from 0.4.1 to 0.4.2. - [Commits](creachadair/atomicfile@v0.4.1...v0.4.2) --- updated-dependencies: - dependency-name: github.com/creachadair/atomicfile dependency-version: 0.4.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/login-action](https://github.com/docker/login-action) from 4.4.0 to 4.5.2. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@v4.4.0...v4.5.2) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 4.5.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/stale](https://github.com/actions/stale) from 10 to 11. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v10...v11) --- updated-dependencies: - dependency-name: actions/stale dependency-version: '11' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.53.0 to 0.54.0. - [Commits](golang/crypto@v0.53.0...v0.54.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.54.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.56.0 to 0.57.0. - [Commits](golang/net@v0.56.0...v0.57.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.57.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.23.2 to 1.24.1. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/v1.24.1/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.23.2...v1.24.1) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-version: 1.24.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/bufbuild/buf](https://github.com/bufbuild/buf) from 1.71.0 to 1.72.0. - [Release notes](https://github.com/bufbuild/buf/releases) - [Changelog](https://github.com/bufbuild/buf/blob/main/CHANGELOG.md) - [Commits](bufbuild/buf@v1.71.0...v1.72.0) --- updated-dependencies: - dependency-name: github.com/bufbuild/buf dependency-version: 1.72.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 6.5.0 to 7.0.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v6.5.0...v7.0.0) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix(blocksync): don't punish peers for duplicate block responses
removePeer re-requested every block it had already received from the peer
being removed, but left those responses in pendingToApply. When the
re-requested block arrived from a different peer, addBlock rejected it as
a duplicate and the synchronizer reported a PeerError against the peer
that had served it correctly, evicting it.
Under peer churn this cascades: each eviction re-requests more heights,
each of those punishes another healthy peer, and block sync can stall
long enough to hit the 60s syncTimeout and switch to consensus without
being caught up.
Delete the pending entry alongside the re-request so the re-fetched block
is accepted, dedupe pushedBack so a height is not fetched twice
concurrently, and treat a duplicate response as benign rather than as a
peer fault.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* perf(blocksync): remove redundant work from the block apply path
Block sync applies blocks on a single goroutine, so sync rate is bounded
by the cost of applying one block. That path repeated several expensive
operations:
- The part set was built twice per block, once inside BlockID(nil) and
once explicitly for SaveBlock. Each build serializes the whole block
and computes a Merkle tree over its parts. Build it once and pass it to
both; BlockID(partSet) is identical to BlockID(nil) for a part set made
with BlockPartSizeBytes.
- Block.Size() converted the whole block to protobuf twice more, once to
feed the peer flow monitor and once for the block size metric. The size
is now measured once while decoding the response, on a worker
goroutine, and the metric reuses partSet.ByteSize(), which is exactly
the length of the serialized block.
- ApplyBlock validated every block twice: ProcessProposal(verify=true)
and then FinalizeBlock, which calls ValidateBlockWithRoundState with
the same arguments and wraps failures in the same ErrInvalidBlock.
Each pass costs a threshold signature verification of block.LastCommit.
ApplyBlock now passes verify=false; FinalizeBlock validates before any
persistence or ABCI call, so no check is lost.
- The p2p receive path recomputed proto.Size on every message purely to
feed a byte counter, walking the whole decoded message again. len(bz)
is the exact wire size and is already in hand.
Also records per-stage timings of the apply pipeline and reports them
alongside the existing block sync rate log line, so a slow sync can be
attributed to serialization, signature verification, disk or the ABCI
application without a profiler.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(blocksync): address review findings on the apply path work
- FinalizeBlock's new comment claimed the validation runs before the block
reaches the app or the stores. That is false on the block sync path:
ProcessProposal has already delivered the block and blockApplier.Apply
has already written it. Say what the call actually guards, and why it
must not be removed.
- NewBlockMeta still converted the whole block to protobuf for its size,
on the same serial path this change set is trying to shorten. The part
set it already receives holds the serialized block, so its byte size is
exactly the block size.
- The timing log reported whole milliseconds, which truncates part set
building and commit verification - routinely sub-millisecond - to 0
both before and after any improvement, defeating the point of adding
the instrumentation. Log durations instead.
- Renamed the abci_ms key to exec: the span also covers
SaveABCIResponses, the state store write and the mempool update, which
are ours rather than the application's.
- updateMonitor read maxPeerHeight on every applied block for a line it
prints once per interval, and carried a comment about a lock ordering
the refactor had already removed.
Adds the missing coverage: applyStats averaging, reset-on-read, and
sub-millisecond retention, plus a regression test pinning that ApplyBlock
still rejects an invalid block. ApplyBlock calls ProcessProposal with
verify=false and relies solely on FinalizeBlock's validation, a coupling
invisible from either function alone; the test was confirmed to fail when
that validation is removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* perf(types): stop serializing the block when a part set is available
NewBlockMeta computed the fallback eagerly:
blockSize := block.Size()
if blockParts != nil {
blockSize = int(blockParts.ByteSize())
}
Go evaluates the initializer unconditionally, so block.Size() - which is
ToProto() plus a full walk of the result - ran on every call and was then
discarded whenever a part set was supplied. Both production callers
(store.SaveBlock and the block sync apply path through blockApplier.Apply)
always supply one, so the serialization this was meant to remove was still
being paid on every block.
Move the fallback into the branch that needs it. The nil case still reports
the real size rather than 0.
No behaviour change and no test: PartSet.ByteSize() is len(data) of the same
marshalling Block.Size() computes, so both branches produce identical values
and no assertion can tell the old code from the new. The change is only about
not doing the work. Verified by inspection; not compiled locally, as the BLS
native dependency is not built on this machine.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com>
* fix(blocksync): don't punish peers for duplicate block responses removePeer re-requested every block it had already received from the peer being removed, but left those responses in pendingToApply. When the re-requested block arrived from a different peer, addBlock rejected it as a duplicate and the synchronizer reported a PeerError against the peer that had served it correctly, evicting it. Under peer churn this cascades: each eviction re-requests more heights, each of those punishes another healthy peer, and block sync can stall long enough to hit the 60s syncTimeout and switch to consensus without being caught up. Delete the pending entry alongside the re-request so the re-fetched block is accepted, dedupe pushedBack so a height is not fetched twice concurrently, and treat a duplicate response as benign rather than as a peer fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * perf(blocksync): remove redundant work from the block apply path Block sync applies blocks on a single goroutine, so sync rate is bounded by the cost of applying one block. That path repeated several expensive operations: - The part set was built twice per block, once inside BlockID(nil) and once explicitly for SaveBlock. Each build serializes the whole block and computes a Merkle tree over its parts. Build it once and pass it to both; BlockID(partSet) is identical to BlockID(nil) for a part set made with BlockPartSizeBytes. - Block.Size() converted the whole block to protobuf twice more, once to feed the peer flow monitor and once for the block size metric. The size is now measured once while decoding the response, on a worker goroutine, and the metric reuses partSet.ByteSize(), which is exactly the length of the serialized block. - ApplyBlock validated every block twice: ProcessProposal(verify=true) and then FinalizeBlock, which calls ValidateBlockWithRoundState with the same arguments and wraps failures in the same ErrInvalidBlock. Each pass costs a threshold signature verification of block.LastCommit. ApplyBlock now passes verify=false; FinalizeBlock validates before any persistence or ABCI call, so no check is lost. - The p2p receive path recomputed proto.Size on every message purely to feed a byte counter, walking the whole decoded message again. len(bz) is the exact wire size and is already in hand. Also records per-stage timings of the apply pipeline and reports them alongside the existing block sync rate log line, so a slow sync can be attributed to serialization, signature verification, disk or the ABCI application without a profiler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(blocksync): survive transient peer failures and stalls Two ways block sync gives up far too easily. A single failed block request dropped the peer. consumeJobResult called RemovePeer and reported a PeerError for any errBlockFetch, including a plain 15s request timeout, and PeerManager.Errored evicts on the first report. Peers serve block requests one at a time, so a busy peer times out long before it is unhealthy, and dropping it also fails the up to 20 requests already in flight to it - each of which then drops another peer. Count consecutive failures per peer instead and drop only after maxConsecutiveFailures in a row, resetting the count on the first successful response. A failed request also stops being counted as pending, which it previously only did by virtue of the peer being deleted. The p2p client reported the peer itself on every request timeout, which bypassed any caller-side policy. Reject the promise and let the caller decide: blocksync now counts timeouts, statesync's backfill already said it did not want to punish on timeout, and a genuinely dead connection is still caught by the transport's ping/pong. GetBlock is the only production caller of a promise-returning client method, so this is confined to the block sync path. A 60s stall handed over to consensus even when far behind. Nothing switches back to block sync except the state sync path, so a stalled node was demoted permanently to consensus catch-up, which is much slower. A stall is now only a reason to stop when no peer reports a height above ours, or when it has outlasted maxSyncStall, so a wedged synchronizer can still hand over rather than blocking forever. WaitForSync returns whether the node actually caught up, and poolRoutine uses that instead of a second IsCaughtUp call racing the synchronizer it just stopped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(blocksync): address review findings on the apply path work - FinalizeBlock's new comment claimed the validation runs before the block reaches the app or the stores. That is false on the block sync path: ProcessProposal has already delivered the block and blockApplier.Apply has already written it. Say what the call actually guards, and why it must not be removed. - NewBlockMeta still converted the whole block to protobuf for its size, on the same serial path this change set is trying to shorten. The part set it already receives holds the serialized block, so its byte size is exactly the block size. - The timing log reported whole milliseconds, which truncates part set building and commit verification - routinely sub-millisecond - to 0 both before and after any improvement, defeating the point of adding the instrumentation. Log durations instead. - Renamed the abci_ms key to exec: the span also covers SaveABCIResponses, the state store write and the mempool update, which are ours rather than the application's. - updateMonitor read maxPeerHeight on every applied block for a line it prints once per interval, and carried a comment about a lock ordering the refactor had already removed. Adds the missing coverage: applyStats averaging, reset-on-read, and sub-millisecond retention, plus a regression test pinning that ApplyBlock still rejects an invalid block. ApplyBlock calls ProcessProposal with verify=false and relies solely on FinalizeBlock's validation, a coupling invisible from either function alone; the test was confirmed to fail when that validation is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com>
* fix(consensus): prevent remote crash/brick via unknown vote-extension type An unprivileged peer could send a Commit (or precommit) carrying a vote-extension whose proto enum Type is undefined. proto3 enums are open, so gogoproto accepts any varint; VoteExtensionFromProto's default arm then panicked, terminating the consensus goroutine (which is designed to re-panic). Because the message is written to the WAL before it is processed, the node re-read it on restart and crash-looped — a one-packet, network-wide kill of every validator and full node, not recoverable by restart. Fix (defense-in-depth): - VoteExtensionFromProto no longer panics on an unknown type; it returns a non-threshold-recoverable extension so construction cannot crash on any path. - MakeQuorumSigns — the shared chokepoint of VerifyCommit (commits) and VerifyExtensionSign (precommit votes) — rejects unknown extension types, so a message carrying one fails closed instead of being silently excluded from the sign items (which on the precommit path would otherwise still reach the ABCI VerifyVoteExtension call). The rejection is deliberately NOT placed in Commit.ValidateBasic: that runs on the WAL decode path, where a rejection becomes a DataCorruptionError that aborts replay and would re-brick already-attacked nodes. On the verification path the error is swallowed by loggingMiddleware on both live and replay, so poisoned nodes restart cleanly. Tests (red→green verified for each): - types/commit_extension_regression_test.go — construction does not panic; VerifyCommit, MakeQuorumSigns and the precommit VerifyExtensionSign reject the unknown type. The vote-path test fails if the MakeQuorumSigns check is removed, pinning the silent-ignore regression. - internal/consensus/commit_extension_unbrick_test.go — writes a poison commit to a real WAL, confirms it still DECODES (a ValidateBasic rejection would abort replay), and that verifying the replayed commit returns an error without panicking. This is the restart-recovery (un-brick) property. Green across types, privval, light, and internal/consensus vote/commit/WAL/ replay tests. Independently validated: crash removed, un-brick preserved, no false-accept, no legitimate traffic rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(types): return an error for unknown vote-extension types VoteExtensionFromProto returned a fabricated GenericVoteExtension for a type outside the defined enum values. proto3 enums are open, so a peer can put any varint on the wire, and vote extensions feed the sign-hash through MakeQuorumSigns: substituting a default silently changes the digest, so at best verification fails with a misleading error and at worst two nodes derive different canonical votes for the same commit. Nothing on a path feeding a signature may be fabricated. The conversion now returns an error, propagated through VoteExtensions.Add, VoteExtensionsFromProto, NewVoteExtensionsFromABCIExtended and Commit.GetCanonicalVote, and out through the callers that already return an error. A poisoned message is rejected by the existing caller instead of terminating the consensus goroutine. A poison commit still decodes from the WAL — CommitFromProto keeps extensions in wire form and Commit.ValidateBasic is unchanged — and is rejected later at VerifyCommit, so an already-attacked node still restarts. A poison precommit now dies at MsgFromProto -> VoteFromProto in the reactor; it could never reach the WAL, because on every prior version that same conversion panicked there. NewVoteExtensionsFromABCIExtended propagates the error and ExtendVote panics on it: that input is our own application, not a peer, so an undefined type is a local bug and must fail loudly, consistent with the panics ExtendVote already raises on ABCI misbehaviour. Drops the defensive loop MakeQuorumSigns used to carry: both paths it guarded funnel through the same VoteExtensionsFromProto call, which now rejects the type before Filter can silently exclude it. Out of scope by instruction: peer punishment, Commit.ValidateBasic validation, WAL write ordering, and recover()/panic-handling middleware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: remove commit-extension-panic exploit spec The spec documented an unpatched vulnerability (exact packet size, ungated reactor branch, exploit cost) under docs/, a VuePress-built site with no exclude rule for this path — publishable before the fix ships, contradicting the doc's own §8 guidance not to push it early. Flagged HIGH by security review; removing rather than gating, since no exclude mechanism exists in the current VuePress config. * fix(types): add a sentinel error and reject nil vote extensions VoteExtensionsFromProto dereferenced every slice element without a nil check, contradicting the fail-closed contract VoteExtensionFromProto documents: the conversion is reachable from untrusted network input and must neither panic nor fabricate a value. A nil element now yields an error naming its index. The unknown-type rejection gains an exported sentinel, ErrUnknownVoteExtensionType, so callers can match it with errors.Is. A future peer-punishment change needs to distinguish "this peer sent an undefined enum value" from any other conversion failure, which a bare fmt.Errorf cannot express. The message text is unchanged. Add, VoteExtensionsFromProto and NewVoteExtensionsFromABCIExtended became fallible without their godoc saying so; each now documents its error contract. Callers of VoteExtensions.Add are the one signature change the compiler cannot enforce - a stale .Add(ve) call still compiles and silently swallows the rejection - so the error is now part of the documented API surface. Tests assert the specific sentinel via errors.Is rather than "some error". VerifyCommit's case previously passed even with the rejection fully removed, because its fake zero signature failed verification first; it now builds a commit that genuinely verifies, asserts that positive control, and only then flips one extension type to an undefined value. Restores direct coverage of Vote.VerifyExtensionSign, which the peer-path test does not exercise: its vote is built as a struct literal, reaching a path no converter can guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: share one vote-extension helper and drop review IDs from comments Making VoteExtensionsFromProto fallible spread the same three-line convert-and- require dance across two packages and a dozen call sites, as two near-identical private helpers whose signatures had already diverged (testing.TB vs *testing.T). Export MustVoteExtensionsFromProto from types/test_util.go instead, following the existing convention of a non-test file importing testing (internal/state/test/ factory), and use it everywhere a *testing.T is in scope. The two production helpers that convert extensions, types.makeCommit and the state test factory, keep returning an error: they have no t to fail. Also drops ephemeral review-finding IDs from test comments in the two files this change touches. Those IDs are reassigned on every review run and go dead after merge, so each is replaced by the invariant it stood for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(kvstore): return errors from the sign-item helpers makeVoteExtensionSignItems panicked on the newly fail-closed conversion of peer-forwarded wire data, and makeBlockSignItem panicked on its own marshalling failures. STYLE_GUIDE.md reserves panic for a broken internal invariant; neither qualifies, since both derive from a FinalizeBlock request. Both now return an error, propagated by FinalizeBlock the same way as its other failure paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover the light-client and ExtendVote extension-type paths The unknown-type rejection has four propagation destinations; the light client and the ABCI-facing ExtendVote had no test. verifyBlockSignatureWithDashCore builds a canonical vote from a provider-supplied commit, so a malicious provider could otherwise panic the light client; its Dash Core client is left nil in the test because the rejection must happen before any RPC round-trip. ExtendVote's panic is asserted rather than removed. That response comes from our own application, so a type outside the enum is a local app/tenderdash mismatch, not a network fault, and failing loudly is the intended behaviour there. The test pins that contract so it cannot be softened by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct two comments on the vote-extension paths MakeQuorumSigns claimed a causal dependency that does not hold for the traced input; the real reason to convert first is that MakeBlockSignItem panics rather than returning an error, so fallible work belongs ahead of it. Marks the ExtendVote panic as a reviewed, deliberate choice: ABCI hardening is out of scope here, and panicking on an ABCI-supplied undefined type is the intended strongest signal, not an oversight to be fixed later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(types): drop the unused Timeout.Commit and Timeout.BypassCommitTimeout ValidateConsensusParams wrote two deprecation warnings straight to stderr, both guarded by fields outside HashedParams, so a remote statesync witness controlled whether an unstructured stderr write fired. Both fields were marked "Unused, TODO: Remove in 0.15" and the proto already reserves commit and bypass_commit_timeout, so nothing reads or writes them: ToProto never sets them, ConsensusParamsFromProto never reads them, and the two warnings were their only consumers. Removed along with the warnings, at v1.6. Genesis files still carrying the keys keep parsing: consensus params are decoded with encoding/json, which ignores unknown fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(config): drop the unsafe commit-timeout override knobs The Timeout.Commit and Timeout.BypassCommitTimeout consensus parameters are gone, but the config surface still offered unsafe-commit-timeout-override and unsafe-bypass-commit-timeout-override: struct fields, validation, TOML template lines, and documentation for values nothing reads. All removed. The config template test now guards against offering overrides for parameters that no longer exist. Existing config files carrying the keys keep loading, since unknown TOML keys are ignored during unmarshalling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(config): report removed commit-timeout overrides instead of ignoring them Addresses review findings on the parent commit, which dropped the unsafe-commit-timeout-override and unsafe-bypass-commit-timeout-override knobs and the TimeoutParams.Commit / BypassCommitTimeout fields. Removing the keys outright left an operator who had set them with no signal at all, since unknown keys decode silently. Both keys are now decoded into deprecated fields and reported at startup, following the existing DeprecatedQuorumType idiom. Each entry says where the setting went, because "removed" alone sends an operator hunting for a replacement that does not exist. The startup message moves from Info to Warn so a node running at a raised log level still sees it. Documentation that described settings which no longer exist is corrected: the spec no longer lists commit / bypass_commit_timeout as genesis fields, four override bullets named keys that never existed, the block spacing advice pointed at timeout-commit rather than create-empty-blocks-interval, the production guide recommended two nonexistent tunables, the Docker entrypoint rewrote a key that is never present, and the claim that these overrides disappear in Tendermint v0.37 is untrue for this fork. Tests would have caught the gaps in CI: - config: operator sets a removed override -> no warning (FAIL before, PASS after) - types: genesis carrying only removed timeout keys is rejected with "timeout.ProposeDelta must be greater than 0" (FAIL before, PASS after) The config test decodes through viper rather than assigning fields, so the mapstructure keys themselves are covered; an explicitly configured false bypass is exercised through the same path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com>
* fix(consensus): prevent remote crash/brick via unknown vote-extension type An unprivileged peer could send a Commit (or precommit) carrying a vote-extension whose proto enum Type is undefined. proto3 enums are open, so gogoproto accepts any varint; VoteExtensionFromProto's default arm then panicked, terminating the consensus goroutine (which is designed to re-panic). Because the message is written to the WAL before it is processed, the node re-read it on restart and crash-looped — a one-packet, network-wide kill of every validator and full node, not recoverable by restart. Fix (defense-in-depth): - VoteExtensionFromProto no longer panics on an unknown type; it returns a non-threshold-recoverable extension so construction cannot crash on any path. - MakeQuorumSigns — the shared chokepoint of VerifyCommit (commits) and VerifyExtensionSign (precommit votes) — rejects unknown extension types, so a message carrying one fails closed instead of being silently excluded from the sign items (which on the precommit path would otherwise still reach the ABCI VerifyVoteExtension call). The rejection is deliberately NOT placed in Commit.ValidateBasic: that runs on the WAL decode path, where a rejection becomes a DataCorruptionError that aborts replay and would re-brick already-attacked nodes. On the verification path the error is swallowed by loggingMiddleware on both live and replay, so poisoned nodes restart cleanly. Tests (red→green verified for each): - types/commit_extension_regression_test.go — construction does not panic; VerifyCommit, MakeQuorumSigns and the precommit VerifyExtensionSign reject the unknown type. The vote-path test fails if the MakeQuorumSigns check is removed, pinning the silent-ignore regression. - internal/consensus/commit_extension_unbrick_test.go — writes a poison commit to a real WAL, confirms it still DECODES (a ValidateBasic rejection would abort replay), and that verifying the replayed commit returns an error without panicking. This is the restart-recovery (un-brick) property. Green across types, privval, light, and internal/consensus vote/commit/WAL/ replay tests. Independently validated: crash removed, un-brick preserved, no false-accept, no legitimate traffic rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(types): return an error for unknown vote-extension types VoteExtensionFromProto returned a fabricated GenericVoteExtension for a type outside the defined enum values. proto3 enums are open, so a peer can put any varint on the wire, and vote extensions feed the sign-hash through MakeQuorumSigns: substituting a default silently changes the digest, so at best verification fails with a misleading error and at worst two nodes derive different canonical votes for the same commit. Nothing on a path feeding a signature may be fabricated. The conversion now returns an error, propagated through VoteExtensions.Add, VoteExtensionsFromProto, NewVoteExtensionsFromABCIExtended and Commit.GetCanonicalVote, and out through the callers that already return an error. A poisoned message is rejected by the existing caller instead of terminating the consensus goroutine. A poison commit still decodes from the WAL — CommitFromProto keeps extensions in wire form and Commit.ValidateBasic is unchanged — and is rejected later at VerifyCommit, so an already-attacked node still restarts. A poison precommit now dies at MsgFromProto -> VoteFromProto in the reactor; it could never reach the WAL, because on every prior version that same conversion panicked there. NewVoteExtensionsFromABCIExtended propagates the error and ExtendVote panics on it: that input is our own application, not a peer, so an undefined type is a local bug and must fail loudly, consistent with the panics ExtendVote already raises on ABCI misbehaviour. Drops the defensive loop MakeQuorumSigns used to carry: both paths it guarded funnel through the same VoteExtensionsFromProto call, which now rejects the type before Filter can silently exclude it. Out of scope by instruction: peer punishment, Commit.ValidateBasic validation, WAL write ordering, and recover()/panic-handling middleware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): bound inbound peer-supplied values Seven ingress-only bounds on data a remote peer controls. Each rejects with an error at the point the value enters; none punish, score or evict the peer. - types: ValidatorSetFromProto rejects a QuorumType outside uint8. It arrives on the statesync LightBlock path, is not covered by ValidatorSet.Hash() (which hashes only ThresholdPublicKey and QuorumHash), and reaches tmmath.MustConvertUint8 in both sign-hash construction and the SignItem zerolog marshaller, so merely logging a tampered set kills the node. Mirrors the SafeConvertInt32 guard ToProto already applies to the outbound value. - statesync: validate peer-supplied ConsensusParams before Bootstrap persists them. ConsensusParamsFromProto validates nothing and the existing hash check covers only Block.MaxBytes, Block.MaxGas and Version.ConsensusVersion. - p2p/conn: cap the two SecretConnection handshake reads at 128 and 1024 bytes, down from 1 MiB each. Both are read before any authentication exists and protoio allocates the declared length eagerly. - consensus: reject a negative Round in VoteSetBitsMessage.ValidateBasic, the only message type missing the check. A negative round matches PeerState's freshly-initialised sentinel rounds. - p2p: cap Envelope.Attributes at 8; honest senders set at most two and the map is retained for the envelope's whole queue lifetime. - p2p: cap stored addresses per peer at 32. PEX lets a peer choose how many addresses it advertises for its own NodeID, prunePeers bounds only the peer count, and every insert rewrites the whole peerInfo to the database. Capped on insert only, so stores already holding more still load. - types: NodeInfo.Validate no longer resolves ListenAddr. It runs on an unauthenticated peer's value after conn.Handshake returns, so HandshakeTimeout does not cover the lookup. ParseAddressString keeps resolving for the startup path; the shared parsing and NodeID validation move to splitAddressString. Also fixes ParseAddressString returning (nil, nil) for an empty host, which made NewEndpoint nil-deref at boot on listen-address "tcp://:26656", and guards an unchecked ips[0]. The QuorumType bound also runs on the state-store read path (dbStore.Load and LoadValidators), which returns errors rather than panicking. That trade is deliberate: a node holding an out-of-range value already dies in MustConvertUint8, so this replaces a later panic with a clean startup error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): evict least valuable address instead of refusing new ones The per-peer address bound had no eviction, so once a NodeID reached it every later address for that peer was silently refused forever — ordinary peer churn alone was enough to freeze a peer's address set with stale entries. Reaching the bound now drops the least valuable stored address (oldest successful dial, then most dial failures, address string breaking ties so eviction does not depend on map iteration order) and stores the new one, mirroring how prunePeers handles the peer-count analogue. The address in use is never the candidate, since Dialed stamps it with the newest LastDialSuccess. The eviction is logged at Debug. The bound moves from an exported const to PeerManagerOptions.MaxAddressesPerPeer, matching every other bound on the struct. Unlike its siblings, 0 selects the default rather than disabling the bound: every constructor outside node setup leaves it unset, and a security bound must not have a zero-value silent disable. routerDashDialer.ConnectAsync no longer discards Add's bool: it reported success for an address that was never stored. Eviction removes the at-capacity case, but an inactive peer still refuses the insert, so that case is logged at Debug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): bound envelope attribute bytes and stop shouting about rejections The attribute cap bounded the entry count but not the retained bytes, so a peer could still attach eight entries whose keys and values are arbitrarily long. The summed key+value length is now bounded too, with the cheaper count check first. Both rejections get a sentinel error wrapped with the offending counts, so callers and tests can classify them with errors.Is instead of substring matching. The rejection also logged at Error under a message shared with a genuine decode failure, letting any peer produce an ERROR line on demand. It now has its own greppable message at Debug. The proto.Unmarshal branch keeps Error: bytes that do not parse as protobuf at all can mean a real bug, version skew or corruption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(privval,protoio): bound privval handshake reads, add a size sentinel privval/secret_connection.go is a byte-fork of internal/p2p/conn that still read both unauthenticated handshake messages with a 1 MiB cap, so the bound applied to the p2p copy never protected the privval listener. It now uses the same caps. The two files are not de-duplicated here: they diverge in wire type (privval.AuthSigMessage vs p2p.AuthSigMessage), in signing call (Sign vs SignDigest) and in failure policy (errors vs panics), so sharing an implementation is a protocol change rather than a refactor. Tracked in lklimek/tenderdash#26. protoio exports ErrMsgExceedsMaxSize and wraps it with the offending lengths, so the handshake test asserts with errors.Is instead of pinning a message string owned by another package. The cap comments now state measured body sizes (34 for the ephemeral key, 102 for an ed25519 AuthSigMessage, 150 for BLS12-381) and say that maxSize bounds the body, not the framed length. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(types): validate QuorumType against the supported LLMQ types The peer-supplied QuorumType was only checked for uint8 representability, which accepts any of 0-255, while the rest of the codebase validates the same value against btcjson.LLMQType.Validate() at six call sites. It also ran after the per-validator loop and threw the converted value away. The check now uses the allowlist, runs immediately after the nil guard so a tampered set is rejected before the expensive loop, and keeps the converted value. ValidateBasic gets the same check next to ThresholdPublicKeyValid and QuorumHashValid: the light client's HTTP provider builds its set with NewValidatorSet and never goes through the proto path, so the guard placed only in ValidatorSetFromProto never fired there. Two test fixtures built ValidatorSet literals with an unset quorum type and relied on it passing validation; they now name a real one. Genesis already rejects a zero quorum type, so no production path supplies it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(types): apply the same host rules on both address parsing paths ParseAddressString and validateAddressString split the same address format but applied different rules: only validateAddressString checked the host with ValidateHostname, so a host the peer handshake rejects was still accepted at startup. ParseAddressString now runs the same check. splitAddressString's doc claimed it validates each part; it validates the node ID and port and only rejects an empty host. Its results are named in the signature. The standalone listen-address rejection and malformed-NodeID tests fold into the existing TestNodeInfoValidate table, which already carries ListenAddr rows. The hostname-is-not-resolved test stays standalone: it asserts timing, not rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(types): drop the unused Timeout.Commit and Timeout.BypassCommitTimeout ValidateConsensusParams wrote two deprecation warnings straight to stderr, both guarded by fields outside HashedParams, so a remote statesync witness controlled whether an unstructured stderr write fired. Both fields were marked "Unused, TODO: Remove in 0.15" and the proto already reserves commit and bypass_commit_timeout, so nothing reads or writes them: ToProto never sets them, ConsensusParamsFromProto never reads them, and the two warnings were their only consumers. Removed along with the warnings, at v1.6. Genesis files still carrying the keys keep parsing: consensus params are decoded with encoding/json, which ignores unknown fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: record the deferred and accepted statesync and node-info risks Two statesync gaps are deferred rather than fixed here: extending HashedParams to cover the remaining consensus params is consensus-breaking (lklimek/tenderdash#24), and mirroring the P2P provider's validation into the RPC provider is tracked separately (lklimek/tenderdash#25). Two accepted risks are recorded where they apply: the stricter ListenAddr hostname check and the removal of DNS resolution from our own startup config validation both carry no regression risk in deployments that only ever use IP literals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(types): return an error for unknown vote-extension types (#18) * fix(types): return an error for unknown vote-extension types VoteExtensionFromProto returned a fabricated GenericVoteExtension for a type outside the defined enum values. proto3 enums are open, so a peer can put any varint on the wire, and vote extensions feed the sign-hash through MakeQuorumSigns: substituting a default silently changes the digest, so at best verification fails with a misleading error and at worst two nodes derive different canonical votes for the same commit. Nothing on a path feeding a signature may be fabricated. The conversion now returns an error, propagated through VoteExtensions.Add, VoteExtensionsFromProto, NewVoteExtensionsFromABCIExtended and Commit.GetCanonicalVote, and out through the callers that already return an error. A poisoned message is rejected by the existing caller instead of terminating the consensus goroutine. A poison commit still decodes from the WAL — CommitFromProto keeps extensions in wire form and Commit.ValidateBasic is unchanged — and is rejected later at VerifyCommit, so an already-attacked node still restarts. A poison precommit now dies at MsgFromProto -> VoteFromProto in the reactor; it could never reach the WAL, because on every prior version that same conversion panicked there. NewVoteExtensionsFromABCIExtended propagates the error and ExtendVote panics on it: that input is our own application, not a peer, so an undefined type is a local bug and must fail loudly, consistent with the panics ExtendVote already raises on ABCI misbehaviour. Drops the defensive loop MakeQuorumSigns used to carry: both paths it guarded funnel through the same VoteExtensionsFromProto call, which now rejects the type before Filter can silently exclude it. Out of scope by instruction: peer punishment, Commit.ValidateBasic validation, WAL write ordering, and recover()/panic-handling middleware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: remove commit-extension-panic exploit spec The spec documented an unpatched vulnerability (exact packet size, ungated reactor branch, exploit cost) under docs/, a VuePress-built site with no exclude rule for this path — publishable before the fix ships, contradicting the doc's own §8 guidance not to push it early. Flagged HIGH by security review; removing rather than gating, since no exclude mechanism exists in the current VuePress config. * fix(types): add a sentinel error and reject nil vote extensions VoteExtensionsFromProto dereferenced every slice element without a nil check, contradicting the fail-closed contract VoteExtensionFromProto documents: the conversion is reachable from untrusted network input and must neither panic nor fabricate a value. A nil element now yields an error naming its index. The unknown-type rejection gains an exported sentinel, ErrUnknownVoteExtensionType, so callers can match it with errors.Is. A future peer-punishment change needs to distinguish "this peer sent an undefined enum value" from any other conversion failure, which a bare fmt.Errorf cannot express. The message text is unchanged. Add, VoteExtensionsFromProto and NewVoteExtensionsFromABCIExtended became fallible without their godoc saying so; each now documents its error contract. Callers of VoteExtensions.Add are the one signature change the compiler cannot enforce - a stale .Add(ve) call still compiles and silently swallows the rejection - so the error is now part of the documented API surface. Tests assert the specific sentinel via errors.Is rather than "some error". VerifyCommit's case previously passed even with the rejection fully removed, because its fake zero signature failed verification first; it now builds a commit that genuinely verifies, asserts that positive control, and only then flips one extension type to an undefined value. Restores direct coverage of Vote.VerifyExtensionSign, which the peer-path test does not exercise: its vote is built as a struct literal, reaching a path no converter can guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: share one vote-extension helper and drop review IDs from comments Making VoteExtensionsFromProto fallible spread the same three-line convert-and- require dance across two packages and a dozen call sites, as two near-identical private helpers whose signatures had already diverged (testing.TB vs *testing.T). Export MustVoteExtensionsFromProto from types/test_util.go instead, following the existing convention of a non-test file importing testing (internal/state/test/ factory), and use it everywhere a *testing.T is in scope. The two production helpers that convert extensions, types.makeCommit and the state test factory, keep returning an error: they have no t to fail. Also drops ephemeral review-finding IDs from test comments in the two files this change touches. Those IDs are reassigned on every review run and go dead after merge, so each is replaced by the invariant it stood for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(kvstore): return errors from the sign-item helpers makeVoteExtensionSignItems panicked on the newly fail-closed conversion of peer-forwarded wire data, and makeBlockSignItem panicked on its own marshalling failures. STYLE_GUIDE.md reserves panic for a broken internal invariant; neither qualifies, since both derive from a FinalizeBlock request. Both now return an error, propagated by FinalizeBlock the same way as its other failure paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover the light-client and ExtendVote extension-type paths The unknown-type rejection has four propagation destinations; the light client and the ABCI-facing ExtendVote had no test. verifyBlockSignatureWithDashCore builds a canonical vote from a provider-supplied commit, so a malicious provider could otherwise panic the light client; its Dash Core client is left nil in the test because the rejection must happen before any RPC round-trip. ExtendVote's panic is asserted rather than removed. That response comes from our own application, so a type outside the enum is a local app/tenderdash mismatch, not a network fault, and failing loudly is the intended behaviour there. The test pins that contract so it cannot be softened by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct two comments on the vote-extension paths MakeQuorumSigns claimed a causal dependency that does not hold for the traced input; the real reason to convert first is that MakeBlockSignItem panics rather than returning an error, so fallible work belongs ahead of it. Marks the ExtendVote panic as a reviewed, deliberate choice: ABCI hardening is out of scope here, and panicking on an ABCI-supplied undefined type is the intended strongest signal, not an oversight to be fixed later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): only evict peer addresses that failed their last dial A PEX-advertised URL carries a sender-chosen NodeID, so a peer can insert addresses under another node's identity. For an address learned but not yet dialed, LastDialSuccess and DialFailures tie with freshly injected junk, and eviction fell through to the lowest address string, which the sender controls: it could make its entries sort above a chosen victim and remove that victim deterministically. Against a bootstrapping node that is a partial eclipse primitive, not a memory bound. leastValuableAddress becomes evictableAddresses, which returns only addresses that failed their last dial, least valuable first. An unproven newcomer can no longer displace an unproven incumbent: with none evictable the new address is refused, and refusing is not a permanent freeze, since unreachable addresses accumulate dial failures and become replaceable. DialFailures resets on a successful dial, so one failure already means the address did not work on the most recent attempt; a higher threshold would buy nothing here and would freeze the set outright when MinRetryTime is 0, where retryDelay returns retryNever and the count can never grow. The eviction decision is now made before any address is dropped, so a refusal leaves the peer's address map untouched. The cap test asserted that an over-cap address is always stored, which is the behaviour this commit removes; it now asserts a refusal while every stored address is still viable. The oldest-dial-success test needs its unproven incumbents to have failed a dial before eviction can consider them at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): prune the address index when an address is evicted peerStore.index only ever grew: Set added every address in peer.AddressInfo and removed none, and Delete could not help either, since by the time it runs the evicted addresses are already gone from AddressInfo. A peer picks the NodeID it advertises over PEX, so each eviction leaked an unreachable index entry at the sender's request, per process and never reclaimed. The per-peer address cap was therefore not the memory bound it claimed to be. Set now drops from the index every address the stored entry carried and the incoming one does not, guarded on the index still pointing at this peer so an address re-pointed to another peer survives. Doing it in Set rather than at the eviction call site keeps the invariant inside peerStore and covers any later removal path. peerInfo.Copy is a prerequisite, not a cleanup: it copied the struct and then rewrote the values of the same map, so a "copy" shared its address map with the store. Set would have diffed that map against itself and found nothing, and Add's delete reached into the store before Set ran. It now allocates its own map, as its name and doc always claimed. Every structural mutation of a fetched map is in Add, and every field mutation is in Dialed, DialFailed and UpdatePeerInfo, all of which persist through Set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(types): name every path the hostname check gates The accepted-risk note reasoned only about NodeInfo.Validate's ListenAddr, but the same change added ValidateHostname to the exported ParseAddressString, which also gates NewEndpoint (node/setup.go, the operator's listen address) and ValidatorAddress.NetAddress. The note now states the accepted risk for each. NetAddress gains no strictness: it has no production caller, and its input has already passed ValidateHostname in ValidatorAddress.Validate. That is checkable here, unlike the shape of the app-supplied masternode list, which this repo cannot confirm, so the note no longer rests on it. ParseAddressString's doc says the host must pass ValidateHostname before it is resolved, which its tests already assert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(types): validate the genesis quorum type against the LLMQ allowlist Genesis is the third entry point for a quorum type, and it only checked for zero while ValidatorSet.ValidateBasic and ValidatorSetFromProto check the allowlist. A genesis naming validator_quorum_type 200 passed ValidateAndComplete and failed later in the block replayer, reporting a validator set rather than the field the operator mistyped. Both zero-checks are replaced by one btcjson.LLMQType.Validate() call, which subsumes them; the second was unreachable with a different message anyway, as nothing between the two touches the field. This is what the TODO sitting on the first check asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p2p): harden per-peer address eviction against sender steering Three related weaknesses in the MaxAddressesPerPeer eviction path: Eviction eligibility now derives from the dial timestamps (peerAddressInfo.failedLastDial) instead of DialFailures. Accepted zeroes DialFailures on every inbound connection for scoring and retry backoff, so a peer that reconnects periodically - the normal case in a validator mesh - kept re-marking its junk addresses as viable and permanently disabled eviction of them. An inbound connection proves the peer is up, not that any stored address is dialable, so it no longer touches eviction eligibility. Ties between eviction candidates are broken pseudo-randomly (shuffle before a stable sort) instead of by the address string. The address string is chosen by the advertising peer, so a deterministic order over it handed the sender its choice of victim whenever candidates tied on dial history. New AddTrusted stores an operator- or executor-supplied address (bootstrap and persistent-peer config, the Dash quorum-connection dialer) even when every stored address is viable, evicting the least valuable one; failed addresses go first. Add keeps refusing a peer-learned newcomer in that state. Previously both paths shared Add, so unproven PEX-learned incumbents could block a validator's real address from ever being stored. Node setup now also reports when a configured peer address was not re-added instead of discarding the result. The most-dial-failures eviction test now has two evictable candidates with different failure counts; with a single candidate it never exercised the comparison it claims to test (verified by mutating the comparator). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(privval): validate the quorum type on both remote-signer transports SignVote and SignProposal on the gRPC signer server and in the socket transport's DefaultValidationRequestHandler converted the request's bare int32 quorum type to btcjson.LLMQType unchecked. Sign-hash construction narrows the type to uint8 with a panicking conversion, so a request carrying an out-of-range type crashed the signer; an in-range but unknown type produced a sign hash for a quorum type that does not exist. Both transports now check the type against the LLMQ allowlist before the privVal is touched, matching the guards on the proto-decode, validator-set, and genesis entry points. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(config): drop the unsafe commit-timeout override knobs The Timeout.Commit and Timeout.BypassCommitTimeout consensus parameters are gone, but the config surface still offered unsafe-commit-timeout-override and unsafe-bypass-commit-timeout-override: struct fields, validation, TOML template lines, and documentation for values nothing reads. All removed. The config template test now guards against offering overrides for parameters that no longer exist. Existing config files carrying the keys keep loading, since unknown TOML keys are ignored during unmarshalling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(types): count every hostname-check call site accurately The listen-address strictness note claimed ValidateHostname gates three paths; it has four non-test call sites. The note now scopes its claim to the three paths whose strictness the shared check changed and names the fourth, NodeAddress.Validate, which runs the same check on its own and gains nothing from the sharing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Revert "fix(config): drop the unsafe commit-timeout override knobs" This reverts commit 9b3da4faa87c6e9b8b9f4d608e6c2dc84869e6e2. * Revert "fix(types): drop the unused Timeout.Commit and Timeout.BypassCommitTimeout" This reverts commit 5c07b3440a1be7686c4bc31529117c4cf3577978. * Revert "fix(p2p): harden per-peer address eviction against sender steering" This reverts commit d935795ef250eaf408b3da082e534f187cdc6ed8. * Revert "fix(p2p): prune the address index when an address is evicted" This reverts commit ffb6c6680d5f4587910252ed5cc8b7296792dc70. * Revert "fix(p2p): only evict peer addresses that failed their last dial" This reverts commit 8caadb105f2b2a1c61a8da68a272d8e9ef2db95f. * Revert "fix(p2p): evict least valuable address instead of refusing new ones" This reverts commit 91f2dc6af0807552c68ddd053d758181529bef2d. * fix(types): let ValidateBasic be the only quorum-type gate on the proto path An empty validator set serializes to an empty proto message whose QuorumType is 0, so the early allowlist check in ValidatorSetFromProto rejected a zero-validator genesis before the empty-set determination could report ErrValidatorSetNilOrEmpty, and the node refused to reload its own state. The terminal ValidateBasic already validates the quorum type, and short-circuits on empty sets first, so the early check only cost a second error string. State.FromProto now compares that sentinel with errors.Is so the initial-height tolerance survives future wrapping. * fix(privval): validate a whole signing request before it reaches the signer Both remote-signer transports checked the quorum type and nothing else, so a request without a vote or proposal was dereferenced and a quorum hash of the wrong size panicked SignItem.UpdateSignHash — either one stops the validator from signing at all. ValidateQuorumParams now carries the quorum checks and their rationale once for all four handlers, replacing the inverted error branches on the socket transport with guard clauses. * refactor(statesync): give the consensus-params checks a name and a test verifyConsensusParams holds the validation and the hash comparison of the P2P path in the order they must run, so both are covered by a unit test instead of requiring a full light-client fixture. Its comment states what the params hash does not cover and what the bound does not reject, and the RPC provider says plainly that it verifies nothing. * docs(p2p,types): state the hostname and address-cap decisions once, in place NodeInfo.Validate carries the whole hostname-strictness rationale, covering the node and seed constructors that reach it, so node/setup.go no longer explains code that is gone. PeerManager.Add says what its cap does not do, the two secret-connection read-cap comments read identically again, and the divergence note names the sibling file instead of a fork issue. NewEndpoint failures now name the config key that produced them, and the envelope attribute counter no longer shadows the bytes package. * fix(p2p): drop the per-peer address cap, correct the statesync note The cap bounded peerInfo.AddressInfo at 32 entries per NodeID and refused anything further, with no eviction. PEX takes the NodeID from the sender's URL without relating it to the sender's identity, and stored addresses are only syntax-validated, so one PEX response could fill all 32 slots of a chosen victim NodeID with junk hostnames. Nothing evicts an individual address, the map is persisted so the state survives restart, and Advertise re-gossips it, so the victim's real address was refused indefinitely. The Dash quorum dialer compounds it by discarding the Add result and then pinning the entry with PeerScorePersistent. The same lockout occurs with no attacker at all, from ordinary address churn, because stored addresses never expire. Removing the cap restores unbounded address growth, which is the existing behaviour on v1.6-dev and a resource concern rather than a targeted one: the victim's real address stays dialable. Bounding the store is still worth doing, but it has to bound per sending peer rather than per advertised NodeID, which is a design change rather than a hotfix. Also corrects the note at the RPC state provider. It claimed that path performs no validation and no ConsensusHash comparison, and that the provider is unused in production. Both are wrong: the equivalent checks run one layer down in lightrpc.Client.ConsensusParams, and use-p2p defaults to false, so this is the provider in use unless it is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ic (#32) * fix(types): reject unknown vote-extension types in Commit.ValidateBasic Every other field of a Commit is either checked by ValidateBasic or covered by the threshold signature. The vote-extension type is neither, and proto3 enums are open, so an undefined varint decodes into the generated enum field unchallenged and rides through every boundary a Commit crosses. Add the type check. Type only: an extension's contents are threshold-signed, so they are not ValidateBasic's to police, and the content rules in VoteExtensions.Validate() were written for a vote, where they are not signed. ValidVoteExtensionType is placed next to VoteExtensionFromProto so the set of dispatchable types has one home. A type accepted by one and missing from the other would be admitted into a struct nothing can convert. This inverts a property PR #18 deliberately pinned -- that a poison commit must still decode from the WAL, because a rejection during WAL decode raises DataCorruptionError and aborts catchupReplay. The inversion is safe because the rejection moved upstream rather than merely being added: MsgFromProto calls ValidateBasic at the p2p boundary, so a poison commit is now rejected as it arrives and walMiddleware never has anything to write. A WAL from an older build could still hold one, and that case is recoverable via repairWalFile. The block store, which panics through mustDecodeCommit and has no repair path, is made safe by verifying the commit at statesync backfill ingress -- this change's predicate, not an assumption. commit_extension_unbrick_test.go is rewritten rather than deleted, and states which property it now pins and why. Two tests in commit_extension_regression_test.go built their poison through CommitFromProto, which now rejects it; they build the struct directly instead, because their subject is defence in depth -- what GetCanonicalVote and VerifyCommit do on the consensus goroutine if a poison commit arrives by a route that skips ValidateBasic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(types): one authority for vote-extension type membership The enum-membership question was answered in two hand-written switches that had to be kept in step by hand, and Commit.ValidateBasic's rejections were anonymous strings that three tests pinned by substring. Membership now has one derivation per layer. tmproto.VoteExtensionType.IsValid reads the generated name table, so it answers for whatever the .proto defines with no hand-maintained case list, and (*VoteExtension).Validate uses it - the obvious place to look for vote-extension validation now checks the type it validates. VoteExtensionFromProto's dispatch switch is the only remaining case list, and Commit.ValidateBasic calls it instead of mirroring it, which removes ValidVoteExtensionType entirely. A table-driven test walks VoteExtensionType_name and fails if a defined type has no dispatch arm. Both rejections wrap sentinels: ErrUnknownVoteExtensionType, now owned by the proto package and aliased in types so a rejection from either side matches one errors.Is check, and ErrNilVoteExtension, shared with VoteExtensionsFromProto. The tests assert with ErrorIs and EqualError rather than substrings. The comment above the check claimed every other Commit field is checked or covered by the threshold signature. Commit.Hash is a checksum of ThresholdBlockSignature alone, so nothing binds the extension list's order, count or duplication; the comment now says only what is true, and a TODO points at the tracking issue. An INTENTIONAL note records the accepted rollback hazard of an ungated rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(types): record the accepted risks around commit extension validation Two hazards the type check does not close, both accepted rather than fixed: a block store poisoned before the statesync backfill gate shipped still panics on load with no repair path, and scripts/scmigrate gates on the returned commit instead of the error, so it can re-persist such a commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(types): make the conversion dispatch the sole vote-extension type gate Drop the redundant IsValid guard from tmproto.VoteExtension.Validate: every wire-input path reaches extensions through types.VoteExtensionFromProto's dispatch switch before Validate can run, so the switch is the one enforcement gate. IsValid stays as a query helper that reads the generated name table; the cross-check test pins that the dispatch covers every .proto-defined type. The ErrUnknownVoteExtensionType sentinel moves back to types/ as the sole definition now that the proto package no longer returns it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: align security markers, godocs, and operator docs with shipped behavior - scmigrate INTENTIONAL marker: the accepted-risk rationale claimed the path is not part of the running node's write path; it is wired to the shipped 'tenderdash key-migrate' subcommand. State the true rationale: the store already holds the record, so re-persisting adds no new exposure. - pre-upgrade-poisoned-store marker: name all three panic surfaces (mustDecodeCommit, LoadSeenCommit, LoadBlock), not just mustDecodeCommit. - mustDecodeCommit panic message: the failure is a proto->commit decode, not 'converting commit to proto'. - CommitFromProto godoc: a validation error does not imply a nil result; callers must branch on the error, never the value. - Commit.ValidateBasic godoc: document the ErrNilVoteExtension and ErrUnknownVoteExtensionType sentinels callers already depend on. - Drop private-fork issue references from comments; state the constraints self-containedly instead. - UPGRADING.md: document the three operator-visible consequences of rejecting undefined vote-extension types (p2p rejection, startup panic on an already poisoned store with no repair path, rollback hazard once a future release adds a new extension type). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(consensus): recover in gossip handlers, return the canonicalization error Tightening Commit.ValidateBasic makes an already-stored commit with an unknown extension type unreadable. The accepted risk was recorded as a startup panic, but startup reads only the last-height seen commit and the replay range. A commit written by statesync backfill sits below those, so the node starts normally and the record is reached later, by a gossip handler loading the height a peer advertised. That handler ran in a bare goroutine while every other peer-facing path in the node recovers, so one unreadable historical record let any peer terminate the process by naming its height, repeatedly. Gossip handlers now recover and log, matching the consensus reactor, the statesync reactor and the p2p client. GenericVoteExtension.SignItem returned an error for its first fallible call and panicked for the other two, one of which is CanonicalizeVoteExtension. That function keeps its own list of the types it can canonicalize, separate from the conversion dispatch this PR makes authoritative, so a type added to the .proto with a dispatch arm but no canonical form would pass the new gate and then panic on the consensus goroutine. It now returns the error, which its only caller already handles. Drops VoteExtensionType.IsValid: it had no production caller, and both tests using it asserted a tautology, since IsValid is the same map lookup they iterated. The dispatch coverage test keeps its value without it. Corrects UPGRADING.md, which promised a startup panic for the exact population that does not get one, and adds the WAL case, where repair rewrites only the most recent segment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ivan Shumkov <ivanshumkov@gmail.com>
…eer that sent it
TestReactor_Backfill_RejectsOutOfRangeQuorumType has never passed since f75f16459
introduced it. It asserts the run names the quorum type; the run reported
"max retries to fetch valid blocks exceeded (20)".
Root cause: the rejection the test names never runs. A response becomes a light
block in Dispatcher.Respond, and LightBlockFromProto validates every part it
decodes - including ValidatorSet.ValidateBasic, which has bounded the quorum type
since 59141c8e7, an ancestor of the commit that added the test. The tampered block
therefore never decoded. Respond returned that error to a caller that only logged
it and dropped the response, leaving the fetch waiting on a call nobody would ever
answer. The fetch expired on its own deadline and took the "peer didn't respond on
time" branch, which reads the sender as merely slow: it charged the retry budget
shared with every peer, marked the peer for reuse, and reported nothing. Twenty
rounds of that produced the retry-budget message. The run's own logs show it
exactly - twenty dropped responses, zero rejections.
Neither limb of the suspected contradiction held. The comment saying rejections are
not charged to the retry budget is true, and the stall detection works; the reject
path simply never executed. What was false is the comment above it, claiming
ValidateBasic never bounds the QuorumType. It does, one layer earlier, which is why
the reactor's own copy of that check cannot fire on anything arriving over p2p. The
copy is kept as a guard for callers that reach the verify loop by another route, and
its comment now says so instead of the opposite.
The fix is at the boundary that swallowed the failure. Respond now hands an
undecodable response to the waiting caller as a refusal, and the fetch loop treats it
the way the verify loop already treats an unverifiable commit: the peer is withdrawn
from the run, whatever else it supplied is discarded, the height is rescheduled
without charging the shared budget, and the reason it gave becomes what the run
reports if no other peer turns out to be able to serve. Both paths go through one
helper so they cannot drift.
This was never only a diagnosis bug. Every rule LightBlockFromProto enforces - the
whole of Header, Commit and ValidatorSet ValidateBasic - was a cheap and
unattributable way for one peer to spend a budget belonging to all of them, at one
full response timeout apiece (ten seconds in production), while staying in the pool
and never being reported.
Scope: the fetch loop's own lb.ValidateBasic branch still retries and reports
non-fatally. It sits on the other side of the line this change draws - refused while
decoding means the peer is withdrawn, wrong content after decoding is retried, as the
neighbouring trusted-hash mismatch already is and as TestReactor_Backfill pins with
its flaky-peer fixture. Moving that branch is a separate decision about how harshly
to treat a well-formed but wrong block.
Consequence worth naming: an undecodable response is now reported with Fatal, which
severs the connection for every reactor. Peers whose light blocks this build cannot
decode for reasons other than malice - notably the undefined vote-extension type that
UPGRADING.md already records as a rollback hazard - are disconnected during backfill
rather than silently timed out.
Tests, both red before the fix and green after:
RejectsOutOfRangeQuorumType every peer serves the tampered
height, so the run must abort naming
the quorum type
before: max retries to fetch valid blocks exceeded (20)
after: malformed light block response: quorumType error: unsupported quorum
type 300
RecoversWhenOnePeerSendsOutOfRangeQuorumType new; three peers send it and one
honest peer can serve, so the run
must complete every height
before: max retries exceeded while the honest peer was still serving
after: passes; the honest peer fills the range
The second test needs the honest peer to be the slower one. With an honest peer that
answers instantly the range completes before any shared budget could drain, and the
test would pass against the unfixed code - which it did, until it was rebuilt on the
adversarial harness that is sized to drain it.
Refusal is also no longer paid for in latency: the rejecting run drops from 2.5s to
0.4s, because each bad response is refused on arrival instead of after a deadline.
internal/statesync is green under -race -tags deadlock; the timing-sensitive backfill
tests were repeated five times. No new golangci-lint issues (8 before, 8 after, all
pre-existing).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…arns
e823f63be split severity between decoding and verification, but left the verification
side reporting three different causes with one disconnectPeer. Two of them do not meet
the rule that commit introduced - "a response that no build of this software could
have produced honestly" - so they were costing peers a connection they had not earned
losing. Each cause now carries its own severity.
QuorumType.Validate disconnect. The valid types are a property of the
network, and one outside them cannot be signed at
all: the sign-hash path panics converting it.
validateThresholdVoteExtensions penalize. maxThresholdVoteExtensions is a ceiling
this build picked to bound its own verification work,
not a rule of the protocol. A chain whose application
legitimately attaches more of them puts an honest peer
on the wrong side of it.
VerifyCommit disconnect, argued below.
VerifyCommit stays fatal because it is structurally unlike the refusals during
decoding: it compares nothing against a constant of this build. Header.ValidateBasic
compares Version.Block against version.BlockProtocol, and the extension bound compares
against maxThresholdVoteExtensions; VerifyCommit verifies a signature against the
threshold public key carried by the validator set the header pins, reached over a hash
chain walked from the trusted block. Every input is either the peer's own or the
chain's. A signature that fails against the chain's own key is forged.
The residual worry - a release that changed the sign-hash derivation would fail honest
older commits - does not describe a rolling upgrade. The same VerifyCommit gates every
block the node applies (internal/state/validation.go, internal/state/execution.go,
internal/blocksync/applier.go), so a build deriving the sign hash differently could not
follow this chain by any path, and its peers are not "one release behind" but on a
different chain. That is a hard fork, which backfill severity does not get to mediate.
Tests. ReportsRefusalsBySeverity previously exercised only an out-of-range quorum type,
which is the one decode refusal that IS unambiguous malice - so it did not defend the
claim the classification rests on, and would have survived someone carving a Fatal path
back out for "obviously malicious" decode failures. It now has four rows, three of them
the local-choice cases:
Header.Version.Block = BlockProtocol-1 penalize. The skew case itself. Confirmed to
take the decode path: "malformed light block response: block protocol is incorrect:
got: 13, want: 14", four times, with no verify-loop refusal in the run.
out-of-range quorum type penalize. Kept precisely to pin that the
unambiguous case still does not disconnect, because the decode boundary cannot tell
it apart from the row above.
extension list over the bound penalize. New; covers the split.
forged threshold block signature disconnect. Now matched on "invalid commit
signatures" so it cannot be satisfied by the bound's error, which also says
"invalid commit".
Each row was mutation-checked, not merely observed passing: flipping the extension
bound to disconnectPeer fails its row, flipping VerifyCommit to penalizePeer fails
its row, and making decode failures fatal again fails all three penalize rows.
Severity assertions are now made only over the reports matching the row's own cause. A
run reports peers for causes of its own - a header missing the trusted hash, a block
failing ValidateBasic - and an incidental one would have failed the disconnect row
spuriously.
Two limits recorded on peerSeverity rather than fixed here, both pre-existing global
policy: a node at its peer limit disconnects on any reported error, so the distinction
collapses on a saturated node; and a peer withdrawn without being disconnected is not
re-admitted to the pool for the rest of the run, since re-admission needs a peer-up
event that only a reconnection produces.
Also renames the refusal log line and the undeliverable-report error, which called
every one of the three causes an unverifiable commit.
internal/statesync green under -race -tags deadlock. golangci-lint unchanged at the 8
pre-existing issues.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects on the pending-request path, all reachable now that #1396 retains timed-out peers instead of evicting them. 1. Send on closed channel. removePending closed the response channel, so a resolver that had already loaded it panicked on the next send. iter calls resolve directly, outside recoveryP2PMessageHandler, so nothing recovered it and the process died. Deleting the close is correct, not a workaround. removePending has exactly one caller - the promise executor's deferred call - and the channel's only reader is that same executor's select, which has already returned by the time the defer runs. The close therefore signaled nobody. It was unsafe in the other direction too: a closed channel yields a zero result, whose nil Value would panic the type assertion on the receive side. Nothing should reinstate it. 2. A late response evicted the peer. With the entry deleted, a response arriving after the timeout fell through to "pending response not found", which iter turns into a PeerError - evicting at capacity and silently defeating blocksync's maxConsecutiveFailures policy. removePending now leaves a tombstone in place of the channel, so resolveMessage can tell a request we issued and retired (dropped, no error) from an ID we never issued (still reported). Tombstones live 2*reqTimeout on the injected clock and sit in an insertion-ordered list swept from addPending: one push and one pop per request, so no background goroutine and no full-map scan. What we retain is set by the requests we issue, not by anything a peer sends. Every retired request is tombstoned, not only expired ones. That also stops a duplicate BlockResponse for the same request ID from being reported - the client-side counterpart of #1393, which fixed only the height-level case in the synchronizer. 3. Blocking send parked the consumer. resolveMessage sent into a one-slot buffer whose reader may already be gone. Removing the close turned the interleaving that used to panic into a permanent hang instead, stopping every blocksync message until node shutdown. The send is now non-blocking: a full buffer means the answer already arrived, so the extra copy is dropped. Found by code review of the first two fixes, and reproduced before fixing. Tests, under -race -tags deadlock. Each one fails on the code it pins: TestRemovePendingKeepsChannelSendable before: panic: send on closed channel after: PASS TestLateResponseAfterTimeoutIsNotAnError before: pending response <uuid> not found after: PASS TestRetiredRequestIDsAreBounded before: Not equal: expected 100, actual 0 after: PASS TestDuplicateResponseDoesNotBlockResolver before: blocked on a response buffer nobody reads after: PASS TestUnsolicitedResponseIDIsAnError passes both before and after by design: it is the regression guard proving the new silence does not swallow fabricated response IDs. The pre-existing TestGetBlockTimeout still passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…throughput Follow-up to ca64f921c, which tombstoned every settled request so a late response could be told apart from an unsolicited one. That made retention scale with total throughput: removePending runs for every settled promise, so a node completing thousands of requests a second held every one of their IDs for the whole 30s window, tens of thousands of entries and megabytes of index, for a fix whose purpose is to stop a node falling over. Only a timed-out request now leaves a tombstone. Success and cancellation delete the entry outright - still without closing the channel, which remains correct for the reasons in ca64f921c. Timeouts are capped by how many requests can be outstanding at once, so retention follows concurrency rather than throughput. The cost is that a second response to an already-answered request is reported again; that requires a peer to answer one request twice, which is misbehaviour, and the synchronizer has handled height-level duplicates since #1393. The lifetime no longer depends on sweeping. Sweeps were driven only by addPending, so a client that stopped issuing requests never swept again and its last tombstones shielded their peers for as long as the node lived. Tombstones now carry their own deadline and resolveMessage enforces it on lookup, so an expired ID reports the peer whether or not a sweep has run. Sweeping also runs on retirement, which reclaims a burst without waiting for a request that may never come. Both remain O(1) amortized with no background goroutine for shutdown to join. Also folded in, from review of ca64f921c: - resolveMessage's unexpected-value branch logs instead of returning an error. It is unreachable today, but returning an error there would have iter raise a PeerError against whoever happened to answer, blaming a peer for our own bug. - Documented why the send select deliberately has no context arm, so it is not reinstated: with a default present the select cannot block, and a ctx arm would win a coin flip against delivering the response. - Corrected a comment claiming the response channel is collected along with the tombstone; it becomes garbage as soon as it leaves the map. Tests. Each new or changed test was verified red by mutating the specific behaviour it guards, then green again: TestRetentionTracksInFlightNotThroughput tombstone every retirement -> FAIL "settled requests must leave nothing behind" now PASS TestTimedOutRequestStopsShieldingPeerWhenIdle drop the lookup-time expiry -> FAIL "An error is expected but got nil" now PASS TestRetirementReclaimsExpiredTombstones remove the sweep on retire -> FAIL on retained count now PASS TestDuplicateResponseDoesNotBlockResolver now also asserts the first response survives; drop-both -> FAIL "the first response was dropped along with the duplicate", and a blocking send still -> FAIL on the 2s guard now PASS TestRetirePendingKeepsChannelSendable covers both retirement paths. Full package suite completes in ~10s under -race -tags deadlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ones
Follow-up to 4e7df97b7, which narrowed tombstones to the timeout path and
deleted the entry outright on every other ending. That was too narrow.
Cancellation is an ordinary path: when the caller gives up, a peer that
answers afterwards has behaved perfectly correctly, and deleting the entry
made that answer look unsolicited and got the peer reported. Penalising a
peer for our own cancellation is the exact fault this path exists to remove.
Retirement now turns on whether the request was answered, not on how it
failed. Timeout and cancellation both leave a tombstone; only an answered
request is forgotten immediately, because a second response to a request the
peer already answered takes two answers to one question and is fair to
attribute. The flag defaults to the cautious case, so a branch added to the
promise select cannot silently start reporting peers.
The memory argument that drove the narrowing is unaffected: cancellations,
like timeouts, are capped by how many requests can be outstanding at once, so
retention still follows concurrency rather than throughput. What was
throughput-scaled was tombstoning answered requests, and that stays deleted.
Also:
- Expiry is now inclusive of the deadline on both paths. resolveMessage used
Now().After(expiresAt) while expireTombstones used !expiresAt.After(now),
so a response landing exactly on the deadline was shielded or reported
depending only on which path ran first.
- The comment on addPending no longer claims issuance is the only place that
shrinks retained state; issuance and retirement are both opportunistic
sweep points, with lookup enforcing the lifetime when neither has run.
Tests. Each guard was verified red by mutating the behaviour it pins, then
green:
TestCancelledRequestDoesNotReportALateResponse
delete on cancel -> FAIL "Received unexpected error" PASS
TestTombstoneLifetimeBoundaryIsInclusive
exclusive lookup predicate -> FAIL "at the deadline the lookup
must stop covering the peer" PASS
TestRetentionTracksInFlightNotThroughput
tombstone answered requests -> FAIL "answered requests must leave
nothing behind" PASS
TestRetentionTracksInFlightNotThroughput now drives 200 real GetBlock
promises to completion instead of calling retirePending directly, so it pins
how newPromise classifies an outcome rather than only what the map does once
told. The boundary test asserts the sweep expires at the same instant the
lookup does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tate
AddPeer replaced the stored PeerData wholesale, and its caller is the
StatusResponse handler, which builds a fresh struct from the peer's
advertised base and height alone. The reactor asks every peer for its
status every 10s and peers may send one unsolicited at any time, while a
block request takes 15s to time out, so every peer's request-tracking
state was wiped roughly once per timeout window.
Three protections rested on that state and none of them worked:
1. maxConsecutiveFailures never fired. Failures at t=15,30,45... against
resets at t=10,20,30... meant numFailures never reached 5, so a peer
failing every request in a row was never dropped.
2. maxPendingRequestsPerPeer stopped binding. numPending reset to 0 with
requests still in flight, and their completions then decremented it
below zero. A negative count always satisfies the limit, so the peer
was handed unbounded concurrent requests.
3. Slow-peer eviction died twice over. FindTimedoutPeers requires
numPending > 0, which a negative count never satisfies, and the
replaced recvMonitor restarted at rate zero every 10s, which the
transfer-rate check excludes.
A status response carries exactly two pieces of new information, so on a
peer we already know it may now change exactly those two. AddPeer goes
through a new InMemPeerStore.Upsert that merges base and height into the
stored peer and leaves numPending, numFailures, recvMonitor and startAt
alone; Put keeps its replace semantics for its remaining callers. The
merge runs inside the store's own update path, so it is atomic against
the request accounting applied from the worker goroutines.
Upsert also recomputes maxHeight when the peer that held it lowers its
advertised height. Put's monotonic max would otherwise leave the maximum
at a block no peer can serve, and IsCaughtUp compares against it.
numPending is now floored at zero in the store rather than at each call
site: the count only grows when a request is issued and only shrinks
when one is accounted for, so a negative value is always a bug, and its
consequences are silent in both directions.
Not addressed here, and a separate defect: numFailures counts concurrent
in-flight timeouts rather than rounds, so a burst of 20 simultaneous
timeouts trips the threshold on the first outage window - exactly the
transient case the policy exists to survive. It needs a different fix
and mixing it in would make both unreviewable.
Tests, under -race -tags deadlock. Each fails on the code it pins:
TestStatusRefreshPreservesFailureCount
before: peer still present after the failure that completes the run
after: PASS
TestStatusRefreshPreservesPendingRequests
before: Not equal: expected 3, actual 0 in-flight after: PASS
TestNumPendingNeverGoesNegative
before: Not equal: expected 0, actual -2 after: PASS
TestStatusRefreshUpdatesAdvertisedRange
before: MaxPeerHeight 200 after the only peer at 200 dropped to 70
after: PASS
TestStatusRefreshKeepsSlowPeerEvictable
before: FindTimedoutPeers returns nothing after a refresh after: PASS
TestStatusRefreshUpdatesAdvertisedRange doubles as the regression guard
on the first two: its base/height assertions pass before and after, so a
"fix" that ignored status responses outright would fail it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of 62b815976 found two races in the fix itself. Both came from the
same place: Upsert did its work in several steps, and maxHeight was a
cached aggregate written by Put, Update, Delete and Upsert, each under
its own lock.
Check-then-insert could erase the state the commit exists to protect.
Upsert looked the peer up, released the store lock, then fell back to a
wholesale Put. Peers are made known by the p2p consumer goroutine while
the job producer is already issuing requests against them, so an insert
landing in that window was overwritten - losing numPending, numFailures,
recvMonitor and startAt, exactly the wipe being fixed. InMemStore gains
an Upsert primitive that inserts an initial value if the key is absent
and applies the updates, all under one lock; the peer store's Upsert is
now that single call. It is not on the store.Store interface because the
interface's mock is generated and nothing else needs the method.
maxHeight could publish a height no peer advertises. A higher Upsert
could write the store while a lower Upsert or a Delete ran its
bookkeeping first, leaving the cached maximum above every peer's. It
decides IsCaughtUp and the stall verdict, so the node then waits in
block sync for a block nobody can serve - the same shape as a stall
attack, reached by accident.
MaxHeight is now derived on read: it scans the peers and takes the
maximum, over a snapshot the store took under its own lock, so the
result is always a state the store actually held. That removes the cache
and its four writers rather than repairing each one. The cost is honest:
the hottest reader is shouldJobBeGenerated on the producing loop, once
per generated job, and that same iteration already runs FindTimedoutPeers
and FindPeer, both full O(peers) scans with allocations. A third scan of
the same order is worth an invariant that cannot be violated by adding a
fifth writer later.
With the aggregate gone, InMemPeerStore holds no state of its own: its
mutex and updateMaxHeight are removed and every method is a delegation,
so all mutation and all reads sit behind the store's single lock.
Tests, under -race -tags deadlock. Both new blocksync tests failed on
62b815976 and pass now; each failed on all 3 runs before the change:
TestUpsertRacingFirstInsertKeepsIssuedRequests
before: Not equal: expected 8, actual 7 in-flight (trial 1)
after: PASS
TestMaxHeightMatchesStoredPeers
before: Not equal: expected 20, actual 149 - a height no stored
peer advertises (trial 18) after: PASS
TestMaxHeightMatchesStoredPeers hammers all three ways a stored height
moves at once: the peer re-advertising a growing range, re-advertising a
lower one, and being dropped. The store tests for the new primitive
(Upsert in TestInMemStore, TestInMemStoreUpsertIsAtomic) pin behaviour
that has no earlier version to fail against.
Noted, not fixed here: a late result from a peer that was removed and
re-added is still charged to the new incarnation, which means the
numPending floor is partly masking that rather than only the wipe. It
pre-dates this work and needs its own change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed height WaitForSync decided whether a stall was worth waiting out from `MaxPeerHeight() - height`, the largest height any peer claims. FindPeer only ever selects a peer whose advertised range covers the height we want (`height >= peer.base && height <= peer.height`), so the two disagreed: a peer could make the stall verdict say "blocks left to fetch" while holding nothing we could use. One peer is enough to exploit that. Against a node at height 100, a peer advertising base 102 and height 1<<60 passes the status handler's bounds check (maxPlausiblePeerHeight is 1<<60 with a strict `>`), covers no height we need, and yet keeps `behind > 0` forever. Job generation loops on height 101 with no peer to ask, and the node sits out maxSyncStall (10 min) instead of the syncTimeout (60s) it used to hand over after - one lying peer multiplying the consensus lock-out by ten with nothing fetchable anywhere. The verdict now asks the peer store whether anyone holds the block we are actually waiting for. Blocks are applied in order, so the synchronizer's own height is the only one that can move it forward, and HasPeerForHeight reuses FindPeer's own heightBetweenPeerHeightRange so "some peer holds this block" cannot mean two different things in two places - two predicates that were supposed to agree and didn't is how this bug happened. FindPeer's other criteria stay out of it deliberately: the pending-request limit and the receive-rate check describe whether a peer can take another request right now, not whether it holds the block, and a peer at its request limit still holds it. maxSyncStall stays as the wall-clock backstop. Servability is judged from what peers advertise about themselves, so it cannot tell a peer that has the block from one that says it does and never answers, and it says nothing at all about a synchronizer wedged on our own side. Its comment now records that reasoning. WaitForSync also went around the injected clock (time.NewTicker, time.Since) while the rest of the synchronizer uses s.clock, which is why the maxSyncStall branch had never been tested - reaching it took ten real minutes. Both now route through s.clock. Red -> green, all under `-race -tags deadlock`: - TestWaitForSyncStopsWhenNoPeerCanServeNextHeight (the defect): peer with base 102 / height 1<<60, local height 100, clock advanced past syncTimeout. FAIL before (block sync never returned - "block sync kept waiting on a peer whose blocks start above height 100"), PASS after. - TestWaitForSyncKeepsGoingWhileAPeerCanServeUs: the guard that stops the above being "fixed" by always stopping. PASS both sides. - TestWaitForSyncHandsOverAfterMaxSyncStall: the branch that had no test. PASS with the clock fix; without it the harness deadlocks in BlockUntilContext because nothing ever registers a waiter on the injected clock - confirmed by running it at HEAD's synchronizer.go, where it panics on the test timeout. - TestWaitForSyncStopsWhenThereIsNothingToFetch: no peers, stops at syncTimeout rather than sitting out the backstop. PASS both sides. - TestHasPeerForHeight: pins the busy-peer decision. Implemented instead as `_, ok := FindPeer(height)`, the "covered only by a peer at its request limit" case FAILs (expected true, got false); PASS as written. stallVerdictFor's table test moves from `behind int64` to `servable bool` and gains the case where the wedge limit must not override nothing-to- fetch. go build ./... clean. internal/blocksync and internal/p2p/... green under -race -tags deadlock. golangci-lint on internal/blocksync reports the same 19 issues as the parent tree, issue-for-issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the servability-based stall verdict, addressing three review
findings.
Rate-rejected peers no longer count as something to wait for.
HasPeerForHeight took only FindPeer's height-range predicate, on the
argument that the other two describe whether a peer can take a request now
rather than whether it holds the block. That argument holds for the
pending-request limit and fails for the receive-rate check, because a peer
below minRecvRate is not transiently unusable but permanently so: FindPeer
will not select it, and selection is the only path that resets its
monitor, so the rate never recovers; FindTimedoutPeers only removes peers
with requests outstanding, so once its last request completes nothing
evicts it either. flowrate never decays a rate on its own - the EMA only
moves on Update - so it stays below the threshold for good. Servability
therefore said "keep waiting" about a peer nothing would ever ask again,
which is the ten-minute lock-out this predicate exists to prevent, reached
by a merely slow peer rather than a lying one. The receive-rate predicate
is now applied; the pending-request limit stays out, since a peer at its
cap is busy only until its in-flight requests complete and excluding it
would end block sync exactly when the pipeline is fullest. The comment
records that the line is transient versus permanent unusability.
The verdict's inputs are now one observation. WaitForSync read the height,
then the stall duration, then servability, in three separate lock
acquisitions. A block applied between the second and third pairs a stale
height with a servability measured for it, so a height that had already
moved on to one a peer serves could still be reported as nothing to fetch.
stallSnapshot reads all three under s.mtx, the same lock advance() holds
while stamping the height and the advance time together, so a concurrent
apply lands wholly inside the snapshot or wholly outside it. MaxPeerHeight
stays outside: nothing is decided on it, it only feeds the log lines.
Evidence, replacing the red->green record in 787ce9b5e. That record does
not hold: the tests fail on the parent because the fake-clock harness
cannot run there at all, not because they detect the defect. Mutation
testing on HEAD instead - one mutation at a time, clock fix left in place,
each test run under -race -tags deadlock. X marks the mutation being
detected:
C1 C2 C3 C4 C5 C6 P1 P2
M1 servable <- advertised-height difference X . . . X X . .
M2 servable <- always false . X . . . X . .
M3 maxSyncStall backstop removed . . X . . . . .
M4 nothing-to-fetch branch removed X . . X X . . .
M5 receive-rate predicate dropped . . . . X . X .
M6 pending-request limit added . . . . . . X .
M7 stall inputs read separately, not under a lock . . . . . X . .
M8 injected clock bypassed for stall duration X . X X X X . .
M9 FindTimedoutPeers evicts with nothing pending . . . . . . . X
C1 WaitForSyncStopsWhenNoPeerCanServeNextHeight
C2 WaitForSyncKeepsGoingWhileAPeerCanServeUs
C3 WaitForSyncHandsOverAfterMaxSyncStall
C4 WaitForSyncStopsWhenThereIsNothingToFetch
C5 WaitForSyncStopsWhenTheOnlyPeerIsRateRejected
C6 StallSnapshotIsOneObservation
P1 HasPeerForHeight
P2 RateRejectedPeerIsNeverUsableAgain
M1 is the original defect and M3, M4 and M8 are the load-bearing branches;
every mutation is caught by at least one test, and M3, M4 and V (the
stallVerdictFor table, omitted from the grid for width) also fail together
where the verdict branches are concerned. M7 is caught only by C6, and C6
caught it on trial 0 of 2000 in 8 runs out of 8, reporting height 100
paired with a stall duration stamped by height 101 - the torn read itself,
not a proxy for it. M9 mutates FindTimedoutPeers rather than this change:
it exists to show P2 guards the premise the receive-rate decision rests
on, so that if eviction ever covers zero-pending peers the decision gets
revisited rather than silently outliving its reason.
Three new tests: C5 (WaitForSync level), C6, and P2, plus a slow-peer case
in P1. The slow-monitor helper asserts the rate it produces is positive as
well as below the minimum, because a zero rate reads as "not measured yet"
and passes the check - a test built on one would prove nothing.
go build ./... clean. internal/blocksync green under -race -tags deadlock.
golangci-lint on internal/blocksync: same 19 issues as the parent,
issue-for-issue. internal/p2p is not green, and not from this change: its
test binary contains no internal/blocksync code (blocksync imports p2p,
not the reverse), and TestPeerManager_Accepted_Timeout - a wall-clock test
with a 10ms budget - fails 4 times in 20 runs on its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pendingToApply holds completed block responses that cannot be applied
because a lower height has not arrived. Nothing bounded it. A peer's
in-flight request count is released in consumeJobResult - AddNumPending(-1)
- before addBlock stores the response, so maxPendingRequestsPerPeer and the
worker pool size bound requests in flight and never responses buffered,
while job generation kept going for as long as p.height <= MaxHeight().
The trigger matters more than the capacity. In normal operation this
self-throttles: consumeJobResult applies synchronously, so while apply is
doing real work the result channel fills, the workers block, the job
channel fills and the producer stalls - back-pressure all the way up. A
missing height removes exactly that. applyBlock short-circuits, the
consumer drains at network speed, every stage above it goes slack, and
what is held is then capped only by maxPeerHeight - s.height whole
deserialised blocks. "Withhold one low height" is an attacker's move, and
the exposure scales with how far behind we are, so a node at the tip is
nearly immune and one doing a fresh sync or a post-statesync backfill is
maximally exposed. This predates the recent block sync changes; the sorted
retry queue shortens each stall window and so reduces it, but does not
bound it.
Job production now refuses to run more than maxOutstandingHeights ahead of
the height being applied. Back-pressure at production rather than eviction
at addBlock: a fetched block already cost the bandwidth to fetch, and
discarding it invites a re-fetch loop. The bound follows from the window
rather than being checked anywhere - a height is only requested while it
is inside the window, addBlock rejects anything below the applied height,
and the window only moves up, so everything held sits in
[s.height, s.height+maxOutstandingHeights) and there are at most that many
of them.
Retries are exempt from the window, and that is what keeps the bound from
wedging sync. A naive "stop producing when full" would also stop the one
height that would unblock everything: the blocking height is either in
flight, and its timeout re-queues it, or already queued, and either way it
only ever goes out through the retry queue. Exempting the queue is safe
for the bound too - a re-queued height was inside the window when it was
first requested and the window only moves up, so it is still inside it
now. Nothing else can widen the window, so the exemption costs nothing and
buys the guarantee. The window also makes the pre-existing wedge in
getPeer less likely, not more: job generation can no longer run thousands
of heights ahead into a range only a lying peer claims.
The bound is a height window, not a byte budget. A byte budget bounds
memory more honestly, but it cannot be enforced at the seam where the
back-pressure has to go: at job production the size of a response is not
known yet, so a byte gate only closes after the bytes have arrived and the
overshoot is however many requests were already in flight - which is the
quantity that was unbounded to begin with. A byte budget would need either
eviction or a count window underneath it anyway. So this bounds the count
and says so: memory is that count times the block size, and at the
protocol's maximum block size the ceiling is large. What it removes is the
unbounded term.
maxOutstandingHeights is poolWorkerSize. The pool is the stage that turns
a requested height into a network request and works on at most that many
at a time, so while nothing is piling up unapplied the window is wide
enough to keep every worker fed, and it binds only when responses are
accumulating - the condition it exists for.
Severity is not claimed beyond the mechanism. Nobody has measured this in
production and this commit does not pretend otherwise.
What was measured, on this tree and cheaply: a full window of 599 buffered
responses from the test chain retains 1,562,928 bytes of heap, about 2.6 kB
per block, against 632 bytes per block on the wire. That is the cost of
holding a decoded block rather than the bytes it arrived as. The test
chain's blocks are empty, so the ratio there is dominated by fixed
per-block overhead and must not be extrapolated to full blocks; what it
does show is that the buffer is not free per entry even when the blocks
are.
Red -> green, all four under -race -tags deadlock. The "before" tree is
this one with the window check removed, carrying the constant declaration
and the unused shouldJobBeGenerated parameter so the tests compile - no
behaviour differs from the parent. All four fail there:
- BacklogIsBoundedWhileTheBlockingHeightIsMissing: "job production never
stopped: 800 heights requested, 799 responses buffered". PASS after,
with the backlog at 599 and 600 heights ever requested.
- BlockingHeightIsStillFetchableAtTheBound: same, from fillWindow.
- InOrderSyncIsNotThrottledByTheBound: "the window did not fill to its
stated width", 800 against 600.
- RequeuedHeightsAreNotStarvedByTheBound: same as the second.
Mutation matrix on the finished tree, one mutation at a time, each test run
on its own under -race -tags deadlock. X marks the mutation being caught:
D1 D2 D3 D4 E1 E2
M1 window check removed (the behaviour before) X X X X . .
M2 window applied to re-queued heights as well . X . X . .
M3 window anchored at the start height . . X . . .
M4 window one height too wide X X X X . .
M5 buffered responses bounded, not heights out X X X X . .
M6 window replaces the highest-peer-height check . . . . . X
D1 BacklogIsBoundedWhileTheBlockingHeightIsMissing
D2 BlockingHeightIsStillFetchableAtTheBound
D3 InOrderSyncIsNotThrottledByTheBound
D4 RequeuedHeightsAreNotStarvedByTheBound
E1 StopReleasesHandlers (existing)
E2 ProduceJob (existing)
M2 and M3 are the ones that show the tests pin semantics rather than
sensitivity, and the failures were read, not just counted. M2 removes the
retry exemption and D2 fails with "the window is full and the height it is
waiting for can never be requested again", D4 with "a re-queued height was
starved by the window" - the wedge, reported as the wedge. M3 stops the
window following the applied height and only D3 fails, with "the window did
not slide when the block at height 1 was applied" - which is the throttle,
and no other test can see it because no other test advances a height. M5
bounds the buffer instead of the heights outstanding, which is a weaker
bound that D1 catches by exactness and D3 catches properly: with in-order
arrival nothing is ever buffered, so that gate never closes and the window
never fills. M6 makes E2 hang rather than fail, in getPeer on a height no
peer holds.
The tests drive the real producer and consumer. The worker pool is given no
workers, so each job is run by the test the way a worker would run it -
through the real client, the real decode and the real validation - which is
also how the test learns which height went to which peer rather than
assuming it. The receive-rate clock is frozen for the duration: peer
selection drops a peer measured below minRecvRate, and that rate would
otherwise be measured over however long the test itself took.
go build ./... clean. internal/blocksync green under -race -tags deadlock
in 17.6s, up from 8.6s: the backlog tests need a chain longer than the
window is wide, and it is built once and shared. internal/p2p/... green
under the same flags. golangci-lint v2.8 on internal/blocksync: 19 issues
on the parent and 19 here, identical issue for issue against a worktree
checked out at the parent commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of ebe06dd1a: maxOutstandingHeights = poolWorkerSize let 599 decoded
blocks stay buffered, which against this repository's 22 MB default block
limit is over 12 GB, and against the 100 MB protocol maximum far more.
Capping unbounded growth there is not a memory bound. The finding is
right, and it is the one the previous commit's own measurement pointed at:
that measurement was reported on empty blocks with a warning not to
extrapolate, and extrapolated properly it condemns the number chosen.
The previous commit argued that a byte budget cannot be enforced where the
back-pressure has to go, because at job production a response has no size
yet, so a byte gate closes only after the bytes have arrived and overshoots
by however many requests were already in flight. That part still holds.
The conclusion drawn from it - bound the count instead - did not follow.
Both quantities need bounding, because each is the one the other cannot
see.
So the backlog now has two limits and job production respects both.
maxPendingApplyBytes, 256 MiB, is what may be held, summed from the
serialized size already recorded while each response was decoded. It is
the limit that decides the memory cost once blocks are large: at the 22 MB
default it stops the backlog after a dozen blocks, where a count of 64
would have allowed 1.4 GB.
maxOutstandingHeights, 64, is how many heights may be requested and not yet
applied. It is not a restatement of the budget. A request in flight has no
size to charge, so without it an empty backlog would let the producer run
as far ahead as the peers and the worker pool allow, and every one of those
requests lands in the backlog the moment a height below it goes missing -
which is the overshoot above, unbounded. It is also the limit that governs
small blocks, where the wire size understates what is actually retained
(see the measurement below). It is no longer tied to poolWorkerSize; the
relationship that matters is with the byte budget, and the comment states
it: the worst case is maxPendingApplyBytes plus maxOutstandingHeights times
the largest block the chain permits, about 1.4 GB at the 22 MB default.
That residual is honest rather than hidden. Its in-flight half is the
fetch pipeline's own footprint - the p2p receive path already accepts a
whole block per outstanding request, so those bytes exist whatever the
backlog does - and bounding the heights turns it from whatever the peer
count and the worker pool happened to permit into a stated 64. Removing it
entirely means charging in-flight requests at the maximum block size, which
caps concurrent fetches at the budget divided by that maximum - about
eleven - and that would throttle a chain that permits 22 MB blocks and
ships 2 kB ones. 64 in flight fetches several hundred blocks a second at a
tenth of a second per round trip, far more than applying them can consume.
The anti-wedge property now has to survive the byte budget as well, and it
does: the retry queue is consulted before either limit, so neither can
refuse the height the backlog is waiting for. Both halves of that are
tested, because the height-window tests cannot see a byte-budget wedge -
their blocks are small enough that the budget is never near spent.
The producer reads the height and the size together under one lock, so a
job cannot be admitted against a height from one moment and a backlog from
another. The size is summed on read rather than kept as a counter:
pendingToApply holds at most maxOutstandingHeights entries, so the sum
costs less than the two peer scans that loop already runs per job, and a
counter maintained separately by addBlock, advance and dropPeer is how the
cached maxHeight in 9382706bb came to disagree with the peers it
aggregated.
Measured again on this tree, and this time it is load-bearing rather than
colour: a full window of 63 buffered responses from the test chain retains
195,336 bytes of heap, 3,100 bytes per block, against 630 bytes per block
on the wire - 4.9 times. So the byte budget, which counts wire bytes,
understates what small blocks really cost by about five, and it is the
height window that bounds them: 64 of these retain roughly 200 kB. Large
blocks are the other way round, and the budget bounds those. The test
chain's blocks are empty, so this ratio is a small-block figure dominated
by fixed per-response overhead and must not be extrapolated to full blocks;
that is precisely why it is the number that justifies keeping a count
limit at all.
Two tests are new beyond the change itself.
The byte budget gets its own pair, since no existing test could distinguish
it: BacklogIsBoundedByBytesWhenBlocksAreLarge, where responses reporting an
eighth of the budget each stop the backlog at eight - well inside the
height window, which the test asserts so that a pass cannot come from the
wrong limit - and BlockingHeightIsStillFetchableAtTheByteBound, the
no-wedge guard for it.
ClientTimeoutUnwedgesAFullWindow closes the assumption the reviewer named:
every no-wedge test handed the consumer a failure directly, so nothing
showed the failure ever arrives. That happens in the block client, in
another package. This test runs a real client on a fake clock, real
workers and the synchronizer's own producer and consumer goroutines against
a peer that drops the first request for the lowest height and answers its
retry. The backlog fills to the window, the height that would release it is
outstanding, one clock advance times the request out, and block sync then
runs to the end of the chain. Deleting that single advance makes it fail
after 60 s with the backlog still wedged, so the timeout is doing the work
and not the harness. The peer answers from its own goroutine after a pause,
because a reply posted from inside Send arrives before the client has
registered the request and is discarded as unsolicited - the pause is the
round trip that ordering comes from, which cost one debugging round to find.
DroppingTheBlockingPeerLeavesTheHeightToTheTimeout closes the other noted
gap: dropping the peer that holds the outstanding lowest request re-queues
only what it had already answered, so that height is not rescued by the
removal and the timeout remains the only thing that frees it.
Mutation matrix, one mutation at a time on the finished tree, each test run
on its own under -race -tags deadlock. X marks the mutation being caught,
H that it was caught by hanging:
D1 D2 D3 D4 D5 D6 D7 E1 E2
M1 both limits removed (behaviour before D) X X H X X X X . .
M2 both limits applied to retries as well . X . X . X X . .
M3 window anchored at the start height . . X . . X . . .
M4 height window one height too wide X X X X . X . . .
M5 buffered responses counted, not heights out X X H X . X . . .
M6 window replaces the highest-peer-height check . . . . . . . . H
M7 byte budget dropped . . . . X . X . .
M8 byte budget checked ahead of the retry queue . . . . . . X . .
M9 byte budget one response too generous . . . . X . X . .
D1 BacklogIsBoundedWhileTheBlockingHeightIsMissing
D2 BlockingHeightIsStillFetchableAtTheBound
D3 InOrderSyncIsNotThrottledByTheBound
D4 RequeuedHeightsAreNotStarvedByTheBound
D5 BacklogIsBoundedByBytesWhenBlocksAreLarge
D6 ClientTimeoutUnwedgesAFullWindow
D7 BlockingHeightIsStillFetchableAtTheByteBound
E1 StopReleasesHandlers (existing) E2 ProduceJob (existing)
M8 is the one worth reading. It moves only the byte half of the check above
the retry queue - the wedge the reviewer asked to be watched for - and D7
is the only test that fails, with "the byte budget is spent and the height
it is waiting for can never be requested again". D2 and D4 do not notice,
because their blocks are small: the guard against a count wedging sync
cannot double as the guard against a budget wedging it. M7 deletes the byte
budget and only D5 and D7 fail, so those two are carrying the new limit and
nothing else silently was. M3 stops the window following the applied height
and D3 fails with "the window did not slide when the block at height 1 was
applied", D6 by never finishing.
M1 and M5 make D3 hang rather than fail, and the reason is worth recording:
with the limit gone the producer runs until every peer is at
maxPendingRequestsPerPeer and then blocks inside getPeer with nothing left
to ask. That is the pre-existing blocking case noted on ebe06dd1a, reached
here by removing the bound - the bound keeps job generation far away from
it.
D5 and D7 have no red-on-a-previous-tree record, since the limit they test
did not exist to be disabled; M7 and M9 are that evidence.
Still not covered, and left so deliberately: the memory figure is measured
on empty blocks, so it characterises per-response overhead and not a real
chain's block sizes, which nobody has measured here; and the worst case
above is arithmetic from the configured block limit, not an observation.
go build ./... clean. internal/blocksync green under -race -tags deadlock
in 12.2s, down from 17.6s, since the shorter window needs a shorter test
chain. internal/p2p/... green under the same flags. golangci-lint v2.8 on
internal/blocksync: 19 issues on ebe06dd1a and 19 here, identical issue for
issue against a worktree checked out at that commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…true
Review of 29dbb2d59 confirmed the mechanism and the anti-wedge property and
rejected the disclosure. It was right to: that paragraph exists to be an
honest statement of what is left uncovered, and a wrong number offered as
a deliberate disclosure is worse than none, because it reads as checked.
Four corrections, all verified here before being written down.
The total was wrong. 256 MiB + 64 * 22,020,096 is 1,677,721,600 bytes,
about 1.68 GB. The reported 1.4 GB was the in-flight term computed
correctly and then printed as the total, with the budget it is added to
dropped.
The anchor was wrong. Pinning the figure to this repository's default
block parameters describes one configuration, and not the worst one:
ValidateConsensusParams accepts up to types.MaxBlockSizeBytes, which is
104,857,600 bytes, and the same expression there is 6,979,321,856 - about
6.98 GB.
So the ceiling is now stated as what it is, an expression rather than a
number:
held <= maxPendingApplyBytes + maxOutstandingHeights * maximum block size
with the last term belonging to the chain, and three instantiations beside
it: blocks that stay small, where the budget dominates at about 256 MiB;
the 22,020,096-byte default, about 1.68 GB; and the 104,857,600-byte
ceiling a valid configuration may set, about 6.98 GB. An operator whose
chain genuinely fills large blocks should lower maxOutstandingHeights,
which brings the second term down in proportion, and the comment says so.
That is accurate at every configuration rather than correct at one, and it
leaves the decision with whoever knows their own block sizes.
maxOutstandingHeights stays at 64. Sizing it for the 100 MiB maximum means
about eleven fetches in flight, which throttles a chain that permits large
blocks and ships small ones - the regime this actually runs in. The
comment now names that trade rather than leaving 64 looking like a number
without an alternative.
Two more gaps in the same paragraph, both now written down where they are
relevant rather than left implied. The byte budget is summed from a
serialized size that covers the block alone: it excludes the commit held
with it and everything decoding allocates, so it is a budget for block
data and not a statement of the memory the backlog occupies. And the
second term is not the only footprint of its kind - the block sync
channel's receive buffer independently holds up to 1024 messages of up to
a whole block each, which nothing here bounds.
No mechanism changed. The gate, the limits and their values are byte for
byte what they were in 29dbb2d59; the diff is comments, one new test, and
the harness peer count.
Hang-killed mutants, the other finding. Three cells of the matrix were
killed only by a test timing out, which shows a mutant broke something
rather than that a test detected the defect. Both causes were the same
shape and both are closed.
M1 and M5 hung TestInOrderSyncIsNotThrottledByTheBound because with the
limit gone the producer requested until every peer sat at
maxPendingRequestsPerPeer and then waited inside peer selection with
nothing left to ask. The harness had peers for a window's worth of
requests, not a chain's. It now has enough for every height in the test
chain to be outstanding at once, so a run that gets past the limits under
test reports what it did instead of stopping short of saying it. Both
mutants now fail that test on its assertion.
M6 was killed only by TestProduceJob hanging, for the same underlying
reason: the mutation lets job generation admit heights above every peer's,
and the consequence is waiting in getPeer rather than an answer. That is
what the pipeline tests can see. TestShouldJobBeGenerated now asks the
admission decision directly, one arm at a time - a peer holds the next
height, the next height is above every peer, either side of the window
edge, either side of the byte budget, and a retry offered while both are
full. It answers where the pipeline stalls, and it kills every mutation of
the gate on an assertion.
Matrix re-run in full, one mutation at a time, each test on its own under
-race -tags deadlock. X is a failed assertion, H a timeout:
D1 D2 D3 D4 D5 D6 D7 D8 E1 E2
M1 both limits removed X X X X X X X X . .
M2 both limits applied to retries as well . X . X . X X X . .
M3 window anchored at the start height . . X . . X . . . .
M4 height window one height too wide X X X X . X . X . .
M5 buffered responses counted, not heights out X X X X . X . . . .
M6 window replaces the peer-height check . . . . . . . X . H
M7 byte budget dropped . . . . X . X X . .
M8 byte budget ahead of the retry queue . . . . . . X X . .
M9 byte budget one response too generous . . . . X . X X . .
D1 BacklogIsBoundedWhileTheBlockingHeightIsMissing
D2 BlockingHeightIsStillFetchableAtTheBound
D3 InOrderSyncIsNotThrottledByTheBound
D4 RequeuedHeightsAreNotStarvedByTheBound
D5 BacklogIsBoundedByBytesWhenBlocksAreLarge
D6 ClientTimeoutUnwedgesAFullWindow
D7 BlockingHeightIsStillFetchableAtTheByteBound
D8 ShouldJobBeGenerated (new)
E1 StopReleasesHandlers (existing) E2 ProduceJob (existing)
Every mutation is now caught by at least one assertion. The single
remaining H is M6 at E2, and it is redundant there: D8 kills M6 on the
assertion, and E2's hang is the same defect seen from the far end of the
pipeline. M8 remains the cell worth reading - only D7 and D8 catch it,
because a guard against a count wedging sync cannot double as a guard
against a budget wedging it.
Still uncovered, and deliberately: the per-response overhead figure quoted
in these comments is measured on empty blocks, so it characterises fixed
cost and not any real chain's blocks, which nobody has measured here; the
ceiling above is arithmetic from a configured limit rather than an
observation; and the p2p receive buffer is outside it entirely.
go build ./... clean. internal/blocksync green under -race -tags deadlock
in 12.0s, internal/p2p/... green under the same flags. golangci-lint v2.8
on internal/blocksync: 19 issues on 29dbb2d59 and 19 here, identical issue
for issue against a worktree checked out at that commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four statements in the ceiling comment were wrong. All four are corrected here and nothing else changes: no line outside a comment differs from 3d89c95b9. Each was re-derived from the source rather than taken on report. The formula used the wrong variable. It multiplied the window by the chain's configured maximum block size, on the assumption that the chain's own parameters bound what can arrive. They do not. The block sync channel declares RecvMessageCapacity as types.MaxBlockSizeBytes plus the message prefix, a static 104,857,600 bytes, so a peer may send a 100 MiB block to a node whose chain permits 21 MiB. Nothing rejects it for its size on the way in: BlockFromProto validates a block against itself, and the size check against consensus parameters lives in block validation at apply time - which is exactly the step that does not run while a lower height is missing, so an oversized block is decoded and held like any other. The term is now the ingress limit, the ceiling is unconditional at about 6.98 GB, and the table of per-configuration figures is gone because there is nothing for it to vary over. That also invalidated the operator guidance, which told an operator running large blocks to lower the window. The exposure is set by what a peer may send, not by what the local chain permits, so the condition was meaningless. Lowering maxOutstandingHeights does still move the term in proportion, and the comment now says that without the false premise: it is the only lever on the ceiling here, and it applies to everyone. The queue figure was wrong by three orders of magnitude. The comment said the block sync receive buffer holds 1024 messages. RecvBufferCapacity is 1024, but that is the size handed to the queue factory, and the default queue type is simple-priority, which squares it: newSimplePriorityQueue sets maxSize to size * size, and the run loop trims the heap to that. The default node configuration selects it and passes it through to the router, so the real figure is 1,048,576 messages, each able to carry a whole block. It remains outside this accounting; only the number was wrong. The small-block case named the wrong limit. It called that regime budget-dominated, when it is the count that binds there and the budget is never approached - sixty-four 2 KiB blocks are 128 KiB of block data against a 256 MiB budget. This was the one error that argued against its own paragraph: the reason both limits exist is that each covers what the other cannot see, and the text had the common case attributed to the limit that does not bind in it. The comment now gives the crossing point, maxPendingApplyBytes/maxOutstandingHeights or 4 MiB a block, and says which limit binds on each side of it. "About eleven fetches" came from nowhere stated. It was the 256 MiB budget divided by the 21 MiB default block size, then quoted as the window that the 100 MiB ingress limit would force - two different block sizes in one figure. The arithmetic is now shown from the numbers in the comment: holding the in-flight term to the budget itself means a window of two, and a 1 GiB allowance for it gives ten. Both throttle every chain whose peers never send a block near the ingress limit, which is the argument for keeping 64, and it is now derivable rather than asserted. maxOutstandingHeights stays 64 and maxPendingApplyBytes stays 256 MiB. The gate, the limits and the tests are untouched, so the mutation matrix in 29dbb2d59 and 3d89c95b9 still describes this tree. go build ./... clean. internal/blocksync green under -race -tags deadlock in 12.4s, internal/p2p/... green under the same flags. golangci-lint v2.8 on internal/blocksync: 19 issues on 3d89c95b9 and 19 here, identical issue for issue against a worktree checked out at that commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…flood DoS, phase 1) (#17) * docs(consensus): spec for vote-flood DoS hardening Records the reviewed analysis and plan for the consensus vote-flood resource-exhaustion DoS (distinct from the Commit-extension crash, which is fixed on branch fix/commit-extension-panic). §1 mechanics are verified and reliable: an unprivileged peer floods a validator with structurally-valid, cryptographically-invalid precommits, forcing ~2.7-2.9 ms of BLS verification each, a WAL write, and two error logs, with no disconnect. §4 records the proposed mitigation layers (L1-L6) and the blocking defect independent review found in each first draft — none may be implemented as first written. §7 is the revised recommendation; §9 the no-code allowlist-only mitigation; §10 a related latent HeightVoteSet issue. No code fix yet — this branch holds the plan; the fix will be implemented and validated here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): bound invalid-vote log amplification under peer flood (L4) An unprivileged peer flooding invalid votes made a validator emit two Error-level log lines per message (measured 46,752 entries for 23,376 bad votes) — log volume scaled with the attacker's message rate. Downgrade to Debug only the validation errors an unprivileged peer can trigger at will, classified by error TYPE (not by whether a peer sent the message): - addVoteLoggingMw: a not-added vote logs at Debug. The notable cases (non-deterministic and conflicting signatures) are still logged at Error by addVoteErrorMw. - loggingMiddleware: uses isPeerFloodableError (matches ErrVoteInvalidBlockSignature, ErrVoteInvalidSignature, ErrVoteUnexpectedStep via errors.Is). Anything not positively matched — including internal faults such as ErrPrivValidatorNotSet surfaced while handling a peer message — stays at Error, so this never hides a real problem. Classifying by PeerID alone was too broad (it would hide internal faults that surface during peer-message handling); this uses explicit error categories instead, which fails safe toward surfacing. Part of docs/CONSENSUS_VOTE_FLOOD_SPEC.md (L4). No control-flow change; logging level only. Tests (red→green): internal/consensus/log_amplification_test.go — isPeerFloodableError matches floodable errors (including the production ErrInvalidVoteSignature wrapping) and rejects internal faults; loggingMiddleware logs a floodable error at Debug but an internal fault at Error even for a peer message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): cap vote-extension count to stop verification amplification (L5) Each threshold-recoverable vote-extension costs one BLS signature verification (~2.7 ms). Nothing bounded the count, so an unprivileged peer could pack ~10^4 extensions into a single ~1 MB commit or precommit and force that many verifications on the single consensus goroutine — the core of the vote-flood DoS. Add MaxVoteExtensions (32) enforced in MakeQuorumSigns, the shared chokepoint of VerifyCommit (commits) and VerifyExtensionSign (precommit votes). Over-cap messages are rejected before any per-extension work. Placed on the verification path, not Commit.ValidateBasic, for the same reason as the unknown-type fix: a rejection on the WAL-decode path would abort replay and re-brick an already-attacked node. Cap rationale: Dash Platform's ExtendVote returns at most withdrawal_transactions_per_block_limit threshold-recoverable extensions (currently 4, a bounded SizedQuery limit, verified in rs-drive-abci). 32 gives 8x headroom for a future protocol increase while bounding attacker-forced work to ~32 verifications (~86 ms) per message instead of ~25 s. If that limit is ever raised above 32, raise this constant in the same release. Part of docs/CONSENSUS_VOTE_FLOOD_SPEC.md (L5). Note: this changes what the p2p layer accepts; a P2PProtocol bump decision is deferred to the maintainer per RELEASES.md (legitimate traffic is unaffected, so practical interop impact is nil). Tests (red→green verified): types/vote_extension_count_test.go pins the count boundary at MakeQuorumSigns and via VerifyCommit; both fail with the check removed. Green across types, privval, light, consensus. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): per-peer vote-channel rate limit (L1) An unprivileged peer can flood the vote channel and force one BLS verification per message (~940/s in the reported attack). Add a per-peer token-bucket rate limit on the vote channel (votes and commits): messages over a peer's budget are dropped in the reactor before reaching signature verification, bounding the CPU one connection can force. - New config consensus.peer-vote-rate-limit (float, messages/s/peer; 0 disables). Default 100: an honest validator gossips ~10-20 vote-channel messages/s, so the limit only bites a flood. - Burst is a small 2x the rate (voteRateBurstMultiplier), not the shared limiter's 10x default: vote-channel messages are individually expensive, so a large burst would let one peer front-load seconds of BLS work in an instant. NewRateLimitWithBurst floors the burst at 1 when the limit is positive, so a small rate cannot truncate to a zero-token bucket that silently drops everything. Config validation rejects NaN/Inf/negative rates. - drop=true (non-blocking): an over-limit peer cannot block the shared channel goroutine, and honest peers on other limiters are unaffected. - Applied in Reactor.processMsgCh via allowVoteChannelMessage; other channels and the disabled (0) case always pass; limiter errors fail open. Scope and limits (honest): this bounds a SINGLE connection. Node identities are free and unbound, so a determined attacker dilutes it across connections/IPs — tighten p2p connection limits alongside. The real fix is the validator-set vote gate (docs/MASTERNODE_VOTE_GATING_SPEC.md). Commits ride this channel and are covered too; a separate typed disconnect on invalid-commit-signature is planned. Tests (red->green): vote_rate_limit_test.go (per-peer drop, isolation, other channels unlimited, disabled=0); ratelimit_test.go (explicit/floored burst); config_test.go (rate validation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(p2p): wire max-incoming-connection-attempts into the router (connection limit) getRouterConfig set IncomingConnectionWindow but never passed MaxIncomingConnectionAttempts, so the config value was silently ignored and the router always fell back to its internal default (100, router.go:99-100). An operator tightening consensus/p2p connection limits to bound a flood got no effect. Pass conf.P2P.MaxIncomingConnectionAttempts through to RouterOptions so the per-IP incoming-attempt cap is operator-controllable. This is the connection- level lever that bounds how many connections/IPs an attacker can use to dilute the per-peer vote-channel rate limit (L1) and to spam commits (which cannot be sender-gated). Default value unchanged (100); tightening it is now an operator config action. Test: node_test.go asserts a non-default config value flows into RouterOptions (fails before the fix: the field was 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: implementer brief for remaining vote-flood hardening Captures the precise, non-obvious decisions for the three remaining items (typed commit-signature disconnect, data-channel proposal rate limit, L6 block-sig-before-extension) so a focused implementer session can execute them without re-deriving the classification/scope nuances from the specs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): disconnect peers sending cryptographically invalid commits Commits ride ConsensusVoteChannel and trigger threshold-signature verification, but cannot be sender-gated: non-validators legitimately relay them for catch-up (shouldCommitBeGossipedForCatchup). A commit whose THRESHOLD SIGNATURE fails to verify is, however, unambiguous misbehaviour — a commit is stored only after a successful VerifyCommit (state_data.go), so no honest node ever relays a cryptographically invalid one. Evict the peer that sends one. Precise, safe classification (the whole risk here is a false positive that partitions the network): - New typed error types.ErrInvalidCommitSignature, emitted ONLY wrapping the final quorumSigns.Verify failure in ValidatorSet.VerifyCommit. Wrong block ID (rejected earlier), wrong quorum hash, extension errors, and local finalization faults keep their distinct, untyped errors. - The disconnect fires only when errors.As(err, *ErrInvalidCommitSignature) succeeds, and never for a WAL-replayed message (FromReplay gate — replay re-dispatches with the original PeerID). - Propagation is non-blocking: the state machine queues a peerErrorMsg (dropped if the buffer is full) and a new reactor peerErrorRoutine drains it and emits a Fatal p2p.PeerError. The consensus goroutine never blocks. Composes with L1 (rate-limits commit spam) and the connection-attempt cap; together they bound commit spam without gating honest relayers. Tests (both pin the classification; they fail if any non-signature error were treated as a signature error): - types/validator_set_test.go: a bad threshold signature is typed; wrong block ID / wrong quorum hash are not. - internal/consensus/state_try_add_commit_test.go: only the typed error queues a fatal eviction; wrong-block / wrong-quorum / local-fault / replay-sourced do not; nil queue tolerated; full queue drops the report. Green across types and internal/consensus. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): per-peer data-channel rate limit for proposal flood The data channel has the same re-verifiable BLS flood as the vote channel: a junk proposal fails signature verification (state_proposaler.go) but never sets rs.Proposal, so there is no dedup and every copy re-verifies. An unprivileged peer (need not be the proposer) can force N verifications with N junk proposals. L1/L5/the vote gate do not touch this channel. Add a per-peer token-bucket limit on the data channel, but cost-WEIGHTED rather than a flat message rate — because the channel also carries block parts, which are legitimately gossiped to non-validators for catch-up and must not be throttled: - proposals cost proposalTokenCost (5): each forces an un-deduplicated BLS verification; - block parts and POL bit arrays cost 1: cheap part-set bookkeeping. New config consensus.peer-data-rate-limit (default 500, 0 disables): ~500 block-part messages/s of headroom while capping proposals near the vote channel's verification budget (~100/s). drop=true, same non-blocking semantics as L1. Burst (dataRateBurstFor) is floored at proposalTokenCost: rate.Limiter.AllowN permanently rejects any request larger than the burst, so a burst below a proposal's cost would make the node silently never admit a proposal and stall consensus. Design note: this deviates from a simpler flat generous limit; weighting was chosen so block-part gossip keeps full headroom while proposals are still bounded (a flat limit high enough for parts would leave proposals effectively unbounded). Tests (run, green): internal/consensus/data_rate_limit_test.go — proposal flood dropped past budget; block-part gossip unaffected; burst admits the most expensive message; disabled at 0. Green across internal/consensus, config, p2p/client. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(consensus): verify block signature before vote-extension signatures (L6) In the precommit add-vote path, extension signatures were verified before the block signature. Extension verification costs one BLS pairing per threshold-recoverable extension and does not short-circuit, so a peer that cannot produce a single valid signature could still force the whole set of pairings (bounded to 32 by L5, but still 32x a single check). Add Vote.VerifyBlockAndExtensionSigns, which authenticates the block signature first and verifies extension signatures only on success. A forged precommit is then rejected for the cost of one pairing. The set of votes admitted is identical to before (a MatchesFullVerifyAdmission test pins this) and the ABCI VerifyVoteExtension call remains gated exactly as before — only the work done before rejecting a forged vote changes. Marginal once the validator-set vote gate lands (only validators can send precommits then), but cheap, localized, and correct. Tests (green): types/vote_test.go — short-circuits on bad block signature; still rejects a valid-block/bad-extension vote; accepts a valid vote; admission matches unconditional verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): gate votes to validator-set senders (the vote-flood fix) This is the root-cause fix for the vote-flood DoS. The mitigations (rate limit, extension cap, log/commit hardening) bound a flood; this changes WHO can send one at all. An unprivileged peer with a freely-generated node key could make a validator perform BLS verification simply by sending votes: nothing checked that the SENDER was a consensus participant (the isValidator gate checks the receiver, and a peer's ProTxHash is self-reported, not bound to its handshake key). Drop votes whose authenticated node ID (envelope.From, bound to the p2p handshake public key) is not in the current or previous validator set. Only validators legitimately send or relay votes — a node gossips a vote to a peer only when that peer is a validator (shouldVoteBeGossiped) — so this rejects nothing honest while restricting the expensive vote path to actual quorum members. In Dash terms the attacker must now own an evonode (collateral-backed, identifiable, PoSe-bannable) rather than any host with a node key. Design (see docs/MASTERNODE_VOTE_GATING_SPEC.md §10): - Set = current Validators UNION LastValidators node IDs. The union covers the quorum-rotation boundary, where late votes for the previous height arrive from the previous quorum. The binding is already in-process (validator.NodeAddress.NodeID, from the ABCI ValidatorUpdate URI) — no Dash Core poll, no new dependency, no registry. - Drop-only, no disconnect (avoids evicting a peer during a rotation window). - Fails OPEN on ANY incomplete identity coverage: if any member of either set lacks a node ID, the gate is disabled entirely rather than risk dropping that validator's votes. The gate is either fully informed or fully off, so a false positive (dropping an honest vote and partitioning the network) is impossible. Current Dash Platform never produces mixed sets (platformNodeID is mandatory for every validator), but generic/non-Dash and transitional states are handled safely. Only VOTES are gated. Commits are not (non-validators relay them for catch-up); they are handled by the per-peer rate limit and the invalid-signature disconnect. Tests: internal/consensus/vote_sender_gate_test.go (red->green) — current and previous-set validators allowed; non-validator dropped; fail-open with no node IDs, at genesis (nil LastValidators), and on a MIXED set (any node-ID-less member) in either set. The full internal/consensus suite passes unchanged. Recommended before release: testnet / e2e validation across a quorum rotation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: bring vote-flood + gating specs up to date with the implementation The vote-flood spec still said "sent back for rework, none may be implemented" — stale now that every layer plus the root-cause validator-set vote gate is built, tested, and reviewed. Update its status to IMPLEMENTED with a table mapping each shipped layer to its commit, and note what was deliberately deferred. Add MASTERNODE_VOTE_GATING_SPEC.md to this branch (the gate's design doc, which belongs with the gate code) and mark it IMPLEMENTED. Both specs now reflect the final validator-scoped design and carry the testnet-validation caveat. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): harden vote/commit signature verification against extension amplification Block-signature short-circuit before vote-extension signatures; typed ErrVoteExtensionCountMismatch so a count/version mismatch is not treated as evictable forgery; vote-extension cap enforced on the verification path only, never on signing. Red-green tests included. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * wip(consensus): vote-gate fail-open metric + processMsgCh log hygiene SEC-006 gate observability (VoteGateFailOpen metric, transition-only Warn) and SEC-008 envelope-dump log downgrade. Gate observability is superseded by the DoS-throttle rework (gate is being removed); log hygiene is kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(consensus): DoS defense plan, per-message analysis, and review trail Reviewed lean throttle plan (Phase 1) + parallelization follow-up (Phase 2), the per-message DoS matrix and findings, open-questions log, and the Codex review trail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): bound peer signature verification Add a shared node-wide 300-operation-per-second token bucket with a bounded burst sized for both current verification passes of the largest accepted precommit. Acquire permits immediately before the block signature check, then acquire extension permits only after the block signature succeeds. Apply the shared budget to peer Votes and Commits while bypassing local and replayed messages. Budget exhaustion is surfaced as a non-punitive, flood-safe drop and cannot be classified as forged commit evidence. Test would have caught this in CI: ✖ before fix (budget APIs and config were absent), ✔ after (staged-permit, affected-package, lint, and build checks pass). Co-Authored-By: OpenAI Codex <noreply@openai.com> * test(consensus): add DoS attack-surface audit POCs Empirical grounding for the DoS defense plan. Each test documents a concrete attack surface and passes by demonstrating current behavior: - HeightVoteSet unbounded round allocation: a peer mints two new rounds per 32 bytes of forged proTxHash, rejected without BLS work (memory cost without CPU cost). - Per-node-identity rate-limit reset: the per-peer limiter is keyed on the p2p node ID, so a fresh Ed25519 keypair resets the budget; the cap is per-keypair, not per-attacker. - Burst-clamp permanent drop: a cost-weighted rate limit silently and permanently drops any single message heavier than the token bucket's burst, halting the chain if the app returns more extensions than burst. - Commit verification cost vs extension count: measures victim CPU per attacker byte, the denomination the rate-limit default must use. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): refine verification budget handling Preserve verification-budget exhaustion as local overload on the Vote path, count live remote Vote and Commit budget drops, and keep wrapped budget errors out of peer-eviction handling. Test would have caught this in CI: ✖ before fix (Vote budget denial was wrapped as an invalid signature and metric wiring was absent), ✔ after (the regression and metric classification tests pass). Co-Authored-By: OpenAI Codex <noreply@openai.com> * docs(consensus): piece 2 spec - verification fairness Design for the required fairness invariant: per-peer lanes drained by deficit round robin, so an attacker flood cannot starve honest consensus traffic out of the shared verification budget added in piece 1. Reviewed by four independent lenses before implementation. Notable outcomes: - Prices every message type on the peer queue. The queue carries Proposal and BlockPart as well as Vote and Commit, so pricing only the latter two left the scheduler's own bounds unprovable and left proposal verification as an unbudgeted pivot that a handful of peers can use to saturate the verifier. - Drops the escrow/lease design in favour of a whole-message affordability check on the state goroutine. The token bucket cannot express partial settlement, a lease cannot ride a message that is also a WAL record, and the reservation leaked across the scheduler handoff where a single leak halts the peer path. - Records what the piece cannot deliver: honest share is bounded by connection slots, not by scheduling, so raising it is folded into piece 3b. - Names the existing full-queue error path that would turn a routine lane overflow into peer eviction, which bounded lanes make reachable. Also surfaces two new Phase 1 items: the unprotected evidence channel, and whether removing the double verification must move out of Phase 2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(consensus): fix starvation flaw in piece 2 budget admission Adversarial re-review of the admission mechanism found it broken for the very property the piece exists to guarantee. Gating dispatch on "can the bucket cover this message, else drop" is biased against expensive messages absolutely rather than proportionally. The bucket is a level, not a queue, and a dropped message costs microseconds, so a saturated supply of cheap messages pins the level below what an expensive one needs and it is never admitted. Round robin cannot compensate: it allocates turns while the check allocates tokens, and a turn granted then discarded is a turn wasted. The mechanism is fair only when all offered costs are equal, which is precisely not the adversarial case. Replaced with bounded, context-aware waiting. Because every draw is serialized on one goroutine, blocking for tokens is itself the reservation, which buys what the rejected lease design was reaching for without any lease. The staged draws also wait rather than fail, so whole-message atomicity holds by construction instead of by argument. Also restates the fairness invariant over admitted verification rather than dispatch. As worded it stayed true while messages were discarded, so it could not fail even as the chain stopped making progress; the test now asserts honest completion latency, and a regression test pins the rejected behaviour. Records three implementation traps found in the process: cancelling a token reservation refunds nothing once its deadline passes, the limiter's own wait consults real time and would defeat the test clock, and equal burst and worst-case cost admit a maximum message only on a fully idle node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(consensus): charge the per-peer vote budget in verification work Implements docs/CONSENSUS_DOS_PIECE2_SPEC.md §8 commit 1 (the §5a work plus the §7 clock-seam prerequisite). The per-peer vote-channel limiter charged one token per message, so a peer could buy a precommit carrying 32 vote extensions — up to 66 BLS signature verifications — for the same price as a prevote. The budget therefore bounded message count, not the CPU it was introduced to bound. Price every message a peer can put on a consensus channel in verification work, per the spec's §5a table: prevote 1; nil-block precommit 1; non-nil precommit with n extensions 2*(1+n) (two verification passes), capped at 66; commit with n extensions 1+n, capped at 33; proposal 1; block part 1. The cost is derived from the message's declared contents alone, so it is charged before conversion, validation or verification. A message declaring more extensions than any legitimate participant produces is refused a price and dropped locally — never reported as a peer error, so neither a protocol revision nor a bug in this model can evict honest peers. Floor the per-peer burst at that worst-case cost. This fixes a live hazard: rate.Limiter rejects any request larger than the burst no matter how long it waits, so with burst = 2 * limit any peer-vote-rate-limit below 33 would make a fully-extended precommit permanently inadmissible — every validator's precommits silently dropped and no block ever committed. Raise the peer-vote-rate-limit default to 600, now denominated in verification work rather than messages. A Dash-realistic precommit (four extensions) costs 10, so this admits ~60/s from one peer. Provisional pending the load test. verificationBudgetBurst now derives from the same constant instead of repeating the expression, so the global burst and the message cost cannot drift apart. Prerequisite for the spec's §7 test plan: the p2p rate limiter read time.Now() directly, so refill behaviour could only be inferred from sleeps. It now meters through an injectable clockwork.Clock (the abstraction already used by the blocksync synchronizer and the p2p client), defaulting to the real clock — production behaviour is unchanged. Out of scope, deliberately: the §5b affordability peek and minimum-rate validation, and the §5c lanes/DRR. Red → green: * TestAllowVoteChannelMessage_ChargesVerificationCost — ✖ "1201 is not greater than 12000" (prevotes and maximum-cost precommits were admitted in equal numbers) → ✔ * TestAllowVoteChannelMessage_CostSharesOneBudget — ✖ → ✔ * TestAllowVoteChannelMessage_UnpriceableMessageDroppedLocally — ✖ → ✔ * TestVoteRateBurstAdmitsMostExpensiveMessage — ✖ against the old burst = 2 * limit formula ("1 is not greater than or equal to 66") → ✔ * TestRateLimit_RefillsOnInjectedClock — ✖ (undefined: WithRateLimitClock) against the previous ratelimit.go → ✔ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): make whole-message verification affordable by waiting Implements docs/CONSENSUS_DOS_PIECE2_SPEC.md §8 commit 2 (§5b, plus the §5d placement argument). Lanes/DRR (§5c) stay out of scope. Piece 1 charges the node-wide verification budget in stages while a message is verified: a remote precommit draws 1 then n, twice over. A later stage can therefore be denied after earlier stages already paid for a BLS pairing and the ABCI VerifyVoteExtension round-trip, wasting the work and dropping a valid vote. Before a peer Vote or Commit is dispatched, wait until the budget can cover that message's own cost — from the piece-1 cost model, never the protocol maximum. Since every draw is made by the single consensus goroutine, blocking for tokens is itself the reservation: nothing can take them while we wait, so the staged charges that follow cannot fail (I3, now true by construction rather than by argument). Waiting, not peeking-and-dropping. An earlier draft gated dispatch on "does the bucket cover W right now, else drop". That is broken: the bucket is a level, not a queue, and a drop costs microseconds, so demand-saturated cheap messages pin the level below what an expensive honest message needs and it is never admitted. The new cost-bias test pins exactly that failure. The wait is bounded and selects on ctx.Done(). A non-ctx-aware wait would deadlock shutdown (readQueueMessages defer -> fanIn -> stop() -> receiveRoutine never returns); an unbounded one would stall consensus behind an over-subscribed budget. Both give up non-punitively — never a PeerError — since local overload says nothing about the sender. maxWait is 2*C_max/R (440ms at defaults), a safety net rather than a working limit: refilling the most expensive message from an empty bucket takes C_max/R = 220ms, so that is the real bound on how long the goroutine is held. It is implemented as the outermost msg middleware rather than by widening types.VerificationBudget or plumbing a ctx into types/: the state goroutine's dispatch boundary is the only place that has both the ctx and the whole message, and withMiddleware makes the last-appended middleware outermost, so a shed message costs zero WAL writes — asserted, not assumed. rate.Limiter.WaitN is avoided (it reads the wall clock internally and would defeat the clock seam), as is ReserveN+Cancel (CancelAt refunds nothing once timeToAct has passed, silently double-charging). The wait is built from TokensAt plus the injected clockwork clock, which the budget now meters against. Burst is no longer a coincidence. verificationBudgetBurst was exactly C_max, so a maximum-cost message was admissible only on a completely idle node. It is now 2*C_max, and B > C_max is a compile-time assertion: MaxVoteExtensions invites being raised, and at 64 a burst left behind would make that message permanently inadmissible. Validate a minimum verification-rate-limit. 0.5 was legal (only NaN/Inf/negative were checked) and would mean minute-scale stalls; the limit must now be 0 (disabled) or at least the cost of the most expensive single message, so the bucket refills it within a second. Red -> green (peek-and-drop counterfactual: waitFor replaced by "TokensAt >= cost, else drop"): * TestVerificationBudgetWaitAdmitsExpensiveMessageUnderCheapFlood — ✖ "expensive message starved in round 0: cheap traffic must delay it, not exclude it" → ✔ * TestVerificationBudgetWaitAdmitsMessageAfterRefill — ✖ "a message that is not affordable yet must wait for the refill, not be dropped" → ✔ * TestVerificationBudgetWaitCompletesStagedPrecommitDraws — ✖ "expected []int{1,4,1,4}, actual []int(nil)" → ✔ Red -> green (piece-1 baseline: middleware wait disabled): * TestVerificationBudgetWaitCompletesStagedPrecommitDraws — ✖ "expected []int{1,4,1,4}, actual []int{1,4}", with the node logging "rejected peer message: verification budget exhausted" mid-verification → ✔ * Whole new API absent → ✖ "undefined: withVerificationBudgetClock", "inner.maxWait undefined", "undefined: config.MinVerificationRateLimit" Also covered: ctx cancellation returns promptly and does not hang shutdown of receiveRoutine; a deadline drop writes nothing to the WAL, queues no peer error and verifies nothing, while the same message admitted does write one WAL record; every budget charge is made on the receiveRoutine goroutine; a commit charges [1, n] and its admitted cost covers it; B > C_max; minimum-rate validation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): decouple the per-peer vote burst from the configured rate Review follow-ups on the piece-2 cost model (5056628, 2064219), per docs/CONSENSUS_DOS_PIECE2_SPEC.md §5a and §7. Burst is no longer a multiple of the rate. voteRateBurstFor returned max(2*limit, C_max), so the new default of 600 work/s gave every peer a 1200-unit bucket: ~1200 cost-1 messages or 18 maximum-cost precommits front-loaded in a single instant, and 64 connection slots exposed 76800 units — nearly four times the 20000-message consensus queue they share. Since a peer starts with a full bucket, that is what one reconnect buys. The burst is now an independent work allowance sized from honest demand (voteRateBurstCatchUp = 200: ten vote-channel messages a second at the ten work units a four-extension Dash precommit costs, i.e. about two seconds of an honest peer's heaviest gossip), with C_max kept as a floor so the most expensive protocol-legal message can never become permanently inadmissible. All 64 slots now front-load 12800 units, below the queue. peer-vote-rate-limit keeps its default of 600. Message pricing is exhaustive. peerMessageCost priced any unrecognised message at the floor of 1, so a message type added later would be admitted at an invented price with nothing to notice it. Every type in the consensus wire union is now priced explicitly and an unknown type is refused a price — dropped locally, never a PeerError, since a version skew says nothing about the sender. The doc comment now states the contract narrowly: the cost covers the direct staged signature verifications only, not threshold recovery/interpolation over a vote set (types/vote_set.go) and not the ABCI VerifyVoteExtension round-trip. The clock seam covers the waiting path. WithRateLimitClock only reached AllowN, access timestamps and GC; with drop=false the limiter still called WaitN(ctx, 1), which reads the wall clock internally (so an injected clock could not release it) and discarded the caller's nTokens entirely, silently dropping cost weighting on that path. It is replaced by a ReserveN-based wait metered against the injected clock, charging the full weight and refunding the reservation when the caller's context is cancelled. The prevote row of the cost table is now documented as the conditional it is: charging 1 regardless of declared extensions is safe only because types.Vote.ValidateBasic rejects extensions on anything but a precommit for a real block, and MsgFromProto runs it before dispatch. A test pins that dependency so deleting the validation cannot silently turn a prevote into a 33x undercharge. Also drops release-status narration from the peer-vote-rate-limit default comment; the provisional status of 600 lives in the spec. Red → green: * TestVoteRateBurstDoesNotScaleWithLimit — ✖ "expected 1200, actual 12000" at a tenfold rate, and 120 at a tenth → ✔ * TestVoteRateBurstBoundsFreshPeerAggregate — ✖ "76800 is not less than 20000" → ✔ * TestPeerMessageCost_RefusesUnrecognisedMessage — ✖ "An error is expected but got nil" (priced at the floor) → ✔ * TestRateLimitWaitPathChargesNTokens (was TestRateLimitWaitPathIgnoresNTokens, which pinned the defect) — ✖ "9999.015583 is not less than 5100" for a 5000-token message → ✔ * TestRateLimit_WaitPathRefillsOnInjectedClock — ✖ hung until the 2-minute test timeout: the wait never registered on the injected clock → ✔ * TestPrevoteWithExtensionsRejectedBeforeVerification — passes as a guard; verified non-tautological by deleting the "unexpected vote extensions" check in types/vote.go, which turns it ✖ "An error is expected but got nil" Verified: go build ./... clean, go vet clean, config, internal/p2p/client and internal/consensus green with -race. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(consensus): record the chosen per-peer vote burst The spec asked for a burst sized independently of the rate but never named a value, leaving it out of sync with the implementation. Record 200 work units and the reasoning: it covers roughly two seconds of an honest peer's heaviest gossip while keeping the aggregate a fresh peer set can front-load below the shared queue's capacity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(consensus): fold piece 2 review findings back into the plan The fairness review surfaced two things the plan did not account for. The evidence channel is undefended and was owned by no piece: two signature verifications per message, no rate limit at all, and de-duplication by hash so that flipping one signature byte forces the work to be redone. Added as a Phase 1 item. Validator connection hardening also has to raise how many honest peers we connect to, not only stop the ones we have from being evicted. We dial twelve of sixty-eight slots and the rest can be taken from a single host with free identities, so the honest share is capped no matter how fairly the work is scheduled. Records the fairness form decision and its arithmetic, and adds an open item on whether removing the double verification has to move out of Phase 2, since honest demand alone already exceeds the budget in normal operation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p2p): reserve connection slots for current-quorum validators Validator connection protection was attached to the dial action rather than to the validator's identity, which left it absent exactly where it matters. `routerDashDialer.ConnectAsync` sets `MutableScore = PeerScorePersistent` but never the `Persistent` flag, so `Score()` caps at `MaxInt16-1`, the value is ephemeral, and every peer error decrements it. Worse, `ConnectAsync` is only reached for validators we dial: `filterAddresses` skips any validator that is already connected, and DIP-6 is a directed overlay, so the neighbours at `(i-2^k) mod n` only ever connect inwards — as does every neighbour after a restart, since the remote retries at 250ms and wins the race against our startup. Those validators sit at score 0, and `findUpgradeCandidate` needs only a strictly lower score, so one `PeerError` (score -1) let any fresh score-0 Sybil evict them. `updateConnections()` runs only on validator-set updates, so the evicted validator was not redialed until the next quorum rotation. Introduce a reserved connection slot, keyed on the node ID the handshake authenticates. `PeerManager.SetProtectedPeers` replaces the reserved set; `configurePeer` derives `peerInfo.Protected` from it, so an inbound connection from a never-before-seen quorum member is covered on arrival. A reserved slot ranks as persistent, so it cannot be eroded by error scoring or dial failures, cannot be selected for eviction to make room for another peer, and wins an upgrade when the node is full. `ValidatorConnExecutor` publishes the set for the quorum members it must stay connected to, whichever side dials. Two findings from spec review are folded in: - `router.routeChannel` disconnected *any* erroring peer whenever `len(connected) >= MaxConnected`, ignoring rank — and a flood is precisely what puts a node at its ceiling, so this alone would have kept the one-error eviction alive. `ShouldDisconnectOnError` now spares peers holding a reserved slot from pressure-driven shedding; fatal errors still disconnect them. - Reservations are granted only to node IDs bound to the address the chain published (inline in the validator address, or from the TCP resolver's secret-connection handshake). `resolveNodeID` also answers from the address book, which peer exchange lets any peer write, so an attacker could otherwise PEX-inject `attackerNodeID@validatorIP:port` and be handed the validator's slot. Address-book answers remain good enough to dial. Design, verification of each claim against the code, and the rejected alternatives (per-/24 conn-tracker keys, raising MaxOutgoingConnections, a numeric inbound reservation) are in docs/CONSENSUS_DOS_PIECE3B_SPEC.md. Tests would have caught this in CI: TestProtectedValidatorSurvivesFloodErrorsAndRestart score -100 -> persistent TestProtectedValidatorDisplacesFloodOnInbound refused -> admitted TestProtectedPeersReplacedOnQuorumChange 0 -> persistent TestReservedSlotIsNotShedUnderConnectionPressure shed -> kept Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(dash): reserve slots for the inbound half of the DIP-6 overlay Reserving slots only for the validators this node dials protects half of its quorum neighbourhood. DIP-6 is a directed overlay: the member at sorted index i dials (i+2^k)%n, so the members that dial it are the ones at (i-2^k)%n — a different set, which `selectValidators` never returns and which is therefore only ever known through the connection it opens. Those are exactly the slots a connection flood takes, and the honest share stays pinned to the outbound half. Add `SelectInboundValidators` as the inverse of `SelectValidators`: same sorted list, same offsets, walked backwards. The two share one implementation parameterised by direction, so they cannot drift apart, and the index arithmetic moves from float to integer because Go's % keeps the sign of the dividend and a negative offset needs the result brought back into range. `ValidatorConnExecutor` now reserves the union of both halves. A failure to compute the inbound half keeps the outbound reservations rather than dropping everything. The union is small and bounded: at most 2*floor(log2(n-1)) identities, 12 for a 100-member LLMQ and fewer where the halves overlap (at n=64, -32 is +32 mod 64, giving 11), and at most n-1 <= 3 below the DIP-6 minimum, which is what keeps 4-node testnets safe. Guaranteed honest connection slots under a flood go from 0 — every neighbour was removable with one peer error before the previous commit — to the whole 12-member DIP-6 neighbourhood, plus the 6 outbound slots an attacker can only reach by poisoning the address book: >= 18 of 68. Test would have caught this in CI: the inverse property test (j in forward(i) <=> i in inverse(j)) fails on every quorum size at or above the DIP-6 minimum when the inbound selection is anything but the true inverse. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(p2p): harden slot reservations after review Independent security and liveness review of the previous two commits found three ways the mechanism could be turned against the node it protects. Reservations no longer run node ID resolution at all; they use only the node ID the chain published with the validator address. Resolution consults the address book first, and peer exchange lets any connected peer put an arbitrary node ID at an arbitrary address there, so an attacker could advertise `attackerID@validatorIP:port` and have the resolver name its own identity for a real validator — either claiming the slot or, since the answer was then treated as unauthenticated, stripping the reservation from that validator on demand. A per-target kill switch on the mitigation, for one PEX message. Resolution is also far too expensive to run here: `WithValidatorSetUpdate` publishes the full validator set on every committed block, so `updateConnections` runs per block, not per rotation. Resolving the inbound half would have added a blocking 1s dial plus 1s handshake per unresolved member, per block, on the event-bus subscriber goroutine — for peers this node never dials. A subscriber that falls behind is evicted by the pubsub server and never resubscribes, which would have frozen the reserved set on a rotated-out quorum. A rejected reservation set is now dropped instead of leaving the previous one in force, which was stale protection of exactly the kind keeping reservations out of the peer store is meant to prevent. A malformed node ID no longer costs the rest of the set its reservations, and the size ceiling now counts the configured persistent peers that hold a slot for the same reason. Score changes are no longer recorded for a peer holding a reservation. They were invisible while it lasted but still accumulated, so an attacker that could not shed a reserved validator could bank errors against it and have it drop to the bottom of the ranking the moment its quorum rotated out. Also: remove `HasMaxPeerCapacity`, dead since the previous commit replaced its only caller. Review found one test that could not fail — the hard-ceiling test passed with protection removed, because the flood peers it left at score -1 could be out-ranked by anyone. It now restores them to a clean rank first, so only a reservation can win the admission. New tests cover the rejected-set, malformed and self ID paths, and the seed-node incoming-connection timer. One review suggestion was rejected: keeping a connection to a member that moved to the half of the overlay that dials us. Holding an outbound connection to a peer DIP-6 says should dial us contradicts the overlay's semantics, and the existing executor tests encode that. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): serve peer messages in per-peer lanes by deficit round robin Every peer shared one arrival-ordered queue of 20 000 slots, drained by a single reader, so service was proportional to how much a peer sent. A peer flooding cheap messages could hold up the vote the node needed to make progress, however honest its sender. Give each peer a lane of its own and serve the lanes by deficit round robin. Plain round robin is not enough here: message cost varies by two orders of magnitude, so a rotation blind to cost lets a lane of maximum-cost precommits take 66 times an honest lane's work per turn. A per-lane deficit counter with a one-unit quantum makes the rotation fair in verification work instead of in message count, and bounds what other lanes can complete ahead of a head of cost W at lanes x max(quantum, W). Also restores the invariant that internal progress is independent of the peer path. The affordability wait introduced in 206421997 ran on the state goroutine because there was no scheduler to put it on; that goroutine also serves this node's own messages and the timeout ticker, so a saturated budget delayed timeouts by up to C_max/R against a one-second vote timeout. The wait now runs on the scheduler, and the state-goroutine check remains only for messages that did not come through the scheduler. Handing over one peer message at a time is what makes reading the budget on another goroutine sound, since a message's charges are made while it is verified. And fixes the non-punitive invariant. chanQueue.send reported a full queue as an error, which reactor.go turned into a p2p.PeerError and peerManager.Errored turned into eviction. With 20 000 shared slots that path was near-dead; with bounded lanes it becomes routine, and the peer that reaches capacity at full load is the honest one. Shedding and a cancelled context now both return nil. Lane capacity is 512 messages: a whole 21 MB proposal block arrives as 337 parts, and a peer's instantaneous vote allowance is 200 work units of the cheapest messages. The lanes are bounded in aggregate at the size of the queue they replace, so per-peer scheduling does not raise the memory a flood can pin; once several dozen lanes are busy that aggregate binds first, and a single peer can no longer hold a whole block's parts. Lanes are retired when a peer disconnects and reclaimed after 60 seconds idle, so an attacker cycling through free node identities cannot grow the rotation without bound. Overflow drops the oldest message, never the arriving one: consensus messages are only valid for the height and round they were made in, so dropping the newest would spend a peer's turns on guaranteed-stale messages and mute it for rounds with no attacker involved. Implements docs/CONSENSUS_DOS_PIECE2_SPEC.md §5c, restores I6 and fixes I4. Test evidence, per invariant: - I4: TestSheddingAPeerMessageNeverReportsThePeer/lane_overflow fails against a send that returns "msg queue is full" ("the peer was reported for a message this node chose to shed"), passes here. - DRR: TestPeerLanesBoundWorkAheadOfAHead/"cheap head, expensive flood" fails against cost-blind round robin ("528" is not less than or equal to "8"), passes here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(consensus): prove peer scheduling holds end to end The lane unit tests exercise the rotation directly. These drive real messages through the reactor, the scheduler, the write-ahead log and the consensus state machine, so they assert what the invariants are actually about rather than what the scheduler reports. - An honest peer's vote completes VERIFICATION — enters the vote set, not merely gets dispatched — while 67 attacker lanes hold the verification budget at zero with forged commits, within the round timeout it belongs to. Both a cheap head and the heaviest precommit Dash validators produce, since only the expensive one distinguishes a scheduler that waits for budget from one that passes over what it cannot afford right now. - Shedding a peer message, whether at the lane or at the peer's own rate limiter, reaches neither the reactor's error path nor the write-ahead log nor the vote set, while a genuinely malformed message from another peer is still reported — so the test cannot pass by never reaching the scheduler. - This node's own messages and its round timeouts are served while the peer path is blocked on the verification budget. - A peer message the dispatcher finds no handler for still lets the scheduler serve the peer's next message. - The node stops while the scheduler is parked on the verification budget. - A peer going down retires its lane, and reconnecting starts it over. Timing is asserted against a fake clock advanced at exactly the rate the verification budget refills, so what the tests measure is the delay the budget imposes and nothing else. Work that rides inside a dispatch without being charged — threshold recovery and the vote-extension round-trip — is invisible to that measurement; bounding it is what the piece's load test is for. Red-before-green evidence: - The honest-vote test fails against a budget check that peeks instead of waiting ("the honest peer's vote was never verified while other peers flooded"), which is the design docs/CONSENSUS_DOS_PIECE2_SPEC.md §5b rejects, and passes here. - The dispatcher-drop test fails when the finished-message report is made from the middleware instead of the dispatch loop, because a message that reaches no middleware then never reports and the scheduler stops serving peers for good. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): fold peer-scheduling review findings back in Findings from independent liveness, concurrency and spec-conformance reviews of the per-peer scheduler. Correctness and robustness: - A budget that loses the ability to defer a message is now a compile error. Both callers ask for it through a type assertion and quietly stop deferring when it fails, which would give up whole-message atomicity with every test still green. - The reactor retires a lane outside its own lock, so the lane lock stays a leaf and cannot take part in a cycle if a future handler ever queues a message while holding the reactor's. - The consensus goroutine now stops the message queue when it leaves on a dispatch error, instead of stranding the queue's reader goroutines. Tests that asserted less than they claimed: - The aggregate bound over all charges is asserted against the real bucket over the flood, which matters more now that the budget is read on the scheduler goroutine and charged on another: the bound rests on the handoff between them rather than on one goroutine doing both. - The flood's premise is asserted rather than assumed: a commit an attacker cannot make good on is charged exactly one verification, and it is that price that decides how many turns it takes to hold the budget down. - Two assertions in the shed test could not fail, since nothing consumes the queue there. Replaced with what that setup can actually show, and the write-ahead log claim moved to the budget test, where a message really is dropped between the queue and the log. - The honest-vote test waited four minutes for a failure, twice, which is longer than the repo's own test timeout — a regression would have shown up as a timeout rather than as the assertion message. Now thirty seconds. Comments that were wrong or claimed too much: - The wait for budget takes no tokens, so it is not the reservation the old comment described; what makes the charges that follow safe is that only one peer message is ever in flight. - A lane's capacity is not a reservation: the aggregate bound is the smaller of the two once several dozen lanes are busy, so under a wide flood a single peer can no longer hold a whole block's parts and the round times out. - The deficit bound scales with the head's own cost. The heaviest precommit Dash validators produce is served inside the propose timeout but not inside the vote timeout; the test now says so instead of quietly grading it against the looser one. - Drop-oldest is argued from time-validity, which holds for votes and commits but not for block parts; the block-part case is now argued on its own terms. - The rotation is bounded at every instant, but the lane table only across a reclamation interval. The identity-rotation test claimed both. - An expensive head against a cheap flood bounds the wait but does not tell deficit round robin apart from plain round robin, which serves such a head sooner. Only the cheap-head-against-expensive-flood case does. - Which functions require the lane lock, that the scheduler's clock does not reach the budget's, and that routing every message with a peer to the peer side is what the finished-message report depends on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(consensus): correct the fairness bound to account for message cost The stated bound assumed every competing message was the cheapest possible, and was then used as the acceptance criterion. The real bound scales with the head message's own verification cost, so a realistic precommit lands around two seconds rather than the fifth of a second quoted, which is longer than the vote timeout. No work-conserving fair scheduler can do better than the fair share, so the criterion as written could never have been met. The measured value is now recorded, the test grades against the propose timeout, and the three inputs that can actually close the gap are named: a higher verification rate, fewer lanes an attacker can hold, or removing the second verification pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(consensus): charge proposal verification to the verification budget Proposal signature verification is one BLS pairing on the consensus goroutine, and it was the one peer-reachable pairing the node-wide verification budget did not price: budgetedMessageCost returned 0 for a proposal, so the peer scheduler never gated one and nothing drew a permit before the check. Nothing de-duplicates a proposal that fails verification either -- rs.Proposal is set only by a proposal that passes -- so every forged copy is verified again. The per-peer data limit admits ~100 proposals/s/peer (PeerDataRateLimit 500 / proposalTokenCost 5) with a 200-message burst, so a handful of peers saturate a ~370 pairings/s verifier while the honest proposer's message waits behind rotating lanes, keeping rs.Proposal nil and the flood self-sustaining. Price a proposal at one work unit and draw the permit where the pairing is, so the gate the scheduler applies and the work actually drawn agree. The permit is taken after proposer selection (the non-validator path runs no pairing and is not charged) and before the sign-digest derivation, so that hashing is inside the bound too. Refusal returns the existing budget-exhausted sentinel, which the logging middleware already treats as a non-punitive local drop: no peer error, no WAL record. Red -> green: TestProposalFloodCannotExceedTheVerificationBudget dispatches 50 forged proposals against a drained budget. before: 50 WAL writes, 50 signature checks after: 0 WAL writes, 0 signature checks; one admitted after refill Refs docs/CONSENSUS_DOS_PLAN.md §1, §2.3; docs/CONSENSUS_DOS_PIECE2_SPEC.md §5a. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(consensus): stop peer-triggerable rejections from writing Error logs Two log-amplification leaks on the proposal and block-part paths. A proposal that fails signature verification wrote an Error line that embedded the whole proposal — an attacker-controlled, unbounded payload — and nothing de-duplicates a failing proposal, so one line was written per copy. The line is now Debug and carries only the round and the key the check was made against; nothing that came off the wire. Block-part proof failures, and the remaining proposal rejections (POL round, core-chain height, blockID-vs-commit mismatch, unverifiable proposal), all reached the logging middleware unclassified and were written at Error. Every one of them is free for a peer to produce, so they join the floodable set and are logged at Debug. Classification only selects a log level; it does not change whether a peer error is raised, and neither of these paths raises one. Red -> green: TestProposalRejectionsAreFloodable, TestBlockPartRejectionsAreFloodable and TestRejectedProposalIsNotEchoedIntoTheLog all fail before the change (8 unclassified errors; an Error line carrying the forged signature). Refs docs/CONSENSUS_DOS_PLAN.md §2.7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(p2p): fail a rate-limit wait fast when the caller cannot wait it out Three review follow-ups. The rewritten wait path lost rate.Limiter.WaitN's fast-fail. WaitN reserves against the caller's remaining context budget and refuses up front when the delay exceeds it; the rewrite reserved without a limit and then blocked on the timer or ctx, so a caller with a short deadline was parked for the whole deadline — holding the reserved tokens, and on a shared channel goroutine every message behind it — only to be refused anyway. It also lost the distinction between "the bucket cannot serve you in time" and "you were cancelled". Restore both: compare the delay against the deadline, cancel the reservation (which refunds in full, since it has not come due) and return a distinct error. The fresh-peer burst aggregate test used 64 connections. The peer manager may hold MaxConnected plus MaxConnectedUpgrade at once, and node/setup.go passes 4 for the latter, so the ceiling is 68; the burst comment in the consensus reactor claimed 64 for the same reason. Narrow two comments that claimed more scope than the code delivers: the data-channel limiter and proposalTokenCost bound what ONE peer can spend, not the aggregate across peers, which is the node-wide verification budget's job. Red -> green: TestRateLimit_WaitFailsFastPastTheCallerDeadline before: returns after 301ms wrapping context.DeadlineExceeded after: returns up front with the limiter's own refusal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): bound the hashing a peer can force with bad block-part proofs The part-set caps bound how many parts a node ACCEPTS. They say nothing about the ones it rejects: a proof that does not check out leaves the slot empty, so the next copy aimed at the same in-range index is hashed from scratch, and verifying a proof hashes the whole ~64 kB leaf before it can find the mismatch. Nothing charged for that work. Add a per-peer allowance denominated in the bytes of leaf hashing spent on proofs that fail. A part is refused before verification once its sender has spent its allowance; only an actual failure is charged, so a peer whose parts verify never draws on it. Per peer rather than node-wide, deliberately. A node-wide bucket would let one peer's garbage stop this node assembling a block at all, trading a bounded CPU cost for an unbounded liveness one. The node-wide bound follows from the connection ceiling instead: 68 slots x 256 KiB/s is a low single-digit percentage of one core, asserted by TestBlockPartProofBudgetNodeWideBound. An accepted part clears what its sender has spent. Honest proof failures come in runs -- a round this node has moved past -- mixed with parts that do verify, and it is the mixture that must not cost a peer the parts we need from it. The burst additionally absorbs 32 full-size failures before anything is refused. Refusal is a silent local drop: no error, no peer error, no WAL record, counted on the new consensus_block_part_proof_drops metric. Red -> green: TestRepeatedInvalidBlockPartProofsAreBounded, with the gate disabled, re-hashes the same invalid part 1000 times (65 MB of hashing from one peer, and it does not stop there); with it, the run ends inside the burst. Refs docs/CONSENSUS_DOS_PLAN.md §2.5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(consensus): bound the State and VoteSetBits channels Neither channel verifies a signature, so the node-wide verification budget does not bound them at all; nothing else did either. Three problems, all reachable by any connected peer. Add a per-peer ceiling across both channels AND a node-wide one. Neither substitutes for the other: the per-peer allowance keeps one sender from occupying the channel goroutine, and the aggregate keeps that guarantee from being bought around with fresh node identities, which are free. Both sit well above honest gossip -- a peer broadcasts a HasVote per vote it adds, plus round-step messages and up to four VoteSetMaj23 per gossip tick -- and both drop silently: the peer at its allowance under load is as likely to be honest and gossiping hard as hostile. Price VoteSetMaj23 above the rest. It is the one message here that asks this node for work: it makes us build a bit array over every validator, up to MaxVotesCount bits, and send it back -- more than it cost to ask. Suppress repeats. A peer's gossip loop resends the same claim on every tick, and the answer comes from our own vote set alone, so until that vote set gains a vote the peer has not been told about, answering again conveys nothing. Tracked per peer by claim plus the number of votes the last answer carried; votes are only ever added, so an unchanged count means an unchanged answer, and the peer still learns of every vote we gain. Bound the response send. It went out with the caller's context, so a router that could not take it parked the goroutine serving every peer's state messages. It now has a short deadline of its own and gives up quietly; the peer asks again on its next tick. Red -> green, with each mechanism disabled in turn: per-peer ceiling: 400000 messages admitted from one peer against a 4000 ceiling aggregate: the fresh-identity loop never terminates suppression: an unchanged claim is answered every time bounded send: the test hangs until the 2m timeout Refs docs/CONSENSUS_DOS_PLAN.md §2.4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(evidence): spec bounding the evidence channel Plan item §2.10. Records what was verified against the code rather than assumed, including three findings the plan did not have: the reactor evicts a peer on the first evidence that fails verification, evidence gossip is retry-based (so a receive-side drop is a delay, not a loss), and signature verification is skipped entirely when the validator set at the evidence height carries no public keys. That last one dictates the whole shape of the de-duplication: acceptance alone cannot be enough to remember an equivocation, or an attacker could claim a real equivocation's identity with garbage signatures and lock the genuine proof out for good. Reviewed by independent security and correctness lenses before any code; their findings are folded in and the pre-existing issues they surfaced are recorded in §6 for other owners. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(evidence): de-duplicate evidence by what it alleges, not how it is encoded De-duplication keyed on the evidence hash, which covers the signature bytes, so flipping one byte of a signature produced a piece of evidence the node had never seen and bought the sender a full re-verification — two BLS pairings plus a block-meta and a validator-set lookup — for the cost of one byte. Nothing rate-limited the evidence channel, so the flood was free to repeat. Remember instead what a piece of evidence alleges: the height, round and step, the accused validator, and the two conflicting block IDs. A re-encoded copy of an equivocation we have already proven is then recognised as the same accusation and costs a map lookup. The memory only ever records evidence whose signatures were actually checked. VerifyDuplicateVote skips them when the validator set at the evidence height carries no public keys, and such evidence is stored anyway — so remembering on acceptance alone would have handed an attacker an evidence-suppression tool: equivocations are public, and a forged copy could claim the identity before the genuine proof arrived. Evidence is a safety mechanism; suppressing it is worse than the flood. The set is bounded at 1024 entries and forgets the lowest heights first, which are the ones closest to ageing out of the window anyway. Forgetting only forfeits a free refusal, so it can never cost correctness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(evidence): bound the work an inbound evidence message can cost The evidence channel had no ceiling of any kind. Every message that got past the hash de-duplication paid for two BLS pairings and two disk lookups on the evidence reactor's goroutine, and nothing limited how many a peer could send. Admit inbound evidence through a gate ordered cheapest-refusal-first: votes that cannot describe one equivocation, vote extensions beyond what a real vote may carry, heights the block store cannot serve, and equivocations we already hold proof of are all refused before anything is spent — including before the message is hashed for the pool's duplicate check, which is itself a full marshal and digest of a message that may approach the channel's 1 MB limit. Only what survives is charged, in pairings, against a per-peer bucket and a node-wide one. Genuine evidence still lands. Each peer has its own bucket, so an attacker cannot spend an honest peer's allowance however many identities it holds; the node-wide rate is set above what every peer together may spend, so against a fixed peer set it refills faster than it drains and stops being the binding constraint; and it is inspected before the per-peer bucket is charged, so a congested node never bills a peer for capacity it then denies. Where that is not enou…
…#17 could not Merging v1.6-dev brings in TestReactor_Backfill_UnknownVoteExtensionTypeAbortsUnattributed from #17. It fails, and it should: it asserted the mis-attribution this branch removed. #17 recorded the limitation deliberately rather than as an oversight. Its doc comment spelled out that a commit refused while decoding was dropped, that the fetch could only read the resulting silence as a slow peer, and that the run therefore ended on the retry budget every peer shares with the offending peer unnamed - "enough such answers end the backfill while honest peers are still available". Its assertion said the same in one line: ErrorContains "max retries", explained as "the abort is charged to the shared retry budget rather than to the peer that caused it". An unknown vote-extension type is the third member of the decode class this branch fixed, alongside the out-of-range quorum type and the block-protocol mismatch, so the limitation is gone: the response is now delivered to the fetcher as a refusal, the peer is withdrawn from the run and reported, and the run ends naming the cause. The assertions are inverted, not relaxed. Both original safety assertions are kept - the run errors rather than panics, and nothing reaches the block store - and the "max retries" assertion becomes three stronger ones: the failure names the extension type, it explicitly does not blame the budget, and the peer that sent it is reported by name. Renamed accordingly, since AbortsUnattributed asserts the opposite of what now happens. The report is asserted non-fatal, which this cause makes the clearest case for: an extension type undefined in this build is defined by whatever release introduces it, so a peer failing the check is as likely to be an honest node one release ahead as a malicious one. UPGRADING.md already records that adding a type has exactly this effect. Mutation-checked rather than observed passing: restoring the dropped-decode-error behaviour makes it fail with "max retries to fetch valid blocks exceeded (20); target height: 10, height reached: 15" - the string #17 asserted, produced by the code path #17 described. No production code changed. internal/statesync and internal/blocksync green under -race -tags deadlock; no golangci-lint delta across either package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
REPO_NAME pointed at github.com/dashevo/tenderdash while the module is github.com/dashpay/tenderdash, so -X named a symbol that does not exist. The Go linker ignores an unknown -X target silently, and every build fell back to version.TMVersionDefault — the previous release number. The 1.6.1-alpha images built from this tree all report 1.6.0, indistinguishable from the 1.6.0 release. Correcting REPO_NAME alone is not enough. Once the symbol resolves, a failed `git describe` stamps an empty string, which overrides TMVersionDefault and leaves the binary reporting no version at all; and `git describe` output carries the tag's leading `v`, which 1.6.0 and earlier never reported. So also strip the `v` and skip the -X flag entirely when git produced nothing. Verified by evaluating the three states the build actually sees: on a branch -> ...TMCoreSemVer=unreleased-<branch>-<sha> detached at v1.6.1 -> ...TMCoreSemVer=1.6.1 no tags reachable -> no -X flag; TMVersionDefault stands No test accompanies this: the behaviour lives in Makefile ldflags and the Go linker's handling of an unresolvable -X target, for which the repo has no test seam. The three evaluations above are the evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three operator-facing notes for this release sat under two headings that cannot survive a release: two Tenderdash sections with no version at all, and a "## Unreleased" heading filed under the legacy Tendermint Core half of the file, which predates the fork and is kept only for historical reference. Group all three under a single `## v1.7.0`, and move the vote-extension type validation note out of the Tendermint Core section, where it never belonged. Text is unchanged; only heading levels and placement move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…clock TestAllowDataChannelMessage_DropsProposalFlood asserts an upper bound on how many proposals a flood may admit: burst/proposalTokenCost + 1, or 201. The limiter is a token bucket metered against the wall clock, so the bound only holds if the 5000-message loop takes almost no time. On a loaded CI runner it does not, the bucket refills mid-loop, and the count drifts past the bound. CI observed 202. Measured against the injected clock, admitted counts are: elapsed 0s -> 200 (bound 201) elapsed 10ms -> 201 elapsed 30ms -> 203 exceeds the bound elapsed 100ms -> 210 So the failure is the assertion depending on execution speed, not the limiter admitting more than it should: refilling over elapsed time is what a token bucket is supposed to do. Freeze the clock in the shared helper. Every test in this file then measures the burst alone, and the flood test's count is exactly burst/proposalTokenCost on every run. The other tests in the file assert lower bounds, which a frozen clock can only make stricter. Test-only change; no production code is touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yamllint's braces rule rejects the spaces inside the flow mappings used for each alert's labels, failing the Super linter job. Block mappings are the style the rest of the file already uses for annotations, and the style Prometheus rule files conventionally use. No change to the alerts themselves; the file parses to the same 1 group and 4 rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The lint job runs with only-new-issues, so every finding it reports is charged to a line this branch introduces. Clearing them: - misspell (locale US): 72 British spellings across 35 files — behaviour, cancelled, neighbours, synchronisation and friends. Comment and identifier text only; the packages build and their tests pass unchanged. - gofmt: import group ordering in config/config_test.go. - ineffassign: honestArrived was reassigned before the precommit send and never read again; the latency it was going to measure is never reported. Dropped the assignment rather than inventing a report for it. - errcheck: the vote-extension Add in the verification-mismatch test is what sets the test up, so a failure there must fail the test, not pass silently. - nolintlint: a //nolint:revive directive that suppressed nothing. - staticcheck SA9004: split the const group so the untyped constants are not silently taking int64 from the first entry. No production behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The lint job runs golangci-lint-action with only-new-issues, which diffs the branch against its base to decide which findings are new. actions/checkout defaults to a shallow clone, so on a branch with more history than that depth the action cannot compute the diff and falls back to reporting every issue in the repository. The effect is invisible on small branches and severe on large ones: this branch's run reported 254 issues, 130 of them in files it does not touch (internal/state/validation.go, cmd/tenderdash/commands/debug/*, scripts/wal2json/main.go). Re-running the lint job for an unchanged, previously green commit on v1.6-dev reproduces it — 200 issues, same commit, same linter version, same Go version. Fetch the full history so the base is reachable and only-new-issues means what it says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comparing the lint run against the same run on the base commit, every linter now matches the base count except two: misspell and dupl. Both are ours. - misspell: unrecognised, modelled, ageing x2, penalising x2. - dupl: SignVote and SignProposal in the privval gRPC server report as duplicates of each other. They are parallel by construction — the same validation order over a different request type, each returning its own proto response. Collapsing them behind a shared helper would hide the two RPC entry points to satisfy a similarity threshold, so they are suppressed with the reason recorded instead. With these cleared, this branch adds no lint findings of its own. The ~200 that remain are pre-existing repository-wide issues that the gate reports because only-new-issues cannot compute a diff for a branch this size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
only-new-issues asks the action to assemble the branch diff itself and filter against it. For a branch with a large diff that assembly does not produce a usable patch, and the action falls back to reporting every pre-existing issue in the repository — 200 findings in files the branch never touches, which no change to the branch can clear. Pass --new-from-rev pointing at the base branch instead, so golangci-lint does the filtering against a revision the full-depth checkout guarantees is present. Guarded on pull_request, since github.base_ref is empty on push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit passed --new-from-rev while only-new-issues was still set. The action rejects that combination outright — "please, don't specify manually --new* args when requesting only new issues" — so the step failed before linting anything. They are alternatives, not layers. Pull requests now filter with --new-from-rev against the base branch, which the full-depth checkout guarantees is resolvable; pushes have no base branch to diff against and keep the action's own filtering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With the gate finally filtering against the base branch, it reports six findings, all on lines this branch introduces: - gosec G115 in the e2e manifest validation: EvidenceAgeHeight is new here and int64, so an unset or negative value wrapped into an enormous unsigned bound that no retain_blocks could satisfy. Guarded on positive, which is also the correct behaviour — a non-positive evidence age has no bound to enforce. - revive unused-parameter: the previous commit removed a //nolint:revive that sat on the wrong line, which exposed the finding it was meant to suppress. Renamed the unused *testing.T to _ instead of restoring the directive. - staticcheck QF1008 x4: embedded-field selectors (stateData.RoundState.AppHash, block.Header.Height and friends) written the long way. Verified locally with the same invocation CI uses: golangci-lint run --new-from-rev=origin/v1.7-dev ./internal/consensus/... ./test/e2e/... 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
4 participants
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.
Issue being fixed or feature implemented
Release new Tenderdash version
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only