✅ Resolved

[Critical] Missing bounds check before array access - This issue has been fixed.

The code now includes proper bounds checking at services/legacy/netsync/handle_block.go:1041-1049:

• Line 1041-1043: Checks if prevTxWrapper.Tx is nil
• Line 1045-1047: Validates PreviousTxOutIndex is in range
• Line 1049-1054: Checks for nil output and nil locking script

Malformed transactions will now return a processing error instead of panicking the node.

Out of scope / no longer applicable

The FSM query code mentioned in this comment doesn't appear in the current version of the PR. This issue appears to have been addressed through refactoring or is no longer relevant to the current implementation.

✅ Resolved - The PR now includes the disableCheckpoints field on SyncManager and gates the belowFinalCheckpoint check accordingly (services/legacy/netsync/handle_block.go:364-368).

Addressed: plumbed DisableCheckpoints from Config onto SyncManager.disableCheckpoints and in prepareSubtrees we now force belowFinalCheckpoint=false when checkpoints are disabled, so the CATCHINGBLOCKS path cannot skip subtree validation in that mode.

✅ Resolved - The data race is fixed. txWrapper.SomeParentsInBlock = true is now set once from the caller goroutine before spawning the per-input goroutines (services/legacy/netsync/handle_block.go:1020).

Good catch — moved the txWrapper.SomeParentsInBlock = true write out of the goroutine. It's now set once from the caller goroutine before g.Go(...), so concurrent writes from the per-input goroutines are gone. Verified via go test -race on the netsync package.

✅ Resolved - The doc comment has been updated to match the current implementation, which reads parent outputs immediately without waiting (services/legacy/netsync/handle_block.go:980-987).

Updated the doc comment to match the new behavior: we no longer wait/poll for parent extension — outputs are available at parse time, so we read them immediately.

The Phase 1 comment says per-tx goroutines "may need to wait for a same-block parent to finish being extended first", but the new implementation no longer waits/polls on parent extension (extendFromTxMap reads parent outputs immediately). Please update/remove the comment to match the current behavior so future changes don't reintroduce the old deadlock workaround.

Updated the Phase 1 comment to remove the stale "wait for a same-block parent" wording. Now reads: goroutines are bounded by OutpointBatcherSize and each tx's inputs are populated independently from txMap lookups — no waiting/polling.

The inline placeholder construction is described as "mirrors the pattern used in PreviousOutputsDecorate above", but PreviousOutputsDecorate currently uses buildINClause with a simple t.hash IN (...) predicate (not composite tuple placeholders). Consider either switching PreviousOutputsDecorate to the same composite-IN helper/pattern or rewording this comment to avoid implying the two implementations are structurally identical.

Reworded the comment as suggested. It no longer implies structural equivalence with PreviousOutputsDecorate — it just states that a composite (t.hash, o.idx) IN clause is built inline to keep the function standalone. I'll leave PreviousOutputsDecorate as-is in this PR (its single-column t.hash IN (...) form is fine for that call site's semantics).

New behavior: CheckBlockSubtrees now reads the subtree structure from FileTypeSubtree as well as FileTypeSubtreeToCheck. The existing unit tests only cover the FileTypeSubtreeToCheck path; please add a test case where only FileTypeSubtree exists (and FileTypeSubtreeToCheck is absent) to ensure we don’t regress back to the HTTP fetch path (especially for baseURL="legacy").

Added TestFindLocalSubtreeFile with three subtests: (1) only FileTypeSubtreeToCheck present, (2) only FileTypeSubtree present (the new legacy quickValidationMode path), and (3) neither present. The FileTypeSubtree-only case is the regression guard you asked for — it fails if we ever drop back to FileTypeSubtreeToCheck-only lookup which would reintroduce the HTTP fallback for baseURL="legacy".

findLocalSubtreeFile returns a raw empty string ("") for the FileType on error/not-found paths. Since FileTypeUnknown is defined as the empty value, returning fileformat.FileTypeUnknown would make the intent clearer and keep return values consistently typed.

Good suggestion — switched all three not-found / error paths in findLocalSubtreeFile to return fileformat.FileTypeUnknown explicitly instead of the raw empty string. Keeps intent clear and return values consistently typed.

The doc comment says that when checkpoints are disabled, full subtree validation is required (no checkpoint anchor), but shouldUseQuickValidationMode currently returns true whenever inLegacySync is true regardless of belowFinalCheckpoint (which the caller forces to false when checkpoints are disabled). Please align the comment and logic: either (a) update the comment to clarify that the checkpoints-disabled restriction only applies to the CATCHINGBLOCKS branch, or (b) incorporate the checkpoints-disabled/belowFinalCheckpoint guard into the LEGACYSYNCING branch as well.

Expanded the doc comment: (a) called out that the caller forces belowFinalCheckpoint=false when checkpoints are disabled, and (b) added a paragraph explaining why LEGACYSYNCING doesn't re-check it here — the DisableCheckpoints guard in manager.go (around line 2302) means nextCheckpoint stays nil and headers-first mode never activates, so the checkpoints-disabled guard is applied upstream. Keeping the LEGACYSYNCING branch unconditional matches bsvd's model; the comment now makes that reasoning explicit instead of implicit.

extendFromTxMap indexes prevTxWrapper.Tx.Outputs[input.PreviousTxOutIndex] without validating that PreviousTxOutIndex is in-range. A malformed/hostile block could trigger an out-of-bounds panic here (DoS) before the block is rejected. Please add a bounds check (and a nil output/script check if applicable) and return a validation/processing error instead of panicking.

Good defensive hardening — added the checks inside the goroutine: (1) prevTxWrapper.Tx nil, (2) PreviousTxOutIndex out of range vs len(prevTxWrapper.Tx.Outputs), (3) prevOutput nil or LockingScript nil. Each returns a NewProcessingError so the block is rejected cleanly instead of panicking. Tests still green under -race.

The comment says maxINClauseSize bounds the total parameter count, but the constant is defined as the maximum number of placeholders in a single IN clause (chosen to stay under SQLite's 999 bind-param limit with headroom). Since this code now uses two bind params per tuple, reword the comment to reflect the actual constraint (e.g., maxINClauseSize is an item-count limit derived from the bind-param cap, so we use maxINClauseSize/2 tuples).

Applied the suggested wording. The comment now states that maxINClauseSize is the per-IN-clause placeholder limit derived from the bind-parameter cap, and that each composite (hash, idx) pair uses two params so each chunk can hold at most maxINClauseSize/2 tuples.

In extendTransactions phase 1, the goroutine closure captures the loop-local variable tx without re-binding it per iteration. Because tx is assigned each loop iteration, goroutines can end up all operating on the last transaction (classic loop-variable capture bug). Rebind tx inside the loop (e.g., tx := tx) or pass it as a parameter to the closure before calling extendFromTxMap.

Thanks, but I believe this is actually safe. Two reasons:

1. tx is not a loop variable — it's declared inside the loop body via tx := txWrapper.Tx (line 953). Each iteration creates a fresh variable, so the closure on line 956 captures that iteration's tx. The classic gotcha applies to the loop variables in the for header (idx, wireTx), not to := declarations inside the loop body.

2. Even if it were a loop variable, this repo is on Go 1.26 (go.mod line 3), so per-iteration scoping has been the default since the Go 1.22 LoopVar change.

For safety I did run the netsync suite under -race after my round 1 fix and saw no races, and the integration tests exercise this path with blocks containing same-block parent/child dependencies.

Leaving as-is unless you can point out a specific case where the current code misbehaves.

extendTransactions phase 2 now calls utxoStore.BatchPreviousOutputsDecorate(ctx, txs) once for the whole block. For the Aerospike UTXO store, BatchPreviousOutputsDecorate currently just loops PreviousOutputsDecorate per-tx sequentially (stores/utxo/aerospike/get.go:1183-1189), which removes the previous per-tx concurrency and prevents the outpoint batcher from coalescing requests across transactions. Consider either (a) making the Aerospike BatchPreviousOutputsDecorate enqueue all inputs across all txs before waiting, or (b) keeping a concurrent per-tx call path here when the backing store doesn’t implement a true bulk operation, to avoid a performance regression on large blocks.

Good catch — fixed by parallelizing BatchPreviousOutputsDecorate inside the Aerospike store itself (rather than in the caller) so the SQL store keeps its true bulk IN-clause path and Aerospike regains cross-tx batching via its shared outpointBatcher.

Details:

• Wrapped the per-tx PreviousOutputsDecorate calls in an errgroup bounded by s.settings.UtxoStore.OutpointBatcherSize (default 100) so goroutine count stays bounded on large blocks while still letting many txs' inputs pipeline into the same batcher flush.
• BatchPreviousOutputsDecorate's contract (dedup via PreviousTxScript != nil, error semantics) is unchanged.

This preserves the single-pass DB read for SQL and recovers the cross-tx outpoint coalescing for Aerospike without having to special-case the call site.

extendFromTxMap bounds-checkes PreviousTxOutIndex by casting it to int (if int(input.PreviousTxOutIndex) >= len(...)), but then indexes the slice using the original uint32 value. On 32-bit builds, the uint32 -> int cast can overflow to a negative value and bypass the check, leading to an out-of-bounds panic (DoS) for a hostile/malformed block. Compare using unsigned types (e.g., if input.PreviousTxOutIndex >= uint32(len(outputs))) or otherwise guard the cast before indexing.

Fair point — switched the bounds check to a uint32 comparison: if input.PreviousTxOutIndex >= uint32(len(prevTxWrapper.Tx.Outputs)). Removes the 32-bit-build int() overflow hole. Also dropped the subsequent int() cast since we index with the uint32 directly.