psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs #19215

achow101 · 2020-06-08T23:34:07Z

Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a witness_utxo to determine whether to produce a segwit signature, we keep that field to ease the transition.

Because all of the sanity checks implemented by the IsSane functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.

Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.

As discussed in the wallet IRC meeting, our own signer will not require non_witness_utxo for segwit inputs.

DrahtBot · 2020-06-08T23:58:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

[BIP 174] PSBT version, proprietary, and xpub fields #17034 (Bip174 extensions by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Rspigler · 2020-06-09T05:27:47Z

Could you explain further what you mean by including non_witness_utxo but not requiring it for signing?

achow101 · 2020-06-09T06:03:47Z

Could you explain further what you mean by including non_witness_utxo but not requiring it for signing?

non_witness_utxo will be added to segwit inputs during updating. However Core's signer will not enforce that it exists for segwit inputs during signing - witness_utxo can be provided instead. I.e. signing will not fail if a segwit input only has a segwit_utxo.

luke-jr

utACK

Github-Pull: bitcoin#19215 Rebased-From: 2789417

Github-Pull: bitcoin#19215 Rebased-From: 8b7d74f

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: bitcoin#19215 Rebased-From: a656eb3

Github-Pull: bitcoin#19215 Rebased-From: 836d6fc

Sjors · 2020-06-17T18:45:06Z

I tested 836d6fc with a Trezor, Ledger Nano X and Nano S that this doesn't break old firmware and seems to work nicely with new firmware, see bitcoin-core/HWI#340

1613a33 trezorlib: Raise a more specific error when a prevtx is not provided (Andrew Chow) 45dbd08 ledger: For app v1.4.0+, do getTrustedInput on segwit inputs (Andrew Chow) fe0f82a btchip: Give the major, minor, and patch versions separately (Andrew Chow) ce5e095 btchip: prefer trustedInput over witness (Andrew Chow) 9800899 ledger: Determine segwit based on scripts and allow both UTXOs (Andrew Chow) c085077 tests: increase signtx keypool (Andrew Chow) a47f7e5 trezor: Determine segwit based on scripts instead of provided info (Andrew Chow) 662eab0 bitbox: Update to inspect scriptcode for signature type (Andrew Chow) 249825d Make imports multiline (Andrew Chow) 9f8e03c trezorlib: Update sign_tx from upstream (Andrew Chow) a264d84 test: Do legacy, then segwit, then mixed (Andrew Chow) 4ba72f3 trezor: handle when psbt has both types of UTXOs (Andrew Chow) 2f40d0f trezor: Use standard derivation paths (Andrew Chow) f862135 trezor: Set coin name as testnet when --testnet (Andrew Chow) d662fad serializations: Refactor script type matching into separate functions (Andrew Chow) 3b6131c test: Patch bitcoind to give out PSBTs with both UTXOs (Andrew Chow) 65c6de6 psbt: Allow PSBTs to have both non_witness_utxo and witness_utxo (Andrew Chow) Pull request description: This is a general update to fix many issues related to newly released signing behavior in Trezors and Ledgers. * The PSBT module is updated to handle PSBT inputs that include both `non_witness_utxo` and `witness_utxo` * For Trezor, Ledger, and Bitbox, instead of checking for existence of `witness_utxo`, `witness_script`, and `redeem_script` to determine signing behavior, we instead inspect the scripts using existing template functions to determine their type and base our behavior on that. * For Trezor and Ledger, the signing behavior is updated to pass in the full previous transaction for segwit inputs if they are available. * For Trezor, all of the derivation paths are updated to fit within Trezor's "known paths". * In the tests, a patch to bitcoind is added that allows for PSBTs with inputs that have both UTXO types. This is mostly from bitcoin/bitcoin#19215 and should be removed once that PR is merged. Note that the Ledger emulator has not been updated to include the Bitcoin app that has the segwit requirements. Also note that the updated app with those requirements seems to have a [bug](LedgerHQ/app-bitcoin#154) when a transaction has multiple segwit inputs. Closes #338 and #337 ACKs for top commit: Sjors: Tested and rather cursory code review ACK 1613a33. I didn't test the BitBox changes. Tree-SHA512: 18e238748e4e17cc692c2725881b62dc13499d127b532922b550cf41bd3c128927520f91b560706c70b4f4059efefccc3e1fe103168340ecb7eb5bb0060f1bfc

Offline signers will always need a non_witness_utxo so make sure it is there.

