Skip to content

fix(blockassembly): validate batch txid lengths#919

Merged
gokutheengineer merged 3 commits into
bsv-blockchain:mainfrom
gokutheengineer:gokhan/assembly-validatetxidlengths
Jun 4, 2026
Merged

fix(blockassembly): validate batch txid lengths#919
gokutheengineer merged 3 commits into
bsv-blockchain:mainfrom
gokutheengineer:gokhan/assembly-validatetxidlengths

Conversation

@gokutheengineer

@gokutheengineer gokutheengineer commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Validate every AddTxBatch request txid is exactly 32 bytes before converting it to chainhash.Hash.
  • Return a clean indexed invalid-argument error for malformed direct gRPC batch requests instead of allowing a slice-to-array conversion panic.
  • Add regression coverage for short and long txids in row-oriented AddTxBatch.

Problem

BlockAssembly.AddTxBatch constructed subtreepkg.Node values using chainhash.Hash(req.Txid) without first checking that req.Txid was exactly 32 bytes. AddTx and the columnar client path already perform this validation, so direct gRPC callers using the row-oriented batch endpoint could send malformed txids and trigger a panic instead of receiving a normal request error.

Fix

AddTxBatch now validates len(req.Txid) == 32 inside the batch loop before constructing the node. The returned error includes the failing request index and observed length, for example:

invalid txid length at index 1: 3

Test Plan

  • go test ./services/blockassembly -run 'TestAddTxBatch'

The regression test verifies malformed short and long txids do not panic, return an error, and include both invalid txid length and the failing index in the error message.

@gokutheengineer
fix(blockassembly): validate batch txid lengths
cbc68e0 
Validate AddTxBatch txids before constructing hashes so malformed direct gRPC requests return clean indexed errors instead of panicking.
@gokutheengineer gokutheengineer marked this pull request as draft May 21, 2026 12:51
@github-actions

