Skip to content

feat(consensus)!: hand a silent proposer's idle round to the next proposer - #1572

Merged
QuantumExplorer merged 46 commits into
devfrom
claude/evonode-waiting-consensus-ccf700
Nov 9, 2026
Merged

QuantumExplorer merged 46 commits into
devfrom
claude/evonode-waiting-consensus-ccf700

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

Basic explanation

What this does: When the chain has nothing to do, each block waits for its proposer (the validator whose turn it is) to have a transaction to put in it. Today nobody checks that this validator is still online while everyone waits. With this change, the waiting proposer sends a small signed "I'm here" message every second. If validators stop hearing it, each one signs a vote to skip that validator's turn. Once two thirds have signed, their votes combine into one signature that moves every node to the next validator, who then waits in its place.

Value: The first transaction after a quiet period no longer pays for an offline proposer. With Dash Platform's mainnet settings (50 s propose timeout) that costs about a minute today; after a handoff, the next proposer proposes as soon as the transaction arrives. The combined signature is also a 96-byte proof of which validator missed its turn, which Dash Platform can use later.

Risks: Medium. This is consensus code that changes how rounds advance, but only on chains that switch to consensus version 2; chains on versions 0 and 1 advance rounds exactly as before. One change reaches every chain: a node's announcements of its round, its target block and the votes it holds now leave through a reactor goroutine, in the same order, instead of from the consensus goroutine itself, so a peer that stops reading can no longer stall consensus. Moving to a later round cannot get two blocks committed, because the locking rules on prevotes and precommits are untouched. So the risk is to liveness: a bug could make validators skip a round they should not, or fail to skip one. Every node must be upgraded before a chain switches to version 2 (older binaries already stop on an unknown consensus version). The PrivValidator interface gains two methods, so signer implementations outside this repo must add them. A peer that sends a heartbeat, skip vote or certificate whose signature does not verify is evicted, as one that sends a forged commit is; a bug that made honest signatures fail to verify would therefore disconnect validators from each other, not only skip rounds.

TL;DR: While a chain waits for transactions, validators notice a silent proposer through missing heartbeats and hand its round to the next proposer with a quorum-signed certificate, so no transaction waits out the propose timeout.

The whole flow in one picture

sequenceDiagram
    autonumber
    participant P as Proposer of round r
    participant V as Other validators
    participant N as Proposer of round r+1
    Note over P,N: Height h, round r. No transactions yet, so everyone waits.
    loop every 1 s while waiting
        P->>V: ProposerHeartbeat, signed with its quorum key share
        Note right of V: each validator re-arms its 5 s silence timer
    end
    Note over P: goes offline
    Note over V: 5 s without a heartbeat
    V->>V: each validator signs one RoundSkipVote for height h, round r
    V-->>V: votes are relayed until every validator has them
    Note over V: +2/3 of the voting power voted, so the vote shares<br/>combine into one RoundSkipCertificate
    V->>V: apply the certificate and enter round r+1, still waiting
    loop every 1 s while waiting
        N->>V: ProposerHeartbeat
    end
    Note over V,N: a transaction arrives
    N->>V: Proposal at once, with no 50 s propose timeout
Loading

User story

As an evonode operator, I want the network to notice an offline proposer while it is idle, so that the next user's transaction is not held up by about a minute because one validator went down.

Scenario

Base flow

Dash Platform runs Tenderdash with create-empty-blocks-interval = 3m, so in round 0 of each height every validator waits until its own mempool has a transaction or 3 minutes pass. One validator is offline, and its turn to propose comes while the chain is idle.

Actual behavior

Nobody notices the proposer is gone until a transaction arrives. Then every validator starts the propose timeout, prevotes and precommits nil, and only round 1 proposes. Rounds after 0 never wait for transactions, so any round skip also forces a block, often an empty one.

Round-0 proposer offline, chain idle, mainnet genesis timeouts
  t=0      a transaction arrives; every validator starts the 50 s propose timeout
  t=50s    nil prevotes, nil precommits, up to 10 s precommit wait
  t~60s    round 1: the next proposer proposes
sequenceDiagram
    participant U as User
    participant V as Validators
    participant P0 as Round-0 proposer, offline
    participant P1 as Round-1 proposer
    Note over V,P0: idle and waiting for a transaction, nobody notices P0 is gone
    U->>V: transaction at t = 0
    Note over V: propose timeout, 50 s
    V->>V: prevote nil, precommit nil at t = 50 s
    Note over V: up to 10 s precommit wait
    P1->>V: proposal for round 1 at about t = 60 s
    V-->>U: block committed about a minute after the transaction
Loading

Expected behavior

On a chain at consensus version 2, the silence is noticed while idle and the round is handed on before anyone is waiting. The next round keeps waiting, so no empty block is made.

Round-0 proposer offline, chain idle, consensus version 2
  idle     no heartbeat for 5 s -> skip votes -> certificate -> round 1, still waiting
  t=0      a transaction arrives; the round-1 proposer proposes at once
sequenceDiagram
    participant U as User
    participant V as Validators
    participant P0 as Round-0 proposer, offline
    participant P1 as Round-1 proposer
    Note over V,P0: idle and waiting for a transaction
    Note over V: 5 s without a heartbeat from P0
    V->>V: skip votes, then a certificate for round 0
    Note over V,P1: round 1, still waiting, so no empty block
    P1-->>V: heartbeat every 1 s
    U->>V: transaction at t = 0
    P1->>V: proposal for round 1 at once
    V-->>U: block committed without waiting out the propose timeout
Loading

Detailed discussion

What was done

Activation and parameters

  • VersionParams.CONSENSUS_VERSION_2. It is hashed into the header like versions 0 and 1, so every node switches at the same height. It keeps the version 1 proposer rotation. Rounds are skipped only where a certificate can prove what the validator set needs: a chain that raises VotingPowerThreshold above the power of the shares the quorum signature is recovered from does not skip rounds.
  • TimeoutParams.proposer_heartbeat (default 1 s) and TimeoutParams.proposer_silence (default 5 s, growing by one heartbeat interval per round). Zero means the default, so chains that never set them need no update, and a chain that sets only one gets the other from it (silence spans five heartbeats), so an update naming one field stays valid. The two are updated as a pair: an update naming one clears the other, which is then derived from the named one again, so what they become depends on the update alone. A node that state-synced over RPC, whose copy from /consensus_params holds them filled in where the chain may hold them unset, stays in step with the chain through later updates. The update that turns version 2 on sets the pair the same way, from what it names and unset otherwise: before version 2 the hash does not cover them, so values held then (set by the application, or given by a state-sync provider) are dropped at activation, and an application that wants other values names them in the activating update or a later one. The other timeouts merge field by field. From version 2 the consensus params hash covers the two timeouts in effect, so state sync authenticates them; the hash of version 0 and 1 params is unchanged. Very large values saturate at the longest duration instead of wrapping.

How long a validator waits without a heartbeat, with the defaults:

Round 0 1 2 3 r
Silence timeout 5 s 6 s 7 s 8 s 5 s + r × 1 s

Why 5 s. Measured on mainnet over heights 455,793 to 460,792 (8.4 days): an offline round-0 proposer occurs at 0.9% of heights and costs about 62.5 s each time. A shorter silence hands the round on before the first transaction more often, but also skips more proposers that are online and only entered their round late (they lose that block):

Silence timeout Online but late proposers skipped Heights with transactions where the handoff finishes before the first one
3 s 2.4% of heights 65.0%
4 s 1.38% 62.5%
5 s 0.82% 61.5%
6 s 0.59% 60.5%
8 s 0.38% 59.2%
10 s 0.29% 58.7%

The other 35% of heights with transactions already have one waiting when the height starts, so the proposer proposes at once and no watch runs.

Proposer heartbeat (types/proposer_heartbeat.go, internal/consensus/proposer_liveness.go)

  • While the node waits for transactions in a round it proposes, it broadcasts a signed ProposerHeartbeat (height, round, sequence, timestamp) on the state channel every interval. It sends one only if it could propose now: the application backends that PrepareProposal reaches answer an ABCI Echo (with a routed ABCI client these can differ from the backend Echo itself goes to), and the propose step would not hold it back after a block-sync handover. A node that cannot propose counts as gone. One readiness probe is out at a time: a probe the heartbeat stops waiting for stays out until the application answers or the node stops, and later heartbeats wait on it instead of sending another. The socket ABCI client keeps every request it sent until its answer comes, so an application that takes requests and never answers would otherwise collect one more every interval. A signer that cannot sign heartbeats at all is asked once, not every interval. A heartbeat whose round the node left while the signer worked is dropped, not sent. Heartbeats are signed one at a time, whatever round they are for: a call to Dash Core cannot be interrupted, so the sender of a later round waits for one still out instead of starting another, and gives up when its own round ends.
