Skip to content

fix: replace unsafe currentSubtree.Load().Length() with channel-synce…#641

Merged
sugh01 merged 4 commits into
bsv-blockchain:mainfrom
sugh01:fix-subtree-sync
Jun 8, 2026
Merged

fix: replace unsafe currentSubtree.Load().Length() with channel-synce…#641
sugh01 merged 4 commits into
bsv-blockchain:mainfrom
sugh01:fix-subtree-sync

Conversation

@sugh01

@sugh01 sugh01 commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

…d GetCurrentLength() in subtree processor tests

@sugh01
fix: replace unsafe currentSubtree.Load().Length() with channel-synce…
9642d20 
…d GetCurrentLength() in subtree processor tests
@github-actions

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

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Current Review:

This PR improves test reliability by replacing racy atomic.Load() polling with the channel-synchronized GetCurrentLength() method. The changes are well-reasoned and the synchronization claim in comments is accurate.

Key improvements:

  • Replaces spin-wait loops polling txCount.Load() or currentSubtree.Load().Length() with require.Eventually() using GetCurrentLength()
  • Eliminates race conditions where test assertions could read partially-committed state
  • The GetCurrentLength() method properly synchronizes through the processing goroutine's select loop (services/blockassembly/subtreeprocessor/SubtreeProcessor.go:837-839), establishing a happens-before edge