github-actions Bot commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-919 (086d628)

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.553µ 1.570µ ~ 0.700
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 71.08n 71.22n ~ 0.500
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 71.10n 71.18n ~ 0.700
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 71.12n 71.16n ~ 0.800
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 31.75n 32.42n ~ 0.300
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 53.45n 52.86n ~ 0.400
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 125.2n 126.4n ~ 0.100
MiningCandidate_Stringify_Short-4 216.1n 216.7n ~ 0.400
MiningCandidate_Stringify_Long-4 1.597µ 1.606µ ~ 0.100
MiningSolution_Stringify-4 844.7n 846.7n ~ 0.100
BlockInfo_MarshalJSON-4 1.708µ 1.716µ ~ 0.200
NewFromBytes-4 92.11n 94.04n ~ 0.100
AddTxBatchColumnar_Validation-4 1.858µ 1.978µ ~ 0.100
OffsetValidationLoop-4 556.0n 421.2n ~ 0.100
Mine_EasyDifficulty-4 60.23µ 60.69µ ~ 0.100
Mine_WithAddress-4 6.756µ 6.686µ ~ 0.400
DirectSubtreeAdd/4_per_subtree-4 59.01n 61.10n ~ 0.100
DirectSubtreeAdd/64_per_subtree-4 32.04n 31.81n ~ 0.200
DirectSubtreeAdd/256_per_subtree-4 30.56n 30.57n ~ 1.000
DirectSubtreeAdd/1024_per_subtree-4 29.20n 29.20n ~ 1.000
DirectSubtreeAdd/2048_per_subtree-4 28.78n 28.72n ~ 0.600
SubtreeProcessorAdd/4_per_subtree-4 293.0n 292.3n ~ 1.000
SubtreeProcessorAdd/64_per_subtree-4 282.6n 303.8n ~ 0.200
SubtreeProcessorAdd/256_per_subtree-4 289.8n 295.1n ~ 1.000
SubtreeProcessorAdd/1024_per_subtree-4 278.4n 284.3n ~ 0.200
SubtreeProcessorAdd/2048_per_subtree-4 277.6n 284.6n ~ 0.200
SubtreeProcessorRotate/4_per_subtree-4 284.4n 305.4n ~ 0.200
SubtreeProcessorRotate/64_per_subtree-4 283.3n 294.9n ~ 0.200
SubtreeProcessorRotate/256_per_subtree-4 283.5n 299.5n ~ 0.700
SubtreeProcessorRotate/1024_per_subtree-4 280.5n 295.8n ~ 0.100
SubtreeNodeAddOnly/4_per_subtree-4 54.58n 55.07n ~ 0.100
SubtreeNodeAddOnly/64_per_subtree-4 34.22n 34.38n ~ 0.100
SubtreeNodeAddOnly/256_per_subtree-4 33.31n 33.63n ~ 0.100
SubtreeNodeAddOnly/1024_per_subtree-4 32.73n 32.87n ~ 0.200
SubtreeCreationOnly/4_per_subtree-4 114.0n 114.8n ~ 0.400
SubtreeCreationOnly/64_per_subtree-4 403.5n 409.8n ~ 0.100
SubtreeCreationOnly/256_per_subtree-4 1.364µ 1.389µ ~ 0.200
SubtreeCreationOnly/1024_per_subtree-4 4.461µ 4.423µ ~ 0.400
SubtreeCreationOnly/2048_per_subtree-4 8.422µ 8.331µ ~ 0.700
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 275.1n 276.7n ~ 0.300
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 277.1n 276.6n ~ 1.000
ParallelGetAndSetIfNotExists/1k_nodes-4 2.195m 2.229m ~ 0.100
ParallelGetAndSetIfNotExists/10k_nodes-4 5.277m 5.555m ~ 0.100
ParallelGetAndSetIfNotExists/50k_nodes-4 7.422m 7.860m ~ 0.100
ParallelGetAndSetIfNotExists/100k_nodes-4 10.26m 10.80m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 1.950m 1.978m ~ 0.200
SequentialGetAndSetIfNotExists/10k_nodes-4 4.510m 5.054m ~ 0.100
SequentialGetAndSetIfNotExists/50k_nodes-4 12.37m 15.51m ~ 0.100
SequentialGetAndSetIfNotExists/100k_nodes-4 22.28m 28.19m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 2.248m 2.278m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 8.255m 8.338m ~ 0.700
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 13.35m 14.03m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 1.970m 2.003m ~ 0.400
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 7.533m 9.999m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 40.20m 54.94m ~ 0.100
BlockAssembler_AddTx-4 0.02598n 0.02633n ~ 1.000
AddNode-4 10.56 11.38 ~ 0.200
AddNodeWithMap-4 10.69 10.81 ~ 0.400
DiskTxMap_SetIfNotExists-4 3.639µ 3.713µ ~ 0.700
DiskTxMap_SetIfNotExists_Parallel-4 3.595µ 3.409µ ~ 0.100
DiskTxMap_ExistenceOnly-4 325.5n 317.6n ~ 0.100
Queue-4 185.3n 185.6n ~ 0.500
AtomicPointer-4 3.276n 3.236n ~ 0.700
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 788.8µ 795.6µ ~ 0.400
ReorgOptimizations/DedupFilterPipeline/New/10K-4 758.1µ 763.9µ ~ 0.200
ReorgOptimizations/AllMarkFalse/Old/10K-4 102.1µ 106.1µ ~ 0.100
ReorgOptimizations/AllMarkFalse/New/10K-4 64.11µ 64.83µ ~ 0.700
ReorgOptimizations/HashSlicePool/Old/10K-4 51.26µ 49.93µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/10K-4 10.92µ 10.91µ ~ 0.300
ReorgOptimizations/NodeFlags/Old/10K-4 4.987µ 4.203µ ~ 0.100
ReorgOptimizations/NodeFlags/New/10K-4 2.072µ 1.447µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 9.438m 9.228m ~ 0.400
ReorgOptimizations/DedupFilterPipeline/New/100K-4 9.866m 9.585m ~ 0.700
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.079m 1.078m ~ 0.700
ReorgOptimizations/AllMarkFalse/New/100K-4 705.3µ 706.9µ ~ 0.400
ReorgOptimizations/HashSlicePool/Old/100K-4 597.0µ 479.0µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 217.8µ 210.2µ ~ 0.100
ReorgOptimizations/NodeFlags/Old/100K-4 45.34µ 46.53µ ~ 0.700
ReorgOptimizations/NodeFlags/New/100K-4 16.82µ 15.99µ ~ 0.100
TxMapSetIfNotExists-4 49.36n 50.02n ~ 0.100
TxMapSetIfNotExistsDuplicate-4 41.16n 41.51n ~ 0.200
ChannelSendReceive-4 573.1n 603.9n ~ 0.100
CalcBlockWork-4 509.4n 519.9n ~ 1.000
CalculateWork-4 692.1n 689.6n ~ 0.700
BuildBlockLocatorString_Helpers/Size_10-4 1.333µ 1.349µ ~ 0.100
BuildBlockLocatorString_Helpers/Size_100-4 13.26µ 12.84µ ~ 0.100
BuildBlockLocatorString_Helpers/Size_1000-4 144.7µ 143.7µ ~ 1.000
CatchupWithHeaderCache-4 104.5m 104.7m ~ 0.700
_BufferPoolAllocation/16KB-4 3.986µ 3.761µ ~ 0.700
_BufferPoolAllocation/32KB-4 7.744µ 10.578µ ~ 0.100
_BufferPoolAllocation/64KB-4 20.96µ 18.45µ ~ 0.700
_BufferPoolAllocation/128KB-4 27.16µ 37.55µ ~ 0.100
_BufferPoolAllocation/512KB-4 111.0µ 121.6µ ~ 0.100
_BufferPoolConcurrent/32KB-4 18.59µ 22.43µ ~ 0.200
_BufferPoolConcurrent/64KB-4 29.77µ 29.77µ ~ 1.000
_BufferPoolConcurrent/512KB-4 152.4µ 143.3µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 626.0µ 684.4µ ~ 0.700
_SubtreeDeserializationWithBufferSizes/32KB-4 622.4µ 690.6µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/64KB-4 648.3µ 680.0µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/128KB-4 646.8µ 665.2µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 623.4µ 691.5µ ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/16KB-4 36.16m 36.12m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/32KB-4 36.33m 36.04m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/64KB-4 35.93m 36.13m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/128KB-4 35.68m 36.11m ~ 0.200
_SubtreeDataDeserializationWithBufferSizes/512KB-4 35.55m 35.97m ~ 0.200
_PooledVsNonPooled/Pooled-4 741.1n 739.9n ~ 0.100
_PooledVsNonPooled/NonPooled-4 7.572µ 7.400µ ~ 0.700
_MemoryFootprint/Current_512KB_32concurrent-4 6.650µ 6.652µ ~ 1.000
_MemoryFootprint/Proposed_32KB_32concurrent-4 9.783µ 9.581µ ~ 0.100
_MemoryFootprint/Alternative_64KB_32concurrent-4 9.742µ 9.386µ ~ 0.100
SubtreeSizes/10k_tx_4_per_subtree-4 1.350m 1.374m ~ 0.400
SubtreeSizes/10k_tx_16_per_subtree-4 319.4µ 331.9µ ~ 0.700
SubtreeSizes/10k_tx_64_per_subtree-4 80.81µ 78.09µ ~ 0.700
SubtreeSizes/10k_tx_256_per_subtree-4 19.60µ 19.95µ ~ 0.200
SubtreeSizes/10k_tx_512_per_subtree-4 9.875µ 9.751µ ~ 0.400
SubtreeSizes/10k_tx_1024_per_subtree-4 4.976µ 4.840µ ~ 0.100
SubtreeSizes/10k_tx_2k_per_subtree-4 2.433µ 2.380µ ~ 0.100
BlockSizeScaling/10k_tx_64_per_subtree-4 77.17µ 75.91µ ~ 0.200
BlockSizeScaling/10k_tx_256_per_subtree-4 19.68µ 19.23µ ~ 0.100
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.831µ 4.795µ ~ 0.400
BlockSizeScaling/50k_tx_64_per_subtree-4 397.9µ 398.2µ ~ 1.000
BlockSizeScaling/50k_tx_256_per_subtree-4 97.39µ 96.60µ ~ 0.700
BlockSizeScaling/50k_tx_1024_per_subtree-4 24.01µ 23.65µ ~ 0.100
SubtreeAllocations/small_subtrees_exists_check-4 159.1µ 157.7µ ~ 0.400
SubtreeAllocations/small_subtrees_data_fetch-4 167.6µ 166.2µ ~ 0.400
SubtreeAllocations/small_subtrees_full_validation-4 338.5µ 330.3µ ~ 0.100
SubtreeAllocations/medium_subtrees_exists_check-4 9.434µ 9.555µ ~ 0.400
SubtreeAllocations/medium_subtrees_data_fetch-4 9.926µ 9.775µ ~ 0.700
SubtreeAllocations/medium_subtrees_full_validation-4 20.13µ 19.39µ ~ 0.100
SubtreeAllocations/large_subtrees_exists_check-4 2.308µ 2.251µ ~ 0.100
SubtreeAllocations/large_subtrees_data_fetch-4 2.416µ 2.364µ ~ 0.100
SubtreeAllocations/large_subtrees_full_validation-4 5.037µ 4.850µ ~ 0.100
_prepareTxsPerLevel-4 404.3m 384.5m ~ 0.100
_prepareTxsPerLevelOrdered-4 4.142m 4.647m ~ 0.400
_prepareTxsPerLevel_Comparison/Original-4 384.9m 387.3m ~ 0.700
_prepareTxsPerLevel_Comparison/Optimized-4 4.049m 3.973m ~ 0.400
StoreBlock_Sequential/BelowCSVHeight-4 329.3µ 337.3µ ~ 0.200
StoreBlock_Sequential/AboveCSVHeight-4 330.6µ 338.0µ ~ 0.100
GetUtxoHashes-4 211.6n 208.6n ~ 1.000
GetUtxoHashes_ManyOutputs-4 35.16µ 35.11µ ~ 0.400
_NewMetaDataFromBytes-4 166.4n 165.2n ~ 0.200
_Bytes-4 306.8n 304.4n ~ 0.400
_MetaBytes-4 107.2n 106.9n ~ 1.000