flowchart TD
    W["Waiting for transactions in a round this node proposes"] --> T["every heartbeat interval, 1 s"]
    T --> READY{"Could it propose now?<br/>proposal backends answer Echo,<br/>not catching up after block sync"}
    READY -- no --> NONE["send nothing this time;<br/>validators will hand the round on"]
    READY -- yes --> SIGN["sign with the quorum key share<br/>request ID: dphb, height, round, sequence<br/>one signing call at a time, across rounds"]
    SIGN --> STILL{"still in that round?"}
    STILL -- no --> STOP["drop it and stop sending"]
    STILL -- yes --> SEND["send to validator peers<br/>at this height within 4 rounds"]
    SEND --> T
Loading
  • Other nodes check the heartbeat cheaply first (a round within four of their own, the proposer of that round, signed within the silence timeout, not seen before, and within the round's pace of two at once and one per half interval after that), verify the signature, then relay it once. A heartbeat is identified by its signed sequence and timestamp and is not ordered, so a proposer that restarts, or whose clock goes back, is heard again at once. Only a heartbeat for a node's own round re-arms its watch; one for a nearby round is relayed for validators still, or already, waiting there. Heartbeats go only to validator peers in their height and near their round. Relaying is needed because validator connections are not a full mesh:
flowchart LR
    P["Proposer of round r"] -- heartbeat --> A["Validator A<br/>in round r<br/>re-arms its watch"]
    A -- "relays once" --> B["Validator B<br/>in round r+1<br/>relays only"]
    B -- "relays once" --> C["Validator C<br/>in round r, no link to the proposer<br/>re-arms its watch"]
Loading
  • A validator that hears nothing for the round's silence timeout reports the proposer silent. Each arming of the timer has its own token, so a report that a heartbeat overtook is ignored. A timer checks its token and hands over its report under one lock, so a timer that fired just before a heartbeat cannot replace the report of the timer armed after it.

Skip votes and the certificate (types/round_skip.go, internal/consensus/round_skip_collector.go, internal/consensus/state_add_round_skip.go)

  • On silence, a validator signs a RoundSkipVote and broadcasts it. While the round stays silent it sends the same vote again every silence timeout, never signing it a second time; if signing fails, it tries again after another silence timeout. It is not a prevote: it locks nothing, and the validator can still vote for the round's block if it arrives. Every validator signs the same bytes for a height and round, so there is only one skip vote to make per round.
  • The vote is signed on a goroutine of its own, one vote at a time. The signer can be Dash Core over an RPC client whose calls cannot be interrupted, and the consensus goroutine goes on taking proposals, votes and certificates while it waits. A vote that comes back after the node left the round, or heard from the proposer, is dropped. While a call is still out, a later silence is retried after the silence timeout instead of starting a second call.

One validator in a round that waits for transactions:

stateDiagram-v2
    direction LR
    [*] --> Watching: enter round r with no transactions
    Watching --> Watching: heartbeat from the proposer of r, timer re-armed
    Watching --> Voted: silence timeout, sign and send the skip vote
    Voted --> Voted: silent for another timeout, send the same vote again
    Watching --> NextRound: certificate for round r
    Voted --> NextRound: certificate for round r
    NextRound --> [*]: round r+1 waits like round r
    Watching --> Propose: transactions arrive or the empty-block interval ends
    Voted --> Propose: transactions arrive or the empty-block interval ends
    Propose --> [*]: an ordinary Tendermint round
Loading
  • Nodes verify skip votes and relay each as they count it. Once validators holding +2/3 of the voting power have voted, their votes are recovered into a RoundSkipCertificate: one threshold signature over the bytes every voter signed. Recovery also waits for the shares the threshold signature needs, in case a chain lowered VotingPowerThreshold, and a failed recovery is retried with the votes counted while it ran, or else with the next vote. Like a commit's threshold signature, a certificate proves those shares, not a raised VotingPowerThreshold, which the validators collecting the votes enforce.
flowchart LR
    V1["skip vote, validator 1"] --> R
    V2["skip vote, validator 2"] --> R
    V3["skip vote, validator 3"] --> R
    VK["skip vote, validator k"] --> R
    R{"+2/3 of the voting power,<br/>and enough shares for<br/>the threshold signature?"} -- yes --> C["RoundSkipCertificate<br/>height, round, quorum hash,<br/>96-byte BLS signature"]
    R -- "not yet" --> WAIT["wait for the next vote"]
    C --> CHECK["anyone checks it against<br/>the quorum public key"]
Loading
  • A relay can be lost on its way out, and to a node that counted a vote, later copies are not news. So a copy that arrives at least half a silence timeout after the node last relayed that vote has the node relay its own verified copy again, until the round is certified. That is at most two relays per vote per silence timeout, with no new signature check:
sequenceDiagram
    participant X as Validator X, voted
    participant B as Bridge node
    participant Y as Validators only B reaches
    X->>B: skip vote
    B--xY: relay lost, outbox full
    Note over X: still silent one silence timeout later
    X->>B: the same skip vote again
    B->>Y: relays its verified copy again
Loading
  • A valid certificate for round r moves a node in round r, or an earlier one, to round r+1. One from a peer travels through the state machine's message queue; one the node recovered itself is applied at once on the consensus goroutine, so a full queue cannot lose it. Either way it is written to the WAL once it is found news and valid, before it is applied, and replayed after a restart. A chain that skips rounds replays every round of the height, even with wal-skip-rounds-to-last on: whether a round a later certificate leads to waits depends on a polka in any round before it. The WAL does not record the mempool's report of transactions, so on such a chain the propose step the WAL last records settles where replay leaves the node: a later round of the height is entered as the node entered it, and a round replay rebuilt as waiting goes on to the propose step. The node then announces its new round step, so peers send it the next proposal.
  • A node already past round r keeps the certificate anyway, and broadcasts it once. It can get past round r without it, by following +2/3 prevotes of a later round, and may be the only link to a peer still in round r. Each node holds the certificate for the latest round of the height it has one for.
  • A round entered this way keeps waiting for transactions while nothing at the height is locked or has a polka, and the mempool has not reported transactions at this height yet (it reports them once per height). A polka counts once its prevotes are in, before its block is: skip votes do not stop validators from voting, so a late proposal can win a polka in the round being skipped, and the next proposer must not wait with that block's transactions held back. A waiting node also ends its wait and enters the propose step, whose timeout moves the round on, when it takes a proposal for the round (a proposer could otherwise send one, withhold its block's parts and stop the silence watch) or records a polka in its round or one before, such as one completed after the certificate (version 2 only). In a round after round 0, a prevote from another validator ends the wait too, whether it arrives while the node waits or before the node enters the round. Nodes decide whether to wait from what they have seen, a polka's last prevote can reach one validator alone, and a node does not pass on a round's votes to peers past that round. A validator that cannot wait would otherwise vote alone while the rest wait under a proposer whose heartbeats arrive; its prevote reaches them as votes of their own round do. Vote gossip takes a vote as delivered once the channel takes it, so a copy lost after that is not sent again; a peer seen waiting in such a round is therefore sent one of the round's prevotes again every peer-query-maj23-sleep-duration (2 s), until it announces the propose step or another round. A peer seen in a round of the height the node has left is likewise sent one of that round's prevotes or precommits, by turns, every 2 s until it announces a later round: +2/3 precommits of any kind move it on from any step, so it follows even when every vote of the round sent it before was lost. By turns it is also sent the votes of the latest later round that +2/3 of any kind prevoted or precommitted in, which take it there in one step: a node can reach its round on those alone, holding no vote of the peer's round. A peer keeps a vote of a round it has not reached as one of the two catch-up rounds a voter has at a height, and drops any more without blaming the sender. A faulty validator can use this to make that round propose at once, perhaps an empty block, which it can already do by proposing one in its own round. Round 0 still needs a polka: a validator with transactions prevotes there too, and the mempool passes the transactions on. Round 0 does not begin waiting at all with its proposal already taken, as a node can do while it waits out the start of a height after it starts or switches to consensus from block sync. The empty-block interval still counts from the start of the height.
  • A certificate does not move a node that is already committing the height. The reactor drops a certificate that is not news (for a round no later than the one the node holds), any before version 2, and any while the node commits, before it is queued. A certificate whose signature does not verify is never written to the WAL, so replay, which draws on no verification budget, checks no certificate a peer forged.
  • The node starts announcing or watching as soon as its receive routine starts, so a node restarted into a waiting round does not wait for an input first.
  • Every node resends the certificate it holds every peer-query-maj23-sleep-duration (2 s) to each peer at the same height that has not sent it one for that round or a later one. A peer sends one when it first holds it, so an honest peer is sent it until its own broadcast arrives. Nothing else moves a peer still in round r on: a waiting round has no prevotes to follow, and the next round's votes and proposal go only to peers in that round.
