fix(afs)!: preserve input timelock requirements - #65
Conversation
d6a0843 to
dc4b147
Compare
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Thanks for working on this.
PR description is misleading - none of these bugs were flagged in the post-merge review. It also says this "fixes two consensus-validity issues", however this PR attempts to close 3 issues?
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
There are two versions of the random_probability and random_range methods. The std versions add no meaningful value imo - I would just remove them.
67dcf29 to
8487ab4
Compare
|
Thank you for the review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
f3d7e60 to
3b72e6d
Compare
3b72e6d to
b0b0eb1
Compare
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Make apply_anti_fee_sniping explicitly private!
15943a8 to
adf472f
Compare
adf472f to
8085f45
Compare
|
We are lacking commit messages, PR description has factual errors, missing I'll push it forward! |
evanlinjin
changed the title
b8671c3 to
c83079a
Compare
c83079a to
1211bc3
Compare
|
@aagbotemi I pushed a Claude-generated test commit on top, let me know what you think. |
059752f to
2fcd3c5
Compare
Anti-fee-sniping could produce consensus-invalid transactions when inputs had timelock requirements: - The nLockTime strategy could overwrite `tx.lock_time` with a value lower than the accumulated input CLTV. - The nSequence strategy zeroed `tx.lock_time`, also breaking accumulated CLTV. - The nSequence strategy could select a Taproot input that already had a relative-timelock (CSV) requirement and overwrite its sequence. The locktime path now only updates `tx.lock_time` when the proposed AFS value still satisfies the existing requirement. The sequence path leaves `tx.lock_time` alone and filters CSV-bearing Taproot inputs out of the candidate pool. API changes: - `PsbtParams::enable_anti_fee_sniping: bool` and `fallback_locktime` are replaced with `anti_fee_sniping: Option<absolute::Height>` and `min_locktime`. The tip height is now required when AFS is enabled and time-based tips are unrepresentable. - `apply_anti_fee_sniping` is now `pub(crate)`; the `rbf_enabled` parameter is removed (derived from `tx.is_explicitly_rbf()`). - New `AntiFeeSnipingError` type, wrapped under `CreatePsbtError::AntiFeeSniping`. Closes bitcoindevkit#62 Closes bitcoindevkit#63 Closes bitcoindevkit#64
Three new tests, each verified to fail on the pre-fix algorithm: - `test_anti_fee_sniping_preserves_input_cltv` — input CLTV above tip must not be lowered. - `test_anti_fee_sniping_skips_taproot_csv_input` — a Taproot input carrying a CSV requirement must be excluded from the nSequence candidate pool; a regular Taproot input remains to ensure the sequence path is still reachable. - `test_anti_fee_sniping_rejects_time_based_locktime` — a time-based CLTV must surface `AntiFeeSnipingError::UnsupportedLockTime`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2fcd3c5 to
5df9bb1
Compare
| .iter() | ||
| .find(|input| input.prev_outpoint() == txin.previous_output)?; | ||
| if input.prev_txout().script_pubkey.is_p2tr() { | ||
| if input.prev_txout().script_pubkey.is_p2tr() && input.relative_timelock().is_none() { |
There was a problem hiding this comment.
BIP326 is under-specified when it comes to inputs that already come with a CSV attached. We chose a conservative reading by excluding all inputs with a relative timelock, but a refinement could be to admit block-based CSV inputs whose requirement is dominated by their confirmation depth.
Then eligibility becomes "is p2tr, and either has no relative timelock, or has a block-based one whose value is < input.confirmations(tip_height)" at which point the logic collapses to just confirmations (except that the 10% random backoff must floor at max(csv_blocks, 1) instead of 1).
| } | ||
| } else { | ||
| // Use Sequence | ||
| tx.lock_time = LockTime::ZERO; |
There was a problem hiding this comment.
The sequence branch exists so your transaction resembles an off-chain settlement spending a timelock path, and those transactions have nLockTime = 0 with a CSV-driven sequence. A transaction carrying both a near-tip locktime and a confirmation-depth sequence resembles neither an ordinary AFS wallet spend nor a contract closing. You've built a third fingerprint, which is worse than either branch alone. So the current code isn't just semantically loose, it actively undercuts the branch's only purpose. The sequence branch is only ever coherent when tx.lock_time is already zero, so make that a precondition as part of must_use_locktime.
let must_use_locktime = tx.lock_time != LockTime::ZERO
|| taproot_inputs.is_empty()
|| inputs.iter().any(|input| { /* ... */ });| /// an input's CLTV, provided the locktime units agree. If `min_locktime` uses a different unit | ||
| /// (block-height vs. time) than an input's CLTV, it is ignored — a height-based `min_locktime` | ||
| /// will not be combined with a time-based CLTV (and vice versa). | ||
| pub min_locktime: absolute::LockTime, |
There was a problem hiding this comment.
There is essentially no value of min_locktime that is simultaneously meaningful and compatible with AFS. That's a strong signal the two fields are answering the same question - "who decides nLockTime when the inputs don't force it, and should both be settable?"
- Below
tip - 100: AFS always wins the max, so the floor does nothing - Within
(tip - 100, tip]: The conflict zone, truncates or erases the 10% backoff - Above tip: The floor always wins, so AFS does nothing
So I'd fold them into a LockTimePolicy with the default as Min(LockTime::ZERO), replacing both PsbtParams fields. Input CLTV requirements still max on top in either arm; document that once in the enum's doc, instead of scattered across two fields that have to cross-reference each other.
pub enum LockTimePolicy {
/// Floor `tx.lock_time` at this value.
Min(absolute::LockTime),
/// Apply BIP-326 anti-fee-sniping using this chain tip.
AntiFeeSniping(absolute::Height),
}
The nSequence path exists so the tx resembles an off-chain settlement spending a timelock path, and those carry nLockTime = 0. bitcoindevkit#65 dropped the `tx.lock_time = ZERO` that the path used to perform (correctly, since it regressed input CLTVs) but added no precondition in its place, so a tx whose locktime was already pinned — by an input's CLTV or by `min_locktime` — could come out with both a near-tip locktime and a confirmation-depth sequence. That matches neither an ordinary wallet spend nor a contract close: a third fingerprint, worse than either branch alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nSequence path exists so the tx resembles an off-chain settlement spending a timelock path, and those carry nLockTime = 0. bitcoindevkit#65 dropped the `tx.lock_time = ZERO` that the path used to perform (correctly, since it regressed input CLTVs) but added no precondition in its place, so a tx whose locktime was already pinned — by an input's CLTV or by `min_locktime` — could come out with both a near-tip locktime and a confirmation-depth sequence. That matches neither an ordinary wallet spend nor a contract close: a third fingerprint, worse than either branch alone. Co-Authored-By: ValuedMammal <95981133+ValuedMammal@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e05146e fix(afs): require zero locktime for the nSequence path (志宇) Pull request description: ### Description Fixes #65 (comment) — point 2 of #73 (comment). The nSequence path exists so the tx resembles an off-chain settlement spending a timelock path, and those carry `nLockTime = 0` with a CSV-driven sequence. #65 removed the `tx.lock_time = LockTime::ZERO` that path used to perform — correctly, since it regressed input CLTVs — but added no precondition in its place. So a tx whose locktime is already pinned, by an input's CLTV or by `min_locktime`, can come out with **both** a near-tip locktime and a confirmation-depth sequence. That matches neither an ordinary wallet spend nor a contract close: a third fingerprint, worse than either branch alone. The path is only coherent at `lock_time == 0`, so make that a precondition: ```rust let must_use_locktime = tx.lock_time != LockTime::ZERO || taproot_inputs.is_empty() || inputs.iter().any(|input| { /* ... */ }); ``` ### Changelog notice ```md Fixed: - Anti-fee-sniping only takes the `nSequence` path when `tx.lock_time` is zero. A locktime pinned by an input's CLTV or by `min_locktime` now always takes the `nLockTime` path. ``` ### Before submitting - [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-tx/blob/master/CONTRIBUTING.md) - [ ] This PR breaks the existing API 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: ValuedMammal: tACK e05146e Tree-SHA512: 2c2e0f8630d5ab33dbf6a848325040d175ac4d10d116b5813bde51f668230308443cfd659b0658fc36ac0ad4ab4150de90e3c4a9600eea2a9a17aef96068e874
3 participants
Description
Fixes consensus-validity issues in BIP-326 anti-fee-sniping.
Closes #62
Closes #63
Closes #64
Bugs
tx.lock_timewith a value lower than the accumulated input CLTV.tx.lock_time, also breaking accumulated CLTV.Fixes
tx.lock_timewhen the AFS-proposed value still satisfies the existing requirement.tx.lock_timealone and filters CSV-bearing Taproot inputs out of the candidate pool.Breaking changes
PsbtParams::enable_anti_fee_sniping: bool+fallback_locktime→anti_fee_sniping: Option<absolute::Height>+min_locktime. The tip height is now required when AFS is enabled, and time-based tips are unrepresentable.apply_anti_fee_snipingis nowpub(crate);rbf_enabledparam removed (derived fromtx.is_explicitly_rbf()).AntiFeeSnipingErrortype, wrapped underCreatePsbtError::AntiFeeSniping.