[Minor] Context propagation inconsistency

The batched write path on line 428 submits items that will later be flushed using context.Background() (see line 120 in Submit and line 171 in timerLoop), discarding the ctx parameter passed to writeSubtree.

In contrast, writeSubtreeDirect uses the provided context throughout (line 439), allowing cancellation and tracing to propagate correctly.

Impact: During catch-up, if the parent context is cancelled (e.g., shutdown signal), already-queued items will flush with context.Background() and ignore the cancellation. The Stop() call will still drain pending items, so this is primarily a tracing/observability gap rather than a correctness issue.

Suggestion: Consider passing ctx through to Submit and storing it with each item, or document this intentional behavior if the background context is preferred for durability guarantees.

✅ Fixed in latest commit

The code now passes ctx to Submit() on lines 467-473, so the caller context propagates correctly for count-threshold flushes.

Stop() does not wait for a count-threshold flush that was triggered by Submit() just before Stop acquired the mutex. Because Submit() performs flushFn() outside the batcher goroutine and is not tracked by b.wg, Stop() can return before that in-flight flush completes, violating the documented “all pending items flushed before Stop returns” contract and risking data loss on shutdown/mode transitions. Consider tracking all flushFn invocations with a dedicated WaitGroup (increment before calling flushFn in Submit/timerLoop and decrement after) and waiting for it in Stop() before returning.

Fixed. Added a dedicated inflight WaitGroup that covers every flushFn invocation (Submit count-threshold flushes, timerLoop, and the final Stop-path flush). The slot is reserved under the same mutex that gates stopped/buf so a Submit flush racing Stop cannot slip past. Stop now blocks on inflight.Wait() in addition to the timer goroutine wg before returning.

Submit()/timerLoop call flushFn with context.Background(), which drops cancellation/timeouts and any tracing context from the caller. In particular, writeSubtree() passes a ctx that may be cancelled on shutdown or block-processing abort, but count-based flushes will ignore it. Consider changing Submit to accept a context (or storing a context in the batcher) and passing that through to flushFn so flushes can be cancelled and can participate in tracing/log scoping.

Fixed. Submit now takes a ctx context.Context parameter which is forwarded to flushFn for count-threshold flushes. writeSubtree passes the caller ctx through, so a cancellation on shutdown or block-processing abort now propagates. Timer-path flushes continue to use context.Background() since they have no caller ctx to inherit, and errors still surface via lastErr.

On quickValidationMode transition, batcher.Stop(ctx) errors are only logged and then the code proceeds to the direct write path. If Stop fails, there may be buffered subtree items that never made it to disk, and subsequent validation/readers will see missing blobs. Consider returning the Stop error (or wrapping it) so block processing fails fast rather than continuing with a partially persisted subtree set.

Fixed. writeSubtree now returns the wrapped Stop error instead of falling through to the direct path, so block processing fails fast if the drain failed. Using errors.NewStorageError so callers can classify the failure the same way as other blob-store errors.

txWrapper.SomeParentsInBlock is written from multiple per-input goroutines without synchronization, which will trigger a data race under -race and can lead to undefined behavior. Set this flag before launching the goroutine (when you detect a same-block parent), or protect it with a mutex/atomic.

Fixed. Moved txWrapper.SomeParentsInBlock = true outside the goroutine so the write happens on the single loop goroutine rather than the per-input errgroup workers. All writers set the same value, but an unsynchronised concurrent write is still a data race under -race.

This test uses a fixed time.Sleep to wait for the timer flush to run, which can be flaky or unnecessarily slow under variable CI load. Prefer require.Eventually (or a signal/channel from flushFn) to deterministically wait until the timer-path flush has executed and its error has been captured.

Fixed. Replaced the fixed time.Sleep with a flushed channel signalled from inside the flush callback, plus a require.Eventually on the observable Submit-returns-boom condition. This removes the fixed-timeout dependency while still tolerating the small window between the flush signal and lastErr being published.

This PR introduces substantial additional behavior beyond subtree write batching (two-phase transaction extension plus new SQL query strategy in BatchPreviousOutputsDecorate). The PR title/description currently focus on subtree batching; consider updating them to explicitly mention the previous-output batching/extendTransactions changes (or splitting into a separate PR) to make review/rollback scope clearer.

Acknowledged. The previous-outputs batch-decoration / two-phase extend work is logically coupled to the subtree write batching (both are catch-up throughput optimisations that only activate in quickValidationMode), but the title does understate the scope. I will update the PR title/description to call out both changes so the review/rollback unit is explicit.

Stop() calls takeLastErr() only at the beginning. If a timer-path flush is in progress while Stop is called, it can fail and set b.lastErr after the initial takeLastErr, and then Stop will return nil (or only the pending-flush error) and the timer error will never be surfaced because the batcher is stopped. Consider calling takeLastErr() again after b.wg.Wait() (and possibly after the final pending flush) and returning/combining that error so timer flush failures aren’t lost during shutdown.

Fixed. Stop now re-reads takeLastErr() after wg.Wait() and the final pending flush, so timer-path errors captured during shutdown are surfaced. If the final pending flush itself fails, that error takes precedence (it is the batch the caller most directly relied on being persisted) and any queued timer error is cleared so it is not returned by a future Stop call.

[Minor] Documentation inaccuracy

The comment says "It waits up to 120 seconds for each same-block parent" but the implementation does not wait at all. Looking at lines 1138-1141 below, the code directly reads parent outputs without any polling or timeout:

// Parent's Outputs are populated at wire-parse time and never mutated
// afterwards, so we can read them immediately without waiting for the
// parent tx itself to finish being extended.