sequenceDiagram
    participant A as Node in round r+1
    participant B as Peer still in round r
    Note over B: missed the votes and the certificate
    loop every 2 s until B sends one back
        A->>B: RoundSkipCertificate for round r
    end
    B->>B: verify it and enter round r+1
    B->>A: broadcast it once, so A stops resending
Loading
  • Peers in a later round are sent it too. Without that, a node that followed prevotes past round r would never pass the certificate on:
sequenceDiagram
    participant A as A, holds the certificate for round r
    participant B as B, in round r+1 by prevotes, no certificate
    participant C as C, still in round r, reaches only B
    A->>B: RoundSkipCertificate for round r
    B->>B: verify and keep it, stay in round r+1
    B->>A: broadcast it once, so A stops resending
    B->>C: broadcast it once, then every 2 s until C sends one back
    C->>C: verify it and enter round r+1, with A and B
Loading

Signing

  • PrivValidator gains SignProposerHeartbeat and SignRoundSkipVote. Both sign with the validator's quorum key share under their own request IDs (dphb, which includes the sequence, and dpskip). They are implemented for the Dash Core signer (which now caches its key share per quorum), FilePV and the mocks. The socket and gRPC remote signers refuse with RemoteSignerError 501, which retrying clients treat as final, so a validator using one sends neither heartbeats nor skip votes.

Wire, scheduling and accounting

  • New consensus messages ProposerHeartbeat, RoundSkipVote and RoundSkipCertificate (consensus oneof fields 12 to 14, p2p Envelope fields 31 to 33, all on the state channel), canonical sign-bytes messages, and signed message types 48 and 49.
  • As it arrives, each costs what any state message costs in the per-peer rate limit. One that the reactor's checks find new to this node costs one signature verification more, charged before it is queued. A copy of what the node already has costs no more than that first charge, so a peer that relays a quorum's skip votes again, in the same order, gets the ones its allowance could not carry the first time through. The reactor only runs the checks that need no signature; what is worth verifying waits in the sender's lane of the per-peer scheduler, which reserves the check in the node-wide verification budget, in turn with that peer's votes and proposals. Heartbeats and skip votes are not written to the WAL. Skip votes are not collected before consensus version 2.
  • Heartbeats, skip votes and certificates go out through a 256-message outbox that never makes the consensus goroutine wait; each send gives up after 5 ms, and anything dropped is sent again on its own schedule.
  • The consensus goroutine's other state-channel broadcasts (round steps, target blocks, HasVote, HasCommit) now go out the same way, through a 1024-message outbox and a sender of their own that keeps their order. With the fifo or priority router queues (dashmate configures priority), one peer that stops reading can hold up every send on the channel, and a round handoff announces the new round from the consensus goroutine. If the outbox ever fills, the sender broadcasts the node's current round step, and which block parts it holds, once it catches up.
  • A malformed heartbeat, skip vote or certificate, a vote naming a validator not at its index, or a certificate naming another quorum is a peer error, logged at debug, and scores the peer like any other. One whose signature does not verify is found out on the consensus goroutine, after the reactor queued it. Nodes relay these only once they have verified them, so only a peer that forged it sends one, and it evicts its sender, as a forged commit does; replayed messages and this node's own are exempt. Both are reported as rejected peer input (p2p.ErrRejectedPeerInput), so the peer manager logs the eviction at debug too.

The path of a heartbeat or skip vote from a peer:

flowchart TD
    IN["heartbeat or skip vote from a peer"] --> RL{"per-peer state channel<br/>rate limit"}
    RL -- "over the limit" --> DROP["dropped"]
    RL -- ok --> DEC{"decodes and passes<br/>basic checks?"}
    DEC -- no --> ERR["peer error, logged at debug"]
    DEC -- yes --> SCR{"reactor screen,<br/>no signature check"}
    SCR -- "wrong proposer or<br/>wrong validator index" --> ERR
    SCR -- "too old, seen, out of the<br/>round window or past the pace" --> DROP
    SCR -- "skip vote already counted" --> DUE{"last relay at least half<br/>a silence timeout ago?"}
    DUE -- yes --> OUT
    DUE -- no --> DROP
    SCR -- "worth verifying" --> NEWS{"sender's allowance pays<br/>one verification more?"}
    NEWS -- no --> DROP
    NEWS -- yes --> LANE["sender's lane in the<br/>per-peer scheduler"]
    LANE --> BUD["scheduler reserves one check<br/>in the node-wide verification budget"]
    BUD --> VER{"consensus goroutine<br/>verifies the signature"}
    VER -- forged --> EVICT["sender evicted,<br/>logged at debug"]
    VER -- "heartbeat for its own round" --> ARM["re-arm the silence timer"]
    VER -- "skip vote" --> CNT["count the vote"]
    CNT -- "+2/3 reached" --> CERT["recover the certificate<br/>and apply it at once"]
    ARM --> OUT
    VER -- "heartbeat for a nearby round" --> OUT
    CNT --> OUT["liveness outbox, 256 messages,<br/>never makes consensus wait"]
    CERT --> OUT
    OUT --> SND["sender goroutine,<br/>each send gives up after 5 ms"]
    SND --> PEERS["validator peers"]
Loading
  • Metrics: proposer_heartbeats_sent, proposer_heartbeats_received, proposer_silences (once per silent round), round_skips.

Not in this PR

  • Keeping the watch running while the proposer builds a block, so a silent proposer is skipped after the silence timeout even when transactions were already waiting.
  • Passing certificates to the application.
  • Bounding Dash Core RPC calls themselves. The pinned dashd-go client takes no context and sets no HTTP timeout, so a call Core never answers is never cut off. Skip votes no longer wait for it on the consensus goroutine, and skip votes and heartbeats each keep at most one such call out at a time. Votes and proposals waited for it there before this PR and still do. Fixing that needs a dashd-go change.
  • Switching Dash Platform to consensus version 2.

Testing

