blockchain: publish the chain state, the tip it describes and their label as one thing - #615
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds immutable atomic chain views containing chain state, tip hash, and generation. Startup, UTXO synchronization, and reorganization paths publish views at connected heights. Validation and mining APIs consume one coherent snapshot, with integration tests covering updates and failure paths. ChangesChain view publication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant block_tasks
participant reorg
participant block_chain
participant chain_view_readers
block_tasks->>block_chain: Publish view at applied batch tip
block_chain-->>chain_view_readers: Expose immutable chain view
reorg->>block_chain: Publish view at validated fork height
block_chain-->>chain_view_readers: Expose updated generation and tip
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/blockchain/src/interface/block_chain.cpp (1)
1167-1174: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the branch-state path against a null published view.
chain_state()now returnsnullptrbefore the first publication.chain_state(branch)passes that value topopulate_chain_state::populate(pool, branch), which dereferences it (pool->height(),pool->is_lobachevski_enabled()). The previous member was non-null for the whole run, so this dereference had no null path before. Return early instead.🛡️ Proposed guard
domain::chain::chain_state::ptr block_chain::chain_state(branch::const_ptr branch) const { - return chain_state_populator_.populate(chain_state(), branch); + auto const pool = chain_state(); + if ( ! pool) { + spdlog::error("[blockchain] No chain view has been published; cannot seed a branch state"); + return nullptr; + } + return chain_state_populator_.populate(pool, branch); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blockchain/src/interface/block_chain.cpp` around lines 1167 - 1174, Update block_chain::chain_state(branch::const_ptr branch) to return nullptr immediately when the parameterless chain_state() returns a null published view, before passing it to chain_state_populator_.populate; preserve the existing populate behavior for non-null state.
🧹 Nitpick comments (2)
src/node/test/chain_view.cpp (1)
153-168: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueYield inside the reader spin loop.
The reader thread spins with no pause for the whole connect sequence. On a machine with few cores it competes with the connect work that the test drives. Add a yield so the loop stays a sampler.
♻️ Proposed change
std::thread reader([&] { while ( ! stop) { auto const view = built.chain().chain_view(); - if ( ! view) continue; + if ( ! view) { + std::this_thread::yield(); + continue; + } ++reads; @@ if (view->tip_hash != expected) ++mismatches; + std::this_thread::yield(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/node/test/chain_view.cpp` around lines 153 - 168, Add a thread-yield operation inside the reader loop in the reader lambda, ensuring each iteration pauses appropriately while continuing to sample chain_view() until stop is set. Keep the existing view validation and mismatch counting unchanged.src/blockchain/src/populate/populate_chain_state.cpp (1)
266-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the alias and name the height in the failure log.
toponly renames the parameter. The message still says "last header", but no last-height lookup happens here. Log the requested height instead, because a failed publication is reported to the caller as fatal.♻️ Proposed change
chain_state::ptr populate_chain_state::populate(size_t connected_top) const { - auto const top = connected_top; - auto const header_result = chain_.get_header_and_abla_state(top); + auto const header_result = chain_.get_header_and_abla_state(connected_top); if ( ! header_result) { - spdlog::error("[blockchain] Failed to populate chain state, last header."); + spdlog::error("[blockchain] Failed to populate chain state: no header at height {}", connected_top); return {}; }The remaining uses of
topin the function body then readconnected_top.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blockchain/src/populate/populate_chain_state.cpp` around lines 266 - 272, In populate_chain_state::populate, remove the redundant top alias and use connected_top for the header lookup and remaining body references. Update the failure spdlog::error message to include the requested connected_top height instead of referring to an unspecified “last header.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/blockchain/src/interface/block_chain.cpp`:
- Around line 1167-1174: Update block_chain::chain_state(branch::const_ptr
branch) to return nullptr immediately when the parameterless chain_state()
returns a null published view, before passing it to
chain_state_populator_.populate; preserve the existing populate behavior for
non-null state.
---
Nitpick comments:
In `@src/blockchain/src/populate/populate_chain_state.cpp`:
- Around line 266-272: In populate_chain_state::populate, remove the redundant
top alias and use connected_top for the header lookup and remaining body
references. Update the failure spdlog::error message to include the requested
connected_top height instead of referring to an unspecified “last header.”
In `@src/node/test/chain_view.cpp`:
- Around line 153-168: Add a thread-yield operation inside the reader loop in
the reader lambda, ensuring each iteration pauses appropriately while continuing
to sample chain_view() until stop is set. Keep the existing view validation and
mismatch counting unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43001c42-2c8f-470b-91af-2851e302c8fe
📒 Files selected for processing (11)
src/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/include/kth/blockchain/populate/populate_chain_state.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/src/populate/populate_chain_state.cppsrc/blockchain/src/validate/validate_transaction.cppsrc/node/CMakeLists.txtsrc/node/src/sync/block_tasks.cppsrc/node/src/sync/reorg.cppsrc/node/test/chain_view.cppsrc/node/test/mempool_admission.cppsrc/node/test/reorg_cycle.cpp
fpelliccioni
force-pushed
the
fix/published-chain-view
branch
from
August 6, 2026 16:18
d65bbd1 to
fd290bf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #615 +/- ##
===========================================
+ Coverage 0.39% 80.43% +80.04%
===========================================
Files 278 287 +9
Lines 14335 14453 +118
===========================================
+ Hits 56 11625 +11569
+ Misses 14279 2828 -11451 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…abel as one thing `chain_state()` was computed once, in `start()`, and never advanced. Its only mutator, `set_chain_state`, had no callers. So a running node validated every transaction against the height it had when the process began — along with the median time past and the fork-activation flags in force then. Immediately after starting on a synced directory that is about right; a thousand blocks later it is a thousand blocks wrong, and nothing says so. Admission was one of four readers, and the only one anyone had proposed to fix. The other three are mining, and two of them already combined the frozen value with a fresh read: - `fetch_mining_info` reported the current height beside the difficulty in force at startup; - `fetch_mining_template` derived the template's parent from the frozen height — `get_block_hash(height - 1)` — so on a node that had connected anything since starting, that was not the tip. The template cache guards against exactly this, refusing to serve a snapshot from a previous tip, and it decides by comparing that hash: a guard whose input never changes is a guard that keeps passing. The state, the tip and a number labelling the pair now travel together, in one immutable object behind one pointer, swapped once. A reader holds the previous triple or the next one; there is no third possibility, and the two mixing readers have nothing left to mix — `get_block_hash` and `fetch_last_height` are gone from those paths, not by discipline but because there is nothing to call. `publish_chain_view` takes one thing: the height of the last block whose state is coherently applied. State and hash are derived from it here and the generation advances internally, so a caller cannot assemble a combination that never existed. It fails rather than keeping the previous view: at the close of a batch or a reorganization, being unable to describe the state just reached is not something to carry on from — the old view is still coherent and says nothing about being stale, so the return value is the only signal there is. The height is given and not read, because the two tips differ. `populate()` built from `heights.header`, and during a sync that runs thousands of blocks ahead of what is connected: a state carrying a height, a median time past and activation flags for a block whose UTXO delta has not been applied. Internally consistent, describing nothing. The overload that could do that is gone. Published at three points, and nowhere else. At startup from `heights.block`. At the close of a batch, once the delta is applied *in full* — including the deletions the store deferred, which are part of that delta and not work that follows it — the undo records are written, the height marker has moved and the mempool no longer holds what those blocks confirmed. Those deletions used to run after the publication and to be logged when they failed; a spent output left in the set is a double spend the node would accept, so they now run first and a failure is fatal. What makes that window recoverable rather than merely fatal is the marker of #600, which #602 adds; this orders the work and refuses to continue past a failure, and does not make the window crash-safe. And at the close of a reorganization, while the writers are still parked — with the validated tip, not the branch head: the switch rewinds the connected chain to the fork, and the blocks above it are headers until the ordinary path applies their deltas. So a reorganization moves the height down while the generation moves up, which is what counting published states rather than heights is for. The generation is taken last, after everything that can fail. Reading it while building the argument to the allocation would let a throw leave a number no published view carries — a reader comparing generations would see one it can never be given and conclude something was published that was not. The allocation is also caught and returned as a code, because callers publish at the close of a batch or a reorganization and decide what to do about a failure; an exception out of an API that promises a code would bypass that decision. Nine tests. The initial publication; headers arriving without connecting anything, which move neither the view nor the generation; a batch, and a second; the triple read whole while batches are published, checking that the hash a view carries is the hash of the block below the height it carries; mining info from one publication; a template naming the new tip after a batch rather than the cached one; a failed publication leaving both the view and the generation untouched, with several failures followed by a success that lands exactly one above where it started — so the counter is shown to have no hole in it; and the reorganization, where the height falls, the hash changes and the generation rises. The branch path — the one seeded from the published state, for block validation — answers null before anything is published rather than reading through nothing. That window is a chain constructed and not started, since start() is what publishes the first, and it is reachable through the public API; a test builds exactly that and asks. The admission tests from #607 no longer restart the chain. That restart was explicitly a way around this bug, and it is not something a running node could have done. What this does not fix: during a transition, template construction can still run against stores that are mid-mutation. This makes the published state coherent and whole; keeping readers out of the window while it is being reached is separate, and depends on settling ownership and readiness with #590. Closes #605
fpelliccioni
force-pushed
the
fix/published-chain-view
branch
from
August 7, 2026 13:22
fd290bf to
daa188b
Compare
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…ng whether a transition is running (#622) #615 made the published state coherent. It did not stop a template being built while a batch or a reorganization is between its first mutation and its publication — in that window the parent and the flags are coherent with each other, and the mempool and the UTXO set are being changed underneath the selection. A flag read on entry does not close that. The request reads it, the transition sets it and starts mutating, and the request captures during or after the mutation: the check and the capture are two moments and nothing joins them. What joins them is entry. A template is admitted as a capture reader or refused; inside, it takes one chain view and one coherent copy of the pool, and leaves; it builds outside, on what it captured. A transition closes entry first, waits for the captures already admitted to finish, and only then mutates. So a request that finished capturing completes on copies nothing can reach, and no new one can capture stores that are about to change. Reopening is earned. `end_transition` runs after the state has been published and nowhere else — a scope guard would reopen after a failure too, which is the one case where the gate must stay shut while the node winds down. A second `begin_transition` is refused rather than nested: sharing a flag would let the first to finish reopen for both. The lease covers the capture and not the build. Everything after it runs on private copies, so holding it across the graph, the ordering and the selection would make a transition wait on work that cannot be affected by it — the exclusion #611 measured, plus the whole template. Two conditions refuse a template, and they stay apart because they answer different questions. A transition is a correctness barrier and clears in milliseconds. Being unsynchronized is a service policy, and it is defined here rather than borrowed: `is_stale()` reads the stored block marker, which during a sync sits with the headers rather than with the UTXO set, so a node in the middle of downloading read as synchronized. `caught_up` compares the published view's connected tip against the active header tip; `fresh` measures that tip's age; a limit of zero disables the clock half and nothing else, because "do not judge by the clock" is not "you are at the head". `switch_result` now carries whether anything was disconnected. Several rejections happen before the first disconnect and still report the current tip, so comparing heights cannot tell them from a switch that rewound to where the chain already was — and publishing on those would move the generation and drop the template cache for a switch that did nothing. It is set after a disconnect applies, and forced on the unclean path, where the delta was applied even though the switch is abandoned. A rewind that failed part way still publishes where it ended up, so a node that resyncs from there can serve work again. Breaking surface change: `kth_mining_info_t` gains `transition_in_progress`, `caught_up` and `fresh`, mirroring the C++ struct the memcpy conversion asserts against, and `getmininginfo` gains the three fields. Three flags rather than one because they have different remedies: a transition clears on its own, a node that is behind is downloading, and a node that is caught up and stale has a clock or a connectivity problem. An operator reading one boolean could not tell them apart, and that is what this response is for. `error_code` gains `transition_in_progress`, `node_behind` and `node_stale`. Twenty-one tests. Seven on the gate itself, including the crossing this exists for — a capture parked while a transition tries to begin, which must not proceed — and its converse, that closing entry does not revoke a capture already admitted. Seven at node level: the refusal and its reason, service restored on reopening, mining info answering during a transition, the lease released before the build, the two shapes of a switch that touched nothing, and the two that decide whether `mutated` describes what happened or merely echoes `ok`. Those last two provoke a real failure rather than adding a seam to reach it. An undo record carries the hash of the block that owns it, and `read_undo` refuses one that disagrees — before the inverse delta is applied, which makes it a clean failure. Damaging the second record to be disconnected lets the block above it come off and stops there: `mutated` true, the view published where it stopped, the generation advanced, the gate reopened, and the node serving again while honestly reporting it is no longer caught up. Damaging the first stops before anything moves: `mutated` false, generation untouched. Five on synchronization, each asserting which half fails rather than only that the answer is false. Two on the rendered JSON. `fetch_mining_info` stays answerable throughout and is documented as a composite diagnostic: the height, difficulty and network come from one publication, while the pooled count and the three flags are read live. It is available exactly when something is wrong, which is when it is read, and calling it an atomic snapshot would be a claim it cannot keep. Closes #621
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chain_state()was computed once, instart(), and never advanced — its only mutator,set_chain_state, had no callers. A running node validated every transaction against the height it had when the process began, along with the median time past and the fork-activation flags in force then.Admission was one of four readers, and the only one anyone had proposed to fix. The other three are mining, and two already combined the frozen value with a fresh read:
fetch_mining_inforeported the current height beside the difficulty in force at startup;fetch_mining_templatederived the template's parent from the frozen height —get_block_hash(height - 1)— so on a node that had connected anything, that was not the tip. The cache guards against exactly this, refusing a snapshot from a previous tip, and it decides by comparing that hash. A guard whose input never changes is a guard that keeps passing.What changes
The state, the tip and a number labelling the pair travel together, in one immutable object behind one pointer, swapped once. A reader holds the previous triple or the next one. The two mixing readers have nothing left to mix:
get_block_hashandfetch_last_heightare gone from those paths — not by discipline, but because there is nothing to call.publish_chain_view(connected_tip_height)takes one thing: the last block whose state is coherently applied. State and hash are derived from it here, the generation advances internally, so a caller cannot assemble a combination that never existed. It returns acodeand fails rather than keeping the previous view — the old view is still coherent and says nothing about being stale, so the return value is the only signal there is.The height is given and not read, because the two tips differ.
populate()built fromheights.header, which during a sync runs thousands of blocks ahead of what is connected: a state carrying a height, an MTP and activation flags for a block whose UTXO delta has not been applied. Internally consistent, describing nothing. That overload is gone; so isset_chain_state; andchain_state()alone is now private.Where it publishes
heights.block, neverheights.headerThe switch rewinds the connected chain to the fork; blocks above are headers until the ordinary path applies their deltas. So a reorganization moves the height down while the generation moves up — which is what counting published states rather than heights is for. A counter derived from the height would have gone backwards, and a reader comparing it would conclude nothing had changed.
Tests
Nine. Initial publication; headers arriving without connecting anything, moving neither view nor generation; a batch, and a second; the triple read whole while batches are published, checking that the hash a view carries belongs to the block below the height it carries; mining info from one publication; a template naming the new tip rather than the cached one; a failed publication leaving view and generation untouched; and the reorganization — height down, hash changed, generation up.
The admission tests from #607 no longer restart the chain. That restart was explicitly a way around this bug, and not something a running node could do.
Blockchain 3751/208, node 2191/152.
What this does not fix
During a transition, template construction can still run against stores that are mid-mutation. This makes the published state coherent and whole; keeping readers out of the window while it is being reached is separate, and depends on settling ownership and readiness with #590.
Closes #605
Summary by CodeRabbit
New Features
Bug Fixes
Tests