test: reorganize, restart, and refuse to write — the reorg paths that end a run - #579
Merged
Conversation
📝 WalkthroughWalkthroughThe change restores the organizer’s validated height after synchronization, adds fixture restart support, and tests reorganization state persistence across restart. ChangesReorganization restart persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReorgTest
participant chain_fixture
participant header_organizer
participant chain
ReorgTest->>chain_fixture: Build and connect competing branches
ReorgTest->>chain_fixture: Execute reorganization
chain_fixture->>header_organizer: Persist winning branch state
chain_fixture->>chain: Persist headers and UTXO state
ReorgTest->>chain_fixture: Restart from existing data directory
chain_fixture->>header_organizer: Restore validated height
chain_fixture->>chain: Reload persisted chain state
ReorgTest->>chain_fixture: Verify winning branch and active heights
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
fpelliccioni
force-pushed
the
test/reorg-11-restart
branch
from
August 4, 2026 20:05
24c9374 to
7bccb50
Compare
… end a run The header index is rebuilt at startup from the persisted by-height headers, so those decide which chain the node resumes. A switch that moved the chain in memory without rewriting them would look right for as long as the process lived, and come back up on the branch it had abandoned — with the UTXO set already rewound below it. #578 added the handling for that; nothing exercised it, in either direction. Adds chain_fixture::restart(), which drops the chain and brings it up again on the same directory: nothing in memory survives, so what comes back is whatever was written to disk. The switch that succeeds: a short cycle (trunk to 12, one block on A, three on B), switched, restarted. Heights 13-15 name B's blocks, the organizer's tip is B's head, the validated tip and the UTXO-built height both return at 15, and the UTXO set holds B's coinbases and not A's. The switch whose header write fails: reaching that needs the write to fail on demand. Corrupting a database to get there would test the corruption, not the handling, and filling LMDB's map would depend on page sizes and break on changes that have nothing to do with this. So execute_reorg takes the persister as a parameter — not a test flag: the write is what decides whether a switch can be lived with, and a caller that runs a reorg has to say where it goes. The coordinator passes block_chain::replace_headers_from; the test passes one that refuses. On refusal: the switch itself succeeded and it is describing that on disk which failed; the by-height table still answers with A's block, so nothing changed it on the way out (not that the write is atomic — that persister never reaches the database); the reorg is reported fatal; and after a restart the node is on A, whole, with the validated tip and the UTXO-built height both back at the fork, so it re-downloads rather than trust a UTXO state that no longer matches the chain it came back on. The heavier branch can then be announced again and becomes a candidate again. test/fatal_shutdown.cpp covers the hinge above that: notify_fatal hands the reason to the owner once and verbatim, and a node with no handler is still safe to report to. What is NOT covered is stated there — the coordinator acting on the flag, and the executor's own stop path, are wired by inspection only. Writing it turned up a defect. The organizer was not told how far blocks are validated at startup: nothing tells it until the first newly stored block, and deep-reorg parking measures rewind depth against that height — so a node that had just come up would treat any heavier branch as costing no rewind and follow it, however deep it forked. That is the same window in which finalization is not protecting either, since it needs headers older than the finalization delay. full_node::run_sync now reports the persisted validated height right after sync_tip(), the fixture does the same, and both tests assert it. And side branches do not survive a restart. The by-height table is the only persisted header store and it describes the active chain alone, so the rebuilt index has no entry for the abandoned branch at all. Its block bytes stay in the flat files, held by nothing, until a peer announces the branch again and its headers are re-downloaded. BCHN persists its whole block index and does remember. The node still converges either way; the test now says so out loud.
fpelliccioni
force-pushed
the
test/reorg-11-restart
branch
from
August 4, 2026 21:30
d2beaaa to
201e8a3
Compare
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
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.
Absorbs #580, which was merged into this branch (it was stacked on it). One commit, covering both.
The header index is rebuilt at startup from the persisted by-height headers, so those decide which chain the node resumes. A switch that moved the chain in memory without rewriting them would look right for as long as the process lived, and come back up on the branch it had abandoned — with the UTXO set already rewound below it. #578 added the handling for that; nothing exercised it, in either direction.
chain_fixture::restart()drops the chain and brings it up again on the same directory: nothing in memory survives, so what comes back is whatever was written to disk.The switch that succeeds
Trunk to 12, one block on A, three on B; switch, restart. Heights 13–15 name B's blocks, the organizer's tip is B's head, the validated tip and the UTXO-built height both return at 15, and the UTXO set holds B's coinbases and not A's.
The switch whose header write fails
Reaching that needs the write to fail on demand. Corrupting a database would test the corruption, not the handling; filling LMDB's map would depend on page sizes and break on unrelated changes.
So
execute_reorgtakes the persister as a parameter — not a test flag: the write is what decides whether a switch can be lived with, and a caller that runs a reorg has to say where it goes, the same shape as theabortcallback already there. The coordinator passesblock_chain::replace_headers_from; the test passes one that refuses.On refusal: the switch itself succeeded and it is describing that on disk which failed; the by-height table still answers with A's block, so nothing changed it on the way out (not that the write is atomic — that persister never reaches the database); the reorg is reported
fatal; after a restart the node is on A, whole, with the validated tip and the UTXO-built height both back at the fork, so it re-downloads rather than trust a UTXO state that no longer matches the chain it came back on. The heavier branch can then be announced again and becomes a candidate again.test/fatal_shutdown.cppcovers the hinge above that:notify_fatalhands the reason to the owner once and verbatim, and a node with no handler is still safe to report to. What is not covered is stated in the file: the coordinator acting on the flag, and the executor's own stop path, are wired by inspection only.What writing it turned up
A defect. The organizer was not told how far blocks are validated at startup. Nothing tells it until the first newly stored block, and deep-reorg parking measures rewind depth against that height — so a node that had just come up would treat any heavier branch as costing no rewind and follow it, however deep it forked. That is the same window in which finalization is not protecting either, since it needs headers older than the finalization delay.
full_node::run_syncnow reports the persisted validated height right aftersync_tip(), the fixture does the same, and both tests assert it.And side branches do not survive a restart. The by-height table is the only persisted header store and it describes the active chain alone, so the rebuilt index has no entry for the abandoned branch at all. Its block bytes stay in the flat files, held by nothing, until a peer announces the branch again. BCHN persists its whole block index and does remember. The node converges either way; the test now says so out loud.
Local:
kth_node_test775 assertions / 114 cases,kth_blockchain_test1990 / 178.