New tests:

  • types: heartbeat and skip vote signatures (tampered fields, wrong key, wrong chain), certificate recovery from exactly the threshold and from more votes, refusal below the threshold, duplicates, mixed rounds and strangers, request ID separation, ValidateBasic, silence timeouts that would overflow.
  • internal/consensus/proposer_liveness_test.go: silence after no heartbeats, quiet while heartbeats arrive, no report after stop, relay of the first copy only, forged and misattributed heartbeats refused, stale and off-round heartbeats ignored, signed heartbeats with growing sequence, none while the application is down, the heartbeat pace, the reactor-side screen, paces pruned with the round window, and no heartbeat for a round left while signing.
  • internal/consensus/round_skip_collector_test.go: certificate returned once with the vote that completes +2/3, relay once, relay again when due, wrong index and forged votes refused, out-of-range rounds and old heights ignored, the reactor-side screen.
  • internal/consensus/liveness_delivery_test.go: the reactor queues heartbeats and skip votes in the sender's lane without verifying them, relays a counted vote again when due, the liveness messages are priced and dispatched and stay out of the WAL, a recovered certificate is applied at once, and the outbox never blocks and bounds each send.
  • TestStateProposerHeartbeats: a one-validator version 2 chain announces itself while waiting and stops to propose; a version 1 chain sends no heartbeats.
  • TestStateRoundSkip: a four-validator consensus state where the proposers of rounds 0, 1 and 2 at height 2 are silent. The node votes to skip each round, two other validators' votes complete each certificate, and the node moves on. At round 3 it is the proposer: it waits, sends heartbeats, and proposes as soon as a transaction arrives. TestStateRoundSkipVoteSentAgainWhileSilent and TestStateLivenessMessagesFromPeers cover the resent vote and the path from peers.
  • proto/tendermint/p2p/envelope_test.go and internal/p2p/channel_params_test.go: the p2p Envelope carries, and the channel resolver knows, every consensus message type.
  • Review fixes: a restarted proposer is heard at once; a timer or report overtaken by a heartbeat reports nothing; budget-exhausted heartbeats and skip votes are dropped unverified; no collection before version 2; recovery below a lowered threshold and retry after a failure; certificate resend to peers behind (TestRoundSkipGossipHandler); floodable wrong-quorum and malformed-message errors; derived liveness timeouts, single-field updates and proto encoding; the gRPC signer's 501.
  • Review fixes from the protocol audit: a node past a skipped round keeps, broadcasts and resends its certificate (TestStateKeepsTheCertificateOfARoundItLeftByPrevotes, TestReactorRecordsTheCertificateAPeerSent, TestRoundSkipCertificateWanted, TestRoundSkipGossipHandler); replay with wal-skip-rounds-to-last keeps the certificates of earlier rounds (TestReplayKeepsTheCertificateOfASkippedRound); a stalled signer holds up neither the consensus goroutine nor more than one call (TestStateRoundSkipVoteSignedOffTheConsensusGoroutine); a timer that fired before a heartbeat never replaces the current report (TestProposerLivenessStaleTimerKeepsTheCurrentReport). Each of these fails on the previous head.
  • Review fixes after that: with 160 validators, skip votes that one peer relays in repeated bursts, in the same order, reach the threshold because copies of counted votes do not spend the allowance (TestReactorTakesUnseenSkipVotesBehindRepeatedCopies); a malformed liveness message is reported as rejected peer input, logged at debug (TestReactorReportsMalformedLivenessMessagesAsRejectedPeerInput); applying a certificate with the reactor's broadcast listeners installed and the state channel held neither waits nor loses the new round's announcement, even when it found the outbox full (TestRoundSkipHandoffDoesNotWaitForTheStateChannel); a sender that caught up and went to sleep before a drop was recorded is woken to send the round step again (TestStateSenderWakesForADropRecordedAfterItCaughtUp); params taken from /consensus_params hash like the chain's own after each kind of timeout update, and a synced copy holding substituted version 1 timeouts hashes like the chain's after the update that turns version 2 on (TestProposerLivenessTimeouts); a heartbeat signer that ignores its context and does not answer is asked once while the node moves through five more rounds it proposes in, and once it answers only the round the node is in gets heartbeats (TestProposerLivenessSignsOneHeartbeatAtATime); certificates from a peer that look like news and do not verify are not written to the WAL, and a valid one is written before the round it starts (TestPeerCertificateIsWrittenToTheWALOnlyOnceVerified); the sender of a heartbeat, skip vote or certificate whose signature does not verify is reported for eviction, against the connection that delivered it (TestForgedLivenessMessagesAreReportedOnceVerified). Each fails on the previous head. A replayed certificate and one naming another quorum report no one (TestRefusedCertificatesThatAreNoPeerFault). A retried skip-vote signing counts its round once in proposer_silences (TestStateRoundSkipVoteRetriedAfterSigningFails), and readiness probes the backends a routed ABCI client sends PrepareProposal to, not Echo's (TestAppReadyProbesTheProposalBackends, TestEchoRouteProbesTheBackendsOfTheMethod); a round entered on a certificate does not wait for transactions with a polka whose block parts never came, whether the polka was recorded before the certificate or after (TestStateRoundSkipAfterAPolkaWithoutItsBlock); a waiting node that takes a proposal without its parts reaches the prevote step on the propose timeout (TestStateWaitingRoundTakesAProposalWithoutItsBlock), and so does a node that took round 0's proposal before the round began (TestStateRoundZeroTakesAProposalFromBeforeItBegan); a replay that skips rounds does not leave the node waiting in a round it left for the propose step, or for a later round (TestReplayDoesNotWaitInARoundTheNodeLeftForPropose); each fails on the previous head. Four validators connected through their reactors hand on the round of a proposer that goes silent at height 2 and then commit the same block once a transaction comes (TestReactorHandsASilentProposersRoundOn). An application that reads requests and never answers gets one readiness probe in all, across heartbeat intervals and the rounds the node proposes in (TestProposerReadinessKeepsOneSocketRequestUnanswered, TestAppReadinessSendsOneProbeAtATime). A round entered on a certificate follows a validator that prevotes in it, whether the prevote comes while the node waits or before it entered the round, and round 0 does not (TestStateRoundAfterASkipFollowsAValidatorThatPrevotes). Four validators where a polka from the skipped round reaches only one of them, after all entered the next round, commit the height with no transaction anywhere, even with the first copy of that validator's prevote to each peer lost after its channel took it (TestReactorRoundAfterASkipFollowsTheValidatorThatSawAPolka); a peer waiting in this node's round after round 0 is sent a prevote of it at every pass, a peer in a round this node left is sent that round's prevotes and precommits and those of the latest later round +2/3 voted in, and no other peer is sent one (TestRoundVoteResendGossipHandler). Each fails on the previous head. Four validators where the two that saw a polka from the skipped round leave the next round for the one after it, on a prevote and a precommit for another block from the round's proposer, which signs no other vote of the round, while every vote of the round sent to the other two is lost, commit the height once nothing more is lost, with no transaction anywhere (TestReactorValidatorsLeftWaitingFollowTheRoundAfter); it fails on the previous head, and with only prevotes resent. Four validators waiting in a round, where one is given another round's prevotes and precommits of the other three, for different blocks, and goes on past it holding no vote of the round the rest wait in, commit the height with no transaction anywhere (TestReactorWaitingValidatorsFollowANodeThatSkippedAhead); it fails on the previous head. With wal-skip-rounds-to-last on, a node that stops after a second certificate, before it records the propose step of the round it leads to, comes back from replay knowing the polka of the round before and does not wait, and a node that left a wait on the mempool's report, which the WAL does not record, comes back in the propose step (TestReplayDoesNotWaitInARoundTheNodeLeftForPropose); the first fails on the previous head.

The liveness, collector and round-skip tests also pass under -race. Existing tests run on the branch merged with current v1.8-dev: internal/consensus (state, mempool, reactor, handshake, WAL, replay, message, cost, scheduler and budget tests), types params and proposal tests, the proposer selector, privval/grpc and the consensus proto package. privval TestListenerTimeoutReadWrite fails locally on a TCP timeout assertion; this change does not touch that code.

Breaking changes

  • On a chain at consensus version 2, the consensus params hash in the header also covers the proposer heartbeat and silence timeouts.

  • types.PrivValidator has two new methods, SignProposerHeartbeat and SignRoundSkipVote. Implementations outside this repository must add them.

  • A chain that sets consensus version 2 can only be followed by nodes running this version. Nothing changes until a chain sets it.

  • On a chain that skips rounds (consensus version 2), wal-skip-rounds-to-last has no effect: replay reads every round of the height.

Attribution

🤖 Generated with Claude Code

PR Hygiene · 0569cc6

  • Bots — thepastaclaw skipped after the window
  • Self-review — post /self-reviewed
  • Build failed
  • Approvals
    • files with no dedicated owner (abci/client/client.go, abci/client/routed_client.go, abci/client/routed_client_test.go and 79 more) — approved by lklimek

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

QuantumExplorer and others added 2 commits October 22, 2026 10:48
…oser

While a chain waits for transactions, nothing checks that the proposer of
the round is still there. An offline proposer is only found once a
transaction arrives, and that transaction then waits out the whole propose
timeout. CONSENSUS_VERSION_2 lets validators notice the silence while idle
and hand the round on before anyone is waiting.

- The proposer of a round that waits for transactions broadcasts a signed
  ProposerHeartbeat every timeout.proposer_heartbeat, as long as its
  application answers. Validators verify it and relay it once.
- A validator that hears no heartbeat for timeout.proposer_silence (growing
  by one heartbeat interval per round) signs a RoundSkipVote. It is not a
  prevote: it locks nothing and the validator can still vote for the
  round's block.
- Once validators holding +2/3 of the voting power have voted, any node
  recovers their quorum signature into a RoundSkipCertificate. A node that
  holds a valid certificate moves to the next round; the certificate goes
  through the WAL so a restart replays the move.
- A round entered this way keeps waiting for transactions while nothing is
  locked or valid, so no empty block is made. The empty-block interval
  still counts from the start of the height.

Heartbeats and skip votes are signed with the validator's quorum key share
under their own request IDs ("dphb", "dpskip"). Remote signers over socket
or gRPC cannot sign them, so a validator using one sends neither. Chains on
consensus version 0 or 1 behave as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iting-consensus-ccf700

# Conflicts:
#	internal/consensus/types/round_state.go
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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.

Share

@thepastaclaw

thepastaclaw commented Oct 27, 2026 •

Copy link
Copy Markdown

⛔ Final review complete — 1 blocking finding(s) (commit 0569cc6)

The heartbeat request ID takes the bits of height and round as they are,
like heightRoundRequestID, so the two conversions carry a gosec
exemption. abci/types/types.pb.go is regenerated with `make proto-gen`;
the generator now lays out one doc comment's list differently.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@QuantumExplorer
QuantumExplorer marked this pull request as ready for review October 28, 2026 16:23
@github-actions

Copy link
Copy Markdown

@thepastaclaw review

No review for 8f244f09 yet, so PR Hygiene is asking once. If nothing arrives, the requirement is dropped for this commit and the pull request is labelled bot-review-skipped.

