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

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:psbt-segwit-fixes changing 8 files +22 −63
  1. achow101 commented at 11:34 PM on June 8, 2020: member

    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.

  2. fanquake added the label PSBT on Jun 8, 2020
  3. achow101 force-pushed on Jun 8, 2020
  4. achow101 cross-referenced this on Jun 8, 2020 from issue Updates for new Trezor and Ledger firmware by achow101
  5. DrahtBot commented at 11:58 PM on June 8, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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.

  6. Rspigler commented at 5:27 AM on June 9, 2020: contributor

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

  7. achow101 commented at 6:03 AM on June 9, 2020: member

    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.

  8. DrahtBot cross-referenced this on Jun 9, 2020 from issue [BIP 174] PSBT version, proprietary, and xpub fields by achow101
  9. jonatack cross-referenced this on Jun 9, 2020 from issue Newsletters: add 101 (2020-06-10) by harding
  10. luke-jr approved
  11. luke-jr commented at 4:31 AM on June 11, 2020: member

    utACK

  12. luke-jr referenced this in commit 1b408b970b on Jun 12, 2020
  13. luke-jr referenced this in commit 160d9e5b51 on Jun 12, 2020
  14. luke-jr referenced this in commit d0ca01633e on Jun 12, 2020
  15. luke-jr referenced this in commit b2fb092d6f on Jun 12, 2020
  16. Sjors cross-referenced this on Jun 17, 2020 from issue travis: spurious feature_notifications.py failure by Sjors
  17. Sjors commented at 6:45 PM on June 17, 2020: member

    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 https://github.com/bitcoin-core/HWI/pull/340

  18. achow101 referenced this in commit 2ce8734a5e on Jun 22, 2020
  19. SomberNight cross-referenced this on Jun 22, 2020 from issue Issue importing multi-sig PSBT from electrum into bitcoin core & LND by kornpow
  20. DrahtBot added the label Needs rebase on Jun 24, 2020
  21. rpc: show both UTXOs in decodepsbt 72f6bec1da
  22. psbt: Allow both non_witness_utxo and witness_utxo 5279d8bc07
  23. psbt: always put a non_witness_utxo and don't remove it
    Offline signers will always need a non_witness_utxo so make sure it is
there.
    4600479058
  24. tests: Check that segwit inputs in psbt have both UTXO types 84d295e513
  25. achow101 force-pushed on Jun 24, 2020
  26. DrahtBot removed the label Needs rebase on Jun 24, 2020
  27. SomberNight cross-referenced this on Jun 25, 2020 from issue Invalid PSBT? (bip143 sighash vulnerability; coldcard psbt root fingerprint and derivation prefix missing) by jlopp
  28. laanwj added this to the "PRs" column in a project
  29. sipa commented at 3:52 PM on June 29, 2020: member

    utACK. I think we'll want this for 0.20.1

  30. MarcoFalke added the label Needs backport (0.20) on Jun 29, 2020
  31. MarcoFalke added this to the milestone 0.20.1 on Jun 29, 2020
  32. MarcoFalke commented at 5:25 PM on June 29, 2020: member

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

  33. in src/rpc/rawtransaction.cpp:1141 in 72f6bec1da outdated 
    1134 | @@ -1132,7 +1135,9 @@ UniValue decodepsbt(const JSONRPCRequest& request)
