BIP 174 PSBT Serializations and RPCs

achow101 commented at 3:04 am on June 28, 2018: member

This Pull Request fully implements the updated 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

in src/script/sign.h:385 in bfb37cb741 outdated

357+        Unserialize(s);
358+    }
359+};
360+
361+/** A structure for PSBTs which contains per output information */
362+struct PSBTOutput

sipa commented at 5:01 am on June 28, 2018:

Inconsistent class name (PartiallySignedInput vs PSBTOutput).

achow101 commented at 7:53 pm on June 28, 2018:

Done. Renamed PartiallySignedInput to PSBTInput

in src/script/sign.h:122 in bfb37cb741 outdated

103+// The separator is 0x00. Reading this in means that the unserializer can interpret it
104+// as a 0 length key. which indicates that this is the separator. The separator has no value.
105+static const uint8_t PSBT_SEPARATOR = 0x00;
106+
107+template<typename Stream, typename... X>
108+void SerializeToVector(Stream& s, const X&... args)

sipa commented at 5:02 am on June 28, 2018:

Perhaps add a comment to explain this function and the one below.

achow101 commented at 7:53 pm on June 28, 2018:

Done

in src/script/sign.cpp:263 in a86d0679db outdated

260+        // Build a map of pubkey -> signature by matching sigs to pubkeys:
261+        assert(solutions.size() > 1);
262+        unsigned int num_pubkeys = solutions.size()-2;
263+        unsigned int last_success_key = 0;
264+        for (const valtype& sig : stack.script) {
265+            for (unsigned int i = last_success_key; i < num_pubkeys; i++) {

promag commented at 9:04 am on June 28, 2018:

Commit “Make SignatureData able to store signatures and scripts”

nit, ++i

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

Done

in src/script/sign.h:94 in a86d0679db outdated

90@@ -81,4 +91,15 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
91  * Solvability is unrelated to whether we consider this output to be ours. */
92 bool IsSolvable(const SigningProvider& provider, const CScript& script);
93 
94+class SignatureExtractorChecker : public BaseSignatureChecker

promag commented at 9:09 am on June 28, 2018:

Commit “Make SignatureData able to store signatures and scripts”

class SignatureExtractorChecker final?

achow101 commented at 7:12 pm on June 28, 2018:

Done

in src/script/sign.cpp:182 in a86d0679db outdated

179-SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn)
180+bool SignatureExtractorChecker::CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
181+{
182+    if (checker->CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) {
183+        CPubKey pubkey(vchPubKey);
184+        sigdata->signatures.emplace(pubkey.GetID(), SigPair(pubkey, scriptSig));

promag commented at 9:15 am on June 28, 2018:

Commit “Make SignatureData able to store signatures and scripts”

This is called when pubkey.GetID() doesn’t exists in signatures, maybe assert it is new:

0auto i = sigdata->signatures.emplace(...);
1assert(i.second);

achow101 commented at 7:12 pm on June 28, 2018:

Done

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

90@@ -81,4 +91,15 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
91  * Solvability is unrelated to whether we consider this output to be ours. */
92 bool IsSolvable(const SigningProvider& provider, const CScript& script);
93 
94+class SignatureExtractorChecker : public BaseSignatureChecker
95+{
96+private:
97+    SignatureData* sigdata;

promag commented at 9:16 am on June 28, 2018:

Commit “Make SignatureData able to store signatures and scripts”

Use references instead of pointers?

achow101 commented at 7:12 pm on June 28, 2018:

Done

in src/script/sign.cpp:36 in c5d47a58fd outdated

32@@ -33,14 +33,60 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid
33     return true;
34 }
35 
36+static bool GetCScript(const SigningProvider* provider, const SignatureData* sigdata, const CScriptID &scriptid, CScript& script)

promag commented at 9:20 am on June 28, 2018:

Commit “Replace CombineSignatures with ProduceSignature”

Looks like these could be references:

0static bool GetCScript(const SigningProvider& provider, const SignatureData& sigdata, const CScriptID &scriptid, CScript& script)

and remove provider != nullptr below.

promag commented at 9:25 am on June 28, 2018:

Otherwise fix space before scriptid argument. Same below in the definition.

achow101 commented at 7:12 pm on June 28, 2018:

Done

in src/script/sign.cpp:58 in c5d47a58fd outdated

53+{
54+    if (provider != nullptr && provider->GetPubKey(address, pubkey)) {
55+        return true;
56+    }
57+    // Look for pubkey in all partial sigs
58+    const auto& it = sigdata->signatures.find(address);

promag commented at 9:27 am on June 28, 2018:

Commit “Replace CombineSignatures with ProduceSignature”

As pointed by @MarcoFalke, don’t use references to iterators. Check throughout too.

achow101 commented at 7:12 pm on June 28, 2018:

Done.

DrahtBot commented at 9:28 am on June 28, 2018: member

#13359 (wallet: Directly operate with CMutableTransaction by MarcoFalke)
#13266 (refactoring: privatize SignatureExtractorChecker by Empact)
#13144 (RPC: Improve error messages on RPC endpoints that use GetTransaction by jimpo)
#13098 (Skip tx-rehashing on historic blocks by MarcoFalke)

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.

promag commented at 9:33 am on June 28, 2018: member

Review comments for:

fdcbcb380 - Inline Sign1 and SignN
a86d0679d - Make SignatureData able to store signatures and scripts
c5d47a58f - Replace CombineSignatures with ProduceSignature
95f5a38dd - Remove CombineSignatures and replace tests

fdcbcb380 is kind of unrelated here no?

achow101 commented at 5:52 pm on June 28, 2018: member

@promag Just so you know, those commits are part of #13425

promag commented at 6:01 pm on June 28, 2018: member

Ops @achow101 do you want me to comment there?

in src/script/sign.h:268 in bfb37cb741 outdated

259+                    if (partial_sigs.count(pubkey.GetID()) > 0) {
260+                        throw std::ios_base::failure("Duplicate Key, input partial signature for pubkey already provided");
261+                    }
262+
263+                    // Read in the signature from value
264+                    uint64_t value_len = ReadCompactSize(s);

sipa commented at 6:01 pm on June 28, 2018:

I think this can be written as just std::vector<unsigned char> sig; s >> sig.

achow101 commented at 7:53 pm on June 28, 2018:

Done

achow101 commented at 6:05 pm on June 28, 2018: member

No need to comment twice. I will update that PR and then rebase this onto that.

in src/script/sign.h:286 in bfb37cb741 outdated

231+            // First byte of key is the type
232+            unsigned char type = key[0];
233+
234+            // Do stuff based on type
235+            switch(type) {
236+                case PSBT_IN_NON_WITNESS_UTXO:

sipa commented at 6:06 pm on June 28, 2018:

For this one and the next, check whether you don’t already have the other UTXO type?

achow101 commented at 7:54 pm on June 28, 2018:

Done

in src/script/sign.h:349 in bfb37cb741 outdated

340+                default:
341+                    if (unknown.count(key) > 0) {
342+                        throw std::ios_base::failure("Duplicate Key, key for unknown value already provided");
343+                    }
344+                    // Read in the value
345+                    uint64_t value_len = ReadCompactSize(s);

sipa commented at 6:15 pm on June 28, 2018:

Here you can again just use std::vector<unsigned char> val_bytes; s >> val_bytes;.

achow101 commented at 7:54 pm on June 28, 2018:

Done

in src/script/sign.h:459 in bfb37cb741 outdated

439+                        throw std::ios_base::failure("Duplicate Key, output witnessScript already provided");
440+                    }
441+                    s >> witness_script;
442+                    break;
443+                }
444+                case PSBT_OUT_BIP32_DERIVATION:

sipa commented at 6:16 pm on June 28, 2018:

Can you abstract out the serialization/deserialization code for derivation paths and scripts into separate functions? Otherwise it is needlessly duplicated across input and outputs.

achow101 commented at 7:54 pm on June 28, 2018:

Done

in src/script/sign.h:508 in bfb37cb741 outdated

497+{
498+    CMutableTransaction tx;
499+    std::vector<PartiallySignedInput> inputs;
500+    std::vector<PSBTOutput> outputs;
501+    std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
502+    uint64_t num_ins = 0;

sipa commented at 6:44 pm on June 28, 2018:

Do you still need num_ins and use_in_index?

achow101 commented at 7:54 pm on June 28, 2018:

Removed

in src/script/sign.h:587 in bfb37cb741 outdated

612+            throw std::ios_base::failure("No unsigned transcation was provided");
613+        }
614+
615+        // Read input data
616+        unsigned int i = 0;
617+        while (!s.empty() && i < tx.vin.size()) {

sipa commented at 6:46 pm on June 28, 2018:

Why the !s.empty() check here? If we reach EOF in the stream an error should be raised, not ignore it.

achow101 commented at 7:52 pm on June 28, 2018:

If we reach EOF in the stream, this loop will exit and the if block below will throw the error as the number of inputs/outputs will differ from the number of inputs/outputs in the unsigned tx.

sipa commented at 9:36 pm on June 28, 2018:

Ah, I see. That will probably give a more useful error.

sipa commented at 6:47 pm on June 28, 2018: member

Just comments on the serialization commit.

achow101 force-pushed on Jun 28, 2018

in src/script/sign.h:84 in fdd7cd5a1d outdated

75@@ -73,6 +76,565 @@ struct SignatureData {
76     void MergeSignatureData(SignatureData sigdata);
77 };
78 
79+
80+
81+// Note: These constants are in reverse byte order because serialization uses LSB
82+static constexpr uint32_t PSBT_MAGIC_BYTES = 0x74627370;

sipa commented at 9:28 pm on June 28, 2018:

You could also write it as uint8_t PSBT_MAGIC_BYTES[4] = "PSBT";. Byte arrays can be serialized directly now.

achow101 commented at 10:22 pm on June 28, 2018:

Done

in src/script/sign.h:107 in fdd7cd5a1d outdated

100+static constexpr uint8_t PSBT_OUT_WITNESSSCRIPT = 0x01;
101+static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
102+
103+// The separator is 0x00. Reading this in means that the unserializer can interpret it
104+// as a 0 length key. which indicates that this is the separator. The separator has no value.
105+static const uint8_t PSBT_SEPARATOR = 0x00;

sipa commented at 9:28 pm on June 28, 2018:

constexpr

achow101 commented at 9:53 pm on June 28, 2018:

Done

in src/script/sign.h:132 in fdd7cd5a1d outdated

127+        throw std::ios_base::failure("Size of value was not the stated size");
128+    }
129+}
130+
131+// Deserialize HD keypaths into a map
132+static void DeserializeHDKeypaths(const std::vector<unsigned char>& key, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths, Stream& s)

sipa commented at 9:29 pm on June 28, 2018:

Add template <typename Stream> before.

achow101 commented at 9:53 pm on June 28, 2018:

Fixed

in src/script/sign.h:161 in fdd7cd5a1d outdated

156+    // Add to map
157+    hd_keypaths.emplace(pubkey, keypath);
158+}
159+
160+// Serialize HD keypaths to a stream from a map
161+static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s)

