PSBT backports to 0.17

sipa commented at 2:21 am on November 22, 2018: member

This is a backport of #14588, #14377, and #14197’s test to 0.17.

sipa commented at 2:21 am on November 22, 2018: member

Please review, @achow101 and @gwillen. All tests from the original PRs are included, and all commits pass, though some code changes may have been missed or ended up in the wrong commit.

fanquake added the label Backport on Nov 22, 2018

fanquake added this to the milestone 0.17.1 on Nov 22, 2018

gmaxwell commented at 3:55 am on November 22, 2018: contributor

Concept ACK

gwillen commented at 5:16 am on November 22, 2018: contributor

ACK on my changes appearing correctly-included.

MarcoFalke commented at 6:11 pm on November 22, 2018: member

nit: Are these cherry-picks? It might help to include the id they were cherry-picked from.

See for example https://github.com/bitcoin-core/bitcoin-maintainer-tools#backport which might do just that (haven’t tried it)

sipa commented at 6:36 pm on November 22, 2018: member

@MarcoFalke I created it by taking the merged #14588, rebasing it on top of the 0.17 branch, and removing all the commits except the ones from the 3 PRs listed in the description. Some commits needed nontrivial changes to work on top of the 0.17 (which lacks some of the refactors that happened in master). They’re not clean cherry-picks.

Is there any marker or tag I need to put on the PR or commits so it gets picked up by the release notes scripts?

DrahtBot added the label Needs rebase on Dec 1, 2018

laanwj commented at 11:28 am on December 2, 2018: member

Is there any marker or tag I need to put on the PR or commits so it gets picked up by the release notes scripts?

yes: Github-Pull: [#1234](/bitcoin-bitcoin/1234/) There’s also Rebased-From: <commitid> if the commit is directly based on another one on the master branch,this is not used by the scripts at the moment but may help other tooling as well as later troubleshooting.

sipa force-pushed on Dec 3, 2018

sipa commented at 6:29 pm on December 3, 2018: member

Rebased.

check that a separator is found for psbt inputs, outputs, and global map

Github-Pull: #14377
Rebased-From: 4fb3388db95f408566e43ebb9736842cfbff0a7d

a3fe125490

More concise conversion of CDataStream to string

Use .str() instead of .data() and .size() when converting CDataStream to
a string. Uses std::string, avoiding conversion to a C string.

Github-Pull: #14588
Rebased-From: fe5d22bc676f158e8d567d71edb3451118759d62

cfdd6b2f6c

Remove redundant txConst parameter to FillPSBT

Github-Pull: #14588
Rebased-From: 4f3f5cb4b142f0fcb36241fa33b52a257901dbee

a9eab081d5

New PartiallySignedTransaction constructor from CTransction

New constructor that creates a PartiallySignedTransaction from a
CTransaction, automatically sizing the inputs and outputs vectors for
convenience.

Github-Pull: #14588
Rebased-From: 65166d4cf828909dc4bc49dd68a58103d015f1fd

70ee1f8709

Add bool PSBTInputSigned

Refactor out a "PSBTInputSigned" function to check if a PSBT is signed,
for use in subsequent commits.

Also improve a related comment.

GitHub-Pull: #14588
Rebased-From: 53e6fffb8f5b10f94708d33d667a67cb91c2d09d

39ece4fc28

Simplify arguments to SignPSBTInput

Remove redundant arguments to SignPSBTInput -- since it needs several
bits of the PartiallySignedTransaction, pass in a reference instead of
doing it piecemeal. This saves us having to pass in both a PSBTInput and
its index, as well as having to pass in the CTransaction. Also avoid
redundantly passing the sighash_type, which is contained in the
PSBTInput already.

Github-Pull: #14588
Rebased-From: 0f5bda2bd941686620ef0eb90bd7ed973cc7ef73

ad94165db9

Refactor PSBTInput signing to enforce invariant

Refactor the process of PSBTInput signing to enforce the invariant that
a PSBTInput always has _either_ a witness_utxo or a non_witness_utxo,
never both.

This simplifies the logic of SignPSBTInput slightly, since it no longer
has to deal with the "both" case. When calling it, we now give it, in
order of preference: (1) whichever of the utxo fields was already
present in the PSBT we received, or (2) if neither, the
non_witness_utxo field, which is just a copy of the input transaction,
which we get from the wallet.

SignPSBTInput no longer has to remove one of the two fields; instead, it
will check if we have a witness signature, and if so, it will replace
the non_witness_utxo with the witness_utxo (which is smaller, as it is
just a copy of the output being spent.)

Add PSBTInput::IsSane checks in two more places, which checks for
both utxo fields being present; we will now give an RPC error early on
if we are supplied such a malformed PSBT to fill in.

Also add a check to FillPSBT, to avoid touching any input that is
already signed. (This is now redundant, since we should no longer
potentially harm an already-signed input, but it's harmless.)

fixes #14473

Github-Pull: #14588

db445d4e5a

Add regression test for PSBT signing bug #14473

Github-Pull: #14588
Rebased-From: e13fea975d5e4ae961faba36379a1cdaf9e50c1c

ff56bb9b44

Add test for conversion from non-witness to witness UTXO

Github-Pull: #14197
Rebased-From: 862d159d635c1de219d94e030b186a745fe28eb9

7bee41452b

sipa force-pushed on Dec 3, 2018

sipa commented at 6:39 pm on December 3, 2018: member

Added Github-Pull: and Rebased-From: annotations

DrahtBot removed the label Needs rebase on Dec 3, 2018

gmaxwell commented at 6:46 am on December 5, 2018: contributor

kinda tested ACK.

Sjors commented at 9:51 am on December 5, 2018: member

tACK 7bee414

I reviewed it along side with the originals, which I hadn’t seen before, so that’s a fresh perspective :-)

meshcollider commented at 10:24 am on December 5, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14780/commits/7bee41452bee4d10ab075999580853bdc3431e8b

meshcollider requested review from achow101 on Dec 5, 2018

MarcoFalke merged this on Dec 5, 2018

MarcoFalke closed this on Dec 5, 2018

MarcoFalke referenced this in commit 5d12143c73 on Dec 5, 2018

promag commented at 3:36 pm on December 5, 2018: member

ACK 7bee414, indeed not a clean rebase, at least for me FillPSBT was the most hard.

MarcoFalke locked this on Sep 8, 2021

PSBT backports to 0.17 #14780