fix(wallet): reject txs exceeding MAX_STANDARD_TX_WEIGHT

[cestercian]

Description

Neither create_tx nor create_psbt checked the assembled transaction against Bitcoin's standardness weight limit (bitcoin::policy::MAX_STANDARD_TX_WEIGHT, 400_000 WU). Dust was already rejected (OutputBelowDustLimit); weight was not.

A drain of many small UTXOs could therefore produce a fully signed PSBT over 400k WU. Every standardness-enforcing mempool rejects that transaction, but sign still returned Ok(true) — a misleading success.

Root cause: After coin selection the unsigned transaction is assembled with empty witnesses. Transaction::weight() on that value undercounts the final signed size by each input's satisfaction (witness / scriptSig) weight. No later check compared the estimated signed weight to the standardness limit.

Fix: After the unsigned tx is assembled, estimate the signed weight (tx.weight() plus each input's satisfaction weight, plus the 2-WU segwit marker when witnesses will be present) and reject when it exceeds MAX_STANDARD_TX_WEIGHT.

• New CreateTxError::TxWeightLimitExceeded { weight, limit } (stable create_tx path)
• New CreatePsbtError::TxWeightLimitExceeded { weight, limit } (unstable create_psbt / create_psbt_from_selector path, so RBF is covered too)

Tests:

• Drain of 1,500 small P2WPKH UTXOs (the reported case) via a single in-memory funding transaction — no Electrum/Esplora/RPC
• Unit-level foreign UTXO whose satisfaction weight alone exceeds the limit
• Matching create_psbt drain regression

Fixes #543

Notes to the reviewers

• This is a breaking change: CreateTxError is exhaustive, so downstream matches must handle the new variant. CreatePsbtError is already #[non_exhaustive].
• The check uses estimated signed weight, not raw unsigned tx.weight(). Checking only the unsigned weight would miss the 1,500-P2WPKH-input case (~246k WU unsigned vs ~408k WU signed).
• Satisfaction weights for create_tx come from the WeightedUtxos used in coin selection (including foreign UTXOs). The create_psbt path looks up local descriptors via max_weight_to_satisfy.

Changelog notice

• Fixed create_tx / create_psbt producing unrelayable transactions over MAX_STANDARD_TX_WEIGHT by returning TxWeightLimitExceeded.

Before submitting

• I followed the contribution guidelines
• This PR breaks the existing API

Assistance: implementation drafted with Cursor (cloud agent); author is Cestercian. Commits are SSH-signed; GitHub may show Unverified if the cloud-agent SSH signing key is not registered on the account as a signing key.

[j-kon]

Thanks for tackling issue #543! Preventing wallets from building transactions that standard mempools will inevitably reject is an important safety improvement.

While the high-level intent and the tests for local P2WPKH drains are solid, our line-by-line review and test verification identified three concrete issues in the implementation that need to be addressed before this is ready to merge:

1. Foreign / planned input satisfaction weights are dropped in create_psbt:
   In src/wallet/mod.rs:3488-3494, create_psbt resolves satisfaction weights solely via self.satisfaction_weight_for_outpoint. For foreign or planned inputs, this returns Weight::ZERO, meaning the standardness check underestimates transactions containing external or planned inputs.
2. Witness serialization discrepancies in check_max_standard_tx_weight:
   In src/wallet/mod.rs:2995-3000:
   • Pure legacy transactions (+2 WU): Any non-zero satisfaction weight (including legacy scriptSig satisfaction like 428 WU for P2PKH) triggers + 2 WU for SegWit marker/flag, which are never serialized.
   • SegWit & Mixed transactions (-1 WU per input): When an unsigned transaction has empty witnesses, tx.weight() evaluates as a legacy transaction with zero witness bytes. Under BIP 141 extended serialization, every input requires a 1-byte witness stack length varint (0x00). Miniscript's max_weight_to_satisfy() returns the delta from TxIn::default().segwit_weight() (which already assumed that 1 byte was present). Adding only + 2 WU fails to account for this 1 byte per input ($K$ WU total), underestimating multi-input SegWit and mixed transactions.
3. Unchecked arithmetic leads to debug panic and release-mode validation bypass:
   In src/wallet/mod.rs:2994-3000, satisfaction_weights.into_iter().sum() and tx.weight() + satisfaction + segwit_marker use unchecked Weight addition. When foreign UTXOs have large satisfaction weights:
   • In debug builds, it panics with attempt to add with overflow instead of returning TxWeightLimitExceeded.
   • In release builds (overflow-checks = false), the addition silently wraps around modulo $2^{64}$. A transaction with an astronomical satisfaction weight wraps to a small number (e.g. 129 WU), passing weight <= MAX_STANDARD_TX_WEIGHT and completely bypassing the policy check.

Detailed suggestions and minimal reproductions are provided in the inline comments below.

[j-kon]

In create_psbt, satisfaction weights are resolved solely via self.satisfaction_weight_for_outpoint(txin.previous_output):

fn satisfaction_weight_for_outpoint(&self, outpoint: OutPoint) -> Weight {
    self.get_utxo(outpoint)
        .and_then(|utxo| {
            self.public_descriptor(utxo.keychain)
                .max_weight_to_satisfy()
                .ok()
        })
        .unwrap_or(Weight::ZERO)
}

For foreign or planned inputs (e.g. added via PsbtParams::add_planned_input or coin-selected from external sources), self.get_utxo returns None, so their satisfaction weight evaluates to Weight::ZERO.

As a result, create_psbt completely discounts satisfaction weights for non-local inputs. A PSBT spending foreign/planned inputs whose signed size would exceed MAX_STANDARD_TX_WEIGHT passes with Ok, whereas the equivalent transaction in create_tx correctly fails with TxWeightLimitExceeded.

Reproduction:
Adding a planned/foreign input with satisfaction weight exceeding 400,000 WU to PsbtParams results in wallet.create_psbt(params) returning Ok(psbt) instead of Err(CreatePsbtError::TxWeightLimitExceeded).

Suggestion:
Consider retrieving the planned satisfaction weights from the selection candidates (e.g., selection.inputs(), where bdk_tx::Input::satisfaction_weight() is already tracked) rather than querying self.satisfaction_weight_for_outpoint.

[cestercian]

create_psbt now takes satisfaction_weight() from the selected inputs, so planned/foreign ones are not zeroed out anymore.

[j-kon]

The witness overhead calculation has two accounting discrepancies with BIP 141 serialization:

1. Pure legacy overestimation (+2 WU):
   satisfaction > Weight::ZERO is true for legacy transactions because scriptSig satisfaction weights are positive (e.g., 428 WU for P2PKH). This adds 2 WU for a SegWit marker/flag (0x0001) that is never serialized in legacy transactions.

2. SegWit & Mixed underestimation (-1 WU per input):
   When tx is unsigned, all input witnesses are empty, so tx.weight() evaluates as a legacy transaction with zero witness bytes.
   Under BIP 141 extended serialization, every input requires a 1-byte witness stack length varint (0x00). Miniscript's max_weight_to_satisfy() returns txin.segwit_weight() - TxIn::default().segwit_weight(), where TxIn::default().segwit_weight() already assumed that 1-byte empty witness was present. Adding only + 2 WU misses 1 WU for every input ($K$ WU total).

On transactions with many inputs (such as UTXO consolidation), this systematically underestimates the worst-case signed weight by $K$ WU:

Case	Inputs ($K$)	Unsigned	Satisfaction	PR Estimate	Actual Worst-Case Signed	Discrepancy
Pure P2PKH	1	340 WU	428 WU	770 WU	768 WU	+2 WU (overcounted)
Mixed (1 WPKH + 2 PKH)	3	656 WU	963 WU	1,621 WU	1,624 WU	-3 WU (undercounted)
Pure P2WPKH	100	16,564 WU	10,700 WU	27,266 WU	27,366 WU	-100 WU (undercounted)

Suggestion:
Consider differentiating transactions where witnesses will be present:

• If no inputs have witness satisfaction (pure legacy): witness overhead is 0 WU.
• If any input has witness satisfaction (SegWit): witness overhead is 2 WU + (1 WU * tx.input.len()).

[cestercian]

Yeah — pure legacy adds no marker now; if any input has a witness, it's 2 WU plus 1 per input. Checked against 768 / 1,624 / 27,366 from your table.

[j-kon]

bitcoin::Weight wraps u64 and implements Add and Sum via primitive integer arithmetic without overflow protection. If foreign inputs have large caller-supplied satisfaction weights (via TxBuilder::add_foreign_utxo), this arithmetic overflows:

• In debug builds (cargo test), tx.weight() + satisfaction panics with attempt to add with overflow instead of returning CreateTxError::TxWeightLimitExceeded.
• In release builds (where overflow-checks = false), the addition silently wraps around modulo $2^{64}$. A transaction with an astronomical satisfaction weight wraps to a small number, passing weight <= MAX_STANDARD_TX_WEIGHT and completely bypassing the check.

Reproduction:

let mut builder = wallet.build_tx();
builder.add_foreign_utxo(outpoint, psbt_in, Weight::from_wu(u64::MAX - 200)).unwrap();
builder.manually_selected_only();
builder.fee_absolute(Amount::from_sat(1000));
builder.drain_wallet().drain_to(addr.script_pubkey());

let res = builder.finish();

• Under cargo test, this panics with attempt to add with overflow at line 3000.
• Under cargo test --release, unsigned weight 328 WU + $(2^{64} - 201)\text{ WU} + 2\text{ WU}$ wraps to 129 WU, returning Ok(psbt) and bypassing the standardness limit.

Suggestion:
Use checked arithmetic (satisfaction_weights.into_iter().try_fold(Weight::ZERO, Weight::checked_add) and checked_add), and return Err((Weight::MAX, limit)) if an overflow occurs.

[cestercian]

Switched to checked_add. Overflow comes back as TxWeightLimitExceeded with Weight::MAX instead of panicking or wrapping past the limit.

[cestercian]

Thanks for the review — addressed all three.

Planned/foreign inputs keep their satisfaction weight from the selection, witness overhead now splits pure legacy (0) vs segwit/mixed (2 + 1 per input), and a huge foreign satisfaction returns TxWeightLimitExceeded instead of panicking or wrapping. The 1,500-input drains still fail the check; added coverage for the planned-input case, the overhead table, and the overflow path.

[codecov]

Codecov Report

❌ Patch coverage is 67.59259% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.43%. Comparing base (6fc6846) to head (793fd44).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/wallet/mod.rs	71.56%	28 Missing and 1 partial ⚠️
src/wallet/error.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   81.91%   81.43%   -0.48%     
==========================================
  Files          25       25              
  Lines        6535     6448      -87     
  Branches      302      313      +11     
==========================================
- Hits         5353     5251     -102     
- Misses       1075     1089      +14     
- Partials      107      108       +1     

Flag	Coverage Δ
rust	81.43% <67.59%> (-0.48%)	⬇️

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.

[j-kon]

Follow-up from re-review: spends_with_witness() currently treats psbt_input.witness_utxo.is_some() as proof that the input will use witness serialization.

I reproduced a case through the public add_foreign_utxo API using a legacy P2PKH prevout with a matching non_witness_utxo and witness_utxo. The input is accepted, but it is classified as SegWit even though the final spend is legacy.

At the standardness boundary this becomes observable: an actual 400,000 WU legacy transaction is estimated as 400,003 WU and rejected with TxWeightLimitExceeded.

Could we derive witness serialization from the prevout/redeem script instead of using the presence of witness_utxo itself? For example, native witness programs are identifiable from the prevout, and nested SegWit can be identified from a P2SH redeem script that is itself a witness program.

[tvpeter]

Please rebase and sign all your commits. Take a look at contributing guide here

[cestercian]

yeah makes sense. spends_with_witness now looks at the prevout / redeem script instead of whether witness_utxo is set, and i rebased + signed the commits.

[cestercian]

rebased on master and pushed the weight fixes. satisfaction weight comes from the selected inputs now, segwit overhead only gets counted when an input is actually segwit, and the sums are checked so overflow returns TxWeightLimitExceeded.