sipa commented at 9:29 pm on June 28, 2018:

Add template<typename Stream> before.

achow101 commented at 9:54 pm on June 28, 2018:

Fixed

in src/script/sign.h:164 in fdd7cd5a1d outdated

159+
160+// Serialize HD keypaths to a stream from a map
161+static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s)
162+{
163+    for (auto keypath_pair : hd_keypaths) {
164+        SerializeToVector(s, PSBT_IN_BIP32_DERIVATION, MakeSpan(keypath_pair.first));

sipa commented at 9:32 pm on June 28, 2018:

Make the field type a function argument; it differs between input and output types.

achow101 commented at 9:54 pm on June 28, 2018:

Fixed

achow101 force-pushed on Jun 28, 2018

in src/rpc/rawtransaction.cpp:1370 in af544a96fa outdated

1357+            + HelpExampleCli("decodepsbt", "\"base64string\"")
1358+    );
1359+
1360+    RPCTypeCheck(request.params, {UniValue::VSTR});
1361+
1362+    // Unserialize the transactions

sipa commented at 10:25 pm on June 28, 2018:

Would it make sense to abstract out this convert-base64-to-psbt RPC routine into a separate function? It’s probably called from many RPCs.

achow101 commented at 2:24 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1469 in af544a96fa outdated

1453+                in.pushKV("final_scriptSig", HexStr(input.final_script_sig));
1454+            }
1455+            if (!input.final_script_witness.IsNull()) {
1456+                in.pushKV("final_scripWitness", input.final_script_witness.ToString());
1457+            }
1458+        }

sipa commented at 10:26 pm on June 28, 2018:

Also add unknowns to the decode?

achow101 commented at 2:24 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1345 in af544a96fa outdated

1340+            "      }\n"
1341+            "    }\n"
1342+            "  \"bip32_derivs\" : [          (array of json objects, optional)\n"
1343+            "    {\n"
1344+            "      \"pubkey\" : \"pubkey\",                     (string) The public key this path corresponds to\n"
1345+            "      \"master_fingerprint\" : \"fingerprint\"     (string) The fingerprint of the master key\n"

sipa commented at 10:27 pm on June 28, 2018:

What do you think about combing fingerprint and path into one string (like proposed in my descriptor language)?

achow101 commented at 11:30 pm on June 28, 2018:

I think it is clearer to list the fingerprint separately.

in src/rpc/rawtransaction.cpp:1588 in af544a96fa outdated

