Skip to content

fix(validator): require BlockIDs+!Conflicting+!Locked before ErrTxNotFound shortcut#770

Merged
ordishs merged 3 commits into
bsv-blockchain:mainfrom
ordishs:security/4568-tx-not-found-shortcut
May 12, 2026
Merged

fix(validator): require BlockIDs+!Conflicting+!Locked before ErrTxNotFound shortcut#770
ordishs merged 3 commits into
bsv-blockchain:mainfrom
ordishs:security/4568-tx-not-found-shortcut

Conversation

@ordishs

@ordishs ordishs commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Closes #4568.

Summary

When spendUtxos returns ErrTxNotFound (parent missing), the validator looked up the current tx in the UTXO store and, if found, returned (meta, nil) with a warning "assuming already blessed" — without checking whether the metadata actually confirmed prior validation. During a reorg or DAH-eviction window, a tx lookup can succeed while its parent is temporarily absent; the shortcut would silently accept the tx on the basis of its own existence rather than a verified prior validation.

Fix

Gate the shortcut on three conditions that together imply prior full validation:

  1. len(BlockIDs) > 0 — tx has been included in at least one block.
  2. !Conflicting — tx is not flagged as conflicting.
  3. !Locked — tx is not locked (e.g., from a conflict aftermath).

If any condition fails, surface the original ErrTxNotFound. Use a local metaErr so the outer err (still ErrTxNotFound) survives the metadata lookup for the fall-through error path.

Also tightened MockUtxostore.GetMeta so Return(error) now correctly surfaces a lookup failure instead of panicking on a *meta.Data cast.

Test plan

  • Shortcut allowed when BlockIDs non-empty + !Conflicting + !Locked.
  • Shortcut denied when BlockIDs empty.
  • Shortcut denied when Conflicting.
  • Shortcut denied when Locked.
  • Shortcut denied when metadata lookup itself fails.
  • go test -race ./services/validator/... passes.
  • go test -race ./stores/utxo/... passes.

@github-actions

github-actions Bot commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Summary:
This PR addresses a security vulnerability where transactions could be incorrectly validated during DAH-eviction windows. The fix adds proper metadata checks (BlockIDs, Conflicting, Locked) before accepting a transaction as "blessed". Code quality is strong with comprehensive test coverage.

Current Review:

  • No critical issues found
  • Minor suggestion: Consider applying similar fix to store implementations

Previous Reviews:

  • Initial review completed 2026-04-28

@github-actions

github-actions Bot commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-770 (f764029)

Summary

  • Regressions: 0
  • Improvements: 0
  • Unchanged: 142
  • Significance level: p < 0.05
