search

rust-bitcoin / rust-bitcoin

Rust Bitcoin library
Creative Commons Zero v1.0 Universal
2.07k stars 672 forks source link

Documentation of PSBT non_witness_utxo field #2175

Open RCasatta opened 10 months ago

RCasatta commented 10 months ago

Should only be Option::Some for inputs which spend non-segwit outputs or if it is unknown whether an input spends a segwit output.

This seems wrong due to the "segwit bug"

I think both trezor and ledger hardware wallet for example refuse to sign a PSBT if the non_witness_utxo isn't Some for segwit inputs

apoelstra commented 10 months ago

Ouch, great catch!

Kixunil commented 10 months ago

@RCasatta true but I think they should only reject pre-taproot?

RCasatta commented 10 months ago

yes, so only for v0 outputs

IIRC taproot fixed the issue by committing the signature to the previous output

junderw commented 10 months ago

I think it bears mentioning that the requirement for NonWitnessUtxo in segwit transactions is a breaking change for some wallets due to large parent transaction sizes.

ie. We added the feature under the assumption that it was backwards compatible (since any implementation that doesn't check it will just ignore it. But since a lot of our incoming UTXOs are from exchange withdrawals that spend tons of UTXOs and send to tons of outputs, we were hitting JSON payload limits for AWS Lambda and breaking things left and right just by the sheer size of the base64 encoded PSBT growth. We eventually removed the feature and replaced it with proprietary methodology for checks.

junderw commented 10 months ago

As a side note, I find it unfortunate that duplicate parent transactions were not accounted for...

Our wallet (exchange) has tons of transactions in our history where multiple inputs are pointing to multiple outputs on the same parent transaction. (ie. multiple users of our competitor withdraw to our exchange at the same time)