Skip to content

fix(validator): set Valid=false when ValidateTransactionBatch has per-item errors#769

Open
ordishs wants to merge 2 commits intobsv-blockchain:mainfrom
ordishs:security/4567-validate-batch-valid-flag
Open

fix(validator): set Valid=false when ValidateTransactionBatch has per-item errors#769
ordishs wants to merge 2 commits intobsv-blockchain:mainfrom
ordishs:security/4567-validate-batch-valid-flag

Conversation

@ordishs
Copy link
Copy Markdown
Collaborator

@ordishs ordishs commented Apr 28, 2026

Closes #4567.

Summary

The batch gRPC response hard-codes Valid: true regardless of per-item errors, so callers that branch on the top-level flag treat a fully-failed batch as success. The audit (#4567) classified this as HIGH because any client-side optimization that short-circuits on Valid==true silently accepts invalid transactions.

Fix

Compute valid from errReasons after g.Wait(): false if any per-item error is non-nil, true otherwise. Set Valid: valid in the response.

Test plan

  • Regression test feeds one valid + one invalid tx; asserts response.Valid == false AND per-item errors are correctly populated.
  • Happy-path subtest: two valid txs => Valid == true, no errors.
  • All-invalid subtest: two invalid txs => Valid == false.
  • go test -race ./services/validator/... passes.

@github-actions
Copy link
Copy Markdown
Contributor

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

🤖 Claude Code Review

Status: Complete

This PR correctly addresses a HIGH severity bug where ValidateTransactionBatch hard-coded Valid: true regardless of per-item errors. The fix computes the Valid flag by checking if any transaction in the batch failed, which now matches the semantic contract in the protobuf definition (line 85: "Overall batch validation status").

Code Quality: The implementation is correct, efficient (early exit on first error), and well-tested with three comprehensive regression test cases covering mixed, all-valid, and all-invalid scenarios.

Documentation: Godoc accurately updated to reflect the new behavior - the Valid flag now correctly represents aggregated per-item results rather than always returning success.

@github-actions
Copy link
Copy Markdown
Contributor

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

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-769 (9dd3a86)

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.705µ 1.699µ ~ 0.300
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 59.23n 59.24n ~ 1.000
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 60.43n 59.47n ~ 1.000
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 59.35n 59.29n ~ 1.000
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 36.91n 34.74n ~ 0.200
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 61.65n 63.59n ~ 0.700
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 165.7n 162.7n ~ 0.100
MiningCandidate_Stringify_Short-4 255.5n 253.6n ~ 0.700
MiningCandidate_Stringify_Long-4 1.759µ 1.744µ ~ 0.100
MiningSolution_Stringify-4 931.5n 917.0n ~ 0.100
BlockInfo_MarshalJSON-4 1.747µ 1.725µ ~ 0.100
NewFromBytes-4 152.6n 125.3n ~ 0.200
Mine_EasyDifficulty-4 63.42µ 63.69µ ~ 1.000
Mine_WithAddress-4 5.007µ 5.230µ ~ 0.100
DirectSubtreeAdd/4_per_subtree-4 58.77n 63.83n ~ 0.400
DirectSubtreeAdd/64_per_subtree-4 28.97n 29.05n ~ 1.000
DirectSubtreeAdd/256_per_subtree-4 27.61n 28.11n ~ 0.400
DirectSubtreeAdd/1024_per_subtree-4 26.44n 26.67n ~ 0.400
DirectSubtreeAdd/2048_per_subtree-4 26.07n 26.24n ~ 0.100
SubtreeProcessorAdd/4_per_subtree-4 318.2n 328.1n ~ 0.100
SubtreeProcessorAdd/64_per_subtree-4 317.6n 336.1n ~ 0.100
SubtreeProcessorAdd/256_per_subtree-4 313.9n 324.7n ~ 0.400
SubtreeProcessorAdd/1024_per_subtree-4 314.7n 318.6n ~ 0.100
SubtreeProcessorAdd/2048_per_subtree-4 313.3n 317.7n ~ 0.800
SubtreeProcessorRotate/4_per_subtree-4 321.3n 327.7n ~ 0.700
SubtreeProcessorRotate/64_per_subtree-4 320.5n 330.9n ~ 0.700
SubtreeProcessorRotate/256_per_subtree-4 320.3n 318.0n ~ 0.800
SubtreeProcessorRotate/1024_per_subtree-4 308.8n 314.7n ~ 0.700
SubtreeNodeAddOnly/4_per_subtree-4 67.69n 67.40n ~ 0.700
SubtreeNodeAddOnly/64_per_subtree-4 39.58n 39.50n ~ 0.400
SubtreeNodeAddOnly/256_per_subtree-4 38.28n 38.38n ~ 1.000
SubtreeNodeAddOnly/1024_per_subtree-4 37.24n 37.51n ~ 0.100
SubtreeCreationOnly/4_per_subtree-4 164.8n 163.5n ~ 0.200
SubtreeCreationOnly/64_per_subtree-4 689.0n 814.2n ~ 0.700
SubtreeCreationOnly/256_per_subtree-4 2.087µ 2.137µ ~ 1.000
SubtreeCreationOnly/1024_per_subtree-4 5.121µ 5.139µ ~ 1.000
SubtreeCreationOnly/2048_per_subtree-4 8.717µ 8.789µ ~ 1.000
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 307.8n 309.4n ~ 0.400
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 314.1n 314.5n ~ 0.700
ParallelGetAndSetIfNotExists/1k_nodes-4 1036.7µ 983.8µ ~ 0.100
ParallelGetAndSetIfNotExists/10k_nodes-4 2.083m 1.958m ~ 0.100
ParallelGetAndSetIfNotExists/50k_nodes-4 8.713m 8.800m ~ 1.000
ParallelGetAndSetIfNotExists/100k_nodes-4 17.48m 17.68m ~ 0.400
SequentialGetAndSetIfNotExists/1k_nodes-4 766.7µ 765.6µ ~ 1.000
SequentialGetAndSetIfNotExists/10k_nodes-4 2.992m 3.062m ~ 0.700
SequentialGetAndSetIfNotExists/50k_nodes-4 10.74m 11.42m ~ 0.100
SequentialGetAndSetIfNotExists/100k_nodes-4 20.59m 21.00m ~ 0.200
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 1.040m 1.018m ~ 0.400
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 4.906m 4.883m ~ 0.700
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 19.70m 19.53m ~ 1.000
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 854.8µ 847.5µ ~ 0.700
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 6.129m 6.156m ~ 0.700
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 41.74m 40.24m ~ 0.100
DiskTxMap_SetIfNotExists-4 4.037µ 3.843µ ~ 0.700
DiskTxMap_SetIfNotExists_Parallel-4 3.946µ 3.637µ ~ 0.200
DiskTxMap_ExistenceOnly-4 428.2n 335.1n ~ 0.700
Queue-4 197.1n 193.7n ~ 0.100
AtomicPointer-4 3.251n 3.246n ~ 1.000
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 894.3µ 854.8µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/10K-4 850.2µ 845.8µ ~ 0.700
ReorgOptimizations/AllMarkFalse/Old/10K-4 114.9µ 132.5µ ~ 0.100
ReorgOptimizations/AllMarkFalse/New/10K-4 64.81µ 63.91µ ~ 0.100
ReorgOptimizations/HashSlicePool/Old/10K-4 61.13µ 66.62µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/10K-4 11.30µ 10.97µ ~ 0.100
ReorgOptimizations/NodeFlags/Old/10K-4 5.183µ 5.785µ ~ 0.100
ReorgOptimizations/NodeFlags/New/10K-4 1.838µ 2.262µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 11.674m 9.475m ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/100K-4 10.55m 10.13m ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.171m 1.201m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 709.5µ 706.6µ ~ 0.200
ReorgOptimizations/HashSlicePool/Old/100K-4 632.6µ 667.0µ ~ 1.000
ReorgOptimizations/HashSlicePool/New/100K-4 198.7µ 200.9µ ~ 1.000
ReorgOptimizations/NodeFlags/Old/100K-4 55.43µ 55.57µ ~ 1.000
ReorgOptimizations/NodeFlags/New/100K-4 19.37µ 19.10µ ~ 1.000
TxMapSetIfNotExists-4 46.82n 46.56n ~ 1.000
TxMapSetIfNotExistsDuplicate-4 38.76n 38.69n ~ 1.000
ChannelSendReceive-4 578.8n 623.7n ~ 0.100
BlockAssembler_AddTx-4 0.03135n 0.02966n ~ 1.000
AddNode-4 11.78 11.99 ~ 0.400
AddNodeWithMap-4 12.10 12.11 ~ 0.700
CalcBlockWork-4 506.5n 501.0n ~ 0.400
CalculateWork-4 677.4n 670.6n ~ 0.100
BuildBlockLocatorString_Helpers/Size_10-4 1.315µ 1.548µ ~ 0.700
BuildBlockLocatorString_Helpers/Size_100-4 12.57µ 12.54µ ~ 0.400
BuildBlockLocatorString_Helpers/Size_1000-4 143.5µ 123.9µ ~ 0.200
CatchupWithHeaderCache-4 104.3m 104.3m ~ 0.700
_BufferPoolAllocation/16KB-4 3.477µ 3.467µ ~ 1.000
_BufferPoolAllocation/32KB-4 9.492µ 7.212µ ~ 0.100
_BufferPoolAllocation/64KB-4 16.32µ 17.49µ ~ 0.700
_BufferPoolAllocation/128KB-4 31.89µ 30.75µ ~ 0.400
_BufferPoolAllocation/512KB-4 114.3µ 114.4µ ~ 0.700
_BufferPoolConcurrent/32KB-4 17.86µ 19.31µ ~ 0.400
_BufferPoolConcurrent/64KB-4 27.65µ 30.98µ ~ 0.100
_BufferPoolConcurrent/512KB-4 141.1µ 146.2µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 619.4µ 622.1µ ~ 1.000
_SubtreeDeserializationWithBufferSizes/32KB-4 622.0µ 612.7µ ~ 0.700
_SubtreeDeserializationWithBufferSizes/64KB-4 629.7µ 609.8µ ~ 0.200
_SubtreeDeserializationWithBufferSizes/128KB-4 623.2µ 611.1µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 634.5µ 626.1µ ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/16KB-4 35.30m 35.17m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/32KB-4 34.97m 35.01m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/64KB-4 35.06m 35.00m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/128KB-4 35.14m 34.97m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/512KB-4 35.24m 34.76m ~ 0.400
_PooledVsNonPooled/Pooled-4 738.8n 740.5n ~ 0.100
_PooledVsNonPooled/NonPooled-4 6.560µ 7.466µ ~ 0.100
_MemoryFootprint/Current_512KB_32concurrent-4 6.658µ 7.056µ ~ 0.100
_MemoryFootprint/Proposed_32KB_32concurrent-4 9.250µ 11.383µ ~ 0.100
_MemoryFootprint/Alternative_64KB_32concurrent-4 8.775µ 10.027µ ~ 0.100
_prepareTxsPerLevel-4 419.1m 407.5m ~ 0.200
_prepareTxsPerLevelOrdered-4 3.757m 3.762m ~ 0.700
_prepareTxsPerLevel_Comparison/Original-4 431.5m 416.5m ~ 0.100
_prepareTxsPerLevel_Comparison/Optimized-4 3.516m 3.606m ~ 0.200
SubtreeSizes/10k_tx_4_per_subtree-4 1.377m 1.366m ~ 0.400
SubtreeSizes/10k_tx_16_per_subtree-4 323.3µ 319.7µ ~ 0.100
SubtreeSizes/10k_tx_64_per_subtree-4 77.77µ 76.57µ ~ 0.100
SubtreeSizes/10k_tx_256_per_subtree-4 19.50µ 19.09µ ~ 0.100
SubtreeSizes/10k_tx_512_per_subtree-4 9.585µ 9.527µ ~ 0.400
SubtreeSizes/10k_tx_1024_per_subtree-4 4.726µ 4.690µ ~ 0.700
SubtreeSizes/10k_tx_2k_per_subtree-4 2.387µ 2.355µ ~ 0.100
BlockSizeScaling/10k_tx_64_per_subtree-4 74.94µ 75.04µ ~ 1.000
BlockSizeScaling/10k_tx_256_per_subtree-4 18.86µ 18.99µ ~ 0.400
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.723µ 4.685µ ~ 0.500
BlockSizeScaling/50k_tx_64_per_subtree-4 392.8µ 395.5µ ~ 0.700
BlockSizeScaling/50k_tx_256_per_subtree-4 94.30µ 94.57µ ~ 0.400
BlockSizeScaling/50k_tx_1024_per_subtree-4 23.22µ 23.29µ ~ 0.700
SubtreeAllocations/small_subtrees_exists_check-4 159.5µ 160.3µ ~ 0.200
SubtreeAllocations/small_subtrees_data_fetch-4 165.4µ 164.8µ ~ 1.000
SubtreeAllocations/small_subtrees_full_validation-4 323.0µ 321.9µ ~ 0.400
SubtreeAllocations/medium_subtrees_exists_check-4 9.391µ 9.403µ ~ 1.000
SubtreeAllocations/medium_subtrees_data_fetch-4 9.820µ 9.754µ ~ 0.700
SubtreeAllocations/medium_subtrees_full_validation-4 19.02µ 19.07µ ~ 0.400
SubtreeAllocations/large_subtrees_exists_check-4 2.248µ 2.295µ ~ 0.100
SubtreeAllocations/large_subtrees_data_fetch-4 2.368µ 2.375µ ~ 1.000
SubtreeAllocations/large_subtrees_full_validation-4 4.746µ 4.820µ ~ 0.100
StoreBlock_Sequential/BelowCSVHeight-4 335.7µ 334.1µ ~ 0.700
StoreBlock_Sequential/AboveCSVHeight-4 336.4µ 333.6µ ~ 0.700
GetUtxoHashes-4 259.3n 258.9n ~ 0.700
GetUtxoHashes_ManyOutputs-4 48.47µ 43.49µ ~ 0.100
_NewMetaDataFromBytes-4 237.5n 237.4n ~ 1.000
_Bytes-4 614.9n 618.3n ~ 0.700
_MetaBytes-4 564.7n 562.3n ~ 1.000

Threshold: >10% with p < 0.05 | Generated: 2026-04-28 18:01 UTC

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 28, 2026

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ordishs