Skip to content

revert: skip subtree_data fetch when local txs available (#598)#647

Merged
freemans13 merged 1 commit into
bsv-blockchain:mainfrom
freemans13:revert/pr-598-skip-subtree-data-fetch
Mar 30, 2026
Merged

revert: skip subtree_data fetch when local txs available (#598)#647
freemans13 merged 1 commit into
bsv-blockchain:mainfrom
freemans13:revert/pr-598-skip-subtree-data-fetch

Conversation

@freemans13

@freemans13 freemans13 commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Reverts fix: skip subtree_data fetch when local txs available #598 ("fix: skip subtree_data fetch when local txs available")
  • The localTxsAvailable heuristic compared block assembly's total pool count against the block's transaction count — a false positive that skipped downloading subtree data even when specific transactions (e.g. coinbase splitting txs) were missing locally
  • This caused frequent invalid block errors on dev-scale-2 when receiving blocks from dev-scale-1

Context

A replacement with an improved adaptive strategy is in #646. This revert cleans up the broken heuristic so the replacement PR has a clean base.

Test plan

  • go build ./... passes
  • TestCheckBlockSubtrees tests pass
  • Revert is a clean git revert of merge commit 88379d4

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Mar 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Review Summary:

This PR is a clean revert of #598, which introduced a flawed localTxsAvailable optimization. The revert is appropriate given the false positive issue described where the heuristic compared total pool count against block transaction count, causing invalid block errors when specific transactions (like coinbase splits) were missing locally.

Findings:

✅ No issues found - This is a clean revert with correct implementation:

  • Properly removes the block assembly client integration from subtree validation
  • Restores original subtree_data fetch behavior (fetch from peer when not in store)
  • Updates all test instantiations to match the new signature
  • Removes the flawed test that validated the broken optimization
  • Code is consistent and follows project conventions

Notes:

  • The PR description correctly identifies the root cause (false positive from pool count comparison)
  • A replacement with improved adaptive strategy is planned in feat: adaptive subtree fetch strategy for CheckBlockSubtrees #646
  • All changes are consistent with a git revert of the merge commit
  • Line 262 comment "PHASE 2: Exact pre-allocation" remains from the original code (not introduced by this PR)

This revert is safe to merge.

@sonarqubecloud

sonarqubecloud Bot commented Mar 30, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@github-actions

github-actions Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-647 (63048bf)

Summary

  • Regressions: 0
  • Improvements: 0
  • Unchanged: 151
  • Significance level: p < 0.05
All benchmark results (sec/op)
Benchmark Baseline Current Change p-value
_NewBlockFromBytes-4 1.387µ 1.392µ ~ 0.100
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 61.41n 61.85n ~ 0.200
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 61.59n 61.51n ~ 0.100
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 61.66n 61.67n ~ 1.000
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 29.50n 29.92n ~ 0.500
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 50.97n 52.64n ~ 0.700
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 105.6n 104.2n ~ 0.100
MiningCandidate_Stringify_Short-4 254.1n 256.6n ~ 0.100
MiningCandidate_Stringify_Long-4 1.830µ 1.861µ ~ 0.100
MiningSolution_Stringify-4 921.8n 929.7n ~ 0.700
BlockInfo_MarshalJSON-4 1.747µ 1.760µ ~ 0.700
NewFromBytes-4 131.5n 128.1n ~ 0.600
Mine_EasyDifficulty-4 63.59µ 62.84µ ~ 0.700
Mine_WithAddress-4 4.869µ 5.270µ ~ 0.100
BlockAssembler_AddTx-4 0.03537n 0.03069n ~ 0.700
AddNode-4 12.93 12.71 ~ 1.000
AddNodeWithMap-4 12.19 13.66 ~ 0.400
DirectSubtreeAdd/4_per_subtree-4 59.95n 60.96n ~ 1.000
DirectSubtreeAdd/64_per_subtree-4 31.74n 31.64n ~ 1.000
DirectSubtreeAdd/256_per_subtree-4 30.86n 30.70n ~ 0.800
DirectSubtreeAdd/1024_per_subtree-4 29.35n 29.47n ~ 0.100
DirectSubtreeAdd/2048_per_subtree-4 28.93n 29.12n ~ 0.100
SubtreeProcessorAdd/4_per_subtree-4 298.2n 302.3n ~ 0.200
SubtreeProcessorAdd/64_per_subtree-4 298.5n 297.5n ~ 0.200
SubtreeProcessorAdd/256_per_subtree-4 299.7n 296.9n ~ 0.400
SubtreeProcessorAdd/1024_per_subtree-4 297.6n 299.1n ~ 1.000
SubtreeProcessorAdd/2048_per_subtree-4 298.2n 297.2n ~ 0.400
SubtreeProcessorRotate/4_per_subtree-4 307.0n 305.4n ~ 0.400
SubtreeProcessorRotate/64_per_subtree-4 304.0n 303.7n ~ 0.800
SubtreeProcessorRotate/256_per_subtree-4 306.0n 302.2n ~ 0.100
SubtreeProcessorRotate/1024_per_subtree-4 302.0n 302.7n ~ 0.700
SubtreeNodeAddOnly/4_per_subtree-4 64.33n 63.63n ~ 0.700
SubtreeNodeAddOnly/64_per_subtree-4 39.19n 38.80n ~ 0.100
SubtreeNodeAddOnly/256_per_subtree-4 38.21n 37.27n ~ 0.200
SubtreeNodeAddOnly/1024_per_subtree-4 37.02n 37.06n ~ 1.000
SubtreeCreationOnly/4_per_subtree-4 143.1n 140.6n ~ 0.400
SubtreeCreationOnly/64_per_subtree-4 626.3n 608.1n ~ 0.100
SubtreeCreationOnly/256_per_subtree-4 2.194µ 2.096µ ~ 0.100
SubtreeCreationOnly/1024_per_subtree-4 7.996µ 7.573µ ~ 0.100
SubtreeCreationOnly/2048_per_subtree-4 15.13µ 14.44µ ~ 0.100
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 307.4n 300.0n ~ 0.200
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 298.9n 302.2n ~ 0.200
ParallelGetAndSetIfNotExists/1k_nodes-4 951.8µ 947.9µ ~ 0.100
ParallelGetAndSetIfNotExists/10k_nodes-4 1.882m 1.899m ~ 0.700
ParallelGetAndSetIfNotExists/50k_nodes-4 8.281m 8.255m ~ 1.000
ParallelGetAndSetIfNotExists/100k_nodes-4 16.24m 16.10m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 784.6µ 772.8µ ~ 0.100
SequentialGetAndSetIfNotExists/10k_nodes-4 3.055m 3.019m ~ 0.100
SequentialGetAndSetIfNotExists/50k_nodes-4 11.02m 11.33m ~ 0.400
SequentialGetAndSetIfNotExists/100k_nodes-4 21.36m 21.26m ~ 0.700
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 992.9µ 975.6µ ~ 0.200
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 4.655m 4.685m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 18.86m 19.10m ~ 1.000
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 802.3µ 816.1µ ~ 0.400
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 6.098m 6.389m ~ 0.200
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 40.76m 41.32m ~ 0.100
DiskTxMap_SetIfNotExists-4 3.714µ 3.616µ ~ 0.200
DiskTxMap_SetIfNotExists_Parallel-4 3.514µ 3.624µ ~ 0.500
DiskTxMap_ExistenceOnly-4 573.1n 325.3n ~ 0.100
Queue-4 195.9n 196.0n ~ 1.000
AtomicPointer-4 4.788n 4.766n ~ 0.200
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 904.0µ 873.1µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/10K-4 866.3µ 876.3µ ~ 0.700
ReorgOptimizations/AllMarkFalse/Old/10K-4 116.6µ 118.9µ ~ 0.200
ReorgOptimizations/AllMarkFalse/New/10K-4 61.94µ 62.17µ ~ 1.000
ReorgOptimizations/HashSlicePool/Old/10K-4 67.28µ 76.48µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/10K-4 11.56µ 11.64µ ~ 1.000
ReorgOptimizations/NodeFlags/Old/10K-4 5.425µ 6.739µ ~ 0.100
ReorgOptimizations/NodeFlags/New/10K-4 1.885µ 2.856µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 10.93m 12.91m ~ 0.200
ReorgOptimizations/DedupFilterPipeline/New/100K-4 10.81m 12.13m ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.186m 1.266m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 686.4µ 686.5µ ~ 1.000
ReorgOptimizations/HashSlicePool/Old/100K-4 683.9µ 642.3µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 337.5µ 339.8µ ~ 0.400
ReorgOptimizations/NodeFlags/Old/100K-4 59.39µ 60.50µ ~ 0.100
ReorgOptimizations/NodeFlags/New/100K-4 20.27µ 20.79µ ~ 0.100
TxMapSetIfNotExists-4 52.42n 52.11n ~ 0.100
TxMapSetIfNotExistsDuplicate-4 38.30n 38.19n ~ 0.100
ChannelSendReceive-4 589.7n 594.8n ~ 0.200
CalcBlockWork-4 496.4n 501.1n ~ 0.100
CalculateWork-4 708.0n 690.7n ~ 1.000
BuildBlockLocatorString_Helpers/Size_10-4 1.302µ 1.422µ ~ 0.100
BuildBlockLocatorString_Helpers/Size_100-4 12.46µ 12.47µ ~ 0.700
BuildBlockLocatorString_Helpers/Size_1000-4 134.5µ 123.1µ ~ 0.700
CatchupWithHeaderCache-4 104.4m 104.2m ~ 0.100
_BufferPoolAllocation/16KB-4 3.553µ 3.290µ ~ 0.100
_BufferPoolAllocation/32KB-4 8.327µ 7.323µ ~ 0.700
_BufferPoolAllocation/64KB-4 16.91µ 18.16µ ~ 0.700
_BufferPoolAllocation/128KB-4 27.24µ 31.73µ ~ 0.100
_BufferPoolAllocation/512KB-4 114.7µ 115.4µ ~ 1.000
_BufferPoolConcurrent/32KB-4 19.07µ 19.37µ ~ 0.700
_BufferPoolConcurrent/64KB-4 30.34µ 31.29µ ~ 0.100
_BufferPoolConcurrent/512KB-4 151.6µ 152.8µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 685.6µ 694.9µ ~ 0.400
_SubtreeDeserializationWithBufferSizes/32KB-4 673.5µ 686.9µ ~ 0.200
_SubtreeDeserializationWithBufferSizes/64KB-4 676.3µ 693.0µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/128KB-4 669.8µ 693.9µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 662.9µ 712.8µ ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/16KB-4 35.98m 35.79m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/32KB-4 35.85m 35.87m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/64KB-4 36.08m 34.94m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/128KB-4 35.45m 35.35m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/512KB-4 35.64m 35.16m ~ 0.400
_PooledVsNonPooled/Pooled-4 738.5n 738.6n ~ 1.000
_PooledVsNonPooled/NonPooled-4 6.940µ 7.580µ ~ 0.100
_MemoryFootprint/Current_512KB_32concurrent-4 6.866µ 6.752µ ~ 0.100
_MemoryFootprint/Proposed_32KB_32concurrent-4 10.45µ 10.10µ ~ 0.100
_MemoryFootprint/Alternative_64KB_32concurrent-4 10.66µ 10.09µ ~ 0.100
_prepareTxsPerLevel-4 414.1m 411.8m ~ 0.400
_prepareTxsPerLevelOrdered-4 3.548m 3.526m ~ 1.000
_prepareTxsPerLevel_Comparison/Original-4 421.2m 418.0m ~ 0.700
_prepareTxsPerLevel_Comparison/Optimized-4 3.555m 3.564m ~ 1.000
SubtreeProcessor/100_tx_64_per_subtree-4 83.78m 82.91m ~ 0.200
SubtreeProcessor/500_tx_64_per_subtree-4 397.5m 404.2m ~ 0.100
SubtreeProcessor/500_tx_256_per_subtree-4 414.2m 441.2m ~ 0.700
SubtreeProcessor/1k_tx_64_per_subtree-4 794.4m 793.3m ~ 1.000
SubtreeProcessor/1k_tx_256_per_subtree-4 810.6m 816.0m ~ 0.200
StreamingProcessorPhases/FilterValidated/100_tx-4 2.828m 2.877m ~ 0.700
StreamingProcessorPhases/ClassifyProcess/100_tx-4 249.0µ 251.7µ ~ 1.000
StreamingProcessorPhases/FilterValidated/500_tx-4 13.80m 13.82m ~ 0.200
StreamingProcessorPhases/ClassifyProcess/500_tx-4 615.9µ 616.7µ ~ 0.700
StreamingProcessorPhases/FilterValidated/1k_tx-4 27.92m 27.93m ~ 1.000
StreamingProcessorPhases/ClassifyProcess/1k_tx-4 1.076m 1.076m ~ 1.000
SubtreeSizes/10k_tx_4_per_subtree-4 1.328m 1.345m ~ 0.700
SubtreeSizes/10k_tx_16_per_subtree-4 327.6µ 326.6µ ~ 1.000
SubtreeSizes/10k_tx_64_per_subtree-4 75.44µ 77.20µ ~ 0.100
SubtreeSizes/10k_tx_256_per_subtree-4 19.16µ 19.18µ ~ 0.700
SubtreeSizes/10k_tx_512_per_subtree-4 9.361µ 9.603µ ~ 0.100
SubtreeSizes/10k_tx_1024_per_subtree-4 4.609µ 4.791µ ~ 0.100
SubtreeSizes/10k_tx_2k_per_subtree-4 2.313µ 2.362µ ~ 0.100
BlockSizeScaling/10k_tx_64_per_subtree-4 74.56µ 75.74µ ~ 0.100
BlockSizeScaling/10k_tx_256_per_subtree-4 18.62µ 19.04µ ~ 0.100
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.617µ 4.747µ ~ 0.100
BlockSizeScaling/50k_tx_64_per_subtree-4 387.3µ 396.2µ ~ 0.700
BlockSizeScaling/50k_tx_256_per_subtree-4 92.73µ 94.72µ ~ 0.100
BlockSizeScaling/50k_tx_1024_per_subtree-4 23.02µ 23.65µ ~ 0.100
SubtreeAllocations/small_subtrees_exists_check-4 155.2µ 160.7µ ~ 0.100
SubtreeAllocations/small_subtrees_data_fetch-4 163.2µ 162.3µ ~ 0.100
SubtreeAllocations/small_subtrees_full_validation-4 321.0µ 327.3µ ~ 0.100
SubtreeAllocations/medium_subtrees_exists_check-4 9.134µ 9.575µ ~ 0.100
SubtreeAllocations/medium_subtrees_data_fetch-4 9.598µ 9.666µ ~ 0.800
SubtreeAllocations/medium_subtrees_full_validation-4 18.48µ 18.85µ ~ 0.100
SubtreeAllocations/large_subtrees_exists_check-4 2.197µ 2.256µ ~ 0.100
SubtreeAllocations/large_subtrees_data_fetch-4 2.294µ 2.320µ ~ 0.200
SubtreeAllocations/large_subtrees_full_validation-4 4.652µ 4.736µ ~ 0.100
GetUtxoHashes-4 258.2n 256.0n ~ 0.700
GetUtxoHashes_ManyOutputs-4 42.76µ 47.17µ ~ 0.100
_NewMetaDataFromBytes-4 237.2n 237.2n ~ 1.000
_Bytes-4 618.2n 620.0n ~ 1.000
_MetaBytes-4 563.3n 560.5n ~ 0.200

Threshold: >10% with p < 0.05 | Generated: 2026-03-30 17:28 UTC

@freemans13 freemans13 merged commit 88c13a3 into bsv-blockchain:main Mar 30, 2026
24 checks passed
@freemans13 freemans13 self-assigned this Mar 30, 2026
freemans13 added a commit to freemans13/teranode that referenced this pull request Mar 30, 2026
@freemans13 @claude
merge: update feat/smart-subtree-fetch to latest upstream/main
e5db1eb 
Re-add blockAssemblyClient to subtreevalidation Server after PR bsv-blockchain#647
revert removed it. Wire it up in daemon and fix test callers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
galt-tr pushed a commit to galt-tr/teranode that referenced this pull request Apr 1, 2026
@freemans13 @galt-tr
revert: skip subtree_data fetch when local txs available (bsv-blockch…
ae72855 
…ain#598) (bsv-blockchain#647)
freemans13 added a commit to freemans13/teranode that referenced this pull request Apr 17, 2026
@freemans13 @claude
test(subtreevalidation): regression guard for PR bsv-blockchain#598 F…
c72221a 
…SM cascade

Forces FSM into CATCHINGBLOCKS while a fresh ReceivedAt stamp exists and
asserts the liveness gate still skips subtreeData. This is the test that,
had it existed, would have prevented PR bsv-blockchain#598 from being reverted (PR bsv-blockchain#647).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 17, 2026
Closed
Merged
@freemans13 freemans13 mentioned this pull request Jun 4, 2026
Closed
6 tasks
freemans13 added a commit to freemans13/teranode that referenced this pull request Jun 5, 2026
@freemans13 @claude
fix(adaptivefetch): arm gate on first FSM RUNNING + address PR bsv-bl…
874232d 
…ockchain#745 review

Pin the adaptive-fetch gate pessimistic until the node first reaches FSM
RUNNING, so cold-start IBD / restart-while-behind never skips subtreeData at
the moment txs are least likely to be local. New() now always starts
pessimistic; a one-way, idempotent Arm() latch (tripped by each service on the
first RUNNING) applies the configured bootstrap mode and clears the pre-arm
window so fake-perfect IBD samples can't cause an instant flip. The latch never
re-locks, so a post-RUNNING catch-up burst may still use optimistic mode.

pkg/adaptivefetch stays FSM-free: Arm is a generic trigger and the FSM wait
(WaitForFSMtoTransitionToGivenState RUNNING) lives in the service layer, so
TestNoWallClockOrFSMDependency still holds and the bsv-blockchain#598/bsv-blockchain#647 clock/FSM-gating
class-of-bug is not reintroduced into the algorithm.

Also addresses the review:
- Add TestCheckBlockSubtrees_Optimistic_SkipsFetchSubtreeData covering the
  previously-untested optimistic skip branch in subtreevalidation (runs both
  modes so it cannot pass trivially).
- Extract (*State).RecordSyntheticWarmup, collapsing the two duplicated
  ~34-line observation-recording blocks (SonarQube duplication).
- Log the previously-swallowed fallback adaptivefetch.New error in both
  blockvalidation and subtreevalidation Server constructors.
- Honest docs/TODO for the synthetic MissingFetches=0 one-way limitation;
  fix "four collectors" -> two comment drift; update settings longdesc to
  describe the arm-on-RUNNING behaviour.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icellan icellan icellan approved these changes

Assignees

@freemans13 freemans13

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@freemans13 @icellan