@github-actions

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 28, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The consensus-v2 liveness flow is broadly integrated, and the CI snapshot shows all code checks passing. However, the implementation contains several consensus-liveness and resource-accounting defects: proposer restarts and timer races can cause false skips, liveness signature verification bypasses the node-wide budget, and missed certificates can strand peers in an old round.

🔴 5 blocking | 🟡 3 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to the consensus path that introduces new peer-facing message handling, quorum-signed skip certificates, proposer liveness transitions, and signing/key-management interfaces in files such as internal/consensus/proposer_liveness.go, internal/consensus/round_skip_collector.go, and types/round_skip.go.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/proposer_liveness.go`:
- [BLOCKING] internal/consensus/proposer_liveness.go:196-209: Heartbeat sequence resets across proposer restarts and causes false skips
  The highest accepted heartbeat sequence is kept only in memory, and resetLocked clears lastSeq whenever the liveness target is recreated. If a proposer restarts during the same height and round, its sender starts again at sequence 1, while remote watchers may already have accepted a much higher sequence. Those watchers reject every new heartbeat until the proposer catches up to the old sequence; meanwhile their existing silence timer expires and they can sign a skip certificate for a proposer that is already back online. Persist or otherwise include an incarnation/generation value in the signed heartbeat identity so a proposer restart cannot reuse a stale sequence space.
- [BLOCKING] internal/consensus/proposer_liveness.go:211-218: A stale timer callback can report a live proposer as silent
  armLocked stops the previous timer and creates a new callback using only l.generation. Resetting a timer does not prevent a callback that has already been queued from running, and generation changes only when the target round changes. A valid heartbeat can therefore re-arm the watch while the old callback still calls fire with the same generation. handleProposerSilence checks only height, round, and waiting state, so it can act on that stale silence event and produce a skip vote despite the accepted heartbeat. Associate each timer callback with a reset-specific generation or heartbeat epoch and verify that epoch before delivering and handling the silence event.

In `internal/consensus/reactor.go`:
- [BLOCKING] internal/consensus/reactor.go:823-840: Liveness signature checks bypass the node-wide verification budget
  The state-channel handler calls liveness.heard and roundSkips.add directly, and both perform BLS signature verification before returning. Unlike proposal, vote, commit, and certificate verification, these paths do not consume the shared VerificationBudget. The livenessTokenCost is only a per-peer limiter: a peer can still force roughly 50 BLS verifications per second, and multiple connections can do so concurrently, consuming CPU beyond the node-wide verification allowance and delaying consensus processing. Pass the shared budget into both verification paths and reject or defer the message when the budget is exhausted.
- [BLOCKING] internal/consensus/reactor.go:605-613: Peers that miss the one-shot skip certificate have no catch-up path
  A round-skip certificate is broadcast only when it is applied; there is no targeted or periodic certificate gossip for a peer that was offline or missed that broadcast. Receiving NewRoundStep updates only the local PeerState and does not advance the peer's consensus state. Skip votes are not stored in HeightVoteSet, and the existing vote gossiper does not resend skip votes or certificates. Proposal gossip is also gated on the peer already reporting the same round, so a peer that remains in the old idle round can wait indefinitely and never receive the next proposal. Add certificate retransmission/catch-up handling, or make the certificate recoverable through a reliably replayed gossip path.

In `internal/consensus/round_skip_collector.go`:
- [BLOCKING] internal/consensus/round_skip_collector.go:131-150: The recovered flag is set before certificate recovery succeeds
  The collector sets votes.recovered before calling RecoverRoundSkipCertificate. If recovery fails, the flag remains true, and all later votes for that round return without retrying recovery. This is reachable when the configured voting-power threshold is lower than the BLS DKG recovery threshold, or if recovery encounters a transient failure. The round can then remain without a certificate even after sufficient additional shares arrive. Set recovered only after successful recovery, or clear it when recovery fails so later shares can retry.

In `types/round_skip.go`:
- [SUGGESTION] types/round_skip.go:159-171: Classify wrong-quorum certificates as floodable peer errors
  A certificate with a validly sized random quorum hash and threshold signature reaches Verify, which returns a formatted quorum-hash mismatch error before BLS verification. isPeerFloodableError recognizes ErrRoundSkipCertificateInvalidSignature but not this cheap, attacker-controlled validation failure, so the reactor logs it at Error level and reports it as a peer error. This is inconsistent with the existing invalid-commit-quorum-hash classification and permits log amplification. Add a sentinel error for the quorum-hash mismatch or classify this validation error consistently with invalid commit quorum hashes.

In `types/params.go`:
- [SUGGESTION] types/params.go:394-398: A single-field heartbeat timeout update can create invalid consensus parameters
  UpdateConsensusParams merges ProposerHeartbeat and ProposerSilence independently, while zero values are interpreted as defaults by TimeoutParamsOrDefaults. Starting from a legacy parameter set with both fields unset, an application update that sets only ProposerHeartbeat to a value such as 10 seconds leaves ProposerSilence at zero, which defaults to 5 seconds. ValidateConsensusParams then rejects the resulting set because silence is not longer than the heartbeat interval. Preserve a valid pair during partial updates, require both fields together when enabling these values, or validate and default the update semantics consistently.

In `privval/grpc/client.go`:
- [SUGGESTION] privval/grpc/client.go:175-190: The gRPC remote signer uses a retryable error type inconsistently
  The socket remote signer returns RemoteSignerError with code 501 for unsupported heartbeat and skip-vote operations, which RetrySignerClient recognizes as non-retryable. The gRPC implementation returns a plain errors.New error for the same permanently unsupported operations. If the gRPC client is wrapped by the existing retry decorator or another generic retry layer, those calls will be retried until timeout even though the protocol cannot satisfy them. Return the same RemoteSignerError classification from both implementations.

Comment thread internal/consensus/proposer_liveness.go Resolved
Comment thread internal/consensus/reactor.go Resolved
Comment thread internal/consensus/proposer_liveness.go Resolved
Comment thread internal/consensus/reactor.go Resolved
Comment thread types/round_skip.go Resolved
Comment thread types/params.go Resolved
Comment thread privval/grpc/client.go Resolved
@github-actions

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 28, 2026
…kipping

The p2p Envelope did not carry ProposerHeartbeat, RoundSkipVote or
RoundSkipCertificate, so the router dropped every one and no node heard
another. They are now Envelope fields 31-33, and a test checks that the
Envelope carries every consensus message type.

Review fixes:
- Heartbeat sequences count from the signing time, so a proposer that
  restarts within its round is heard again at once.
- Each arming of the silence timer has its own token; a callback, or a
  silence report, overtaken by a heartbeat is ignored.
- Heartbeat and skip vote signatures draw on the node-wide verification
  budget, and skip votes are not collected before CONSENSUS_VERSION_2.
- A certificate is recovered once votes reach the quorum threshold and
  the shares the threshold signature needs, and a failed recovery is
  retried with the next vote.
- Nodes resend their round skip certificate to peers still in the
  skipped round, which have no other way past it. A certificate no
  longer moves a node that is committing the height.
- A certificate naming another quorum is a floodable peer error.
- A chain that sets one of proposer_heartbeat / proposer_silence gets
  the other derived from it, so a single-field update stays valid; the
  two are no longer written into default params.
- The gRPC signer refuses liveness signing with RemoteSignerError 501,
  like the socket signer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 28, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The implementation still has three blocking consensus issues: certificates do not prove the configured voting-power threshold, failed recovery can lose a concurrent share without retrying, and wall-clock heartbeat sequences are not restart-safe under same-millisecond restarts or clock rollback. Build and lint have passed, while the relevant test and end-to-end checks remain queued or in progress; no failed code check currently provides contrary evidence.

🔴 3 blocking

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large diff directly changes consensus round advancement and implements signed proposer-heartbeat, quorum round-skip certificates, and peer-facing message handling in files such as internal/consensus/proposer_liveness.go, internal/consensus/round_skip_collector.go, and types/round_skip.go.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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 `types/round_skip.go`:
- [BLOCKING] types/round_skip.go:170-181: Round-skip certificates do not prove the configured voting-power threshold
  RoundSkipThresholdPower makes the local collector wait for the maximum of the configured voting-power threshold and the quorum's DKG recovery threshold, but the participation evidence is discarded when the certificate is built. RoundSkipCertificate contains only the height, round, quorum hash, and recovered threshold signature, and Verify checks only the quorum hash and that signature against the threshold public key. A coalition holding only the DKG threshold shares can therefore produce a certificate that passes Verify even when its voting power is below an explicitly configured consensus threshold. For example, with a 12-member quorum whose DKG threshold is 8 and a configured skip threshold of 10, eight compromised validators can recover and send a valid certificate that honest nodes accept in AddRoundSkipCertificateAction. The certificate needs verifiable signer participation/power, or consensus version 2 must reject configurations whose required threshold cannot be represented by the certificate format.

In `internal/consensus/round_skip_collector.go`:
- [BLOCKING] internal/consensus/round_skip_collector.go:154-170: The recovered flag is set before certificate recovery succeeds
  (existing thread: https://github.com/dashpay/tenderdash/pull/1572#discussion_r4404589738)
  The failure-path reset fixes only the sequential retry case. One caller can set recovered, snapshot the threshold-sized vote set, unlock, and begin RecoverRoundSkipCertificate; a concurrent caller can then add another distinct valid share while recovered is true, so that share is stored but no recovery is attempted. If the first recovery fails, the code clears recovered without rechecking the now larger vote set. With no later vote, the round remains without a certificate even though the collector now has enough usable shares. After a failed recovery, recheck the current vote set and schedule another recovery when it is threshold-qualified, or otherwise ensure arrivals during recovery trigger a retry.

In `internal/consensus/proposer_liveness.go`:
- [BLOCKING] internal/consensus/proposer_liveness.go:335-349: Heartbeat sequence resets across proposer restarts and causes false skips
  (existing thread: https://github.com/dashpay/tenderdash/pull/1572#discussion_r4404589701)
  heartbeatSequence uses UnixMilli as a replacement for a persistent incarnation identifier, while resetLocked clears the sender's lastSeq on restart. If the proposer restarts within the same millisecond, its first sequence can equal the sequence already accepted by watchers and is rejected by the <= check. If the wall clock moves backward, every new sequence can remain below the watcher's previous sequence until the clock catches up. The watcher then receives no acceptable heartbeat, its silence timer can expire, and current still treats the report as valid. Use a persisted monotonic generation/incarnation value or a signed restart marker that safely creates a new sequence space independent of wall-clock rollback.

Comment thread types/round_skip.go Resolved
@github-actions

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 28, 2026
… late votes

Watchers no longer order heartbeats by sequence. A heartbeat signed within
the silence timeout and not seen before re-arms the watch, whatever its
sequence, so a proposer that restarts within the same millisecond or
whose clock goes back is still heard. Heartbeats are identified by their
signed sequence and timestamp; the seen set forgets those older than the
silence timeout and holds at most 256.

A round skip recovery that fails now retries at once with any votes
counted while it ran, instead of leaving them to a vote that may never
come.

RoundSkipCertificate documents what it proves: like a commit's threshold
signature, the shares its quorum signature needs, not a raised
VotingPowerThreshold, which honest collectors enforce.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 28, 2026
- A round entered through a skip certificate no longer waits for
  transactions once the mempool has reported them at this height: the
  mempool reports them once per height, so such a round waited until the
  empty-block interval, or forever without one.
- The reactor drops a certificate the state machine would not apply,
  including every certificate before CONSENSUS_VERSION_2, before it is
  written to the WAL.
- The receive routine reconciles proposer liveness once before its first
  input, so a node restarted into a waiting round announces or watches
  at once instead of after its next input.
- A validator silenced again in a round it already voted to skip
  rebroadcasts the vote it holds instead of signing it again, and one
  whose signing failed tries again after another silence timeout.
- From CONSENSUS_VERSION_2 the consensus params hash covers the proposer
  heartbeat and silence timeouts in effect, so state sync authenticates
  them. Earlier versions hash exactly as before.
- A signer that cannot sign heartbeats at all (remote signers, 501) is
  asked once, not every interval; types.ErrSigningNotSupported marks it.
- A heartbeat is sent only when the propose step would not hold the node
  back after a block-sync handover.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The implementation closes the previously reported retry, timer, budget, gossip, parameter, and restart-handling issues, and the available build, test, lint, protobuf, vet, and security checks pass at this head. Two blocking consensus issues remain: certificates do not prove an explicitly raised voting-power threshold, and validators in later rounds drop valid heartbeats instead of relaying them to validators still watching the earlier round.

🔴 2 blocking

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus progression, signed quorum certificates, proposer liveness, and peer-facing message serialization in files such as internal/consensus/proposer_liveness.go, internal/consensus/round_skip_collector.go, and the consensus protobuf types.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/proposer_liveness.go`:
- [BLOCKING] internal/consensus/proposer_liveness.go:354-367: Heartbeats from other rounds are not relayed, causing false silence reports
  heard returns before verification and relay whenever the receiving node is not currently watching the heartbeat's exact height and round. On a non-full-mesh network, a proposer can send a valid round-0 heartbeat to a validator that has already advanced to round 1, while another validator remains in round 0; the round-1 validator drops the heartbeat instead of forwarding it, so the round-0 validator can report the still-live proposer silent and sign a skip vote. Relay eligibility must be determined from the heartbeat's height, round, proposer, and the current validator set, while timer re-arming should remain restricted to the local watch target.

In `types/round_skip.go`:
- [BLOCKING] types/round_skip.go:141-151: Round-skip certificates do not prove the configured voting-power threshold
  (existing thread: https://github.com/dashpay/tenderdash/pull/1572#discussion_r4405335378)
  The collector requires RoundSkipThresholdPower, which is the maximum of the configured VotingPowerThreshold and the DKG recovery threshold, but the certificate discards the participating validators and their voting power. RoundSkipCertificate.Verify at lines 179-190 checks only the quorum hash and recovered threshold signature. Consequently, when an operator raises VotingPowerThreshold above the DKG threshold, a coalition holding only the DKG recovery shares can directly recover a valid signature for the round-skip sign bytes and every node accepts it through AddRoundSkipCertificateAction, despite the coalition having less than the configured skip-vote power. The local collector enforces the higher threshold only for certificates it creates; the receiving path that advances consensus cannot enforce it. Include verifiable signer/power evidence in the certificate, or otherwise prevent version-2 configurations whose threshold cannot be represented by the certificate format.

Comment thread internal/consensus/proposer_liveness.go Resolved
@github-actions

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete diff at b46fbe0: 40 prior findings are fixed, and the previously withdrawn finding remains withdrawn. One blocking liveness issue remains: the new minority-prevote wakeup depends on optimistic gossip that does not retry after a local delivery drop. This was a static-only review; current CI shows build, all six test shards, both e2e suites, protobuf/mock checks, vet, and lint passing, with review and hygiene checks pending.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff adds consensus round advancement through quorum-signed skip certificates in internal/consensus/state_add_round_skip.go and internal/consensus/proposer_liveness.go, alongside new signature handling and peer-facing protocol messages.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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_add_vote.go`:
- [BLOCKING] internal/consensus/state_add_vote.go:218-223: Reliably retransmit the prevote that wakes an idle successor round
  The new wakeup depends on a minority prevote reaching waiting validators, but ordinary vote gossip does not guarantee retry after a local drop. `GossipVote` calls `sync(..., updatePeerVote(...))`, and production gossip workers use `optimistic: true`: the sender marks the peer as holding the vote after router admission, even when sending returns an error. `PickVoteToSend` subsequently excludes that vote. A drop in `allowVoteChannelMessage`, consensus-lane shedding, or the supported `simple-priority` router queue does not clear this optimistic mark.

  On a four-validator version-2 chain with `create-empty-blocks=false`, let only honest A know the skipped round's polka, while honest B and successor proposer C wait in round 1 and Byzantine D abstains. With A connected to C through B, dropping A's round-1 nil prevote before B processes it leaves A believing B already has it. Neither B nor C leaves transaction waiting. A has no round-1 majority to initiate `GossipVoteSetMaj23` repair, and B/C's round-1 announcements reset `ProposalPOLRound` to -1, so A's old-round polka is not requested either. C continues sending valid heartbeats, preventing another silence handoff. The height therefore remains stranded after the temporary congestion ends, until an unrelated transaction or reconnection occurs.

  Make this idle-to-active signal retryable until the peer demonstrates progress, rather than relying only on the receive-side predicate. Extend `TestReactorRoundAfterASkipFollowsTheValidatorThatSawAPolka` with an honest bridge that drops the first wakeup prevote, and require eventual commitment without a transaction notification or reconnect.

Comment thread internal/consensus/state_add_vote.go Resolved
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

…fter a skip

A prevote is what ends a peer's wait for transactions in a round a
skip certificate began, when another validator could not wait there.
Vote gossip takes a vote as the peer's once the channel takes it, so a
copy lost after that (router queue, the peer's limits) was never sent
again, and the peer has no timeout while its proposer's heartbeats
arrive. The peer worker now sends a validator peer it sees in this
node's round (after round 0, still at the new round step) one of the
round's prevotes every peer-query-maj23-sleep-duration, until the peer
announces the propose step or another round. It reads the vote set
through its locked accessors, since the consensus goroutine adds to it.

TestReactorRoundAfterASkipFollowsTheValidatorThatSawAPolka now loses
the first copy of the wakeup prevote to each peer and still commits
without a transaction or reconnect; it strands without the resend.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

gosec G115 flagged the int -> int32 conversion in the waiting-peer
prevote handler. A conversion that would overflow sends nothing, as
the handler runs on a gossip goroutine.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified exact head 03d448c: the prevote retransmission fixes the latest delivery-loss finding, and the earlier fixes remain present. One separate blocking restart-liveness defect remains: a subsequent certificate can reconstruct an untimed idle round after WAL filtering discards the evidence that originally prevented waiting. This was a static review; current CI reports successful build, all six test shards, both e2e configurations, and static checks, with PastaClaw review and PR Hygiene pending.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate change directly alters consensus round advancement in internal/consensus/proposer_liveness.go and internal/consensus/state_add_round_skip.go, introduces quorum-signed round-skip proofs in types/round_skip.go, and extends peer-facing protocol messages.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/wal_iter.go`:
- [BLOCKING] internal/consensus/wal_iter.go:137-140: Retain idle-exit evidence across a subsequent certificate and restart
  Keeping only certificates here loses evidence that must also govern later certificate-driven round entries. At a non-initial version-2 height with `create-empty-blocks=false`, certificate 0 can start waiting round 1, followed by a non-nil round-0 polka whose block parts never arrive. The polka starts Propose in round 1, but this filtering at its Propose marker discards the round-0 prevotes. Certificate 1, recovered from skip votes signed before that polka arrived, can subsequently advance the live node to round 2. The live node still has the polka, so `waitsAfterRoundSkip` correctly refuses to wait there.

  If that node is round 2's proposer, a crash during proposal creation leaves a reachable durable WAL ending after certificate 1 but before round 2's Propose marker: `EnterProposeAction` calls `FlushAndSync` before `proposalCreator.Create`, while publishing the Propose marker only in its defer. The skipping iterator then supplies certificate 0, the round-1 Propose marker, and certificate 1 without the old polka. Replay restores Propose in round 1, but applying certificate 1 reconstructs waiting round 2. The marker-based repair cannot restore this final transition because its marker was never written. Earlier-round timeouts are stale, and no round-2 prevote need exist for the new resend path to deliver. If the other validators also lack the old polka, the restarted round-2 proposer can keep them waiting with heartbeats indefinitely.

  Preserve sufficient idle-exit evidence across subsequent certificates in the state/WAL replay layer, rather than relying exclusively on a successor Propose marker. Extend the replay regression with two certificates and a WAL ending after the second certificate, before its successor's Propose marker; the existing cases both end at a Propose marker and do not cover this crash boundary.

Comment thread internal/consensus/wal_iter.go Resolved
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

…rounds

With wal-skip-rounds-to-last, replay dropped the prevotes of the rounds
before the last, keeping only their certificates. A round a later
certificate leads to waits for transactions unless the node knew a polka
when it entered, and that polka can be in any earlier round. A node that
stopped after a second certificate, before it recorded the propose step
of the round it leads to, came back waiting there with no timeout
running, and as that round's proposer its heartbeats kept the others
waiting too.

A chain that skips rounds now replays every round of the height whatever
wal-skip-rounds-to-last says, both in choosing the WAL iterator and in
the proposal round jump. The iterator no longer needs to keep the
certificates of skipped rounds, so that is reverted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

At head 15a5aae, full-round replay preserves the earlier evidence required by subsequent skip certificates; 41 prior findings are fixed and the previously withdrawn finding remains withdrawn. The wakeup-prevote retransmission finding remains blocking: retries stop when the sender advances rounds, which can strand an honest waiting validator after temporary message loss. This was a static review only; current CI reports passing builds, all six test shards, both e2e jobs, lint, vet, vulnerability, protobuf, and mock checks, with review and PR Hygiene gates pending.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-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); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate change directly modifies consensus round advancement in internal/consensus/proposer_liveness.go and internal/consensus/state_add_round_skip.go, while adding quorum-signed skip certificates, signing interfaces, and peer-facing protocol messages.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/gossip_handlers.go`:
- [BLOCKING] internal/consensus/gossip_handlers.go:88-93: Reliably retransmit the prevote that wakes an idle successor round
  The periodic handler repairs the reported lost-wakeup case while the sender remains in the waiting peer's round, but this equality guard stops retries when the sender advances, even if the peer still announces NewRound. Ordinary vote gossip does not guarantee repair: it optimistically records dropped votes as delivered, and GossipVoteSetMaj23 repairs a majority for one BlockID, not HasTwoThirdsAny.

  A reachable example uses four version-2 validators with create-empty-blocks=false. Certificate 0 starts waiting round 1, whose proposer is Byzantine D. D sends a complete proposal to honest A and B but only fresh heartbeats to honest C. Temporary data/vote-channel congestion drops A/B's proposal and vote gossip to C after optimistic bookkeeping. A/B prevote the proposed block; D supplies a prevote for a different BlockID. A/B therefore collect three prevotes without a block majority and time out into nil precommits. D then supplies a non-nil precommit accepted by the application, giving them three precommits without a block majority. This is admissible with BaseApplication's accepting VerifyVoteExtension. The existing HasTwoThirdsAny path and precommit-wait timeout advance A/B to round 2.

  When congestion ends, this guard prevents A/B from retransmitting a round-1 wakeup prevote to C. Neither round-1 vote set has a block majority to initiate majority repair; certificate 0 cannot advance C beyond round 1; and round-2 proposals and votes are not ordinarily sent to a peer advertising round 1. D can keep sending valid round-1 heartbeats to C while withholding later-round votes. A/B cannot advance with two validators, and C has neither a proposal timeout nor an expiring silence watch, so the height can remain stranded after network recovery without a new transaction or reconnection.

  Retry a retained prevote from the waiting peer's round even after the sender advances. Extend the reactor regression to advance the active validators with mixed votes while dropping the idle peer's wakeup traffic, then require recovery after delivery resumes without another transaction or reconnect.

Comment thread internal/consensus/gossip_handlers.go Resolved
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

The waiting-peer resend sent a prevote only while this node was still in
the peer's round. Once the active validators left that round, a peer
still waiting there, whose copies of the round's votes were lost after
the channel took them, was sent nothing again: vote gossip counts those
votes as delivered, mixed votes give no majority to claim, and a waiting
round has no timeout. A prevote alone does not finish the round either:
the woken peer then waits at the prevote step for votes that never come.

On a chain that skips rounds, a validator peer at this node's height in
a round this node has left is now sent one of that round's prevotes or
precommits, by turns, at every pass until it announces a later round.
+2/3 precommits of any kind move it on from any step. A peer waiting in
this node's round is still sent that round's prevotes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Static verification at eb28fb5 confirms that 43 prior findings are fixed and the previously withdrawn request-ID finding remains withdrawn. One new blocking liveness gap remains: a bridge that advances using future-round prevotes cannot forward that progress to idle peers when it lacks their round's votes. Technical CI checks pass; PR Hygiene and the automated review remain pending, and no local builds or tests were run.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus round advancement in internal/consensus/proposer_liveness.go and internal/consensus/state_add_round_skip.go, adds quorum-signed skip certificate handling, and extends peer-facing protocol messages and signing interfaces.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/gossip_handlers.go`:
- [BLOCKING] internal/consensus/gossip_handlers.go:101-104: Forward later-round quorum evidence through bridges missing the peer's round
  When the peer is behind, this handler selects only votes from the peer's round. An honest bridge can lack those votes: addVoteDispatchPrevoteMw advances directly on a future round's HasTwoThirdsAny, without requiring progress evidence from intervening rounds. The bridge then holds a later-round quorum but sends nothing useful to its idle neighbors. Neither pickVoteForGossip nor GossipVoteSetMaj23 supplies the missing route; both select the peer's round or ProposalPOLRound, rather than the available future-round quorum.

  A reachable version-2 configuration uses a 100-member, threshold-67 quorum, 33 Byzantine validators, and create-empty-blocks=false. After certificate 0 starts waiting round 1, divide the honest validators into 34 active validators, one bridge, and 32 idle validators whose honest path to the active group crosses that bridge. The Byzantine round-1 proposer supplies a complete proposal to the active group and fresh heartbeats to the idle side. Temporarily lose round-1 proposal/vote delivery toward the bridge. The active 34 and Byzantine 33 can form mixed round-1 prevotes and precommits, advancing the active group to round 2 without a block majority. The Byzantine validators can then deliver 67 genuine mixed round-2 prevotes to the bridge, moving it to round 2 while its round-1 vote sets remain empty.

  Once the active group receives the bridge's round-2 announcement, it stops sending round-1 votes to that bridge. Even after congestion ends, the bridge's resends to its round-1 neighbors have no votes to select. Certificate 0 cannot advance them again, and continuing round-1 heartbeats prevent a new silence certificate. With the Byzantine validators withholding further ordinary votes, only 35 honest validators participate in round 2, below the required 67, so the height remains halted without a continuing honest-network partition.

  Add bounded, retryable forwarding of available later-round quorum evidence, or an equivalent catch-up mechanism. Cover a partial-mesh bridge that reaches the later round solely through future-round votes; the current four-validator regression covers senders that retain the old round's progress votes.

Comment thread internal/consensus/gossip_handlers.go Resolved
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

…round +2/3 voted in

A node can reach a later round on that round's votes alone: +2/3 of any
kind prevoting or precommitting in a round takes a node there from any
round before it. Such a node holds no vote of the round its peers wait
in, so resending the votes of the peer's round sent them nothing, and a
peer waiting under a live proposer's heartbeats stayed there.

A validator peer in a round this node has left is now also sent, by
turns with its own round's votes, the prevotes or precommits of the
latest later round that +2/3 of any kind voted in. The peer keeps them
as a catch-up round of each voter, two per voter at a height, and drops
any more without blaming the sender.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

At b8317fe, the code addresses 44 prior findings; the previously withdrawn heartbeat request-ID finding remains withdrawn. One blocking liveness issue remains: latest-round retransmission can permanently target evidence that a lagging validator's exhausted catch-up allocation quotas reject. This was a static-only verification; current CI shows the build, all six unit-test shards, application tests and both e2e jobs passing, but golangci-lint fails on a preallocation diagnostic introduced by this change.

🔴 1 blocking | 🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus round advancement in internal/consensus/proposer_liveness.go and internal/consensus/state_add_round_skip.go, and introduces signed quorum certificates and their verification in types/round_skip.go.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/gossip_handlers.go`:
- [BLOCKING] internal/consensus/gossip_handlers.go:135-144: Make future-round catch-up resumable after allocation quotas are consumed
  This helper stops at the newest quorum round on every pass, but the receiver cannot necessarily allocate that round. HeightVoteSet.addVote permits each signed validator identity to initialize only two unknown rounds at a height; it records an allocation only against the first vote that creates the round, and never reclaims those slots before the height resets. Subsequent votes can complete an existing round regardless of their identities' quotas, but retransmitting a different, unallocated round cannot do so.

  A reachable version-2 ordering uses a 100-validator, threshold-67 quorum: 34 active honest validators, an honest bridge, 32 waiting honest validators, and 33 Byzantine validators. The Byzantine round-1 proposer keeps the waiting side idle with heartbeats, while the active group advances through mixed prevote/precommit quorums without committing. The bridge follows future-round quorums and holds no round-1 votes. During temporary message loss, authentic votes from the 68 participating identities can each initialize two distinct otherwise-unknown rounds above round 2 at every waiting receiver, with all completing votes lost. Include two votes signed by the bridge in intermediate rounds. None of these partial sets advances the waiting nodes. When the bridge subsequently holds a newer quorum round R, all identities represented in that quorum have exhausted their allocation slots, so every possible first vote for R is rejected with ErrGotVoteFromUnwantedRound.

  Restoring honest delivery does not repair this state: this helper sends only the latest future quorum, ordinary vote and majority gossip select the peer's round or ProposalPOLRound, and certificate 0 cannot advance a node already in round 1. The Byzantine validators can stop ordinary voting while continuing round-1 heartbeats, leaving only 35 active honest validators against a threshold of 67.

  Provide a bounded, resumable transfer coordinated with catch-up admission, so an admitted partial target can be completed or obsolete allocations can be safely replaced. Add a regression with genuinely untracked future rounds, exhausted first-voter allocation quotas, and changing targets during loss. TestReactorWaitingValidatorsFollowANodeThatSkippedAhead uses round 2 from round 1, which EnterNewRoundAction already pretracks, so it does not exercise this failure.
- [SUGGESTION] internal/consensus/gossip_handlers.go:107-108: Fix the change-induced preallocation CI failure
  The golangci-lint check on this exact head fails in run 42324194197 with `internal/consensus/gossip_handlers.go:107:4: Consider preallocating voteSets (prealloc)`, followed by `1 issues` and `issues found`. The new append of later-round vote sets causes this diagnostic. This is not a speculative allocation optimization: it is the confirmed cause of a failing code check introduced by the change. Allocate sufficient capacity for the peer-round and later-round vote sets before appending.

Comment thread internal/consensus/gossip_handlers.go Resolved
Comment thread internal/consensus/gossip_handlers.go Resolved
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

golangci-lint's prealloc flagged the vote sets built for a peer in an
earlier round.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Static verification at head 0569cc6 confirms 45 prior findings are fixed and one remains withdrawn, but the future-round catch-up admission blocker remains valid. The preallocation change preserves newest-only retransmission, which cannot recover when every available initializer has exhausted its receiver-side allocation quota. Current CI passes all code checks, including the successful rerun of tests (04); the original failure was a private-validator endpoint connection timeout, while review and hygiene checks remain pending.

🔴 1 blocking

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff changes consensus round advancement in internal/consensus/proposer_liveness.go and internal/consensus/state_add_round_skip.go, and adds quorum-signed skip certificates and their verification in types/round_skip.go.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer
🤖 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/gossip_handlers.go`:
- [BLOCKING] internal/consensus/gossip_handlers.go:137-145: Make future-round catch-up resumable after allocation quotas are consumed
  (existing thread: https://github.com/dashpay/tenderdash/pull/1572#discussion_r4499472402)
  This helper stops at the newest round with HasTwoThirdsAny on every pass, but the receiver cannot necessarily allocate that round. HeightVoteSet.addVote rejects an absent round when its first vote's validator already has two peerCatchupRounds entries (internal/consensus/types/height_vote_set.go:183–195). Only the validator whose accepted vote creates a round spends an entry; subsequent votes in that existing round do not. Those entries persist until Reset, so retrying a rejected new target does not restore admission.

  A reachable failure uses a 100-validator, threshold-67 quorum: 34 active honest validators, one honest bridge, 32 idle honest validators, and 33 Byzantine validators. After a certificate starts idle round 1, the active group can advance through mixed-vote rounds with Byzantine assistance without committing, while the bridge follows future-round quorums without receiving round-1 votes. During finite message loss, sparse authentic votes from distinct future rounds can initialize two incomplete round sets per participating identity at each idle receiver, without completing any quorum there. Once the bridge holds a newer quorum round R that those receivers have not allocated, every available initializer of R can have exhausted its two entries. All retransmitted R votes are then rejected. The bridge holds no round-1 votes, and this helper never falls back to the older admitted targets. Ordinary vote and majority gossip also select the peer's round or proposal POL round, not those incomplete future targets. A Byzantine round-1 proposer can keep the idle validators' silence watches refreshed while withholding further ordinary votes, leaving only 35 honest validators active despite restored honest connectivity.

  The new forwarding route fixes the earlier missing-bridge path, but its receiver admission must also be resumable. Coordinate a bounded catch-up target with admission, complete an admitted target, or safely replace obsolete partial allocations without removing the allocation-abuse bound. Add a regression with genuinely untracked rounds, exhausted initializer quotas, and target changes during loss. TestReactorWaitingValidatorsFollowANodeThatSkippedAhead uses round 2 from waiting round 1; EnterNewRoundAction already pretracks round 2, so it does not exercise this gate. The current preallocation-only commit changes none of these conditions.

@thepastaclaw

Copy link
Copy Markdown

lklimek, you're right: dismissing my review is an authoritative maintainer override, and leaving its check blocking is a Reviewsys bug.

Repair PR: thepastaclaw/review-system#58. It detects explicit dismissal of the exact bot review and updates its check to completed / neutral — Review dismissed — maintainer override, preserving who dismissed it, when, and why. It explicitly does not claim technical validation passed. Unrelated approvals and resolved threads do not override a review, and persisted dismissal evidence prevents reconciliation from resurrecting the original verdict.

Tests: 1,237 passed, 1 skipped, including 76 check-run tests; lint, formatting and type checks pass locally.

Not deployed, and this PR's check has not actually changed yet. I just re-read check 128265152257: it still reports failure for head 0569cc637a3ad3eef0e304fdf6d8f1ae6bd932b0, despite review 5809594985 being DISMISSED. The existing supported checks CLI only shows a plan/probes installation availability; it cannot publish a one-shot repair. I did not access production credentials, patch the database, change Tenderdash code, or start another review. The normal authorized deployment of the fix is still needed for the daemon to repair it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants