fix transaction creation input validation and fee rate calculation

[tvpeter]

Description

This PR addresses input-validation problems on the transaction-building commands (create_tx, create_sp_tx, bump_fee), transaction fee rate and OP_RETURN data size:

• Panics on malformed input: create_tx and bump_fee called .unwrap() on Results so they panic (exit 101) instead of an error (exit 1).
• The create_sp_tx already guarded these paths, but improvements were made to the error type been returned
• Fee rates: --fee_rate was an f32 cast with as u64, which truncates and saturates, and a None from from_sat_per_vb was silently skipped. Parsing now happens in a value_parser, so bad values are rejected with
  a usage message before a wallet is loaded. Because FeeRate counts sat/kwu, fractional rates keep 1/250 sat/vB precision instead of being truncated .
• OP_RETURN size: Both --add_data and --add_string document "max 80 bytes" and neither enforced it. This has now been updated to 100_000 bytes and enforced in transaction building.
• create_dns_tx was had the same fee-rate bug and the same OP_RETURN handling, and has been fixed too.
• bump_fee --utxos and send_payjoin -f are fixed by the same changes.

Fixes #325

Notes to the reviewers

Changelog notice

• 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 the documented 100_000 byte limit on --add_data and --add_string OP_RETURN payloads

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

❌ Patch coverage is 91.33858% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.79%. Comparing base (b2b99b4) to head (1282553).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers/offline.rs	50.00%	6 Missing ⚠️
src/handlers/dns/mod.rs	20.00%	4 Missing ⚠️
src/handlers/payjoin/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
+ Coverage   59.06%   59.79%   +0.73%     
==========================================
  Files          22       22              
  Lines        3857     3878      +21     
==========================================
+ Hits         2278     2319      +41     
+ Misses       1579     1559      -20     

Flag	Coverage Δ
rust	59.79% <91.33%> (+0.73%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

MAX_OP_RETURN_BYTES (99_994) counts the OP_RETURN scriptPubKey only, so create_tx builds transactions over the 100_000 vB standard tx size limit, which no default-policy node relays. The output alone is 100_013 vB (8 value + 5 length prefix + 100_000 script), before any input, change or header.

A repro, not a test I'm suggesting you merge:

/// `--add_string` accepts a payload of up to `MAX_OP_RETURN_BYTES` (99_994),
/// but the resulting transaction is over the 100_000 vB standardness limit,
/// so no default-policy node relays it.
#[test]
fn test_max_op_return_payload_fits_the_standard_tx_size() {
    let (cli, mut cmd_init, env) = setup_online_wallet();
    cmd_init.assert().success();
    fund_and_sync_wallet(&cli, &env);

    let data = "A".repeat(99_994);
    let to = format!("{RECIPIENT}:15000");
    let args = ["create_tx", "--to", &to, "--add_string", &data];
    let psbt = run_wallet_json(&cli, &args)["psbt"].as_str().unwrap().to_owned();

    // Signing it would mean passing ~200 KB of base64 as an argument, and the
    // unsigned tx is enough here: the signed one is only bigger.
    let tx = bdk_wallet::bitcoin::Psbt::from_str(&psbt).unwrap().unsigned_tx;
    let result = env.rpc_client().test_mempool_accept(&[&tx]).unwrap();

    let reason = result[0].reject_reason.as_deref();
    assert!(
        reason != Some("tx-size"),
        "node rejected the {} vB tx built at the documented limit: {}",
        tx.vsize(),
        reason.unwrap_or("accepted")
    );
}

$ cargo test --all-features --test cli test_max_op_return

test ...::test_max_op_return_payload_fits_the_standard_tx_size ... FAILED

node rejected the 100150 vB tx built at the documented limit: tx-size

That tx is 150 vB over the limit with one input, a recipient and change, so the usable payload is at most ~99_844 here, less with more inputs. the_largest_allowed_payload_fits_the_script_limit locks in the same unusable size: a 100_000 byte script never fits a standard tx.

Non-blocking: master enforced no limit here at all, so this is an improvement either way.

[notmandatory]

MAX_OP_RETURN_BYTES (99_994) counts the OP_RETURN scriptPubKey only, so create_tx builds transactions over the 100_000 vB standard tx size limit, which no default-policy node relays. The output alone is 100_013 vB (8 value + 5 length prefix + 100_000 script), before any input, change or header.

Is this something that needs to be checked and enforced in the bdk_tx crate?

[vadim-anfv]

Is this something that needs to be checked and enforced in the bdk_tx crate?

bdk-cli doesn't use bdk_tx, transactions are built with TxBuilder from bdk_wallet.

Is the plan to move transaction building to bdk_tx, in bdk-cli in particular?
If so, the tx size check belongs there.

[notmandatory]

Is the plan to move transaction building to bdk_tx, in bdk-cli in particular? If so, the tx size check belongs there.

The plan is to transition bdk_wallet to depreciate TxBuilder and use bdk_tx via Wallet::create_psbt (see bitcoindevkit/bdk_wallet#516). So I'd suggest adding the check to bdk-tx and updating bdk-cli to enable the Wallet::create_psbt experimental feature and using it. But that all will need to be done in new PRs.

[vadim-anfv]

The plan is to transition bdk_wallet to depreciate TxBuilder and use bdk_tx via Wallet::create_psbt (see bitcoindevkit/bdk_wallet#516). So I'd suggest adding the check to bdk-tx and updating bdk-cli to enable the Wallet::create_psbt experimental feature and using it. But that all will need to be done in new PRs.

Two things here.

1. Moving to Wallet::create_psbt: it is in bdk_wallet 3.2.0, so we can pick it up now. I'll open an issue for it.

2. The size check: 100_000 vB is default relay policy, not consensus, and miners do accept larger txs out of band, so I'd rather bdk_tx didn't refuse to build such a tx. I'd keep the check closer to the user, where refusing or warning is a call the app can make. create_psbt is also early for it: an unsigned tx can be under the limit and go over it once the witnesses are there, so the check belongs after signing.

[tvpeter]

MAX_OP_RETURN_BYTES (99_994) counts the OP_RETURN scriptPubKey only, so create_tx builds transactions over the 100_000 vB standard tx size limit, which no default-policy node relays. The output alone is 100_013 vB (8 value + 5 length prefix + 100_000 script), before any input, change or header.

That tx is 150 vB over the limit with one input, a recipient and change, so the usable payload is at most ~99_844 here, less with more inputs. the_largest_allowed_payload_fits_the_script_limit locks in the same unusable size: a 100_000 byte script never fits a standard tx.

Non-blocking: master enforced no limit here at all, so this is an improvement either way.

So I discovered that the documented max 80 bytes OP_RETURN was not enforced and was out of sync with Core 30, and wanted to fix it in this PR, but it seemed like separating it to a new issue/PR will be better. So I created an issue #339 to properly fix it in another PR.

[vadim-anfv]

tACK 3eacd05

• --utxos unknown outpoint / malformed --add_data: panic (exit 101) on master, error (exit 1) here
• --fee_rate: on master 0 and NaN silently give a zero-fee tx (10000 sat needed for a 10000 sat output) and 1e30 silently falls back to the default rate; here all three are rejected up front (exit 2)
• the same --fee_rate guard now covers bump_fee, create_sp_tx, create_dns_tx and send_payjoin: on master three of them take 0 and carry on (create_sp_tx errors on another argument first), here all four stop at argument parsing
• not tested: fractional rates and resulting fees on a funded wallet, bump_fee --utxos with an unknown outpoint, and overlong --add_data/--add_string payloads

Repro: panics

export NETWORK=regtest DATADIR=$(mktemp -d) WALLET_NAME=demo
DESC=$(cargo run -q -- descriptor --type tr | jq -r .private_descriptors.external)
cargo run -q -- wallet config -e "$DESC" --database-type sqlite
ADDR=$(cargo run -q -- wallet new_address | jq -r .address)

master:

$ cargo run -q -- wallet create_tx --to "$ADDR:10000"; echo "exit=$?"
Error: Create transaction error: Insufficient funds: 0 BTC available of 0.00010054 BTC needed
exit=1
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --utxos aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0; echo "exit=$?"
thread 'main' (1669177) panicked at src/handlers/offline.rs:291:46:
called `Result::unwrap()` on an `Err` value: UnknownUtxo(OutPoint { txid: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, vout: 0 })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
exit=101
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --add_data '!!!not-base64!!!'; echo "exit=$?"
thread 'main' (1669244) panicked at src/handlers/offline.rs:299:70:
called `Result::unwrap()` on an `Err` value: InvalidByte(0, 33)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
exit=101

this branch:

$ cargo run -q -- wallet create_tx --to "$ADDR:10000"; echo "exit=$?"
Error: Create transaction error: Insufficient funds: 0 BTC available of 0.00010054 BTC needed
exit=1
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --utxos aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0; echo "exit=$?"
Error: Add UTXO error: UTXO not found in the internal database for txid: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa with vout: 0
exit=1
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --add_data '!!!not-base64!!!'; echo "exit=$?"
Error: Base64 decoding error: Invalid byte 33, offset 0.
exit=1

Repro: fee rate

Same setup as above.

master:

$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate 0; echo "exit=$?"
Error: Create transaction error: Insufficient funds: 0 BTC available of 0.00010000 BTC needed
exit=1
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate NaN; echo "exit=$?"
Error: Create transaction error: Insufficient funds: 0 BTC available of 0.00010000 BTC needed
exit=1
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate 1e30; echo "exit=$?"
Error: Create transaction error: Insufficient funds: 0 BTC available of 0.00010054 BTC needed
exit=1

this branch:

$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate 0; echo "exit=$?"
error: invalid value '0' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '0' sat/vB is below the smallest usable rate of 0.004 sat/vB

For more information, try '--help'.
exit=2
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate NaN; echo "exit=$?"
error: invalid value 'NaN' for '--fee_rate <FEE_RATE>': Generic error: Invalid fee rate 'NaN', must be a finite number of sat/vB

For more information, try '--help'.
exit=2
$ cargo run -q -- wallet create_tx --to "$ADDR:10000" --fee_rate 1e30; echo "exit=$?"
error: invalid value '1e30' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '1e30' sat/vB is too large to represent

For more information, try '--help'.
exit=2

Repro: fee rate in the other commands

Built with --all-features, same wallet config plus --client-type electrum --url 127.0.0.1:50001.

master, --fee_rate 0 is accepted and each command fails later for its own reason:

$ cargo run -q --all-features -- wallet bump_fee --fee_rate 0 --txid aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Error: FeeBump error: Transaction not found in the internal database with txid: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
exit=1
$ cargo run -q --all-features -- wallet create_dns_tx --fee_rate 0 --to_dns user@example.com:1000
Error: Generic error: Parsing error occured HrnResolutionError(
    "DNS resolution failed",
)
exit=1
$ cargo run -q --all-features -- wallet send_payjoin -f 0 --uri bitcoin:x --ohttp_relay https://x
Error: Electrum error: Connection refused (os error 111)
exit=1

this branch, all four stop at argument parsing:

$ cargo run -q --all-features -- wallet bump_fee --fee_rate 0 --txid aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
error: invalid value '0' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '0' sat/vB is below the smallest usable rate of 0.004 sat/vB
exit=2
$ cargo run -q --all-features -- wallet create_sp_tx --fee_rate 0 --to-sp dummy:1000
error: invalid value '0' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '0' sat/vB is below the smallest usable rate of 0.004 sat/vB
exit=2
$ cargo run -q --all-features -- wallet create_dns_tx --fee_rate 0 --to_dns user@example.com:1000
error: invalid value '0' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '0' sat/vB is below the smallest usable rate of 0.004 sat/vB
exit=2
$ cargo run -q --all-features -- wallet send_payjoin -f 0 --uri bitcoin:x --ohttp_relay https://x
error: invalid value '0' for '--fee_rate <FEE_RATE>': Generic error: Fee rate '0' sat/vB is below the smallest usable rate of 0.004 sat/vB
exit=2

On master create_sp_tx errors on --to-sp first, so the fee rate there is only covered by the branch run.

[vadim-anfv]

tACK 1282553

Forgot to say this in my first review: nice touch parsing straight into FeeRate!