Enforce nLockTime/sequence transaction finality at submit#181
Open
sirdeggen wants to merge 5 commits into
Open
Enforce nLockTime/sequence transaction finality at submit#181sirdeggen wants to merge 5 commits into
sirdeggen wants to merge 5 commits into
Conversation
Add a finality check to ValidateTransaction mirroring teranode util.IsTransactionFinal (consensus rule TNJ-13): a tx is final when all input sequence numbers are 0xffffffff, nLockTime is 0, or nLockTime is satisfied against the chain height / median-time-past. Non-final txs are rejected with StatusMalformed. Height-based locks read the chain tip from the store (next-block height); time-based locks use the wall clock. Chain-tip read failures accept rather than block submission.
sirdeggen
requested review from
galt-tr and
shruggr
and removed request for
mrz1836
gosec G115 flagged the uint64->uint32 height and int64->uint32 time conversions in checkFinality; replace with saturating clamp helpers. Annotate the chain-tip read-failure path with nolint:nilerr — returning nil there is intentional graceful degradation.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds submit-time transaction finality validation for nLockTime/sequence handling before SPV verification, integrating chain-tip height data from the application store.
Changes:
- Adds finality helpers and tests for height/time lock behavior.
- Calls finality validation from
ValidateTransactionand maps non-final transactions to malformed status. - Wires the store into the validator as the chain-height source during app bootstrap.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
validator/finality.go |
Adds finality evaluation logic and chain-tip interface. |
validator/finality_test.go |
Adds coverage for finality edge cases. |
validator/validator.go |
Invokes finality checks during transaction validation. |
app/app.go |
Configures the validator with the store-backed chain-tip source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+103
to
+109
| // Time-based locktime: wall clock is >= median-time-past, so a lock that | ||
| // has elapsed against now() has certainly elapsed against MTP. | ||
| now := v.now | ||
| if now == nil { | ||
| now = func() int64 { return time.Now().Unix() } | ||
| } | ||
| if validLockTime(tx.LockTime, 0, clampI64ToU32(now())) { |
Contributor
Author
There was a problem hiding this comment.
Yeah this definitely needs to be fixed, the MTP can be calculated with header data only I believe that's possible via chaintracks.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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
spv.Verifycovers only fees, merkle paths, and script execution.ValidateTransactionmirroring teranodeutil.IsTransactionFinal(consensus rule TNJ-13).Changes
validator/finality.go(new):IsFinal+validLockTimemirror teranodeutil.IsTransactionFinal/util.ValidLockTime. A tx is final when every input sequence is0xffffffff,nLockTime == 0, ornLockTimeis satisfied (< 500000000→ block height;>=→ timestamp/MTP).checkFinalitysources height-based locks from the chain tip (next-block height) and time-based locks from the wall clock; chain-tip read failures accept rather than block submission.validator/validator.go: callcheckFinalityinValidateTransactionbetween policy andspv.Verify; mapErrTxNotFinal→StatusMalformed. Added nil-safechainTip/nowfields withSetChainTip/SetNowFunc.app/app.go: wire the store (GetActiveTipBlockHeight) as the chain-tip source.Test plan
go test ./validator/— 14 finality tests pass (locktime-zero, all-inputs-final, height/time pass+fail, mixed sequences, threshold edge cases either side of 500000000, nil-tip skip, tip-error accept)go build ./...,go vet ./validator/ ./app/cleanCloses ARC issue: bitcoin-sv/arc#145