All benchmark results (sec/op)
Benchmark Baseline Current Change p-value
_NewBlockFromBytes-4 1.669µ 1.695µ ~ 0.200
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 61.73n 61.63n ~ 0.700
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 61.98n 61.56n ~ 0.400
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 61.60n 61.71n ~ 0.200
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 30.81n 30.72n ~ 1.000
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 52.95n 56.06n ~ 0.200
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 111.0n 109.2n ~ 0.100
MiningCandidate_Stringify_Short-4 264.2n 263.4n ~ 0.200
MiningCandidate_Stringify_Long-4 1.909µ 1.883µ ~ 0.200
MiningSolution_Stringify-4 980.8n 970.0n ~ 0.400
BlockInfo_MarshalJSON-4 1.759µ 1.759µ ~ 1.000
NewFromBytes-4 150.3n 127.7n ~ 0.200
Mine_EasyDifficulty-4 60.34µ 60.63µ ~ 0.400
Mine_WithAddress-4 7.544µ 6.804µ ~ 0.700
DiskTxMap_SetIfNotExists-4 3.689µ 3.480µ ~ 0.100
DiskTxMap_SetIfNotExists_Parallel-4 3.378µ 3.350µ ~ 0.700
DiskTxMap_ExistenceOnly-4 305.9n 296.7n ~ 0.200
Queue-4 198.6n 197.0n ~ 0.100
AtomicPointer-4 5.055n 5.195n ~ 0.700
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 874.0µ 853.9µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/10K-4 885.6µ 852.1µ ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/10K-4 124.3µ 110.4µ ~ 0.100
ReorgOptimizations/AllMarkFalse/New/10K-4 62.24µ 62.53µ ~ 0.200
ReorgOptimizations/HashSlicePool/Old/10K-4 67.40µ 67.88µ ~ 1.000
ReorgOptimizations/HashSlicePool/New/10K-4 11.93µ 11.88µ ~ 0.700
ReorgOptimizations/NodeFlags/Old/10K-4 5.434µ 6.029µ ~ 0.100
ReorgOptimizations/NodeFlags/New/10K-4 1.876µ 2.327µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 10.82m 10.17m ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/100K-4 10.26m 10.10m ~ 0.700
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.120m 1.205m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 689.5µ 687.0µ ~ 0.100
ReorgOptimizations/HashSlicePool/Old/100K-4 642.7µ 727.6µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 290.8µ 290.4µ ~ 1.000
ReorgOptimizations/NodeFlags/Old/100K-4 58.05µ 56.95µ ~ 0.700
ReorgOptimizations/NodeFlags/New/100K-4 20.21µ 20.05µ ~ 0.700
TxMapSetIfNotExists-4 51.55n 52.02n ~ 0.100
TxMapSetIfNotExistsDuplicate-4 37.94n 38.21n ~ 0.200
ChannelSendReceive-4 637.1n 616.3n ~ 0.700
BlockAssembler_AddTx-4 0.02908n 0.02762n ~ 0.700
AddNode-4 11.10 11.19 ~ 1.000
AddNodeWithMap-4 11.27 10.69 ~ 0.100
DirectSubtreeAdd/4_per_subtree-4 60.07n 59.12n ~ 0.800
DirectSubtreeAdd/64_per_subtree-4 28.99n 30.08n ~ 0.100
DirectSubtreeAdd/256_per_subtree-4 27.79n 27.70n ~ 0.700
DirectSubtreeAdd/1024_per_subtree-4 26.52n 26.59n ~ 0.300
DirectSubtreeAdd/2048_per_subtree-4 26.17n 26.15n ~ 0.700
SubtreeProcessorAdd/4_per_subtree-4 282.2n 280.5n ~ 0.400
SubtreeProcessorAdd/64_per_subtree-4 275.9n 276.0n ~ 1.000
SubtreeProcessorAdd/256_per_subtree-4 277.9n 275.7n ~ 0.700
SubtreeProcessorAdd/1024_per_subtree-4 268.7n 269.9n ~ 0.400
SubtreeProcessorAdd/2048_per_subtree-4 268.9n 271.0n ~ 0.200
SubtreeProcessorRotate/4_per_subtree-4 274.8n 270.7n ~ 0.100
SubtreeProcessorRotate/64_per_subtree-4 271.3n 269.0n ~ 0.200
SubtreeProcessorRotate/256_per_subtree-4 268.8n 268.6n ~ 1.000
SubtreeProcessorRotate/1024_per_subtree-4 271.3n 269.0n ~ 0.100
SubtreeNodeAddOnly/4_per_subtree-4 55.23n 55.23n ~ 1.000
SubtreeNodeAddOnly/64_per_subtree-4 36.11n 36.05n ~ 1.000
SubtreeNodeAddOnly/256_per_subtree-4 35.20n 35.13n ~ 0.300
SubtreeNodeAddOnly/1024_per_subtree-4 34.55n 34.70n ~ 0.100
SubtreeCreationOnly/4_per_subtree-4 109.0n 110.7n ~ 0.100
SubtreeCreationOnly/64_per_subtree-4 350.5n 347.9n ~ 0.400
SubtreeCreationOnly/256_per_subtree-4 1.204µ 1.208µ ~ 0.600
SubtreeCreationOnly/1024_per_subtree-4 3.804µ 3.825µ ~ 0.700
SubtreeCreationOnly/2048_per_subtree-4 6.743µ 7.025µ ~ 0.100
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 269.5n 267.4n ~ 0.200
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 272.4n 268.4n ~ 0.100
ParallelGetAndSetIfNotExists/1k_nodes-4 545.3µ 540.4µ ~ 0.400
ParallelGetAndSetIfNotExists/10k_nodes-4 1.326m 1.322m ~ 0.700
ParallelGetAndSetIfNotExists/50k_nodes-4 6.690m 6.509m ~ 0.100
ParallelGetAndSetIfNotExists/100k_nodes-4 13.33m 13.06m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 619.5µ 614.3µ ~ 0.100
SequentialGetAndSetIfNotExists/10k_nodes-4 2.906m 2.834m ~ 0.100
SequentialGetAndSetIfNotExists/50k_nodes-4 11.22m 11.07m ~ 0.100
SequentialGetAndSetIfNotExists/100k_nodes-4 21.96m 21.33m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 603.4µ 584.5µ ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 4.767m 4.529m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 17.23m 16.74m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 676.9µ 664.5µ ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 6.403m 6.263m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 40.72m 39.19m ~ 0.100
CalcBlockWork-4 511.9n 521.5n ~ 0.100
CalculateWork-4 750.6n 733.9n ~ 1.000
BuildBlockLocatorString_Helpers/Size_10-4 1.339µ 1.350µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_100-4 14.99µ 15.40µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_1000-4 124.2µ 124.0µ ~ 1.000
CatchupWithHeaderCache-4 104.6m 104.7m ~ 0.100
_BufferPoolAllocation/16KB-4 3.526µ 3.354µ ~ 0.700
_BufferPoolAllocation/32KB-4 7.572µ 7.418µ ~ 1.000
_BufferPoolAllocation/64KB-4 15.84µ 14.38µ ~ 0.100
_BufferPoolAllocation/128KB-4 31.63µ 29.24µ ~ 0.100
_BufferPoolAllocation/512KB-4 111.3µ 107.5µ ~ 0.100
_BufferPoolConcurrent/32KB-4 19.21µ 16.67µ ~ 0.100
_BufferPoolConcurrent/64KB-4 31.00µ 27.98µ ~ 0.100
_BufferPoolConcurrent/512KB-4 156.3µ 141.1µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 648.4µ 604.3µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/32KB-4 634.4µ 602.1µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/64KB-4 636.8µ 604.1µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/128KB-4 626.9µ 600.8µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 631.2µ 602.4µ ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/16KB-4 36.25m 35.81m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/32KB-4 36.67m 35.63m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/64KB-4 36.12m 35.67m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/128KB-4 35.61m 34.90m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/512KB-4 35.33m 35.26m ~ 1.000
_PooledVsNonPooled/Pooled-4 838.1n 833.7n ~ 0.600
_PooledVsNonPooled/NonPooled-4 7.481µ 7.023µ ~ 0.100
_MemoryFootprint/Current_512KB_32concurrent-4 7.064µ 7.327µ ~ 0.400
_MemoryFootprint/Proposed_32KB_32concurrent-4 10.78µ 10.95µ ~ 1.000
_MemoryFootprint/Alternative_64KB_32concurrent-4 10.17µ 10.00µ ~ 0.400
_prepareTxsPerLevel-4 394.0m 406.8m ~ 0.100
_prepareTxsPerLevelOrdered-4 3.559m 3.755m ~ 0.700
_prepareTxsPerLevel_Comparison/Original-4 410.9m 414.7m ~ 0.400
_prepareTxsPerLevel_Comparison/Optimized-4 3.720m 3.628m ~ 0.700
SubtreeSizes/10k_tx_4_per_subtree-4 1.398m 1.405m ~ 0.700
SubtreeSizes/10k_tx_16_per_subtree-4 323.2µ 326.7µ ~ 0.400
SubtreeSizes/10k_tx_64_per_subtree-4 79.14µ 80.78µ ~ 0.200
SubtreeSizes/10k_tx_256_per_subtree-4 19.80µ 20.22µ ~ 0.100
SubtreeSizes/10k_tx_512_per_subtree-4 9.775µ 10.018µ ~ 0.100
SubtreeSizes/10k_tx_1024_per_subtree-4 4.892µ 4.930µ ~ 0.400
SubtreeSizes/10k_tx_2k_per_subtree-4 2.470µ 2.456µ ~ 0.700
BlockSizeScaling/10k_tx_64_per_subtree-4 80.08µ 77.82µ ~ 0.400
BlockSizeScaling/10k_tx_256_per_subtree-4 19.59µ 19.94µ ~ 0.100
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.847µ 4.944µ ~ 0.200
BlockSizeScaling/50k_tx_64_per_subtree-4 390.8µ 392.7µ ~ 0.700
BlockSizeScaling/50k_tx_256_per_subtree-4 97.44µ 97.97µ ~ 0.700
BlockSizeScaling/50k_tx_1024_per_subtree-4 24.35µ 24.61µ ~ 0.700
SubtreeAllocations/small_subtrees_exists_check-4 160.9µ 160.2µ ~ 0.100
SubtreeAllocations/small_subtrees_data_fetch-4 166.2µ 168.7µ ~ 0.400
SubtreeAllocations/small_subtrees_full_validation-4 318.7µ 324.7µ ~ 0.400
SubtreeAllocations/medium_subtrees_exists_check-4 9.719µ 9.772µ ~ 1.000
SubtreeAllocations/medium_subtrees_data_fetch-4 10.37µ 10.59µ ~ 0.100
SubtreeAllocations/medium_subtrees_full_validation-4 19.99µ 20.15µ ~ 0.100
SubtreeAllocations/large_subtrees_exists_check-4 2.369µ 2.415µ ~ 0.200
SubtreeAllocations/large_subtrees_data_fetch-4 2.570µ 2.590µ ~ 0.200
SubtreeAllocations/large_subtrees_full_validation-4 5.066µ 5.039µ ~ 0.100
StoreBlock_Sequential/BelowCSVHeight-4 251.3µ 251.5µ ~ 1.000
StoreBlock_Sequential/AboveCSVHeight-4 251.4µ 252.4µ ~ 0.100
GetUtxoHashes-4 255.7n 259.6n ~ 0.400
GetUtxoHashes_ManyOutputs-4 45.69µ 46.43µ ~ 1.000
_NewMetaDataFromBytes-4 237.5n 242.0n ~ 0.400
_Bytes-4 619.0n 616.3n ~ 0.400
_MetaBytes-4 562.6n 558.3n ~ 0.100

