diff --git a/CHANGELOG.md b/CHANGELOG.md index 098eee19..d09cc57a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details. ## [Unreleased] - Added support for Multipath (two-paths) descriptors. +- Fixed `create_tx` and `bump_fee` panicking on malformed `--utxos` and `--add_data` values instead of returning an error +- Fixed `--fee_rate` silently truncating to a whole sat/vB, falling back to a default, or producing a zero-fee transaction, unusable values are now rejected +- Enforced a size limit on `--add_data` and `--add_string` OP_RETURN payloads, matching Bitcoin Core v30's default `-datacarriersize` of 100000 bytes ## [4.0.0] diff --git a/src/error.rs b/src/error.rs index bfad510a..9ceb571a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -5,6 +5,9 @@ use thiserror::Error; #[derive(Debug, Error)] pub enum BDKCliError { + #[error("Add UTXO error: {0}")] + AddUtxoError(#[from] bdk_wallet::tx_builder::AddUtxoError), + #[error("Cannot provide both a multipath descriptor and a separate internal descriptor.")] AmbiguousDescriptors, diff --git a/src/handlers/dns/mod.rs b/src/handlers/dns/mod.rs index 628bfbdb..884f8584 100644 --- a/src/handlers/dns/mod.rs +++ b/src/handlers/dns/mod.rs @@ -6,11 +6,12 @@ use crate::handlers::dns::dns_payment_instructions::{ }; use crate::handlers::{AppContext, AsyncAppCommand, Init, OfflineOperations}; use crate::utils::types::{PsbtResult, StatusResult}; -use crate::utils::{parse_dns_recipient, parse_outpoint, parse_recipient}; +use crate::utils::{ + parse_dns_recipient, parse_fee_rate, parse_op_return_data, parse_outpoint, parse_recipient, +}; use bdk_wallet::KeychainKind; use bdk_wallet::bitcoin::base64::Engine; use bdk_wallet::bitcoin::base64::prelude::BASE64_STANDARD; -use bdk_wallet::bitcoin::script::PushBytesBuf; use bdk_wallet::bitcoin::{Amount, FeeRate, OutPoint, ScriptBuf, Sequence}; use clap::Parser; use std::collections::BTreeMap; @@ -57,8 +58,8 @@ pub struct CreateDnsTxCommand { pub utxos: Option>, #[arg(env = "CANT_SPEND_TXID:VOUT", long = "unspendable", value_parser = parse_outpoint)] pub unspendable: Option>, - #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate")] - pub fee_rate: Option, + #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate", value_parser = parse_fee_rate)] + pub fee_rate: Option, #[arg(env = "EXT_POLICY", long = "external_policy")] pub external_policy: Option, #[arg(env = "INT_POLICY", long = "internal_policy")] @@ -129,30 +130,20 @@ impl AsyncAppCommand>> for CreateDnsTxCommand { if self.offline_signer { tx_builder.add_global_xpubs(); } - if let Some(fee_rate) = self.fee_rate - && let Some(fee_rate) = FeeRate::from_sat_per_vb(fee_rate as u64) - { + if let Some(fee_rate) = self.fee_rate { tx_builder.fee_rate(fee_rate); } if let Some(utxos) = &self.utxos { - tx_builder - .add_utxos(&utxos[..]) - .map_err(|_| bdk_wallet::error::CreateTxError::UnknownUtxo)?; + tx_builder.add_utxos(&utxos[..])?; } if let Some(unspendable) = &self.unspendable { tx_builder.unspendable(unspendable.to_vec()); } if let Some(base64_data) = &self.add_data { - let op_return_data = BASE64_STANDARD - .decode(base64_data) - .map_err(|e| Error::Generic(e.to_string()))?; - tx_builder.add_data( - &PushBytesBuf::try_from(op_return_data) - .map_err(|e| Error::Generic(e.to_string()))?, - ); + let op_return_data = BASE64_STANDARD.decode(base64_data)?; + tx_builder.add_data(&parse_op_return_data(op_return_data)?); } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec()) - .map_err(|e| Error::Generic(e.to_string()))?; + let data = parse_op_return_data(string_data.as_bytes().to_vec())?; tx_builder.add_data(&data); } diff --git a/src/handlers/offline.rs b/src/handlers/offline.rs index fe4097e4..1762da32 100644 --- a/src/handlers/offline.rs +++ b/src/handlers/offline.rs @@ -7,10 +7,9 @@ use crate::utils::types::{ AddressResult, BalanceResult, KeychainPair, PsbtResult, RawPsbt, TransactionDetails, UnspentDetails, }; -use crate::utils::{parse_outpoint, parse_recipient}; +use crate::utils::{parse_fee_rate, parse_op_return_data, parse_outpoint, parse_recipient}; use bdk_wallet::bitcoin::base64::Engine; use bdk_wallet::bitcoin::base64::prelude::BASE64_STANDARD; -use bdk_wallet::bitcoin::script::PushBytesBuf; use bdk_wallet::bitcoin::{Address, Amount, FeeRate, OutPoint, Psbt, ScriptBuf, Sequence, Txid}; use bdk_wallet::{KeychainKind, SignOptions}; use clap::Parser; @@ -217,8 +216,8 @@ pub struct CreateTxCommand { pub unspendable: Option>, /// Fee rate to use in sat/vbyte. - #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate")] - pub fee_rate: Option, + #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate", value_parser = parse_fee_rate)] + pub fee_rate: Option, /// Selects which policy should be used to satisfy the external descriptor. #[arg(env = "EXT_POLICY", long = "external_policy")] @@ -228,7 +227,7 @@ pub struct CreateTxCommand { #[arg(env = "INT_POLICY", long = "internal_policy")] pub internal_policy: Option, - /// Optionally create an OP_RETURN output containing given String in utf8 encoding (max 80 bytes) + /// Optionally create an OP_RETURN output containing given String in utf8 encoding (max 99_994 bytes) #[arg( env = "ADD_STRING", long = "add_string", @@ -237,7 +236,7 @@ pub struct CreateTxCommand { )] pub add_string: Option, - /// Optionally create an OP_RETURN output containing given base64 encoded String. (max 80 bytes) + /// Optionally create an OP_RETURN output containing given base64 encoded String. (max 99_994 bytes) #[arg( env = "ADD_DATA", long = "add_data", @@ -281,14 +280,12 @@ impl AppCommand>> for CreateTxCommand { tx_builder.add_global_xpubs(); } - if let Some(fee_rate) = self.fee_rate - && let Some(fee_rate) = FeeRate::from_sat_per_vb(fee_rate as u64) - { + if let Some(fee_rate) = self.fee_rate { tx_builder.fee_rate(fee_rate); } if let Some(utxos) = &self.utxos { - tx_builder.add_utxos(&utxos[..]).unwrap(); + tx_builder.add_utxos(&utxos[..])?; } if let Some(unspendable) = &self.unspendable { @@ -296,10 +293,10 @@ impl AppCommand>> for CreateTxCommand { } if let Some(base64_data) = &self.add_data { - let op_return_data = BASE64_STANDARD.decode(base64_data).unwrap(); - tx_builder.add_data(&PushBytesBuf::try_from(op_return_data).unwrap()); + let op_return_data = BASE64_STANDARD.decode(base64_data)?; + tx_builder.add_data(&parse_op_return_data(op_return_data)?); } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec()).unwrap(); + let data = parse_op_return_data(string_data.as_bytes().to_vec())?; tx_builder.add_data(&data); } @@ -319,8 +316,6 @@ impl AppCommand>> for CreateTxCommand { let psbt = tx_builder.finish()?; - // let psbt_base64 = BASE64_STANDARD.encode(psbt.serialize()); - Ok(PsbtResult::new(&psbt, Some(false))) } } @@ -349,15 +344,15 @@ pub struct CreateSpTxCommand { #[arg(env = "CANT_SPEND_TXID:VOUT", long = "unspendable", value_parser = parse_outpoint)] pub unspendable: Option>, /// Fee rate to use in sat/vbyte. - #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate")] - pub fee_rate: Option, + #[arg(env = "SATS_VBYTE", short = 'f', long = "fee_rate", value_parser = parse_fee_rate)] + pub fee_rate: Option, /// Selects which policy should be used to satisfy the external descriptor. #[arg(env = "EXT_POLICY", long = "external_policy")] pub external_policy: Option, /// Selects which policy should be used to satisfy the internal descriptor. #[arg(env = "INT_POLICY", long = "internal_policy")] pub internal_policy: Option, - /// Optionally create an OP_RETURN output containing given String in utf8 encoding (max 80 bytes) + /// Optionally create an OP_RETURN output containing given String in utf8 encoding (max 99_994 bytes) #[arg( env = "ADD_STRING", long = "add_string", @@ -365,7 +360,7 @@ pub struct CreateSpTxCommand { conflicts_with = "add_data" )] pub add_string: Option, - /// Optionally create an OP_RETURN output containing given base64 encoded String. (max 80 bytes) + /// Optionally create an OP_RETURN output containing given base64 encoded String. (max 99_994 bytes) #[arg( env = "ADD_DATA", long = "add_data", @@ -436,16 +431,12 @@ impl AppCommand>> for CreateSpTxCommand { tx_builder.add_global_xpubs(); } - if let Some(fee_rate) = self.fee_rate - && let Some(fee_rate) = FeeRate::from_sat_per_vb(fee_rate as u64) - { + if let Some(fee_rate) = self.fee_rate { tx_builder.fee_rate(fee_rate); } if let Some(utxos) = &self.utxos { - tx_builder - .add_utxos(&utxos[..]) - .map_err(|_| bdk_wallet::error::CreateTxError::UnknownUtxo)?; + tx_builder.add_utxos(&utxos[..])?; } if let Some(unspendable) = &self.unspendable { @@ -453,16 +444,10 @@ impl AppCommand>> for CreateSpTxCommand { } if let Some(base64_data) = &self.add_data { - let op_return_data = BASE64_STANDARD - .decode(base64_data) - .map_err(|e| Error::Generic(e.to_string()))?; - tx_builder.add_data( - &PushBytesBuf::try_from(op_return_data) - .map_err(|e| Error::Generic(e.to_string()))?, - ); + let op_return_data = BASE64_STANDARD.decode(base64_data)?; + tx_builder.add_data(&parse_op_return_data(op_return_data)?); } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec()) - .map_err(|e| Error::Generic(e.to_string()))?; + let data = parse_op_return_data(string_data.as_bytes().to_vec())?; tx_builder.add_data(&data); } @@ -573,9 +558,10 @@ pub struct BumpFeeCommand { env = "SATS_VBYTE", short = 'f', long = "fee_rate", - default_value = "1.0" + default_value = "1.0", + value_parser = parse_fee_rate )] - pub fee_rate: f32, + pub fee_rate: FeeRate, } impl AppCommand>> for BumpFeeCommand { @@ -585,9 +571,7 @@ impl AppCommand>> for BumpFeeCommand { let wallet = &mut ctx.state.wallet; let mut tx_builder = wallet.build_fee_bump(self.txid)?; - let fee_rate = - FeeRate::from_sat_per_vb(self.fee_rate as u64).unwrap_or(FeeRate::BROADCAST_MIN); - tx_builder.fee_rate(fee_rate); + tx_builder.fee_rate(self.fee_rate); if let Some(address) = &self.shrink_address { let script_pubkey = address.script_pubkey(); @@ -599,7 +583,7 @@ impl AppCommand>> for BumpFeeCommand { } if let Some(utxos) = &self.utxos { - tx_builder.add_utxos(&utxos[..]).unwrap(); + tx_builder.add_utxos(&utxos[..])?; } if let Some(unspendable) = &self.unspendable { diff --git a/src/handlers/online.rs b/src/handlers/online.rs index f2900cbc..25baf889 100644 --- a/src/handlers/online.rs +++ b/src/handlers/online.rs @@ -1,9 +1,10 @@ -use clap::Parser; - #[cfg(feature = "electrum")] use crate::client::BlockchainClient::Electrum; #[cfg(feature = "cbf")] use crate::client::{BlockchainClient::KyotoClient, sync_kyoto_client}; +use crate::utils::parse_fee_rate; +use bdk_wallet::bitcoin::FeeRate; +use clap::Parser; #[cfg(feature = "esplora")] use {crate::client::BlockchainClient::Esplora, bdk_esplora::EsploraAsyncExt}; #[cfg(feature = "rpc")] @@ -438,9 +439,10 @@ pub struct SendPayjoinCommand { env = "PAYJOIN_SENDER_FEE_RATE", short = 'f', long = "fee_rate", - required = true + required = true, + value_parser = parse_fee_rate )] - fee_rate: u64, + fee_rate: FeeRate, } #[cfg(any( feature = "electrum", diff --git a/src/handlers/payjoin/mod.rs b/src/handlers/payjoin/mod.rs index 7a2fe36a..c78fa0af 100644 --- a/src/handlers/payjoin/mod.rs +++ b/src/handlers/payjoin/mod.rs @@ -172,7 +172,7 @@ impl<'a> PayjoinManager<'a> { pub async fn send_payjoin( &mut self, uri: String, - fee_rate: u64, + fee_rate: FeeRate, ohttp_relays: Vec, blockchain_client: &BlockchainClient, ) -> Result { @@ -189,8 +189,6 @@ impl<'a> PayjoinManager<'a> { .amount .ok_or_else(|| Error::Generic("Amount is not specified in the URI.".to_string()))?; - let fee_rate = FeeRate::from_sat_per_vb(fee_rate).expect("Provided fee rate is not valid."); - // Build and sign the original PSBT which pays to the receiver. let mut original_psbt = { let mut tx_builder = self.wallet.build_tx(); diff --git a/src/utils/common.rs b/src/utils/common.rs index 136c9873..cd4dbaf4 100644 --- a/src/utils/common.rs +++ b/src/utils/common.rs @@ -5,7 +5,7 @@ use bdk_kyoto::{Info, Receiver, UnboundedReceiver, Warning}; use bdk_message_signer::SignatureFormat; #[cfg(feature = "silent-payments")] use bdk_sp::encoding::SilentPaymentCode; -use bdk_wallet::bitcoin::{Address, Network, OutPoint, ScriptBuf}; +use bdk_wallet::bitcoin::{Address, FeeRate, Network, OutPoint, ScriptBuf, script::PushBytesBuf}; #[cfg(any( feature = "electrum", feature = "esplora", @@ -20,6 +20,26 @@ use std::{ str::FromStr, }; +/// Maximum `OP_RETURN` `scriptPubKey` size a default Bitcoin Core node relays. +/// +/// `-datacarriersize` limits the whole `scriptPubKey`, not just the payload. Core +/// v30 raised its default to 100000, from the 83 that allowed an 80 byte payload. +const MAX_OP_RETURN_SCRIPT_BYTES: usize = 100_000; + +/// What the script spends on `OP_RETURN` plus the push prefix: any payload large +/// enough to reach the limit is over 65535 bytes, so it always uses +/// `OP_PUSHDATA4` and always costs 1 + 1 + 4 bytes. +const OP_RETURN_OVERHEAD_BYTES: usize = 6; + +/// Maximum number of data bytes such an output may carry. +const MAX_OP_RETURN_BYTES: usize = MAX_OP_RETURN_SCRIPT_BYTES - OP_RETURN_OVERHEAD_BYTES; + +/// 1 vB is 1/4 kwu, so 1 sat/vB is 250 sat/kwu. +const SAT_PER_KWU_PER_SAT_PER_VB: f64 = 250.0; + +/// Smallest fee rate `FeeRate` can represent, in sat/vB. +const MIN_SAT_PER_VB: f64 = 1.0 / SAT_PER_KWU_PER_SAT_PER_VB; + /// Determine if PSBT has final script sigs or witnesses for all unsigned tx inputs. #[cfg(any( feature = "electrum", @@ -78,7 +98,7 @@ pub(crate) fn parse_proxy_auth(s: &str) -> Result<(String, String), Error> { Ok((user, passwd)) } -/// Parse a outpoint (Txid:Vout) argument from cli input. +/// Parse a outpoint (Txid:Vout) argument from input. pub(crate) fn parse_outpoint(s: &str) -> Result { Ok(OutPoint::from_str(s)?) } @@ -89,6 +109,52 @@ pub(crate) fn parse_address(address_str: &str) -> Result { Ok(unchecked_address.assume_checked()) } +/// Parse a fee rate, given in sat/vB, from input. +/// +/// [`FeeRate`] counts sat/kwu, so fractional rates are kept at 1/250 sat/vB +/// precision rather than being truncated to a whole sat/vB. A rate that cannot be +/// represented is rejected instead of silently becoming zero or a default. +pub(crate) fn parse_fee_rate(s: &str) -> Result { + let sat_vb = f64::from_str(s.trim()).map_err(|_| { + Error::Generic(format!( + "Invalid fee rate '{s}', expected a number in sat/vB" + )) + })?; + + if !sat_vb.is_finite() { + return Err(Error::Generic(format!( + "Invalid fee rate '{s}', must be a finite number of sat/vB" + ))); + } + if sat_vb < MIN_SAT_PER_VB { + return Err(Error::Generic(format!( + "Fee rate '{s}' sat/vB is below the smallest usable rate of {MIN_SAT_PER_VB} sat/vB" + ))); + } + + let sat_kwu = (sat_vb * SAT_PER_KWU_PER_SAT_PER_VB).round(); + if sat_kwu >= u64::MAX as f64 { + return Err(Error::Generic(format!( + "Fee rate '{s}' sat/vB is too large to represent" + ))); + } + + Ok(FeeRate::from_sat_per_kwu(sat_kwu as u64)) +} + +/// Build the `OP_RETURN` payload for the `--add_data`/`--add_string` arguments, +/// enforcing the 80 byte limit both of them document. +pub(crate) fn parse_op_return_data(data: Vec) -> Result { + if data.len() > MAX_OP_RETURN_BYTES { + return Err(Error::Generic(format!( + "OP_RETURN data is {} bytes, the maximum is {MAX_OP_RETURN_BYTES}", + data.len() + ))); + } + + PushBytesBuf::try_from(data).map_err(|e| Error::Generic(e.to_string())) +} + /// Prepare bdk-cli home directory /// /// This function is called to check if [`crate::CliOpts`] datadir is set. @@ -315,3 +381,54 @@ pub(crate) fn parse_dns_recipient(s: &str) -> Result<(String, u64), String> { let sending_amount = u64::from_str(parts[1]).map_err(|e| e.to_string())?; Ok((parts[0].to_string(), sending_amount)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_whole_and_fractional_fee_rates() { + let one_sat_vb = FeeRate::from_sat_per_vb(1).unwrap(); + assert_eq!(parse_fee_rate("1").unwrap(), one_sat_vb); + assert_eq!(parse_fee_rate("1.0").unwrap(), one_sat_vb); + assert_eq!(parse_fee_rate(" 1 ").unwrap(), one_sat_vb); + assert_eq!( + parse_fee_rate("2.7").unwrap(), + FeeRate::from_sat_per_kwu(675) + ); + assert_eq!( + parse_fee_rate("0.004").unwrap(), + FeeRate::from_sat_per_kwu(1) + ); + assert_eq!( + parse_fee_rate("0.9").unwrap(), + FeeRate::from_sat_per_kwu(225) + ); + } + + #[test] + fn rejects_fee_rates_that_cannot_be_honoured() { + for input in ["0", "-5", "NaN", "inf", "1e30", "abc", ""] { + assert!( + parse_fee_rate(input).is_err(), + "fee rate '{input}' should be rejected" + ); + } + } + + #[test] + fn op_return_data_is_capped_at_the_documented_limit() { + assert!(parse_op_return_data(vec![0u8; MAX_OP_RETURN_BYTES]).is_ok()); + assert!(parse_op_return_data(vec![0u8; MAX_OP_RETURN_BYTES + 1]).is_err()); + } + + #[test] + fn the_largest_allowed_payload_fits_the_script_limit() { + let data = vec![0u8; MAX_OP_RETURN_BYTES]; + let push_bytes = parse_op_return_data(data).unwrap(); + assert_eq!( + ScriptBuf::new_op_return(&push_bytes).len(), + MAX_OP_RETURN_SCRIPT_BYTES + ); + } +} diff --git a/tests/integration/offline.rs b/tests/integration/offline.rs index 27b6bd16..3ca83d3e 100644 --- a/tests/integration/offline.rs +++ b/tests/integration/offline.rs @@ -7,6 +7,8 @@ mod test_offline { use tempfile::TempDir; static WALLET_NAME: &str = "test_config_wallet"; + /// A `--to` argument, the wallet is unfunded, so nothing is spent. + static RECIPIENT: &str = "tb1p4tp4l6glyr2gs94neqcpr5gha7344nfyznfkc8szkreflscsdkgqsdent4:10000"; /// Helper to spin up a sandboxed CLI with the generated descriptors fn setup_wallet_config() -> (BdkCli, Command) { @@ -151,6 +153,85 @@ mod test_offline { .stderr(predicate::str::contains("Invalid")); } + /// A malformed `create_tx` argument must be reported as an error (exit 1), + /// never as a panic (exit 101). + #[test] + fn test_create_tx_rejects_malformed_input_without_panicking() { + let (cli, mut cmd_init) = setup_wallet_config(); + cmd_init.assert().success(); + + // Unknown outpoint + cli.wallet_cmd(&[ + "--wallet", + WALLET_NAME, + "create_tx", + "--to", + RECIPIENT, + "--utxos", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("UTXO not found")); + + // Malformed base64 + cli.wallet_cmd(&[ + "--wallet", + WALLET_NAME, + "create_tx", + "--to", + RECIPIENT, + "--add_data", + "!!!not-base64!!!", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("Base64 decoding error")); + + // Over-long OP_RETURN payload. + let too_long = "A".repeat(99_995); + cli.wallet_cmd(&[ + "--wallet", + WALLET_NAME, + "create_tx", + "--to", + RECIPIENT, + "--add_string", + &too_long, + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("OP_RETURN data is 99995 bytes")); + } + + /// A `--fee_rate` that cannot be honoured is rejected up front instead of + /// silently becoming a zero fee or the builder default. + #[test] + fn test_create_tx_rejects_unusable_fee_rates() { + let (cli, mut cmd_init) = setup_wallet_config(); + cmd_init.assert().success(); + + for (fee_rate, expected) in [ + ("0", "below the smallest usable rate"), + ("NaN", "must be a finite number"), + ("1e30", "too large to represent"), + ("abc", "expected a number in sat/vB"), + ] { + cli.wallet_cmd(&[ + "--wallet", + WALLET_NAME, + "create_tx", + "--to", + RECIPIENT, + "--fee_rate", + fee_rate, + ]) + .assert() + .code(2) + .stderr(predicate::str::contains(expected)); + } + } + #[cfg(feature = "message_signer")] #[test] fn test_sign_message_and_verify_message() {