1583+            else {
1584+                continue;
1585+            }
1586+        }
1587+
1588+        for (const PartiallySignedTransaction& psbtx : psbtxs) {

sipa commented at 10:28 pm on June 28, 2018:

It feels like this should be written as a method or function operating on PartiallySignedTransaction objects, rather than inside the RPC code.

achow101 commented at 11:32 pm on June 28, 2018:

Why?

sipa commented at 11:34 pm on June 28, 2018:

Because it’s not RPC specific. It’s cleaner to put the operations on the lowest level they can go.

achow101 commented at 2:24 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1555 in af544a96fa outdated

1550+    PartiallySignedTransaction merged_psbtx(psbtxs[0]); // Copy the first one
1551+    // Merge input data
1552+    for (unsigned int i = 0; i < merged_psbtx.tx.vin.size(); ++i) {
1553+        CTxIn& txin = merged_psbtx.tx.vin[i];
1554+
1555+        // Find the utxo from one of the psbtxs

sipa commented at 10:29 pm on June 28, 2018:

I’m not sure what this entire UTXO handling section does. Is it sufficient to (a) rely on a generic combine PSBT function somewhere (suggested below) which also combined the utxo data, and then perhaps convert the logic in this section to a sanity check function?

achow101 commented at 2:24 am on June 29, 2018:

I have refactored the merging stuff into a Merge() method for each PSBT struct. I also added a sanity checking method for each.

in src/rpc/rawtransaction.cpp:1700 in af544a96fa outdated

1695+        CTxOut utxo;
1696+        if (input.non_witness_utxo) {
1697+            utxo = input.non_witness_utxo->vout[txin.prevout.n];
1698+        }
1699+        // Now find the witness utxo if the non witness doesn't exist
1700+        else if (!input.witness_utxo.IsNull()) {

sipa commented at 10:31 pm on June 28, 2018:

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

achow101 commented at 2:25 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1289 in af544a96fa outdated

1284+UniValue decodepsbt(const JSONRPCRequest& request)
1285+{
1286+    if (request.fHelp || request.params.size() != 1)
1287+        throw std::runtime_error(
1288+            "decodepsbt \"base64string\"\n"
1289+            "\nReturn a JSON object representing the serialized, hex-encoded partially signed Bitcoin transaction.\n"

sipa commented at 10:46 pm on June 28, 2018:

Not hex encoded.

achow101 commented at 2:25 am on June 29, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1553 in af544a96fa outdated

1498+UniValue combinepsbt(const JSONRPCRequest& request)
1499+{
1500+    if (request.fHelp || request.params.size() != 1)
1501+        throw std::runtime_error(
1502+            "combinepsbt [\"base64string\",...]\n"
1503+            "\nCombine multiple partially signed Bitcoin transactions into one transaction.\n"

sipa commented at 10:47 pm on June 28, 2018:

Explain that this implements the Combiner and Finalizer roles?

achow101 commented at 2:25 am on June 29, 2018:

Done

sipa commented at 2:24 am on July 4, 2018:

Well perhaps don’t say it implements a Finalizer unless it actually implements that :)

in src/rpc/rawtransaction.cpp:1655 in af544a96fa outdated

1650+            "                             extract and return the complete transaction in normal network serialization.\n"
1651+
1652+            "\nResult:\n"
1653+            "{\n"
1654+            "  \"base64\" : \"value\",        (string) The base64-encoded network transaction\n"
1655+            "  \"hex\" : \"value\",           (string) The hex-encoded partially signed transaction if extracted\n"

sipa commented at 10:49 pm on June 28, 2018:

Fully signed transaction, no? It seems the explanation of this and the field above are swapped.

achow101 commented at 2:25 am on June 29, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1690 in af544a96fa outdated

1685+        // if this input has a final scriptsig or scriptwitness, don't do anything with it
1686+        if (!input.final_script_sig.empty() || !input.final_script_witness.IsNull()) {
1687+            continue;
1688+        }
1689+
1690+        // Fill a SignatureData with input info

sipa commented at 10:51 pm on June 28, 2018:

This whole section that extracts UTXOs and data from a PSBT input and invokes ProduceSignature on it seems like something that can be abstracted out.

achow101 commented at 2:25 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1739 in af544a96fa outdated

1734+UniValue createpsbt(const JSONRPCRequest& request)
1735+{
1736+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
1737+        throw std::runtime_error(
1738+                            "createpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable )\n"
1739+                            "\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"

sipa commented at 10:52 pm on June 28, 2018:

This RPC does not do any funding, I think.

Explain that this implements a Creator role.

achow101 commented at 2:25 am on June 29, 2018:

Done

in src/uint256.h:94 in ed7a484d92 outdated

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

sipa commented at 11:04 pm on June 28, 2018:

You can use ReadLE32 for this, it’s more efficient.

If instead this would be BE (ReadBE32 also exists), you can get rid of Uint32ToUint8VectorLE and instead print using strprintf("%08x", fingerprint).

achow101 commented at 2:26 am on June 29, 2018:

Done

in src/rpc/rawtransaction.cpp:1803 in af544a96fa outdated

1798+UniValue converttopsbt(const JSONRPCRequest& request)
1799+{
1800+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
1801+        throw std::runtime_error(
1802+                            "converttopsbt \"hexstring\" ( permitsigdata iswitness )\n"
1803+                            "\nConverts a network serialized transaction to a PSBT.\n"

sipa commented at 11:06 pm on June 28, 2018:

Perhaps clarify that it’s intended to work with createrawtransaction and fundrawtransaction, and createpsbt/createfundedpsbt should be used for new applications.

achow101 commented at 2:26 am on June 29, 2018:

Done

sipa commented at 11:07 pm on June 28, 2018: member

Comments on rawtransaction.cpp RPCs.

sipa commented at 11:13 pm on June 28, 2018: member

General comment on naming of RPC field names and input arguments, "base64" isn’t very informative I think. I suggest using "psbt" anywhere you have an input or output in base64. There isn’t any other encoding used, so there should never be any confusion. Using "hex" for fully signed or legacy partially signed transaction makes sense for consistency with the existing RPCs.

achow101 force-pushed on Jun 29, 2018

achow101 commented at 2:26 am on June 29, 2018: member

I did a bit of commit splitting and have reduced the size of the RPCs commit by splitting it up. Hopefully this will make review easier.

in src/script/sign.cpp:238 in c751edcff5 outdated

234@@ -235,6 +235,36 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
235     return sigdata.complete;
236 }
237 
238+bool SignPSBTInput(const SigningProvider& provider, const CTxIn& txin, const CMutableTransaction& tx, PSBTInput& input, SignatureData& sigdata, int index, int sighash, bool sign)

sipa commented at 6:37 am on June 29, 2018:

No need to pass txin; it’s always equal to tx.vin[index] I think.

achow101 commented at 6:15 pm on June 29, 2018:

Done

in src/script/sign.cpp:254 in c751edcff5 outdated

249+    CTxOut utxo;
250+    if (input.non_witness_utxo) {
251+        utxo = input.non_witness_utxo->vout[txin.prevout.n];
252+    }
253+    // Now find the witness utxo if the non witness doesn't exist
254+    else if (!input.witness_utxo.IsNull()) {

sipa commented at 6:37 am on June 29, 2018:

Nit: else on the same line as }.

achow101 commented at 6:15 pm on June 29, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4394 in 00d2d6b87d outdated

4390@@ -4388,6 +4391,333 @@ UniValue sethdseed(const JSONRPCRequest& request)
4391     return NullUniValue;
4392 }
4393 
4394+bool parse_hd_keypath(std::string keypath_str, std::vector<uint32_t>& keypath)

sipa commented at 6:41 am on June 29, 2018:

Nit: function naming style

achow101 commented at 6:15 pm on June 29, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4433 in 00d2d6b87d outdated

4428+        first = false;
4429+    }
4430+    return true;
4431+}
4432+
4433+void add_keypath_to_map(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)

sipa commented at 6:41 am on June 29, 2018:

Nit: function naming style

achow101 commented at 6:15 pm on June 29, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4464 in 00d2d6b87d outdated

4459+        keypath.insert(keypath.begin(), ReadLE32(vchPubKey.GetID().begin()));
4460+    }
4461+    hd_keypaths.emplace(vchPubKey, keypath);
4462+}
4463+
4464+bool fill_psbt(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign)

sipa commented at 6:43 am on June 29, 2018:

Nit: function naming style

achow101 commented at 6:16 pm on June 29, 2018:

Fixed

achow101 force-pushed on Jun 29, 2018

nvk commented at 6:16 pm on June 29, 2018: none

We will start implementing as soon as this gets merged. Thanks for sorting it out so fast 👍

in src/script/sign.h:255 in 48f2cf53ea outdated

250+
251+    template <typename Stream>
252+    inline void Unserialize(Stream& s) {
253+        // Read loop
254+        while(!s.empty()) {
255+            // read size of key

sipa commented at 0:34 am on June 30, 2018:

This section (up to s >> MakeSpan(key)) can be written more concisely as:

0std::vector<unsigned char> key;
1s >> key;
2if (key.empty()) return;

achow101 commented at 1:06 am on June 30, 2018:

Done

in src/script/sign.h:278 in 48f2cf53ea outdated

273+            switch(type) {
274+                case PSBT_IN_NON_WITNESS_UTXO:
275+                    if (non_witness_utxo) {
276+                        throw std::ios_base::failure("Duplicate Key, input non-witness utxo already provided");
277+                    }
278+                    if (!witness_utxo.IsNull()) {

sipa commented at 0:39 am on June 30, 2018:

I guess this is now done by the IsSane() function, and doesn’t need to be repeated here?

achow101 commented at 1:06 am on June 30, 2018:

Removed

in src/script/sign.h:288 in 48f2cf53ea outdated

283+                case PSBT_IN_WITNESS_UTXO:
284+                    if (!witness_utxo.IsNull()) {
285+                        throw std::ios_base::failure("Duplicate Key, input witness utxo already provided");
286+                    }
287+                    if (non_witness_utxo) {
288+                        throw std::ios_base::failure("A non-witness utxo was already provided, cannot provide a witness utxo");

sipa commented at 0:39 am on June 30, 2018:

Same thing here.

achow101 commented at 1:06 am on June 30, 2018:

Done

in src/script/sign.h:312 in 48f2cf53ea outdated

307+                    // Read in the signature from value
308+                    std::vector<unsigned char> sig;
309+                    s >> sig;
310+
311+                    // Add to list
312+                    partial_sigs.emplace(pubkey.GetID(), SigPair(pubkey, sig));

sipa commented at 0:40 am on June 30, 2018:

Can be std::move(sig).

achow101 commented at 1:06 am on June 30, 2018:

Done

in src/script/sign.h:420 in 48f2cf53ea outdated

415+
416+    template <typename Stream>
417+    inline void Unserialize(Stream& s) {
418+        // Read loop
419+        while(!s.empty()) {
420+            // read size of key

sipa commented at 0:44 am on June 30, 2018:

The same shorter read routine is possible here.

achow101 commented at 1:07 am on June 30, 2018:

Done

in src/script/sign.h:549 in 48f2cf53ea outdated

544+        s >> magic_sep;
545+
546+        // Read global data
547+        while(!s.empty()) {
548+            // read size of key
549+            uint64_t key_len = ReadCompactSize(s);

sipa commented at 0:44 am on June 30, 2018:

And again here.

achow101 commented at 1:07 am on June 30, 2018:

Done

sipa commented at 0:52 am on June 30, 2018: member

Some more nits on serialization code.

achow101 force-pushed on Jun 30, 2018

in src/core_read.cpp:174 in d59ebaa3c0 outdated

169+        ssData >> psbt;
170+        if (!ssData.empty()) {
171+            error = "extra data after PSBT";
172+            return false;
173+        }
174+    } catch (const std::exception& e) {

sipa commented at 1:53 am on June 30, 2018:

Perhaps check for ssData.eof() here (so that adding garbage at the end of the base64 string is rejected).

achow101 commented at 2:22 am on June 30, 2018:

This is checked for above with if (!ssData.empty()) {

sipa commented at 2:28 am on June 30, 2018:

I should learn to read.

in src/core_read.cpp:166 in d59ebaa3c0 outdated

160@@ -160,6 +161,23 @@ bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk)
161     return true;
162 }
163 
164+bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error)
165+{
166+    std::vector<unsigned char> txData = DecodeBase64(base64_tx.c_str());

sipa commented at 1:54 am on June 30, 2018:

Nit: variable naming style.

sipa commented at 1:54 am on June 30, 2018:

You can drop the .c_str().

achow101 commented at 2:34 am on June 30, 2018:

DecodeBase64 takes an unsigned char* while base64_tx is a std::string. It fails to compile without c_str()

sipa commented at 2:42 am on June 30, 2018:

There is also a version of DecodeBase64 which takes a const std::string&.

achow101 commented at 2:45 am on June 30, 2018:

Yes, but not to a std::vector<unsigned char>. It decodes to a std::string, but I need a std::vector<unsigned char>

achow101 commented at 2:46 am on June 30, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1247 in d59ebaa3c0 outdated

1262@@ -1262,6 +1263,551 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
1263     return result;
1264 }
1265 
1266+static std::string WriteHDKeypath(std::vector<uint32_t>& keypath)

sipa commented at 1:55 am on June 30, 2018:

This adds a superfluous / at the end of the string.

achow101 commented at 2:46 am on June 30, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1289 in d59ebaa3c0 outdated

1284+
1285+UniValue decodepsbt(const JSONRPCRequest& request)
1286+{
1287+    if (request.fHelp || request.params.size() != 1)
1288+        throw std::runtime_error(
1289+            "decodepsbt \"base64string\"\n"

sipa commented at 1:57 am on June 30, 2018:

What do you think about renaming input psbt arguments from "base64" or "base64string" to "psbt" (and similar for output object field names)?

achow101 commented at 2:46 am on June 30, 2018:

Renamed

in src/rpc/rawtransaction.cpp:1569 in d59ebaa3c0 outdated

1564+        throw std::runtime_error(
1565+            "combinepsbt [\"base64string\",...]\n"
1566+            "\nCombine multiple partially signed Bitcoin transactions into one transaction.\n"
1567+            "Implements the Combiner and Finalizer roles.\n"
1568+            "\nArguments:\n"
1569+            "1. \"txs\"                   (string) A json array of hex strings of partially signed transactions\n"

sipa commented at 1:58 am on June 30, 2018:

Not hex.

achow101 commented at 2:46 am on June 30, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1611 in d59ebaa3c0 outdated

1604+        }
1605+    }
1606+
1607+    PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
1608+    // Merge them
1609+    for (const PartiallySignedTransaction& psbtx : psbtxs) {

sipa commented at 1:59 am on June 30, 2018:

No need to merge in the first one again.

achow101 commented at 2:46 am on June 30, 2018:

Changed to erase the first element from the vector.

in src/rpc/rawtransaction.cpp:1613 in d59ebaa3c0 outdated

1608+    // Merge them
1609+    for (const PartiallySignedTransaction& psbtx : psbtxs) {
1610+        merged_psbt.Merge(psbtx);
1611+    }
1612+    if (!merged_psbt.IsSane()) {
1613+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Merged PSBT is not sane");

sipa commented at 1:59 am on June 30, 2018:

“Not sane” sounds funny. What about “Resulting PSBT would be inconsistent”?

achow101 commented at 2:46 am on June 30, 2018:

Done

in src/rpc/rawtransaction.cpp:1598 in d59ebaa3c0 outdated

1614+    }
1615+
1616+    UniValue result(UniValue::VOBJ);
1617+    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
1618+    ssTx << merged_psbt;
1619+    return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());

sipa commented at 2:00 am on June 30, 2018:

Is the cast necessary here?

achow101 commented at 2:34 am on June 30, 2018:

Yes. EncodeBase64 takes an unsigned char* but ssTx.data() is a char *.

in src/rpc/rawtransaction.cpp:1760 in d59ebaa3c0 outdated

1755+                            "converttopsbt \"hexstring\" ( permitsigdata iswitness )\n"
1756+                            "\nConverts a network serialized transaction to a PSBT. This should be used only with createrawtransaction and fundrawtransaction\n"
1757+                            "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
1758+                            "\nArguments:\n"
1759+                            "1. \"hexstring\"              (string, required) The hex string of a raw transaction\n"
1760+                            "2. permitsigdata           (boolean, optional) Whether the transaction hex contains any data in the scriptSigs or scriptWitnesses.\n"

sipa commented at 2:05 am on June 30, 2018:

This explanation is confusing. It’s an input argument, right? How about “If true, the RPC will ignore any signatures present in the input. When this argument is false, it will fail if signatures are present.”

achow101 commented at 2:47 am on June 30, 2018:

Changed

achow101 force-pushed on Jun 30, 2018

in src/wallet/rpcwallet.cpp:3621 in 37236e54c4 outdated

3621 
3622     UniValue result(UniValue::VOBJ);
3623     result.pushKV("hex", EncodeHexTx(tx));
3624-    result.pushKV("changepos", changePosition);
3625-    result.pushKV("fee", ValueFromAmount(nFeeOut));
3626+    result.pushKV("fee", ValueFromAmount(fee));

sipa commented at 7:00 pm on June 30, 2018:

Why swap these?

achow101 commented at 6:51 pm on July 2, 2018:

To match the order in the help text.

in src/script/sign.h:466 in c7154487c6 outdated

461+};
462+
463+/** A version of CTransaction with the PSBT format*/
464+struct PartiallySignedTransaction
465+{
466+    CMutableTransaction tx;

sipa commented at 7:02 pm on June 30, 2018:

Is this were a std::unique_ptr<CMutableTransaction> (or boost::optional<CMutableTransaction>), you could avoid the need for implementing IsNull or converting to CTransaction to decide whether it is set or not.

achow101 commented at 9:13 pm on July 2, 2018:

Made into a boost::optional

in src/wallet/rpcwallet.cpp:4469 in a871194fee outdated

4464+bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign)
4465+{
4466+    // Get all of the previous transactions
4467+    bool complete = true;
4468+    for (unsigned int i = 0; i < txConst->vin.size(); ++i) {
4469+        CTxIn txin = txConst->vin[i];

sipa commented at 7:08 pm on June 30, 2018:

You can use a const reference here to avoid a copy.

achow101 commented at 9:13 pm on July 2, 2018:

Done

in src/wallet/rpcwallet.cpp:4473 in a871194fee outdated

4468+    for (unsigned int i = 0; i < txConst->vin.size(); ++i) {
4469+        CTxIn txin = txConst->vin[i];
4470+        PSBTInput& input = psbtx.inputs.at(i);
4471+
4472+        // If we don't know about this input, skip it and let someone else deal with it
4473+        uint256 txhash = txin.prevout.hash;

sipa commented at 7:09 pm on June 30, 2018:

Same here.

achow101 commented at 9:13 pm on July 2, 2018:

Done

in src/rpc/rawtransaction.cpp:1661 in a871194fee outdated

1656+    }
1657+
1658+    // Get all of the previous transactions
1659+    bool complete = true;
1660+    for (unsigned int i = 0; i < psbtx.tx.vin.size(); ++i) {
1661+        CTxIn txin = psbtx.tx.vin[i];

sipa commented at 7:09 pm on June 30, 2018:

Unused variable.

achow101 commented at 9:13 pm on July 2, 2018:

Removed

in src/wallet/rpcwallet.cpp:4554 in a871194fee outdated

4543+            + HelpRequiringPassphrase(pwallet) + "\n"
4544+
4545+            "\nArguments:\n"
4546+            "1. \"psbt\"                      (string, required) The transaction base64 string\n"
4547+            "2. \"sign\",                     (string, optional, default=true) Also sign the transaction when updating\n"
4548+            "3. \"sighashtype\"            (string, optional, default=ALL) The signature hash type to sign with if not specified by the PSBT. Must be one of\n"

sipa commented at 7:14 pm on June 30, 2018:

I think the semantics should be to fail if the specified sighash type (or the default) does not match the sighash type in the PSBT.

achow101 commented at 9:13 pm on July 2, 2018:

Done

in src/wallet/rpcwallet.cpp:4488 in a871194fee outdated

4483+        if (input.sighash_type > 0) {
4484+            sighash = input.sighash_type;
4485+        }
4486+
4487+        SignatureData sigdata;
4488+        complete &= SignPSBTInput(*pwallet, psbtx.tx, input, sigdata, i, sighash, sign);

sipa commented at 7:17 pm on June 30, 2018:

If you’d have a SigningProvider wrapper that removes access to private keys, you could use that instead of passing down a sign variable. The other call site doesn’t need a sign anyway, as it’s using a dummy provider.

achow101 commented at 9:13 pm on July 2, 2018:

Done.

in src/wallet/rpcwallet.cpp:4609 in a871194fee outdated

4622+        return NullUniValue;
4623+    }
4624+
4625+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)
4626+        throw std::runtime_error(
4627+                            "walletcreatefundedpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable ) ( options )\n"

sipa commented at 7:19 pm on June 30, 2018:

Thinking more about this, I can’t find a reason why someone would not want to also Update the PSBT. Since in this case, the wallet is adding inputs and change, it is exactly in the position to also add keys/scripts/utxos for those. It shouldn’t include signing, though.

achow101 commented at 7:48 pm on July 2, 2018:

In order to sign the PSBT, you still have to use walletupdatepsbt and that will update the PSBT at the same time.

sipa commented at 7:55 pm on July 2, 2018:

I meant: why should createfunded not automatically update? It by definition has all the data available, and I don’t think there is a reason why you wouldn’t run update immediately after anyway.

achow101 commented at 9:13 pm on July 2, 2018:

Changed to also update

achow101 force-pushed on Jul 2, 2018

in src/rpc/rawtransaction.cpp:1282 in e7707b610a outdated

1277+        if (hardened) {
1278+            keypath_str += "'";
1279+        }
1280+        keypath_str += "/";
1281+    }
1282+    return keypath_str.substr(0, keypath_str.size() - 1); // Drop last '/'

sipa commented at 0:11 am on July 4, 2018:

Alternatively: move the keypath_str += "/"; to the front of the loop and initialize with just "m". That way you can avoid adding and then stripping the final slash.

achow101 commented at 2:05 am on July 4, 2018:

Done

in src/rpc/rawtransaction.cpp:1316 in e7707b610a outdated

1311+            "        \"scriptPubKey\" : {          (json object)\n"
1312+            "          \"asm\" : \"asm\",            (string) The asm\n"
1313+            "          \"hex\" : \"hex\",            (string) The hex\n"
1314+            "          \"reqSigs\" : n,            (numeric) The required sigs\n"
1315+            "          \"type\" : \"pubkeyhash\",    (string) The type, eg 'pubkeyhash'\n"
1316+            "          \"addresses\" : [           (json array of string)\n"

sipa commented at 0:17 am on July 4, 2018:

I think it’s bad practice to keep outputting “addresses” that correspond to a script. It’s confusing (they’re not really addresses, just multisig pubkeys represented through their corresponding P2PKH address) and also generally useless as it can’t introspect into P2SH and P2WSH.

I preference would be to just output “hex”, “asm” and “address” (only if a corresponding address is known through ExtractDestination).

achow101 commented at 2:05 am on July 4, 2018:

Done

in src/rpc/rawtransaction.cpp:1366 in e7707b610a outdated

1361+            "    ,...\n"
1362+            "  ]\n"
1363+            "  \"outputs\" : [                 (array of json objects)\n"
1364+            "    {\n"
1365+            "      \"redeem_script\" : {       (json object, optional)\n"
1366+            "          \"script\" : {           (json object)\n"

sipa commented at 0:29 am on July 4, 2018:

What is this “script” intermediate layer? I don’t see that in the implementation (for this and for witness_script). Again, just “hex” and “asm” is sufficient here I think.

achow101 commented at 2:05 am on July 4, 2018:

Removed, that’s a mistake

in src/rpc/rawtransaction.cpp:1437 in e7707b610a outdated

1432+        if (!input.witness_utxo.IsNull()) {
1433+            const CTxOut& txout = input.witness_utxo;
1434+
1435+            UniValue out(UniValue::VOBJ);
1436+
1437+            out.pushKV("value", ValueFromAmount(txout.nValue));

sipa commented at 0:30 am on July 4, 2018:

For compatibility with listunspent you may want to call this "amount".

achow101 commented at 2:05 am on July 4, 2018:

Done

in src/rpc/rawtransaction.cpp:1422 in e7707b610a outdated

1446+            in.pushKV("non_witness_utxo", non_wit);
1447+        }
1448+
1449+        // Partial sigs
1450+        if (!input.partial_sigs.empty()) {
1451+            UniValue partial_sigs(UniValue::VARR);

sipa commented at 0:32 am on July 4, 2018:

How about modelling this as an object whose keys are hex pubkeys, and signatures are hex values?

So {"<p1>":"<sig1>",...} instead of [{"pubkey":"<pk1>","signature":""<sig1>"},...].

achow101 commented at 2:05 am on July 4, 2018:

Done

sipa commented at 2:11 am on July 4, 2018:

Now you have [{"<p1>":"<sig1>"},{"<p2>":"<sig2>"},...]. You can get rid of the internal objects too: {"<p1>":"<sig1>","<p2>":"<sig2>",...}.

achow101 commented at 2:52 am on July 4, 2018:

Done.

sipa commented at 11:52 pm on July 7, 2018:

It seems you’ll need to change the type to UniValue::VOBJ above. Calling pushKV on an array has no effect (and also doesn’t assert/throw anything, it seems…).

achow101 commented at 8:25 pm on July 9, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1497 in e7707b610a outdated

1492+            in.pushKV("bip32_derivs", keypaths);
1493+        }
1494+
1495+        // Final scriptSig and scriptwitness
1496+        if (!input.final_script_sig.empty()) {
1497+            in.pushKV("final_scriptSig", HexStr(input.final_script_sig));

sipa commented at 0:33 am on July 4, 2018:

Here you may want to have both hex and asm.

achow101 commented at 2:05 am on July 4, 2018:

Done

in src/rpc/rawtransaction.cpp:1601 in e7707b610a outdated

1596+        }
1597+        psbtxs.push_back(psbtx);
1598+    }
1599+
1600+    PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
1601+    psbtxs.erase(psbtxs.begin());

sipa commented at 0:36 am on July 4, 2018:

psbtxs.pop_front() will do the same. Alternatively, you can write a loop like for (auto it = std::next(psbtx.begin()); ++it; it !=psbtx.end()) { merged_psbt.Merge(*it); }.

achow101 commented at 2:06 am on July 4, 2018:

std::vector doesn’t have a pop_front(), so I used your other suggestion for the loop.

in src/rpc/rawtransaction.cpp:1780 in e7707b610a outdated

1775+
1776+    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VBOOL, UniValue::VBOOL}, true);
1777+
1778+    // parse hex string from parameter
1779+    CMutableTransaction tx;
1780+    bool try_witness = !request.params[1].isNull() ? (request.params[2].isNull() ? true : request.params[2].get_bool()) : true;

sipa commented at 0:43 am on July 4, 2018:

These boolean expressions and the ones below are pretty hard to follow.

What about first extracting the boolean values of the arguments, and then writing things in function of those?

achow101 commented at 2:06 am on July 4, 2018:

Changed

in src/wallet/rpcwallet.cpp:4458 in b8b9e7e931 outdated

4453+        masterKey.SetSeed(key.begin(), key.size());
4454+        // Add to map
4455+        keypath.insert(keypath.begin(), ReadLE32(masterKey.key.GetPubKey().GetID().begin()));
4456+    }
4457+    // Single pubkeys get the master fingerprint of themselves
4458+    else {

sipa commented at 0:44 am on July 4, 2018:

Style nit: else on the same line

achow101 commented at 2:06 am on July 4, 2018:

Fixed

achow101 force-pushed on Jul 4, 2018

in src/rpc/rawtransaction.cpp:1324 in 336f31d78d outdated

1319+            "        {\n"
1320+            "          \"pubkey\" : \"signature\",           (string) The public key and signature that corresponds to it.\n"
1321+            "        }\n"
1322+            "        ,...\n"
1323+            "      ]\n"
1324+            "      \"sighash\" : \"type\",                  (string, optional) The sighash type recommended to be used\n"

sipa commented at 2:13 am on July 4, 2018:

Not just recommended.

achow101 commented at 2:52 am on July 4, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1335 in 336f31d78d outdated

1330+            "      \"witness_script\" : {       (json object, optional)\n"
1331+            "          \"asm\" : \"asm\",            (string) The asm\n"
1332+            "          \"hex\" : \"hex\",            (string) The hex\n"
1333+            "          \"type\" : \"pubkeyhash\",    (string) The type, eg 'pubkeyhash'\n"
1334+            "        }\n"
1335+            "       \"bip32_derivs\" : [          (array of json objects, optional)\n"

sipa commented at 2:15 am on July 4, 2018:

This could be modeled as an object with the pubkey as key, and {"master_fingerprint":"...","path":"..."} as value.

achow101 commented at 2:52 am on July 4, 2018:

Done

in src/rpc/rawtransaction.cpp:1362 in 336f31d78d outdated

1379+            "         ...\n"
1380+            "      },\n"
1381+            "    }\n"
1382+            "    ,...\n"
1383+            "  ]\n"
1384+            "}\n"

sipa commented at 2:20 am on July 4, 2018:

It would be very useful to add the fee of the transaction here, if known. Make sure there is a consistency check that non-witness utxo’s txid matches the vin.prevout.hash.

achow101 commented at 3:16 am on July 4, 2018:

Done. The consistency check occurs at deserialization time.

in src/rpc/rawtransaction.cpp:1569 in 336f31d78d outdated

1564+            "      ,...\n"
1565+            "    ]\n"
1566+
1567+            "\nResult:\n"
1568+            "{\n"
1569+            "  \"psbt\" : \"value\",          (string) The base64-encoded partially signed transaction\n"

sipa commented at 2:25 am on July 4, 2018:

That doesn’t look like the output it’s actually producing.

achow101 commented at 2:53 am on July 4, 2018:

Fixed here and elsewhere

in src/rpc/rawtransaction.cpp:1706 in 336f31d78d outdated

1701+                            "   ]\n"
1702+                            "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
1703+                            "4. replaceable               (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"
1704+                            "                             Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n"
1705+                            "\nResult:\n"
1706+                            "  \"psbt\": \"value\",        (string)  The resulting raw transaction (base64-encoded string)\n"

sipa commented at 2:26 am on July 4, 2018:

The output looks like just a string, not an object.

achow101 commented at 2:53 am on July 4, 2018:

Fixed here and elsewhere

in src/rpc/rawtransaction.cpp:1754 in 336f31d78d outdated

1749+                            "                              If false, RPC will fail if any signatures are present.\n"
1750+                            "3. iswitness               (boolean, optional) Whether the transaction hex is a serialized witness transaction.\n"
1751+                            "                              If iswitness is not present, heuristic tests will be used in decoding. Only has an effect if\n"
1752+                            "                              permitsigdata is true\n"
1753+                            "\nResult:\n"
1754+                            "  \"psbt\": \"value\",        (string)  The resulting raw transaction (base64-encoded string)\n"

sipa commented at 2:27 am on July 4, 2018:

Same here.

in src/script/sign.h:38 in cc05bb068e outdated

31@@ -32,6 +32,17 @@ class SigningProvider
32 
33 extern const SigningProvider& DUMMY_SIGNING_PROVIDER;
34 
35+class PublicOnlySigningProvider : public SigningProvider
36+{
37+private:
38+    const SigningProvider* provider;

sipa commented at 2:27 am on July 4, 2018:

Style nit: m_provider.

achow101 force-pushed on Jul 4, 2018

laanwj added the label RPC/REST/ZMQ on Jul 4, 2018

in src/wallet/rpcwallet.cpp:4544 in 9e4077a992 outdated

4534+
4535+    if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) {
4536+        return NullUniValue;
4537+    }
4538+
4539+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)

promag commented at 9:08 pm on July 4, 2018:

> 3

achow101 commented at 10:08 pm on July 5, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4588 in 9e4077a992 outdated

4584+            {std::string("NONE"), int(SIGHASH_NONE)},
4585+            {std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE|SIGHASH_ANYONECANPAY)},
4586+            {std::string("SINGLE"), int(SIGHASH_SINGLE)},
4587+            {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
4588+        };
4589+        std::string strHashType = request.params[1].get_str();

promag commented at 9:13 pm on July 4, 2018:

[2]?

promag commented at 9:16 pm on July 4, 2018:

Nit, could move this to a ParseSigHashType()? Like ParseOutputType and ParseConfirmTarget.

achow101 commented at 10:33 pm on July 5, 2018:

Moved to a new helper function.

in src/wallet/rpcwallet.cpp:4586 in 9e4077a992 outdated

4598+    // transaction to avoid rehashing.
4599+    const CTransaction txConst(*psbtx.tx);
4600+
4601+    // Fill transaction with our data and also sign
4602+    bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
4603+    bool fComplete = FillPSBT(pwallet, psbtx, &txConst, nHashType, sign);

promag commented at 9:17 pm on July 4, 2018:

nit, just complete.

achow101 commented at 10:08 pm on July 5, 2018:

Done

in src/wallet/rpcwallet.cpp:4567 in 9e4077a992 outdated

4563+
4564+            "\nExamples:\n"
4565+            + HelpExampleCli("walletprocesspsbt", "\"psbt\"")
4566+        );
4567+
4568+    LOCK2(cs_main, pwallet->cs_wallet);

promag commented at 9:22 pm on July 4, 2018:

Why lock cs_main? Locking could be in FillPSBT?

achow101 commented at 10:09 pm on July 5, 2018:

Removed cs_main, moved to FillPSBT.

in src/wallet/rpcwallet.cpp:4547 in 9e4077a992 outdated

4543+            "that we can sign for.\n"
4544+            + HelpRequiringPassphrase(pwallet) + "\n"
4545+
4546+            "\nArguments:\n"
4547+            "1. \"psbt\"                      (string, required) The transaction base64 string\n"
4548+            "2. \"sign\",                     (string, optional, default=true) Also sign the transaction when updating\n"

promag commented at 9:23 pm on July 4, 2018:

boolean? Drop \" in sign.

achow101 commented at 10:11 pm on July 5, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4540 in 9e4077a992 outdated

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

promag commented at 9:25 pm on July 4, 2018:

"sighashtype"?

achow101 commented at 10:11 pm on July 5, 2018:

Fixed

in src/rpc/client.cpp:120 in 9e4077a992 outdated

108@@ -109,6 +109,11 @@ static const CRPCConvertParam vRPCConvertParams[] =
109     { "combinerawtransaction", 0, "txs" },
110     { "fundrawtransaction", 1, "options" },
111     { "fundrawtransaction", 2, "iswitness" },
112+    { "walletcreatefundedpsbt", 0, "inputs" },
113+    { "walletcreatefundedpsbt", 1, "outputs" },
114+    { "walletcreatefundedpsbt", 2, "locktime" },
115+    { "walletcreatefundedpsbt", 3, "replaceable" },
116+    { "walletcreatefundedpsbt", 4, "options" },
117     { "createpsbt", 0, "inputs" },

promag commented at 9:27 pm on July 4, 2018:

Missing { "walletprocesspsbt", 1, "sign" }?

achow101 commented at 10:11 pm on July 5, 2018:

Added

in src/wallet/rpcwallet.cpp:4471 in 9e4077a992 outdated

4467+        const CTxIn& txin = txConst->vin[i];
4468+        PSBTInput& input = psbtx.inputs.at(i);
4469+
4470+        // If we don't know about this input, skip it and let someone else deal with it
4471+        const uint256& txhash = txin.prevout.hash;
4472+        if (pwallet->mapWallet.count(txhash)) {

promag commented at 9:28 pm on July 4, 2018:

Use find to avoid 2nd lookup below.

achow101 commented at 10:11 pm on July 5, 2018:

Done

promag commented at 9:37 pm on July 4, 2018: member

Comments regarding “Create wallet RPCs for PSBT” (9e4077a)

No cs_wallet lock in walletcreatefundedpsbt?

Instead of walletcreatefundedpsbt, could make a rawtransactiontopsbt or alike?

in src/script/sign.cpp:609 in dbd6d74ec6 outdated

604+bool PSBTOutput::IsNull() const
605+{
606+    return redeem_script.empty() && witness_script.empty() && hd_keypaths.empty() && unknown.empty();
607+}
608+
609+void PSBTOutput::Merge(const PSBTOutput& output)

araspitzu commented at 1:37 pm on July 5, 2018:

Perhaps i’m missing something but shouldn’t we merge the unknowns from the output too? (as done in PSBTInput::Merge)

achow101 commented at 10:11 pm on July 5, 2018:

Good catch, fixed

instagibbs commented at 3:38 pm on July 5, 2018: member

Needs rebase now that #13425 is merged. (I think?)

achow101 force-pushed on Jul 5, 2018

achow101 commented at 6:00 pm on July 5, 2018: member

Rebased onto master after #13425 was merged. Will address comments soon.

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

in src/script/sign.h:103 in dfa5afe665 outdated

 98+static constexpr uint8_t PSBT_OUT_REDEEMSCRIPT = 0x00;
 99+static constexpr uint8_t PSBT_OUT_WITNESSSCRIPT = 0x01;
100+static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
101+
102+// The separator is 0x00. Reading this in means that the unserializer can interpret it
103+// as a 0 length key. which indicates that this is the separator. The separator has no value.

instagibbs commented at 8:55 pm on July 5, 2018:

micro-nit: period after key seems misplaced

achow101 commented at 8:12 pm on July 9, 2018:

Fixed

in src/script/sign.h:145 in dfa5afe665 outdated

127+    }
128+}
129+
130+// Deserialize HD keypaths into a map
131+template<typename Stream>
132+void DeserializeHDKeypaths(Stream& s, const std::vector<unsigned char>& key, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)

instagibbs commented at 9:00 pm on July 5, 2018:

this deserialized a single keypath, right?

Update description, function name to something like DeserializeAndAppendHDKeypath, and rename hd_keypaths to hd_keypath.

sipa commented at 2:28 pm on July 9, 2018:

No, it deserializes all keypaths for one input or one output.

instagibbs commented at 2:30 pm on July 9, 2018:

I deleted this comment a while back, github reviews thought it was too good I guess

in src/script/sign.h:173 in dfa5afe665 outdated

168+            s << path;
169+        }
170+    }
171+}
172+
173+/** A structure for PSBTs which contain per input information */

instagibbs commented at 9:02 pm on July 5, 2018:

micro-nit: per-input

achow101 commented at 8:12 pm on July 9, 2018:

Fixed

achow101 force-pushed on Jul 5, 2018

achow101 commented at 10:44 pm on July 5, 2018: member

Instead of walletcreatefundedpsbt, could make a rawtransactiontopsbt or alike?

There already is a converttopsbt method. I do not think that we should make using PSBT require using raw transactions first. It should be self contained as I hope that we will move from raw transactions to PSBT entirely.

in src/script/sign.h:175 in 8415956f70 outdated

170+    hd_keypaths.emplace(pubkey, keypath);
171+}
172+
173+// Serialize HD keypaths to a stream from a map
174+template<typename Stream>
175+void SerializeHDKeypaths(Stream& s, const std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths, uint8_t type)

araspitzu commented at 10:47 am on July 6, 2018:

I’m having some trouble matching this code to the spec, with the HD keypaths shouldn’t we prefix the “value” (as in the key/value entry) with the fingerprint of the public key? I don’t see where it’s done here (and neither in ‘DeserializeHDKeypaths’).

achow101 commented at 6:24 pm on July 6, 2018:

Since the fingerprint is a 4 byte value, we store it with the keypaths as the first element of the std::vector<uint32_t>

in test/functional/rpc_psbt.py:41 in 8415956f70 outdated

36+        pubkey0 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())['pubkey']
37+        pubkey1 = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())['pubkey']
38+        pubkey2 = self.nodes[2].getaddressinfo(self.nodes[2].getnewaddress())['pubkey']
39+        p2sh = self.nodes[1].addmultisigaddress(2, [pubkey0, pubkey1, pubkey2], "", "legacy")['address']
40+        p2wsh = self.nodes[1].addmultisigaddress(2, [pubkey0, pubkey1, pubkey2], "", "bech32")['address']
41+        p2wpkh = self.nodes[1].getnewaddress("", "legacy")

sipa commented at 0:42 am on July 7, 2018:

That doesn’t look very witnessy.

achow101 commented at 0:57 am on July 7, 2018:

Fixed. Also added tests for p2pkh and p2sh nested things.

achow101 force-pushed on Jul 7, 2018

sipa commented at 1:15 am on July 7, 2018: member

utACK 14339b0214f2976cae772ae836322d285138c353. Going to play around with it more before I give a tACK.

promag commented at 10:23 pm on July 7, 2018: member

utACK 14339b0. I have a couple of comments but are all nits here and there.

There already is a converttopsbt method. I do not think that we should make using PSBT require using raw transactions first. It should be self contained as I hope that we will move from raw transactions to PSBT entirely.

Ok got it.

in src/rpc/rawtransaction.cpp:1247 in 29df1b87e4 outdated

1259@@ -1259,6 +1260,546 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
1260     return result;
1261 }
1262 
1263+static std::string WriteHDKeypath(std::vector<uint32_t>& keypath)

instagibbs commented at 12:45 pm on July 9, 2018:

Doesn’t have to be addressed now, but I feel like this should be useful elsewhere too?

achow101 commented at 8:13 pm on July 9, 2018:

Maybe? It doesn’t seem useful outside of this context specifically since keypaths are stored as strings in the wallet currently.

in src/rpc/rawtransaction.cpp:1377 in 29df1b87e4 outdated

1372+            "         ...\n"
1373+            "      },\n"
1374+            "    }\n"
1375+            "    ,...\n"
1376+            "  ]\n"
1377+            "  \"fee\" : fee                      (numeric, optional) The transaction fee paid if all UTXOs are present\n"

instagibbs commented at 12:50 pm on July 9, 2018:

clarity nit: s/all UTXOs are present/all outputs are currently unspent/

sipa commented at 2:31 pm on July 9, 2018:

That’s not what it means. The fee can be reported whenever the UTXO information for each transaction input is present in the psbt (this information is generally added by Updater roles).

It’s clearly confusing though, do you have a better way of formulating this?

instagibbs commented at 2:37 pm on July 9, 2018:

something like: “The transaction fee paid if all UTXO slots have been filled in the PSBT”?

achow101 commented at 8:13 pm on July 9, 2018:

Done

in src/rpc/rawtransaction.cpp:1612 in 29df1b87e4 outdated

1618+            "Finalize the inputs of a PSBT. If the transaction is fully signed, it will produce a\n"
1619+            "network serialized transaction which can be broadcast with sendrawtransaction\n"
1620+            "Implements the Finalizer and Extractor roles.\n"
1621+            "\nArguments:\n"
1622+            "1. \"psbt\"                 (string) A base64 string of a PSBT\n"
1623+            "2. \"extract\"              (boolean, optional, default=true) If true and the transaction is complete, \n"

instagibbs commented at 1:04 pm on July 9, 2018:

I’m not sure that this argument is needed. Who is going to use this?

achow101 commented at 7:36 pm on July 9, 2018:

It is there so that each role can be done separately instead of at the same time. It’s useful for testing.

in src/rpc/rawtransaction.cpp:1648 in 29df1b87e4 outdated

1654+    }
1655+
1656+    UniValue result(UniValue::VOBJ);
1657+    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
1658+    bool extract = request.params[1].isNull() || (!request.params[1].isNull() && request.params[1].get_bool());
1659+    if (complete && extract) {

instagibbs commented at 1:15 pm on July 9, 2018:

help didn’t make it clear it will only return one or the other. Might explain why someone would want to turn “extract” off

achow101 commented at 7:28 pm on July 9, 2018:

I think the help text is fairly clear that it only has an effect if the transaction is complete. Suggestions for better help text?

instagibbs commented at 7:36 pm on July 9, 2018:

something like “extract and return the complete transaction in normal network serialization instead of the PSBT.” in addition to my other suggestion in the return values section.

achow101 commented at 8:13 pm on July 9, 2018:

Done

in src/rpc/rawtransaction.cpp:1694 in 29df1b87e4 outdated

1700+                            "    }\n"
1701+                            "    ,...                     More key-value pairs of the above form. For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also\n"
1702+                            "                             accepted as second parameter.\n"
1703+                            "   ]\n"
1704+                            "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
1705+                            "4. replaceable               (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"

instagibbs commented at 1:17 pm on July 9, 2018:

mu-nit: BIP125-replaceable

Perhaps the default should be true if the sequence values are compatible? It’d be messy to break API later.

achow101 commented at 7:30 pm on July 9, 2018:

A lot of this is shared with createrawtransaction so changing this will also also cause createrawtransaction’s API to change. Having it be separate though means that there will be a lot of code duplication.

in src/rpc/rawtransaction.cpp:1750 in 29df1b87e4 outdated

1745+                            "converttopsbt \"hexstring\" ( permitsigdata iswitness )\n"
1746+                            "\nConverts a network serialized transaction to a PSBT. This should be used only with createrawtransaction and fundrawtransaction\n"
1747+                            "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
1748+                            "\nArguments:\n"
1749+                            "1. \"hexstring\"              (string, required) The hex string of a raw transaction\n"
1750+                            "2. permitsigdata           (boolean, optional) If true, any signatures in the input will be discarded.\n"

instagibbs commented at 1:20 pm on July 9, 2018:

s/will be discarded/will be discarded but processing will continue/

what’s the default value?

achow101 commented at 8:13 pm on July 9, 2018:

Added default value

in src/rpc/rawtransaction.cpp:1741 in 29df1b87e4 outdated

1747+                            "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
1748+                            "\nArguments:\n"
1749+                            "1. \"hexstring\"              (string, required) The hex string of a raw transaction\n"
1750+                            "2. permitsigdata           (boolean, optional) If true, any signatures in the input will be discarded.\n"
1751+                            "                              If false, RPC will fail if any signatures are present.\n"
1752+                            "3. iswitness               (boolean, optional) Whether the transaction hex is a serialized witness transaction.\n"

instagibbs commented at 1:22 pm on July 9, 2018:

default value?

achow101 commented at 7:33 pm on July 9, 2018:

There really isn’t a default value for iswitness. Without it being specified, both witness and non-witness are tried. If it is specified, then if it is true, only witness is tried, and false only non-witness is tried.

instagibbs commented at 7:39 pm on July 9, 2018:

ok, then please say that in the help

achow101 commented at 8:13 pm on July 9, 2018:

Done

in src/wallet/rpcwallet.h:33 in db3ce1c3b3 outdated

27@@ -28,4 +28,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
28 
29 UniValue getaddressinfo(const JSONRPCRequest& request);
30 UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);
31+bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type = 1, bool sign = true);

instagibbs commented at 1:26 pm on July 9, 2018:

why not pass by reference if it’s going to be there?

achow101 commented at 7:34 pm on July 9, 2018:

Which?

instagibbs commented at 7:39 pm on July 9, 2018:

was referring to pwallet, which you dispute as an improvement

in src/wallet/rpcwallet.cpp:4549 in db3ce1c3b3 outdated

4544+            "that we can sign for.\n"
4545+            + HelpRequiringPassphrase(pwallet) + "\n"
4546+
4547+            "\nArguments:\n"
4548+            "1. \"psbt\"                      (string, required) The transaction base64 string\n"
4549+            "2. sign,                         (boolean, optional, default=true) Also sign the transaction when updating\n"

instagibbs commented at 1:31 pm on July 9, 2018:

comma after sign?

achow101 commented at 8:13 pm on July 9, 2018:

Fixed

in src/wallet/rpcwallet.cpp:4432 in db3ce1c3b3 outdated

4427+        first = false;
4428+    }
4429+    return true;
4430+}
4431+
4432+void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)

instagibbs commented at 1:32 pm on July 9, 2018:

change this to passing wallet by reference as well?

achow101 commented at 7:35 pm on July 9, 2018:

Why? It comes as a pointer, why not just pass the pointer around?

instagibbs commented at 7:40 pm on July 9, 2018:

Just not crazy about passing around pointers more than we have to.

instagibbs commented at 1:35 pm on July 9, 2018: member

Left some comments, looking good overall.

I’d also like the RPC help to be a bit more descriptive of what the calls are doing, in plain english, to avoid users having to be BIP174 experts. e.g., what does “finalizer” mean.

laanwj commented at 2:21 pm on July 9, 2018: member

