Skip to content

Commit

Permalink
Merge #1216: Migrate to bitcoin::FeeRate
Browse files Browse the repository at this point in the history 
475a772 refactor(bdk)!: Remove trait Vbytes (vmammal)
0d64beb chore: organize some imports (vmammal)
89608dd refactor(bdk): display CreateTxError::FeeRateTooLow in sat/vb (vmammal)
09bd86e test(bdk): initialize all feerates from `u64` (vmammal)
004957d refactor(bdk)!: drop FeeRate from bdk::types (vmammal)

Pull request description:

  ### Description

  This follows a similar approach to #1141 namely to remove `FeeRate` from `bdk::types` and instead defer to the upstream implementation for all fee rates. The idea is that making the switch allows BDK to benefit from a higher level abstraction, leaving the implementation details largely hidden.

  As noted in #774, we should avoid extraneous conversions that can result in deviations in estimated transaction size and calculated fee amounts, etc. This would happen for example whenever calling a method like `FeeRate::to_sat_per_vb_ceil`. The only exception I would make is if we must return a fee rate error to the user, we might prefer to display it in the more familiar sats/vb, but this would only be useful if the rate can be expressed as a float.

  ### Notes to the reviewers

  `bitcoin::FeeRate` is an integer whose native unit is sats per kilo-weight unit. In order to facilitate the change, a helper method `feerate_unchecked` is added and used only in wallet tests and psbt tests as necessary to convert existing fee rates to the new type. It's "unchecked" in the sense that we're not checking for integer overflow, because it's assumed we're passing a valid fee rate in a unit test.

  Potential follow-ups can include:
  - [x] Constructing a proper `FeeRate` from a `u64` in all unit tests, and thus obviating the need for the helper `feerate_unchecked` going forward.
  - [x] Remove trait `Vbytes`.
  - Consider adding an extra check that the argument to `TxBuilder::drain_to` is within "standard" size limits.
  - Consider refactoring `coin_selection::select_sorted_utxos` to be efficient and readable.

  closes #1136

  ### Changelog notice

  - Removed `FeeRate` type. All fee rates are now rust-bitcoin [`FeeRate`](https://docs.rs/bitcoin/latest/bitcoin/blockdata/fee_rate/struct.FeeRate.html).
  - Removed trait `Vbytes`.

  ### 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:
  evanlinjin:
    ACK 475a772

Tree-SHA512: 511dab8aa7a65d2b15b160cb4feb96964e8401bb04cda4ef0f0244524bf23a575b3739783a14b90d2dccc984b3f30f5dabfb0a890ffe7c897c2dc23ba301bcaf
  • Loading branch information
@evanlinjin
evanlinjin committed Mar 22, 2024
2 parents fc637a7 + 475a772 commit 6e8a4a8
Show file tree
Hide file tree
Showing 11 changed files with 194 additions and 335 deletions.
5 changes: 3 additions & 2 deletions crates/bdk/src/psbt/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@

//! Additional functions on the `rust-bitcoin` `PartiallySignedTransaction` structure.

use crate::FeeRate;
use alloc::vec::Vec;
use bitcoin::psbt::PartiallySignedTransaction as Psbt;
use bitcoin::Amount;
use bitcoin::FeeRate;
use bitcoin::TxOut;

// TODO upstream the functions here to `rust-bitcoin`?
Expand Down Expand Up @@ -65,7 +66,7 @@ impl PsbtUtils for Psbt {
let fee_amount = self.fee_amount();
fee_amount.map(|fee| {
let weight = self.clone().extract_tx().weight();
FeeRate::from_wu(fee, weight)
Amount::from_sat(fee) / weight
})
}
}
183 changes: 1 addition & 182 deletions crates/bdk/src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@

use alloc::boxed::Box;
use core::convert::AsRef;
use core::ops::Sub;

use bdk_chain::ConfirmationTime;
use bitcoin::blockdata::transaction::{OutPoint, Sequence, TxOut};
use bitcoin::{psbt, Weight};
use bitcoin::psbt;

use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -47,116 +46,6 @@ impl AsRef<[u8]> for KeychainKind {
}
}

/// Fee rate
#[derive(Debug, Copy, Clone, PartialEq, PartialOrd)]
// Internally stored as satoshi/vbyte
pub struct FeeRate(f32);

impl FeeRate {
/// Create a new instance checking the value provided
///
/// ## Panics
///
/// Panics if the value is not [normal](https://doc.rust-lang.org/std/primitive.f32.html#method.is_normal) (except if it's a positive zero) or negative.
fn new_checked(value: f32) -> Self {
assert!(value.is_normal() || value == 0.0);
assert!(value.is_sign_positive());

FeeRate(value)
}

/// Create a new instance of [`FeeRate`] given a float fee rate in sats/kwu
pub fn from_sat_per_kwu(sat_per_kwu: f32) -> Self {
FeeRate::new_checked(sat_per_kwu / 250.0_f32)
}

/// Create a new instance of [`FeeRate`] given a float fee rate in sats/kvb
pub fn from_sat_per_kvb(sat_per_kvb: f32) -> Self {
FeeRate::new_checked(sat_per_kvb / 1000.0_f32)
}

/// Create a new instance of [`FeeRate`] given a float fee rate in btc/kvbytes
///
/// ## Panics
///
/// Panics if the value is not [normal](https://doc.rust-lang.org/std/primitive.f32.html#method.is_normal) (except if it's a positive zero) or negative.
pub fn from_btc_per_kvb(btc_per_kvb: f32) -> Self {
FeeRate::new_checked(btc_per_kvb * 1e5)
}

/// Create a new instance of [`FeeRate`] given a float fee rate in satoshi/vbyte
///
/// ## Panics
///
/// Panics if the value is not [normal](https://doc.rust-lang.org/std/primitive.f32.html#method.is_normal) (except if it's a positive zero) or negative.
pub fn from_sat_per_vb(sat_per_vb: f32) -> Self {
FeeRate::new_checked(sat_per_vb)
}

/// Create a new [`FeeRate`] with the default min relay fee value
pub const fn default_min_relay_fee() -> Self {
FeeRate(1.0)
}

/// Calculate fee rate from `fee` and weight units (`wu`).
pub fn from_wu(fee: u64, wu: Weight) -> FeeRate {
Self::from_vb(fee, wu.to_vbytes_ceil() as usize)
}

/// Calculate fee rate from `fee` and `vbytes`.
pub fn from_vb(fee: u64, vbytes: usize) -> FeeRate {
let rate = fee as f32 / vbytes as f32;
Self::from_sat_per_vb(rate)
}

/// Return the value as satoshi/vbyte
pub fn as_sat_per_vb(&self) -> f32 {
self.0
}

/// Return the value as satoshi/kwu
pub fn sat_per_kwu(&self) -> f32 {
self.0 * 250.0_f32
}

/// Calculate absolute fee in Satoshis using size in weight units.
pub fn fee_wu(&self, wu: Weight) -> u64 {
self.fee_vb(wu.to_vbytes_ceil() as usize)
}

/// Calculate absolute fee in Satoshis using size in virtual bytes.
pub fn fee_vb(&self, vbytes: usize) -> u64 {
(self.as_sat_per_vb() * vbytes as f32).ceil() as u64
}
}

impl Default for FeeRate {
fn default() -> Self {
FeeRate::default_min_relay_fee()
}
}

impl Sub for FeeRate {
type Output = Self;

fn sub(self, other: FeeRate) -> Self::Output {
FeeRate(self.0 - other.0)
}
}

/// Trait implemented by types that can be used to measure weight units.
pub trait Vbytes {
/// Convert weight units to virtual bytes.
fn vbytes(self) -> usize;
}

impl Vbytes for usize {
fn vbytes(self) -> usize {
// ref: https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#transaction-size-calculations
(self as f32 / 4.0).ceil() as usize
}
}

/// An unspent output owned by a [`Wallet`].
///
/// [`Wallet`]: crate::Wallet
Expand Down Expand Up @@ -244,73 +133,3 @@ impl Utxo {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn can_store_feerate_in_const() {
const _MIN_RELAY: FeeRate = FeeRate::default_min_relay_fee();
}

#[test]
#[should_panic]
fn test_invalid_feerate_neg_zero() {
let _ = FeeRate::from_sat_per_vb(-0.0);
}

#[test]
#[should_panic]
fn test_invalid_feerate_neg_value() {
let _ = FeeRate::from_sat_per_vb(-5.0);
}

#[test]
#[should_panic]
fn test_invalid_feerate_nan() {
let _ = FeeRate::from_sat_per_vb(f32::NAN);
}

#[test]
#[should_panic]
fn test_invalid_feerate_inf() {
let _ = FeeRate::from_sat_per_vb(f32::INFINITY);
}

#[test]
fn test_valid_feerate_pos_zero() {
let _ = FeeRate::from_sat_per_vb(0.0);
}

#[test]
fn test_fee_from_btc_per_kvb() {
let fee = FeeRate::from_btc_per_kvb(1e-5);
assert!((fee.as_sat_per_vb() - 1.0).abs() < f32::EPSILON);
}

#[test]
fn test_fee_from_sat_per_vbyte() {
let fee = FeeRate::from_sat_per_vb(1.0);
assert!((fee.as_sat_per_vb() - 1.0).abs() < f32::EPSILON);
}

#[test]
fn test_fee_default_min_relay_fee() {
let fee = FeeRate::default_min_relay_fee();
assert!((fee.as_sat_per_vb() - 1.0).abs() < f32::EPSILON);
}

#[test]
fn test_fee_from_sat_per_kvb() {
let fee = FeeRate::from_sat_per_kvb(1000.0);
assert!((fee.as_sat_per_vb() - 1.0).abs() < f32::EPSILON);
}

#[test]
fn test_fee_from_sat_per_kwu() {
let fee = FeeRate::from_sat_per_kwu(250.0);
assert!((fee.as_sat_per_vb() - 1.0).abs() < f32::EPSILON);
assert_eq!(fee.sat_per_kwu(), 250.0);
}
}

0 comments on commit 6e8a4a8

Please sign in to comment.