Repository navigation
fix(blockvalidation): release subtrees on the valid-block error paths in setTxMinedStatus - #2044
codenlighten wants to merge 4 commits into
Conversation
… in setTxMinedStatus setTxMinedStatus's valid-block (else) branch loads the block's subtrees via GetSubtrees but only releases them with ReleaseSubtreeNodes after a successful UpdateTxMinedStatus. The error returns below — ErrBlockInvalid -> markBlockAsInvalid, ErrBlockParentNotMined -> nil, the generic update error, and the GetBlockHeaderIDs failure — all skip that release. The block is owned by this call (fetched fresh, or taken from and deleted out of lastValidatedBlocks), and an mmap-backed subtree has no finalizer, so each failed attempt leaks one set of mmap mappings and temp files; the setTxMined retry loop runs up to 10 times and the mined-not-set sweep retries beyond that. Release on every exit with a defer, mirroring the unset-mined reload path in the same function. On success the explicit release still runs first and the defer finds only nil entries. Adds a regression test: a failed mined update on the valid path must still leave the block's subtrees released (nil SubtreeSlices entries). Fixes bsv-blockchain#2029.
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-26 19:01 UTC |
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
The defer lands too late to close the leak 2029 describes, and the premise is inverted: the fresh-fetch block is heap-only, the cached block is the one that can hold mmap.
Not in the diff, so here:
services/blockvalidation/BlockValidation.go:1625— this comment says the block "may still be reachable via lastValidatedBlocks". That contradicts the new "owned by this call" comment at :1569, and the new one is right (theDeleteat :1504). Remove the stale one in this PR.model/update-tx-mined.go:236— pre-existing: the "missing subtree" return skipsg.Wait(), so already-started goroutines keep readingNodes/ passing&Nodes[idx].HashtoSetMinedMultiafter the call returns. The new defer assumes they are joined. Addg.Wait()before returning.
| return errors.NewProcessingError("[setTxMined][%s] failed to get subtrees from block", block.Hash().String(), err) | ||
| } | ||
|
|
||
| // Release the subtrees on EVERY exit, not just the success path below. |
There was a problem hiding this comment.
"each failed attempt leaks one set of mmap mappings ... retries up to 10 times" is wrong for this path. GetSubtrees loads via getAndValidateSubtrees -> DeserializeFromReaderWithAllocator, which is heap-only, and retries 2-10 fetch a fresh block. At most one mmap set leaks, and only when the block came from the cache after the quick-validate local-failure fallback at catchup.go:2099 (with blockvalidation_subtreeMmapDir set). Fix the comment and the PR body, otherwise the next person reasoning about ownership here gets misled.
There was a problem hiding this comment.
Code comment is fixed. The PR body and the test doc comment at set_tx_mined_revert_test.go:264 still carry the retry claim, so leaving this open.
There was a problem hiding this comment.
Code side is done; only the test doc comment at :264 and the PR body still carry it.
…, join tx-mined goroutines before the missing-subtree return Addresses review of bsv-blockchain#2044 (issue bsv-blockchain#2029). setTxMinedStatus takes a block from lastValidatedBlocks (owned once Deleted from the cache) that can hold mmap-backed subtrees, but released them only after a successful UpdateTxMinedStatus. The GetBlockHeader and CheckBlockIsInCurrentChain error returns run before that release, so a cached block's one mmap mapping and its temp backing file leaked on those paths (for example during a blockchain service restart). Register a single ReleaseSubtreeNodes defer as soon as the block is owned; it covers those returns, the valid path's heap-only GetSubtrees, the unset path's reload, and success. The per-branch defers and the explicit success-path release the defer now subsumes are removed, along with the stale "may still be reachable via lastValidatedBlocks" comment (the block is owned once deleted from the cache). The unset path still releases the cached subtrees before reloadSubtreesForInvalidBlock swaps in a fresh set with ReplaceSubtreeSlices, since the single defer only sees the final slice. model.updateTxMinedStatus returned on a missing subtree (!unsetMined) from inside the subtree loop, before g.Wait(), so goroutines already started for earlier subtrees kept reading &subtree.Nodes[idx].Hash. With the caller now releasing (closing mmap-backed Nodes) on that error, that was a use-after-free; join the group before returning. The tests use mmap-backed subtrees and assert the backing files are gone, which a nil-entry check cannot see (the pinned release path nils entries without closing). They cover the cached-block GetBlockHeader-failure path that leaked, the generic mined-update error path, and the goroutine join.
|
Thanks @icellan — this was the right catch; the premise was inverted and the defer landed too late. Reworked in c3366c3, addressing every point: Defer placement (BlockValidation.go). Replaced the two per-branch defers with a single Inverted premise / stale comments. Corrected the comments and the PR body: the valid path's Pre-reload release (unset path) kept. The single defer only sees the final slice, and
Tests. Rewritten to use mmap-backed subtrees and assert the backing files are gone (
One honest limitation: a deterministic use-after-free reproduction for the Verified on a Go 1.27 host: |
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Round 2 at c3366c3. The single early defer, the stale-comment removal, g.Wait() and the mmap-dir tests are in, and both blockvalidation tests fail against the base BlockValidation.go. Blocking on the release-timing change below. The PR body still carries the retry-leak claim and describes TestSetTxMinedStatus_ValidPathReleasesSubtreesOnError with a nil-entry check, which no longer exists — please update it.
| // successful UpdateTxMinedStatus. The error returns (ErrBlockInvalid, | ||
| // ErrBlockParentNotMined, the generic update error, and the GetBlockHeaderIDs | ||
| // failure) skipped that release, so the one owned mmap mapping and its temp | ||
| // backing file leaked, and the setTxMined retry loop runs up to 10 times. Here a |
There was a problem hiding this comment.
"the setTxMined retry loop runs up to 10 times" repeats the claim from the last round. Retries fetch a fresh block whose GetSubtrees load is heap-only, so at most one cached mmap set can leak. Drop it here and in the PR body.
There was a problem hiding this comment.
Still at :264 in 22e631c ("the setTxMined retry loop runs up to 10 times"). Non-blocking, leaving open.
| // headerFailingClient fails GetBlockHeader on demand. On the cached-block path | ||
| // only GetBlockHeader is reached before the failure, so the embedded nil ClientI | ||
| // is never called. | ||
| type headerFailingClient struct { |
There was a problem hiding this comment.
Embedding a nil blockchain.ClientI goes against AGENTS.md ("Don't mock the blockchain client/store — use the sqlitememory store") and the package's own decorators (setMinedFaultClient, trackingBlockchainClient), which wrap a real sqlitememory client. If another client call is added before GetBlockHeader this panics on a nil interface instead of failing cleanly. fail is always true, so drop the flag or use it.
There was a problem hiding this comment.
Still embeds a nil ClientI in 22e631c, and fail is still always true. Non-blocking, leaving open.
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
The cleanup now covers the cached-block header failure and mined-update errors, but the added goroutine-join test does not protect the new synchronization guard. I would hold this until the test catches removal of the wait.
model/update-tx-mined_test.go:756— The new subtest passes under-racewith_ = g.Wait()removed frommodel/update-tx-mined.go:242. It checks only the missing-subtree error, andSetMinedMultiis optional. Block the first worker inSetMinedMulti, prove the update has not returned, then release the worker and check the error. This needs to fail when the join is removed.
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Request changes at c3366c3. I independently confirmed the two existing concerns below and found no additional defects. Linking the existing threads to keep the discussion together.
- Release subtrees before publishing mined_set. Removing the explicit success-path release retains the full subtree set throughout
SetBlockMinedSet. A slow RPC extends that residency, and publishing the flag lets child validation proceed before the parent's cleanup finishes. Keep the early defer for error paths and restore the explicit release before the RPC. Existing discussion. - Make the join test detect a missing wait. With only
_ = g.Wait()removed through a Go build overlay, the new subtest passes all five runs under-race. Its optional mock call and error assertion do not establish that the worker finished. BlockSetMinedMultiwith a channel, verify the update has not returned, then release the worker and check the error. That test must fail when the wait is removed. Existing discussion.
Verification used Go 1.27.3 on darwin/arm64, SETTINGS_CONTEXT=test, -race, and -tags testtxmetacache:
TestUpdateTxMinedStatus_MissingSubtreeandTestSetTxMinedStatus*passed with-count=2.- Both new mmap cleanup tests failed twice with the base
BlockValidation.gooverlaid, leaving backing files behind. They therefore detect the original leaks. - The new join subtest passed with
-count=5after removing the wait through an overlay. - CI's failing
TestShouldHandleReorgWithLongerChainpassed twice in isolation. I have not established that the CI failure is caused by this PR.
I did not rerun the full suite or lint locally. The checkout is unchanged.
…n test fail without the wait Review follow-ups on the setTxMinedStatus subtree release. The success path releases the block's subtrees again before SetBlockMinedSet. With only the deferred release, the full subtree set stayed resident for the duration of that RPC, and publishing mined_set let child validation proceed before this block's cleanup had finished. The defer stays for the error returns; on a slice already released it is a no-op, because a release nils every entry it frees. TestSetTxMinedStatus_ReleasesMmapSubtreesBeforeSettingMinedSet records the mmap directory at the moment SetBlockMinedSet is called and requires it empty, then releases once more to show the repeat is a clean no-op. Without the early release the directory still holds the backing file at that point and the test fails; the other tests here only look after the function returns, so they pass either way. The join subtest in TestUpdateTxMinedStatus_MissingSubtree now holds the worker inside SetMinedMulti, requires updateTxMinedStatus not to have returned while it is there, then releases it and checks the missing-subtree error. Removing the g.Wait before the missing-subtree return fails it, without -race. The not-returned check is a bounded 250 ms window, so it can miss a regression on a very slow scheduler but cannot fail a correct implementation.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Round 3 at 22e631c. Fix is correct: one defer from the point of ownership, success path still releases before SetBlockMinedSet, and the join test now blocks the worker so it can actually fail. Approving.
Before merge:
- Merge order with #2018 (approved). It changes
SetBlockMinedSetto(ctx, hash, minedSetGeneration); whichever lands second has to update the new calls andminedSetProbeClienthere, or the blockvalidation test package stops compiling. #2065 touches the sameErrBlockInvalidbranch, so expect a text conflict there too. - The PR body still names
TestSetTxMinedStatus_ValidPathReleasesSubtreesOnErrorand a per-branch defer, neither of which exists. Update it so the squash message matches the code. - Two round-2 threads are still open (retry claim at set_tx_mined_revert_test.go:264, nil
ClientIembed inheaderFailingClient). Not blocking.
| readErr error | ||
| } | ||
|
|
||
| func (c *minedSetProbeClient) SetBlockMinedSet(ctx context.Context, blockHash *chainhash.Hash) error { |
| // goroutine still in flight here reads &subtree.Nodes[idx].Hash and | ||
| // would race that release or read an unmapped region. g.Wait's own | ||
| // error is discarded in favour of the specific missing-subtree error. | ||
| _ = g.Wait() |
There was a problem hiding this comment.
#2042 fixes the same return-before-g.Wait class by rejecting a nil entry before any g.Go. Here the earlier subtrees still get SetMinedMulti with this blockID (gCtx is never cancelled) and then the call errors, leaving the block partially tagged. A nil pre-scan before the loop when !unsetMined returns the same error with no goroutines, no wasted writes and no mid-loop wait, and keeps one pattern for one bug.
| } else { | ||
| // All subtrees should already be available for fully processed blocks | ||
| // All subtrees should already be available for fully processed blocks. | ||
| // This load is heap-only (getAndValidateSubtrees -> |
There was a problem hiding this comment.
Not always heap-only: for a cached block whose subtrees are already loaded mmap-backed (the case your tests build), getAndValidateSubtrees takes the already-loaded early return and keeps the mmap subtrees. Reword so nobody later pools nodes or skips Close here on the strength of this comment.
…kchain#2029) The load in setTxMinedStatus is heap-only for a fresh fetch but keeps whatever a cached block already holds, possibly mmap-backed, so say that instead of calling it heap-only. The defer comment still described the success-path release as replaced; it was restored before SetBlockMinedSet. The revert test's doc comment no longer claims a retry-loop leak: the leak is one cached mmap set.
|
Thanks @icellan. Pushed 5662ead (comments only, no code or test change) and replaced the PR body.
|
Fixes #2029.
Problem
setTxMinedStatus(services/blockvalidation/BlockValidation.go) owns the block it works on: fetched fresh from the blockchain, or taken from and deleted out oflastValidatedBlocks. A cached block can carry mmap-backed subtrees (set when it reached the cache through the catchup quick-validate local-failure fallback withblockvalidation_subtreeMmapDirset), and an mmap-backed subtree has no finalizer. Before this change the subtrees were released only after a successfulmodel.UpdateTxMinedStatus, so every earlier error return left that one set of mappings and temp files behind until process exit:GetBlockHeaderandCheckBlockIsInCurrentChainfailures, which are reached before the valid path's load (the cached-block case),GetBlockHeaderIDsfailure,ErrBlockInvalid(markBlockAsInvalid),ErrBlockParentNotMined, and the genericerror updating tx mined status.A freshly fetched block loads its subtrees on the heap, so the leak is bounded to the cached mmap set, not one per retry.
Fix
One
ReleaseSubtreeNodesdefer, registered as soon as the block is owned (after the cacheDeleteor the fresh fetch).ReleaseSubtreeNodeson a nil or already-released slice is a no-op, so the defer is a safety net on every exit. The success path still releases explicitly beforeSetBlockMinedSet, so the subtrees are not held across that RPC. The unset-mined path still releases the cached subtrees beforereloadSubtreesForInvalidBlockswaps in a fresh set.model/update-tx-mined.go: the missing-subtree return now joins the goroutines already started (g.Wait()), so the caller's release cannot race a worker still readingNodes.Tests
TestSetTxMinedStatus_CachedBlockReleasesMmapSubtreesWhenHeaderFetchFails: a cached mmap block,GetBlockHeaderfails; the backing files must be gone.TestSetTxMinedStatus_ValidPathReleasesMmapSubtreesOnMinedUpdateError: the generic mined-update error path, same proof.TestSetTxMinedStatus_ReleasesMmapSubtreesBeforeSettingMinedSet: the mmap directory is empty at the momentSetBlockMinedSetis called.TestUpdateTxMinedStatus_MissingSubtree: holds a worker insideSetMinedMulti, requires the call not to have returned, then releases it; it fails when theg.Wait()is removed.The first two fail against the base
BlockValidation.go.Merge order
#2018 changes
SetBlockMinedSetto(ctx, hash, minedSetGeneration); whichever of #2018 and this PR lands second updates the new calls andminedSetProbeClient. #2065 touches the sameErrBlockInvalidbranch, so a text conflict is likely.