1135 |                  // Hack to just not show fee later
1136 |                  have_all_utxos = false;
1137 |              }
1138 | -        } else {
1139 | +            have_a_utxo = true;
1140 | +        }
1141 | +        if (!have_a_utxo) {
1142 |              have_all_utxos = false;
    ryanofsky commented at 5:57 PM on June 30, 2020:

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

    Note: Commit description mentions allowing both "witness_utxo" and "non_witness_utxo" fields, but it doesn't mention the slight change in behavior here.

    After this change have_all_utxos is true unless there's an input with no utxo information or an input with either "witness_utxo" or "non_witness_utxo" information containing an invalid amount.

    Before this change have_all_utxos used to be true if there was an input with both witness and non-witness utxo information and the witness amount was valid but the non-witness amount was invalid.

    I guess an effect of this is that fee may not be shown in some cases it would have been shown before.

    achow101 commented at 6:43 PM on June 30, 2020:

    Before this PR, it wouldn't have been possible to have an input with both witness and non-witness utxo information. Such a PSBT wouldn't even make it to this step.

  34. in src/psbt.cpp:139 in 4600479058 outdated 
     135 | @@ -136,8 +136,8 @@ void PSBTInput::Merge(const PSBTInput& input)
 136 |  {
 137 |      if (!non_witness_utxo && input.non_witness_utxo) non_witness_utxo = input.non_witness_utxo;
 138 |      if (witness_utxo.IsNull() && !input.witness_utxo.IsNull()) {
 139 | +        // 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
    ryanofsky commented at 6:20 PM on June 30, 2020:

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

    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.

  35. ryanofsky approved
  36. ryanofsky commented at 6:24 PM on June 30, 2020: contributor

    Code review ACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3, 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.

  37. sipa commented at 6:27 PM on June 30, 2020: member

    @ryanofsky Context: https://blog.trezor.io/details-of-firmware-updates-for-trezor-one-version-1-9-1-and-trezor-model-t-version-2-3-1-1eba8f60f2dd

    BIP341 (taproot, which will hopefully become witness v1) proposes signing all prevout amounts/scriptpubkeys in every (sighash_all) input, making this attack impossible.

  38. Sjors commented at 7:11 PM on July 2, 2020: member

    utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)

  39. in src/psbt.cpp:163 in 5279d8bc07 outdated 
     149 | @@ -158,18 +150,6 @@ void PSBTInput::Merge(const PSBTInput& input)
 150 |      if (final_script_witness.IsNull() && !input.final_script_witness.IsNull()) final_script_witness = input.final_script_witness;
 151 |  }
 152 |  
 153 | -bool PSBTInput::IsSane() const
 154 | -{
 155 | -    // Cannot have both witness and non-witness utxos
    ryanofsky commented at 8:02 PM on July 2, 2020:

    In commit "psbt: Allow both non_witness_utxo and witness_utxo" (5279d8bc07d601fe6a67ad665fbc7591fe73c7de):

    Previously we didn't allow both witness and non-witness utxos. Now we do allow them but we aren't checking if they are consistent. It seems like instead of removing the IsSane checks, it'd be better to keep them and verify non_witness_utxo->vout[prevout_index] == witness_utxo if both witness_utxo and non_witness_utxo are present.

    achow101 commented at 8:35 PM on July 2, 2020:

    I think we can add checks in a followup.

    I'm also uncertain about adding application logic checks that would disallow deserialization. The PSBT could be correctly formatted but contain the wrong UTXOs or scripts. We might still want to allow decodepsbt to show the PSBT, but not allow updating or signing it.

    meshcollider commented at 8:52 PM on July 2, 2020:

    Agree, the check would be nice but its not necessary for merge

  40. ryanofsky approved
  41. ryanofsky commented at 8:09 PM on July 2, 2020: contributor

    Code review re-ACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested #19215 (review) and maybe refer to https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-17, because I don't think intent of code is very clear currently

  42. meshcollider commented at 8:51 PM on July 2, 2020: contributor

    utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3

  43. meshcollider merged this on Jul 2, 2020
  44. meshcollider closed this on Jul 2, 2020
  45. fanquake referenced this in commit 68e0e6f852 on Jul 3, 2020
  46. fanquake referenced this in commit ed5ec30804 on Jul 3, 2020
  47. fanquake referenced this in commit 3228b59b17 on Jul 3, 2020
  48. fanquake referenced this in commit cf0b5a933d on Jul 3, 2020
  49. fanquake cross-referenced this on Jul 3, 2020 from issue [0.20] Backports by fanquake
  50. fanquake commented at 1:38 AM on July 3, 2020: member

    Added to #19224 for backport.

  51. fanquake removed the label Needs backport (0.20) on Jul 3, 2020
  52. RCasatta cross-referenced this on Jul 8, 2020 from issue Remove shrink of tx by RCasatta
  53. azuchi referenced this in commit 713c6034cf on Jul 10, 2020
  54. fanquake referenced this in commit 01c563708f on Jul 10, 2020
  55. bitcoinhodler commented at 3:42 PM on July 14, 2020: contributor

    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.

  56. Sjors commented at 3:46 PM on July 14, 2020: member

    @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.

  57. sipa cross-referenced this on Jul 14, 2020 from issue psbt: Increment input value sum only once per UTXO in decodepsbt by achow101
  58. in src/rpc/rawtransaction.cpp:1124 in 72f6bec1da outdated 
    1121 | @@ -1121,7 +1122,9 @@ UniValue decodepsbt(const JSONRPCRequest& request)
1122 |              ScriptToUniv(txout.scriptPubKey, o, true);
1123 |              out.pushKV("scriptPubKey", o);
1124 |              in.pushKV("witness_utxo", out);
1125 | -        } else if (input.non_witness_utxo) {
    ryanofsky commented at 9:53 PM on July 14, 2020:

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

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

  59. bitcoinhodler cross-referenced this on Jul 15, 2020 from issue `decodepsbt` fee overstated since #19215 by bitcoinhodler
  60. meshcollider moved this from the "PRs" to the "Done" column in a project
  61. guggero referenced this in commit c5f199e40f on Jul 20, 2020
  62. guggero cross-referenced this on Jul 20, 2020 from issue psbt: restore compatibility with wallets that patch CVE-2020-14199 by guggero
  63. achow101 cross-referenced this on Jul 31, 2020 from issue doc: Update 0.20.1 release notes with psbt changes by achow101
  64. laanwj referenced this in commit bf0dc356ac on Aug 1, 2020
  65. Fonta1n3 cross-referenced this on Aug 3, 2020 from issue Bitcoin Core 0.20.1 not finalizing my psbt (not recognizing as complete) - breaks my wallet by Fonta1n3
  66. LeoComandini cross-referenced this on Aug 4, 2020 from issue PSBT updates, continued by jgriffiths
  67. RCasatta cross-referenced this on Aug 12, 2020 from issue add CI job with bitcoin 0.20.1 by RCasatta
  68. backpacker69 referenced this in commit d52d8d1388 on Sep 8, 2020
  69. backpacker69 referenced this in commit 58f36f9847 on Sep 8, 2020
  70. backpacker69 referenced this in commit d4c7be5db6 on Sep 8, 2020
  71. backpacker69 referenced this in commit 3d7834c846 on Sep 8, 2020
  72. azuchi cross-referenced this on Oct 20, 2020 from issue `psbt.to_base64` version v0.5.0 is different from version v0.4.0 by longhoangwkm
  73. Bushstar referenced this in commit aeb0bac408 on Oct 21, 2020
  74. Bushstar referenced this in commit ccb7adf8c8 on Oct 21, 2020
  75. Bushstar referenced this in commit 6fb9ccec1e on Oct 21, 2020
  76. Bushstar referenced this in commit b944eef98e on Oct 21, 2020
  77. niftynei cross-referenced this on Oct 21, 2020 from issue fix: bitcoind version breaks some recent psbt tests by m-schmoock
  78. bitcoinhodler referenced this in commit 83a29b3630 on Oct 25, 2020
  79. bitcoinhodler referenced this in commit c526d68480 on Oct 25, 2020
  80. bitcoinhodler referenced this in commit 1245fb3758 on Oct 25, 2020
  81. Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020
  82. bitcoinhodler referenced this in commit fea0a15f2d on Oct 25, 2020
  83. Fabcien referenced this in commit 94289404b7 on Feb 2, 2021
  84. bitcoin locked this on Feb 15, 2022
Contributors
achow101DrahtBotRspiglerluke-jrSjorssipaMarcoFalkeryanofskymeshcolliderfanquakebitcoinhodler
Labels
PSBT

Milestone
0.20.1

Linked (view graph)
#19224 [0.20] Backports#19311 travis: spurious feature_notifications.py failure#19516 `decodepsbt` fee overstated since #19215#19517 psbt: Increment input value sum only once per UTXO in decodepsbt#19636 doc: Update 0.20.1 release notes with psbt changes#19650 Bitcoin Core 0.20.1 not finalizing my psbt (not recognizing as complete) - breaks my wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC