Repository navigation
fix(signing): refuse rounds an earlier run can have signed after a restart - #1098
Conversation
…ter a restart The signing gates judged only the state files on disk. An older but consistent copy of the signing record, commit lock and prevote record, or a prevote record lost while the signing record survived, let a restarted node sign the same height again in a round it had already signed in. At boot, before the clock is calibrated, every signature is held. Once it is, the node sets an in-memory floor at network time plus the maximum round lead, raised to the highest round found in the state files, and the block signature, prevote and precommit gates refuse any round at or below it. The runbook documents the floor and what it does not cover.
…k file A commit lock at (H, X, round p) beside a signing record at (H, Y, q > p), or for another block in the lock's round, is an older copy of the lock: every precommit is signed after its lock is written. Candidate selection still used that lock, so the node could prevote X or a block with a proof of lock from a round below q, and the precommit path judged the lock rule against it. lockAt no longer reports a lock the record outranks, so candidate selection and getLock take the record-bound path, and the precommit path judges by the record and rewrites the lock. The comment on the write order of lock and record now matches the code.
…e restart floor The restart signing floor is derived once from the boot clock. A single unsynchronised NTP answer (1900) or a stray reading could reopen rounds an earlier run signed, or block signing until the next restart. Boot now needs two readings that agree within 30 s, rejects implausible times, and stops when no trusted clock is found.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Caution Review failedThe pull request is closed. No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Trusted boot clock calibration src/libs/utils/calibrateTime.ts, src/libs/utils/calibrateTime.boot.test.ts, src/index.ts, scripts/test-consensus.sh, docs/runbooks/validator-key.md |
Boot calibration requires agreeing NTP readings and rejects non-finite or implausibly old timestamps. Startup uses the boot calibration function, and the consensus test script includes its tests. |
Restart floor calculation and enforcement src/libs/identity/signingRecord.ts, src/libs/consensus/v2/commitLock/restartFloor.ts, src/libs/consensus/v2/commitLock/restartFloor.test.ts, src/libs/consensus/v2/commitLock/prevoteRecord.ts, src/index.ts, docs/runbooks/validator-key.md |
Startup holds signing until the floor is set from calibrated time and the highest available recorded round. Signing and prevote paths reject rounds at or below the floor. The runbook documents the floor, clock requirements, and stated limits. |
Signing-record-aware lock recovery src/libs/consensus/v2/commitLock/lockStore.ts, src/libs/consensus/v2/commitLock/precommit.ts, src/libs/consensus/v2/commitLock/candidate.ts, src/libs/consensus/v2/commitLock/*test.ts |
lockAt disregards a lock when a matching signing record outranks it. Precommit restoration and candidate tests cover older or conflicting lock state. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant main
participant getBootTimestampCorrection
participant NTPServers
participant armRestartSigningFloor
participant SigningStateFiles
participant signingRecord.evaluate
participant recordPrevoteRound
main->>main: holdSigningUntilRestartFloor
main->>getBootTimestampCorrection: request trusted clock correction
getBootTimestampCorrection->>NTPServers: request paired readings
NTPServers-->>getBootTimestampCorrection: return readings
getBootTimestampCorrection-->>main: return correction
main->>armRestartSigningFloor: pass identity file and network timestamp
armRestartSigningFloor->>SigningStateFiles: read recorded rounds
SigningStateFiles-->>armRestartSigningFloor: return available state
armRestartSigningFloor->>main: set and return restart floor
main->>signingRecord.evaluate: evaluate signing request
signingRecord.evaluate-->>main: refuse rounds at or below floor
main->>recordPrevoteRound: record prevote request
recordPrevoteRound-->>main: refuse rounds at or below floor
Possibly related PRs
- kynesyslabs/node#1095: Updates lost commit-lock handling in the same consensus signing path and uses signing records during recovery.
- kynesyslabs/node#1097: Changes the boot error path used when trusted NTP readings cannot be obtained.
Suggested reviewers: hackobi
Merge Risk: 
c60d2
-
Two consistent but incorrect future clock readings can prevent the validator from signing for an extended period; reject implausibly future NTP times before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: refusing rounds that may have been signed before a restart. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 16 functions across 14 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the clock at dawn
Then marks the rounds that must be gone
Old locks yield when records say
The newer signed round leads the way
Safe startup hops into the day
Comment @coderabbitai help to get the list of available commands.
 [High impact] The PR appears safe to merge; no new actionable issues were found.
|
…restore-floor # Conflicts: # scripts/test-consensus.sh
Problem
After a restart, a validator could sign a second block in a round an earlier run of the same key had already signed. This could happen when:
.prevote.jsonwas lost beside a surviving.signed.json;Change
main()until the floor is set, because the RPC server can start a round before calibration.recordOutranksLock).docs/runbooks/validator-key.mdexplains the floor, its limits and the clock requirement.Evidence
scripts/test-consensus.sh: 69 files, 0 failed.Refs kynesyslabs/mainnet-launch#2
Summary by CodeRabbit
Reliability
Documentation