sipa · 2020-06-29T15:52:47Z

utACK. I think we'll want this for 0.20.1

maflcko · 2020-06-29T17:25:23Z

@luke-jr @Sjors mind to re-ACK?

ryanofsky

Code review ACK 84d295e, but I'm fuzzy on motivation for this change. I can vaguely see how not including full utxo transaction and signing offline might not be safe, but not sure what in segwit v1 will make it safe. Would be good to clarify this in one of the TODO comments.

src/rpc/rawtransaction.cpp

ryanofsky · 2020-06-30T18:20:24Z

src/psbt.cpp

@@ -136,8 +136,8 @@ void PSBTInput::Merge(const PSBTInput& input)
 {
    if (!non_witness_utxo && input.non_witness_utxo) non_witness_utxo = input.non_witness_utxo;
    if (witness_utxo.IsNull() && !input.witness_utxo.IsNull()) {
+        // TODO: For segwit v1, we will want to clear out the non-witness utxo when setting a witness one. For v0 and non-segwit, this is not safe


In commit "psbt: always put a non_witness_utxo and don't remove it" (4600479)

Can this comment be elaborated a little? I haven't been following this closely and don't know what in segwit v1 makes it safe to clear the non_witness_utxo transaction when signing offline.

2b79ac7 Clean up separated ban/discourage interface (Pieter Wuille) 0477348 Replace automatic bans with discouragement filter (Pieter Wuille) e7f06f9 test: remove Cirrus CI FreeBSD job (fanquake) eb6b82a qa: Test concurrent wallet loading (João Barbosa) c9b49d2 wallet: Handle concurrent wallet loading (João Barbosa) cf0b5a9 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow) 3228b59 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow) ed5ec30 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow) 68e0e6f rpc: show both UTXOs in decodepsbt (Andrew Chow) 27786d0 trivial: Suggested cleanups to surrounding code (Russell Yanofsky) 654420d wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) febebc4 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) 5c7151a gui: update Qt base translations for macOS release (fanquake) c219d21 build: improved output of configure for build OS (sachinkm77) 0596a6e util: Don't reference errno when pthread fails. (MIZUTA Takeshi) Pull request description: Currently backports the following to the 0.20 branch: * #18700 - Fix locking on WSL using flock instead of fcntl * #18982 - wallet: Minimal fix to restore conflicted transaction notifications * #19059 - gui: update Qt base translations for macOS release * #19152 - build: improve build OS configure output * #19194 - util: Don't reference errno when pthread fails. * #19215 - psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs * #19219 - Replace automatic bans with discouragement filter * #19300 - wallet: Handle concurrent wallet loading ACKs for top commit: amitiuttarwar: ACK 0477348 2b79ac7 by comparing to original changes, double checking the diff sipa: utACK 2b79ac7 laanwj: ACK 2b79ac7 Tree-SHA512: e5eb31d08288fa4a6e8aba08e60b16ce1988f14f249238b1cfd18ab2c8f6fcd9f07e3c0c491d32d2361fca26e3037fb0374f37464bddcabecea29069d6737539

bitcoinhodler · 2020-07-14T15:42:00Z

Is there any way to get walletprocesspsbt not to add the non_witness_utxo now?

My use case is offline signing of PSBTs via printed QR codes, and this makes that much more difficult since the PSBTs can be so much bigger now.

I understand why Trezor requires this, but I'm not concerned about that attack vector.

Sjors · 2020-07-14T15:46:18Z

@bitcoinhodler that could be achieved by adding an argument to that RPC call. For the GUI that would cause additional clutter, but perhaps an "advanced" section in the Send or PSBT dialog is useful anyway.

ryanofsky · 2020-07-14T21:53:41Z

src/rpc/rawtransaction.cpp

@@ -1121,7 +1122,9 @@ UniValue decodepsbt(const JSONRPCRequest& request)
            ScriptToUniv(txout.scriptPubKey, o, true);
            out.pushKV("scriptPubKey", o);
            in.pushKV("witness_utxo", out);
