PSBT: missing witness_utxo for segwit inputs

[bigspider]

Currently, electrum seems to be adding the witness_utxo for legacy P2PKH inputs; per BIP-174, the PSBT_IN_WITNESS_UTXO should only be present for inputs which spend segwit outputs.

This affects the new Ledger bitcoin app, which relies on this assumption when signing PSBTs with legacy inputs.

[SomberNight]

I think we only do this when displaying QR codes. Is that what you mean?
If not, do you have steps to reproduce?

See e.g. this test:

electrum/electrum/tests/test_wallet_vertical.py

Line 2654 in 30ac889

self.assertEqual("70736274ff010074010000000162878508bdcd372fc4c33ce6145cd1fb1dcc5314d4930ceb6957e7f6c54b57980200000000fdffffff02a0252600000000001600140bf6c540d0218c99511f7c62a49784c88b1870b8585d7200000000001976a9149b308d0b3efd4e3469441bc83c3521afde4072b988ac1c391400000100fd4c0d01000000000116e9c9dac2651672316aab3b9553257b6942c5f762c5d795776d9cfa504f183c000000000000fdffffff8085019852fada9da84b58dcf753d292dde314a19f5a5527f6588fa2566142130000000000fdffffffa4154a48db20ce538b28722a89c6b578bd5b5d60d6d7b52323976339e39405230000000000fdffffff0b5ef43f843a96364aebd708e25ea1bdcf2c7df7d0d995560b8b1be5f357b64f0100000000fdffffffd41dfe1199c76fdb3f20e9947ea31136d032d9da48c5e45d85c8f440e2351a510100000000fdffffff5bd015d17e4a1837b01c24ebb4a6b394e3da96a85442bd7dc6abddfbf16f20510000000000fdffffff13a3e7f80b1bd46e38f2abc9e2f335c18a4b0af1778133c7f1c3caae9504345c0200000000fdffffffdf4fc1ab21bca69d18544ddb10a913cd952dbc730ab3d236dd9471445ff405680100000000fdffffffe0424d78a30d5e60ac6b26e2274d7d6e7c6b78fe0b49bdc3ac4dd2147c9535750100000000fdffffff7ab6dd6b3c0d44b0fef0fdc9ab0ad6eee23eef799eee29c005d52bc4461998760000000000fdffffff48a77e5053a21acdf4f235ce00c82c9bc1704700f54d217f6a30704711b9737d0000000000fdffffff86918b39c1d9bb6f34d9b082182f73cedd15504331164dc2b186e95c568ccb870000000000fdffffff15a847356cbb44be67f345965bb3f2589e2fec1c9a0ada21fd28225dcc602e8f0100000000fdffffff9a2875297f81dfd3b77426d63f621db350c270cc28c634ad86b9969ee33ac6960000000000fdffffffd6eeb1d1833e00967083d1ab86fa5a2e44355bd613d9277135240fe6f60148a20100000000fdffffffd8a6e5a9b68a65ff88220ca33e36faf6f826ae8c5c8a13fe818a5e63828b68a40100000000fdffffff73aab8471f82092e45ed1b1afeffdb49ea1ec74ce4853f971812f6a72a7e85aa0000000000fdffffffacd6459dec7c3c51048eb112630da756f5d4cb4752b8d39aa325407ae0885cba0000000000fdffffff1eddd5e13bef1aba1ff151762b5860837daa9b39db1eae8ea8227c81a5a1c8ba0000000000fdffffff67a096ff7c343d39e96929798097f6d7a61156bbdb905fbe534ba36f273271d40100000000fdffffff109a671eb7daf6dcd07c0ceff99f2de65864ab36d64fb3a890bab951569adeee0100000000fdffffff4f1bdc64da8056d08f79db7f5348d1de55946e57aa7c8279499c703889b6e0fd0200000000fdffffff042f280000000000001600149c756aa33f4f89418b33872a973274b5445c727b80969800000000001600146c540c1c9f546004539f45318b8d9f4d7b4857ef80969800000000001976a91422a6daa4a7b695c8a2dd104d47c5dc73d655c96f88ac809698000000000017a914a6885437e0762013facbda93894202a0fe86e35f8702473044022075ef5f04d7a63347064938e15a0c74277a79e5c9d32a26e39e8a517a44d565cc022015246790fb5b29c9bf3eded1b95699b1635bcfc6d521886fddf1135ba1b988ec012102801bc7170efb82c490e243204d86970f15966aa3bce6a06bef5c09a83a5bfffe02473044022061aa9b0d9649ffd7259bc54b35f678565dbbe11507d348dd8885522eaf1fa70c02202cc79de09e8e63e8d57fde6ef66c079ddac4d9828e1936a9db833d4c142615c3012103a8f58fc1f5625f18293403104874f2d38c9279f777e512570e4199c7d292b81b0247304402207744dc1ab0bf77c081b58540c4321d090c0a24a32742a361aa55ad86f0c7c24e02201a9b0dd78b63b495ab5a0b5b161c54cb085d70683c90e188bb4dc2e41e142f6601210361fb354f8259abfcbfbdda36b7cb4c3b05a3ca3d68dd391fd8376e920d93870d0247304402204803e423c321acc6c12cb0ebf196d2906842fdfed6de977cc78277052ee5f15002200634670c1dc25e6b1787a65d3e09c8e6bb0340238d90b9d98887e8fd53944e080121031104c60d027123bf8676bcaefaa66c001a0d3d379dc4a9492a567a9e1004452d02473044022050e4b5348d30011a22b6ae8b43921d29249d88ea71b1fbaa2d9c22dfdef58b7002201c5d5e143aa8835454f61b0742226ebf8cd466bcc2cdcb1f77b92e473d3b13190121030496b9d49aa8efece4f619876c60a77d2c0dc846390ecdc5d9acbfa1bb3128760247304402204d6a9b986e1a0e3473e8aef84b3eb7052442a76dfd7631e35377f141496a55490220131ab342853c01e31f111436f8461e28bc95883b871ca0e01b5f57146e79d7bb012103262ffbc88e25296056a3c65c880e3686297e07f360e6b80f1219d65b0900e84e02483045022100c8ffacf92efa1dddef7e858a241af7a80adcc2489bcc325195970733b1f35fac022076f40c26023a228041a9665c5290b9918d06f03b716e4d8f6d47e79121c7eb37012102d9ba7e02d7cd7dd24302f823b3114c99da21549c663f72440dc87e8ba412120902483045022100b55545d84e43d001bbc10a981f184e7d3b98a7ed6689863716cab053b3655a2f0220537eb76a695fbe86bf020b4b6f7ae93b506d778bbd0885f0a61067616a2c8bce0121034a57f2fa2c32c9246691f6a922fb1ebdf1468792bae7eff253a99fc9f2a5023902483045022100f1d4408463dbfe257f9f778d5e9c8cdb97c8b1d395dbd2e180bc08cad306492c022002a024e19e1a406eaa24467f033659de09ab58822987281e28bb6359288337bd012103e91daa18d924eea62011ce596e15b6d683975cf724ea5bf69a8e2022c26fc12f0247304402204f1e12b923872f396e5e1a3aa94b0b2e86b4ce448f4349a017631db26d7dff8a022069899a05de2ad2bbd8e0202c56ab1025a7db9a4998eea70744e3c367d2a7eb71012103b0eee86792dbef1d4a49bc4ea32d197c8c15d27e6e0c5c33e58e409e26d4a39a0247304402201787dacdb92e0df6ad90226649f0e8321287d0bd8fddc536a297dd19b5fc103e022001fe89300a76e5b46d0e3f7e39e0ee26cc83b71d59a2a5da1dd7b13350cd0c07012103afb1e43d7ec6b7999ef0f1093069e68fe1dfe5d73fc6cfb4f7a5022f7098758c02483045022100acc1212bba0fe4fcc6c3ae5cf8e25f221f140c8444d3c08dfc53a93630ac25da02203f12982847244bd9421ef340293f3a38d2ab5d028af60769e46fcc7d81312e7e012102801bc7170efb82c490e243204d86970f15966aa3bce6a06bef5c09a83a5bfffe024830450221009c04934102402949484b21899271c3991c007b783b8efc85a3c3d24641ac7c24022006fb1895ce969d08a2cb29413e1a85427c7e85426f7a185108ca44b5a0328cb301210360248db4c7d7f76fe231998d2967104fee04df8d8da34f10101cc5523e82648c02483045022100b11fe61b393fa5dbe18ab98f65c249345b429b13f69ee2d1b1335725b24a0e73022010960cdc5565cbc81885c8ed95142435d3c202dfa5a3dc5f50f3914c106335ce0121029c878610c34c21381cda12f6f36ab88bf60f5f496c1b82c357b8ac448713e7b50247304402200ca080db069c15bbf98e1d4dff68d0aea51227ff5d17a8cf67ceae464c22bbb0022051e7331c0918cbb71bb2cef29ca62411454508a16180b0fb5df94248890840df0121028f0be0cde43ff047edbda42c91c37152449d69789eb812bb2e148e4f22472c0f0247304402201fefe258938a2c481d5a745ef3aa8d9f8124bbe7f1f8c693e2ddce4ddc9a927c02204049e0060889ede8fda975edf896c03782d71ba53feb51b04f5ae5897d7431dc012103946730b480f52a43218a9edce240e8b234790e21df5e96482703d81c3c19d3f1024730440220126a6a56dbe69af78d156626fc9cf41d6aac0c07b8b5f0f8491f68db5e89cb5002207ee6ed6f2f41da256f3c1e79679a3de6cf34cc08b940b82be14aefe7da031a6b012102801bc7170efb82c490e243204d86970f15966aa3bce6a06bef5c09a83a5bfffe024730440220363204a1586d7f13c148295122cbf9ec7939685e3cadab81d6d9e921436d21b7022044626b8c2bd4aa7c167d74bc4e9eb9d0744e29ce0ad906d78e10d6d854f23d170121037fb9c51716739bb4c146857fab5a783372f72a65987d61f3b58c74360f4328dd0247304402207925a4c2a3a6b76e10558717ee28fcb8c6fde161b9dc6382239af9f372ace99902204a58e31ce0b4a4804a42d2224331289311ded2748062c92c8aca769e81417a4c012102e18a8c235b48e41ef98265a8e07fa005d2602b96d585a61ad67168d74e7391cb02483045022100bbfe060479174a8d846b5a897526003eb2220ba307a5fee6e1e8de3e4e8b38fd02206723857301d447f67ac98a5a5c2b80ef6820e98fae213db1720f93d91161803b01210386728e2ac3ecee15f58d0505ee26f86a68f08c702941ffaf2fb7213e5026aea10247304402203a2613ae68f697eb02b5b7d18e3c4236966dac2b3a760e3021197d76e9ad4239022046f9067d3df650fcabbdfd250308c64f90757dec86f0b08813c979a42d06a6ec012102a1d7ee1cb4dc502f899aaafae0a2eb6cbf80d9a1073ae60ddcaabc3b1d1f15df02483045022100ab1bea2cc5388428fd126c7801550208701e21564bd4bd00cfd4407cfafc1acd0220508ee587f080f3c80a5c0b2175b58edd84b755e659e2135b3152044d75ebc4b501210236dd1b7f27a296447d0eb3750e1bdb2d53af50b31a72a45511dc1ec3fe7a684a19391400220602ab053d10eda769fab03ab52ee4f1692730288751369643290a8506e31d1e80f00c233d2ae40000000002000000000022020327295144ffff9943356c2d6625f5e2d6411bab77fd56dce571fda6234324e3d90c233d2ae4010000000000000000",

In the normal flow, where tx.to_qr_data() is not called, the input includes PSBT_IN_NON_WITNESS_UTXO and lacks PSBT_IN_WITNESS_UTXO.

[bigspider]

It happened while signing from a standard BIP-44 legacy account; converting the PartialTransaction received by the plugin during sign_transaction resulted in the following psbt:

cHNidP8BAHcCAAAAAXT0yaTajRSLu1boaayjaQ3aDOOsvPgWCcyUbRtvFkrOAAAAAAD9////AhAnAAAAAAAAGXapFCeDycrsILcRWnbAlev4DTq9ohuCiKwkwgAAAAAAABl2qRQ0sv5FvvH8hS5yUi3rDcYH2ol3gIisf0okAAABAPoCAAAAAAEB0OO1kdxITttx6rZqDplHMQB/630IKlDHQvAj6gkBTR4BAAAAFxYAFOMQ0ET4jasbQnaeSoTK8INj+ezB/f///wJg6gAAAAAAABl2qRRFiB7Q01h1UPeUhHt8i5+gPt0KPIisf2YbAAAAAAAXqRTwRk2foOpC2A5NXxRXiDmC4juO7IcCSDBFAiEA4NgNxVQH8N5sYoW6VVzBleEOiUBo35E01ME306Uzi68CIC2B5IlKIAnyqPNVj1wgoj3XHGeNfKzgOBbMAGDITQZoASECqA8AcZTVPTf2+ZU59jU5BYjE4cMosJgpX2GvQNYMsooAAAAAAQEiYOoAAAAAAAAZdqkURYge0NNYdVD3lIR7fIufoD7dCjyIrCIGAtMSuZbG2GtAnMBFWs/KVFyDeyhqZJRU1uLfA7JCoaCaGPWswv0sAACAAQAAgAAAAIAAAAAABgAAAAAiAgN4ZyP/Vn84KuygtxCT15W2TM/5XVIgJ1lm7EurXJmZRxj1rML9LAAAgAEAAIAAAACAAAAAAAcAAAAAIgIDmXMPrZCIByQHALeZ1wN0Rryr8uW9VyY0ruqb/4zVRYcY9azC/SwAAIABAACAAAAAgAEAAAAFAAAAAA==

It's from my custom branch, but it shouldn't be related to anything I do, since I create the psbt with tx.serialize_as_bytes().

If it's not enough info to reproduce yet, I should be able to open the initial PR for #7897 tomorrow; it might be easier to reproduce on the PR's branch, since the current Ledger plugin doesn't use psbt.

[SomberNight]

Something is weird with your tx. The input has both PSBT_IN_NON_WITNESS_UTXO and PSBT_IN_WITNESS_UTXO set.
Electrum never does this.

electrum/electrum/transaction.py

Line 1253 in 335f55a

self.ensure_there_is_only_one_utxo()

[bigspider]

Indeed, another weird thing about this psbt is that the both outputs have the BIP32 derivation info, while that's only relevant for the change output.
I made the transaction by sending to a different receiving address from the same wallet (first output); I wonder if that could be the source of the weirdness?

[SomberNight]

both outputs have the BIP32 derivation info, while that's only relevant for the change output.
I made the transaction by sending to a different receiving address from the same wallet (first output)

That's expected. The wallet adds all the info it has.
I guess it's a design decision and you could argue either way, but I think this behaviour is fine.

No idea about the witness utxo weirdness though.

[bigspider]

You are completely right, the witness_utxo was added from my plugin.
My mistake, sorry for wasting your time, and thanks for the clarifications!

[bigspider]

Reopened, as there might still be an inconsistency with the standard. My understanding is the following:

• for legacy inputs, witness_utxo shouldn't be there
• for segwitv0 inputs, both non_witness_utxo and witness_utxo should be present. Having the witness_utxo without the non_witness_utxo currently causes a warning with Ledger hardware wallets (and probably other devices, too), because there are some possible attacks.
• for segwitv1 inputs, just the witness_utxo is enough,

Currently, electrum seems to only put the non_witness_utxo and skip the witness_utxo for segwitv0 inputs.

I can easily fix it in the plugin code before sending to the device − but mentioning it anyway as I think it's not matching the expectations of BIP174.

[SomberNight]

for segwitv0 inputs, both non_witness_utxo and witness_utxo should be present. Having the witness_utxo without the non_witness_utxo currently causes a warning with Ledger hardware wallets (and probably other devices, too), because there are some possible attacks.

I don't see why you would want both non_witness_utxo and witness_utxo present at the same time. It is a waste of space and potential for inconsistency (if they mismatch). The witness_utxo can be cleanly calculated from the non_witness_utxo (and the prevout_idx). IIRC Bitcoin Core might be including both but only for compatibility reasons with old versions of itself.

In my reading, the spec allows this:

PSBT_IN_NON_WITNESS_UTXO
This should be present for inputs that spend non-segwit outputs and can be present for inputs that spend segwit outputs. An input can have both PSBT_IN_NON_WITNESS_UTXO and PSBT_IN_WITNESS_UTXO.

PSBT_IN_WITNESS_UTXO
This should only be present for inputs which spend segwit outputs

Why can both UTXO types be provided?
In order to be compatible with software which were expecting PSBT_IN_WITNESS_UTXO

So PSBT_IN_NON_WITNESS_UTXO can be present any time, and PSBT_IN_WITNESS_UTXO should not be present for legacy inputs. They can both be present but it's redundant.

I mean, if there is a real motivating use case, we could include both, but if it's only old versions of Bitcoin Core then I don't see the point. If the ledger firmware code is expecting both, I would argue it is that firmware that should be changed. :)

[bigspider]

The specs is indeed not super explicit. Since PSBT_IN_WITNESS_UTXO can be computed from PSBT_IN_NON_WITNESS_UTXO, it would seem that it's redundant at first sight. My reasoning on why that's not the right reading of the specs is the following:

• the non-witness-utxo is a full transaction, that can be arbitrarily big (imagine if you're spending an output from a transaction out of an exchange that does batching; that could easily be tens of kb, and it all needs to be sent to the signing device!); instead, the witness-utxo is always less 60 bytes.
• segwitv0 tried to fix the problem by making the sighash computation depend on the witness-utxo field, but fell short and there is a bug (arguably not easy to exploit, but a bug nonetheless; see footnote 7 in BIP 174); that's why hardware signers started expecting the non-witness-utxo anyway.
• segwitv1 actually did fix the bug, so it's safe to sign transactions without non-witness-utxo. Therefore, for taproot inputs, the non-witness-utxo should not be included, allowing the psbt to be much smaller.

Because of this, my impression is that relying on the non-witness-utxo should be considered the legacy quirk, and the witness-utxo is the way to go. It's true that the witness utxo is redundant data if the non-witness-utxo is present, but it is very little redundant data − while for segwitv1 the non-witness-utxo includes potentially a lot of useless data.

I'd like to hear the opinion of @achow101 if he wants to chime in.

In the current implementation, the Ledger app always uses the witness utxo for segwitv0 or segwitv1 inputs, but gives a warning for segwitv0 inputs if the non-witness-utxo is missing (to protect from the exploitable edge-cases).
The PR #8041 already includes a workaround (adding the witness-utxo if missing before sending to the device), so it's not really impacting me at this point.
Feel free to close if you deem it not relevant − but I still find the discussion about the specs valuable.

[achow101]

Non-witness UTXO is required for non-segwit inputs, and may be included for segwit inputs. Witness UTXO is required for segwit inputs, and should not be included for non-segwit inputs. Segwit inputs can have both fields in order to deal with the sighash issue, but also for backwards compatibility with (versions of) software written before the issue was discovered.

[SomberNight]

Witness UTXO is required for segwit inputs

where does the spec say that? weird that no one complained for years that we are only putting non-witness-utxo for segwit v0 inputs.

[achow101]

s/required/expected

The simple signer algorithm described in the BIP looks for the presence of witness UTXO to determine how to sign.

[SomberNight]

The simple signer algorithm described in the BIP looks for the presence of witness UTXO to determine how to sign.

The simple signer algorithm also expects the lack of non_witness_utxo for witness v0 inputs. So it is broken regardless and not a good argument for this change.

Anyway, I don't think the spec is explicit about this, but I've found practical examples of incompatibility (cryptoadvance/specter-desktop#868, cryptoadvance/specter-desktop#1046), so I've changed this now in d3227d7.