might want to fix this warning:

 0/.../bitcoin/src/wallet/rpcwallet.h:14:1: warning: class 'PartiallySignedTransaction' was previously declared as a struct [-Wmismatched-tags]
 1class PartiallySignedTransaction;
 2^
 3/.../bitcoin/src/script/sign.h:486:8: note: previous use is here
 4struct PartiallySignedTransaction
 5       ^
 6/.../bitcoin/src/wallet/rpcwallet.h:14:1: note: did you mean struct here?
 7class PartiallySignedTransaction;
 8^~~~~
 9struct
101 warning generated.

in src/rpc/rawtransaction.cpp:1612 in 14339b0214 outdated

1607+            "2. \"extract\"              (boolean, optional, default=true) If true and the transaction is complete, \n"
1608+            "                             extract and return the complete transaction in normal network serialization.\n"
1609+
1610+            "\nResult:\n"
1611+            "{\n"
1612+            "  \"psbt\" : \"value\",          (string) The base64-encoded partially signed transaction\n"

instagibbs commented at 7:35 pm on July 9, 2018:

The base64-encoded partially signed transaction if network encoded transaction not extracted

achow101 commented at 8:14 pm on July 9, 2018:

Done (kind of)

sipa commented at 7:55 pm on July 9, 2018: member

We should probably have a document (in doc/ or so) that explain the most common workflows (hardware wallet, coinjoin, multisig) with the relevant RPCs.

achow101 force-pushed on Jul 9, 2018

achow101 commented at 8:14 pm on July 9, 2018: member

might want to fix this warning

I don’t see this warning locally, but fixed anyways.

achow101 force-pushed on Jul 9, 2018

laanwj commented at 2:18 pm on July 10, 2018: member

I don’t see this warning locally, but fixed anyways.

Heh, building with clang-latest-master is sometimes useful.

We should probably have a document (in doc/ or so) that explain the most common workflows (hardware wallet, coinjoin, multisig) with the relevant RPCs.

Good idea, though probably not in this PR if we want it in before the feature freeze.

in test/functional/rpc_psbt.py:2 in 52a5f17a68 outdated

0@@ -0,0 +1,171 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2016 The Bitcoin Core developers

Sjors commented at 4:22 pm on July 10, 2018:

Nit: 2018

achow101 commented at 11:03 pm on July 10, 2018:

Fixed

in test/functional/rpc_psbt.py:21 in 52a5f17a68 outdated

16+        self.num_nodes = 3
17+
18+    def run_test(self):
19+
20+        # Get some money and activate segwit
21+        self.nodes[0].generate(500)

Sjors commented at 4:34 pm on July 10, 2018:

SegWit is activated immediately in Regtest by default these days (nStartTime ... ALWAYS_ACTIVE ). If you change setup_clean_chain = True to False, you can leave out this generate() line completely.

achow101 commented at 11:03 pm on July 10, 2018:

Done

in test/functional/rpc_psbt.py:135 in 52a5f17a68 outdated

106+
107+        # Explicilty allow converting non-empty txs
108+        new_psbt = self.nodes[0].converttopsbt(rawtx['hex'])
109+        self.nodes[0].decodepsbt(new_psbt)
110+
111+        # BIP 174 Test Vectors

Sjors commented at 4:51 pm on July 10, 2018:

Move to test/functional/data/psbt.json?

achow101 commented at 11:03 pm on July 10, 2018:

Done

Sjors commented at 7:28 pm on July 10, 2018: member

Lightly tested, great stuff! Though multisig remains a bit tedious and confusing.

explain the most common workflows (hardware wallet, coinjoin, multisig) with the relevant RPCs

Adding test for those would be useful, and could catch if we forgot any useful params in the RPC.

I’m missing a test case for bip32_derivs.

Bit of a privacy issue: let’s say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn’t sign it. Wallet A then calls walletprocesspsbt which signs it and spontaneously adds the master_fingerprint and bip32 path. Same issue with walletcreatefundedpsbt.

Adding bip32_derivs should probably be opt-in.

At the risk over expanding the scope too much, it would be quite useful if inputs of createpsbt can add add a redeemScript argument, as a hint for multisig addresses the wallet might not be aware of. Although see below…

Somewhat orthogonal bug(?): walletcreatefundedpsbt is unable to construct a transaction if it can’t find the relevant UTXO. E.g. if you call addmultisig for a 2 of 2 where you only control one key, and then fund it from an external wallet. Although getaddressinfo will be aware of the mutlisig, getreceivedbyaddress will claim it’s not part of the wallet. You have to import the address and rescan for this to work. It seems to me that the wallet really should care about these transactions even if it doesn’t list them.

walletprocesspsbt won’t sign a transaction if it doesn’t know about the multisig address (i.e. addmultisig was never called). This is the case even if the PSBT has sufficient information, since it could match the witness_script against its own pub keys. Might be a nice improvement for later.

walletprocesspsbt also won’t sign a transaction if addmultisig was called but it can’t find the UTXO (see above for why). However in this case it could in principle blind sign it, or at least throw a more clear error message than "complete": false.

These rescans are rather painful, so - if it’s not too late - adding a blockheight hint to BIP-174 might be really helpful. I can suggest that on the mailing list if others think it’s useful.

It would be nice if all RPC calls which can produce a complete transaction have a flag that lets you produce the hex immediately, rather than going through the finalizepsbt dance.

achow101 commented at 11:03 pm on July 10, 2018: member

Adding test for those would be useful, and could catch if we forgot any useful params in the RPC.

There already are test cases for multisig. I have added one for coinjoin where inputs come from multiple nodes.

I’m missing a test case for bip32_derivs.

This is done in the updater tests which is a unit test (so we can add specific prevtxs).

Adding bip32_derivs should probably be opt-in.

Done

At the risk over expanding the scope too much, it would be quite useful if inputs of createpsbt can add add a redeemScript argument, as a hint for multisig addresses the wallet might not be aware of. Although see below…

While that could be useful, createpsbt does not do any updating, and I do not think it would be good to include any other data in an input before a UTXO is included for the input.

Somewhat orthogonal bug(?): walletcreatefundedpsbt is unable to construct a transaction if it can’t find the relevant UTXO. E.g. if you call addmultisig for a 2 of 2 where you only control one key, and then fund it from an external wallet. Although getaddressinfo will be aware of the mutlisig, getreceivedbyaddress will claim it’s not part of the wallet. You have to import the address and rescan for this to work. It seems to me that the wallet really should care about these transactions even if it doesn’t list them.

This is more of a problem with the wallet structure in general. I do not think that PSBT should really be doing anything about this.

These rescans are rather painful, so - if it’s not too late - adding a blockheight hint to BIP-174 might be really helpful. I can suggest that on the mailing list if others think it’s useful.

In general, I don’t think this is really useful. If you already know where to start rescanning for UTXOs, why can’t you just find the UTXO and add it yourself? There isn’t really any need to tell someone else to do it.

It would be nice if all RPC calls which can produce a complete transaction have a flag that lets you produce the hex immediately, rather than going through the finalizepsbt dance.

This was considered, but I think it is easier to follow and understand if the roles are more separated rather than integrated into one command. Instead of having many commands that can produce variable output, we will only have one command that does. The rest will have fixed, known outputs.

achow101 force-pushed on Jul 10, 2018

sipa commented at 10:24 pm on July 11, 2018: member

Bit of a privacy issue: let’s say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn’t sign it. Wallet A then calls walletprocesspsbt which signs it and spontaneously adds the master_fingerprint and bip32 path. Same issue with walletcreatefundedpsbt.

I think it’s generally hard to maintain much privacy in PSBT. You should assume that those you share a PSBT with will learn more about you than the general public will, even if just by observing which parties add data at what time.

Somewhat orthogonal bug(?): walletcreatefundedpsbt is unable to construct a transaction if it can’t find the relevant UTXO.

That’s inevitable. If you don’t know what is being spent, you don’t know what data to add the the PSBT.

walletprocesspsbt won’t sign a transaction if it doesn’t know about the multisig address (i.e. addmultisig was never called). This is the case even if the PSBT has sufficient information, since it could match the witness_script against its own pub keys. Might be a nice improvement for later.

That sounds like a bug to me. If the PSBT actually has all the information available (UTXO, redeemscript, witnessscript), walletprocesspsbt should sign with all the keys it has.

However in this case it could in principle blind sign it, or at least throw a more clear error message than “complete”: false.

These rescans are rather painful, so - if it’s not too late - adding a blockheight hint to BIP-174 might be really helpful. I can suggest that on the mailing list if others think it’s useful.

I think that generally no normal operation should involve rescans. PSBT are for formalizing interactions between pieces of wallet software, and wallet software should already have ways to know about its own UTXOs (which may in exceptional situations mean rescan, but generally shouldn’t).

There can also be more low-level separate RPCs that permit simulating some of the functionality without having strict wallets participate. None of that needs to be part of the initial functionality in Bitcoin Core, and some of it can be separate tools even. For example:

utxoupdatepsbt (node RPC) which adds UTXO data using chainstate/mempool to a PSBT rather than from a wallet.
descriptorupdatepsbt (utility RPC) Which is provided an output descriptor (see https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82) and adds derivation paths and scripts.
analyzepsbt (utility RPC, but aware of signing logic) which could report more data on PSBT than just decoding it (it could include fee, estimated vsize after signing, estimated feerate, what stage each input is towards finalizing and/or what’s missing to progress further).
joinpsbts (utility RPC) which would combine the inputs and outputs from multiple PSBTs for possible different transactions (effectively constructing a coinjoin), maintaining all utxo/script/derivation data for existing inputs.

achow101 commented at 10:36 pm on July 11, 2018: member

walletprocesspsbt won’t sign a transaction if it doesn’t know about the multisig address (i.e. addmultisig was never called). This is the case even if the PSBT has sufficient information, since it could match the witness_script against its own pub keys. Might be a nice improvement for later.

This shouldn’t be the case as it is explicitly tested for in the test cases. A multisig address is used with keys from 3 nodes, but the multisig address is only added to one of them. One of the other nodes (without having called addmultisigaddress) is still able to sign the transaction and produce a complete, sendable transaction.

There can also be more low-level separate RPCs that permit simulating some of the functionality without having strict wallets participate

Certainly, but I think such functionality should be done in a separate PR to avoid adding more things to review in this PR which is already somewhat large.

Sjors commented at 9:16 am on July 12, 2018: member