Minor observations:

  • TestMoveForwardBlock:657 expects length 2, but the comment and variable n suggest processing all n transactions. Verified this is correct: n=5 total, first is coinbase placeholder (doesn't increment length), next 4 are batched but 2 get chained, leaving 2 in current subtree.
  • All timeout values (5 seconds) are consistent and reasonable for CI environments

No issues found. The changes correctly address the race conditions in the original code.

@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
0.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 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-641 (95faa55)

Summary

  • Regressions: 0
  • Improvements: 0
  • Unchanged: 144
  • Significance level: p < 0.05
All benchmark results (sec/op)
Benchmark Baseline Current Change p-value
_NewBlockFromBytes-4 1.585µ 1.588µ ~ 1.000
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 71.26n 71.06n ~ 0.400
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 70.99n 71.55n ~ 0.200
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 71.03n 71.06n ~ 0.700
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 31.60n 32.38n ~ 0.100
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 53.46n 54.80n ~ 0.200
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 125.6n 134.7n ~ 0.100
MiningCandidate_Stringify_Short-4 217.1n 216.9n ~ 1.000
MiningCandidate_Stringify_Long-4 1.593µ 1.608µ ~ 0.100
MiningSolution_Stringify-4 837.0n 840.9n ~ 0.200
BlockInfo_MarshalJSON-4 1.714µ 1.712µ ~ 1.000
NewFromBytes-4 130.0n 128.1n ~ 0.100
AddTxBatchColumnar_Validation-4 2.536µ 2.519µ ~ 0.200
OffsetValidationLoop-4 641.5n 635.0n ~ 0.400
Mine_EasyDifficulty-4 60.53µ 60.50µ ~ 1.000
Mine_WithAddress-4 7.052µ 6.808µ ~ 0.400
DiskTxMap_SetIfNotExists-4 3.316µ 3.368µ ~ 0.400
DiskTxMap_SetIfNotExists_Parallel-4 3.178µ 3.195µ ~ 1.000
DiskTxMap_ExistenceOnly-4 295.5n 296.4n ~ 0.700
Queue-4 186.0n 187.8n ~ 0.100
AtomicPointer-4 4.743n 5.040n ~ 0.400
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 829.9µ 838.5µ ~ 0.400
ReorgOptimizations/DedupFilterPipeline/New/10K-4 787.8µ 785.9µ ~ 1.000
ReorgOptimizations/AllMarkFalse/Old/10K-4 101.4µ 102.3µ ~ 0.100
ReorgOptimizations/AllMarkFalse/New/10K-4 62.48µ 62.25µ ~ 0.700
ReorgOptimizations/HashSlicePool/Old/10K-4 54.49µ 52.89µ ~ 0.200
ReorgOptimizations/HashSlicePool/New/10K-4 12.17µ 11.23µ ~ 0.100
ReorgOptimizations/NodeFlags/Old/10K-4 4.671µ 4.793µ ~ 0.100
ReorgOptimizations/NodeFlags/New/10K-4 1.602µ 1.635µ ~ 0.700
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 9.205m 9.218m ~ 1.000
ReorgOptimizations/DedupFilterPipeline/New/100K-4 9.582m 9.374m ~ 0.200
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.049m 1.081m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 682.5µ 681.9µ ~ 1.000
ReorgOptimizations/HashSlicePool/Old/100K-4 575.9µ 508.6µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 288.5µ 291.0µ ~ 0.700
ReorgOptimizations/NodeFlags/Old/100K-4 46.40µ 48.23µ ~ 0.100
ReorgOptimizations/NodeFlags/New/100K-4 13.44µ 16.77µ ~ 0.100
TxMapSetIfNotExists-4 52.27n 52.18n ~ 0.800
TxMapSetIfNotExistsDuplicate-4 40.34n 40.19n ~ 1.000
ChannelSendReceive-4 604.1n 610.5n ~ 0.200
BlockAssembler_AddTx-4 0.02682n 0.02927n ~ 1.000
AddNode-4 10.97 10.98 ~ 1.000
AddNodeWithMap-4 11.34 11.17 ~ 1.000
DirectSubtreeAdd/4_per_subtree-4 56.21n 59.07n ~ 0.100
DirectSubtreeAdd/64_per_subtree-4 29.09n 28.87n ~ 0.500
DirectSubtreeAdd/256_per_subtree-4 28.44n 27.67n ~ 0.100
DirectSubtreeAdd/1024_per_subtree-4 26.57n 26.46n ~ 0.200
DirectSubtreeAdd/2048_per_subtree-4 26.13n 25.99n ~ 0.200
SubtreeProcessorAdd/4_per_subtree-4 290.1n 291.4n ~ 1.000
SubtreeProcessorAdd/64_per_subtree-4 284.3n 284.4n ~ 1.000
SubtreeProcessorAdd/256_per_subtree-4 282.6n 283.6n ~ 0.200
SubtreeProcessorAdd/1024_per_subtree-4 272.8n 274.8n ~ 0.700
SubtreeProcessorAdd/2048_per_subtree-4 273.8n 275.6n ~ 0.100
SubtreeProcessorRotate/4_per_subtree-4 278.8n 279.9n ~ 0.200
SubtreeProcessorRotate/64_per_subtree-4 277.4n 276.9n ~ 0.600
SubtreeProcessorRotate/256_per_subtree-4 279.6n 277.7n ~ 0.100
SubtreeProcessorRotate/1024_per_subtree-4 279.2n 277.6n ~ 0.100
SubtreeNodeAddOnly/4_per_subtree-4 54.66n 55.00n ~ 0.100
SubtreeNodeAddOnly/64_per_subtree-4 35.81n 36.19n ~ 0.100
SubtreeNodeAddOnly/256_per_subtree-4 34.93n 35.31n ~ 0.100
SubtreeNodeAddOnly/1024_per_subtree-4 34.33n 34.66n ~ 0.100
SubtreeCreationOnly/4_per_subtree-4 110.1n 110.5n ~ 1.000
SubtreeCreationOnly/64_per_subtree-4 344.1n 342.7n ~ 0.600
SubtreeCreationOnly/256_per_subtree-4 1.214µ 1.223µ ~ 0.500
SubtreeCreationOnly/1024_per_subtree-4 3.799µ 3.790µ ~ 1.000
SubtreeCreationOnly/2048_per_subtree-4 6.724µ 6.731µ ~ 1.000
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 275.5n 276.3n ~ 0.700
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 273.9n 279.9n ~ 0.100
ParallelGetAndSetIfNotExists/1k_nodes-4 1.976m 1.983m ~ 0.400
ParallelGetAndSetIfNotExists/10k_nodes-4 5.046m 5.114m ~ 0.200
ParallelGetAndSetIfNotExists/50k_nodes-4 7.014m 7.086m ~ 0.700
ParallelGetAndSetIfNotExists/100k_nodes-4 9.560m 9.731m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 1.776m 1.782m ~ 0.400
SequentialGetAndSetIfNotExists/10k_nodes-4 4.436m 4.379m ~ 0.100
SequentialGetAndSetIfNotExists/50k_nodes-4 13.58m 13.35m ~ 0.100
SequentialGetAndSetIfNotExists/100k_nodes-4 25.03m 24.56m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 2.032m 2.028m ~ 1.000
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 8.315m 8.347m ~ 0.200
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 13.06m 12.95m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 1.779m 1.774m ~ 1.000
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 8.007m 7.869m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 43.02m 42.55m ~ 0.100
CalcBlockWork-4 510.2n 543.0n ~ 0.700
CalculateWork-4 695.3n 693.2n ~ 0.700
BuildBlockLocatorString_Helpers/Size_10-4 1.358µ 1.357µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_100-4 12.91µ 13.15µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_1000-4 128.8µ 133.9µ ~ 1.000
CatchupWithHeaderCache-4 104.2m 104.3m ~ 0.400
_BufferPoolAllocation/16KB-4 3.851µ 3.879µ ~ 1.000
_BufferPoolAllocation/32KB-4 7.517µ 10.925µ ~ 0.100
_BufferPoolAllocation/64KB-4 18.24µ 17.31µ ~ 0.700
_BufferPoolAllocation/128KB-4 28.86µ 27.66µ ~ 0.700
_BufferPoolAllocation/512KB-4 99.69µ 108.17µ ~ 0.200
_BufferPoolConcurrent/32KB-4 18.50µ 18.68µ ~ 0.700
_BufferPoolConcurrent/64KB-4 29.90µ 30.30µ ~ 1.000
_BufferPoolConcurrent/512KB-4 146.8µ 148.2µ ~ 0.400
_SubtreeDeserializationWithBufferSizes/16KB-4 622.8µ 621.6µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/32KB-4 633.6µ 617.5µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/64KB-4 619.0µ 614.5µ ~ 0.200
_SubtreeDeserializationWithBufferSizes/128KB-4 605.2µ 593.1µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 625.8µ 608.7µ ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/16KB-4 36.56m 36.40m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/32KB-4 36.41m 36.48m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/64KB-4 36.63m 36.28m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/128KB-4 36.45m 36.03m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/512KB-4 35.92m 36.06m ~ 1.000
_PooledVsNonPooled/Pooled-4 738.8n 742.4n ~ 0.100
_PooledVsNonPooled/NonPooled-4 7.449µ 7.786µ ~ 0.100
_MemoryFootprint/Current_512KB_32concurrent-4 6.651µ 6.708µ ~ 0.400
_MemoryFootprint/Proposed_32KB_32concurrent-4 9.229µ 10.773µ ~ 0.100
_MemoryFootprint/Alternative_64KB_32concurrent-4 9.290µ 9.933µ ~ 0.100
SubtreeSizes/10k_tx_4_per_subtree-4 1.345m 1.366m ~ 1.000
SubtreeSizes/10k_tx_16_per_subtree-4 317.4µ 322.2µ ~ 0.400
SubtreeSizes/10k_tx_64_per_subtree-4 75.49µ 75.79µ ~ 0.700
SubtreeSizes/10k_tx_256_per_subtree-4 18.92µ 18.82µ ~ 1.000
SubtreeSizes/10k_tx_512_per_subtree-4 9.370µ 9.345µ ~ 1.000
SubtreeSizes/10k_tx_1024_per_subtree-4 4.623µ 4.725µ ~ 0.200
SubtreeSizes/10k_tx_2k_per_subtree-4 2.317µ 2.321µ ~ 0.700
BlockSizeScaling/10k_tx_64_per_subtree-4 74.80µ 74.55µ ~ 0.400
BlockSizeScaling/10k_tx_256_per_subtree-4 18.65µ 18.79µ ~ 0.400
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.673µ 4.616µ ~ 0.300
BlockSizeScaling/50k_tx_64_per_subtree-4 391.6µ 386.9µ ~ 0.400
BlockSizeScaling/50k_tx_256_per_subtree-4 93.96µ 93.22µ ~ 0.100
BlockSizeScaling/50k_tx_1024_per_subtree-4 22.92µ 22.88µ ~ 1.000
SubtreeAllocations/small_subtrees_exists_check-4 159.1µ 161.5µ ~ 0.100
SubtreeAllocations/small_subtrees_data_fetch-4 160.1µ 161.4µ ~ 1.000
SubtreeAllocations/small_subtrees_full_validation-4 322.9µ 323.9µ ~ 0.200
SubtreeAllocations/medium_subtrees_exists_check-4 9.570µ 9.500µ ~ 0.700
SubtreeAllocations/medium_subtrees_data_fetch-4 9.366µ 9.333µ ~ 0.700
SubtreeAllocations/medium_subtrees_full_validation-4 18.76µ 18.71µ ~ 0.700
SubtreeAllocations/large_subtrees_exists_check-4 2.289µ 2.270µ ~ 1.000
SubtreeAllocations/large_subtrees_data_fetch-4 2.259µ 2.297µ ~ 0.200
SubtreeAllocations/large_subtrees_full_validation-4 4.699µ 4.710µ ~ 0.400
_prepareTxsPerLevel-4 408.1m 405.3m ~ 1.000
_prepareTxsPerLevelOrdered-4 3.580m 3.614m ~ 1.000
_prepareTxsPerLevel_Comparison/Original-4 406.7m 398.6m ~ 0.100
_prepareTxsPerLevel_Comparison/Optimized-4 3.598m 3.928m ~ 0.100
StoreBlock_Sequential/BelowCSVHeight-4 321.0µ 314.4µ ~ 0.700
StoreBlock_Sequential/AboveCSVHeight-4 314.7µ 314.3µ ~ 0.700
GetUtxoHashes-4 270.3n 276.1n ~ 0.700
GetUtxoHashes_ManyOutputs-4 45.44µ 45.36µ ~ 0.700
_NewMetaDataFromBytes-4 213.1n 215.5n ~ 0.100
_Bytes-4 395.3n 397.0n ~ 0.700
_MetaBytes-4 138.1n 138.5n ~ 0.800

Threshold: >10% with p < 0.05 | Generated: 2026-05-23 07:31 UTC

@oskarszoon oskarszoon mentioned this pull request May 14, 2026
Closed
oskarszoon
oskarszoon approved these changes May 14, 2026
View reviewed changes

@oskarszoon oskarszoon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. This is the clean standalone race fix — currentSubtree.Load().Length() is genuinely unsafe (atomic-pointer load + non-atomic method call on the loaded value), GetCurrentLength() provides the channel-sync happens-before edge.

One coordination note: this overlaps exactly with the SubtreeProcessor_test.go changes in #639, so exactly one should land. Asked #639 author to drop the duplicate. If you'd rather close this one and let #639 carry the fix instead, fine either way — but this branch is the cleaner option (no settings-test cargo).

@sonarqubecloud

sonarqubecloud Bot commented May 23, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

@sugh01 sugh01 requested a review from liam May 26, 2026 13:06
@sugh01 sugh01 merged commit b904347 into bsv-blockchain:main Jun 8, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liam liam liam approved these changes

@oskarszoon oskarszoon oskarszoon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sugh01 @liam @oskarszoon