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. DrahtBot commented at 11:58 pm on June 8, 2020: member

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

    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.

  5. 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?
  6. 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.

  7. luke-jr approved
  8. luke-jr commented at 4:31 am on June 11, 2020: member
    utACK
  9. luke-jr referenced this in commit 1b408b970b on Jun 12, 2020
  10. luke-jr referenced this in commit 160d9e5b51 on Jun 12, 2020
  11. luke-jr referenced this in commit d0ca01633e on Jun 12, 2020
  12. luke-jr referenced this in commit b2fb092d6f on Jun 12, 2020
  13. 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
  14. achow101 referenced this in commit 2ce8734a5e on Jun 22, 2020
  15. DrahtBot added the label Needs rebase on Jun 24, 2020
  16. rpc: show both UTXOs in decodepsbt 72f6bec1da
  17. psbt: Allow both non_witness_utxo and witness_utxo 5279d8bc07
  18. 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
  19. tests: Check that segwit inputs in psbt have both UTXO types 84d295e513
  20. achow101 force-pushed on Jun 24, 2020
  21. DrahtBot removed the label Needs rebase on Jun 24, 2020
  22. laanwj added this to the "PRs" column in a project
  23. sipa commented at 3:52 pm on June 29, 2020: member
    utACK. I think we’ll want this for 0.20.1
  24. MarcoFalke added the label Needs backport (0.20) on Jun 29, 2020
  25. MarcoFalke added this to the milestone 0.20.1 on Jun 29, 2020
  26. MarcoFalke commented at 5:25 pm on June 29, 2020: member
    @luke-jr @Sjors mind to re-ACK?
  27. 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.
  28. 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.

  29. ryanofsky approved
  30. ryanofsky commented at 6:24 pm on June 30, 2020: member
    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.
  31. 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.

  32. 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)
  33. 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
  34. ryanofsky approved
  35. ryanofsky commented at 8:09 pm on July 2, 2020: member
    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
  36. meshcollider commented at 8:51 pm on July 2, 2020: contributor
    utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3
  37. meshcollider merged this on Jul 2, 2020
  38. meshcollider closed this on Jul 2, 2020
  39. fanquake referenced this in commit 68e0e6f852 on Jul 3, 2020
  40. fanquake referenced this in commit ed5ec30804 on Jul 3, 2020
  41. fanquake referenced this in commit 3228b59b17 on Jul 3, 2020
  42. fanquake referenced this in commit cf0b5a933d on Jul 3, 2020
  43. fanquake commented at 1:38 am on July 3, 2020: member
    Added to #19224 for backport.
  44. fanquake removed the label Needs backport (0.20) on Jul 3, 2020
  45. azuchi referenced this in commit 713c6034cf on Jul 10, 2020
  46. fanquake referenced this in commit 01c563708f on Jul 10, 2020
  47. 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.

  48. 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.
  49. 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)

  50. meshcollider moved this from the "PRs" to the "Done" column in a project
  51. guggero referenced this in commit c5f199e40f on Jul 20, 2020
  52. laanwj referenced this in commit bf0dc356ac on Aug 1, 2020
  53. backpacker69 referenced this in commit d52d8d1388 on Sep 8, 2020
  54. backpacker69 referenced this in commit 58f36f9847 on Sep 8, 2020
  55. backpacker69 referenced this in commit d4c7be5db6 on Sep 8, 2020
  56. backpacker69 referenced this in commit 3d7834c846 on Sep 8, 2020
  57. Bushstar referenced this in commit aeb0bac408 on Oct 21, 2020
  58. Bushstar referenced this in commit ccb7adf8c8 on Oct 21, 2020
  59. Bushstar referenced this in commit 6fb9ccec1e on Oct 21, 2020
  60. Bushstar referenced this in commit b944eef98e on Oct 21, 2020
  61. bitcoinhodler referenced this in commit 83a29b3630 on Oct 25, 2020
  62. bitcoinhodler referenced this in commit c526d68480 on Oct 25, 2020
  63. bitcoinhodler referenced this in commit 1245fb3758 on Oct 25, 2020
  64. Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020
  65. bitcoinhodler referenced this in commit fea0a15f2d on Oct 25, 2020
  66. Fabcien referenced this in commit 94289404b7 on Feb 2, 2021
  67. DrahtBot locked this on Feb 15, 2022


achow101 DrahtBot Rspigler luke-jr Sjors sipa MarcoFalke ryanofsky meshcollider fanquake bitcoinhodler

Labels
PSBT

Milestone
0.20.1

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: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me