Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve fee sniping deterrence [BIP326] #1251

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ValuedMammal
Copy link
Contributor

@ValuedMammal ValuedMammal commented Dec 30, 2023

Description

First attempt at bip326

for reference:
BIP326
Bitcoin Core
Sparrow

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

closes #1134

This change adds two tests to wallet.rs. In
`test_create_tx_fee_sniping_tr` we check that for a taproot
transaction with rbf enabled, either the tx LockTime is set to the most
recent checkpoint, or the input's Sequence is set to its confirmations
(allowing for randomness in both cases).

In `test_create_tx_fee_sniping_tr_csv` we check that policy
csv requirements take precedence over a default nSequence. A new
test descriptor getter is added to common.rs
`get_test_tr_with_taptree_csv` that contains a script path with a csv
condition.
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 1, 2024
@ValuedMammal ValuedMammal marked this pull request as ready for review January 7, 2024 19:11
// Occasionally, randomly pick a lower sequence.
confs = confs.saturating_sub(rng.gen_range(0u16..100));
}
transaction.input[idx].sequence = Sequence::from_height(confs);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to find the input corresponding to the randomly selected utxo above, rather than assume the same idx

@LLFourn
Copy link
Contributor

LLFourn commented Mar 15, 2024

@ValuedMammal since lightning doesn't us taproot yet is there a strong motivation for doing this now?
I do think the goal of this is admirable and I think implementing it as the default as part of a bdk_tx_builder crate would make it cleaner (where we'd drop the policy stuff in favor of the miniscript planning module).

@notmandatory notmandatory added module-wallet enhancement New feature or request labels Mar 16, 2024
@ValuedMammal ValuedMammal marked this pull request as draft March 17, 2024 00:18
@notmandatory
Copy link
Member

I propose we push to to a post 1.0 release since it's a functional improvement that shouldn't change the current wallet API.

@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module-wallet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

BIP 326 Anti Fee Sniping
4 participants