feat: header timestamp consensus rules, index reorg rollback, mempool policy, fuzz targets - #14
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7e45e86db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ebb0fb445
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 299bec57f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
352817e to
ad9d9c4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9af200ba78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Header acceptance checked proof of work and difficulty retargeting but never the timestamp, so two consensus rules went unenforced. Both now run in accept_headers: - median-time-past: the timestamp must exceed the median of the parent and up to ten ancestors. Batch parents count, because accept_headers inserts each header before moving to the next. - future drift: at most 7200s ahead of the clock. The clock is read once per batch and passed in, so validation is a pure function of its inputs. Five tests pin the boundaries: at the median (reject), median+1 (accept), exactly +7200s (accept), +7201s (reject), and a parentless header (exempt). is_peer_fault splits the two deliberately. A median violation is decided by the chain the peer itself sent, so it is the peer's fault. Future drift is judged against our clock, and a wrong local clock would otherwise let us ban every honest peer and partition ourselves.
Three relay-policy gaps closed. 64 mempool tests pass. standardness: Bitcoin Core's IsStandardTx — version 1..=3, 400000 weight cap, push-only scriptSig under 1650 bytes, standard output types, one OP_RETURN of at most 83 bytes, and a dust floor. One error variant per rule so a caller can report which policy failed. orphan: a transaction arriving before its parent was dropped, losing ordinary out-of-order relay. The pool releases a child only when its LAST missing parent arrives, and bounds itself by both entry count and total weight — count alone lets a few enormous transactions pin memory. Expiry takes the clock as a parameter so the tests are deterministic. fee_estimator: estimatesmartfee bucketed the current mempool and called that an estimate, observing nothing about what actually confirmed. This records confirmation delay per fee-rate bucket with per-block decay and returns None below the evidence threshold. A refusal beats a fabricated number. The estimator's hooks are not yet called from the apply path and the standardness check is not yet called from mempool acceptance; both are wired in a later commit.
crates/index could only move forward, so a chain reorganization left stale rows and the Electrum server would answer with transactions no longer in the chain. rollback_block deletes the rows an ingest of the same block at the same height wrote, deriving keys from the shared row-construction helper rather than a second key format that could drift. The method sits on the IndexerLike trait, because the node holds the indexer as a trait object and an inherent method would be unreachable from the disconnect path. Its default returns UnsupportedRollback rather than succeeding: a successful no-op would let the node advance its tip believing the index is consistent, which is the exact failure this method exists to prevent. Eight existing IndexerLike impls compile untouched and fail loudly only if a reorg is ever driven through them. The real Indexer overrides it.
Two policies the project requires and never wrote down, grounded in the storage traits, the versioned snapshot format, and the pinned toolchain rather than generic advice. Gaps in current behaviour are named as gaps with a recommended rule.
Review caught a real bug in the rollback landed one commit earlier, and the fact it landed with zero tests is why it survived. rollback_block writes its deletions straight to the store, but rows buffered in pending_rows were untouched. An ingest inside an open batch followed by a rollback therefore left the block's rows pending, and the next end_batch wrote them back, resurrecting the very block that was just disconnected. Flushing first fixes it and keeps the all-or-nothing property, since a failing flush returns before anything is deleted. Four tests, none of which existed: - ingest then rollback restores the exact pre-ingest row set, asserting first that the fixture wrote to all four column families so the test cannot pass vacuously - rolling back a never-indexed block is not an error - a repeated rollback is observationally inert - begin_batch, ingest, rollback, end_batch leaves the index empty The last is mutation-verified: removing the flush fails that test and only that test.
The workspace had none. Five targets, each calling a real public entry
point and consuming the Result rather than unwrapping it, since the
point is to find panics and not to cause them:
p2p_message wire::read_message
block_decode the node's inbound block decode path
tx_decode transaction consensus decode
script_eval the portable script interpreter, length-bounded so the
fuzzer does not simply time out
utxo_snapshot read_snapshot, the deserializer that runs against
on-disk state after a crash
fuzz/ declares its own empty [workspace] table so it stays out of the
root workspace, which is the standard cargo-fuzz layout. Verified with
cargo check inside fuzz/: zero errors.
Both were asserted in doc comments and verified by nothing. Injected write failure: a store that delegates to RocksDB but fails write proves a failed rollback surfaces as Err and leaves every row in place. Scope stated honestly — this store rejects every write, so the test pins the failure contract, not partial-batch atomicity. Atomicity within a batch is a property of KvStore::write and belongs to the backend, not to this method. Default refusal: a minimal IndexerLike that persists nothing and does not override rollback_block must return UnsupportedRollback. A silent successful no-op there would let the node advance its tip believing a stale index is consistent, which is the failure the erroring default exists to prevent.
The design was worked out in full while the implementation was not written. Recording it so the next attempt starts from the conclusions rather than rediscovering them, and so the g10 gap stays visible. The load-bearing point is that the UTXO set and the index have different crash models and must not share one mechanism: the UTXO set is RAM-resident with checkpoint durability, so a crash discards partial mutation and no durable phase marker is needed, while the index is on disk and must roll back in one atomic batch. Also records two traps found the hard way: UtxoSet::undo_block is NOT idempotent, because a competing block can re-create an outpoint between two applications and the second undo then deletes a live output; and undo records must be retained rather than deleted on disconnect, since reorg flip-flop is normal.
Layer 1 of node reorg: the storage and encoding a block disconnect needs. No behaviour change yet, since nothing writes undo records so far. ColumnFamily::UndoData across the enum, its ALL list, and all four backends. The codec's two load-bearing properties: - versioned, so a record from an older binary is refused rather than misread - bound to its block hash, so a stale record from an abandoned branch can never be replayed against a different block at the same height, which would silently corrupt the UTXO set Decoding is strict because these bytes come off disk and a corrupt record must not be mistaken for a valid one: unknown version, wrong block, truncation, trailing bytes, an entry count larger than the remaining bytes can hold, a non-canonical coinbase flag, and any repeated outpoint are all refused. Duplicate detection spans both halves, because a block cannot legally both restore and remove the same outpoint. Output payloads reuse rust-bitcoin consensus encoding rather than a hand-rolled layout. Ten tests, one per rejection. Two test fixtures were silently invalid and the new header timestamp rule exposed them: chain and node helpers built headers at time 1, 2, 3 on top of a regtest genesis timestamped 1296688602, so every one violated median-time-past. They now advance from the genesis time. The node helper also never mined its nonce and merely relied on regtest's easy target accepting nonce=height, so changing any other field broke proof-of-work; it mines now.
Layer 2 of node reorg. The node now records, for every connected block, what disconnecting it would require. Nothing reads the records yet; the disconnect path is layer 3. The undo batch is derived in the SAME pass as the forward UTXO changes, not by a second function. Both sides share one set of filters for OP_RETURN outputs, oversized scripts, and same-block spends, so they cannot drift apart. A second pass duplicating those filters was written first and deleted for exactly that reason. UndoStore is a mandatory handle, not an Option. Undo is consensus state: if a node may run without it, it can advance its tip into a chain it cannot leave. Both implementations are real — an in-memory store that round-trips, and a KvStore-backed one over the new UndoData column family. There is deliberately no no-op implementation, which would recreate the silent failure the type exists to prevent. Persistence runs before the block body, the index, and the UTXO commit. All three are derived state for a block that is about to apply; if the undo record cannot be written the block must not apply at all. A spend with no resolved prevout is likewise fatal rather than skipped, since the record would otherwise be quietly unable to restore that output. Proven, and the mutation matters: moving persistence after the UTXO commit while keeping the error fatal fails the ordering test on the UTXO assertion. Demoting the error to best-effort was the first mutation tried and is weaker, since it only shows the test wants an error. Not yet proven: durability of KvUndoStore across a restart. The tests here use the in-memory store. Layer 3 needs the reopen test before the disconnect path can rely on it.
The in-memory store cannot show that a record outlives the process that wrote it, which is the only property that matters for a node restarted mid-chain. This closes a real Fjall backend, reopens it at the same path, and checks every restored field rather than the byte length: outpoint, spent output, coinbase flag, creating height, and the removes list. Mutation-verified: corrupting the stored bytes before the write fails the round-trip assertions, so the test reads persisted content rather than passing vacuously. Also records why persist_undo does not fsync. Nothing in the apply path does; durability comes from the crash_recovery replay watermark, and undo records are regenerated by that replay since they derive from the block and the pre-block UTXO state. Syncing only this write would put an fsync on every connected block while the UTXO commit beside it stayed journaled.
The trait said 'durable storage' and persist_undo cited crash-recovery replay as the safety net. Neither holds. Nothing in production writes the crash-recovery metadata. The only producer is NodeState::record_synthetic_block_for_recovery, whose own doc calls it a test helper, so read_meta returns None on every real boot and recover_if_needed takes its 'fresh node' path. The replay guarantee does not exist, and a comment pointing at it is worse than no comment. What is true and now documented: records survive an orderly restart, proven by the reopen test; block connection fsyncs nowhere, so a crash can lose the UTXO commit and this record together; an fsync here alone would slow every connected block without making either recoverable. Closing it needs a real replay path, which the node does not have.
Layer 3 of node reorg. Reads a block's undo record and restores the UTXO set, the transaction index, and applied_tip. No production caller yet; layer 4 wires the branch-switch loop. Takes no height. The applied tip already knows it, and a second source for the same fact is a second source of disagreement, since both the undo key and the index rollback are keyed by height. The parameter was written first, then deleted: a caller cannot pass a stale height if there is no parameter to pass. Checks the body against its own header. The header hash proves the caller named the right block, not that they handed over that block's transactions, and index rollback walks the body. A forged body under a matching header would delete rows for transactions the block never contained. Ordering, all four claims mutation-verified by breaking them one at a time and watching the matching test fail: - read and decode before mutating, so a missing or corrupt record costs nothing - index before UTXO, so an indexer that cannot roll back stops the disconnect instead of being skipped - UTXO before the tip - the tip last, so a concurrent reader never sees a tip whose outputs are still spent The doc classifies every handle connection touches as restored, owed, or deliberately retained, and names the one partial-failure window that remains open. An earlier draft called this the exact reverse of connection; coin_stats, the filter index, and the RPC caches make that false, and they are prerequisites for wiring it.
The note still said undo data is never persisted and plan_reorg has no caller, which was true when it was written and is now half wrong. It also said recovery is checkpoint-plus-replay. Corrected: - Status names what each layer actually produced, and says plainly that disconnect_block is a primitive with no caller because coin_stats, the filter index, and the RPC caches are still owed. - The replay claim is gone. run.rs does call recover_if_needed, but no production code writes the metadata it reads, so a normal boot finds no sidecar and takes the fresh-node path. - A first correction said a crash loses the UTXO commit and its undo record together. That contradicted this note's own central point: the undo record is a journaled KV write and the UTXO set is RAM behind checkpoints, so either can outlive the other. - Work remaining is split into done, prerequisites for wiring, and layer 4, instead of listing finished work as pending. CONCEPTS.md gains 'undo record' and 'owed derived state' per the repo rule that terms and learnings move together.
Three claims were wrong or unsupported and are now fixed together, since they are the same fact stated in three places. The partial-failure window is worse than documented. UtxoSet::undo_block goes through commit_adds_and_removes, which walks shards: the serial path returns on the first failing shard after earlier ones committed, and the parallel path collects errors while other shards succeed. A failed undo therefore leaves the set partly undone, not untouched. Retry converges only if both rollbacks are idempotent. Each individual UTXO operation is, since restoring a live output and removing an absent one are both no-ops, but that is stated as reasoning and not proven, and index rollback idempotence is not proven either. Both are listed as owed. CONCEPTS.md's commit-point entry claimed every step before the commit point is safe to re-enter and that no durable phase marker is needed. The first is refuted by the window above; the second by a checkpoint that can retain a UTXO commit whose undo record was lost with the journal. The entry now says the boundary question is open and belongs with the recovery protocol.
…odel The Guidance section still carried both claims the rest of the note had already dropped: that every step before the commit point is safe to re-enter, and that the phase marker was a mistake protecting an impossible failure. Rule 1 now asks which of atomic, compensatable, or recoverable applies per store, because 'safe to re-enter' assumed each step either happens or does not, and the shard-walking UTXO commit does not honour that. Rule 2 keeps its point about mechanisms for impossible failures and drops the example, which was not one: a checkpoint can retain a UTXO commit whose undo record was lost with the journal, and a durable boundary is a way to detect it.
Connection pushes a BlockRecord that RPC serves getblock from. Leaving it after a disconnect means answering for a block the chain no longer contains. Removal is opportunistic on purpose. The vector is a best-effort in-process cache, not authoritative state: it starts empty on every boot while applied_tip resumes from a checkpoint at height N, and the prune service removes records from it. An earlier version of the comment called a missing tail a bug; making it one would refuse the first disconnect after any restart. The hash check is what stops the pop from taking a record that is not ours, and the tail can only be ours or gone, since disconnect runs on the applied tip and connection pushed that tip's record last. Two entries leave the owed list. The transactions map never needed one: connection does not populate it, which an existing test already pins. Also records that retry is not a recovery strategy for the partial failure window. Each UTXO operation looks idempotent on the set, but undo_block runs through commit_adds_and_removes, which fires UtxoSet's listener, and coin_stats is one. A second pass re-emits callbacks for operations that changed nothing, so a cumulative listener double-counts even where the set converges.
Coinstats needed less than the owed list said and more than the first attempt gave it. Less: the listener already does most of it. CoinStatsListener is registered as the UtxoSet change listener, so undo_block delivers the inverse as ordinary inserts and removals and the per-coin fields reverse themselves. A second feed on the disconnect path would double-count. More: finish_block sets height and tx_count directly on connect, and no listener callback touches those. A first version of the test compared two per-coin fields, passed, and hid exactly that. Comparing the whole snapshot fails, which is how the gap surfaced. rewind_block is the inverse of finish_block for those two fields. It is fallible, not saturating. A saturating subtraction turns a second rewind of the same block into a silent clamp, and that clamp is the failure the function exists to catch. Both invariants are checked before either field moves, so a refusal changes nothing. The precondition is also checked at the top of disconnect_block, before the index rollback, because the rewind itself has to run after undo_block and by then a refusal is no longer free. MuHash is compared by digest. It is a ratio, so inserting a coin and removing it leaves numerator equal to denominator without returning to the limbs it started from. The digest is the observable value and it does return; comparing the struct would have failed for no real reason. Mutation-verified: removing the rewind fails the round trip, swapping checked for saturating fails the underflow test, and removing the height guard fails two. The saturating swap passed before these tests existed, which is why they do.
Looking at the filter index for disconnect found a live bug in connect. previous_filter_header ended in .ok().flatten().unwrap_or_default(), so a storage error or an absent row both produced zero. Zero is the genesis parent header, and a BIP157 header is a hash over its predecessor, so that restarted the chain mid-way. Every later header still verified against its own neighbour and none of them verified against the real chain. A light client checking filter headers would have been told a consistent lie. It now returns Option and writes nothing when there is no predecessor header. Skipping is right where both refusing and zeroing are wrong: filters are optional derived state written after the block has already applied, so a missing row must not fail the block, and a wrong row must not be written. The index simply stops being available from that point, which a backfill can repair. Core avoids the question by keeping the index separate and backfilling from genesis. The fixtures were hiding it. Two tests installed genesis with applied_header_tip, which writes a header and no filter, then asserted that block 1 chained from zero. That assertion encoded the bug. They now seed genesis's real computed filter and assert block 1 chains from genesis, which is what a real node does, since production applies genesis through apply_block. Mutation-verified, on the second attempt. The first mutation passed because a seeded fixture never reaches the missing-header path; the new test drives that path directly and fails when zero comes back. Disconnect also replaces the filter header cache with the parent's entry instead of leaving the removed block there. plan_disconnect is split out because the function crossed the length limit, and the seam the limit forced is the right one: everything that can refuse, then everything that mutates. A check that can live in the plan must, since refusing there is free.
Listed as open in three places: prove rollback idempotence, make the UTXO commit atomic, or add compensation. The first is refuted, so the question is closed rather than carried. Each UTXO operation is idempotent on the set. Restoring a live output and removing an absent one both leave it unchanged. That is not the contract: undo_block runs through commit_adds_and_removes, which fires UtxoSet's change listener, and CoinStatsListener is registered as one. A second pass re-emits callbacks for operations that changed nothing, so a cumulative listener double-counts even where the set converges. So a failed disconnect is fatal. The caller stops applying blocks and reports where it wedged, rather than trying again. That is the same poison path the branch-switch layer already needs for a failed compensating rollback, which makes it one mechanism instead of two. The crash-time question is separate and stays open: a checkpoint can retain a UTXO commit whose undo record was lost with the journal, and whether a durable boundary should detect that belongs with the recovery protocol.
A comment said which failures were safe. A caller cannot read comments, so DisconnectError says it instead: Refused means nothing was touched, Fatal means the rollback started and stopped part-way. Refused covers more than expected. Index rollback flushes buffered rows and then issues every delete in one write batch, so a failure at either step leaves the rollback un-started. An indexer that cannot roll back at all therefore refuses freely rather than wedging the node, which is what its test already asserted about the UTXO set and the tip. Fatal begins at the UTXO commit, which walks shards and can stop after some have committed. It carries the block hash and height, because the contract is that the operator gets told where it wedged and a caller should not have to reconstruct that. Retry is not the answer for Fatal and that half is settled: each UTXO operation is idempotent on the set, but the commit fires the set's change listener and coinstats is registered as one, so a second pass double-counts where the set converges. The recovery half is not built, and the design note now says so rather than marking the row done. The poison is a return value, so a restart clears it while an index rollback that already reached disk survives. That needs a durable marker written before mutation, a startup that refuses or recovers until it clears, and gating for RPC, P2P and Electrum rather than the apply path alone.
Three fixes after the rebase onto merged main. The conflict resolution concatenated two sections, so 'Commit point' lost its blank line. 'Commit point' claimed a failed rollback poisons the path. It does not yet: DisconnectError classifies the failure, and the poison is a return value that a restart clears while an index rollback that reached disk survives. It also said index rollback leaves the index rolled back on failure, which is backwards — that rollback is one write batch, so a failure leaves it un-started, which is why it is now Refused. 'Owed derived state' listed five things as owed. Four are done and the answers differed, which is the useful part: coinstats needed only its block-level fields, the filter index needed nothing because its rows are hash-addressed, the blocks cache needed a pop, and transactions never needed anything because connection does not populate it. What is still owed is recovery, not derived state.
dc1375b to
5e82fa0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eee87c611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(crate::reorg::ReorgError::MissingBody { height, .. }) => { | ||
| tracing::trace!(height, "block sync: heavier branch still downloading"); |
There was a problem hiding this comment.
When the heavier chain forks below the applied tip, treating MissingBody as ordinary download lag cannot make progress: send_getdata_for_pending_blocks and expected_block_hashes request only from applied_tip + 1, so replacement blocks at heights at or below the old applied tip are never requested. For example, after a fork at H-1 replaces H and extends to H+1, sync can request H+1 but never the winning H, causing every reorg attempt to return MissingBody; schedule downloads from the common ancestor when the tips are on different branches.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| let body = handles | ||
| .block_body_store | ||
| .as_ref() | ||
| .and_then(|store| store.load_block_body(node.height, node.hash).ok().flatten()) |
There was a problem hiding this comment.
When newly downloaded blocks make a competing branch heavier, their bodies exist only in BlockStager, while this loader reads exclusively from block_body_store; the repo's production persist_block_body call occurs during block application, which these side-branch blocks have not undergone. The tests mask this by manually inserting winning bodies into MapBodyStore, but production therefore reports MissingBody forever even after receiving them; consume the staged bodies or persist received side-branch bodies before switching.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| // would apply blocks on top of the tear. Closing admission | ||
| // gives the live node the same answer the restart gets: | ||
| // every further apply and disconnect fails. | ||
| self.handles.admission.close_permanently(); |
There was a problem hiding this comment.
When a disconnect returns Fatal, this only closes future apply admission and logs; it does not set the separate shutdown flag used by the RPC, P2P, and Electrum loops in run.rs. Those servers therefore continue answering from the explicitly torn UTXO/index/tip state until an operator restarts the process, so propagate the fatal result to node shutdown or gate every serving path as well.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| /// hold a guard: it needs the flag set and every later `enter` refused, | ||
| /// including the one that would otherwise apply the next block. | ||
| pub(crate) fn close_permanently(&self) { | ||
| self.closed.store(true, Ordering::Release); |
There was a problem hiding this comment.
When another connect or window has already acquired the admission read permit and is waiting on chain_transition while the disconnect fails, setting only this flag does not revoke that permit. Once the fatal disconnect releases the transition lock, the waiter can still enter apply_block_admitted and mutate the torn chainstate despite admission being "closed"; close while the transition is still held, drain/hold the write barrier, or recheck the closed state after acquiring the transition lock.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| return None; | ||
| } | ||
| let tree = self.handles.block_tree.read(); | ||
| let ancestor = tree.node_at_height_from(chain_tip.tip_id, applied.height)?; |
There was a problem hiding this comment.
When the published best header chain has greater chainwork but a lower height than the applied chain, node_at_height_from(..., applied.height) returns None here and the ? suppresses the branch switch. Since BlockTree selects its tip by chainwork rather than height, this is a valid state—for example after branch-dependent difficulty changes—and the node then remains on a lower-work chain indefinitely; compare ancestry via plan_reorg or explicitly treat a shorter best-work tip as a reorg target.
Useful? React with 👍 / 👎.
| self.block_body_store.sync()?; | ||
| let applied_tip = self.applied_tip.load_full(); | ||
| crate::checkpoint::write_checkpoint_from_dir( | ||
| let written = crate::checkpoint::write_checkpoint_from_dir( |
There was a problem hiding this comment.
When a fatal disconnect leaves an InFlight marker, a later ordinary shutdown reaches this call and publishes the partially rolled-back UTXO/coinstats as the new CURRENT checkpoint before disarm_disconnect silently declines to clear the marker. Startup initially refuses because the marker remains, but the operator-facing recovery instruction removes chainstate—and therefore the marker—while leaving chainstate-checkpoints, so the next open can restore this poisoned checkpoint; check the marker phase before publishing any checkpoint and fail without advancing CURRENT.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| return Err(ReorgError::Refused { | ||
| stopped_at: height, | ||
| source: Box::new(error), | ||
| }); |
There was a problem hiding this comment.
When one or more disconnects succeed and a later one is refused—for example when a pruned deep reorg crosses into missing undo data—this return leaves applied_tip on the losing side branch while BlockTree still publishes the competing branch as best. write_headers requires the applied tip to be a prefix of that best chain, so clean shutdown cannot checkpoint this supposedly consistent lower tip, the earlier RolledBack marker remains, and restart requires a full resync; preflight every disconnect before the first mutation, reconnect the completed prefix on refusal, or make checkpoints represent this state.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
|
|
||
| for (block, height) in disconnect { | ||
| match crate::apply::disconnect_block(handles, &block) { | ||
| Ok(_) => {} |
There was a problem hiding this comment.
When a block is successfully disconnected, its non-coinbase transactions are dropped with the loaded block here and never returned to the mempool, even though the connect path previously evicted them as confirmed. A wallet transaction that existed only in the losing branch consequently disappears from the node instead of being reconsidered and relayed on the winning chain; collect disconnected transactions and reaccept the ones not confirmed or conflicted by the replacement branch.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| a node you should rely on. | ||
|
|
||
| Also incomplete: relay, metrics coverage, and parts of the CLI and RPC surface. | ||
| `plan_reorg` exists in the chain crate with no production caller. |
There was a problem hiding this comment.
Fresh evidence after the earlier status corrections is that later commits added switch_to_branch and now call it from the sync loop, while this documentation index still says the node cannot reorganize and that plan_reorg has no production caller; the architecture solution repeats those claims, whereas the root README and CONCEPTS.md say the opposite. Reconcile these durable guides so operators and maintainers see the actual implementation and its remaining limitations rather than mutually exclusive status reports.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
Serialize whole branch switches, bind preloaded bodies to their plans, preserve checkpoint and fatal-admission invariants, and correct the reviewed consensus, policy, and documentation contracts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e21d64c066
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(source) => { | ||
| failure = Some(ReorgError::ConnectFailed { | ||
| stopped_at: body.height.saturating_sub(1), | ||
| source: Box::new(source), | ||
| }); | ||
| break; |
There was a problem hiding this comment.
When a higher-work branch contains a consensus-invalid block, this path keeps that branch published as the best header chain after disconnecting the previously valid chain. Any valid prefix remains applied, while apply_buffered_blocks subsequently drops and re-downloads the same invalid blocker because neither it nor its descendants are marked NodeStatus::Invalid; the node cannot return to the old valid branch until another header chain overtakes the invalid branch's work. Classify permanent validation failures and invalidate/reselect that branch, or restore the prior branch instead of leaving the node pinned to it.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| validate_pow(header, hash, network)?; | ||
| validate_empty_tree_root(tree, header, hash, network)?; | ||
| validate_candidate_nbits(tree, header, network)?; | ||
| validate_header_timestamp(tree, header, hash, now_secs)?; |
There was a problem hiding this comment.
When an otherwise valid header is more than two hours ahead of this node's local clock, this rejects the entire headers batch even though Bitcoin's two-hour future-time check applies when validating the full block; contextual header acceptance enforces median-time-past but may retain a future-dated header. The sync path also treats this as a non-peer fault without clearing the pending header request, so clock skew or a legitimately future-dated header stalls header progress until timeout or until the clock catches up. Split the checks: enforce MTP here and enforce the future bound during block application, including for previously known headers.
Useful? React with 👍 / 👎.
| || script.is_p2tr() | ||
| || is_p2a(script) |
There was a problem hiding this comment.
When a version 1 or 2 transaction contains the pay-to-anchor template, this classifies the output as standard, but Bitcoin Core permits the TX_ANCHOR output type only on version 3 transactions. Since this module deliberately caps accepted versions at 2 until TRUC policy exists, every anchor transaction accepted here is outside the policy context that makes anchors standard; reject P2A for the currently supported versions rather than exposing a public standardness result that disagrees with Core.
Useful? React with 👍 / 👎.
| // Load every body before the first disconnect. A missing body halfway | ||
| // through must not strand the chain at the common ancestor. | ||
| let connect = load_branch_bodies(handles, &plan.connect, &mut staged_body)?; | ||
| let disconnect = load_branch_bodies(handles, &plan.disconnect, &mut staged_body)?; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier staging-path finding is this all-at-once load: replacement-branch bodies are not persisted until they connect, the production stager holds at most 128 blocks and 256 MiB, and this requires every plan.connect body to be present before even the first connect can retire one. A valid winning fork longer than 128 blocks, or one whose bodies exceed the byte cap, therefore returns MissingBody forever—the downloader cannot stage the missing suffix while the prefix occupies the bounded pool. Preflight availability without retaining every body, or stream/spool the branch through a bounded reorg pipeline.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| let parallel_started = Instant::now(); | ||
| let verdict = verify_prepared_units(core::slice::from_ref(&unit), &[flags]); | ||
| timings.parallel_seconds = parallel_started.elapsed().as_secs_f64(); |
There was a problem hiding this comment.
For every fully checked block, verify_prepared_units performs both the script fan-out and the subsequent serial, transaction-ordered scan of all results, so timing the entire call here assigns that scan to script_parallel_seconds. This histogram is consumed directly by the replay artifacts and benchmark decomposition, while ScriptStageTimings explicitly promises that it excludes the ordered scan; comparisons against earlier captures therefore attribute serial work to the parallel stage and can lead to incorrect tuning conclusions. Split execution from verdict scanning or return the fan-out duration separately.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
bitcoin-rs/crates/node/src/sync.rs
Lines 908 to 909 in 85a1b1f
Invalidate permanent failures in the forward suffix
Fresh evidence beyond the earlier invalid-branch finding is the new bounded-prefix flow: after switch_to_branch commits the available reorg prefix and returns MissingBody, subsequently downloaded suffix blocks are processed here rather than by the reorg path. If one of those blocks is consensus-invalid, this branch merely drops it for retry; is_permanent_invalid and invalidate_subtree are called exclusively from reorg.rs. The header tree therefore keeps publishing the invalid branch and repeatedly downloads the same blocker, so a fork whose invalid block lies beyond the initially available prefix pins synchronization. Apply the same permanent-error classification and subtree invalidation here.
AGENTS.md reference: AGENTS.md:L3-L4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(node_ids) => { | ||
| self.handles.assume_valid_gate.evaluate(&tree); |
There was a problem hiding this comment.
When a header batch inserts a valid prefix and then rejects a later header, accept_headers leaves that prefix in the tree but this gate is reevaluated only on Ok. If the prefix is the point where a competing branch overtakes a previously trusted chain yet does not contain the pinned assume-valid anchor, the gate remains stale at trusted = true; sync can then switch to that branch and skip script verification through the configured anchor height. Evaluate the gate after every call that may have mutated the tree, including both error arms, or make header-batch insertion atomic.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| | bitcoin_rs_consensus::ConsensusError::Kernel(_) | ||
| | bitcoin_rs_consensus::ConsensusError::Encoding(_) | ||
| ), | ||
| _ => false, |
There was a problem hiding this comment.
When a reorg connect returns ApplyError::UtxoCommit after commit_borrowed_block has begun walking multiple shards, other shards may already have committed—the same partial-mutation behavior for this UTXO operation is why disconnect failures are treated as fatal. This wildcard instead classifies the error as operational, so switch_branch_if_outweighed only warns, leaves admission and read servers active, and permits retries while applied_tip still names the parent of a partially installed block. Route UtxoCommit to the fatal shutdown lifecycle rather than retrying it or invalidating the header.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| | `block_decode` | Block consensus deserialization | | ||
| | `tx_decode` | Transaction consensus deserialization | | ||
| | `script_eval` | Portable script interpreter (`Interpreter::execute`) | | ||
| | `utxo_snapshot` | UTXO snapshot deserializer (`read_snapshot`) | |
There was a problem hiding this comment.
The utxo_snapshot harness was changed specifically to fuzz read_snapshot_strict_v4 and read_snapshot_strict_v4_observed, while this target inventory still says it exercises the legacy read_snapshot entry point. That distinction is material because the harness itself documents that the legacy reader accepts older versions, missing trailers, count mismatches, and trailing bytes that the production checkpoint readers reject; maintainers using this table to select or assess fuzz coverage are therefore told the opposite surface from the one actually executed. Update the inventory alongside the harness.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
1 participant
Six commits from a wide parallel wave. Everything here is gate-verified; everything that did not land is named below rather than implied.
What landed
feat(chain): header timestamp validation. Header acceptance checked proof of work and difficulty retargeting but never the timestamp, leaving two consensus rules unenforced. Both now run inaccept_headers:The clock is read once per batch and passed in, so validation is a pure function of its inputs. Five tests pin the boundaries: at the median (reject), median+1 (accept), exactly +7200s (accept), +7201s (reject), parentless (exempt).
is_peer_faultsplits the two deliberately. A median violation is decided by the chain the peer sent, so it is the peer's fault. Future drift is judged against our clock, and a wrong local clock would otherwise let us ban every honest peer and partition ourselves.feat(index)+fix(index): reorg rollback. The indexer could only move forward, so a reorg left stale rows and Electrum would serve transactions no longer in the chain.rollback_blockdeletes what a matching ingest wrote, reusing the shared row-construction helper so a second key format cannot drift.It sits on the
IndexerLiketrait, because the node holds a trait object. Its default returnsUnsupportedRollbackrather than succeeding — a successful no-op would let the node advance its tip believing the index is consistent, which is the exact failure the method exists to prevent.The follow-up commit fixes a real bug review caught in the first: rollback wrote deletions straight to the store while buffered rows survived in
pending_rows, sobegin_batch → ingest → rollback → end_batchresurrected the disconnected block. Four tests added, and the regression one is mutation-verified — removing the flush fails that test and only that test.feat(mempool): standardness, orphan pool, fee estimator. 64 mempool tests pass. The orphan pool releases a child only when its last missing parent arrives and bounds itself by count and weight, since count alone lets a few enormous transactions pin memory. The fee estimator returnsNonebelow its evidence threshold rather than fabricating a number.test(fuzz): five cargo-fuzz targets over the untrusted-input surfaces.docs(policies): DB migration and source compatibility.What did NOT land
Node-level reorg execution is still absent — the headline gap this branch was opened for.
crates/node/src/apply.rsremains forward-only, UTXO undo data is never persisted, andplan_reorgstill has no production caller.bin/bitcoin-rs/tests/gates/g10_reorg_deep.rsstays#[ignore]d and still documents this accurately.The index half of reorg is ready and tested; the node half is not written. Several partial artifacts were produced and reverted rather than merged, because each would have made the tree green while implying the gap was closed: a
StateError::Reorgvariant pointing at a module that does not exist, an unusedshutdownfield with nostophandler, an unused inventory predicate, and a CLI crate containing only aCargo.toml.Also not landed: P2P transaction relay, the metrics exporter, the CLI client, and the
stop/dumptxoutset/loadtxoutsetRPCs.Verification
cargo test --workspace --no-fail-fastcargo clippy -p bitcoin-rs --all-targets --features "$FULL_NODE_FEATURES" -- -D warningscargo clippy -p bitcoin-rs-node --all-targets -- -D warningscargo clippy -p bitcoin-rs-consensus --all-targets -- -D warningscargo test -p bitcoin-rs-consensus -- --include-ignoredcargo deny checkcargo fmt --all --checkcargo checkinsidefuzz/Depends on #13, which restores
mainto a green suite.