fix(legacy/netsync): nil-deref guards in input decoration paths#862
Merged
freemans13 merged 5 commits intoMay 21, 2026
Merged
Conversation
Both extendFromTxMap (phase-1 same-block parents) and ExtendTransaction (parallel-decoration path) dereference prevTxWrapper.Tx and the parent's Output / LockingScript without first checking they're non-nil. A malformed or hostile peer block could land us in any of these states: - TxMapWrapper present but Tx == nil (entry created before parse) - parent.Outputs[i] == nil - parent.Outputs[i].LockingScript == nil In production those would panic on the deref into bscript.NewFromBytes or on the Outputs index. The existing bounds-check from bsv-blockchain#768 covers out-of-range indices but not nil entries within the slice. Add the three guards on both paths, returning TxInvalidError so the caller's normal error path runs. Tests: - TestSyncManager_extendFromTxMap_NilParentTx - TestSyncManager_extendFromTxMap_NilOutput - TestSyncManager_extendFromTxMap_NilLockingScript - TestSyncManager_ExtendTransaction_NilParentTx - new buildInRangeFixture helper (parallel to buildOOBFixture) so the bounds-check passes and the nil-check is the one under test Originally bundled in bsv-blockchain#723; pulled out as a focused PR after bsv-blockchain#723's core change (CATCHINGBLOCKS quick-validation gating) was superseded by bsv-blockchain#847. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
🤖 Claude Code Review Status: Complete Current Review:
Summary:
The only minor suggestion is to add two additional tests (ExtendTransaction nil output/locking script) to achieve complete symmetry with the extendFromTxMap test suite. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-21 09:52 UTC |
…extend-nil-checks
…extend-nil-checks
…extend-nil-checks
oskarszoon
approved these changes
May 21, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Guards are correct, minimal, at the right layer. Approve with comments.
Worth fixing before merge: extendFromTxMap has three nil-path tests (NilParentTx, NilOutput, NilLockingScript); ExtendTransaction only has NilParentTx. The prevOutput == nil || prevOutput.LockingScript == nil guard at handle_block.go:1382-1386 is unverified. Add ExtendTransaction_NilOutput and ExtendTransaction_NilLockingScript to match — the two paths are independent goroutine-based code, regression in the parallel path would slip through.
Worth a comment or follow-up: ExtendTransaction wraps the goroutine error in NewProcessingError at g.Wait() (line 1417). The TxInvalid sentinel ends up two levels deep after extendTransactions wraps it again (line 1016-1017). errors.Is(err, ErrTxInvalid) still reaches it via the custom Is, but the test asserts this on ExtendTransaction's direct return, not on the orchestrating caller's return. No production caller currently checks for ErrTxInvalid specifically (all errors → block reject), so this is fine for safety. Classification just silently lost — worth clarifying intended semantics.
Questions:
- Is
TxMapWrapperever populated with nilTxoutside tests? PR description says "entry created before parse" — is that a real two-phase population state, or purely defensive against malicious peer messages? ExtendTransactionis exported. Called from outsidenetsyncother than tests? Callers may depend on prior panic / error-type behaviour.
…extend-nil-checks
| require.Error(t, err) | ||
| require.True(t, errors.Is(err, errors.ErrTxInvalid), "expected TxInvalid error, got %v", err) | ||
| require.Contains(t, err.Error(), parentHash.String()) | ||
| } |
Contributor
There was a problem hiding this comment.
[Minor] Test coverage gap: ExtendTransaction has the same nil output and nil locking script guards (lines 1418-1422 in handle_block.go) as extendFromTxMap, but only the nil parent test exists for ExtendTransaction. Consider adding TestSyncManager_ExtendTransaction_NilOutput and TestSyncManager_ExtendTransaction_NilLockingScript to mirror the extendFromTxMap test suite for consistency and completeness.
|
gokutheengineer
approved these changes
May 21, 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.
Summary
Both
extendFromTxMap(phase-1 same-block parents) andExtendTransaction(parallel-decoration path) dereferenceprevTxWrapper.Tx, the parent'sOutput, and the output'sLockingScriptwithout first checking they're non-nil. A malformed or hostile peer block could leave us in any of these states:TxMapWrapperpresent butTx == nil(entry created before parse)parent.Outputs[i] == nilparent.Outputs[i].LockingScript == nilIn production those would panic on the deref into
bscript.NewFromBytesor on theOutputsindex. The bounds-check added in #768 covers out-of-range indices but not nil entries within the slice.This PR adds the three guards on both paths, returning
TxInvalidErrorso the caller's normal error path runs.Tests
TestSyncManager_extendFromTxMap_NilParentTxTestSyncManager_extendFromTxMap_NilOutputTestSyncManager_extendFromTxMap_NilLockingScriptTestSyncManager_ExtendTransaction_NilParentTxbuildInRangeFixturehelper (parallel tobuildOOBFixture) so the bounds-check passes and the nil-check is the one under testgo test -race -short ./services/legacy/netsync/— 56 passed.go vetclean.Context
Originally bundled in #723; pulled out as a focused PR after #723's core change (broadening
quickValidationModetoCATCHINGBLOCKS) was superseded by #847.