Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs #12136

achow101 · 2018-01-10T04:08:39Z

BIP 174 specifies a binary transaction format which contains the information necessary for a signer to produce signatures for the transaction and holds the signatures for an input while the input does not have a complete set of signatures.

BIP 174 is fully implemented in this pull request. It contains a struct for the a PSBT, serialization and deserialization functions, and a PSBT specific versions of ProduceSignature (SignPartialTransaction), SignStep(SignSigsOnly) and CombineSignatures(FinalizePartialTransaction`).

Currently PSBT functionality is limited to 4 new RPC calls, walletupdatepsbt, walletcreatepsbt, combinepsbt, and decodepsbt.

walletupdatepsbt updates a given PSBT with data from the wallet. For each input, it will attempt to add the proper UTXO, sign for the input, and finalize the input. It will also add any known redeem scripts, witness scripts, and public key derivation paths if they are known to the wallet.

walletcreatepsbt takes a network serialized raw transaction as produced by the *rawtransaction commands and converts it to a PSBT. Inputs are filled using information known to the wallet.

combinepsbt combines multiple PSBTs and finalizes them if possible.

decodepsbt decodes a PSBT and into a human readable format in order to more easily examine them. It is analogous to decoderawtransaction.

All of the test vectors currently in BIP 174 are also implemented.

jonasschnelli · 2018-01-10T07:47:17Z

src/script/sign.cpp

@@ -302,6 +381,218 @@ struct Stacks
 };
 }

+// Iterates through all inputs of the partially signed transaction and just produces signatures for what it can and adds them to the psbt partial sigs
+bool SignPartialTransaction(PartiallySignedTransaction& psbt, const CKeyStore* keystore, int nHashType)


SignPartialTransaction sounds after we have partial transactions. Suggest SignPartialSignedTransaction.

Changed to SignPartiallySignedTransaction

jonasschnelli · 2018-01-10T07:51:19Z

Great work!
General concept ack, though the PR is large and maybe there is a way to make smaller steps towards BIP174 (reduce of risks).

achow101 · 2018-01-10T21:27:28Z

I'm not sure what's causing the travis failure.

gmaxwell · 2018-01-11T20:37:42Z

Concept ACK!

dcousens · 2018-01-16T22:27:06Z

Concept ACK

laanwj · 2018-02-14T13:53:29Z

Neat! Concept ACK.

instagibbs · 2018-02-14T19:29:47Z

src/script/sign.cpp

+                switch (whichType)
+                {
+                case TX_PUBKEY:
+                    script_ret.push_back(psbt.inputs[i].partial_sigs.find(CPubKey(vSolutions[0]))->second);


this may segfault when there is no corresponding partial_sig, I believe this is the cause of your test failure.

instagibbs · 2018-02-20T16:49:46Z

src/wallet/rpcwallet.cpp

+        }
+        // Get public keys if hd is enabled
+        if (pwallet->IsHDEnabled()) {
+            if (type == TX_PUBKEYHASH) {


should be:

if (type == TX_PUBKEYHASH || type == TX_WITNESS_V0_KEYHASH) {

instagibbs · 2018-02-20T16:56:54Z

src/wallet/rpcwallet.cpp

+            // Add to inputs
+            psbtx.inputs.push_back(input);
+        }
+    }


this isn't mentioned in the spec anywhere, but passing along the the change output's pubkey(s)/hdpath(s)/redeemscript allows hww to understand change outputs.

I decided to make this an option instead of the default. I'll add tests for it later.

Should that be mentioned in the spec, then?

achow101 · 2018-02-20T23:08:21Z

Rebased and squashed fixup commits.

instagibbs · 2018-02-21T16:15:40Z

src/wallet/rpcwallet.cpp

@@ -4082,6 +4163,8 @@ UniValue walletcreatepsbt(const JSONRPCRequest& request)
                            "using fundrawtransaction.\n"
                            "\nArguments:\n"
                            "1. \"hexstring\"            (string, required) The hex string of the raw transaction\n"
+                            "2. \"include_output_info\"  (boolean, optional, default=false) If true, returns the PSBT with the redeem scripts, witness\n"


you need to edit number of allowed args, and include the optional one in parens above.

you also need to add it to the list of arguments that will be parsed as json rather than strings in rpc/client.cpp

Dang, I knew I forgot something. Fixed.

instagibbs · 2018-02-21T16:25:29Z

src/wallet/rpcwallet.cpp

+                if (type == TX_PUBKEYHASH || type == TX_WITNESS_V0_KEYHASH) {
+                    uint160 hash(solns[0]);
+                    CKeyID keyID(hash);
+                    if (!pwallet->HaveKey(keyID)) {


What we really care about is that we have the hdkeypath, not particularly that the key is present or not. I don't think these HaveKey checks are necessary since add_keypath_to_map will deal with that case specifically.

Right, fixed.

instagibbs

"ERROR: walletcreatepsbt argument 2 (named include_output_info in vRPCConvertParams) is not defined in dispatch table
ERROR: walletupdatepsbt argument 3 (named include_output_info in vRPCConvertParams) is not defined in dispatch table"

You need to add the new named args to the dispatch table

instagibbs · 2018-02-22T15:13:48Z

src/rpc/client.cpp

@@ -102,6 +102,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "fundrawtransaction", 1, "options" },
    { "fundrawtransaction", 2, "iswitness" },
    { "walletupdatepsbt", 2, "psbtformat"},
+    { "walletupdatepsbt", 3, "include_output_info"},
+    { "walletcreatepsbt", 2, "include_output_info"},


should be 1

instagibbs · 2018-03-06T14:13:38Z

light tACK

I have rebased the externalhd branch onto this PR, with minor modifications, for ledger support, combined with @achow101 's HWI repo for signing. https://github.com/instagibbs/bitcoin/tree/external_psbt

achow101 · 2018-06-10T00:18:15Z

Rebased on top of #13425 which simplifies the signer logic.

sipa · 2018-06-10T01:06:41Z

src/script/sign.cpp

+        CPubKey pubkey(vSolutions[0]);
+        keyID = pubkey.GetID();
+        if (!creator.CreateSig(provider, sig, keyID, scriptPubKey, sigversion)) return false;
+        sigdata.signatures.emplace(keyID, SigPair(pubkey, sig));


Why is this needed? Signing a TX_PUBKEY should always result in a full signature.

A PSBT does not have to create the final scriptSig even when a complete set of signatures is available. The user has the option to not finalize the scriptSigs even when they can be.

In that case you should also be able to deal with unfinalized signatures already present, and turn them into scriptSigs. In other words, you need to test whether sigdata.signatures contains a matching signature and reuse it. If that's the case, we're probably better off reintroducing SignatureDataSignatureCreator and SignatureDataSigningProvider, so these keys/script/signatures are automatically available everywhere.

Another interpretation is that TX_PUBKEY and TX_PUBKEYHASH etc just don't have "partial" forms - they're either fully signed or not, and as a result there never is anything to finalize.

We should discuss this on the mailing list as it requires changing the BIP.

For now, I have added a helper function for createSig which fetches and places signatures to and from the SignatureData.

sipa · 2018-06-10T01:06:54Z

src/script/sign.cpp

        CPubKey pubkey;
        GetPubKey(&provider, &sigdata, keyID, pubkey);
+        sigdata.signatures.emplace(keyID, SigPair(pubkey, sig));


araspitzu · 2018-06-11T11:03:38Z

src/script/sign.h

+                    if (in_globals) {
+                        num_ins = ReadCompactSize(s);
+                    } else {
+                        // Make sure that we are using inout indexes or this is the first input


small typo: Make sure that we are using input indexes or this is the first input

Sign1 and SignN are kind of redundant so remove them and inline their behavior into SignStep

achow101 · 2018-06-12T19:03:29Z

Updated the title to indicate that this is primarily serializations and RPCs.

In addition to having the scriptSig and scriptWitness, have SignatureData also be able to store just the signatures (pubkeys mapped to sigs) and scripts (script ids mapped to scripts). Also have DataFromTransaction be able to extract signatures and scripts from the scriptSig and scriptWitness of an input to put them in SignatureData. Adds a new SignatureChecker which takes a SignatureData and puts pubkeys and signatures into it when it successfully verifies a signature. Adds a new field in SignatureData which stores whether the SignatureData was complete. This allows us to also update the scriptSig and scriptWitness to the final one when updating a SignatureData with another one.

Instead of using CombineSignatures to create the final scriptSig or scriptWitness of an input, use ProduceSignature itself. To allow for ProduceSignature to place signatures, pubkeys, and scripts that it does not know about, we pass down the SignatureData to SignStep which pulls out the information that it needs from the SignatureData.

Removes CombineSignatures and replaces its use in tests with ProduceSignature to test the same behavior for ProduceSignature.

walletupdatepsbt takes a PSBT format transaction, updates the PSBT with any inputs related to this wallet, and signs the transaction walletcreatepsbt takes a raw network serialized transaction like what createrawtransaction and fundrawtransaction output and converts it to a PSBT using whatever information about the inputs it knows from the wallet. decodepsbt takes a PSBT and decodes it to JSON combinepsbt takes multiple PSBTs for the same tx and combines them.

luke-jr · 2018-06-16T22:58:38Z

If the only reason not to use protobufs is "another dependency", that's a bad reason to avoid it. Dependencies are better than reinventing the same thing... no reason to avoid them. (not to mention that protobuf is already a dependency)

sipa · 2018-06-16T23:22:22Z

@luke-jr They also have a non-unique encoding, and are too complicated for what we need. Protobuf is also not a dependency already of all (or even most) software (including hardware devices) we expect to implement this, while all of those already need to implement Bitcoin P2P-like serialization, which this extends.

The discussion for that probably belongs on the ML.

luke-jr · 2018-06-17T01:35:42Z

@sipa My goal was not to argue for a change in BIP 174, only to get a better explanation/rationale for why protobufs aren't used. :)

achow101 · 2018-06-24T17:36:00Z

Since BIP 174 is changing soon, I'm closing this for now.

020628e Tests for PSBT (Andrew Chow) a4b06fb Create wallet RPCs for PSBT (Andrew Chow) c27fe41 Create utility RPCs for PSBT (Andrew Chow) 8b5ef27 SignPSBTInput wrapper function (Andrew Chow) 58a8e28 Refactor transaction creation and transaction funding logic (Andrew Chow) e9d86a4 Methods for interacting with PSBT structs (Andrew Chow) 12bcc64 Add pubkeys and whether input was witness to SignatureData (Andrew Chow) 41c607f Implement PSBT Structures and un/serialization methods per BIP 174 (Andrew Chow) Pull request description: This Pull Request fully implements the [updated](bitcoin/bips#694) BIP 174 specification. It is based upon #13425 which implements the majority of the signing logic. BIP 174 specifies a binary transaction format which contains the information necessary for a signer to produce signatures for the transaction and holds the signatures for an input while the input does not have a complete set of signatures. This PR contains structs for PSBT, serialization, and deserialzation code. Some changes to `SignatureData` have been made to support detection of UTXO type and storing public keys. *** Many RPCs have been added to handle PSBTs. `walletprocesspsbt` takes a PSBT format transaction, updates the PSBT with any inputs related to this wallet, signs, and finalizes the transaction. There is also an option to not sign and just update. `walletcreatefundedpsbt` creates a PSBT from user provided data in the same form as createrawtransaction. It also funds the transaction and takes an options argument in the same form as `fundrawtransaction`. The resulting PSBT is blank with no input or output data filled in. It is analogous to a combination of `createrawtransaction` and `fundrawtransaction` `decodepsbt` takes a PSBT and decodes it to JSON. It is analogous to `decoderawtransaction` `combinepsbt` takes multiple PSBTs for the same tx and combines them. It is analogous to `combinerawtransaction` `finalizepsbt` takes a PSBT and finalizes the inputs. If all inputs are final, it extracts the network serialized transaction and returns that instead of a PSBT unless instructed otherwise. `createpsbt` is like `createrawtransaction` but for PSBTs instead of raw transactions. `convertpsbt` takes a network serialized transaction and converts it into a psbt. The resulting psbt will lose all signature data and an explicit flag must be set to allow transactions with signature data to be converted. *** This supersedes #12136 Tree-SHA512: 1ac7a79e5bc669933f0a6fcc93ded55263fdde9e8c144a30266b13ef9f62aacf43edd4cbca1ffbe003090b067e9643c9298c79be69d7c1b10231b32acafb6338

jonasschnelli reviewed Jan 10, 2018

View reviewed changes

nopara73 mentioned this pull request Jan 10, 2018

Evaluate BIP147 nopara73/ZeroLink#60

Open

achow101 force-pushed the psbt branch from 324f4e3 to 743ffc6 Compare January 11, 2018 19:31

achow101 force-pushed the psbt branch from 743ffc6 to 804d216 Compare January 11, 2018 21:11

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

fanquake added the Consensus label Jan 19, 2018

achow101 force-pushed the psbt branch 3 times, most recently from 12a88c4 to 55a7223 Compare February 2, 2018 01:28

instagibbs reviewed Feb 14, 2018

View reviewed changes

achow101 force-pushed the psbt branch from 7aca9a0 to 0314551 Compare February 15, 2018 04:39

instagibbs reviewed Feb 20, 2018

View reviewed changes

achow101 force-pushed the psbt branch from 0314551 to 9b2a685 Compare February 20, 2018 23:08

achow101 force-pushed the psbt branch 5 times, most recently from e5009df to 0582662 Compare February 21, 2018 00:56

instagibbs reviewed Feb 21, 2018

View reviewed changes

achow101 force-pushed the psbt branch 2 times, most recently from 4881490 to f259cd5 Compare February 22, 2018 05:49

instagibbs reviewed Feb 22, 2018

View reviewed changes

achow101 force-pushed the psbt branch from f259cd5 to 85fbed4 Compare February 22, 2018 15:58

achow101 force-pushed the psbt branch from cf0cb2e to 7b636e9 Compare June 10, 2018 00:17

sipa reviewed Jun 10, 2018

View reviewed changes

araspitzu reviewed Jun 11, 2018

View reviewed changes

achow101 force-pushed the psbt branch from 7b636e9 to bd68c20 Compare June 11, 2018 19:17

Inline Sign1 and SignN

51ed700

Sign1 and SignN are kind of redundant so remove them and inline their behavior into SignStep

achow101 changed the title ~~Implement BIP 174 Partially Signed Bitcoin Transactions~~ Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs Jun 12, 2018

achow101 added 3 commits June 14, 2018 10:42

Remove CombineSignatures and replace tests

3d2cbe6

Removes CombineSignatures and replaces its use in tests with ProduceSignature to test the same behavior for ProduceSignature.

achow101 added 4 commits June 14, 2018 14:26

Implement PSBT structures and un/serliazation methods per BIP 174

b7de8de

Implement a signer that takes an unserialized PSBT and signs it

2fa2655

Test the PSBT RPCs

9507467

achow101 force-pushed the psbt branch from bd68c20 to 9507467 Compare June 14, 2018 21:26

jeffrade mentioned this pull request Jun 15, 2018

Avoid "test_runner.py" script array #11964

Closed

araspitzu mentioned this pull request Jun 20, 2018

[WIP] Implement BIP174 ACINQ/bitcoin-lib#22

Closed

achow101 closed this Jun 24, 2018

achow101 mentioned this pull request Jun 28, 2018

BIP 174 PSBT Serializations and RPCs #13557

Merged

HashUnlimited mentioned this pull request Sep 11, 2018

BIP 174 PSCT Serializations and RPCs chaincoin/chaincoin#543

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs #12136

Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs #12136

Conversation

achow101 commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 10, 2018

achow101 commented Jan 10, 2018

gmaxwell commented Jan 11, 2018

dcousens commented Jan 16, 2018

laanwj commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Mar 6, 2018 • edited

achow101 commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Jun 12, 2018

luke-jr commented Jun 16, 2018 • edited

sipa commented Jun 16, 2018 • edited

luke-jr commented Jun 17, 2018

achow101 commented Jun 24, 2018

instagibbs commented Mar 6, 2018 •

edited

luke-jr commented Jun 16, 2018 •

edited

sipa commented Jun 16, 2018 •

edited