[psbt] Convert non-witness UTXOs to witness if witness sig created #14197

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:psbt-utxos changing 2 files +51 −1

achow101 commented at 4:15 AM on September 11, 2018: member

If a witness signature was created when a non-witness UTXO is used, convert the non-witness UTXO to a witness one.

Port of #14196 to master.
achow101 force-pushed on Sep 11, 2018

in src/script/sign.cpp:279 in f03eb813df outdated

 275 | @@ -276,6 +276,13 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
 276 |      // Verify that a witness signature was produced in case one was required.
 277 |      if (require_witness_sig && !sigdata.witness) return false;
 278 |      input.FromSignatureData(sigdata);
 279 | +

fanquake commented at 5:20 AM on September 11, 2018:

Travis is complaining about whitespace here.

achow101 commented at 6:54 PM on September 11, 2018:

Fixed

achow101 force-pushed on Sep 11, 2018
laanwj added this to the "Blockers" column in a project

gmaxwell commented at 4:48 PM on September 14, 2018: contributor

ACK
sipa commented at 1:39 AM on September 16, 2018: member

utACK 52d9adcdfbf545cb9a9facb482cb3673578abfa0. I'll test this soon.

in src/script/sign.cpp:283 in 52d9adcdfb outdated

 276 | @@ -277,6 +277,13 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
 277 |      if (require_witness_sig && !sigdata.witness) return false;
 278 |      input.FromSignatureData(sigdata);
 279 |  
 280 | +    if (sigdata.witness) {
 281 | +        // Convert the non-witness utxo to witness
 282 | +        if (input.witness_utxo.IsNull() && input.non_witness_utxo) {
 283 | +            input.witness_utxo = input.non_witness_utxo->vout[tx.vin[index].prevout.n];

promag commented at 2:08 PM on September 17, 2018:

IIUC this could be

assert(!utxo.IsNull());
input.witness_utxo = utxo;

achow101 commented at 7:26 PM on September 22, 2018:

Indeed. Done.

sipa commented at 10:59 PM on September 20, 2018: member

Here is a test that fails before and succeeds after this PR: https://github.com/sipa/bitcoin/commit/ce238f2aff3253cdde88d0f9b995b7e0016cb52b
sipa commented at 5:53 PM on September 22, 2018: member

@achow101 Feel free to cherry-pick the test into this PR.

That RPC test is a reproduction of what I did to notice the issue in the first place, so Tested ACK from me.
achow101 force-pushed on Sep 22, 2018
achow101 commented at 7:26 PM on September 22, 2018: member

I've added @sipa's test
achow101 force-pushed on Sep 22, 2018

Convert non-witness UTXOs to witness if witness sig created

If a witness signature was created when a non-witness UTXO is used,
convert the non-witness UTXO to a witness one.

f8c1714634

Add test for conversion from non-witness to witness UTXO 862d159d63
achow101 force-pushed on Sep 22, 2018
DrahtBot commented at 8:04 PM on September 22, 2018: contributor
Reviewers, this pull request conflicts with the following ones:
- #14588 (Refactor PSBT signing logic to enforce invariant and fix signing bug by gwillen)
- #13932 (Additional utility RPCs for PSBT 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.
DrahtBot cross-referenced this on Sep 22, 2018 from issue Additional utility RPCs for PSBT by achow101
sipa commented at 8:26 PM on September 23, 2018: member

ACK 862d159d635c1de219d94e030b186a745fe28eb9

in test/functional/rpc_psbt.py:9 in 862d159d63

   5 | @@ -6,7 +6,7 @@
   6 |  """
   7 |  
   8 |  from test_framework.test_framework import BitcoinTestFramework
   9 | -from test_framework.util import assert_equal, assert_raises_rpc_error, find_output
  10 | +from test_framework.util import assert_equal, assert_raises_rpc_error, find_output, disconnect_nodes, connect_nodes_bi, sync_blocks

promag commented at 4:02 PM on September 26, 2018:

nit, nit, nit, sort.

promag commented at 4:09 PM on September 26, 2018: member

utACK 862d159.
DrahtBot commented at 11:23 AM on September 28, 2018: contributor



Coverage Change (pull 14197) Reference (master)

Lines +0.0163 % 87.0361 %

Functions +0.1081 % 84.1130 %

Branches -0.0057 % 51.5451 %
AdamISZ cross-referenced this on Oct 1, 2018 from issue PSBT BIP 174 implementation by AdamISZ
AdamISZ referenced this in commit fcb0c6b68b on Oct 18, 2018
practicalswift cross-referenced this on Oct 22, 2018 from issue Enable flake8 rule E225 which checks for missing whitespace around op… by jbampton
DrahtBot cross-referenced this on Oct 27, 2018 from issue Refactor PSBT signing logic to enforce invariant and fix signing bug by gwillen
laanwj commented at 3:10 PM on November 1, 2018: member

utACK 862d159d635c1de219d94e030b186a745fe28eb9
laanwj added the label RPC/REST/ZMQ on Nov 1, 2018
laanwj merged this on Nov 1, 2018
laanwj closed this on Nov 1, 2018
laanwj referenced this in commit f6df989842 on Nov 1, 2018
laanwj removed this from the "Blockers" column in a project

sipa cross-referenced this on Nov 22, 2018 from issue PSBT backports to 0.17 by sipa
fanquake commented at 2:30 PM on November 30, 2018: member

862d159 will be backported in #14780.
sipa referenced this in commit 7bee41452b on Dec 3, 2018
MarcoFalke referenced this in commit 5d12143c73 on Dec 5, 2018
bitcoin locked this on Sep 8, 2021

Coverage	Change (pull 14197)	Reference (master)
Lines	+0.0163 %	87.0361 %
Functions	+0.1081 %	84.1130 %
Branches	-0.0057 %	51.5451 %

Contributors

Labels

Linked (view graph)

#14196 [0.17][psbt] always drop the unnecessary utxo and convert non-witness utxo to witness when necessary #14534 Enable flake8 rule E225 which checks for missing whitespace around op…#14780 PSBT backports to 0.17