Threshold: >10% with p < 0.05 | Generated: 2026-05-12 08:30 UTC

@oskarszoon oskarszoon mentioned this pull request May 4, 2026
Merged
github-actions[bot]
github-actions Bot reviewed May 12, 2026
View reviewed changes
Comment thread services/validator/Validator.go
v.logger.Warnf("[Validate][%s] parent tx DAH-evicted, child already mined and not conflicting/locked, assuming blessed (BlockIDs=%v)", txID, txMetaData.BlockIDs)

return txMetaData, nil
return txMetaData, nil

@github-actions github-actions Bot May 12, 2026

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.

[Minor] Consider similar fix for store implementations

This PR correctly addresses the validator-level shortcut, but similar patterns exist in:

  • stores/utxo/aerospike/spend.go - uses s.Get() without checking BlockIDs/Conflicting/Locked
  • stores/utxo/sql/sql.go - uses simple existence check without metadata validation

Those store-level shortcuts may have the same vulnerability: accepting a transaction as "blessed" based solely on its existence during a DAH-eviction or reorg window.

Recommendation: Consider creating a follow-up issue to audit and potentially apply similar metadata checks (BlockIDs, Conflicting, Locked) in the store implementations.

@sonarqubecloud

sonarqubecloud Bot commented May 12, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
1 New issue
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

@ordishs ordishs requested review from freemans13 and liam May 12, 2026 09:41
@ordishs ordishs enabled auto-merge (squash) May 12, 2026 09:42
@ordishs ordishs merged commit bf96b7c into bsv-blockchain:main May 12, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@liam liam liam approved these changes

@freemans13 freemans13 freemans13 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

@ordishs @liam @freemans13