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

Change coin_selection and DescriptorExt::dust_value to use Amount type #1763

Merged

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Dec 8, 2024

Description

refactor(coin_selection)!: use Amount and SignedAmount for API and internally
refactor(chain)!: use Amount for DescriptorExt::dust_value()

Using named types make the API and internal code easier to read and reason about since it makes it clear that the values are bitcoin amounts. Also to create these types the units (ie .from_sat()) must be specified.

Notes to the reviewers

For coin_selection using Amount and SignedAmount makes internal code safer against overflow errors. In particular because these types will panic if an amount overflow occurs. Using u64/i64 on the other hand can silently rollover. See: https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow

This is a continuation of the work done in #1595. Since this is an API breaking change I would like to include it in the 1.0.0-beta milestone if possible.

Changelog notice

  • Change coin_selection to use Amount instead of u64 for all bitcoin amounts.
  • Change DescriptorExt::dust_value() to return an Amount instead of u64.

Checklists

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory self-assigned this Dec 8, 2024
@notmandatory notmandatory added the api A breaking API change label Dec 8, 2024
@notmandatory notmandatory changed the title refactor(coin_selection)!: use Amount and SignedAmount for API and internally Change coin_selection and DescriptorExt::dust_value to not use u64 for bitcoin amounts Dec 8, 2024
@notmandatory notmandatory changed the title Change coin_selection and DescriptorExt::dust_value to not use u64 for bitcoin amounts Change coin_selection and DescriptorExt::dust_value to use Amount type Dec 8, 2024
@notmandatory notmandatory force-pushed the refactor/coin_selection_amounts branch 2 times, most recently from 3504440 to 5a0e19d Compare December 8, 2024 03:06
@notmandatory
Copy link
Member Author

notmandatory commented Dec 8, 2024

Thanks to @thesimplekid for bringing to my attention that native rust integer types can silently roll-over. Even though this is very unlikely to happen, using Amount/SignedAmount in coin_selection is a good way to make sure it never happens.

The rest of the code base is already updated to use Amount in APIs and internally.

@notmandatory notmandatory added this to the 1.0.0-beta milestone Dec 8, 2024
crates/wallet/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/coin_selection.rs Show resolved Hide resolved
crates/wallet/src/wallet/coin_selection.rs Show resolved Hide resolved
crates/wallet/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory
Copy link
Member Author

Just as an FYI, I did a little test to confirm that rust integer underflow does wrap and isn't caught as a static check if the values aren't known at compile time. This only applies when built in release mode, in debug mode it panics.

#[test]
#[cfg(not(debug_assertions))]
fn test_integer_underflow() {
    use rand::Rng;
    let num = rand::thread_rng().gen_range(11..21);
    let ten: u64 = 10;
    let underflow = ten - num;
    // println!("10 - {} => underflow {}", num, underflow);
    assert!(underflow > 0)
}

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK d2f49a7

@notmandatory notmandatory force-pushed the refactor/coin_selection_amounts branch from d2f49a7 to 7f1e4d2 Compare December 10, 2024 19:59
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 7f1e4d2

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 7f1e4d2

@notmandatory notmandatory force-pushed the refactor/coin_selection_amounts branch from 7f1e4d2 to 2198e85 Compare December 11, 2024 20:43
…ternally

Using named types make the API and internal code easier to read and reason
about since it makes it clear that the values are bitcoin amounts. Also to
create these types the units (ie .from_sat()) must be specified.

Using Amount and SignedAmount also makes internal code safer against overflow
errors. In particular because these types will panic if an amount overflow
occurs. Using u64/i64 on the otherhand can silently rollover. See:
https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow
@notmandatory notmandatory force-pushed the refactor/coin_selection_amounts branch from f1ed237 to eeb5372 Compare December 11, 2024 21:04
@notmandatory notmandatory force-pushed the refactor/coin_selection_amounts branch from eeb5372 to 2a9eeed Compare December 11, 2024 21:11
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 2a9eeed

@notmandatory notmandatory merged commit 49d6477 into bitcoindevkit:master Dec 11, 2024
21 checks passed
@notmandatory notmandatory mentioned this pull request Dec 11, 2024
32 tasks
notmandatory added a commit that referenced this pull request Dec 23, 2024
…te_tx

a2f7a8b refactor(wallet): cleanup and remove unused code in create_tx (Steve Myers)

Pull request description:

  ### Description

  Cleanup and remove unused code in `Wallet::create_tx`, this was noticed during review of #1763. See: #1763 (comment)

  fixes #1710

  ### Notes to the reviewers

  In addition to removing the unneeded assignments to `fee_amount` and `received` I also refactored creation of the change output to be an `if let` instead of `match` statement since it only needs to do something if there is `Excess::Change`.

  I should have done this cleanup as part of #1048.

  ### Changelog notice

  None, only internal cleanup.

  ### 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:
  ValuedMammal:
    reACK a2f7a8b

Tree-SHA512: 64e5895ff3dc11f71c48b6c436d5c812504d0a24e92f1fdf451936f372d95ccdd8d89e5ac634a041bdee0a4836182f05127864ed744d560c9f8ec560e092c559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants