Conversation
- `--fee_rate` was parsed as `f32` and cast with `as u64`, which is both truncating and saturating, and when `FeeRate::from_sat_per_vb` returned None the value was silently dropped. Update parsing fee_rate at the clap boundary instead and `FeeRate` counts sat/kwu, so fractional rates keep 1/250 sat/vB precision rather than being truncated, and anything that cannot be represented is rejected. - The `--add_data` and `--add_string` length was not checked. The limit is now 99994 data bytes, Core v30's default `-datacarriersize` of 100_000 minus the 6 bytes the `OP_RETURN` opcode and `OP_PUSHDATA4` prefix occupy, since Core measures the whole scriptPubKey. A unit test pins that arithmetic against `ScriptBuf::new_op_return`.
- update `create_tx`, `create_sp_tx`,` `bump_fee` commands where `unwrap()` was called on Results and panicked. - update fee_rate calculations accross create_tx, create_sp_tx - update OP_RETURN data parsing accross the commands - add test to check against panics and invalid fee_rates
- update fee_rate in dns module and payjoin - update error propagation in dns and payjoin - update CHANGELOG
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #326 +/- ##
==========================================
+ Coverage 57.78% 58.75% +0.97%
==========================================
Files 22 22
Lines 3733 3773 +40
==========================================
+ Hits 2157 2217 +60
+ Misses 1576 1556 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tvpeter
changed the title
vadim-anfv
left a comment
vadim-anfv
left a comment
There was a problem hiding this comment.
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.
Is this something that needs to be checked and enforced in the |
bdk-cli doesn't use Is the plan to move transaction building to |
The plan is to transition |
Two things here.
|
3 participants
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:
create_txandbump_feecalled.unwrap()on Results so they panic (exit 101) instead of an error (exit 1).create_sp_txalready guarded these paths, but improvements were made to the error type been returned--fee_ratewas anf32cast withas u64, which truncates and saturates, and aNonefromfrom_sat_per_vbwas silently skipped. Parsing now happens in avalue_parser, so bad values are rejected witha usage message before a wallet is loaded. Because
FeeRatecounts sat/kwu, fractional rates keep 1/250 sat/vB precision instead of being truncated .--add_dataand--add_stringdocument "max 80 bytes" and neither enforced it. This has now been updated to 100_000 bytes and enforced in transaction building.create_dns_txwas had the same fee-rate bug and the same OP_RETURN handling, and has been fixed too.bump_fee --utxosandsend_payjoin -fare fixed by the same changes.Fixes #325
Notes to the reviewers
Changelog notice
create_txandbump_feepanicking on malformed--utxosand--add_datavalues instead of returning an error--fee_ratesilently truncating to a whole sat/vB, falling back to a default, or producing a zero-fee transaction; unusable values are now rejected--add_dataand--add_stringOP_RETURN payloadsChecklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: