fix(legacy/netsync): bounds-check parent output index in ExtendTransaction#768
Merged
ordishs merged 3 commits intoMay 14, 2026
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete This PR adds critical bounds checking for parent output indices in No issues found. The implementation:
|
…ction (refs #4564)
143dda4 to
799533e
Compare
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-13 14:07 UTC |
Move the OOB bounds check ahead of the 120s WaitForParent loop in ExtendTransaction. Parent Outputs are populated at wire-parse time and never mutated afterwards, so the check is safe to run before waiting and lets a malformed peer block be rejected without burning up to two minutes per offending input on the polling loop. Refactor the test fixture into a buildOOBFixture helper and add TestSyncManager_extendFromTxMap_OOB so the sibling phase-1 path's TxInvalidError contract is now covered by a regression test in its own right. Loosen the string assertions in the existing test to errors.Is plus the parent hash so benign error-message rewording no longer breaks the test. Refs #4564
|
oskarszoon
approved these changes
May 14, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Approve. Fix is correct, both regression tests trigger the original panic when the bounds check is mentally removed, error type harmonised to TxInvalidError on both paths, the "Outputs populated at wire-parse and never mutated" justification for placing the check before WaitForParent checks out (grep for mutation on the netsync path returns nothing).
Two things worth flagging:
- Same DoS shape exists unfixed in
services/blockvalidation/quick_validate.goat:1061-1062and:1399-1400— unguardedparentTx.Outputs[input.PreviousTxOutIndex]against peer-controlledparentTxlookups. Same panic shape as #4564, but in the block-validation hot path (so worse blast radius). Out of scope for this PR; worth filing as a separate issue / follow-up PR. - For the PR description / commit log: worth noting that
ExtendTransaction(the parallel path getting the new check) has no production callers in the current tree — only the phase-1 pathextendFromTxMapis reachable from peer blocks today, and that path already had the bounds check pre-PR. So the practical runtime impact of #768 is the error-class harmonisation at line 1011; the new check at line 1256 is forward-looking defense-in-depth for when the parallel path gets wired up. Doesn't change the fix or the urgency, but clarifies what's actually closing the live exposure vs what's pre-emptive.
freemans13
approved these changes
May 14, 2026
This was referenced May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4564.
Summary
ExtendTransactiondereferencesprevTxWrapper.Tx.Outputs[input.PreviousTxOutIndex]without a bounds check. A peer can send a block where a child tx references its in-block parent withPreviousTxOutIndex >= len(parent.Outputs), panicking the node before full block validation runs. The phase-1 path (extendFromTxMap, around line 1011) already had this guard; the parallel path inExtendTransaction(lines ~1272-1273) was missed.Fix
Insert the same shape of bounds check that exists in
extendFromTxMap, immediately before the dereference. On out-of-range, returnerrors.NewTxInvalidError(...)with the full context (offending tx, input index, parent, output count). The block came from a peer; the transaction is invalid, soTxInvalidErroris more semantically correct thanProcessingError.Test plan
TestSyncManager_ExtendTransaction_OOBconstructs a parent with 2 outputs and a child whose input hasPreviousTxOutIndex == 5; pre-fix panics withindex out of range [5] with length 2; post-fix returnsTxInvalidErrorwith the expected message.go test -race ./services/legacy/netsync/...passes.