feat: Add fee rate handling and bump fee functionality #571
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces new abstractions and methods to support fee bumping functionality in a Bitcoin Core RPC client. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (caller)
participant BitcoinClient
participant BitcoinCoreRPC
Test->>BitcoinClient: bump_fee(txid, options)
BitcoinClient->>BitcoinClient: Serialize options (version-aware)
BitcoinClient->>BitcoinCoreRPC: Call "bumpfee" with txid and options
BitcoinCoreRPC-->>BitcoinClient: Return bumpfee result (JSON)
BitcoinClient->>BitcoinClient: Deserialize result into BumpFeeResult
BitcoinClient-->>Test: Return BumpFeeResult
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/utils/src/bitcoin_client.rs (3)
89-118: Consider resolving TODO comment and clarifying the constants.The FeeRate implementation is well-structured with clear unit conversions, but:
- There's an unresolved TODO comment on line 111 which should be addressed before merging:
// TODO: Changed this- The constant
100_000.0into_btc_per_kvbyte()could benefit from a named constant or clearer comments explaining the calculation (10^8 for sat-to-btc divided by 10^3 for vbyte-to-kvbyte)- 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 - } + pub fn to_btc_per_kvbyte(&self) -> f64 { + // Convert from satoshis/vbyte to BTC/kvbyte: + // 1. satoshis → BTC: divide by 10^8 + // 2. vbyte → kvbyte: multiply by 10^3 + // Combined: divide by 10^5 + const SAT_TO_BTC_PER_KVBYTE_DIVISOR: f64 = 100_000.0; // 10^8 / 10^3 + self.0.to_sat() as f64 / SAT_TO_BTC_PER_KVBYTE_DIVISOR + }
149-166: Extract version constant for better code readability.The version check uses a magic number
210000which makes the code less maintainable. Consider extracting this to a named constant with a comment explaining that it refers to Bitcoin Core version 0.21.0.+// Version at which Bitcoin Core switched from BTC/kvB to sat/vB fee rate units +const BITCOIN_CORE_SAT_VB_VERSION: usize = 210000; // Version 0.21.0 impl BumpFeeOptions { pub fn to_serializable(&self, version: usize) -> SerializableBumpFeeOptions { let fee_rate = self.fee_rate.map(|x| { - if version < 210000 { + if version < BITCOIN_CORE_SAT_VB_VERSION { x.to_btc_per_kvbyte() } else { x.to_sat_per_vbyte() } });
132-147: Consider adding validation for overlapping options.The
SerializableBumpFeeOptionsstruct looks good, but you might want to add validation to ensure that users don't provide bothfee_rateandconf_targetsimultaneously, as they could conflict.Consider adding a validation method:
impl SerializableBumpFeeOptions { pub fn validate(&self) -> Result<(), &'static str> { if self.fee_rate.is_some() && self.conf_target.is_some() { return Err("Cannot specify both fee_rate and conf_target"); } Ok(()) } }Then call this in the
to_serializablemethod before returning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/utils/src/bitcoin_client.rs(3 hunks)
🔇 Additional comments (2)
crates/utils/src/bitcoin_client.rs (2)
168-180: LGTM: Well-structured response type.The
BumpFeeResultstruct is well-designed with proper field documentation and appropriate serialization attributes.
17-18: LGTM: Appropriate imports added.The additions of
serdeimports and thejsonmacro are appropriate for the new functionality.
nakul1010
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/utils/src/bitcoin_core.rs (1)
285-299: Generating 100 blocks is unnecessarily slow – a single block is sufficientFor the assertions you make, one block (or even zero) is enough to:
- Evict the original tx from the mempool (
confirmations == -1),- Confirm the replacement tx (
confirmations == 1).Mining 100 blocks adds ±2 s to the test on a fast machine and much more on slower CI runners.
- bitcoin_client.rpc.generate_to_address(100, &to_addr).unwrap(); + bitcoin_client.rpc.generate_to_address(1, &to_addr).unwrap();This speeds up the suite and makes intermittent time‑outs far less likely.
crates/utils/src/bitcoin_client.rs (1)
423-456: Error handling leaks viaprintln!, bypassing tracing and breaks library ergonomicsInside a library call we should never print to stdout:
println!("Failed to deserialize into BumpFeeResult");Replace with the structured logging already in use elsewhere:
- println!("Failed to deserialize into BumpFeeResult"); + error!("Failed to deserialize `bumpfee` result");or propagate a descriptive error:
Err(Error::SerdeError(err))This keeps consumer logs clean and respects the caller’s chosen logging
configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/utils/src/bitcoin_client.rs(3 hunks)crates/utils/src/bitcoin_core.rs(2 hunks)
🔇 Additional comments (1)
crates/utils/src/bitcoin_core.rs (1)
291-294:is_negative()/is_positive()is fragile – assert the exact expected valueDepending on mempool eviction policy and timing there are cases where the
original transaction simply disappears and the RPC returns an error rather
than a negative confirmation count. Prefer explicit pattern matching so the
test fails with a clear message instead of panicking inside the RPC layer.- assert!(tx_info.info.confirmations.is_negative()); + assert_eq!(tx_info.info.confirmations, -1, "original tx should be replaced");Same comment applies to the
is_positive()assertion below.
| #[tokio::test] | ||
| async fn test_bump_fee() -> Result<()> { | ||
| // Step 1: Create and initialize BitcoinCore instance for test | ||
| let bitcoin = BitcoinCore::new().spawn(); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
| /// 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
to_sat_per_vbyte / to_btc_per_kvbyte: misleading comments and loss of precision
- The comment “multiply by the number of decimals to get sat” no longer
matches the implementation – nothing is multiplied. - Returning
f64while 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) inbumpfeeoptions.
- 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.
| 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, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
Taken from rust-bitcoin/rust-bitcoincore-rpc#226 |
Summary by CodeRabbit