bitcoindevkit / bitcoindevkit/bdk-tx

AFS may complete the locktime-branch with all sequence values as `Sequence::MAX`, making AFS a no-op

Open
#70 0 comments 0 reactions 1 assignee Claimed by @evanlinjin View on GitHub
bug
Dominant language
Rust
Stars
13
Forks
17
Avg merge
17h 22m
Merged PRs (30d)
1

Description

### Describe the bug

There is a clause in the BIP-326 spec that has been excluded in our AFS implementation:

```
# Initialize all nsequence to indicate the requested RBF state
# nsequence can not be 2**32 - 1 in order for nlocktime to take effect
for input in transaction.inputs:
if rbf_set:
input.nsequence = 2**32 - 3
else:
input.nsequence = 2**32 - 2
```

This is bad.

When all input `nSequence` values are `Sequence::MAX`, the `nLockTime` value has no effect (still consensus-valid, but `nLockTime` is not enforced), essentially making the locktime-branch of AFS a no-op.

### Expected behavior

AFS should never complete the locktime-branch with all input sequence values as `Sequence::MAX`.

### Proposed Solution

The claude in BIP-326 is aggressive with setting nSequence values. For us, I think it's better to retain as much user intent as possible. I.e. what if the input needs to satisfy CSV?

I propose only changing `nSequence` if it is set to `Sequence::MAX`, and only doing so in the AFS locktime-branch.

I think it's better to have #66 and #65 merged before tackling this issue.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.