-        } else if (input.non_witness_utxo) {


In commit "rpc: show both UTXOs in decodepsbt" (72f6bec)

Note: Bug here is fixed in #19517 (changing else if to if can allow total_in to be incremented twice with the same amount)

As described in CVE-2020-14199 it is unsafe to only rely on witness UTXO information when signing. Hardware wallets fixed this by also requiring the full non-witness UTXO to be present for a witness input. To be compatible with those newer hardware wallet firmware, we need to remove the sanity checks that disallowed setting witness and non-witness UTXOs at the same time. See bitcoin/bitcoin#19215 for comparison which removed the sanity checks in Bitcoin Core.

7c1c153 doc: Update 0.20.1 release notes with psbt changes (Andrew Chow) Pull request description: #19215 was missing from the list. Also felt it was important to mention this change. Top commit has no ACKs. Tree-SHA512: b795cf73954ff493747a793039918a5e19c377d9325e6156a8e23ba8f510af12f48b2d63854f57d482640531a865190f1fe1ece0c78a5e45d6926f9533c6d695

Github-Pull: #19215 Rebased-From: 72f6bec

Github-Pull: #19215 Rebased-From: 5279d8b

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: #19215 Rebased-From: 4600479

Github-Pull: #19215 Rebased-From: 84d295e

Github-Pull: bitcoin#19215 Rebased-From: 72f6bec

Github-Pull: bitcoin#19215 Rebased-From: 5279d8b

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: bitcoin#19215 Rebased-From: 4600479

Github-Pull: bitcoin#19215 Rebased-From: 84d295e

See: bitcoin/bitcoin#19215 As long as the user doesn't sign twice, the security issue fixed by that PR is not an issue. In Glacier, it would be really, really hard to accidentally sign two subtly different transactions. Without this change, all the self-checking PSBT tests would start failing in Bitcoin Core 0.21, because online_regtest_wallet.py expects the PSBTs to be the same as it created last time. Instead they will now include both witness and non-witness UTXO data.

See: bitcoin/bitcoin#19215 As long as the user doesn't sign twice, the security issue fixed by that PR is not an issue. In Glacier, it would be really, really hard to accidentally sign two subtly different transactions. Without this change, all the self-checking PSBT tests would start failing in Bitcoin Core 0.21, because online_regtest_wallet.py expects the PSBTs to be the same as it created last time. Instead they would now include both witness and non-witness UTXO data.

…gwit inputs Summary: Backport of core [[bitcoin/bitcoin#19215 | PR19215]]. The relevant commits are: - bitcoin/bitcoin@5279d8b - bitcoin/bitcoin@84d295e Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9128

fanquake added the PSBT label Jun 8, 2020

achow101 force-pushed the psbt-segwit-fixes branch from 7215431 to 836d6fc Compare June 8, 2020 23:35

achow101 mentioned this pull request Jun 8, 2020

Updates for new Trezor and Ledger firmware bitcoin-core/HWI#340

Merged

DrahtBot mentioned this pull request Jun 9, 2020

[BIP 174] PSBT version, proprietary, and xpub fields #17034

Merged

jonatack mentioned this pull request Jun 9, 2020

Newsletters: add 101 (2020-06-10) bitcoinops/bitcoinops.github.io#417

Merged

3 tasks

luke-jr approved these changes Jun 11, 2020

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

rpc: show both UTXOs in decodepsbt

1b408b9

Github-Pull: bitcoin#19215 Rebased-From: 2789417

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

psbt: Allow both non_witness_utxo and witness_utxo

160d9e5

Github-Pull: bitcoin#19215 Rebased-From: 8b7d74f

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

psbt: always put a non_witness_utxo and don't remove it

d0ca016

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: bitcoin#19215 Rebased-From: a656eb3

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

tests: Check that segwit inputs in psbt have both UTXO types

b2fb092

Github-Pull: bitcoin#19215 Rebased-From: 836d6fc

Sjors mentioned this pull request Jun 17, 2020

travis: spurious feature_notifications.py failure #19311

Closed

SomberNight mentioned this pull request Jun 22, 2020

Issue importing multi-sig PSBT from electrum into bitcoin core & LND spesmilo/electrum#6257

Closed

DrahtBot added the Needs rebase label Jun 24, 2020

achow101 added 4 commits June 24, 2020 16:31

rpc: show both UTXOs in decodepsbt

72f6bec

psbt: Allow both non_witness_utxo and witness_utxo

5279d8b

psbt: always put a non_witness_utxo and don't remove it

4600479

Offline signers will always need a non_witness_utxo so make sure it is there.

tests: Check that segwit inputs in psbt have both UTXO types

84d295e

achow101 force-pushed the psbt-segwit-fixes branch from 836d6fc to 84d295e Compare June 24, 2020 20:36

DrahtBot removed the Needs rebase label Jun 24, 2020

SomberNight mentioned this pull request Jun 25, 2020

Invalid PSBT? (bip143 sighash vulnerability; coldcard psbt root fingerprint and derivation prefix missing) spesmilo/electrum#6270

Open

maflcko added the Needs backport (0.20) label Jun 29, 2020

maflcko added this to the 0.20.1 milestone Jun 29, 2020

ryanofsky approved these changes Jun 30, 2020

View reviewed changes

sipa mentioned this pull request Jul 14, 2020

psbt: Increment input value sum only once per UTXO in decodepsbt #19517

Merged

ryanofsky reviewed Jul 14, 2020

View reviewed changes

bitcoinhodler mentioned this pull request Jul 15, 2020

decodepsbt fee overstated since #19215 #19516

Closed

guggero mentioned this pull request Jul 20, 2020

psbt: restore compatibility with wallets that patch CVE-2020-14199 btcsuite/btcutil#178

Merged

achow101 mentioned this pull request Jul 31, 2020

doc: Update 0.20.1 release notes with psbt changes #19636

Merged

Fonta1n3 mentioned this pull request Aug 3, 2020

Bitcoin Core 0.20.1 not finalizing my psbt (not recognizing as complete) - breaks my wallet #19650

Closed

LeoComandini mentioned this pull request Aug 4, 2020

PSBT updates, continued ElementsProject/libwally-core#211

Merged

RCasatta mentioned this pull request Aug 12, 2020

add CI job with bitcoin 0.20.1 RCasatta/firma#59

Closed

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

rpc: show both UTXOs in decodepsbt

d52d8d1

Github-Pull: #19215 Rebased-From: 72f6bec

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

psbt: Allow both non_witness_utxo and witness_utxo

58f36f9

Github-Pull: #19215 Rebased-From: 5279d8b

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

psbt: always put a non_witness_utxo and don't remove it

d4c7be5

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: #19215 Rebased-From: 4600479

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

tests: Check that segwit inputs in psbt have both UTXO types

3d7834c

Github-Pull: #19215 Rebased-From: 84d295e

azuchi mentioned this pull request Oct 20, 2020

psbt.to_base64 version v0.5.0 is different from version v0.4.0 chaintope/bitcoinrb#59

Closed

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

rpc: show both UTXOs in decodepsbt

aeb0bac

Github-Pull: bitcoin#19215 Rebased-From: 72f6bec

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

psbt: Allow both non_witness_utxo and witness_utxo

ccb7adf

Github-Pull: bitcoin#19215 Rebased-From: 5279d8b

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

psbt: always put a non_witness_utxo and don't remove it

6fb9cce

Offline signers will always need a non_witness_utxo so make sure it is there. Github-Pull: bitcoin#19215 Rebased-From: 4600479

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

tests: Check that segwit inputs in psbt have both UTXO types

b944eef

Github-Pull: bitcoin#19215 Rebased-From: 84d295e

niftynei mentioned this pull request Oct 21, 2020

fix: bitcoind version breaks some recent psbt tests ElementsProject/lightning#4144

Closed

Platinumwrist referenced this pull request in syscoin/syscoin Oct 25, 2020

doc: Add historical release notes for 0.20.1

05e2740

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs #19215

psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs #19215

Uh oh!

achow101 commented Jun 8, 2020

Uh oh!

DrahtBot commented Jun 8, 2020

Uh oh!

Rspigler commented Jun 9, 2020

Uh oh!

achow101 commented Jun 9, 2020

Uh oh!

luke-jr left a comment

Uh oh!

Sjors commented Jun 17, 2020 •

edited

Loading

Uh oh!

sipa commented Jun 29, 2020

Uh oh!

maflcko commented Jun 29, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

ryanofsky Jun 30, 2020

Uh oh!

bitcoinhodler commented Jul 14, 2020

Uh oh!

Sjors commented Jul 14, 2020

Uh oh!

ryanofsky Jul 14, 2020

Uh oh!

Uh oh!

psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs #19215

psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs #19215

Uh oh!

Conversation

achow101 commented Jun 8, 2020

Uh oh!

DrahtBot commented Jun 8, 2020

Conflicts

Uh oh!

Rspigler commented Jun 9, 2020

Uh oh!

achow101 commented Jun 9, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 29, 2020

Uh oh!

maflcko commented Jun 29, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

bitcoinhodler commented Jul 14, 2020

Uh oh!

Sjors commented Jul 14, 2020

Uh oh!

ryanofsky Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 17, 2020 •

edited

Loading