Skip to content

feat: Add fee rate handling and bump fee functionality #571

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

Merged
merged 5 commits into from
Apr 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 132 additions & 1 deletion crates/utils/src/bitcoin_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ use bitcoin::{
};
use bitcoincore_rpc::{
bitcoin::{BlockHash, Transaction},
json::TestMempoolAcceptResult,
json::{EstimateMode, TestMempoolAcceptResult},
jsonrpc::{error::RpcError, Error as JsonRpcError},
Auth, Client, Error as BitcoinError, RpcApi,
};
use num_derive::FromPrimitive;
use serde::{Deserialize, Serialize};
use serde_json::error::Category as SerdeJsonCategory;
use std::{sync::Arc, time::Duration};
use tokio::time::{error::Elapsed, sleep, timeout};
Expand Down Expand Up @@ -85,6 +86,101 @@ pub enum BitcoinRpcError {
RpcUnknownError = 0,
}

/// A representation of a fee rate. Bitcoin Core uses different units in different
/// versions. To avoid burdening the user with using the correct unit, this struct
/// provides an umambiguous way to represent the fee rate, and the lib will perform
/// the necessary conversions.
#[derive(Copy, Clone, PartialEq, Eq, Debug, Default)]
pub struct FeeRate(Amount);

impl FeeRate {
/// Construct FeeRate from the amount per vbyte
pub fn per_vbyte(amount_per_vbyte: Amount) -> Self {
// internal representation is amount per vbyte
Self(amount_per_vbyte)
}

/// Construct FeeRate from the amount per kilo-vbyte
pub fn per_kvbyte(amount_per_kvbyte: Amount) -> Self {
// internal representation is amount per vbyte, so divide by 1000
Self::per_vbyte(amount_per_kvbyte / 1000)
}

pub fn to_sat_per_vbyte(&self) -> f64 {
// multiply by the number of decimals to get sat
self.0.to_sat() as f64 // TODO: Changed this
}

pub fn to_btc_per_kvbyte(&self) -> f64 {
// divide by 10^8 to get btc/vbyte, then multiply by 10^3 to get btc/kbyte
self.0.to_sat() as f64 / 100_000.0
}
}
Comment on lines +89 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

to_sat_per_vbyte / to_btc_per_kvbyte: misleading comments and loss of precision

  1. The comment “multiply by the number of decimals to get sat” no longer
    matches the implementation – nothing is multiplied.
  2. Returning f64 while the input is discrete satoshis discards information
    about sub‑sat/vB values before they reach the JSON layer. Bitcoin Core
    accepts sub‑sat values (e.g. 0.75) in bumpfee options.
-    pub fn to_sat_per_vbyte(&self) -> f64 {
-        // multiply by the number of decimals to get sat
-        self.0.to_sat() as f64 // TODO: Changed this
+    pub fn to_sat_per_vbyte(&self) -> f64 {
+        self.0.to_sat_f64()
     }

You can add

trait ToSatF64 {
    fn to_sat_f64(self) -> f64;
}
impl ToSatF64 for Amount {
    fn to_sat_f64(self) -> f64 {
        self.to_sat() as f64 / 1.0 // placeholder – implement once upstream exposes sub‑sat support
    }
}

to preserve eventual sub‑sat support while keeping the public API stable.


#[derive(Clone, PartialEq, Eq, Debug, Default)]
pub struct BumpFeeOptions {
/// Confirmation target in blocks.
pub conf_target: Option<u16>,
/// Specify a fee rate instead of relying on the built-in fee estimator.
pub fee_rate: Option<FeeRate>,
/// Whether this transaction could be replaced due to BIP125 (replace-by-fee)
pub replaceable: Option<bool>,
/// The fee estimate mode
pub estimate_mode: Option<EstimateMode>,
}

#[derive(Serialize, Clone, PartialEq, Debug, Default)]
#[serde(rename_all = "camelCase")]
pub struct SerializableBumpFeeOptions {
#[serde(rename = "conf_target", skip_serializing_if = "Option::is_none")]
/// Confirmation target in blocks.
pub conf_target: Option<u16>,
/// Specify a fee rate instead of relying on the built-in fee estimator.
#[serde(rename = "fee_rate")]
pub fee_rate: Option<f64>,
/// Whether this transaction could be replaced due to BIP125 (replace-by-fee)
#[serde(skip_serializing_if = "Option::is_none")]
pub replaceable: Option<bool>,
/// The fee estimate mode
#[serde(rename = "estimate_mode", skip_serializing_if = "Option::is_none")]
pub estimate_mode: Option<EstimateMode>,
}

impl BumpFeeOptions {
pub fn to_serializable(&self, version: usize) -> SerializableBumpFeeOptions {
let fee_rate = self.fee_rate.map(|x| {
if version < 210000 {
x.to_btc_per_kvbyte()
} else {
x.to_sat_per_vbyte()
}
});

SerializableBumpFeeOptions {
fee_rate,
conf_target: self.conf_target,
replaceable: self.replaceable,
estimate_mode: self.estimate_mode,
}
}
}
Comment on lines +149 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid sending null when no options are provided

bumpfee treats the absence of the second parameter differently from a
null value (the latter triggers a type error in ≥ 25.0). Assemble the
parameter list lazily:

- let args = vec![serde_json::to_value(txid)?, serde_json::to_value(opts)?];
+ let mut args = vec![serde_json::to_value(txid)?];
+ if let Some(o) = opts {
+     args.push(serde_json::to_value(o)?);
+ }

This mirrors the CLI behaviour (bitcoin-cli bumpfee <txid> [options]) and
keeps the code forward‑compatible.

Committable suggestion skipped: line range outside the PR's diff.


#[derive(Deserialize, Clone, Debug)]
#[serde(rename_all = "camelCase")]
pub struct BumpFeeResult {
/// The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private
/// keys are disabled.
pub psbt: Option<String>,
/// The id of the new transaction. Only returned when wallet private keys are enabled.
pub txid: Option<bitcoin::Txid>,
/// The fee of the original transaction (before bumping), denominated in BTC.
pub origfee: f64,
/// The fee of the newly created bumped transaction, denominated in BTC.
pub fee: f64,
/// Errors encountered during processing.
pub errors: Vec<String>,
}

impl From<RpcError> for BitcoinRpcError {
fn from(err: RpcError) -> Self {
match num::FromPrimitive::from_i32(err.code) {
Expand Down Expand Up @@ -323,6 +419,41 @@ impl BitcoinClient {
let transaction = signed_tx.transaction()?;
Ok(transaction)
}

pub fn bump_fee(
&self,
txid: &Txid,
options: Option<&BumpFeeOptions>,
) -> Result<BumpFeeResult, Error> {
// Serialize options if provided
let opts = match options {
Some(options) => Some(options.to_serializable(self.rpc.version()?)),
None => None,
};

// Prepare arguments
let args = vec![serde_json::to_value(txid)?, serde_json::to_value(opts)?];

// Call the "bumpfee" RPC method with borrowed args
let result = self.rpc.call("bumpfee", &args);

// Handle the result
match result {
Ok(result_value) => {
// Try to deserialize the result into the expected BumpFeeResult
let result: Result<BumpFeeResult, _> = serde_json::from_value(result_value);

match result {
Ok(bump_fee_result) => Ok(bump_fee_result),
Err(err) => {
println!("Failed to deserialize into BumpFeeResult");
Err(err.into())
}
}
}
Err(err) => Err(err.into()), // Handle the case where the RPC call fails
}
}
}

fn merklize(left: Sha256dHash, right: Sha256dHash) -> Sha256dHash {
Expand Down
68 changes: 68 additions & 0 deletions crates/utils/src/bitcoin_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ impl BitcoinCore {
#[cfg(test)]
mod tests {
use super::*;
use crate::{BitcoinClient, BumpFeeOptions};
use bitcoin::Amount;

#[test]
fn can_launch_bitcoin() {
Expand All @@ -231,4 +233,70 @@ mod tests {
5000000000
);
}

#[tokio::test]
async fn test_bump_fee() -> Result<()> {
// Step 1: Create and initialize BitcoinCore instance for test
let bitcoin = BitcoinCore::new().spawn();

Comment on lines +237 to +241
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Blocking synchronous RPC calls inside #[tokio::test] can stall the async runtime

bitcoin_client.rpc is a blocking client. Calling it directly from an async test runs the blocking code on a core Tokio worker thread, potentially starving the scheduler (especially when more async tests are run concurrently).

let to_addr = bitcoin_client.rpc.get_new_address(/* … */).unwrap();

Consider off‑loading these calls:

- let to_addr = bitcoin_client.rpc.get_new_address(None, None).unwrap().assume_checked();
+ let to_addr = tokio::task::spawn_blocking(move || {
+     bitcoin_client
+         .rpc
+         .get_new_address(None, None)
+         .map(|a| a.assume_checked())
+ })
+ .await
+ .expect("task panicked")?;

(or wrap the client in tokio::task::block_in_place).
Doing so keeps the async test cooperative and avoids hidden time‑outs on CI.

// Fund Alice's wallet
bitcoin.fund_wallet("Alice").expect("Should fund Alice");

// Fund Bob's wallet
bitcoin.fund_wallet("Bob").expect("Should fund Alice");

// Check that Bob's balance is 5000000000 satoshis (i.e., 5 BTC)
assert_eq!(
bitcoin.client(Some("Bob")).unwrap().get_balance(None, None).unwrap().to_sat(),
5000000000
);

// Initialize BitcoinClient for Alice (make sure Alice's wallet is used)
let bitcoin_client = BitcoinClient::from(bitcoin.client(Some("Alice"))?);

// Step 2: Send to yourself with very low fee (simulate low fee transaction)
let to_addr = bitcoin_client.rpc.get_new_address(None, None).unwrap().assume_checked();

// Set the amount to send (you can adjust the amount as needed)
let amount = Amount::from_sat(100_000); // 0.001 BTC (adjust as necessary)

// Send the transaction to yourself (low fee expected)
let txid = bitcoin_client
.rpc
.send_to_address(&to_addr, amount, None, None, None, Some(true), None, None)
.unwrap();

// Step 3: Bump the fee for the low-fee transaction by calling bump_fee
let bump_fee = bitcoin_client
.bump_fee(
&txid,
Some(&BumpFeeOptions {
conf_target: None,
fee_rate: None,
replaceable: Some(true), // Allow the transaction to be replaceable
estimate_mode: None,
}),
)
.unwrap();

// Assert there are no errors when bumping the fee
assert!(bump_fee.errors.is_empty());

// Step 4: Generate 100 blocks to confirm the bump fee transaction
bitcoin_client.rpc.generate_to_address(100, &to_addr).unwrap();

// Check the original transaction
let tx_info = bitcoin_client.rpc.get_transaction(&txid, None).unwrap();

// Assert that the original transaction has negative confirmations
assert!(tx_info.info.confirmations.is_negative());

// Get the bumped fee transaction's
let tx_info = bitcoin_client.rpc.get_transaction(&bump_fee.txid.unwrap(), None).unwrap();

// Assert that the bumped fee transaction has confirmations
assert!(tx_info.info.confirmations.is_positive());

Ok(())
}
}
Loading