Suggested fix: Remove the "waits up to 120 seconds" phrase. For example:

// extendFromTxMap populates a transaction's inputs whose parents are in the same
// block (available via txMap). Parent outputs are read directly from the txMap
// without waiting, as they are populated at wire-parse time. Child and parent may be
// processed by different goroutines in the enclosing errgroup.

✅ Fixed in latest commit

The docstring at lines 1090-1098 has been corrected and no longer mentions waiting. It accurately describes the immediate read behavior.

The goroutine launched in this loop captures the tx variable from the surrounding loop scope. Since tx is reassigned each iteration, all goroutines may end up calling extendFromTxMap on the last transaction (classic closure-capture bug), leading to incorrect/missing input decoration and potential data races. Capture the per-iteration tx in a new variable (e.g., txLocal := txWrapper.Tx) and have the closure reference that (or pass it as an argument) before calling g.Go.

Not a bug — tx := txWrapper.Tx is declared inside the loop body with :=, so it is a fresh variable in each iteration (block-scoped, not the loop header), and each g.Go closure captures its own tx. This is independent of the Go 1.22 per-iteration-loop-variable change. Additionally this module is on Go 1.26, where even the loop-header variables are per-iteration. The existing txs = append(txs, tx) call on the preceding line would also be broken if the closure capture were wrong, but phase 2 consumes txs correctly in the tests.

The doc comment for extendFromTxMap says it “waits up to 120 seconds for each same-block parent to be fully extended”, but the implementation no longer waits (and explicitly states waiting is unnecessary). Please update the comment to match the current behavior to avoid misleading future maintainers.

Fixed — updated the extendFromTxMap doc comment to state that same-block parent Outputs are read directly without waiting, with a brief explanation of why the read is safe (Outputs are populated at wire-parse time and not mutated during input extension). Dropped the stale "waits up to 120 seconds" language.

This function now always runs the two-phase + BatchPreviousOutputsDecorate path, but the PR description says the behavior is gated on quickValidationMode and that non-quick blocks should fall back to the original per-tx PreviousOutputsDecorate loop. Since extendTransactions is called before quickValidationMode is computed in prepareSubtrees, this change currently applies in all modes. If the gating is required, pass quickValidationMode into extendTransactions (or move the call until after it’s determined) and keep the legacy per-tx decoration for non-quick blocks.

Correct observation — the PR description was misleading on this point. The two-phase + BatchPreviousOutputsDecorate path is intentionally not gated on quickValidationMode: it is a functional-equivalent refactor of the same decoration step, applied to every legacy-sync block. BatchPreviousOutputsDecorate skips inputs that phase 1 already populated and returns the same per-input result the old per-tx loop produced, just via a single batched query; on a missing-parent / processing error it falls back to the original extendPerTxFallback path so the DAH-recovery via utxoStore.Get(fields.Tx) still applies. The quickValidationMode gating claim in the earlier description draft was wrong — I have rewritten the PR description to make the subtree-batcher-only gating explicit and explain why the decoration refactor is safe to apply universally.

extendFromTxMap indexes prevTxWrapper.Tx.Outputs[input.PreviousTxOutIndex] without validating that the referenced output index exists. A malformed/invalid block could cause an out-of-range panic here before normal validation has a chance to return a structured error. Add a bounds check on input.PreviousTxOutIndex against len(prevTxWrapper.Tx.Outputs) and return a processing/tx error when the outpoint is invalid.

Fixed. Added an explicit bounds check on input.PreviousTxOutIndex before indexing into prevTxWrapper.Tx.Outputs; returns an errors.NewTxError with the offending tx, input index, outpoint index, and parent hash if the outpoint is out of range. The error propagates through the errgroup back up to extendTransactions, which wraps it as a processing error so the block is rejected via the normal error path rather than panicking mid-goroutine.

SyncManager.Stop() logs and discards errors returned by SubtreeWriteBatcher.Stop(), but Stop() itself returns an error. If the drain fails, this can silently lose buffered subtree writes during shutdown. Consider propagating the error (or aggregating it with other shutdown errors) so callers can detect an unclean shutdown.

Fixed. SyncManager.Stop() now captures the drain error into stopErr (in addition to logging it) and returns that from Stop so callers can detect an unclean shutdown. Used stopErr rather than a multi-error aggregator because this is currently the only shutdown-path operation that returns an error; we can promote to a multi-error pattern later if more sources emerge.

The Exists() calls here intentionally skip serialization when blobs are already present, but they currently ignore any Exists() error. If Exists fails (e.g., store unavailable), the code will proceed to Serialize() and Submit anyway, which can trigger the very multi-GB allocations this guard is meant to avoid and also hides the underlying storage failure. Capture and propagate the Exists error (wrap as a storage/processing error) rather than discarding it.

Fixed. All three Exists() calls in the batched writeSubtree path now propagate errors as [writeSubtree] check subtree/data/meta exists storage errors rather than swallowing them — a store failure now aborts the block before any Serialize() allocation instead of silently proceeding.

This meta existence check also discards the error return from Exists(). If Exists fails, the flush will proceed as if the blob is absent, potentially doing unnecessary work and masking a store failure until later. Prefer handling the error explicitly (return a storage/processing error) so the batch flush fails with the real root cause.

Fixed. flushSubtreeWriteBatch now propagates the Exists() error for the meta existence check as a storage error instead of discarding it.

The comment bullet says "Stop() returns the final-flush error and/or lastErr", but Stop currently returns either finalErr (preferred) or lastErr (and clears lastErr when returning finalErr). Update this comment to match the actual semantics (it never returns both errors).