chore: sync v1.7.1-dev.2 into v1.8-dev - #1440
Merged
Merged
Conversation
chore(release): 1.7.0
* chore(deps): bump Go to 1.26.6 (#1417) govulncheck fails identically on v1.7-dev and every open PR branch (#1412, #1411, #1410) with 6 Go standard-library findings, all "Found in: go1.26.5 / Fixed in: go1.26.6": - GO-2026-6218 (net/url) - GO-2026-6091 (html/template) - GO-2026-6090 (crypto/tls) - GO-2026-6089 (net/http, x2) - GO-2026-5972 (encoding/asn1) Reachable from our TLS/RPC/HTTP server and Dash Core client paths (same call sites as PR #1395). Bumping the pinned toolchain from 1.26.5 to 1.26.6 turns govulncheck green everywhere at once and unblocks #1412, #1411, #1410. Mirrors PR #1395's pattern (1.26.4 -> 1.26.5): pure patch-release toolchain swap, no language/API changes, same 16 files touched (go.mod, CI workflow go-version pins, Dockerfiles, docs). Verified locally with go1.26.6: go build ./... succeeds, and govulncheck ./... reports 0 vulnerabilities (down from 6). Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 0a7a33e) * ci(lint): bump golangci-lint from v2.12 to v2.13 golangci-lint v2.12.2 (the latest v2.12.x, matching the current floating `version: v2.12` selector) panics rather than reporting findings when analyzing this codebase under Go 1.27.1: the bundled staticcheck analyzer (honnef.co/go/tools@v0.7.0) crashes its SSA/IR builder with "unexpected expr: *ast.KeyValueExpr" while building IR for the stdlib package internal/poll. Confirmed deterministic (reproduced twice) and not a stale-binary artifact (rebuilt golangci-lint v2.12.2 from source with the go1.27.1 toolchain itself, same panic). Isolating the culprit with --disable=staticcheck makes the panic disappear and returns 0 issues, but disabling staticcheck in CI is rejected as a fix: it is the linter catching the QF1008 class, and turning it off to land a version bump would trade a real gate for a green tick. golangci-lint v2.13.0 shipped 2026-08-19, the same day as Go 1.27.0 — the 2.13 line is Go 1.27-compatible where 2.12 predates Go 1.27 entirely. Measured v2.13.2 (the latest v2.13.x) built fresh with both toolchains: no panic and 0 --new-from-rev findings at go1.26.6 and at go1.27.1. The two changes are independent, so this commit lands ahead of and does not depend on the Go 1.27.1 bump that follows it — every intermediate commit in this branch keeps the lint job runnable. Kept the existing floating-minor pin convention (`v2.13`, not an exact `v2.12.2`-style pin): the crash was caused by the minor version predating Go 1.27, not by floating within a minor, so an exact pin would have failed identically and there is no reason to change the convention in response to a problem it did not cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * chore(mockery): bump to v3.7.4 and regenerate The `check-mocks` CI job pins `mockery=3.7.0` (released 2026-03-06, five months before Go 1.27.0). Under a go1.27.1 toolchain, mockery 3.7.0 fails outright rather than producing a diff: internal error: package "github.com/dashpay/tenderdash/libs/ds" without types was imported from "github.com/dashpay/tenderdash/libs/store" This is mockery's bundled golang.org/x/tools/go/packages loader failing to attach type information to a transitively-imported package when driven by the newer toolchain — not a defect in this repository's source. Reproduced locally under the exact conditions CI runs (GOTOOLCHAIN=local, the go1.27.1 binary setup-go installs, no auto-download); the CI log itself can't show this, since the generation step runs `make mockery 2>/dev/null` and discards stderr — anyone hitting this in CI needs to reproduce it locally to see anything past "exit code 2". mockery v3.7.4 (released 2026-08-23, four days after Go 1.27.0) fixes it: same reproduction conditions, clean exit. Measured it under both go1.26.6 and go1.27.1 and got byte-identical regenerated output either way — the interface{}->any / parameter-naming differences below come entirely from mockery's own codegen version, not from which Go toolchain drives it. That's what makes this commit stand on its own ahead of the Go 1.27.1 bump that follows it in this branch, rather than being a consequence of it. Bumped both pin sites: check-generated.yml's MOCKERY and scripts/mockery_generate.sh's VERSION. The version bump and the regenerated mocks can't be split into separate commits: CI regenerates and diffs in the same step, so a pin bump without regenerated mocks leaves check-mocks red (diff appears), and regenerated mocks without the pin bump leaves it red too (CI regenerates with 3.7.0 and hits the error above again). Regenerated all mocks under mockery v3.7.4 (`make mockery`). The diff is mechanical and cosmetic only, inspected file-by-file rather than trusted from the diffstat: every change is either `interface{}` -> `any` in generated _Expecter method signatures (Go 1.18+ built-in alias, identical type), or a generated parameter name recovering the interface's real source name where 3.7.0 fell back to a generic placeholder (`v`, `vs`, `context1`). No method set changed, no mock behavior changed. Confirmed behaviorally rather than just by reading the diff: `go build ./...` is clean with the regenerated mocks in place, and `go test ./internal/consensus/... -count=1` is green (one run hit the already-known-flaky TestByzantinePrevoteEquivocation from this task's original brief; a second run was clean, consistent with that test's known ~18% flake rate and unrelated to this change). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * test(rpc): close idle client connections in TestMaxOpenConnections Go 1.27 changed HTTP/1 Response.Body.Close() to automatically drain any unread content, up to a conservative limit, to allow the underlying connection to be reused (see the "net/http" section of the Go 1.27 release notes). TestMaxOpenConnections never reads its response bodies before closing them, so under Go 1.27 the requests it makes leave healthy, idle keep-alive connections sitting in the client's pool instead of being closed outright as they were under Go 1.26. Those idle connections keep the corresponding server-side handler goroutines parked in a read wait, which the leaktest check registered at the top of the test flags as leaked: leaktest: leaked goroutine: ... [IO wait]: ... net/http.(*conn).serve ... created by net/http.(*Server).Serve This is not a defect in the server's shutdown path, and reading the response body before closing it does not fix it — 1.27 already drains it for you, which is the entire behavior change; an explicit drain reaches the identical end state (healthy, pooled connection, server goroutine still parked). Confirmed by testing it directly: draining the body explicitly still leaks 100% of the time under go1.27.1. Fixed by sharing one Client across the request goroutines (http.Client is safe for concurrent use) and calling its CloseIdleConnections after they've all completed, before the leaktest check fires. This does not change what the test measures: instrumented it locally to confirm peak concurrent open connections still reaches exactly `max` with the shared client, matching the per-goroutine-client version — concurrent in-flight requests still each need their own connection regardless of pooling, since pooling only affects connections after they go idle. Verified at both go1.26.6 and go1.27.1 (5 consecutive runs each, clean at both, plus one run with -race): this fix is a general test correctness improvement, not conditional on either toolchain, which is why it lands ahead of the Go 1.27.1 bump in this branch rather than after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * test(rpc): assert on our own error family, not stdlib phrasing TestRPCParams and TestParseURI/Decode asserted on the stdlib's exact wording for a malformed json.Number ("invalid number literal" / "invalid number"). That wording changed between Go versions — under Go 1.27 it reads "cannot unmarshal string ... into Go value of type json.Number: invalid syntax" instead — which breaks the assertion even though the actual behavior (the malformed input is still rejected, with the same RPC error code) hasn't changed at all. Traced the code path rather than guessing a new substring: RPCFunc.parseParams wraps every parameter-decode failure via invalidParamsError(), which always builds an rpctypes.RPCError with Code: CodeInvalidParams and Message: CodeInvalidParams.String() — our own constant, literally "Invalid params", unconditionally. Only RPCError.Data ever carries the wrapped stdlib error text. Asserting on "Invalid params" instead of the stdlib phrase pins our own contract (this rejection landed in the invalid-params family) rather than a description we don't own and that a future Go release is free to reword again. This is also a stronger assertion than the one it replaces: the old substring match could not distinguish an invalid-params rejection from any other error that happened to contain the same words, whereas the new one names the specific error family. Checked for the same pattern elsewhere before touching only these two sites: types/genesis_test.go:164 pins a different stdlib JSON error string ("cannot unmarshal string into Go struct field ... of type int64"), but that's the struct-field type-mismatch error class, not the json.Number-literal class this Go release changed, and it already passes clean under go1.27.1 (16/16 subtests) — confirmed by running it, not assumed. Two sites, not a repo-wide problem. Verified at both go1.26.6 and go1.27.1 (all TestRPCParams and TestParseURI subtests green at both, full package test suite green at go1.26.6): version-neutral, so this lands ahead of the Go 1.27.1 bump rather than after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * chore(deps): bump Go to 1.27.1 Follow-on to the previous two commits: with golangci-lint on v2.13 (previous commit), it is safe to move the pinned Go toolchain past 1.26.6 to 1.27.1. This is a minor-version bump, not a patch bump — Go 1.26.6 -> 1.27.1 crosses a Go release boundary, so the "no language/API changes" claim that held for the 1.26.5 -> 1.26.6 patch swap does not carry here and was verified fresh rather than assumed. Verified for the full three-commit stack (this commit on top of the golangci-lint v2.13 bump on top of the Go 1.26.6 cherry-pick): - go build ./... clean. - go vet ./... reports the same 4 findings, byte-for-byte, before and after this commit (3x fmt.Errorf %q in test/e2e/pkg/testnet.go, 1x IPv6 address format in dash/quorum/nodeid_resolver.go) — all pre-existing and unrelated to this change. No new vet strictness under Go 1.27. - govulncheck ./... reports the same finding sets before and after: 0 stdlib vulnerabilities affecting our code, and the same 4 dependency-only findings our code doesn't call (GO-2026-6355, GO-2026-6354, GO-2026-6303, GO-2026-5932) on both sides. - golangci-lint v2.13.2 (what the v2.13 selector resolves to) with --new-from-rev=origin/v1.7-dev: 0 issues, no panic. - go test ./internal/consensus/... -count=1, twice: both green (61.1s, 69.0s) on the final three-commit stack. - GOTOOLCHAIN=auto correctly resolves and downloads go1.27.1 from this module's go.mod, confirmed via `go version` run from inside the module. What moved, reported rather than fixed: - golangci-lint's gofmt formatter (.golangci.yml formatters.enable) flags one pre-existing, untouched file — internal/rpc/core/ mempool.go:149 — as differently formatted under Go 1.27.1's bundled gofmt than under 1.26.6's (a multi-value return with a composite literal inside a select/case reindents differently). --new-from-rev correctly excludes it since the affected line isn't part of any diff in this branch, so no CI gate fails on it. It is an active check, not an absent one — it simply isn't triggered against a line this branch touches. Left unformatted: out of scope for a toolchain-pin change. - During earlier measurement (not the two required runs above, both of which were clean), one go1.27.1 run of `go test ./internal/consensus/...` hit a failure in TestPoC_HeightVoteSet_UnboundedRoundAllocation (retained 308 bytes per message against a <149 threshold), not one of the two previously-known flaky tests in this package. Isolated reruns of just this test passed 5/5 under go1.27.1 and 3/3 under go1.26.6. The 1.26.6 baseline never ran this test under equivalent full-package concurrent load, so this is not a like-for-like comparison between Go versions — left unresolved and not claimed to be version-related. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * style: reformat mempool.go for Go 1.27's gofmt Go 1.27's gofmt reindents a multi-value return containing a composite literal inside a select/case block differently than Go 1.26's gofmt does: the composite literal body and the trailing fmt.Errorf continuation drop one level of indentation. Formatted here with the go1.27.1 toolchain's own gofmt -w -s (`go env GOROOT`'s bin/gofmt under this branch's now-pinned toolchain) — the same flags `make format`'s gofmt step uses; `-s` (simplify) produces an identical diff to plain gofmt here, so it isn't smuggling in any rewrite beyond the reindent. Ran the full-repository gofmt -l -s sweep under go1.27.1 first, using make format's own exclusions (skip *.pb.go, *pb_test.go, .git) rather than fixing only the single file golangci-lint's filtered lint run had named: internal/rpc/core/mempool.go was the only file the sweep flagged, before and after -s. Did not run the rest of `make format` (golines, goimports) since those are separate tools operating on unrelated lines (line-wrapping, import grouping) — out of scope for a pure gofmt-drift fix. This repository has no standalone gofmt gate that this drift was failing: .golangci.yml's `formatters.enable` list does include gofmt (a distinct section from `linters.enable` in golangci-lint v2's config schema — it is an active check, not an absent one), but the one finding it produced was on a line outside any diff in this branch, so `--new-from-rev` filtered it and no CI gate was failing. `make format` itself writes rather than checks, so it enforces nothing on its own either. This commit removes latent formatting drift ahead of it mattering, not a fix for a red gate. Confirmed asymmetric in both directions: after this reformat, `gofmt -l -s` is clean under go1.27.1 (this branch's pinned toolchain) but go1.26.6's gofmt now wants the same lines re-indented back the other way. Anyone building this file with a local Go 1.26.x toolchain and running gofmt/make format against it will see a diff again — expected, since the two Go versions' gofmt disagree on this construct in both directions, and this branch is pinned to 1.27.1. No logic changes: the diff is pure reindentation of already-existing lines, confirmed via `gofmt -d` before applying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix(consensus): bound catch-up block-part resends and restore round timeouts in reactor test (#1418) * test(consensus): restore real timeout ticker in TestReactorValidatorSetChanges The test drove its 7-node network with newMockTickerFunc(true), which forwards only RoundStepNewHeight timeouts and only the first one; every other scheduled timeout is discarded. That leaves the network without timeoutPropose, without timeoutPrevoteWait/timeoutPrecommitWait and, after the first height, without timeoutNewHeight — so consensus has no way to change round and no way to start a height it committed without receiving a peer commit. Any perturbation is therefore permanent, and the test hangs until its 2-minute context deadline. Both observed CI failure shapes follow from this. A proposal delayed past MessageDelay+Precision makes every validator prevote nil, and the round change that would recover never fires. Separately, a node that applies a commit itself sits at RoundStepNewHeight forever; when it is the next proposer, nobody proposes and nobody times out waiting. Restore the real ticker, which "chore(consensus): stabilize consensus algorithm (#284)" had already established before #1204 reverted it, and give rounds enough wall clock to complete on a loaded runner. Round timeouts only fire when the network stalls, so larger values cost nothing on the healthy path, while 2s/1s makes a starved CI runner abandon rounds it would have completed, leaving it to churn instead of converge. Verified with go test -race -count=10: the old code fails 1/10 on an idle 16-core host, reproducing the CI signature exactly (all seven nodes report a waitForAndValidateBlock deadline, one with "subscription terminated by publisher"); the new code passes 20/20 over two such runs. Under three concurrent copies of the test the pass rate rises from 10/18 to 14/15. Refs #1405 <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va * fix(consensus): bound catch-up block-part resends to one pass per interval GossipBlockPartsForCatchup never marks catch-up parts as delivered, so prs.ProposalBlockParts.Not().PickRandom() keeps finding the same index missing and the part is re-sent on every gossip tick. The loop ends only when our own view of the peer's height advances, and the traffic it generates delays exactly the NewRoundStep messages that would advance it. Measured in a 7-node race-enabled reactor test: 1091 height-11 block parts pushed at a node that had already reached height 12, every one discarded, against 9 completed proposal blocks in the same window. The live round cannot assemble its proposal, every validator prevotes nil, and the height churns rounds until the test deadline. The same loop drives the part-set-header mismatch branch, which logs at error level on every tick without sending anything. Bound it to one part-set pass per catchupResendInterval. A pass spends one send per part; once complete, further ticks are skipped until the interval elapses, then a new pass begins. A peer that silently dropped its parts is therefore still served repeatedly for as long as it stays behind, so the wedge fixed in #1365 stays fixed, while a peer we only believe to be lagging costs one pass rather than one send per tick. Parts are still never marked delivered, keeping that guarantee independent of PeerState bookkeeping — which is unreliable here, since SetHasProposalBlockPart ignores updates whose round differs from the peer's current round and catch-up passes the committed block's round. At 4 concurrent race-enabled copies of TestReactorValidatorSetChanges, where the unbounded loop passes 2/16, this passes 16/16 — matching a full revert of the resend behaviour while keeping its protection. Also 18/18 at 3 concurrent copies and 10/10 unloaded. Wasted catch-up parts drop from 164592 to 2845 per batch (a revert scores 2736), and the wedge precondition "Commit came in before proposal" occurs 632 times across those runs with zero wedges. The two tests from #1365 now assert the bounded contract: parts are still never marked delivered, no resend occurs inside the interval, and a resend follows once the clock advances past it. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va * fix(consensus): budget catch-up passes by missing parts, not bit-array size beginCatchupAttempt sized a catch-up pass with prs.ProposalBlockParts.Size(), the length of the peer's bit-array, while GossipBlockPartsForCatchup draws indices from prs.ProposalBlockParts.Not().PickRandom() — the unset bits alone. A peer reporting all but one part present therefore had every draw in the pass return that same index, so the one missing part was re-sent once per entry in the array before the interval pause. That bit-array arrives from the network. NewValidBlockMessage. ValidateBasic checks only that its length equals BlockPartSetHeader.Total, never the bit pattern, and PeerState.ApplyNewValidBlockMessage installs it wholesale. A peer can thus pick the amplification factor, up to MaxBlockPartsCount (1601) sends of a single part per interval, which largely defeats the bound. Budget on the number of parts the peer reports missing instead, and return early when none are. CountTrueBits has no nil receiver guard, unlike Size, so the nil bit-array is rejected first; the sole production caller already checks it, but the previous Size call was nil-safe and that property is worth keeping. The bound can now only tighten: missing is at most the array length, and is equal to it in the common case where a lagging peer holds none of the parts, so catch-up throughput for genuinely lagging peers is unchanged. Adds a regression case for a bit-array with a single unset bit, which draws 3 sends per pass before this change and 1 after, plus a case for a peer that reports the complete part set, which must open no pass at all. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va * fix(consensus): fix the catch-up pass budget against peer bitmap swaps beginCatchupAttempt derived the pass threshold from prs.ProposalBlockParts on every call while catchupAttempts and catchupRetryAt persisted across calls, so the peer that owns that bit-array could move the threshold out from under the counter. Two ways, both reachable through NewValidBlockMessage, whose ValidateBasic checks only the array's length: Lowering the reported missing count to the number of sends already made takes the "pass spent" branch while catchupRetryAt is still zero, because that field was only assigned once the counter reached the earlier, larger threshold. The wait is then measured against the zero time, the counter resets and sending continues. Repeating one decrement at a time telescopes into roughly N^2/2 sends before any wait applies. Raising it afterwards is simpler still: the counter no longer meets the threshold, so the branch holding the deadline check is skipped entirely and a deadline already set is stepped over. Draw from a remaining-budget counter instead. Opening a pass fixes both the budget and the deadline together, and the budget only counts down, so neither can be re-derived from anything the peer sends mid-pass. There is no longer a state where a pass is spent but no deadline exists. The height reset stays: ApplyNewRoundStepMessage drops any message that does not advance the peer's height/round/step, so a peer cannot replay it to reopen passes, and it can only keep doing so by advancing out of the catch-up window. Removing it would instead make every legitimately advancing peer wait an interval per height. Sends per interval remain bounded by the block's real part count: ensurePeerPartSetHeader rejects a fabricated part-set header, so a peer inflating the count buys early exits rather than sends. The regression test drops the reported count to the sends already made and then raises it again, drawing 5 sends from a 3-send pass before this change and 3 after. TestGossiper, TestPeerGossipWorker and TestReactorValidatorSetChanges pass; 16/16 at 4 concurrent race-enabled copies with wasted catch-up parts at 3002 against 2845 before, so the storm bound is unchanged. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va * fix(consensus): end a catch-up pass that cannot send beginCatchupAttempt charged a budget slot before loadMeta, the part-set header check and loadPart, none of which ended the pass when they failed. The peer supplies both the missing bit-array a pass is budgeted from and the part-set header those checks run against, and ValidateBasic accepts any well-formed pair, so it could open a maximum-size pass under a header matching no stored block. That bought two things. A pass that can never send charged a block-store read and an error log to every gossip tick for its whole budget - roughly 160s at the production 100ms cadence - and since the retry deadline was armed when the pass opened, it had long expired by the time the budget ran out, so the next pass began immediately and the interval never throttled anything. The same inflated budget also survived the peer replacing its state, at the same height, with the stored block's real header and a single missing part: the budget is deliberately not re-derived once a pass is open, so the remainder landed as duplicate sends of that one part. Move the send into sendCatchupBlockPart and abandon the pass, arming a fresh retry deadline, whenever it reports that no part reached the peer. A pass now costs at most one failed attempt per interval, and only a pass that got as far as sending keeps its budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXmbFrVf1BqVW1BHZv6Va * fix(consensus): close catch-up pass gaps thepastaclaw flagged as still exploitable Two gaps let a peer defeat the catch-up throttle this PR is adding: - beginCatchupAttempt armed catchupRetryAt when a pass opened. A pass with enough missing parts to span more gossip ticks than catchupResendInterval takes longer than the interval to exhaust its budget, so the deadline had already elapsed by the time the pass finished - the next tick reopened a fresh pass with no quiet gap at all. Arm the deadline when the pass's last attempt is spent instead. - The pass budget was fixed at open and never revisited downward. A peer fully controls ProposalBlockParts over the wire (NewValidBlockMessage. ValidateBasic checks only length, ApplyNewValidBlockMessage installs it wholesale) and can open a pass reporting many parts missing, then swap in a bit-array with only one unset bit — every remaining budgeted attempt then draws that same index, funding repeated duplicate sends of it. Clamp the remaining budget down whenever the peer's current missing count drops below it; never raise it back up. Adds regression tests for both: a 6-part pass ticked across catchupResendInterval to show the deadline is measured from the pass's last send, and a 5-part pass whose bitmap shrinks mid-pass to show the budget clamps to the peer's current report. Both fail against the pre-fix code. Addresses thepastaclaw findings on PR #1418. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf(consensus): avoid a full bit-array copy to count missing catch-up parts beginCatchupAttempt derived the peer's missing-part count via Not().CountTrueBits(), which allocates and copies the whole (peer-controlled, up to MaxBlockPartsCount) bit-array on every gossip tick just to count it. sendCatchupBlockPart already performs its own Not().PickRandom() when a send actually happens, so this was a second, avoidable full-array allocation on every tick regardless of whether a send occurs. Derive the same count as size minus set bits instead. Addresses a copilot review comment on PR #1418. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): honor catch-up send failures, close height-advance evasion Layers on top of review-stream's retryAt-at-close and downward-clamp fix in beginCatchupAttempt. Distinct fixes: - syncProposalBlockPart (pre-existing, untouched elsewhere in this PR) unconditionally returned nil, so a failed send never reached endCatchupPass despite the caller's contract assuming it would. sendCatchupBlockPart now reports success as a bool instead of an error nobody was meant to inspect, and its "no missing part" branch can no longer be hit (the comment claiming otherwise was already false: prs is a fresh per-tick deep copy nothing else can mutate). - A peer's height advancing reset the retry deadline as well as the budget, granting a free pass on every height step; only the budget resets now. - The three catch-up fields were guarded only by a comment claiming a single caller, despite the same *msgGossiper being shared across three concurrent handler goroutines; a mutex now guards them structurally. - Documented the rate limit's actual scope (per gossip worker, not per peer -- a reconnect gets a fresh one) on the Gossiper interface and the actual tick-to-interval relationship on catchupResendInterval, both previously undocumented. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(consensus): fix stale catch-up comment in dataGossipHandler GossipBlockPartsForCatchup is rate-limited and returns without sending on most ticks, so "block parts already delivered" no longer describes what happened before the unconditional GossipCommit call on the same branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): propagate the real send error from syncProposalBlockPart syncProposalBlockPart unconditionally returned nil regardless of the underlying send's actual result, so sendCatchupBlockPart's error check against it (added in the prior commit) was dead code: a send that never reached the peer was indistinguishable from one that did, and endCatchupPass never fired for that failure mode -- the pass kept spending its budget one failed attempt per tick instead of ending on the first one, exactly the spin endCatchupPass exists to prevent. Fix at the source: return g.sync's result directly and drop the now-redundant local log, since both callers already log a non-nil error themselves. The other caller, GossipProposalBlockParts, only ever logged this error and took no other action, so propagating it for real is behavior-preserving there. Regression test advances a fake clock across many ticks with every send forced to fail and asserts exactly one block-part read per catchupResendInterval, not one per tick; confirmed failing (5 reads instead of 1) against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): arm catch-up retry deadline when a peer's height advance interrupts a live pass A peer that advances its reported height mid-pass (before the pass exhausts its own budget) reset catchupRemaining to 0 without arming catchupRetryAt, since the deadline was only ever armed when a pass ran its budget down naturally. A peer whose height ticks forward every gossip tick therefore got a fresh full-budget pass every tick forever, completely bypassing catchupResendInterval for the common multi-part-block case. Arm the deadline on the height-change branch too whenever a pass was actually in flight, matching the same invariant beginCatchupAttempt's normal exhaustion path already upholds. Document that invariant on the struct fields it protects. Also, while re-auditing this function for the same iteration: - stop re-deriving the budget from a peer-controlled bit-array once a pass is open (now g.catchupRemaining = min(g.catchupRemaining, missing), replacing an equivalent but more verbose branch) - de-duplicate the block-store-failure error logs in sendCatchupBlockPart; blockRepository.loadMeta/loadPart already log the failure, so the caller's second log was pure noise - correct sendCatchupBlockPart's doc (it reports handoff to the p2p channel, not peer delivery) and give its error log a distinct message from GossipProposalBlockParts's - correct the Gossiper interface, catchupResendInterval, and endCatchupPass doc comments, which described a flat one-send-per- interval bound that does not hold for multi-part blocks Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(consensus): pin the height-advance throttle bug and fix stale/weak catch-up assertions - Add TestGossipBlockPartsForCatchupHeightAdvanceInterruptingPassIsThrottled, a regression test for the height-advance bug fixed in gossiper.go: confirmed RED (20 sends) against the pre-fix code via git stash, GREEN (1 send) after the fix. - Rename/rewrite TestGossipBlockPartsForCatchupBudgetIsFixedAtPassOpen to TestGossipBlockPartsForCatchupBudgetIsNeverRaisedMidPass: the old sequence never actually drove catchupRemaining below the reported missing count it then 'raised', so the raise-attempt it claimed to guard against was a no-op the test would pass on regardless. The new version forces a real clamp (5-part block, drop to 1 missing after two sends) before attempting to raise the count back up, and asserts the raise is not honored before the interval elapses. - Strengthen TestGossipBlockPartsForCatchupPeerHasEveryPart with an explicit AssertNotCalled, so it fails loudly instead of silently if a send is ever wired in for a peer that already has every part. - Update the two wantLog assertions in TestGossipBlockPartsForCatchup that still expected the removed 'couldn't find a block meta/part' caller-side logs; the block-store-failure messages they should match are now blockRepository's own 'failed to load block meta'/'failed to load block part'. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): share one clock between msgGossiper and its peerGossipWorker newPeerGossipWorker constructed two independent clockwork.NewRealClock() instances for what is meant to be one gossip worker's shared notion of time. They agree in practice (both wrap wall-clock time), but nothing enforces that, and it defeats point-in-time comparisons in tests that inject a single fake clock expecting both call sites to observe it. Construct one clock and pass it to both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(consensus): correct dataGossipHandler's stale catch-up rate-limit comment The comment described GossipBlockPartsForCatchup as rate-limited and a no-op on most ticks, which held before this PR's changes but not after: a pass now spends one send per tick for as many ticks as the peer reports parts missing, and the quiet gap applies only between passes, not on most individual ticks. Reword to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(config): cross-reference PeerGossipSleepDuration's coupling to the catch-up throttle PeerGossipSleepDuration sets the tick rate that internal/consensus/gossiper.go's catchupResendInterval throttle is metered against: configuring it above that fixed interval silently disables the throttle. Document the coupling at both the struct field and the generated TOML template so an operator tuning gossip cadence sees the side effect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf(consensus): skip the missing-part scan while a catch-up pass is backed off beginCatchupAttempt computed missing (Size() - CountTrueBits(), an O(bits) scan of the peer-controlled ProposalBlockParts array) unconditionally, even on ticks that are going to return false immediately because the pass is still within catchupRetryAt's backoff. At production defaults most gossip ticks land in backoff (a pass's quiet interval is ~5x the gossip cadence), so this scanned a bit-array up to MaxBlockPartsCount long on nearly every tick for no reason. Move the backoff check ahead of the scan: when no pass is open and the retry deadline hasn't elapsed, return early without touching ProposalBlockParts at all. The scan still runs whenever it can affect the outcome - opening a new pass, or clamping an active one's budget down to a shrunk missing count. Addresses a copilot review comment on PR #1418. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): close an open catch-up pass when the peer reports it complete beginCatchupAttempt returned early on missing == 0 without touching catchupRemaining/catchupRetryAt. When that happened mid-pass (catchupRemaining > 0, budget not yet exhausted), the pass stayed open indefinitely: noOpenPass would keep evaluating false forever, so the backoff check above it — which only runs while no pass is open — was skipped on every later tick regardless of elapsed time. A peer could freeze catchupRemaining above zero by reporting a complete part set whenever it liked, then resume sending immediately the next time it reported parts missing, with no retry interval honored at all. Close the pass in that case too: zero the remaining budget and arm the retry deadline, matching what happens when a send fails or a height change interrupts a pass. Only do this when a pass was actually open; a peer that has simply always reported a complete set must not have a wait imposed on it. Also corrects a stale doc comment on PeerGossipSleepDuration added by the same throttle: catchupResendInterval is a fixed quiet gap that does not scale with the gossip tick duration, it only fits more ticks into that same window. Adds a regression test that opens a pass, spends one send, has the peer report complete, then report missing again on the same simulated instant - asserting no send until the retry interval elapses. Fails against the pre-fix code (an immediate second send). Addresses copilot review comments on PR #1418. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(consensus): reclassify the missing==0 mid-pass fix as CPU-amplification, not a throttle bypass df8a6057c's commit message and code comments described the missing==0 mid-pass fix as letting a peer "resume sending... with no retry interval honored at all", which reads as a flooding/throttle-bypass concern. On closer trace (independently by team-lead and grumpy-stream) that's not accurate: catchupRemaining only ever counts down, never up, so total sends per pass are identically bounded by the budget fixed at pass open whether or not this fix exists. Nothing here lets a peer exceed that budget or the one-send-per-tick ceiling. The actual defect is CPU amplification: leaving a pass open indefinitely (via alternating missing==0 reports) keeps noOpenPass false forever, which permanently defeats 3b9463dd0's backoff-skip check - the O(bits) scan of the peer-controlled, up-to-MaxBlockPartsCount bit-array then runs on every single gossip tick instead of being skipped during backoff, for as long as the peer holds the pass open this way. It also predates this PR: grumpy traced the same missing==0 short-circuit in the 5978b27 baseline and both its parent commits, so this isn't new here - it only started costing more once 3b9463dd0 added a backoff-skip path for it to defeat. Also records a deliberate behavior change for honest peers: closing the pass on missing==0 means a peer that legitimately completes its part set and later falls behind again now waits out one catchupResendInterval before catch-up resumes, rather than resuming on its very next report. Accepted: a peer that just reported complete is not starving, and the wait is negligible against block time. No functional change from df8a6057c - comments only, on both the fix and its regression test. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): end a catch-up pass when a block-store read panics GossipBlockPartsForCatchup only called endCatchupPass() on a normal false return from sendCatchupBlockPart. But sendCatchupBlockPart's block-store reads (blockRepository.loadMeta/loadPart, wrapping LoadBlockMeta/ LoadBlockPart) panic deliberately on a decode or read failure rather than returning an error - runGossipHandler recovers that panic per gossip tick, logs a full stack, and keeps ticking, same as it does for every other peer-facing panic path in the node. A panic there unwinds straight past the if-check, so endCatchupPass() never runs and the pass stays open with its remaining budget intact. Since the peer controls both the height and the missing bitmap a pass is budgeted on, a peer that names a height whose stored record is corrupt or unreadable gets a fresh attempt - and a fresh panic, and a fresh full-stack log - on every tick for the rest of the pass's budget (up to ~160s for a maximum-size 1,601-part report at the default cadence) instead of ending on the first failed attempt like every other failure mode this PR's earlier commits already cover. Settle the attempt in a defer instead of an if-check on the return value, so panic unwinding also reaches it. sendCatchupBlockPart's own return value still selects the fast path (success needs no settlement); the defer only changes what happens when it doesn't get the chance to return at all. Adds a regression test using a block-store mock that panics on LoadBlockMeta, asserting exactly one read (and thus one panic) per pass rather than one per tick for the whole budget - mirrors TestGossipBlockPartsForCatchupSendFailureEndsPass's structure for the already-covered normal-failure case. Confirmed failing (2+ reads instead of 1) against the pre-fix code. Addresses a thepastaclaw blocking finding on PR #1418. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): don't throttle a genuinely-advancing peer's next catch-up pass 25da4c2 removed the unconditional catchupRetryAt clear from the height-change branch to close a bypass, and ea2b81498 later armed it properly for a pass genuinely interrupted mid-flight (catchupRemaining > 0). The catchupRemaining == 0 case was never revisited: a deadline armed by the OLD height (by exhausting its own budget, or by the peer reporting complete) silently carried forward and throttled the NEW height's first pass by up to catchupResendInterval, for a peer that isn't replaying anything and owes nothing - it's genuinely further along. Catch-up is the only path offered to a peer at a different height than ours (regular gossip requires equal heights), so this taxes every honestly-advancing lagging peer once per height. This throttle does not exist on origin/v1.7-dev; it's a cost this PR's own commits introduced. Naively clearing catchupRetryAt whenever catchupRemaining == 0 at a height change (the obvious fix) reopens a worse gap than intended, though: it cannot tell "the previous pass genuinely finished" apart from "an INTERMEDIATE height in a rapid chain never got a chance to open a pass at all, and is still serving an earlier interruption's penalty" - both look identical (remaining == 0) at the moment of the next height change. Verified by writing TestGossipBlockPartsForCatchupHeightAdvanceInterruptingPassIsThrottled first: a naive version of this fix turns its pinned 1 send across 20 rapid height-hops into 10, because every OTHER hop's remaining == 0 came from the previous hop being blocked, not from a pass finishing. Track WHY catchupRetryAt is currently armed instead: catchupRetrySticky is true when the deadline covers unrendered service (a pass interrupted before exhausting its own budget, or a failed attempt via endCatchupPass) and must survive a height change - including a chain of several - and false when the pass closed because it had genuinely nothing left to do (spent its own budget in full, or the peer reported complete). The height-change branch only clears catchupRetryAt when it's non-sticky (or already elapsed); otherwise it's left untouched, whatever height change caused it to be checked. This does still leave a bounded gap in the non-sticky case, stated explicitly in the code rather than glossed: a peer can open a pass, let it close cleanly (one real send, e.g. by reporting a single missing part), then immediately claim a new height and repeat, getting one send per gossip tick indefinitely instead of one pass per interval. Bounded to at most 1 send/PeerGossipSleepDuration tick (10/s, ~640KB/s of block parts at production defaults), because CompareHRS enforces monotonic height only (no replay or ping-ponging) and ensurePeerPartSetHeader requires the real, currently-stored PartSetHeader for each claimed height (a mismatch ends the pass via the now-sticky endCatchupPass, costing a full interval rather than a send). Sustaining it requires marching forward through genuine, still-retained history, capped by blockStoreBase - no worse than the pre-#1365 catch-up rate this codebase already tolerated in production. Accepted as strictly cheaper than taxing every honestly-advancing lagging peer by up to 500ms per height; a verified-progress-gated redesign that closes it for real is tracked as follow-up, not done here. Two new regression tests, both proven against the pre-fix code: - TestGossipBlockPartsForCatchupCleanHeightAdvanceIsNotThrottled: a single-part pass closes cleanly, the peer immediately claims a new height with a fresh block - asserts an immediate send, not a catchupResendInterval wait. RED before this fix. - TestGossipBlockPartsForCatchupHeightAdvanceInterruptingPassIsThrottled (existing, extended doc comment): still asserts full throttling across 20 rapid, always-interrupted height hops - GREEN before this fix, and the guard that caught the naive version's regression above. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(consensus): start the catch-up quiet gap after the send, not before it beginCatchupAttempt armed catchupRetryAt from a timestamp taken on entry, before the caller had even reached sendCatchupBlockPart. The p2p channel applies backpressure on that very goroutine, so a slow send burned its own duration out of the quiet gap it was supposed to precede - the slower the peer's channel, the weaker the throttle. It now reports the pass's last slot to the caller instead of arming the deadline itself, and GossipBlockPartsForCatchup settles the pass from a fresh clock reading once the send returns. endCatchupPass and the new completeCatchupPass share settleCatchupPass, keeping one place where a drawn attempt closes a pass. The window where the pass is spent but its deadline is not yet armed keeps the documented invariant (a stale deadline with catchupRemaining == 0 is legal) and is unobservable: the single per-peer goroutine is inside the send for all of it, so no further attempt and no height change can run before it settles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 9bf9a0b) * fix(consensus): check that a proposal's block ID describes its block A Proposal carries a BlockID and a proposer signature over it. The BlockID has three fields, and until the block arrives none of them can be checked against anything: PartSetHeader is validated implicitly, because the parts have to assemble under it, but Hash and StateID are taken on the proposer's word. RoundState.BlockID() then hands that BlockID, unexamined, to the prevote signer. So a proposer can gossip a genuine block and sign a BlockID that does not describe it. Every node that receives the block assembles it, finds nothing that disagrees, and signs a prevote for the proposer's value. A commit forms over a BlockID no node ever verified. The height that commit seals is fine: finalization derives the BlockID from the block itself, so the committed state records the real one. The next height is not. Its LastCommit carries the BlockID the votes actually signed, and validation compares that against the previous height's recorded value. They differ, and neither can be revised -- one is persisted state, the other is sealed by a threshold signature over a finalized height. Every proposer at every subsequent round fails the same comparison. The chain does not recover, and restarting does not help. An honest proposer derives its BlockID from the block and the parts it is gossiped in, so once the block is assembled the claim is exactly checkable. Two changes, at the two points that matter: - When the part set completes, compare the Proposal's BlockID against the one derived from the block just assembled. On disagreement the block is kept -- it is the bytes, and they are what they are -- and the Proposal is dropped with its receive time, which leaves the proposal incomplete so the round prevotes nil and moves on. A round is a cheap price; the alternative is signing the value that ends the chain. - RoundState.BlockID() returns the BlockID derived from the block and parts whenever it holds them, and falls back to the Proposal's only when there is no block to derive from. The check above is what reports the lie; this is what keeps an unexamined claim from reaching a signature by some other route. Honest operation is unaffected, and that is measured rather than argued. An instrumented build that compared the two values at every block completion, without acting on the result, recorded 767 agreements and no disagreement across the consensus suite, replay included. Forging one StateID byte in the derived value turned all 778 of them into disagreements, so the comparison was in a position to report one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj (cherry picked from commit 2c2a8f6) * fix(consensus): check a commit's state ID against the block it commits A commit names a block by hash, part set header and state ID. Before applying one, a node checks that the block it holds matches the first two and never looks at the third. A commit that agrees on hash and part set header while naming a different state ID is therefore applied against this block, and then recorded carrying its own BlockID. That record is what the next height is validated against. Its LastCommit holds the BlockID the votes signed; the previous height's committed state holds the one derived from the block. They disagree, and neither side can be revised: one is sealed by a threshold signature over a finalized height, the other is persisted state. Every proposer at every subsequent round fails the same comparison, and restarting changes nothing. Ask the third field. It is asked separately from the other two, rather than as one comparison over the whole BlockID, so an operator is told which field disagreed instead of only that something did. The three checks move into verifyCommitBlock. That is an extraction made for testability and worth naming as such: reaching them previously required an ABCI client, because the surrounding function hands the block to the application immediately afterwards. Separated, each field's rejection is tested directly, with a nil executor so that a test which stops rejecting panics rather than passing quietly. Making the new comparison tautological fails exactly one subtest across the package, and nothing else notices. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj (cherry picked from commit 665ff68) * fix(consensus): drop a disagreeing proposal instead of rejecting the block Two checks in addProposalBlockPart detect the same condition -- a Proposal that disagrees with the block just assembled -- and they disagreed about the remedy. The block ID check drops the proposal and keeps the block. The core chain locked height comparison returned an error, which discards the block. Discarding it is terminal. AddPart consumes the part before either check runs, and PartSet.AddPart returns (false, nil) for a part it already holds (types/part_set.go), so the completion branch is guarded by `added && IsComplete()` and is never re-entered. The part that completed the set is spent, the block is never assigned, and nothing assembles it again. Replay feeds the same sequence to the same rejection, so restarting does not clear it. Make the two agree: on disagreement the proposal is dropped and the block is kept. That is what gives the round its way back, and the mechanism is not tidiness. Proposaler.Set returns early while rs.Proposal is non-nil, so a second proposal for the same height and round is discarded unread -- clearing the field is the only thing that lets the honest copy be accepted. Re-delivery is ordinary behaviour rather than luck: shouldProposalBeGossiped sends when `!prs.Proposal`, and that flag is set only by the sender in updatePeerProposal after its own send, so every honest validator that lost the race still believes we lack the proposal and keeps offering it. Dropping is safe because the block is checked without reference to any proposal. StateData.isValidForPrevote calls sm.ValidateBlockChainLock before the node prevotes, comparing the block's own core chain locked height against state, and it guards Proposal == nil explicitly, so a cleared proposal makes the round prevote nil and advance rather than panic. The remedy does not depend on knowing what caused the disagreement, which is the property worth having: a proposer can build the value wrongly, and any peer relaying the message can raise it, because CanonicalizeProposal omits the dash fields from what the proposer signs while state_proposaler.go enforces only a lower bound. One test covers both because the code cannot distinguish them and is not required to. The part set header, not the block hash, decides whether the proposal describes these bytes: CoreChainLockedHeight reaches Header.Hash() through cdcEncode, which has no uint32 case and contributes an empty leaf for every value, so two blocks differing only in that field share a hash. That predicate selects which diagnosis is reported rather than the outcome, and it is commented as such. An earlier version of this change gated the comparison and left it terminal. That closes the cases where the proposal describes different bytes and does nothing when a relayed proposal names this block with the field altered, since the part set header still matches. It is recorded here so the narrower shape is not reintroduced as an obvious hardening. Measured over the consensus suite: the two values agree 778 times and disagree 3, all at height one and off by one. Every disagreement was attributed by capturing a stack at the comparison. One comes from the reactor test that installs a proposer proposing an incorrect chain lock height; the other two are the tests added here, which construct the disagreement deliberately -- including the reversed case, a proposal claiming a higher chain lock than the block it names, which is this commit's own fixture rather than anything arising at genesis. So the pre-existing suite contains exactly one disagreement, produced by a deliberately byzantine proposer, and none from an honest flow. The suite also passes with the mismatch made non-fatal, so nothing depended on the rejection. Dropping discards ProposalReceiveTime with the proposal, so timeliness is re-evaluated against the re-delivered copy and a slow re-delivery can still cost that round. That cost is per round and self-clearing, and keeping the two paths identical is worth more than softening it here. The proposal is cleared inline rather than through a helper shared with the block ID check above. The duplication is deliberate: that check is a cherry pick, and keeping it byte-comparable with the branch it came from is worth more on a release branch than removing four lines. The helper belongs where the code is native. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * test(p2p): draw distinct addresses in TestConnTracker loops TestConnTracker/BaseSmall/AddingMany failed in CI with "expected: 100, actual: 99". It is not a timing flake: randLocalIPv4 returns 127.<r>.<r>.<r> where randByte is rand.Intn(math.MaxUint8), so the three random octets span 255^3 = 16,581,375 addresses. connTrackerImpl.Len is len(rat.cache) and the cache is keyed by address, so two equal draws collapse into one entry and the count comes up short. AddConn also returns an error for the duplicate, but the loop discards it with `_ =`, so nothing reports the real cause. Measured over 1,000,000 simulated runs of the 100-draw loop: 298 collided, i.e. once in ~3,356 runs, matching the analytic 100*99/(2*255^3). Four loops in this test draw that way (AddingMany and Cycle, each under both the BaseSmall and BaseLarge factories), so the observed rate is closer to once in ~840 CI runs. Cycle is the worse of the two — a collision re-adds an address inside the window, so AddConn errors and trips require.NoError directly rather than only skewing a count. VeryShort draws ten and is exposed the same way. The fix already exists in this file: seqIPv4 ("deterministic, distinct IPv4 address for the given index") was added alongside the tests in #1356, which draw 21,000 addresses and would collide with near-certainty, but it was never applied to the pre-existing loops. Use it in all three loops that need distinct addresses. randLocalIPv4 stays for RepeatedAdding and Window, which each need exactly one address and are collision-immune, and its doc comment now states when it is safe to use so the next reader does not reintroduce the defect. Verified by mutation rather than by re-running until green: collapsing the address space to 3^3 = 27 (below the 100 drawn, so a collision is forced by pigeonhole) reproduces the exact CI signature on the pre-fix code — AddingMany "expected: 100, actual: 27" under both factories, plus Cycle and VeryShort — while the post-fix code passes ten consecutive runs at that same collapsed space. Run in an isolated module containing only conn_tracker.go and its test, since the package's BLS cgo dependency is not buildable in this environment. Not changed, but noted: randByte is rand.Intn(math.MaxUint8), which yields 0..254 and never 255. It is inert for the two remaining single-address callers and is left alone to keep this change scoped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * fix(consensus): validate block IDs across message ordering Check commits against assembled blocks before application and persistence, including commit-first and locally assembled commit paths. Propagate apply errors before broadcasting a commit or advancing the round. Validate late proposals against retained blocks while allowing matching replacements. Add failing-first regressions with signed commits and proposal controls. Co-Authored-By: Codex GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…1437) * fix(consensus): do not apply a commit whose block we no longer hold AddCommitAction asks proposalUpdater.updateStateData to reconcile the round state with the commit being applied. When the node is holding some block other than the committed one, that function clears ProposalBlock, points the part set at the committed block and returns nil -- it is reporting "we do not have this block, I have set up to fetch it, keep waiting". AddCommitAction ignored the distinction and dispatched ApplyCommitEvent regardless, which reads the block it just cleared and hands the round state to mustEnsureProcess. The nil block reaches ProcessProposal and the process dies on the first field access. The sequence is remotely reachable. A node holding a parked commit -- ordinary catch-up state -- that advances a round and then assembles a different block will crash. A commit attests its own round only, so a proposal for a later round naming another block is not refused, and the round's designated proposer can therefore choose to cause this. It is a crash, not a stall: the node exits rather than falling behind. Return without applying when the block is gone. The completing part of the committed block dispatches this event again, so the commit is applied when its own block arrives, which is what the parked commit was waiting for in the first place. Also refuse a nil block in ensureProcess rather than passing it on. This is defence in depth and not the fix: mustEnsureProcess panics on any error, so on its own it converts a nil dereference into a panic one line later -- verified by removing the AddCommitAction guard, which turns the crash into "panic: proposal block is not set: height 1, round 0". The guard goes above the condition rather than inside it because both operands can reach the block: the || short-circuits when the round state did not come from ProcessProposal, so the nil reaches ProcessProposal, and when it did not short-circuit MatchesBlock dereferences the block one frame earlier. Guarding either site alone leaves the other live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj * fix(consensus): discard stale proposal when fetching committed block * fix(consensus): ignore conflicting proposals during commit recovery * fix(consensus): use committed block header during recovery --------- 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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* fix(consensus): apply late commits for completed blocks Validate and apply retained blocks when commit confirmation arrives after conflicting proposal metadata has been dropped, including during propose. Cover arrival order, round differences and invalid commit rejection. Co-Authored-By: Codex GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(consensus): fetch committed block when retained parts differ Keep immediate validation for matching retained parts and preserve download selection for an authenticated different block. Cover current and earlier rounds, propose-step arrival, and invalid signatures. Co-Authored-By: Codex <noreply@openai.com> --------- Co-authored-by: Codex <noreply@openai.com>
Retain Go 1.27.1 while incorporating v1.8-dev dependency updates, Docker actions, and state-sync changes. Co-Authored-By: OpenAI GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
lklimek
changed the title
lklimek
marked this pull request as ready for review
September 8, 2026 09:39
|
🕓 Queued for automated review — 62nd in line, estimated start in ~86 h (commit 673b791)
|
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
3 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.
TL;DR: Bring the current 1.7 development changes, including the consensus crash fix, into the 1.8 development line.
User story
As a node operator, I want the newer development line to include the reliability fixes available in the 1.7 line.
Scenario
The 1.7 development line contains additional consensus fixes and a Go upgrade. This PR proposes bringing that history into the 1.8 line.
Detailed discussion
Issue being fixed or feature implemented
Synchronize
v1.7-devintov1.8-devafter #1437 merged. This is the source revision used by the Docker build fordashpay/tenderdash:1.7.1-dev.1.What was done?
backport/v1.7-dev-to-v1.8-dev-1.7.1-dev.1atf6325b8a177951de324352d58c51a005bd72a088, exactly matchingv1.7-devat creation. No additional commits or conflict resolutions were added.v1.8-dev. The source must remain an exact copy of the requested 1.7 revision.How Has This Been Tested?
v1.8-dev.git merge-tree --write-tree origin/v1.8-dev f6325b8a177951de324352d58c51a005bd72a088detects 16 conflicting files.TestRouter_EvictPeerstimed out waiting forPeerStatusDown; focused repetition passed 4/5 times. The test and router/peer-manager source are unchanged from v1.7.0, but the root cause is unresolved.dashpay/tenderdash:1.7.1-dev.1, digestsha256:bf0fec75b01acb39fe53ec25133d6305d6588fafc8bf38d12c8fc6c6dfeb7c7b. AMD64tenderdash versionsmoke passed and reports the matching branch/source revision.Breaking Changes
The minimum build toolchain in the source is Go 1.27.1. No intentional wire-protocol or public-API breaking change is declared in the synchronization commits. Integration behavior remains unverified until conflicts are resolved.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent