fix(consensus): recover committed blocks across stale proposals and round changes - #1415
Merged
Merged
Conversation
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 |
lklimek
marked this pull request as ready for review
August 26, 2026 09:49
|
✅ Final review complete — no blockers (commit 8242815) · triage: critical |
thepastaclaw
suggested changes
Aug 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The commit now verifies the threshold signature against the correct BlockID, and the regression test covers the primary commit-before-block path. However, adoption still relies on the part-set header as a proxy for the full proposal identity and always defers processing after a proposal mismatch, so valid commits can remain permanently unapplied when votes or block parts arrive first.
Source: reviewers: Codex general and Codex TenderDash consensus security (exact backend model IDs were not supplied); final verifier: Claude agent (exact backend model ID was not supplied). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— tenderdash-consensus-security (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `internal/consensus/state_data.go`:
- [BLOCKING] internal/consensus/state_data.go:482-500: Clear stale proposal state and apply an already-assembled committed block
`ProposalBlockParts` can be retargeted independently of `Proposal`: `addVoteUpdateValidBlockMw` replaces the part set after a 2/3 prevote majority while leaving the old proposal in place. Consequently, a matching part-set header here does not prove that the proposal matches the commit's full BlockID. If the committed block is still incomplete, the surviving proposal can reject its final part on `CoreChainLockedHeight`; if the block is already complete, `adoptCommit` stores the commit and `verifyCommit` returns false, so `TryAddCommitAction.Execute` exits without applying it. No later successful `AddPart` can dispatch the stored commit, and subsequent commits are ignored because `stateData.Commit` is non-nil. Clear a proposal whose full BlockID differs regardless of whether useful parts already target the commit, and return true when the assembled block and current part set independently derive the commit's full BlockID. Add action-level regression coverage for both commit-before-final-part and final-part-before-commit orderings.
StateData.verifyCommit checked a peer-sent commit's threshold signature against the BlockID of a proposal we happen to hold. A node that is behind holds a proposal the network never committed, so the genuine commit that would catch it up failed verification and the node stayed stuck. A forged signature for a block we did not propose was, conversely, dismissed as a mere block-ID mismatch and never evicted its sender. The signature only ever covers the commit's own BlockID, so it is now always verified against that. A verified commit for a block other than ours is catch-up traffic and takes the same path as a commit that arrives before any proposal: the commit is kept, the part set is prepared for the committed block, and proposal state describing the dead block is dropped, since Proposaler.Set would otherwise ignore the real proposal and the committed block's parts would be rejected against the stale one. Fixes #1414 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A +2/3 prevote majority for a block we do not hold points ProposalBlockParts at that block and drops ProposalBlock, but leaves Proposal alone (addVoteUpdateValidBlockMw). A Proposal describing a block the network dropped therefore coexists with a part set already collecting the committed one, and adoptCommit's single part-set-header guard read that matching header as proof the whole round state was in order. It is not: the header commits to the block's bytes and says nothing about which Proposal object happens to be lying around next to it. So the stale Proposal survived, and the node stalled at that height in either of two ways. If the block was still incomplete, its completing part was checked against the stale Proposal's core chain locked height and rejected. A part set completes exactly once, so nothing re-entered that path, and the commit parked by adoptCommit made every later commit a no-op. If the block was already assembled, verifyCommit compared the commit to the stale Proposal, called it someone else's block and returned false - with the committed block sitting in ProposalBlock, complete, and no part left that could trigger another attempt. Staleness of the Proposal is now decided by the commit's full BlockID, independently of the part set: the part set keeps its own header guard, because parts already collected under the committed block's header are that block's parts and discarding them would only force the whole block to be fetched again. That part-set preservation is the split's operative effect, and it is what the stall above needs to recover. The converse case - a Proposal that does describe the committed block surviving a part set that does not - is not reachable, and an earlier version of this message wrongly claimed it as a benefit. adoptCommit runs only after a Proposal matching the commit has already returned true, so on the same-round path the Proposal it sees never matches; on the future-round path EnterNewRound clears Proposal, ProposalReceiveTime, ProposalBlock and ProposalBlockParts unconditionally in the very next dispatch. The two criteria are stated independently because they answer different questions, not because that case occurs. verifyCommit additionally accepts a commit whose block it already holds assembled and under the commit's own part set header. That is the assembled block proving the match on its own, so the branch stays side-effect-free like the proposal-match branch above it: parking the commit before TryAddCommitAction has run ProcessProposal and validation would let a validation failure gate every later commit at that height. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
verifyCommit no longer answers one question. It checks the threshold
signature, then decides whether the round state can act on the commit.
Rename it readyToApplyCommit and document what it decides. The doc added
here says a false return with a nil error means "authentic, but parked",
which is one of that return's two meanings: the function also returns it
for a commit that is not for this height, which is ignored before any
signature is checked and is not parked at all. The commit
"fix(consensus): decide commit readiness on the block, not on a proposal"
states both. Rewrite
adoptCommit's doc to name both of its guards separately -- a Proposal
goes on a full BlockID mismatch, the block and its parts only on a
part-set-header mismatch -- and to record that EnterNewRound owns the
reset for any round above zero, so the preservation reaches the caller
on the same-round path alone.
"Do we already hold this block?" was answered four ways across the
package; proposalUpdater.updateStateData returned early when either the
block hashed to the commit or the part set carried its header. Extract
StateData.holdsBlock, which requires both, and use it there and in
readyToApplyCommit.
Requiring both changes each arm, and an earlier version of this message
described only the first:
- the block hashes to the commit but the part set carries another
header. This returned early and left the round state in the shape
finalizeCommit panics on. It now rebuilds the part set.
- the part set carries the commit's header but no block has assembled
from it yet -- parts collected for the committed block, still
arriving. This returned early and kept them. It now rebuilds the part
set, discarding them, and the block must be fetched again from the
first part.
The second arm is a loss of collected parts on a path that had none
before. It costs a re-fetch, not the height: the rebuilt set accepts the
same parts as they are re-gossiped. It was not noticed when this change
was made, and was found three commits later; the commit
"refactor(consensus): give the round-state retarget a single owner"
restores the two-guard split.
Checking the signature against the commit's own BlockID makes
ValidatorSet.verifyCommit's BlockID guard tautological, so every peer
commit for the current height that clears ValidateBasic now reaches the
quorum-hash and vote-extension-count rejections and spends a pairing on
the way. Neither rejection was classified:
loggingMiddleware wrote an Error line per message and no eviction
followed. Give the quorum-hash disagreement a type so it can be matched
at all, add both to isPeerFloodableError, and pin the remaining bound
with a test that a spent verification budget refuses the commit.
Also: the log line claiming a block "we proposed" describes one the node
merely received; both adoptCommit lines lacked height and round; one key
was camelCase among snake_case; two messages were capitalised among
lower-case neighbours.
Tests: add the untested forged-signature-on-future-round combination,
an end-to-end future-round commit through the real Controller (the part set
is retargeted, then wiped on the round change, then rebuilt -- correct only
by that ordering, and previously unexercised), and extract
the setup the two integration tests duplicated. Drop the eviction
assertion that reached through handleCommitVerifyError, which has its own
tests, and keep the error-type assertion readyToApplyCommit owns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
TryAddCommitAction parked a verified commit whenever the round step had not passed Propose, on the assumption that the block was still to come. A node that never received the proposal cannot leave RoundStepPropose -- isProposalComplete requires a Proposal, and ProposalCompletedAction advances the step only when it is true -- while its block arrives by gossip after a polka all the same. The part set completes exactly once and every later commit is short-circuited by the parked one, so a commit held back on the step is held back for good. That is the lagging-node profile of #1414, on a path the fix for it did not reach. Key the early return on the question the parking is actually about: whether we hold the block the commit names. Reaching that line already implies we do -- verifyCommit returns true only after ProposalBlock is non-nil, the part-set header matches, the hash matches, and the block has been processed and validated -- so the guard is deliberately redundant. It states the condition instead of inheriting it from another function, which is what let the step stand in for it in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
A +2/3 prevote majority for a block we do not hold repoints ProposalBlockParts at that block and leaves Proposal describing the one the network dropped. enterPrecommit does the same. Every consumer of the resulting inconsistency then has to work around it, and the one that does not -- the core chain locked height check on the completing block part -- rejects the real block's last part. A part set completes exactly once, so nothing re-enters that path: the node stalls at the height holding the very block it needs, with no commit required to trigger it. Clear the Proposal and its receive time at both retarget sites. The criterion for staleness is the whole BlockID, since a part set header that happens to match says nothing about the hash or the state ID, and a Proposal that does describe the repointed block survives with its receive time, so its timeliness is not lost. Where the criterion is applied differs by site, and an earlier version of this message said it did not. adoptCommit asks it of every commit it adopts. The two retarget sites here ask it only inside the branch that rebuilds the part set, so a Proposal for another block survives whenever that block's part set header happens to equal the retargeted one. One definition, three call sites, two of them reached conditionally. Consequences worth stating plainly: Dropping the Proposal makes isProposalComplete false, so a completing block no longer advances the step out of Propose and a nil prevote waits for timeoutPropose instead. Same vote, later, bounded. The commit-apply path no longer depends on the step, so it is unaffected. Proposaler.Set gates only on rs.Proposal != nil, so this adds one more moment where a second proposal for the round can be installed. Against a validator node, a designated proposer that equivocates can put a proposal for another block in front of the completing part and reproduce the rejection above. That leaves the height in a terminal condition: the node cannot finish it on its own. That terminal condition is not new, and it was reachable accidentally, with no attacker at all, before this change -- any polka retarget whose stale proposal disagreed on the core chain locked height reached it, which is the stall this series exists to fix. What changes here is who can reach it: an accident becomes an equivocating proposer. The validator path is also the only one missing the rule, since verifyProposalForNonValidatorSet already refuses a proposal that disagrees with a parked commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
A commit we have verified fixes which block the round produced, and the
round state is collecting that block's parts. A proposal naming another
block cannot be acted on: installed, it makes the completing part of the
committed block fail the core chain locked height check, and a part set
completes exactly once.
The rule already exists. verifyProposalForNonValidatorSet refuses such a
proposal for a node outside the validator set, where a commit is the only
thing that can attest a proposal at all. A validator checks the proposer
signature instead and never applied the rule, so the round's proposer
could equivocate -- a second proposal, for another block, in the round
the commit names -- and strand the node on a block it can no longer
assemble.
Lift the check into verifyProposal, above the branch that splits the two
cases, so it governs both paths. The non-validator function is left with
the rule that is unambiguously its own: with no proposer public key, a
proposal with no commit to vouch for it cannot be verified at all. It no
longer performs the BlockID comparison itself, and so accepts on a
precondition its caller established -- on the one path in this file where
no signature is checked at all. An earlier version of this message
presented that as a tidy division of rules rather than as what it is.
The check sits above the verification budget, so a refusal spends no
permit; the data channel's rate limit is what bounds how many a sender can
force. Each one costs a BlockID comparison and the proposer lookup
performed to name the sender in its log line. An earlier version of this
message noted only that no signature check is paid for.
What it does not reject, which is most of what happens: with no parked
commit the first condition returns and nothing changes, and that is the
state for essentially every proposal in normal operation. A proposal
naming the block the parked commit fixed is the one the node is waiting
for and is accepted with its receive time. A commit attests its own
height and round only.
This closes the second of the two ways into the stall this series
addresses, and the two are closed by different commits:
- the accidental variant, where a polka retarget leaves a proposal
describing a block the network dropped, is closed by clearing that
proposal at the retarget sites
- the attacker variant, where the designated proposer equivocates into
the window that clearing opens, is closed here
Both remove ways of reaching the underlying state. Recovery from that
state remains limited.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
Four call sites repointed ProposalBlockParts at a block the node does not hold, each open-coding the same three-part update: adoptCommit, the polka handler in addVoteUpdateValidBlockMw, EnterPrecommitAction.Execute and proposalUpdater.updateStateData. They did not agree. Only two marked block gossip as started; one nilled ProposalBlock on a part-set-header mismatch rather than a hash mismatch; and updateStateData repointed the part set while leaving the Proposal in place, which is the state that makes the committed block's completing part fail its core chain locked height check and stalls the height for good (#1414). StateData.retargetTo now owns the invariant and all four sites call it, so the part set cannot be repointed without the Proposal being reconsidered. Each of the three fields is judged on its own criterion: the Proposal on the whole BlockID, the assembled block on the hash, the part set on the part set header, which keeps parts already collected for the target block instead of discarding them. Callers pass a reason so the shared log lines still name their source. Three behaviours change as a consequence: - adoptCommit drops the assembled block when its hash does not match, where before it kept a mismatched block whenever the part set header happened to agree. replaceProposalBlockOnLockedBlock can produce that combination. - The stale Proposal is dropped at all four sites, not at two of them. - Block gossip is marked as started at all four sites. TestEnterCommitDropsProposalForAnotherBlock covers the commit-application site, which had no drop at all, and TestEnterCommitKeepsPartsAlreadyCollectedForTheCommittedBlock pins the part preservation that the uniform rewrite would otherwise be free to lose. Both fail before this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
Two rules met in the middle of the same state. A proposal was refused when it disagreed with a parked commit, but not when it disagreed with a part set already under way; and a completed part set was judged against whatever Proposal the round held, whether or not that Proposal described the block that had just been assembled. Between them a node could install a proposal for one block while collecting another, and then reject the block it had asked for over the unrelated proposal's core chain locked height. A part set completes exactly once and EnterNewRound does not rebuild it, so the height is lost with blocksync the only way back (#1414). A part set header is a Merkle root over exactly one block's bytes, so a set already under way fixes its block as firmly as a commit does. Proposal admission now asks both questions, under the wider name checkProposalAgainstRoundState, and the two refusals stay distinct: contradicting a verified threshold signature is a different observation about the sender than naming a block the round cannot assemble. This is the check the TODO in Proposaler.Set asked for, so that comment goes. The core chain locked height comparison now runs only against a Proposal naming the assembled block. Against any other proposal the two headers have no reason to agree, and the comparison rejects a block the node requested. That path should now be unreachable, which is why it is worth closing: it is the step that turns an inconsistency into a lost height. TestProposalDisagreeingWithCollectedPartsIsRefused covers the route no commit is involved in, TestAssembledBlockIsNotJudgedByAnUnrelatedProposal the rejection itself, and TestRoundStateRefusalNamesWhatFixedTheBlock pins which observation refuses, since the dispatcher swallows the error. All three fail before this change. TestProposalAgreeingWithCollectedPartsIsAccepted guards the other direction: an instrumented run of the package shows the new refusal firing twice in total, both times in a test that intends it, and never in the honest multi-node tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
readyToApplyCommit accepted a commit outright whenever the round held a Proposal naming the same block. A Proposal attests which block the round is collecting, not that the block has arrived, so the checks behind that acceptance then found no block and returned unverified — and the commit was dropped rather than parked. The node ends up waiting for a commit the network has already sent it. The one remaining question is whether the block is held, so that is the only question asked. Two more places treated a value as evidence of something it does not establish: - updateValidBlock copied ProposalReceiveTime into ValidBlockRecvTime unconditionally. That time is cleared whenever a proposal stops describing the block the round is collecting, so it is zero whenever no live proposal dates the block the round holds, and the zero time is not timely: this node would refuse to propose the block it holds as valid. A zero now falls back to the time the block was learned. - verifyProposalForNonValidatorSet accepted on a comparison its caller had made. There is no proposer public key outside the validator set and so no signature check on that path at all, which makes the commit comparison the whole attestation; it is repeated where the acceptance happens rather than inherited. The guard in TryAddCommitAction cannot fire — prepareCommitForApply has already established that the block is held — so it now says so and logs at Error, which is what reaching it would mean. The proposer is resolved once in verifyProposal and passed down, replacing a second lookup that existed only to decorate a Debug line and treated its own failure as non-fatal twelve lines from where the same failure aborts. The verifyCommit fixture built its proposal block as a bare header. Such a block hashes to nil, so every "do we hold this block" comparison in that table was vacuous; it now builds real blocks with real part sets, and asserts that they hash. That is what makes the first change above testable at all. Naming and comments follow the code: holdsProposalBlock says which of the round state's three block slots it reads, prepareCommitForApply says that it processes and validates the block rather than only checking a signature, the readyToApplyCommit doc states a contract per return path instead of describing one of them, and the fixture and the test that names the contract are renamed to the function they call. Two further test identifiers in the same file still carry the old name; the commit "fix(consensus): keep the assembled block tied to the parts it came from" renames those and corrects this doc's claim that every non-nil error identifies the sender, which is not true of a local budget refusal. The shared fixture is named for the commit it builds rather than for the first scenario that used it, its part size has a name and its "more than one part" requirement is asserted once in the constructor instead of at five call sites, and the helpers it travels with live beside it. Also two staticcheck QF1008 hits in tests, and the log-key and errors.New conventions the rest of state_try_add_commit.go already follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
Giving the retarget one owner, in the commit "refactor(consensus): give the round-state retarget a single owner", also split a coupling that had been structural. adoptCommit dropped the assembled block whenever it replaced the part set; retargetTo judged the two independently, so a block whose hash matched could survive a part set being replaced beneath it. The round state then reports holding a block over a set with nothing in it: holdsProposalBlock is true, which admits the commit, and finalizeCommit's guards pass -- one asks the block's hash, the other the part set header, which the fresh set carries by construction. Neither asks whether the set has any parts in it. Nothing does until SaveBlock, which panics on an incomplete one. No live route reaches that combination today. It was reachable only because nothing prevented it, and nothing needed the freedom: a block exists only because its part set completed, so it belongs to that set. The block is dropped with the set it came from, and a block that disagrees with a part set being kept is dropped on its own. Part preservation is untouched, since that only ever concerned the case where the header matches. Two more places read a value as evidence for a block it does not describe: - updateValidBlock dated the valid block with ProposalReceiveTime whenever it was set. On the polka path the Proposal can name a different block -- the retarget on the next statement drops it as stale for exactly that reason -- and its receive time measures the arrival of something else. Only a proposal naming the block being made valid dates it now. - readyToApplyCommit's doc attributed every non-nil error to the sender. ErrVerificationBudgetExhausted is a local refusal and attributes nothing. The retarget reason becomes a type with four constants rather than four string literals, and the two commit-driven reasons say which parks a commit and which applies one. Two test identifiers still carried the name the series removed from the function they call, and both call readyToApplyCommit directly. verifyProposalForNonValidatorSet takes the round state by pointer, as the check beside it does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
A node whose validator set has gone stale disagrees with every commit the network produces on the quorum hash. Each rejection is logged at debug, because that class is reachable by an honest peer and an error line per message is a flood amplifier. The node then finalizes nothing and says nothing, and the only surviving signal is that the height stops moving. Count commit verification failures by the class of refusal. The classes separate what they say about this node from what they say about the sender: a quorum-hash disagreement usually means our validator set is stale, a forged threshold signature means the sender is dishonest, an exhausted budget means neither and is already counted on its own. A sustained rate on the first is alertable without restoring the log line. The same debug treatment was reaching messages this node produced. The floodable classes describe what a peer can force; on our own message they describe a local fault -- a proposal of ours refused means this node has stopped being able to propose -- so those are logged at warn. Anything outside those classes still logs at error, from any sender, as before. Three properties were relied on without being written down: - retargetTo repoints the round state at a BlockID and must only be given one the round has +2/3 evidence for. A BlockID named by a single peer would let that peer discard the parts and the proposal this round has collected. Every caller satisfies this and nothing checks it. - Block gossip receive latency is measured from one timestamp shared across heights and rounds, so a retarget elsewhere restarts the clock the next observation is measured against. retargetTo marks it only where it replaces the part set: a retarget that keeps the set is not starting to fetch anything, and marking there would report less time than the block took. - The polka site calls updateValidBlock before retargetTo, and the order is load-bearing: the retarget can drop the assembled block and replace the part set, and updateValidBlock copies both into ValidBlock and ValidBlockParts. The two handleCommitVerifyError tests that build the action directly now supply metrics. The queue stays optional, which is what they were written to pin; the metric is not, and every production construction has one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
…he commit Four sites repoint the round state at a block through retargetTo, which drops a Proposal describing something else. A fifth does not. replaceProposalBlockOnLockedBlock installs LockedBlock and LockedBlockParts directly when the commit names the block this round locked, and that is an ordinary relock rather than an exotic state. Its caller then finds the block already held and returns before the retarget runs, so a Proposal for another block survives the round being repointed -- the inconsistency this series removes everywhere else (#1414). The drop moves to the writer. The locked block and its parts are installed together and already satisfy the commit, so there is nothing else for the retarget to do there; what is missing is the one part of it that concerns the Proposal rather than the block. No live route turns this into a stall today: the assembled-block path is guarded where the completing part would be judged, and this path bypasses incremental part gossip entirely. It is the fifth instance of one defect, which is reason enough. Seven places write the proposal block slots. This one and retargetTo are the two that repoint the round state at a block it was not already collecting, and both now reconsider the Proposal. The other five cannot leave one behind: updateToState and EnterNewRoundAction clear the Proposal, its receive time, the block and the part set together; TryAddCommitAction and Proposaler.Set build a part set only when there is none, the first straight after EnterNewRound has cleared all four and the second having just admitted the Proposal against that round state; and addProposalBlockPart assigns the block the current part set completed into, which is the invariant rather than a threat to it. Also a test for the retarget branch that runs when the part set already carries the target header and the block does not match it. That branch had no coverage: deleting it left the whole package green. Commit rejection classes are asserted against wrapped errors as well as bare ones. Every one of them reaches the classifier wrapped by readyToApplyCommit, so matching on the bare error would file all of them under "other" and the counter would read zero while the condition it exists to expose fires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
…the reasons name Two comments claimed more than the code does. retargetTo's doc said every path through it marks the start of block gossip. MarkBlockGossipStarted is inside the branch that replaces the part set, so two of three paths do not mark, and the claim was never true. The behaviour is right and the doc now says why: there is one gossip clock, and restarting it on a retarget that keeps the part set would discard the time a fetch already under way has spent, making the latency histogram report less than the block took to arrive. That is the property a reader of this comment is trying to establish, so the comment was wrong in the one way that matters. TestRetargetToRestartsTheGossipClockOnlyWhenTheSetIsReplaced pins it from both sides. Marking on every path -- making the old claim true -- fails it, and nothing else in the package notices. retargetReason's doc counted four reasons and attributed all of them to retargetTo's log lines. There are five, and the fifth names the one site that repoints the round state without going through retargetTo at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
A floodable rejection of a message this node produced is a local fault: our own proposal refused means the node has stopped being able to propose, and debug buries it. Replay is not that. It re-plays what already happened and says nothing about what the node can do now, so warning there fires on every restart and teaches an operator to ignore the line. Every other replay-sensitive decision in this file already reads envelope.fromReplay; this one did not. TestLoggingMiddlewareSeparatesLocalFaultsFromReplay covers all three senders. Removing the exemption fails the replay case and nothing else. Also record at the round assignment in readReplayMessage that it bypasses EnterNewRound, which is what clears the proposal, its receive time, the block and the part set together -- so an earlier round's part set outlives the round it belonged to, and proposal admission judges the next proposal against it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
The table drew its proposal and commit block IDs from two independent factory.MakeBlockID() values and chose the round state's block and part set from the whole BlockID at once. Every row was therefore either "the same block" or "a different block", and no row could describe a BlockID that agrees on one field and not the others. That mattered for one assertion in particular. "A proposal the network did not commit must not block the real one" reads as a general claim, but every adopted row reached it with a part set whose header differed from the commit's, which is the only case the drop used to cover. The assertion held by arrangement rather than by coverage, and six rows including an explicit forgery case made it look thorough. Choose the block by hash and the part set by header, independently, and add the row that combination now permits: a proposal agreeing with the commit on the part set header alone. That is the shape a stale proposal hides behind, because the round is already collecting the committed block's parts, so every test of the header is satisfied while the proposal still names another block. Restoring the conditional drop fails this row and no other. The budget parameter is nil throughout, which is deliberate: the budget path has its own test rather than a column here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
lklimek
force-pushed
the
fix/1414-verify-commit-blockid
branch
from
September 7, 2026 07:34
fec49b0 to
d03c2c6
Compare
The guard that rejects a commit verified against a block the round state does not hold ran after `stateData.Commit` was already assigned. On that branch the round kept a commit it never dispatched: `Commit != nil` is what stops a later commit being reconsidered, so the round would wait for an event that no longer had a sender. Moving the assignment below the guard leaves nothing behind when the guard fires. The assignment still precedes the dispatch, so nothing changes on the path that dispatches; the two orderings differ only on the rejecting branch, which `prepareCommitForApply` makes unreachable today. The guard exists so that a later change there cannot silently reopen it, and this completes the defence it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
The three lines this branch adds to addProposalBlockPart spell out the embedded field before the one they want — `stateData.RoundState.Proposal` and `block.Header.CoreChainLockedHeight` — copying the style of the surrounding logging calls. staticcheck's QF1008 rejects them. `StateData` embeds `cstypes.RoundState` and declares no `Proposal` of its own; `Block` embeds `Header` and `Data`, and `CoreChainLockedHeight` is declared only in `Header`. Both shortened forms therefore resolve to the same fields with nothing shadowed or ambiguously promoted. The neighbouring logging calls use the long form too, but they predate this branch and the gate filters per changed line, so they stay as they are: a lint fix should not become a file-wide reformat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gAC8rAzQagn5sgDxUJNjj
5 tasks
thepastaclaw
suggested changes
Sep 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The signature correction verifies peer commits against the correct BlockID, but the expanded adoption path leaves three confirmed catch-up liveness failures, including one triggerable through unsigned proposal metadata. Consensus and types package tests passed normally and with race detection and the deadlock tag using a matching BLS build; targeted probes reproduced the reported failures.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — The change spans consensus state transitions, commit signature verification, proposal and block-part adoption, replay, and peer eviction, where regressions could compromise consensus safety or liveness and disconnect honest validators. - Phase 1 reviewers: not run (skipped for throughput: 52 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🔴 3 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `internal/consensus/state_data.go`:
- [BLOCKING] internal/consensus/state_data.go:516-519: Announce the adopted part-set header to gossip peers
Adopting a same-height/same-round commit for block B replaces the local collector but does not notify peers that previously recorded the stale proposal's header A. This parked path does not publish EventValidBlock, and the sender's SetHasCommit does not replace an existing header for the same height and round. Consequently, sendCatchupBlockPart rejects the B/A header mismatch before sending any parts. Catch-up initialization does not repair this because it only initializes a nil peer part set. A targeted probe following the reactor's valid-block announcement handling reproduced the retained header and gossip rejection. Publish the updated collection target when adoption retargets the part set, as proposalUpdater.updateStateData does, and add coverage through peer state and catch-up gossip rather than only injecting parts directly.
- [BLOCKING] internal/consensus/state_data.go:506-509: Preserve parked-commit recovery across round timeouts
This path now parks a commit when the matching Proposal is present but its block is incomplete, without moving out of the voting step. An already-scheduled precommit-wait timeout can therefore dispatch EnterNewRound, which clears ProposalBlockParts while retaining Commit. Subsequent TryAddCommitAction calls immediately return because Commit is non-nil, and incoming parts are discarded because the collector is nil. A four-validator probe with two block precommits and one nil precommit scheduled the timeout without equivocation; after receiving the genuine commit, firing the timeout, and retransmitting the commit and all parts, the node remained at height 1. This is newly exposed for the held-proposal/incomplete-block case: the base implementation returned before storing the commit, allowing retransmission after the round reset to restart collection. Preserve the committed-block collector across round transitions, or prevent those transitions while waiting for its block, and add a pending-timeout regression test.
- [BLOCKING] internal/consensus/state_data.go:506-509: Do not park a commit over an already-completed, rejected part set
A P2P peer can relay an honest current-round proposal first while increasing only CoreChainLockedHeight. CanonicalizeProposal omits that field, so the original signature still verifies and the proposal retains the genuine BlockID. Delivering the genuine block parts before the commit then makes AddProposalBlockPartAction reject assembly on the core-height mismatch, leaving a complete part set but no ProposalBlock. This new adoption path preserves both the completed set and the manipulated proposal because their identifiers match the commit, then stores Commit. Duplicate parts cannot retry assembly because AddPart returns added=false, and subsequent commits are ignored. A round advance also fails to restore recovery because it clears the collector but retains Commit. The reproduction verified the unchanged signature, accepted the manipulated proposal through the dispatcher, and remained at height 1 after commit and block retransmission. Although the unsigned metadata predates this PR, parking the commit in this held-proposal/block-nil state newly removes the previous round-reset retry path. Handle completed-but-unassembled sets explicitly and ensure unsigned proposal metadata cannot prevent recovery of a threshold-committed block.
Preserve commit adoption and centralized round-state retargeting while retaining base metadata repair and full BlockID checks before application. Keep regression cases from both branches and align the late-commit test with authenticated commit parking, asserting no mismatching block is saved. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
suggested changes
Sep 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The commit signature is now verified against the commit's own BlockID, fixing the original stale-proposal rejection path. However, the adopted-commit state machine still has three blocking liveness defects: it can retain a completed part set that rejects the adopted block, it loses the part-set download after a round timeout, and it does not announce the adopted commit to peers that could provide or relay its parts.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a broad consensus-layer change affecting block/commit adoption, threshold-signature verification, peer-eviction behavior, proposal state, and networking-related recovery paths, where subtle regressions could stall nodes or mishandle forged commits. - Phase 1 reviewers: not run (skipped for throughput: 16 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `internal/consensus/state_enter_new_round.go`:
- [BLOCKING] internal/consensus/state_enter_new_round.go:77-83: Preserve parked-commit recovery across round timeouts
adoptCommit stores the authenticated commit and initializes the part set, but EnterNewRound unconditionally clears ProposalBlockParts whenever a timeout advances to a round greater than zero. The commit remains stored, so later TryAddCommitAction calls short-circuit, while AddProposalBlockPartAction drops incoming parts because ProposalBlockParts is nil. The node can therefore never recover or apply the parked commit after a round timeout. Preserve the parked commit's part set across round transitions, or explicitly reconstruct it from StateData.Commit before accepting later parts.
In `internal/consensus/state_data.go`:
- [BLOCKING] internal/consensus/state_data.go:570-580: Do not park a commit over an already-completed, rejected part set
(existing thread: https://github.com/dashpay/tenderdash/pull/1415#discussion_r3963327449)
When the adopted commit has the same part-set header as the existing set but the assembled block does not hash to the commit's BlockID, this branch clears only ProposalBlock and preserves the completed ProposalBlockParts. adoptCommit then stores the commit, but no later part can replace or complete an already-complete set, and subsequent commits are ignored because StateData.Commit is non-nil. A validly authenticated but inconsistent commit can therefore leave the height permanently stuck. If the matching-header set is complete and its assembled block fails the committed hash check, replace the part set as well as clearing the assembled block.
- [BLOCKING] internal/consensus/state_data.go:514-521: Announce the adopted part-set header to gossip peers
(existing thread: https://github.com/dashpay/tenderdash/pull/1415#discussion_r3963327435)
adoptCommit only mutates local StateData; it does not emit an event or otherwise update peer state to announce the current-height commit and its part-set header. The commit listener broadcasts HasCommit only from EventCommitValue, which occurs after the block has already been received and applied. Same-height peers therefore do not learn that this node has the commit, and the data gossip path will not send them the adopted block parts unless their peer state already reports HasCommit. A node that received the commit and parts from one peer may consequently be unable to relay the block to other validators, leaving them stalled.
5 tasks
Keep the authenticated commit's collected block parts when a round advances, announce the download target through valid-block gossip, and apply a retained complete block when a future-round commit arrives. Mark pending committed targets so peers can accept their announcements across round changes. Cover partial downloads through timeouts, peer-state target updates, and complete future-round blocks with focused consensus tests. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
lklimek
changed the title
Update the later-round proposal test to require preservation of the pending commit's part set and early rejection of unrelated proposals and parts. Keep its final assertions that the committed block advances the height. Validated the full internal/consensus/... suite with race and deadlock checks, and golangci-lint on the changed code. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Reject authenticated commits conflicting with an assembled block under the same complete part set before parking or changing rounds. Restore late-commit rejection assertions and keep retargeting fixtures consistent with block assembly. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Publish the new round step before its pending block target so automatic or delayed proposal entry cannot erase the target in peer state. Cover both event streams with auto-propose enabled and verify same-height block-part gossip remains eligible. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
approved these changes
Sep 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The reviewed head correctly addresses all four previously identified consensus recovery defects. Pending committed-block downloads and their collected parts survive round transitions, conflicting hashes are rejected before adoption, and pending targets are announced in an order that survives peer round resets; no new in-scope defects were identified.
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: tenderdash-consensus-security); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate diff that directly changes consensus commit verification, block adoption and recovery, round transitions, proposal/commit agreement, and peer-facing state announcements in files such as state_try_add_commit.go, state_data.go, and msg_handlers.go. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— tenderdash-consensus-security (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 91% left, 5h 46% left - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
5 tasks
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
2 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: Let a lagging validator recover the block the network committed even when it holds a stale proposal or advances to another round while downloading. Preserve useful block parts and tell peers which block is needed.
User story
As a validator operator, I want my node to accept genuine commits and finish downloading their blocks, so catch-up continues without rejecting honest peers or losing progress on a timeout.
Scenario
A peer supplies the network's commit while the receiving validator still holds another proposal or an incomplete block. The receiver authenticates the commit, collects its block, and applies it. Pending round changes preserve that download; peers learn the adopted target even if their recorded round differs. If the block is already complete, the receiver applies it immediately.
Detailed discussion
Issue being fixed or feature implemented
Closes #1414. The original report concerns commit verification against a stale local proposal and rejection of the honest relayer. The current development base already contains part of that correction; this PR completes the surrounding proposal/commit agreement and block-recovery behavior.
The historical-height proposing root cause tracked in #1413 is separate and remains outside this PR.
What was done?
readyToApplyCommitandprepareCommitForApplyto distinguish authentic commits from blocks ready for application. Verify signatures against the commit's own BlockID, and check the held block's full identity before application.retargetTo, dropping unrelated proposal metadata while preserving matching block parts. Apply the same agreement rules to proposal admission, polkas, precommit, locked-block reuse and commit handling.How Has This Been Tested?
On Linux with Go 1.27.1 and the native Dash BLS libraries:
-race -tags=deadlock -p 1, including late commits, readiness, pending downloads, automatic proposal entry, full rounds and WAL round replay. The new event-order regression consumes both state-message types and covers immediate and delayed proposal entry; both cases failed before the fix.internal/consensus/...suite passed with-race -tags=deadlock -p 1, including all six later-round proposal/parked-commit cases previously failing in CI.gofmt,git diff --checkand golangci-lint v2.13.2 on the changed consensus code passed.Breaking Changes
None. The wire format is unchanged.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent