Re-implement BIP68#547
Conversation
|
🤖 Claude Code Review Status: Complete This PR re-implements BIP68 (relative lock-time via sequence numbers) after reverting PR 546. The implementation includes comprehensive unit and E2E tests. Observations:
The existing inline comment threads adequately cover the main architectural considerations. Human reviewers should verify the MTP approximation strategy and CodeQL findings. |
| // covers [blockHeight-12, blockHeight-2] — a 1-block-shifted approximation. | ||
| blockMTPHeight := blockHeight | ||
| if blockHeight > 0 { | ||
| blockMTPHeight = blockHeight - 1 |
There was a problem hiding this comment.
Critical: MTP off-by-one error breaks BIP68 consensus
The code uses blockHeight - 1 to fetch MTP for block validation, but this is incorrect and breaks consensus with BSV nodes.
What BSV C++ does:
For block N validation, uses block.GetPrev()->GetMedianTimePast() = median([N-11, N-1])
What this code does:
Fetches stored_mtp(blockHeight-1) = median([N-12, N-2]) - off by one block
Impact:
Time-based BIP68 sequence locks will be validated against the wrong MTP, causing consensus divergence and potential chain splits.
Fix:
Calculate MTP as median([N-11, N-1]) during validation, not from pre-stored values.
There was a problem hiding this comment.
Thank you for flagging this. After analyzing the code and tests:
Current Behavior:
The code at line 1211 does use blockHeight - 1, which the inline comment at line 1208 acknowledges creates a "1-block-shifted approximation."
Why E2E Tests Pass:
The E2E test TestBIP68_TimeBased_Accept validates against real BSV nodes successfully. However, TestBIP68_TimeBased_Reject is skipped (line 385 in bip68_test.go) with the note: "model.NewBlockFromMsgBlock only populates the coinbase subtree, so ValidateBlock never sees non-coinbase txs."
This suggests BIP68 may only be enforced during mempool validation (not block validation during IBD). During mempool validation, block N has already been persisted, making stored_mtp(blockHeight) available.
Recommendation:
Before treating this as critical, please verify:
- Is BIP68 enforced during block validation in the current codebase?
- If only during mempool validation, does the correct MTP become available?
Note: Line 1196 also has a documentation error claiming BSV uses median of [H-10, H] (including H), which contradicts BIP113 spec and other docs stating [H-11, H-1].
There was a problem hiding this comment.
Issue resolved in current code
The previous concern about blockHeight - 1 has been addressed. The code now correctly uses blockMTPHeight := blockHeight and fetches v.mtpStore[blockMTPHeight].
However, documentation error at line 1237: The comment incorrectly states BSV uses "median of [H-10, H] (includes H itself)". Per BIP113, MTP always uses the previous 11 blocks excluding the current block. The correct range is [H-11, H-1].
9d1ce4d to
5ce231d
Compare
|
🤖 Claude Code Review Status: Complete SummaryThis PR re-implements BIP68 (relative locktime) after reverting #546. The implementation adds:
Key ChangesMTP Storage (stores/blockchain/sql/StoreBlock.go:411)
MTP Retrieval (services/blockchain/median_time_past.go)
BIP68 Validation (services/validator/)
Test Coverage
Review FindingsNo issues found in current code. The implementation appears correct and well-tested. Note on Existing Inline CommentsSeveral older inline comments on Validator.go reference code from earlier iterations of this PR that has since been corrected:
These inline comments can be safely dismissed as they no longer apply to the current code. Minor Observations
RecommendationApprove for merge. Implementation is well-designed, thoroughly tested, and correctly implements BIP68 relative locktimes. |
4edf4e8 to
9a4169b
Compare
| // Build utxoMTPs and blockMTP from the pre-loaded mtpStore (populated by EnsureMTPLoaded). | ||
| // | ||
| // Teranode stores MTP(H) = median of block timestamps [H-11, H-1]. | ||
| // BSV's GetMedianTimePast() at block H = median of [H-10, H] (includes H itself), |
There was a problem hiding this comment.
Documentation error confirmed - comment is incorrect
The comment at line 1237 incorrectly states BSV GetMedianTimePast() includes block H itself in the median calculation [H-10, H].
This is wrong. Per BIP113 and BSV implementation, MTP of block H = median of previous 11 blocks only: [H-11, H-1]. Block H itself is never included.
The implementation is correct (uses stored_mtp(blockHeight) = median [blockHeight-11, blockHeight-1]), but this comment should be corrected.
9a4169b to
603ecfe
Compare
|
🤖 Claude Code Review Status: Complete Current Review: This PR re-implements BIP68 (relative lock-time via sequence numbers) after reverting #546. The implementation is well-structured with comprehensive tests and follows Bitcoin SV consensus rules. Issue Found:
Implementation Quality:
History:
|
603ecfe to
878029e
Compare
878029e to
2c47434
Compare
oskarszoon
left a comment
There was a problem hiding this comment.
Please dont merge until I've done some performance testing on Testnet
2c47434 to
ec8d0bb
Compare
ec8d0bb to
6ffee13
Compare
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-03-31 08:52 UTC |
Reverts #546
Re-implements #514