In this test case node1 created the multisig and is the first to sign it. In the scenario I tested, the multisig was created by an independent wallet, so the participating wallets only have the matching public and private keys, but don’t know there’s a multisig involved.

sipa commented at 5:38 pm on July 12, 2018: member

In this test case node1 created the multisig and is the first to sign it. In the scenario I tested, the multisig was created by an independent wallet, so the participating wallets only have the matching public and private keys, but don’t know there’s a multisig involved.

So run walletprocesspsbt on the PSBT with the wallet that does have the information about the multisig construction? It should add it to the file, and permit the signers to use that information.

If I’m misunderstanding the scenario, can you give a script to reproduce the issue? If you’re right, this may be an issue.

Sjors commented at 3:30 pm on July 13, 2018: member

@sipa I described the behavior in more detail in a functional test: https://github.com/achow101/bitcoin/compare/psbt...Sjors:2018/07/psbt-tests

achow101 commented at 6:25 pm on July 13, 2018: member

@sjors this is expected behavior due to how the wallet works. It is orthogonal to this PR and is just a side effect of the current wallet structure.

sipa commented at 6:38 pm on July 13, 2018: member

@Sjors So what is happening is that none of the wallets involved in your tests treat the address as “mine”, and:

As a result none know about the UTXO being spent,
As a result none can add UTXO information to the PSBT
As a result none can add script information (even those that know about the multisig, because they don’t know that it is relevant for that input).
As a result none sign (because they don’t know their key is relevant)

The solution is calling importaddress with P2WSH address on one of the nodes, then giving the PSBT to that node, then giving it to the node with the multisig information, and then giving it to all signers.

It is a highly confusing situation that adding a multisig address doesn’t automatically watch it, but it is consistent with how everything works: things are only treated as mine if (A) you know the scripts involved and have ALL the private keys or (B) the address was imported as watch-only.

Longer term this is something I want to address with my descriptors work.

Sjors commented at 7:03 pm on July 13, 2018: member

I see, at least we have it documented again :-)

Implement PSBT Structures and un/serialization methods per BIP 174 41c607f09b

Add pubkeys and whether input was witness to SignatureData

Stores pubkeys in SignatureData and retrieves them when using GetPubKey().

Stores whether the signatures in a SignatureData are for a witness input.

12bcc64f27

Methods for interacting with PSBT structs

Added methods which move data to/from SignaturData objects to
PSBTInput and PSBTOutput objects.

Added sanity checks for PSBTs as a whole which are done immediately
after deserialization.

Added Merge methods to merge a PSBT into another one.

e9d86a43ad

Refactor transaction creation and transaction funding logic

In preparation for more create transaction and fund transcation RPCs,
refactor the transaction creation and funding logic into separate
functions.

58a8e28918

SignPSBTInput wrapper function

The SignPSBTInput function takes a PSBTInput, SignatureData, SigningProvider,
and other data necessary for signing. It fills the SignatureData with data from
the PSBTInput, retrieves the UTXO from the PSBTInput, signs and finalizes the
input if possible, and then extracts the results from the SignatureData and
puts them back into the PSBTInput.

8b5ef27937

in src/script/sign.h:550 in 96237e8c75 outdated

545+        s >> magic;
546+        if (!std::equal(magic, magic + 4, PSBT_MAGIC_BYTES)) {
547+            throw std::ios_base::failure("Invalid PSBT magic bytes");
548+        }
549+        unsigned char magic_sep;
550+        s >> magic_sep;

nodech commented at 9:13 pm on July 13, 2018:

Does not magic_sep need to be 0xff ?

achow101 commented at 9:27 pm on July 13, 2018:

Good catch. I have fixed this by adding 0xff to the magic bytes so it will be checked above.

achow101 force-pushed on Jul 13, 2018

DrahtBot commented at 0:50 am on July 14, 2018: member

#13359 (wallet: Directly operate with CMutableTransaction by MarcoFalke)
#13266 (refactoring: Convert DataFromTransaction to a SignatureData constructor, and privatize helpers by Empact)
#13144 (RPC: Improve error messages on RPC endpoints that use GetTransaction by jimpo)
#13098 (Skip tx-rehashing on historic blocks by MarcoFalke)

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.

in src/rpc/rawtransaction.cpp:1739 in 3e3fb6e737 outdated

1750+                            "converttopsbt \"hexstring\" ( permitsigdata iswitness )\n"
1751+                            "\nConverts a network serialized transaction to a PSBT. This should be used only with createrawtransaction and fundrawtransaction\n"
1752+                            "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
1753+                            "\nArguments:\n"
1754+                            "1. \"hexstring\"              (string, required) The hex string of a raw transaction\n"
1755+                            "2. permitsigdata           (boolean, optional, default=false) If true, any signatures in the input will be discarde and conversion.\n"

sipa commented at 10:16 pm on July 16, 2018:

Typo: discarde

achow101 commented at 11:07 pm on July 16, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1776 in 3e3fb6e737 outdated

1771+
1772+    // parse hex string from parameter
1773+    CMutableTransaction tx;
1774+    bool permitsigdata = request.params[1].isNull() ? false : request.params[1].get_bool();
1775+    bool iswitness = request.params[2].isNull() ? true : request.params[2].get_bool();
1776+    bool try_witness = permitsigdata ? iswitness : true;

sipa commented at 10:18 pm on July 16, 2018:

This seems wrong. With the current code, if permitsigdata is false it will try both witness and no_witness (where it should only permit non-witness, I think?). Further, when iswitness is not specified, it will only try witness.

I think it should be something like:

0bool witness_specified = !request.params[2].isNull();
1bool iswitness = witness_specified ? request.params[2].get_bool() : false;
2bool try_witness = permitsigdata ? (witness_specified ? iswitness : true) : false;
3bool try_no_witness = permitsigdata ? (witness_specified ? !iswitness : true) : true;

achow101 commented at 11:08 pm on July 16, 2018:

Done

in src/rpc/rawtransaction.cpp:1686 in 3e3fb6e737 outdated

1681+UniValue createpsbt(const JSONRPCRequest& request)
1682+{
1683+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
1684+        throw std::runtime_error(
1685+                            "createpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable )\n"
1686+                            "\nCreates a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"

sipa commented at 10:23 pm on July 16, 2018:

No inputs will be added.

achow101 commented at 11:08 pm on July 16, 2018:

Fixed

in src/rpc/rawtransaction.cpp:1742 in 4afc08e472 outdated

1738@@ -1755,8 +1739,9 @@ UniValue converttopsbt(const JSONRPCRequest& request)
1739                             "2. permitsigdata           (boolean, optional, default=false) If true, any signatures in the input will be discarde and conversion.\n"
1740                             "                              will continue. If false, RPC will fail if any signatures are present.\n"
1741                             "3. iswitness               (boolean, optional) Whether the transaction hex is a serialized witness transaction.\n"
1742-                            "                              If iswitness is not present, heuristic tests will be used in decoding. Only has an effect if\n"
1743-                            "                              permitsigdata is true\n"
1744+                            "                              If iswitness is not present, heuristic tests will be used in decoding. If true, only witness deserializaion\n"

sipa commented at 10:25 pm on July 16, 2018:

Nit: RPC documentation is being changed in unrelated wallet RPC commit.

achow101 commented at 11:08 pm on July 16, 2018:

Fixed

achow101 force-pushed on Jul 16, 2018

Create utility RPCs for PSBT

decodepsbt takes a PSBT and decodes it to JSON

combinepsbt takes multiple PSBTs for the same tx and combines them.

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.

c27fe419ef

Create wallet RPCs for PSBT

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.

a4b06fb42e

achow101 force-pushed on Jul 16, 2018

Tests for PSBT

Added functional tests for PSBT that test the RPCs. Also added all
of the BIP 174 test vectors (except for the updater tests) in the
functional tests.

Added a Unit test for the BIP 174 updater test vector.

020628e3a4

achow101 force-pushed on Jul 17, 2018

sipa commented at 1:20 am on July 17, 2018: member

utACK 020628e3a4e88e36647eaf92bac4b3552796ac6a

laanwj commented at 6:25 pm on July 18, 2018: member

utACK 020628e3a4e88e36647eaf92bac4b3552796ac6a

laanwj merged this on Jul 18, 2018

laanwj closed this on Jul 18, 2018

laanwj referenced this in commit b654723461 on Jul 18, 2018

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

MarcoFalke referenced this in commit aba2e666d7 on Jul 19, 2018

meshcollider referenced this in commit 31c0006a6c on Feb 25, 2020

sidhujag referenced this in commit 421889f107 on Feb 25, 2020

sidhujag referenced this in commit 5813be17ef on Nov 10, 2020

UdjinM6 referenced this in commit 47f20465c3 on May 19, 2021

UdjinM6 referenced this in commit 1d240e7108 on May 19, 2021

UdjinM6 referenced this in commit 6f7b52ac63 on May 19, 2021

kittywhiskers referenced this in commit 5a4e75cdc3 on May 20, 2021

kittywhiskers referenced this in commit dfd2a197d7 on May 20, 2021

kittywhiskers referenced this in commit bc2f11f230 on May 20, 2021

barrystyle referenced this in commit 8e74396e6e on May 27, 2021

barrystyle referenced this in commit a8669d0a93 on May 27, 2021

barrystyle referenced this in commit 4a6b6f21dd on May 31, 2021

kittywhiskers referenced this in commit bcfec0b047 on Jun 4, 2021

kittywhiskers referenced this in commit dbb1546f6e on Jun 4, 2021

UdjinM6 referenced this in commit 6964099ba2 on Jun 7, 2021

barrystyle referenced this in commit acb122e6cf on Jun 8, 2021

barrystyle referenced this in commit 9166796714 on Jun 8, 2021

kittywhiskers referenced this in commit 2b7d4a3487 on Jun 9, 2021

kittywhiskers referenced this in commit 2799f70622 on Jul 9, 2021

kittywhiskers referenced this in commit bf9ae7cf99 on Jul 13, 2021

kittywhiskers referenced this in commit 04f913f374 on Jul 13, 2021

kittywhiskers referenced this in commit c00b3e942f on Jul 13, 2021

PastaPastaPasta referenced this in commit e98241da5d on Jul 13, 2021

gades referenced this in commit 58d7b3b1cd on Apr 26, 2022

gades referenced this in commit 6beff1d0b3 on Apr 30, 2022

malbit referenced this in commit e163b6e23f on Aug 7, 2022

DrahtBot locked this on Aug 16, 2022

BIP 174 PSBT Serializations and RPCs #13557