diff --git a/CHANGELOG.md b/CHANGELOG.md index 916b581a..9f32fd01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details. - Rejected `--proxy` on the `rpc` backend, and unsupported proxy options (`--proxy_auth`, `--timeout`) on the `rpc` and `cbf` backends, instead of silently ignoring them - Fixed `compile` rejecting policies that are valid for the requested script type - Fixed `warn!` messages, including the mainnet notice, being hidden unless `RUST_LOG` was set; warnings are now shown by default +- Fixed `create_tx`, `create_dns_tx`, `create_sp_tx` and `bump_fee` building transactions over Bitcoin Core's 400,000 wu (100,000 vB) standard weight limit, which no default node relays ## [4.0.0] diff --git a/src/error.rs b/src/error.rs index 685ccdff..c0e22c36 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,6 +1,6 @@ use bdk_wallet::bitcoin::hex::HexToBytesError; use bdk_wallet::bitcoin::psbt::ExtractTxError; -use bdk_wallet::bitcoin::{base64, consensus}; +use bdk_wallet::bitcoin::{Transaction, Weight, base64, consensus}; use thiserror::Error; #[derive(Debug, Error)] @@ -63,6 +63,15 @@ pub enum BDKCliError { #[error("Miniscript compiler error: {0}")] MiniscriptCompilerError(#[from] bdk_wallet::miniscript::policy::compiler::CompilerError), + #[error( + "Transaction can weigh up to {weight:#} ({} vB) once signed, over the {:#} ({} vB) \ + standard limit, so Bitcoin Core nodes will not relay it", + weight.to_vbytes_ceil(), + Transaction::MAX_STANDARD_WEIGHT, + Transaction::MAX_STANDARD_WEIGHT.to_vbytes_ceil() + )] + NonStandardTxWeight { weight: Weight }, + #[error("ParseError: {0}")] ParseError(#[from] bdk_wallet::bitcoin::address::ParseError), diff --git a/src/handlers/dns/mod.rs b/src/handlers/dns/mod.rs index 645672c1..e1c5b8df 100644 --- a/src/handlers/dns/mod.rs +++ b/src/handlers/dns/mod.rs @@ -6,12 +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_fee_rate, parse_outpoint, parse_recipient}; +use crate::utils::{ + check_standard_weight, max_signed_weight, parse_dns_recipient, parse_fee_rate, + parse_op_return_base64, parse_op_return_string, 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 bdk_wallet::bitcoin::{Amount, FeeRate, OutPoint, ScriptBuf, Sequence, script::PushBytesBuf}; use clap::Parser; use std::collections::BTreeMap; @@ -67,16 +67,18 @@ pub struct CreateDnsTxCommand { env = "ADD_STRING", long = "add_string", short = 's', - conflicts_with = "add_data" + conflicts_with = "add_data", + value_parser = parse_op_return_string, )] - pub add_string: Option, + pub add_string: Option, #[arg( env = "ADD_DATA", long = "add_data", short = 'o', - conflicts_with = "add_string" + conflicts_with = "add_string", + value_parser = parse_op_return_base64, )] - pub add_data: Option, + pub add_data: Option, } impl AsyncAppCommand>> for CreateDnsTxCommand { @@ -138,12 +140,8 @@ impl AsyncAppCommand>> for CreateDnsTxCommand { 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)?; - tx_builder.add_data(&PushBytesBuf::try_from(op_return_data)?); - } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec())?; - tx_builder.add_data(&data); + if let Some(data) = self.add_data.as_ref().or(self.add_string.as_ref()) { + tx_builder.add_data(data); } let policies = vec![ @@ -160,6 +158,7 @@ impl AsyncAppCommand>> for CreateDnsTxCommand { } let psbt = tx_builder.finish()?; + check_standard_weight(max_signed_weight(ctx.state.wallet, &psbt)?)?; Ok(PsbtResult::new(&psbt, Some(false))) } } diff --git a/src/handlers/offline.rs b/src/handlers/offline.rs index 15cdd26b..977b8548 100644 --- a/src/handlers/offline.rs +++ b/src/handlers/offline.rs @@ -2,12 +2,14 @@ use crate::commands::OfflineWalletSubCommand; use crate::error::BDKCliError as Error; use crate::handlers::{AppCommand, AppContext, OfflineOperations}; use crate::utils::output::{FormatOutput, ListResult}; -use crate::utils::parse_address; use crate::utils::types::{ AddressResult, BalanceResult, KeychainPair, PsbtResult, RawPsbt, TransactionDetails, UnspentDetails, }; -use crate::utils::{parse_fee_rate, parse_outpoint, parse_recipient}; +use crate::utils::{ + check_standard_weight, max_signed_weight, parse_address, parse_fee_rate, + parse_op_return_base64, parse_op_return_string, parse_outpoint, parse_recipient, +}; use bdk_wallet::bitcoin::base64::Engine; use bdk_wallet::bitcoin::base64::prelude::BASE64_STANDARD; use bdk_wallet::bitcoin::script::PushBytesBuf; @@ -228,23 +230,26 @@ 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. The transaction must stay within Bitcoin Core's 100,000 vB standard size limit. + #[arg( env = "ADD_STRING", long = "add_string", short = 's', - conflicts_with = "add_data" + conflicts_with = "add_data", + value_parser = parse_op_return_string, )] - pub add_string: Option, + 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. The transaction must stay within Bitcoin Core's 100,000 vB standard size limit. #[arg( env = "ADD_DATA", long = "add_data", short = 'o', - conflicts_with = "add_string" + conflicts_with = "add_string", + value_parser = parse_op_return_base64, )] - pub add_data: Option, + pub add_data: Option, } impl AppCommand>> for CreateTxCommand { @@ -293,12 +298,8 @@ impl AppCommand>> for CreateTxCommand { tx_builder.unspendable(unspendable.to_vec()); } - if let Some(base64_data) = &self.add_data { - let op_return_data = BASE64_STANDARD.decode(base64_data)?; - tx_builder.add_data(&PushBytesBuf::try_from(op_return_data)?); - } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec())?; - tx_builder.add_data(&data); + if let Some(data) = self.add_data.as_ref().or(self.add_string.as_ref()) { + tx_builder.add_data(data); } let policies = vec![ @@ -317,6 +318,8 @@ impl AppCommand>> for CreateTxCommand { let psbt = tx_builder.finish()?; + check_standard_weight(max_signed_weight(ctx.state.wallet, &psbt)?)?; + Ok(PsbtResult::new(&psbt, Some(false))) } } @@ -353,22 +356,24 @@ pub struct CreateSpTxCommand { /// 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. The transaction must stay within Bitcoin Core's 100,000 vB standard size limit. #[arg( env = "ADD_STRING", long = "add_string", short = 's', - conflicts_with = "add_data" + conflicts_with = "add_data", + value_parser = parse_op_return_string, )] - pub add_string: Option, - /// Optionally create an OP_RETURN output containing given base64 encoded String. (max 80 bytes) + pub add_string: Option, + /// Optionally create an OP_RETURN output containing given base64 encoded String. The transaction must stay within Bitcoin Core's 100,000 vB standard size limit. #[arg( env = "ADD_DATA", long = "add_data", short = 'o', - conflicts_with = "add_string" + conflicts_with = "add_string", + value_parser = parse_op_return_base64, )] - pub add_data: Option, //base 64 econding + pub add_data: Option, } #[cfg(feature = "silent-payments")] @@ -444,12 +449,8 @@ impl AppCommand>> for CreateSpTxCommand { tx_builder.unspendable(unspendable.to_vec()); } - if let Some(base64_data) = &self.add_data { - let op_return_data = BASE64_STANDARD.decode(base64_data)?; - tx_builder.add_data(&PushBytesBuf::try_from(op_return_data)?); - } else if let Some(string_data) = &self.add_string { - let data = PushBytesBuf::try_from(string_data.as_bytes().to_vec())?; - tx_builder.add_data(&data); + if let Some(data) = self.add_data.as_ref().or(self.add_string.as_ref()) { + tx_builder.add_data(data); } let policies = vec![ @@ -528,6 +529,8 @@ impl AppCommand>> for CreateSpTxCommand { let raw_tx = psbt.extract_tx()?; + check_standard_weight(raw_tx.weight())?; + Ok(RawPsbt::new(&raw_tx)) } } @@ -593,7 +596,7 @@ impl AppCommand>> for BumpFeeCommand { let psbt = tx_builder.finish()?; - // let psbt_base64 = BASE64_STANDARD.encode(psbt.serialize()); + check_standard_weight(max_signed_weight(wallet, &psbt)?)?; Ok(PsbtResult::new(&psbt, Some(false))) } diff --git a/src/utils/common.rs b/src/utils/common.rs index ae86dab8..114420d5 100644 --- a/src/utils/common.rs +++ b/src/utils/common.rs @@ -1,3 +1,4 @@ +use crate::commands::OfflineWalletSubCommand; use crate::{commands::WalletOpts, config::WalletConfig, error::BDKCliError as Error}; #[cfg(feature = "cbf")] use bdk_kyoto::{Info, Receiver, UnboundedReceiver, Warning}; @@ -5,16 +6,20 @@ 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, FeeRate, Network, OutPoint, ScriptBuf}; +use bdk_wallet::Wallet; #[cfg(any( feature = "electrum", feature = "esplora", feature = "cbf", feature = "rpc" ))] -use bdk_wallet::{WalletEvent, bitcoin::Psbt}; - -use crate::commands::OfflineWalletSubCommand; +use bdk_wallet::WalletEvent; +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, FeeRate, Network, OutPoint, Psbt, ScriptBuf, Transaction, Weight, +}; use std::{ path::{Path, PathBuf}, str::FromStr, @@ -135,6 +140,46 @@ pub(crate) fn parse_fee_rate(s: &str) -> Result { Ok(FeeRate::from_sat_per_kwu(sat_kwu as u64)) } +/// Parse an `OP_RETURN` payload given as a UTF-8 string. +pub(crate) fn parse_op_return_string(s: &str) -> Result { + Ok(PushBytesBuf::try_from(s.as_bytes().to_vec())?) +} + +/// Parse an `OP_RETURN` payload given as base64. +pub(crate) fn parse_op_return_base64(s: &str) -> Result { + Ok(PushBytesBuf::try_from(BASE64_STANDARD.decode(s)?)?) +} +/// Reject a transaction over Bitcoin Core's standard weight limit (`tx-size`). +/// +/// This also bounds `OP_RETURN` data: Core derives its default `-datacarriersize` +/// from this limit, so its `datacarrier` check cannot reject a transaction that +/// passes this one. +pub(crate) fn check_standard_weight(weight: Weight) -> Result<(), Error> { + if weight > Transaction::MAX_STANDARD_WEIGHT { + return Err(Error::NonStandardTxWeight { weight }); + } + Ok(()) +} + +/// The most `psbt` can weigh once its inputs are signed. +/// +/// An unsigned transaction can be under the standard limit and over it once +/// signed, so each wallet input counts the largest witness its descriptor can +/// need, as coin selection does. Inputs the wallet does not own count none. +pub(crate) fn max_signed_weight(wallet: &Wallet, psbt: &Psbt) -> Result { + let tx = &psbt.unsigned_tx; + // The unsigned transaction is serialized without the segwit marker and flag + // (2 wu) or the witness count of each input (1 wu). + let mut weight = tx.weight() + Weight::from_wu(2 + tx.input.len() as u64); + for txin in &tx.input { + // Not `get_utxo`: the inputs of a fee bump are already spent. + if let Some(((keychain, _), _)) = wallet.spk_index().txout(txin.previous_output) { + weight += wallet.public_descriptor(keychain).max_weight_to_satisfy()?; + } + } + Ok(weight) +} + /// Prepare bdk-cli home directory /// /// This function is called to check if [`crate::CliOpts`] datadir is set. @@ -569,6 +614,111 @@ mod tests { ); } + #[test] + fn parses_op_return_payloads() { + assert_eq!(parse_op_return_string("hi").unwrap().as_bytes(), b"hi"); + assert_eq!(parse_op_return_base64("aGk=").unwrap().as_bytes(), b"hi"); + assert!(parse_op_return_base64("!!!not-base64!!!").is_err()); + } + + #[test] + fn standard_weight_limit_is_inclusive() { + let max = Transaction::MAX_STANDARD_WEIGHT; + assert!(check_standard_weight(max).is_ok()); + assert!(check_standard_weight(max + Weight::from_wu(1)).is_err()); + } + + mod max_signed_weight_tests { + use super::*; + use bdk_wallet::bitcoin::hashes::Hash; + use bdk_wallet::bitcoin::{Amount, TxIn, TxOut, Txid, absolute, transaction}; + use bdk_wallet::{KeychainKind, SignOptions}; + + const TPRV: &str = "tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L"; + + /// A wallet for `template`, with `KEY` replaced by each keychain's key, + /// holding `utxos` unconfirmed coins. + fn funded_wallet(template: &str, utxos: usize) -> Wallet { + let desc = |keychain: u32| template.replace("KEY", &format!("{TPRV}/{keychain}/*")); + let mut wallet = Wallet::create(desc(0), desc(1)) + .network(Network::Regtest) + .create_wallet_no_persist() + .unwrap(); + let output = (0..utxos) + .map(|_| TxOut { + value: Amount::from_sat(100_000), + script_pubkey: wallet + .reveal_next_address(KeychainKind::External) + .script_pubkey(), + }) + .collect(); + let funding = Transaction { + version: transaction::Version::TWO, + lock_time: absolute::LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint::new(Txid::from_byte_array([1; 32]), 0), + ..Default::default() + }], + output, + }; + wallet.apply_unconfirmed_txs([(funding, 0)]); + wallet + } + + /// Sign `psbt` and check `estimate` bounds the result. + /// + /// It may overshoot by the segwit marker, flag and witness counts a + /// legacy transaction does not have, and per input by the byte or two a + /// signature is shorter than assumed, at 4 wu each in a scriptSig. + fn assert_bounds(wallet: &Wallet, mut psbt: Psbt, estimate: Weight) -> Transaction { + assert!(wallet.sign(&mut psbt, SignOptions::default()).unwrap()); + let tx = psbt.extract_tx().unwrap(); + let slack = Weight::from_wu(2 + 9 * tx.input.len() as u64); + assert!( + tx.weight() <= estimate && estimate - tx.weight() <= slack, + "signed weight {} vs estimate {estimate}", + tx.weight() + ); + tx + } + + #[test] + fn bounds_the_signed_weight_for_each_script_type() { + for template in ["tr(KEY)", "wpkh(KEY)", "sh(wpkh(KEY))", "pkh(KEY)"] { + let mut wallet = funded_wallet(template, 3); + let to = wallet.peek_address(KeychainKind::External, 100); + let mut builder = wallet.build_tx(); + builder.drain_wallet().drain_to(to.script_pubkey()); + let psbt = builder.finish().unwrap(); + + let estimate = max_signed_weight(&wallet, &psbt).unwrap(); + assert_bounds(&wallet, psbt, estimate); + } + } + + #[test] + fn bounds_a_fee_bump_spending_the_same_inputs() { + let mut wallet = funded_wallet("wpkh(KEY)", 2); + let to = wallet.peek_address(KeychainKind::External, 100); + let mut builder = wallet.build_tx(); + builder + .add_recipient(to.script_pubkey(), Amount::from_sat(150_000)) + .fee_rate(FeeRate::from_sat_per_vb(1).unwrap()); + let psbt = builder.finish().unwrap(); + let estimate = max_signed_weight(&wallet, &psbt).unwrap(); + let original = assert_bounds(&wallet, psbt, estimate); + let txid = original.compute_txid(); + wallet.apply_unconfirmed_txs([(original, 1)]); + + // The inputs are now spent, so they are no longer wallet UTXOs. + let mut builder = wallet.build_fee_bump(txid).unwrap(); + builder.fee_rate(FeeRate::from_sat_per_vb(10).unwrap()); + let psbt = builder.finish().unwrap(); + let estimate = max_signed_weight(&wallet, &psbt).unwrap(); + assert_bounds(&wallet, psbt, estimate); + } + } + #[test] fn rejects_fee_rates_that_cannot_be_honoured() { for input in ["0", "-5", "NaN", "inf", "1e30", "abc", ""] { diff --git a/tests/integration/offline.rs b/tests/integration/offline.rs index 417bf72b..c4be0a21 100644 --- a/tests/integration/offline.rs +++ b/tests/integration/offline.rs @@ -148,7 +148,8 @@ mod test_offline { } /// A malformed `create_tx` argument must be reported as an error (exit 1), - /// never as a panic (exit 101). + /// never as a panic (exit 101). an unknown outpoint when building (exit 1), and + /// malformed base64 already at argument parsing (exit 2). #[test] fn test_create_tx_rejects_malformed_input_without_panicking() { let (cli, mut cmd_init) = setup_wallet_config(); @@ -179,7 +180,7 @@ mod test_offline { "!!!not-base64!!!", ]) .assert() - .code(1) + .code(2) .stderr(predicate::str::contains("Base64 decoding error")); } diff --git a/tests/integration/online.rs b/tests/integration/online.rs index 4d3f11af..39721173 100644 --- a/tests/integration/online.rs +++ b/tests/integration/online.rs @@ -552,6 +552,37 @@ mod test_online { ); } + // An OP_RETURN payload is limited only by the transaction it is in having + // to stay within Bitcoin Core's 400_000 wu standard weight. A payload pushing + //it over must fail when building, not at broadcast after signing. + #[test] + fn test_create_tx_op_return_within_standard_weight() { + let (cli, mut cmd_init, env) = setup_online_wallet(); + cmd_init.assert().success(); + fund_and_sync_wallet(&cli, &env); + + let to = format!("{RECIPIENT}:15000"); + + let data = "A".repeat(99_000); + run_wallet_json(&cli, &["create_tx", "--to", &to, "--add_string", &data]); + + let too_large = "A".repeat(99_994); + cli.wallet_cmd(&[ + "--wallet", + WALLET_NAME, + "create_tx", + "--to", + &to, + "--add_string", + &too_large, + ]) + .assert() + .code(1) + .stderr(predicates::str::contains( + "over the 400000 wu (100000 vB) standard limit", + )); + } + // `create_tx --utxos` forces a specific UTXO to be spent; // `--unspendable`excludes one. Funds two UTXOs so the selection is actually observable. #[test]