Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs

achow101 commented at 4:08 am on January 10, 2018: member

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.

in src/script/sign.cpp:385 in 324f4e3b83 outdated

380@@ -302,6 +381,218 @@ struct Stacks
381 };
382 }
383 
384+// 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
385+bool SignPartialTransaction(PartiallySignedTransaction& psbt, const CKeyStore* keystore, int nHashType)

jonasschnelli commented at 7:47 am on January 10, 2018:

SignPartialTransaction sounds after we have partial transactions. Suggest SignPartialSignedTransaction.

achow101 commented at 8:37 pm on April 5, 2018:

Changed to SignPartiallySignedTransaction

jonasschnelli commented at 7:51 am on January 10, 2018: contributor

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 commented at 9:27 pm on January 10, 2018: member

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

achow101 force-pushed on Jan 11, 2018

gmaxwell commented at 8:37 pm on January 11, 2018: contributor

Concept ACK!

achow101 force-pushed on Jan 11, 2018

dcousens commented at 10:27 pm on January 16, 2018: contributor

Concept ACK

fanquake added the label Consensus on Jan 19, 2018

achow101 force-pushed on Feb 1, 2018

achow101 force-pushed on Feb 2, 2018

laanwj commented at 1:53 pm on February 14, 2018: member

Neat! Concept ACK.

in src/script/sign.cpp:496 in 55a7223362 outdated

491+            bool found_pk = false;
492+            if (Solver(spk, whichType, vSolutions)) {
493+                switch (whichType)
494+                {
495+                case TX_PUBKEY:
496+                    script_ret.push_back(psbt.inputs[i].partial_sigs.find(CPubKey(vSolutions[0]))->second);

instagibbs commented at 7:29 pm on February 14, 2018:

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

achow101 commented at 3:11 am on February 15, 2018:

Fixed

achow101 force-pushed on Feb 15, 2018

in src/wallet/rpcwallet.cpp:3659 in 03145519c7 outdated

3654+            solns.clear();
3655+            Solver(witness_script, type, solns);
3656+        }
3657+        // Get public keys if hd is enabled
3658+        if (pwallet->IsHDEnabled()) {
3659+            if (type == TX_PUBKEYHASH) {

instagibbs commented at 4:49 pm on February 20, 2018:

should be:

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

achow101 commented at 11:08 pm on February 20, 2018:

Done.

in src/wallet/rpcwallet.cpp:4506 in 03145519c7 outdated

3685+
3686+        if (psbtx_blank) {
3687+            // Add to inputs
3688+            psbtx.inputs.push_back(input);
3689+        }
3690+    }

instagibbs commented at 4:56 pm on February 20, 2018:

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.

achow101 commented at 11:49 pm on February 20, 2018:

Done

achow101 commented at 0:07 am on February 21, 2018:

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

laanwj commented at 1:50 pm on May 31, 2018:

Should that be mentioned in the spec, then?

achow101 force-pushed on Feb 20, 2018

achow101 commented at 11:08 pm on February 20, 2018: member

Rebased and squashed fixup commits.

achow101 force-pushed on Feb 20, 2018

achow101 force-pushed on Feb 21, 2018

in src/wallet/rpcwallet.cpp:4699 in 0582662168 outdated

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

instagibbs commented at 4:15 pm on February 21, 2018:

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

instagibbs commented at 4:18 pm on February 21, 2018:

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

achow101 commented at 5:47 am on February 22, 2018:

Dang, I knew I forgot something. Fixed.

in src/wallet/rpcwallet.cpp:4010 in 0582662168 outdated

4005+            // Get public keys if hd is enabled
4006+            if (pwallet->IsHDEnabled()) {
4007+                if (type == TX_PUBKEYHASH || type == TX_WITNESS_V0_KEYHASH) {
4008+                    uint160 hash(solns[0]);
4009+                    CKeyID keyID(hash);
4010+                    if (!pwallet->HaveKey(keyID)) {

instagibbs commented at 4:25 pm on February 21, 2018:

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.

achow101 commented at 5:47 am on February 22, 2018:

Right, fixed.

achow101 force-pushed on Feb 22, 2018

in src/rpc/client.cpp:106 in f259cd513f outdated

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

instagibbs commented at 3:13 pm on February 22, 2018:

should be 1

achow101 commented at 3:58 pm on February 22, 2018:

fixed

instagibbs commented at 3:15 pm on February 22, 2018: member

“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

achow101 force-pushed on Feb 22, 2018

instagibbs commented at 2:13 pm on March 6, 2018: member

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

instagibbs commented at 5:05 pm on April 5, 2018: member

needs rebase

achow101 force-pushed on Apr 5, 2018

achow101 commented at 8:38 pm on April 5, 2018: member

Rebased

achow101 force-pushed on Apr 26, 2018

achow101 commented at 11:16 pm on April 26, 2018: member

Rebased

in src/script/sign.cpp:446 in 15840187f5 outdated

440@@ -441,3 +441,60 @@ bool IsSolvable(const SigningProvider& provider, const CScript& script)
441     }
442     return false;
443 }
444+
445+PartiallySignedTransaction::PartiallySignedTransaction() : tx(), redeem_scripts(), witness_scripts(), inputs() {}
446+PartiallySignedTransaction::PartiallySignedTransaction(CMutableTransaction& tx, std::map<uint160, CScript>& redeem_scripts, std::map<uint256, CScript>& witness_scripts, std::vector<PartiallySignedInput>& inputs)

sipa commented at 11:25 pm on April 26, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

This constructor can take const reference arguments.

in src/script/sign.cpp:456 in 15840187f5 outdated

451+    this->inputs = inputs;
452+
453+    sanitize_for_serialization();
454+}
455+
456+void PartiallySignedTransaction::sanitize_for_serialization()

sipa commented at 11:25 pm on April 26, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: method naming convention

in src/script/sign.cpp:466 in 15840187f5 outdated

461+        PartiallySignedInput psbt_in = inputs[i];
462+        CTxOut vout;
463+        if (psbt_in.non_witness_utxo) {
464+            vout = psbt_in.non_witness_utxo->vout[in.prevout.n];
465+        }
466+        else if (!psbt_in.witness_utxo.IsNull()) {

sipa commented at 11:26 pm on April 26, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: else on the same line as } (and elsewhere).

in src/script/sign.cpp:461 in 15840187f5 outdated

456+void PartiallySignedTransaction::sanitize_for_serialization()
457+{
458+    // Remove sigs from inputs
459+    for (unsigned int i = 0; i < tx.vin.size(); i++) {
460+        CTxIn& in = tx.vin[i];
461+        PartiallySignedInput psbt_in = inputs[i];

sipa commented at 11:28 pm on April 26, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

You can avoid making a copy here (use a const reference instead).

in src/script/sign.h:74 in 15840187f5 outdated

67@@ -68,6 +68,392 @@ struct SignatureData {
68     explicit SignatureData(const CScript& script) : scriptSig(script) {}
69 };
70 
71+
72+
73+// Note: These constants are in reverse byte order because serialization uses LSB
74+static const uint32_t PSBT_MAGIC_BYTES = 0x74627370;

sipa commented at 11:30 pm on April 26, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

These can be marked constexpr instead.

in src/script/sign.h:441 in 15840187f5 outdated

436+                    Span<unsigned char> value = MakeSpan(val_bytes);
437+                    s >> value;
438+
439+                    // global data
440+                    if (in_globals) {
441+                        unknown.emplace(key, val_bytes);

sipa commented at 11:34 pm on April 26, 2018:

Using std::move(key) and std::move(val_bytes) here and below will avoid copies.

in src/script/sign.h:105 in 15840187f5 outdated

100+    std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
101+    int sighash_type = 0;
102+    uint64_t index;
103+    bool use_in_index = false;
104+
105+    PartiallySignedInput(): non_witness_utxo(), witness_utxo(), partial_sigs(), unknown() {}

sipa commented at 7:16 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

I don’t think any of these constructor initializers are necessary (the default constructor will do the same).

in src/script/sign.h:102 in 15840187f5 outdated

 97+    CTransactionRef non_witness_utxo;
 98+    CTxOut witness_utxo;
 99+    std::map<CPubKey, std::vector<unsigned char>> partial_sigs;
100+    std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
101+    int sighash_type = 0;
102+    uint64_t index;

sipa commented at 7:16 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Specify an initializer for index?

in src/script/sign.cpp:445 in 15840187f5 outdated

440@@ -441,3 +441,60 @@ bool IsSolvable(const SigningProvider& provider, const CScript& script)
441     }
442     return false;
443 }
444+
445+PartiallySignedTransaction::PartiallySignedTransaction() : tx(), redeem_scripts(), witness_scripts(), inputs() {}

sipa commented at 7:17 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

These initializers are not necessary (they just invoke the default constructor).

in src/script/sign.cpp:491 in 15840187f5 outdated

486+    }
487+}
488+
489+void PartiallySignedInput::SetNull()
490+{
491+    non_witness_utxo = CTransactionRef();

sipa commented at 7:19 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

You can use non_witness_utxo.reset(); here.

in src/script/sign.h:145 in 15840187f5 outdated

140+        }
141+
142+        // Write redeem scripts and witness scripts
143+        for (auto& entry : redeem_scripts) {
144+            s << SerializeToVector(PSBT_REDEEMSCRIPT_WITNESS_UTXO, entry.first);
145+            WriteCompactSize(s, entry.second.size());

sipa commented at 7:22 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

You can replace this line + the next with s << entry.second (the default serializer for CScript uses length prefixing).

in src/script/sign.h:150 in 15840187f5 outdated

145+            WriteCompactSize(s, entry.second.size());
146+            s << MakeSpan(entry.second);
147+        }
148+        for (auto& entry : witness_scripts) {
149+            s << SerializeToVector(PSBT_WITNESSSCRIPT_PARTIAL_SIG, entry.first);
150+            WriteCompactSize(s, entry.second.size());

sipa commented at 7:22 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Likewise, replace this line and the next with s << entry.second.

in src/script/sign.h:185 in 15840187f5 outdated

151+            s << MakeSpan(entry.second);
152+        }
153+        // Write any hd keypaths
154+        for (auto keypath_pair : hd_keypaths) {
155+            s << SerializeToVector(PSBT_BIP32_KEYPATH_SIGHASH, Span<const unsigned char>(keypath_pair.first.begin(), keypath_pair.first.size()));
156+            WriteCompactSize(s, keypath_pair.second.size() * sizeof(uint32_t));

sipa commented at 7:25 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Replace this line and the line below with s << keypath_pair.second;.

achow101 commented at 5:35 am on April 28, 2018:

This does not work because it will be prefixed with the number of items in the keypath, not with the total size in bytes that that will be. Each item in the keypath is a uint32_t, not an unsigned char.

sipa commented at 0:48 am on April 29, 2018:

Ah yes, indeed.

in src/script/sign.h:192 in 15840187f5 outdated

159+            }
160+        }
161+
162+        // Write the number of inputs
163+        if (num_ins > 0) {
164+            s << PSBT_NUM_IN_VIN;

sipa commented at 7:32 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

This record does not follow the keylen-key-valuelen-value format, and thus wouldn’t be parseable by software that doesn’t know about it.

Also, BIP174 question - why is this record necessary? It should be implied by tx.vin.size().

achow101 commented at 3:59 am on April 28, 2018:

It specifies the number of inputs that have input data in the PSBT, not the number of inputs in the transaction in general.

sipa commented at 10:35 pm on April 30, 2018:

@achow101 Ok, makes sense.

But you’re still writing PSB_NUM_IN_VIN without a keylength prefix here.

in src/script/sign.h:243 in 15840187f5 outdated

209+
210+                // Write the index
211+                if (use_in_index) {
212+                    WriteCompactSize(s, sizeof(PSBT_NUM_IN_VIN));
213+                    s << PSBT_NUM_IN_VIN;
214+                    WriteCompactSize(s, sizeof(psbt_in.index));

sipa commented at 7:45 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

This line and the next write the index as a 4-byte uint32, not a compact size as described by BIP174.

The correct way to follow the BIP would be to first write the size of the compactsize-encoded input index in compactsize encoding, and then the compactsize encoding of the input. This is a lot easier if you’d have a single-argument SerializeToVector first (see above).

Also, this record requires 4 bytes (for smallish transactions), while a separator is only 1. It’s only advantageous to write it everywhere (as opposed to having separators for already-signed inputs) if over 75% of the inputs are already signed. Perhaps it’s generally better to use the separator approach instead?

achow101 commented at 4:13 am on April 28, 2018:

This is a lot easier if you’d have a single-argument SerializeToVector first (see above).

Above where?

I’m not necessarily sure that that will always work due to the different data types.

Perhaps it’s generally better to use the separator approach instead?

I think it generally is better to use the separator approach. By default, this code does not use input indexes unless a psbt that uses input indexes was provided (at least that’s what it is supposed to do). That option was provided at the suggestion of someone else in order to handle very large transactions where almost all inputs were signed.

sipa commented at 10:46 pm on April 30, 2018:

Above where?

https://github.com/bitcoin/bitcoin/pull/12136/commits/e25f290841de2c446a237ad3942a0c2c0427ee62#r184790215

I’m not necessarily sure that that will always work due to the different data types.

It should. Char vectors are always serialized as compactsize-in-bytes + data-bytes. SerializeToVector first serializes to a vector, and then you serialize that. I can create a small example if you want.

This is still writing the input index as a 4-byte int, rather than as a compactsize, btw.

in src/script/sign.h:300 in 15840187f5 outdated

295+
296+            // Read in value length
297+            uint64_t value_len = ReadCompactSize(s);
298+
299+            // Do stuff based on type
300+            switch(type)

sipa commented at 7:46 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: { on the same line as switch.

in src/script/sign.h:159 in 15840187f5 outdated

121+
122+    PartiallySignedTransaction();
123+    PartiallySignedTransaction(CMutableTransaction& tx, std::map<uint160, CScript>& redeem_scripts, std::map<uint256, CScript>& witness_scripts, std::vector<PartiallySignedInput>& inputs);
124+
125+    template <typename Stream>
126+    inline void Serialize(Stream& s) const {

sipa commented at 7:50 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Overall comment on the serialization code: you could simplify things a lot by also having a single-argument SerializeToVector (which takes an object but no header byte).

All instances of WriteCompactSize(s, ::GetSerializeSize(foo)); s << foo; could then turn into s << SerializeToVector(foo);. There are dozens of cases of this.

in src/script/sign.h:308 in 15840187f5 outdated

303+                case PSBT_UNSIGNED_TX_NON_WITNESS_UTXO:
304+                    if (in_globals) {
305+                        s >> tx;
306+                    } else {
307+                        // Read in the transaction
308+                        CMutableTransaction mtx;

sipa commented at 8:21 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

You can use CTransactionRef prev_tx; s >> prev_tx; instead here (and input.non_witness_utxo = std::move(prev_tx); below).

in src/script/sign.h:251 in 15840187f5 outdated

226+        }
227+    }
228+
229+
230+    template <typename Stream>
231+    inline void Unserialize(Stream& s) {

sipa commented at 8:33 pm on April 27, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

General comment on deserialization code: check that keys and values (where relevant) have the expected length. Things like uint160(key.begin() + 1, key.end()) will assertion fail if the length is wrong.

sipa commented at 8:34 pm on April 27, 2018: member

First set of comments on serialization code.

achow101 commented at 4:32 am on April 28, 2018: member

Addressed all of @sipa’s comments.

achow101 force-pushed on Apr 28, 2018

in src/script/sign.h:200 in e25f290841 outdated

195+                }
196+
197+                // Write any partial signatures
198+                for (auto sig_pair : psbt_in.partial_sigs) {
199+                    s << SerializeToVector(PSBT_WITNESSSCRIPT_PARTIAL_SIG, Span<const unsigned char>(sig_pair.first.begin(), sig_pair.first.size()));
200+                    WriteCompactSize(s, sig_pair.second.size());

sipa commented at 10:40 pm on April 30, 2018:

This and the line below can be written as s << sig_pair.second, I think - it’s just an unsigned char vector.

sipa commented at 11:18 pm on April 30, 2018: member

@achow101 Here’s a commit with a few suggested improvements to the serialization code: https://github.com/sipa/bitcoin/commit/8733102e713b1ef8e96f6fca80e77a5d8c47fc0b

achow101 force-pushed on May 1, 2018

achow101 commented at 3:41 pm on May 1, 2018: member

I’ve included @sipa’s serialization improvements.

achow101 force-pushed on May 1, 2018

instagibbs commented at 4:55 pm on May 14, 2018: member

needs rebase

achow101 commented at 10:48 pm on May 17, 2018: member

Rebased

achow101 force-pushed on May 17, 2018

achow101 force-pushed on May 26, 2018

achow101 commented at 4:42 am on May 26, 2018: member

Rebased this and did some reorganization.

I squashed down a lot of the commits so that the changes to different parts aren’t scattered around in different commits.

I also de-duplicated the signing code by using a PSBT specific SIgningProvider and SignatureCreator. The SigningProvider handles providing pubkeys and scripts from the PSBT itself along with the wallet and the SignatureCreator adds signatures and public keys directly to the PSBT once the signature is created. This avoids the previous code duplication.

Hopefully the reorganization and the PSBT specific SigningProvider and SignatureCreator will make this easier to review.

in src/uint256.h:96 in 21f67ff4f0 outdated

90@@ -91,6 +91,15 @@ class base_blob
91                ((uint64_t)ptr[7]) << 56;
92     }
93 
94+    uint32_t GetUint32(int pos) const
95+    {
96+        const uint8_t* ptr = data + pos * 8;

laanwj commented at 7:13 pm on May 29, 2018:

why *8? that doesn’t look in line with 32 bit values (also the uint64_t below should probably be uint32_t).

Also, please add LE to make clear that this is always little-endian.

achow101 commented at 8:23 pm on May 29, 2018:

Whoops. That’s a bad copy-paste from the GetUint64 function above this one.

Fixed.

kallewoof commented at 8:19 am on May 30, 2018:

Maybe I am seeing outdated code, but it still says * 8 here and uint64_t below.

in test/functional/test_runner.py:148 in 21f67ff4f0 outdated

144@@ -145,6 +145,7 @@
145     'feature_blocksdir.py',
146     'feature_config_args.py',
147     'feature_help.py',
148+    "rpc_psbt.py",

laanwj commented at 7:14 pm on May 29, 2018:

Single quote like the others?

achow101 commented at 8:23 pm on May 29, 2018:

Done

achow101 force-pushed on May 29, 2018

dcousens commented at 11:58 pm on May 29, 2018: contributor

@achow101 have you thought about making this a 3rd party library for integration into other software, and then integration here?

in test/functional/test_runner.py:148 in 92cca779d6 outdated

144@@ -145,6 +145,7 @@
145     'feature_blocksdir.py',
146     'feature_config_args.py',
147     'feature_help.py',
148+    'rpc_psbt.py',

jeffrade commented at 2:08 am on May 30, 2018:

0rpc_psbt.py                           | ✓ Passed  | 4 s
1.
2.
3.
4rpc_users.py                          | ✓ Passed  | 3 s

Since rpc_psbt.py ran locally for me in 4 seconds and to avoid possible merge conflict, 'rpc_psbt.py' can be inserted before 'rpc_users.py' in this list.

achow101 commented at 9:01 pm on May 30, 2018:

Done

in src/script/sign.h:98 in c3d7c855d2 outdated

90+{
91+    std::vector<unsigned char> ret;
92+    CVectorWriter ss(SER_NETWORK, PROTOCOL_VERSION, ret, 0);
93+    SerializeMany(ss, args...);
94+    return ret;
95+}

kallewoof commented at 7:52 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Should this be in serialize.h?

(btw, the commit message has a typo (serliazation))

achow101 commented at 3:51 pm on May 30, 2018:

Not sure, it’s kind of specific to PSBT.

kallewoof commented at 5:50 am on May 31, 2018:

Looks very generic, but I have no strong feelings about it (commit still has typo btw).

in src/script/sign.h:194 in c3d7c855d2 outdated

189+            s << SerializeToVector(PSBT_NUM_IN_VIN);
190+            s << SerializeToVector(COMPACTSIZE(num_ins));
191+        }
192+
193+        // Write the unknown things
194+        for(auto& entry : unknown) {

kallewoof commented at 7:59 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: space between for and (, here and on line 236.

achow101 commented at 8:56 pm on May 30, 2018:

Done

in src/script/sign.h:359 in c3d7c855d2 outdated

354+                        // Check that the redeemscript hash160 matches the one provided
355+                        hash160_hasher.Write(&redeemscript_bytes[0], redeemscript_bytes.size());
356+                        unsigned char rs_hash160_data[hash160_hasher.OUTPUT_SIZE];
357+                        hash160_hasher.Finalize(rs_hash160_data);
358+                        uint160 rs_hash160(std::vector<unsigned char>(rs_hash160_data, rs_hash160_data + hash160_hasher.OUTPUT_SIZE));
359+                        if (hash160.Compare(rs_hash160) != 0) {

kallewoof commented at 8:08 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Doesn’t really make a huge difference, but I think if (hash160 != rs_hash160) is a tiny bit more readable.

achow101 commented at 8:56 pm on May 30, 2018:

Done

in src/script/sign.h:396 in c3d7c855d2 outdated

391+                        // Check that the witnessscript sha256 matches the one provided
392+                        sha256_hasher.Write(&witnessscript_bytes[0], witnessscript_bytes.size());
393+                        unsigned char ws_sha256_data[sha256_hasher.OUTPUT_SIZE];
394+                        sha256_hasher.Finalize(ws_sha256_data);
395+                        uint256 ws_sha256(std::vector<unsigned char>(ws_sha256_data, ws_sha256_data + sha256_hasher.OUTPUT_SIZE));
396+                        if (hash.Compare(ws_sha256) != 0) {

kallewoof commented at 8:09 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Same here about (hash != ws_sha256).

achow101 commented at 8:56 pm on May 30, 2018:

Done

in src/script/sign.h:422 in c3d7c855d2 outdated

417+                        input.partial_sigs.emplace(pubkey, sig);
418+                    }
419+                    break;
420+                // BIP 32 HD Keypaths and sighash types
421+                case PSBT_BIP32_KEYPATH_SIGHASH:
422+                {

kallewoof commented at 8:13 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Why the {} here and in case PSBT_NUM_IN_VIN: below? No vars are declared.

achow101 commented at 8:56 pm on May 30, 2018:

Removed

in src/script/sign.h:431 in c3d7c855d2 outdated

424+                        // Make sure that the key is the size of pubkey + 1
425+                         if (key.size() != CPubKey::PUBLIC_KEY_SIZE + 1 && key.size() != CPubKey::COMPRESSED_PUBLIC_KEY_SIZE + 1) {
426+                            throw std::ios_base::failure("Size of key was not the expected size for the type BIP32 keypath");
427+                        }
428+                        // Read in the pubkey from key
429+                        CPubKey pubkey(key.begin() + 1, key.end());

kallewoof commented at 8:16 am on May 30, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Might be good to check validity of the pubkey. E.g.

0if (!pubkey.IsValid()) {
1  throw std::ios_base::failure("Invalid pubkey");
2}

instagibbs commented at 3:07 pm on May 30, 2018:

Might want IsFullyValid()

achow101 commented at 8:57 pm on May 30, 2018:

Done

in src/script/sign.cpp:39 in ed1a6e3ea2 outdated

32@@ -33,6 +33,23 @@ bool TransactionSignatureCreator::CreateSig(const SigningProvider& provider, std
33     return true;
34 }
35 
36+bool PSBTSignatureCreator::CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const
37+{
38+    CPubKey pubkey;
39+    provider.GetPubKey(address, pubkey);

kallewoof commented at 8:22 am on May 30, 2018:

In commit “Implement a signer that takes an unserialized PSBT and signs it”:

Check result of provider.GetPubKey() and react if false?

achow101 commented at 8:57 pm on May 30, 2018:

Done

in src/script/sign.cpp:335 in ed1a6e3ea2 outdated

330+        }
331+    }
332+    // Look for scripts in redeem_scripts
333+    auto mi = psbt->redeem_scripts.find(scriptid);
334+    if (mi != psbt->redeem_scripts.end())
335+    {

kallewoof commented at 8:24 am on May 30, 2018:

Nit/style: { on same line as if

achow101 commented at 8:57 pm on May 30, 2018:

Done

in src/script/sign.h:504 in ed1a6e3ea2 outdated

523+};
524+
525 /** Produce a script signature using a generic signature creator. */
526 bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreator& creator, const CScript& scriptPubKey, SignatureData& sigdata);
527 
528+bool SignPartiallySignedTransaction(PartiallySignedTransaction& psbt, SigningProvider* provider, int nHashType, bool finalize = false);

kallewoof commented at 8:30 am on May 30, 2018:

In commit “Implement a signer that takes an unserialized PSBT and signs it”:

Maybe add a comment about this function, in particular saying that it returns true if it was complete, and if finalize was set, and false does not necessarily mean failure to partially sign.

achow101 commented at 8:57 pm on May 30, 2018:

Done

in src/uint256.h:94 in ed1a6e3ea2 outdated

90@@ -91,13 +91,13 @@ class base_blob
91                ((uint64_t)ptr[7]) << 56;
92     }
93 
94-    uint32_t GetUint32(int pos) const
95+    uint32_t GetUint32LE(int pos) const

kallewoof commented at 8:30 am on May 30, 2018:

In commit “Implement a signer that takes an unserialized PSBT and signs it”:

Was this meant to go into the previous commit?

achow101 commented at 8:59 pm on May 30, 2018:

Oops, yes. Fixed

kallewoof commented at 8:32 am on May 30, 2018: member

Partially reviewed:

c3d7c855d239943fb85a2fce1c5dae1a3f999a71 Implement PSBT structures and un/serliazation methods per BIP 174
ed1a6e3ea2ed526dab76f15ec71b2f7031bd2335 Implement a signer that takes an unserialized PSBT and signs it
ac2b5093282e66a4ec1a579fb0ea133e9880ae08 Create RPCs for PSBT
92cca779d6604762c0e2ee4bcd61dc17502e12f4 Test the PSBT RPCs

in src/core_write.cpp:77 in 92cca779d6 outdated

69@@ -70,6 +70,15 @@ const std::map<unsigned char, std::string> mapSigHashTypes = {
70     {static_cast<unsigned char>(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY")},
71 };
72 
73+std::string SighashToStr(unsigned char sighash_type)
74+{
75+    if (mapSigHashTypes.count(sighash_type)) {
76+        return mapSigHashTypes.find(sighash_type)->second;
77+    } else {

pedrobranco commented at 12:55 pm on May 30, 2018:

Unnecessary else.

achow101 commented at 8:59 pm on May 30, 2018:

Removed

in src/rpc/client.cpp:113 in 92cca779d6 outdated

109@@ -110,6 +110,11 @@ static const CRPCConvertParam vRPCConvertParams[] =
110     { "combinerawtransaction", 0, "txs" },
111     { "fundrawtransaction", 1, "options" },
112     { "fundrawtransaction", 2, "iswitness" },
113+    { "walletupdatepsbt", 2, "psbtformat"},

pedrobranco commented at 1:30 pm on May 30, 2018:

The RPC’s convention is confusing. WDYT of renaming all RPC from:

walletcreatepsbt to createpartiallysignedtransaction
walletupdatepsbt to updatepartiallysignedtransaction
combinepsbt to combinepartiallysignedtransactions
decodepsbt to decodepartiallysignedtransaction

instagibbs commented at 3:39 pm on May 30, 2018:

I think the wallet prefixes are helpful

achow101 commented at 4:02 pm on May 30, 2018:

I prefer the wallet prefixes and expanding out psbt to partiallysignedtransaction just makes the RPC names too long IMO.

pedrobranco commented at 4:27 pm on May 30, 2018:

I think the wallet prefixes are helpful

I prefer the wallet prefixes and expanding out psbt to partiallysignedtransaction just makes the RPC names too long IMO.

We now have several wallet RPC methods that are not prefixed with wallet, so IMO is an unnecessary prefix.

Also AFAIK, by the spec PSBT is a transaction format, so the transaction itself is a partially signed transaction, or pst, so using psbt as suffix somehow seems improper.

Given that, WDYT about not using the prefix wallet, use the ps (term for partially signed) as a transaction type, and use the suffix transaction (expect for combine because is more than one, so we use transactions):

walletcreatepsbt to createpstransaction
walletupdatepsbt to updatepstransaction
combinepsbt to combinepstransactions
decodepsbt to decodepstransaction

sipa commented at 4:39 pm on May 30, 2018:

I think it’s the other way around. Historically no RPCs had a wallet prefix, which led to a lot of confusion (e.g. gettransaction vs getrawtransaction).

I’m in favor of having wallet in the name of RPCs that require a wallet to interact with.

dcousens commented at 8:29 pm on May 30, 2018:

bip174-create bip174-update bip174-combine bip174-decode

Unambiguous, easily searched and semantics known to adhere to the BIP

achow101 commented at 9:01 pm on May 30, 2018:

I agree with @sipa and prefer to have the wallet prefix, especially because there will probably be a non-wallet version of those RPCs at some point in the future.

I don’t think psbt is really that ambiguous..

laanwj commented at 1:57 pm on May 31, 2018:

+1 for wallet prefix. No naming opinions otherwise.

pedrobranco commented at 9:17 am on June 4, 2018:

I think it’s the other way around. Historically no RPCs had a wallet prefix, which led to a lot of confusion (e.g. gettransaction vs getrawtransaction).

Agree on that. If all methods will be prefixed LGTM.

in src/wallet/rpcwallet.cpp:4305 in 92cca779d6 outdated

4300+    while (std::getline(ss, item, '/')) {
4301+        if (item.compare("m") == 0) {
4302+            if (first) {
4303+                first = false;
4304+                continue;
4305+            } else {

pedrobranco commented at 1:36 pm on May 30, 2018:

Unnecessary else.

achow101 commented at 9:01 pm on May 30, 2018:

Removed

in src/wallet/rpcwallet.cpp:4605 in 92cca779d6 outdated

4600+            {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
4601+            {std::string("SINGLE"), int(SIGHASH_SINGLE)},
4602+            {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
4603+        };
4604+        std::string strHashType = request.params[1].get_str();
4605+        if (mapSigHashValues.count(strHashType))

pedrobranco commented at 1:38 pm on May 30, 2018:

Keep the if blocks consistent (with or without {).

achow101 commented at 9:01 pm on May 30, 2018:

Fixed.

laanwj added this to the "Blockers" column in a project

achow101 force-pushed on May 30, 2018

achow101 commented at 9:02 pm on May 30, 2018: member

@dcousens I have, but I’m not sure what the third party library would really be.

in src/script/sign.h:97 in b408bfef3d outdated

92+    CVectorWriter ss(SER_NETWORK, PROTOCOL_VERSION, ret, 0);
93+    SerializeMany(ss, args...);
94+    return ret;
95+}
96+
97+/** An structure for PSBTs which contain per input information */

kallewoof commented at 4:49 am on May 31, 2018:

In “Implement PSBT structures and un/serliazation methods per BIP 174”:

Typo: A_n_ structure

kallewoof commented at 5:51 am on May 31, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Typo: “A_n_ structure”

achow101 commented at 6:48 pm on May 31, 2018:

Done

in src/script/sign.h:427 in b408bfef3d outdated

422+                    break;
423+                // BIP 32 HD Keypaths and sighash types
424+                case PSBT_BIP32_KEYPATH_SIGHASH:
425+                    if (in_globals) {
426+                        // Make sure that the key is the size of pubkey + 1
427+                         if (key.size() != CPubKey::PUBLIC_KEY_SIZE + 1 && key.size() != CPubKey::COMPRESSED_PUBLIC_KEY_SIZE + 1) {

kallewoof commented at 5:40 am on May 31, 2018:

In “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: mis-aligned indentation. (remove 1 space I think?)

kallewoof commented at 5:53 am on May 31, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

Nit: Mis-aligned indentation (delete 1 space?).

achow101 commented at 6:48 pm on May 31, 2018:

Done

in src/script/sign.h:472 in b408bfef3d outdated

467+                default:
468+                    // Read in the value
469+                    std::vector<unsigned char> val_bytes;
470+                    val_bytes.resize(value_len);
471+                    Span<unsigned char> value = MakeSpan(val_bytes);
472+                    s >> value;

kallewoof commented at 5:48 am on May 31, 2018:

In “Implement PSBT structures and un/serliazation methods per BIP 174”:

I think this (and other places where Span is used) would look less confusing if you did s >> MakeSpan(val_bytes), since you are not using value, and it looks like the data is written to some unrelated thing (i.e. not val_bytes, but value). Not sure if that is possible though (it should be, if it isn’t, but that’s a separate story).

kallewoof commented at 5:54 am on May 31, 2018:

In commit “Implement PSBT structures and un/serliazation methods per BIP 174”:

This is a little confusing because value is not actually used, even though it is streamed to. Can this simply be

0s >> MakeSpan(val_bytes);

?

(Several other places where this is done as well, IIRC.)

achow101 commented at 6:49 pm on May 31, 2018:

Done here and everywhere else this happens.

in src/rpc/rawtransaction.cpp:1283 in 78dadeac85 outdated

1278+}
1279+
1280+UniValue decodepsbt(const JSONRPCRequest& request)
1281+{
1282+    if (request.fHelp || request.params.size() != 1)
1283+    throw std::runtime_error(

kallewoof commented at 6:00 am on May 31, 2018:

In commit “Create RPCs for PSBT”:

Indentation.

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/wallet/rpcwallet.cpp:4543 in 78dadeac85 outdated

4538+
4539+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
4540+        throw std::runtime_error(
4541+            "walletupdatepsbt \"hexstring\" ( sighashtype psbtformat include_output_info)\n"
4542+            "\nUpdate a psbt with input information from our wallet and then sign inputs\n"
4543+            "that we an sign for.\n"

kallewoof commented at 6:15 am on May 31, 2018:

In commit “Create RPCs for PSBT”:

Typo: an -> can

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/wallet/rpcwallet.cpp:4558 in 78dadeac85 outdated

4553+            "       \"NONE|ANYONECANPAY\"\n"
4554+            "       \"SINGLE|ANYONECANPAY\"\n"
4555+            "3. \"psbtformat\"              (boolean, optional, default=false) If true, return the complete transaction \n"
4556+            "                             in the PSBT format. Otherwise complete transactions will be in normal network serialization.\n"
4557+            "4. \"include_output_info\"     (boolean, optional, default=false) If true, returns the PSBT with the redeem scripts, witness\n"
4558+            "                             scripts, and bip32 keypaths of the outputs if they are available. This is useful for hardware wllets\n"

kallewoof commented at 6:17 am on May 31, 2018:

In commit “Create RPCs for PSBT”:

Typo: wllets

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/wallet/rpcwallet.cpp:4541 in 78dadeac85 outdated

4536+        return NullUniValue;
4537+    }
4538+
4539+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
4540+        throw std::runtime_error(
4541+            "walletupdatepsbt \"hexstring\" ( sighashtype psbtformat include_output_info)\n"

kallewoof commented at 6:22 am on May 31, 2018:

In commit “Create RPCs for PSBT”:

Add space before ). Also some inconsistency (other methods do (x y) while this does ( x y).

achow101 commented at 6:49 pm on May 31, 2018:

Done

kallewoof approved

kallewoof commented at 6:23 am on May 31, 2018: member

utACK 30c0cf203e8c82798e047ab4b8805638612421a5

kallewoof commented at 6:26 am on May 31, 2018: member

Sorry, some comments are duplicate (I thought I lost my review and re-did).

in src/core_write.cpp:75 in 30c0cf203e outdated

69@@ -70,6 +70,14 @@ const std::map<unsigned char, std::string> mapSigHashTypes = {
70     {static_cast<unsigned char>(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY")},
71 };
72 
73+std::string SighashToStr(unsigned char sighash_type)
74+{
75+    if (mapSigHashTypes.count(sighash_type)) {

laanwj commented at 1:56 pm on May 31, 2018:

Nit: avoid double lookup

0auto it = mapSigHashTypes.find(sighash_type);
1if (it != mapSigHashTypes.end()) {
2    return it->second;
3}

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1262 in 30c0cf203e outdated

1258@@ -1259,6 +1259,422 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
1259     return result;
1260 }
1261 
1262+void write_hd_keypath(std::vector<uint32_t>& keypath, std::string& keypath_str)

laanwj commented at 1:59 pm on May 31, 2018:

Any reason to not simply return the string here? It’s always cleared, anyhow. “Write” implies append to me otherwise.
Function name is in snakecase, WriteHDKeypath would be more akin to the rest of our style.
Could it be static?

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1293 in 30c0cf203e outdated

1288+        "1. \"hexstring\"      (string, required) The transaction hex string\n"
1289+
1290+        "\nResult:\n"
1291+        "{\n"
1292+        "  \"tx\" : {                   (json object) The decoded network serialized transaction\n"
1293+        "    ...                                      decoding is the same as decoderawtransaction's."

laanwj commented at 2:03 pm on May 31, 2018:

This wording is a bit confusing to me. Maybe “The decoded network-serialized transaction. The layout is the same as the output of decoderawtransaction.”? Also you forgot a \n

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1300 in 30c0cf203e outdated

1295+        "  \"redeem_scripts\" : [       (array of json objects, optional)\n"
1296+        "    {\n"
1297+        "      \"hash\" : \"hash\",       (string) The hash160 of the redeem script\n"
1298+        "      \"script\" : {           (json object)\n"
1299+        "        \"asm\":\"asm\",         (string) Script public key\n"
1300+        "        \"hex\":\"hex\",         (string) hex encoded public key\n"

laanwj commented at 2:04 pm on May 31, 2018:

Help message capitalization inconsistent for this, and “address” below.

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1436 in 30c0cf203e outdated

1431+        for (auto entry : psbtx.hd_keypaths) {
1432+            UniValue keypath(UniValue::VOBJ);
1433+            keypath.pushKV("pubkey", HexStr(entry.first));
1434+
1435+            uint32_t fingerprint = entry.second.at(0);
1436+            std::vector<uint8_t> fingerprint_bytes;

laanwj commented at 2:34 pm on May 31, 2018:

would make sense to factor this out, to a function that converts a uint32_t to a LE vector of uint8_t.

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1382 in 30c0cf203e outdated

1377+    try {
1378+        ssData >> psbtx;
1379+        if (!ssData.empty()) {
1380+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed, extra data after PSBT");
1381+        }
1382+    }

laanwj commented at 2:41 pm on May 31, 2018:

} on same line as catch

achow101 commented at 6:49 pm on May 31, 2018:

Done

in src/rpc/rawtransaction.cpp:1432 in 30c0cf203e outdated

1439+            fingerprint_bytes.push_back(fingerprint >> 16);
1440+            fingerprint_bytes.push_back(fingerprint >> 24);
1441+            keypath.pushKV("master_fingerprint", HexStr(fingerprint_bytes));
1442+
1443+            std::string keypath_str;
1444+            entry.second.erase(entry.second.begin());

laanwj commented at 2:44 pm on May 31, 2018:

Do you really intend to update the psbtx in place here?

achow101 commented at 6:50 pm on May 31, 2018:

Since entry is not a reference or a pointer, psbtx isn’t actually modified.

laanwj commented at 3:39 pm on May 31, 2018: member

some comments, I did not get around to reviewing all changes yet

achow101 force-pushed on May 31, 2018

achow101 commented at 7:12 pm on May 31, 2018: member

Also rebased to (hopefully) fix the travis error

~~edit: I have no idea why travis is failing. Builds without error for me..~~ edit2: I didn’t rebase far enough

achow101 force-pushed on May 31, 2018

in src/core_write.cpp:75 in 47103a1b2e outdated

69@@ -70,6 +70,25 @@ const std::map<unsigned char, std::string> mapSigHashTypes = {
70     {static_cast<unsigned char>(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY")},
71 };
72 
73+std::string SighashToStr(unsigned char sighash_type)
74+{
75+    auto it = mapSigHashTypes.find(sighash_type);

promag commented at 10:48 pm on June 4, 2018:

nit,

0const auto& it =

achow101 commented at 4:58 am on June 5, 2018:

Done

in src/core_write.cpp:76 in 47103a1b2e outdated

69@@ -70,6 +70,25 @@ const std::map<unsigned char, std::string> mapSigHashTypes = {
70     {static_cast<unsigned char>(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY")},
71 };
72 
73+std::string SighashToStr(unsigned char sighash_type)
74+{
75+    auto it = mapSigHashTypes.find(sighash_type);
76+    if (it != mapSigHashTypes.end()) {

promag commented at 10:49 pm on June 4, 2018:

nit

0if (it == mapSigHashTypes.end()) return "";
1return it->second;

achow101 commented at 4:58 am on June 5, 2018:

Done

promag commented at 0:03 am on June 7, 2018:

Missing change here and above.

achow101 commented at 6:24 pm on June 7, 2018:

Whoops, should be fixed now.

in src/wallet/rpcwallet.cpp:4587 in 47103a1b2e outdated

4537+    }
4538+
4539+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
4540+        throw std::runtime_error(
4541+            "walletupdatepsbt \"hexstring\" ( sighashtype psbtformat include_output_info )\n"
4542+            "\nUpdate a psbt with input information from our wallet and then sign inputs\n"

promag commented at 10:56 pm on June 4, 2018:

s/psbt/PSBT?

achow101 commented at 4:58 am on June 5, 2018:

Done

in src/wallet/rpcwallet.cpp:4632 in 47103a1b2e outdated

4582+    try {
4583+        ssData >> psbtx;
4584+        if (!ssData.empty()) {
4585+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed, extra data after PSBT");
4586+        }
4587+    }

promag commented at 10:58 pm on June 4, 2018:

nit

0} catch (...) {

achow101 commented at 4:58 am on June 5, 2018:

Done

in src/rpc/rawtransaction.cpp:1366 in 47103a1b2e outdated

1364+            "\nExamples:\n"
1365+            + HelpExampleCli("decodepsbt", "\"hexstring\"")
1366+            + HelpExampleRpc("decodepsbt", "\"hexstring\"")
1367+    );
1368+
1369+    RPCTypeCheck(request.params, {UniValue::VSTR});

promag commented at 11:00 pm on June 4, 2018:

I believe this can be removed, get_str() below will validate it.

achow101 commented at 4:59 am on June 5, 2018:

I don’t see the harm in leaving it.

in src/rpc/rawtransaction.cpp:1376 in 47103a1b2e outdated

1371+    // Unserialize the transactions
1372+    PartiallySignedTransaction psbtx;
1373+    if (!IsHex(request.params[0].get_str())) {
1374+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed, not hex");
1375+    }
1376+    std::vector<unsigned char> txData(ParseHex(request.params[0].get_str()));

promag commented at 11:04 pm on June 4, 2018:

Use ParseHexV instead of IsHex + ParseHex?

achow101 commented at 4:59 am on June 5, 2018:

Done

promag commented at 11:10 pm on June 4, 2018: member

This is a lot to review and will probably receive a lot of more comments. Have you thought of splitting it?

in src/script/sign.h:139 in 446731a50c outdated

123+
124+    // Only checks if they refer to the same transaction
125+    friend bool operator==(const PartiallySignedTransaction& a, const PartiallySignedTransaction &b)
126+    {
127+        // Check the inputs
128+        for (unsigned int i = 0; i < a.tx.vin.size(); ++i) {

promag commented at 0:06 am on June 5, 2018:

First check?

0if (a.tx.vin.size() != b.tx.vin.size() || a.tx.vout.size() != b.tx.vout.size()) return false;

Otherwise this allows b to have more in/outputs and if b has fewer then it oob.

achow101 commented at 4:59 am on June 5, 2018:

Done

achow101 force-pushed on Jun 5, 2018

achow101 commented at 4:59 am on June 5, 2018: member

Have you thought of splitting it?

Splitting into what components? All of it kind of goes together.

achow101 force-pushed on Jun 5, 2018

in src/script/sign.cpp:425 in aecb40ae5c outdated

431+            psbt.tx.vin[i].scriptWitness = sigdata.scriptWitness;
432+        }
433+        solved &= sig_complete;
434+    }
435+
436+    return solved;

pedrobranco commented at 9:57 am on June 5, 2018:

Keep the code format consistent (other places without new lines before return).

IMHO we should have some set of rules for code formatting (maybe a linter would be nice).

achow101 commented at 11:15 pm on June 5, 2018:

IMO this doesn’t matter

achow101 force-pushed on Jun 5, 2018

achow101 commented at 11:14 pm on June 5, 2018: member

Rebased to fix a silent merge conflict.

in src/core_write.cpp:82 in 3f6d5d43e7 outdated

79+    return "";
80+}
81+
82+std::vector<uint8_t> Uint32ToUint8VectorLE(uint32_t in)
83+{
84+    std::vector<uint8_t> bytes;

promag commented at 0:18 am on June 7, 2018:

Alternative

0return { uint8_t(in), uint8_t(in >> 8), uint8_t(in >> 16), uint8_t(in >> 24) };

achow101 commented at 6:25 pm on June 7, 2018:

I think it is more readable as it is now so I will leave it as is.

tokutech commented at 3:44 am on June 7, 2018: none

Hello,

Why not using JSON(like protocolbuffer) instead this BIP format? Its more simpler.

achow101 commented at 9:14 am on June 7, 2018: member

@tokutech Because stuff like JSON or protobufs are more cumbersome to use. For JSON, the size of the objects are much larger. They are also in a form that is human readable and human readability doesn’t matter for this. Furthermore, in order for devices to use something that is JSON formatted, it needs to have a JSON parser, which is not necessarily something that exists on such devices. While those devices need to have a PSBT parser to use PSBT, that is easier as much of the functionality required for a PSBT parser is already required to parse other Bitcoin network messages, including normal transactions.

As for protobufs, that introduces another dependency and we don’t want to do that.

achow101 force-pushed on Jun 7, 2018

MarcoFalke commented at 7:25 pm on June 7, 2018: member

From travis:

0The locale dependent function isdigit(...) appears to be used:
1src/wallet/rpcwallet.cpp:            if (!std::isdigit(c)) {
2Unnecessary locale dependence can cause bugs that are very
3tricky to isolate and fix. Please avoid using locale dependent
4functions if possible.
5Advice not applicable in this specific case? Add an exception
6by updating the ignore list in test/lint/lint-locale-dependence.sh
7^---- failure generated from test/lint/lint-locale-dependence.sh

achow101 commented at 7:46 pm on June 7, 2018: member

I think I fixed the linter error.

achow101 force-pushed on Jun 7, 2018

achow101 commented at 0:22 am on June 9, 2018: member

Apparently I lost some changes in a rebase or something. They should be back.

achow101 force-pushed on Jun 9, 2018

achow101 force-pushed on Jun 10, 2018

achow101 commented at 0:18 am on June 10, 2018: member

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

in src/script/sign.cpp:99 in 4fe585eacf outdated

 96-        if (!creator.CreateSig(provider, sig, CPubKey(vSolutions[0]).GetID(), scriptPubKey, sigversion)) return false;
 97+    case TX_PUBKEY: {
 98+        CPubKey pubkey(vSolutions[0]);
 99+        keyID = pubkey.GetID();
100+        if (!creator.CreateSig(provider, sig, keyID, scriptPubKey, sigversion)) return false;
101+        sigdata.signatures.emplace(keyID, SigPair(pubkey, sig));

sipa commented at 1:06 am on June 10, 2018:

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

achow101 commented at 1:19 am on June 10, 2018:

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.

sipa commented at 1:34 am on June 10, 2018:

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.

achow101 commented at 7:08 pm on June 11, 2018:

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.

in src/script/sign.cpp:108 in 4fe585eacf outdated

106         keyID = CKeyID(uint160(vSolutions[0]));
107         if (!creator.CreateSig(provider, sig, keyID, scriptPubKey, sigversion)) return false;
108-        ret.push_back(std::move(sig));
109         CPubKey pubkey;
110         GetPubKey(&provider, &sigdata, keyID, pubkey);
111+        sigdata.signatures.emplace(keyID, SigPair(pubkey, sig));

sipa commented at 1:06 am on June 10, 2018:

Same here.

in src/script/sign.h:456 in 7b636e9180 outdated

453+                // Number of inputs and input index
454+                case PSBT_NUM_IN_VIN:
455+                    if (in_globals) {
456+                        num_ins = ReadCompactSize(s);
457+                    } else {
458+                        // Make sure that we are using inout indexes or this is the first input

araspitzu commented at 11:03 am on June 11, 2018:

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

achow101 commented at 7:17 pm on June 11, 2018:

Fixed

achow101 force-pushed on Jun 11, 2018

Inline Sign1 and SignN

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

51ed700dc2

achow101 renamed this:
~~Implement BIP 174 Partially Signed Bitcoin Transactions~~
Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs
on Jun 12, 2018

achow101 commented at 7:03 pm on June 12, 2018: member

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

Make SignatureData able to store signatures and scripts

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.

0cc5c73f3a

Replace CombineSignatures with ProduceSignature

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.

fc99ba94bd

Remove CombineSignatures and replace tests

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

3d2cbe67e4

laanwj removed this from the "Blockers" column in a project

Implement PSBT structures and un/serliazation methods per BIP 174 b7de8dede7

Implement a signer that takes an unserialized PSBT and signs it 2fa265513c

Create RPCs for PSBT

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.

d35077262d

Test the PSBT RPCs 950746725a

achow101 force-pushed on Jun 14, 2018

luke-jr commented at 10:58 pm on June 16, 2018: member

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 commented at 11:22 pm on June 16, 2018: member

@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 commented at 1:35 am on June 17, 2018: member

@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 commented at 5:36 pm on June 24, 2018: member

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

achow101 closed this on Jun 24, 2018

laanwj referenced this in commit b654723461 on Jul 18, 2018

MarcoFalke locked this on Sep 8, 2021

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