Discourage fee sniping with nLockTime by danielabrozzoni · Pull Request #611 · bitcoindevkit/bdk

danielabrozzoni · 2022-05-24T10:49:23Z

Description

By default bdk sets the transaction's nLockTime to current_height
to prevent fee sniping.
current_height can be provided by the user through TxParams; if the user
didn't provide it, we use the last sync height, or 0 if we never synced.

Fixes #533

Notes to the reviewers:

If you want to know more about fee sniping: https://bitcoinops.org/en/topics/fee-sniping/

Checklists

All Submissions:

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

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

src/wallet/tx_builder.rs

rajarshimaitra

tACK f8da54e

Just for the sake of completeness, maybe one more test showing, when the wallet isn't synced locktime defaults to 0?

And few other non blocking nits..

src/wallet/mod.rs

danielabrozzoni · 2022-06-01T18:23:00Z

Thanks for the review! I updated the code removing the duplicated test, and adding some checks to the already existing tests (for example, testing that the current_height won't override the user specified nlocktime).
Turns out that the "never synced" case was already tested, in test_create_tx_default_locktime - but I added a comment to explicit this.

rajarshimaitra

tACK dc5471f

Thanks for the update.. Just one more comment for future PR maybe..

This one seems good to go..

src/wallet/tx_builder.rs

danielabrozzoni · 2022-06-06T14:24:01Z

Rebased

CHANGELOG.md

src/wallet/mod.rs

src/wallet/tx_builder.rs

vladimirfomene

Tested ACK. Just had a question

src/wallet/mod.rs

rajarshimaitra

ReACK 07c77f9

afilini · 2022-06-28T06:44:13Z

This needs rebasing and then it's ready to go

By default bdk sets the transaction's nLockTime to current_height to discourage fee sniping. current_height can be provided by the user through TxParams; if the user didn't provide it, we use the last sync height, or 0 if we never synced. Fixes bitcoindevkit#533

danielabrozzoni · 2022-06-28T08:35:21Z

Rebased :)

afilini

ACK 97bc9dc

e85aa24 Avoid using immature coinbase inputs (Daniela Brozzoni) 0e0d5a0 populate_test_db accepts a `coinbase` param (Daniela Brozzoni) Pull request description: ### Description With this PR we start considering how many confirmations a coinbase has. If it's not mature yet, we don't use it for building transactions. Fixes #413 ### Notes to the reviewers This PR is based on #611, review that one before reviewing this 😄 007c5a7 adds a coinbase parameter to `populate_test_db`, to specify if you want the db to be populated with immature coins. This is useful for `test_spend_coinbase`, but that's probably going to be the only use case. I don't think it's a big deal to have a test function take an almost_always_useless parameter - it's not an exposed API, anyways. But, if you can come up with a different way of implementing `test_spend_coinbase` that doesn't require 007c5a7, even better! I looked for it for a while, but other than duplicating the whole `populate_test_db` code, which made the test way harder to comprehend, I didn't find any other way. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: afilini: ACK e85aa24 Tree-SHA512: 30f470c33f9ffe928500a58f821f8ce445c653766459465eb005031ac523c6f143856fc9ca68a8e1f23a485c6543a9565bd889f9557c92bf5322e81291212a5f

92b9597 Rename `set_current_height` to `current_height` (Alekos Filini) Pull request description: ### Description Usually we don't have any prefix except for methods that can *add* to a list or replace the list entirely (e.g. `add_recipients` vs `set_recipients`) I missed this during review of #611 ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: danielabrozzoni: utACK 92b9597 - I'm sorry I didn't notice it! Tree-SHA512: 3391068b2761bcd04d740ef41f9e772039fca7bc0e0736afcbc582ec74b6c91eb155d9e09dd7a07462eec29e32ac86e41ba339d9a550af3f754164cab6bdbf61

danielabrozzoni force-pushed the prevent_fee_sniping branch 3 times, most recently from d9753d7 to e809697 Compare May 24, 2022 10:58

danielabrozzoni changed the title ~~Prevent fee sniping with nLockTime~~ Discourage fee sniping with nLockTime May 24, 2022

notmandatory added this to the Release 0.20.0 Feature Freeze milestone May 24, 2022

danielabrozzoni force-pushed the prevent_fee_sniping branch from e809697 to a7859d4 Compare May 25, 2022 18:10

danielabrozzoni mentioned this pull request May 25, 2022

Avoid using immature coinbase inputs #614

Merged

6 tasks

afilini reviewed May 26, 2022

View reviewed changes

src/wallet/tx_builder.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the prevent_fee_sniping branch from a7859d4 to 6556fbb Compare May 26, 2022 17:19

notmandatory assigned danielabrozzoni May 26, 2022

danielabrozzoni force-pushed the prevent_fee_sniping branch from 6556fbb to f8da54e Compare May 26, 2022 21:26

rajarshimaitra reviewed Jun 1, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the prevent_fee_sniping branch from f8da54e to dc5471f Compare June 1, 2022 18:20

rajarshimaitra approved these changes Jun 5, 2022

View reviewed changes

src/wallet/tx_builder.rs Show resolved Hide resolved

danielabrozzoni force-pushed the prevent_fee_sniping branch from dc5471f to a46bf9b Compare June 6, 2022 14:23

afilini reviewed Jun 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/tx_builder.rs Show resolved Hide resolved

vladimirfomene reviewed Jun 7, 2022

View reviewed changes

src/wallet/mod.rs Show resolved Hide resolved

notmandatory mentioned this pull request Jun 7, 2022

W23 BDK Library Team Call bitcoindevkit/.github#8

Closed

16 tasks

danielabrozzoni force-pushed the prevent_fee_sniping branch from a46bf9b to a546738 Compare June 8, 2022 10:41

danielabrozzoni requested a review from afilini June 8, 2022 10:42

kafaichoi reviewed Jun 10, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the prevent_fee_sniping branch 2 times, most recently from 3f528f2 to 07c77f9 Compare June 13, 2022 08:38

rajarshimaitra approved these changes Jun 14, 2022

View reviewed changes

danielabrozzoni force-pushed the prevent_fee_sniping branch from 07c77f9 to 97bc9dc Compare June 28, 2022 08:35

afilini approved these changes Jun 29, 2022

View reviewed changes

afilini merged commit bb55923 into bitcoindevkit:master Jun 29, 2022

afilini mentioned this pull request Jul 13, 2022

Rename set_current_height to current_height #667

Merged

3 tasks

danielabrozzoni deleted the prevent_fee_sniping branch August 16, 2022 17:06

notmandatory mentioned this pull request Apr 7, 2025

Wallet Privacy Improvements bitcoindevkit/bdk_wallet#28

Open

12 tasks

Conversation

danielabrozzoni commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers:

Checklists

All Submissions:

New Features:

Uh oh!

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni commented Jun 1, 2022

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielabrozzoni commented Jun 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vladimirfomene left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

afilini commented Jun 28, 2022

Uh oh!

danielabrozzoni commented Jun 28, 2022

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danielabrozzoni commented May 24, 2022 •

edited

Loading

vladimirfomene left a comment •

edited

Loading