Threshold: >10% with p < 0.05 | Generated: 2026-05-22 13:21 UTC

@gokutheengineer gokutheengineer marked this pull request as ready for review May 22, 2026 12:33
@github-actions

github-actions Bot commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Current Review:

Found one consistency issue:

  • Error type inconsistency (services/blockassembly/Server.go:994): The sibling methods AddTx (line 884) and AddTxColumnar (line 944) both use NewProcessingError for invalid txid length, while this PR uses NewInvalidArgumentError. While InvalidArgument is semantically correct for client input errors, the inconsistency may confuse callers. Consider using NewProcessingError to match existing methods, or update all three in a separate PR for consistency.

Positives:

  • Fix correctly prevents panic from slice-to-array conversion
  • Validation placement is optimal (before constructing the node)
  • Test coverage is thorough (both short and long txid cases)
  • Error message includes helpful context (index and observed length)
  • Follows minimal diff principle from AGENTS.md

@gokutheengineer gokutheengineer requested review from freemans13 and icellan and removed request for icellan May 22, 2026 12:36
@sonarqubecloud

sonarqubecloud Bot commented May 22, 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

@gokutheengineer gokutheengineer requested a review from icellan May 22, 2026 13:26
@gokutheengineer gokutheengineer merged commit 05a2955 into bsv-blockchain:main Jun 4, 2026
25 checks passed
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

@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

@gokutheengineer @icellan @freemans13