BIP 174 PSBT Serializations and RPCs #13557

Merged
merged 8 commits into from Jul 18, 2018

Conversation

Projects
None yet
10 participants
@achow101
Member

achow101 commented Jun 28, 2018

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

@DrahtBot

This comment has been minimized.

Show comment
Hide comment
@DrahtBot

DrahtBot Jun 28, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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.

DrahtBot commented Jun 28, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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

Review comments for:

  • fdcbcb3 - Inline Sign1 and SignN
  • a86d067 - Make SignatureData able to store signatures and scripts
  • c5d47a5 - Replace CombineSignatures with ProduceSignature
  • 95f5a38 - Remove CombineSignatures and replace tests

fdcbcb3 is kind of unrelated here no?

src/script/sign.cpp
+ unsigned int num_pubkeys = solutions.size()-2;
+ unsigned int last_success_key = 0;
+ for (const valtype& sig : stack.script) {
+ for (unsigned int i = last_success_key; i < num_pubkeys; i++) {

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

nit, ++i

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

nit, ++i

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

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

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

class SignatureExtractorChecker final?

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

class SignatureExtractorChecker final?

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.cpp
+{
+ if (checker->CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) {
+ CPubKey pubkey(vchPubKey);
+ sigdata->signatures.emplace(pubkey.GetID(), SigPair(pubkey, scriptSig));

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

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:

auto i = sigdata->signatures.emplace(...);
assert(i.second);
@promag

promag Jun 28, 2018

Member

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:

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+class SignatureExtractorChecker : public BaseSignatureChecker
+{
+private:
+ SignatureData* sigdata;

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

Use references instead of pointers?

@promag

promag Jun 28, 2018

Member

Commit "Make SignatureData able to store signatures and scripts"

Use references instead of pointers?

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.cpp
@@ -33,14 +33,60 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid
return true;
}
+static bool GetCScript(const SigningProvider* provider, const SignatureData* sigdata, const CScriptID &scriptid, CScript& script)

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

Commit "Replace CombineSignatures with ProduceSignature"

Looks like these could be references:

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

and remove provider != nullptr below.

@promag

promag Jun 28, 2018

Member

Commit "Replace CombineSignatures with ProduceSignature"

Looks like these could be references:

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

and remove provider != nullptr below.

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

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

@promag

promag Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.cpp
+ return true;
+ }
+ // Look for pubkey in all partial sigs
+ const auto& it = sigdata->signatures.find(address);

This comment has been minimized.

@promag

promag Jun 28, 2018

Member

Commit "Replace CombineSignatures with ProduceSignature"

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

@promag

promag Jun 28, 2018

Member

Commit "Replace CombineSignatures with ProduceSignature"

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jun 28, 2018

Member

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

Member

achow101 commented Jun 28, 2018

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

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jun 28, 2018

Member

Ops @achow101 do you want me to comment there?

Member

promag commented Jun 28, 2018

Ops @achow101 do you want me to comment there?

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jun 28, 2018

Member

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

Member

achow101 commented Jun 28, 2018

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

@sipa

Just comments on the serialization commit.

+};
+
+/** A structure for PSBTs which contains per output information */
+struct PSBTOutput

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

Inconsistent class name (PartiallySignedInput vs PSBTOutput).

@sipa

sipa Jun 28, 2018

Member

Inconsistent class name (PartiallySignedInput vs PSBTOutput).

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done. Renamed PartiallySignedInput to PSBTInput

@achow101

achow101 Jun 28, 2018

Member

Done. Renamed PartiallySignedInput to PSBTInput

src/script/sign.h
+static const uint8_t PSBT_SEPARATOR = 0x00;
+
+template<typename Stream, typename... X>
+void SerializeToVector(Stream& s, const X&... args)

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+ }
+
+ // Read in the signature from value
+ uint64_t value_len = ReadCompactSize(s);

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

+
+ // Do stuff based on type
+ switch(type) {
+ case PSBT_IN_NON_WITNESS_UTXO:

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+ throw std::ios_base::failure("Duplicate Key, key for unknown value already provided");
+ }
+ // Read in the value
+ uint64_t value_len = ReadCompactSize(s);

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

+ s >> witness_script;
+ break;
+ }
+ case PSBT_OUT_BIP32_DERIVATION:

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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.

@sipa

sipa Jun 28, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+ std::vector<PartiallySignedInput> inputs;
+ std::vector<PSBTOutput> outputs;
+ std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
+ uint64_t num_ins = 0;

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

Do you still need num_ins and use_in_index?

@sipa

sipa Jun 28, 2018

Member

Do you still need num_ins and use_in_index?

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Removed

@achow101

achow101 Jun 28, 2018

Member

Removed

src/script/sign.h
+
+ // Read input data
+ unsigned int i = 0;
+ while (!s.empty() && i < tx.vin.size()) {

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

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.

@achow101

achow101 Jun 28, 2018

Member

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.

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

src/script/sign.h
+
+
+// Note: These constants are in reverse byte order because serialization uses LSB
+static constexpr uint32_t PSBT_MAGIC_BYTES = 0x74627370;

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+
+// The separator is 0x00. Reading this in means that the unserializer can interpret it
+// as a 0 length key. which indicates that this is the separator. The separator has no value.
+static const uint8_t PSBT_SEPARATOR = 0x00;

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

constexpr

@sipa

sipa Jun 28, 2018

Member

constexpr

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Done

src/script/sign.h
+}
+
+// Deserialize HD keypaths into a map
+static void DeserializeHDKeypaths(const std::vector<unsigned char>& key, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths, Stream& s)

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

Add template <typename Stream> before.

@sipa

sipa Jun 28, 2018

Member

Add template <typename Stream> before.

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Fixed

src/script/sign.h
+}
+
+// Serialize HD keypaths to a stream from a map
+static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s)

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

Add template<typename Stream> before.

@sipa

sipa Jun 28, 2018

Member

Add template<typename Stream> before.

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Fixed

src/script/sign.h
+static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s)
+{
+ for (auto keypath_pair : hd_keypaths) {
+ SerializeToVector(s, PSBT_IN_BIP32_DERIVATION, MakeSpan(keypath_pair.first));

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Fixed

src/script/sign.h
+
+ // Read input data
+ unsigned int i = 0;
+ while (!s.empty() && i < tx.vin.size()) {

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

@sipa

Comments on rawtransaction.cpp RPCs.

+
+ RPCTypeCheck(request.params, {UniValue::VSTR});
+
+ // Unserialize the transactions

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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.

@sipa

sipa Jun 28, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ if (!input.final_script_witness.IsNull()) {
+ in.pushKV("final_scripWitness", input.final_script_witness.ToString());
+ }
+ }

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

Also add unknowns to the decode?

@sipa

sipa Jun 28, 2018

Member

Also add unknowns to the decode?

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ " \"bip32_derivs\" : [ (array of json objects, optional)\n"
+ " {\n"
+ " \"pubkey\" : \"pubkey\", (string) The public key this path corresponds to\n"
+ " \"master_fingerprint\" : \"fingerprint\" (string) The fingerprint of the master key\n"

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

I think it is clearer to list the fingerprint separately.

@achow101

achow101 Jun 28, 2018

Member

I think it is clearer to list the fingerprint separately.

src/rpc/rawtransaction.cpp
+ }
+ }
+
+ for (const PartiallySignedTransaction& psbtx : psbtxs) {

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 28, 2018

Member

Why?

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ for (unsigned int i = 0; i < merged_psbtx.tx.vin.size(); ++i) {
+ CTxIn& txin = merged_psbtx.tx.vin[i];
+
+ // Find the utxo from one of the psbtxs

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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?

@sipa

sipa Jun 28, 2018

Member

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?

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

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

@achow101

achow101 Jun 29, 2018

Member

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

src/rpc/rawtransaction.cpp
+ "\nResult:\n"
+ "{\n"
+ " \"base64\" : \"value\", (string) The base64-encoded network transaction\n"
+ " \"hex\" : \"value\", (string) The hex-encoded partially signed transaction if extracted\n"

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+ continue;
+ }
+
+ // Fill a SignatureData with input info

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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.

@sipa

sipa Jun 28, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
+ throw std::runtime_error(
+ "createpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable )\n"
+ "\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

This RPC does not do any funding, I think.

Explain that this implements a Creator role.

@sipa

sipa Jun 28, 2018

Member

This RPC does not do any funding, I think.

Explain that this implements a Creator role.

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/uint256.h
@@ -91,6 +91,15 @@ class base_blob
((uint64_t)ptr[7]) << 56;
}
+ uint32_t GetUint32LE(int pos) const

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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).

@sipa

sipa Jun 28, 2018

Member

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).

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
+ throw std::runtime_error(
+ "converttopsbt \"hexstring\" ( permitsigdata iswitness )\n"
+ "\nConverts a network serialized transaction to a PSBT.\n"

This comment has been minimized.

@sipa

sipa Jun 28, 2018

Member

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

@sipa

sipa Jun 28, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jun 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.

Member

achow101 commented Jun 29, 2018

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.

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

This comment has been minimized.

@sipa

sipa Jun 29, 2018

Member

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

@sipa

sipa Jun 29, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Done

src/script/sign.cpp
+ utxo = input.non_witness_utxo->vout[txin.prevout.n];
+ }
+ // Now find the witness utxo if the non witness doesn't exist
+ else if (!input.witness_utxo.IsNull()) {

This comment has been minimized.

@sipa

sipa Jun 29, 2018

Member

Nit: else on the same line as }.

@sipa

sipa Jun 29, 2018

Member

Nit: else on the same line as }.

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Fixed

src/wallet/rpcwallet.cpp
@@ -4388,6 +4391,333 @@ UniValue sethdseed(const JSONRPCRequest& request)
return NullUniValue;
}
+bool parse_hd_keypath(std::string keypath_str, std::vector<uint32_t>& keypath)

This comment has been minimized.

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Fixed

src/wallet/rpcwallet.cpp
+ return true;
+}
+
+void add_keypath_to_map(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)

This comment has been minimized.

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Fixed

src/wallet/rpcwallet.cpp
+ hd_keypaths.emplace(vchPubKey, keypath);
+}
+
+bool fill_psbt(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign)

This comment has been minimized.

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

@sipa

sipa Jun 29, 2018

Member

Nit: function naming style

This comment has been minimized.

@achow101

achow101 Jun 29, 2018

Member

Fixed

@nvk

This comment has been minimized.

Show comment
Hide comment
@nvk

nvk Jun 29, 2018

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

nvk commented Jun 29, 2018

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

@sipa

Some more nits on serialization code.

src/script/sign.h
+ inline void Unserialize(Stream& s) {
+ // Read loop
+ while(!s.empty()) {
+ // read size of key

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

std::vector<unsigned char> key;
s >> key;
if (key.empty()) return;
@sipa

sipa Jun 30, 2018

Member

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

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

src/script/sign.h
+ if (non_witness_utxo) {
+ throw std::ios_base::failure("Duplicate Key, input non-witness utxo already provided");
+ }
+ if (!witness_utxo.IsNull()) {

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Removed

@achow101

achow101 Jun 30, 2018

Member

Removed

src/script/sign.h
+ throw std::ios_base::failure("Duplicate Key, input witness utxo already provided");
+ }
+ if (non_witness_utxo) {
+ throw std::ios_base::failure("A non-witness utxo was already provided, cannot provide a witness utxo");

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Same thing here.

@sipa

sipa Jun 30, 2018

Member

Same thing here.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

src/script/sign.h
+ s >> sig;
+
+ // Add to list
+ partial_sigs.emplace(pubkey.GetID(), SigPair(pubkey, sig));

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Can be std::move(sig).

@sipa

sipa Jun 30, 2018

Member

Can be std::move(sig).

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

src/script/sign.h
+ inline void Unserialize(Stream& s) {
+ // Read loop
+ while(!s.empty()) {
+ // read size of key

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

The same shorter read routine is possible here.

@sipa

sipa Jun 30, 2018

Member

The same shorter read routine is possible here.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

src/script/sign.h
+ // Read global data
+ while(!s.empty()) {
+ // read size of key
+ uint64_t key_len = ReadCompactSize(s);

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

And again here.

@sipa

sipa Jun 30, 2018

Member

And again here.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

src/core_read.cpp
+ error = "extra data after PSBT";
+ return false;
+ }
+ } catch (const std::exception& e) {

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

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

@achow101

achow101 Jun 30, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

I should learn to read.

@sipa

sipa Jun 30, 2018

Member

I should learn to read.

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

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Nit: variable naming style.

@sipa

sipa Jun 30, 2018

Member

Nit: variable naming style.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Fixed

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

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

You can drop the .c_str().

@sipa

sipa Jun 30, 2018

Member

You can drop the .c_str().

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

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

@achow101

achow101 Jun 30, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

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

@achow101

achow101 Jun 30, 2018

Member

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

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

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+{
+ if (request.fHelp || request.params.size() != 1)
+ throw std::runtime_error(
+ "decodepsbt \"base64string\"\n"

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Renamed

@achow101

achow101 Jun 30, 2018

Member

Renamed

src/rpc/rawtransaction.cpp
+ "\nCombine multiple partially signed Bitcoin transactions into one transaction.\n"
+ "Implements the Combiner and Finalizer roles.\n"
+ "\nArguments:\n"
+ "1. \"txs\" (string) A json array of hex strings of partially signed transactions\n"

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Not hex.

@sipa

sipa Jun 30, 2018

Member

Not hex.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+
+ PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
+ // Merge them
+ for (const PartiallySignedTransaction& psbtx : psbtxs) {

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

No need to merge in the first one again.

@sipa

sipa Jun 30, 2018

Member

No need to merge in the first one again.

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Changed to erase the first element from the vector.

@achow101

achow101 Jun 30, 2018

Member

Changed to erase the first element from the vector.

src/rpc/rawtransaction.cpp
+ merged_psbt.Merge(psbtx);
+ }
+ if (!merged_psbt.IsSane()) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Merged PSBT is not sane");

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Done

+ UniValue result(UniValue::VOBJ);
+ CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
+ ssTx << merged_psbt;
+ return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Is the cast necessary here?

@sipa

sipa Jun 30, 2018

Member

Is the cast necessary here?

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

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

@achow101

achow101 Jun 30, 2018

Member

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

src/rpc/rawtransaction.cpp
+ "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
+ "\nArguments:\n"
+ "1. \"hexstring\" (string, required) The hex string of a raw transaction\n"
+ "2. permitsigdata (boolean, optional) Whether the transaction hex contains any data in the scriptSigs or scriptWitnesses.\n"

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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."

@sipa

sipa Jun 30, 2018

Member

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."

This comment has been minimized.

@achow101

achow101 Jun 30, 2018

Member

Changed

@achow101

achow101 Jun 30, 2018

Member

Changed

UniValue result(UniValue::VOBJ);
result.pushKV("hex", EncodeHexTx(tx));
- result.pushKV("changepos", changePosition);
- result.pushKV("fee", ValueFromAmount(nFeeOut));
+ result.pushKV("fee", ValueFromAmount(fee));

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Why swap these?

@sipa

sipa Jun 30, 2018

Member

Why swap these?

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

To match the order in the help text.

@achow101

achow101 Jul 2, 2018

Member

To match the order in the help text.

src/script/sign.h
+/** A version of CTransaction with the PSBT format*/
+struct PartiallySignedTransaction
+{
+ CMutableTransaction tx;

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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.

@sipa

sipa Jun 30, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Made into a boost::optional

@achow101

achow101 Jul 2, 2018

Member

Made into a boost::optional

src/wallet/rpcwallet.cpp
+ // Get all of the previous transactions
+ bool complete = true;
+ for (unsigned int i = 0; i < txConst->vin.size(); ++i) {
+ CTxIn txin = txConst->vin[i];

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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

@sipa

sipa Jun 30, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Done

src/wallet/rpcwallet.cpp
+ PSBTInput& input = psbtx.inputs.at(i);
+
+ // If we don't know about this input, skip it and let someone else deal with it
+ uint256 txhash = txin.prevout.hash;

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Same here.

@sipa

sipa Jun 30, 2018

Member

Same here.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ // Get all of the previous transactions
+ bool complete = true;
+ for (unsigned int i = 0; i < psbtx.tx.vin.size(); ++i) {
+ CTxIn txin = psbtx.tx.vin[i];

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

Unused variable.

@sipa

sipa Jun 30, 2018

Member

Unused variable.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Removed

@achow101

achow101 Jul 2, 2018

Member

Removed

src/wallet/rpcwallet.cpp
+ "\nArguments:\n"
+ "1. \"psbt\" (string, required) The transaction base64 string\n"
+ "2. \"sign\", (string, optional, default=true) Also sign the transaction when updating\n"
+ "3. \"sighashtype\" (string, optional, default=ALL) The signature hash type to sign with if not specified by the PSBT. Must be one of\n"

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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.

@sipa

sipa Jun 30, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Done

src/wallet/rpcwallet.cpp
+ }
+
+ SignatureData sigdata;
+ complete &= SignPSBTInput(*pwallet, psbtx.tx, input, sigdata, i, sighash, sign);

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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.

@sipa

sipa Jun 30, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Done.

@achow101

achow101 Jul 2, 2018

Member

Done.

src/wallet/rpcwallet.cpp
+
+ if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)
+ throw std::runtime_error(
+ "walletcreatefundedpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable ) ( options )\n"

This comment has been minimized.

@sipa

sipa Jun 30, 2018

Member

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.

@sipa

sipa Jun 30, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

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

@achow101

achow101 Jul 2, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jul 2, 2018

Member

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.

@sipa

sipa Jul 2, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 2, 2018

Member

Changed to also update

@achow101

achow101 Jul 2, 2018

Member

Changed to also update

src/rpc/rawtransaction.cpp
+ }
+ keypath_str += "/";
+ }
+ return keypath_str.substr(0, keypath_str.size() - 1); // Drop last '/'

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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.

@sipa

sipa Jul 4, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ " \"hex\" : \"hex\", (string) The hex\n"
+ " \"reqSigs\" : n, (numeric) The required sigs\n"
+ " \"type\" : \"pubkeyhash\", (string) The type, eg 'pubkeyhash'\n"
+ " \"addresses\" : [ (json array of string)\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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).

@sipa

sipa Jul 4, 2018

Member

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).

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ " \"outputs\" : [ (array of json objects)\n"
+ " {\n"
+ " \"redeem_script\" : { (json object, optional)\n"
+ " \"script\" : { (json object)\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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.

@sipa

sipa Jul 4, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Removed, that's a mistake

@achow101

achow101 Jul 4, 2018

Member

Removed, that's a mistake

src/rpc/rawtransaction.cpp
+
+ UniValue out(UniValue::VOBJ);
+
+ out.pushKV("value", ValueFromAmount(txout.nValue));

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

src/rpc/rawtransaction.cpp
+
+ // Partial sigs
+ if (!input.partial_sigs.empty()) {
+ UniValue partial_sigs(UniValue::VARR);

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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>"},...].

@sipa

sipa Jul 4, 2018

Member

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>"},...].

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done.

@achow101

achow101 Jul 4, 2018

Member

Done.

This comment has been minimized.

@sipa

sipa Jul 7, 2018

Member

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...).

@sipa

sipa Jul 7, 2018

Member

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...).

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Fixed

@achow101

achow101 Jul 9, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+
+ // Final scriptSig and scriptwitness
+ if (!input.final_script_sig.empty()) {
+ in.pushKV("final_scriptSig", HexStr(input.final_script_sig));

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

Here you may want to have both hex and asm.

@sipa

sipa Jul 4, 2018

Member

Here you may want to have both hex and asm.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ }
+
+ PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
+ psbtxs.erase(psbtxs.begin());

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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); }.

@sipa

sipa Jul 4, 2018

Member

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); }.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

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

@achow101

achow101 Jul 4, 2018

Member

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

src/rpc/rawtransaction.cpp
+
+ // parse hex string from parameter
+ CMutableTransaction tx;
+ bool try_witness = !request.params[1].isNull() ? (request.params[2].isNull() ? true : request.params[2].get_bool()) : true;

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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?

@sipa

sipa Jul 4, 2018

Member

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?

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Changed

@achow101

achow101 Jul 4, 2018

Member

Changed

src/wallet/rpcwallet.cpp
+ keypath.insert(keypath.begin(), ReadLE32(masterKey.key.GetPubKey().GetID().begin()));
+ }
+ // Single pubkeys get the master fingerprint of themselves
+ else {

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

Style nit: else on the same line

@sipa

sipa Jul 4, 2018

Member

Style nit: else on the same line

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Fixed

@achow101

achow101 Jul 4, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+
+ // Partial sigs
+ if (!input.partial_sigs.empty()) {
+ UniValue partial_sigs(UniValue::VARR);

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

src/rpc/rawtransaction.cpp
+ " }\n"
+ " ,...\n"
+ " ]\n"
+ " \"sighash\" : \"type\", (string, optional) The sighash type recommended to be used\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

Not just recommended.

@sipa

sipa Jul 4, 2018

Member

Not just recommended.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Fixed

@achow101

achow101 Jul 4, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+ " \"hex\" : \"hex\", (string) The hex\n"
+ " \"type\" : \"pubkeyhash\", (string) The type, eg 'pubkeyhash'\n"
+ " }\n"
+ " \"bip32_derivs\" : [ (array of json objects, optional)\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ " }\n"
+ " ,...\n"
+ " ]\n"
+ "}\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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.

@sipa

sipa Jul 4, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Done. The consistency check occurs at deserialization time.

@achow101

achow101 Jul 4, 2018

Member

Done. The consistency check occurs at deserialization time.

src/rpc/rawtransaction.cpp
+ if (request.fHelp || request.params.size() != 1)
+ throw std::runtime_error(
+ "combinepsbt [\"base64string\",...]\n"
+ "\nCombine multiple partially signed Bitcoin transactions into one transaction.\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

src/rpc/rawtransaction.cpp
+
+ "\nResult:\n"
+ "{\n"
+ " \"psbt\" : \"value\", (string) The base64-encoded partially signed transaction\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Fixed here and elsewhere

@achow101

achow101 Jul 4, 2018

Member

Fixed here and elsewhere

src/rpc/rawtransaction.cpp
+ "4. replaceable (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"
+ " 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"
+ "\nResult:\n"
+ " \"psbt\": \"value\", (string) The resulting raw transaction (base64-encoded string)\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

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

@sipa

sipa Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 4, 2018

Member

Fixed here and elsewhere

@achow101

achow101 Jul 4, 2018

Member

Fixed here and elsewhere

src/rpc/rawtransaction.cpp
+ " If iswitness is not present, heuristic tests will be used in decoding. Only has an effect if\n"
+ " permitsigdata is true\n"
+ "\nResult:\n"
+ " \"psbt\": \"value\", (string) The resulting raw transaction (base64-encoded string)\n"

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

Same here.

@sipa

sipa Jul 4, 2018

Member

Same here.

src/script/sign.h
+class PublicOnlySigningProvider : public SigningProvider
+{
+private:
+ const SigningProvider* provider;

This comment has been minimized.

@sipa

sipa Jul 4, 2018

Member

Style nit: m_provider.

@sipa

sipa Jul 4, 2018

Member

Style nit: m_provider.

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

@promag

Comments regarding "Create wallet RPCs for PSBT" (9e4077a)

No cs_wallet lock in walletcreatefundedpsbt?

Instead of walletcreatefundedpsbt, could make a rawtransactiontopsbt or alike?

+ return NullUniValue;
+ }
+
+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

> 3

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Fixed

@achow101

achow101 Jul 5, 2018

Member

Fixed

src/wallet/rpcwallet.cpp
+ {std::string("SINGLE"), int(SIGHASH_SINGLE)},
+ {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
+ };
+ std::string strHashType = request.params[1].get_str();

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

[2]?

@promag

promag Jul 4, 2018

Member

[2]?

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

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

@promag

promag Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Moved to a new helper function.

@achow101

achow101 Jul 5, 2018

Member

Moved to a new helper function.

src/wallet/rpcwallet.cpp
+
+ // Fill transaction with our data and also sign
+ bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
+ bool fComplete = FillPSBT(pwallet, psbtx, &txConst, nHashType, sign);

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

nit, just complete.

@promag

promag Jul 4, 2018

Member

nit, just complete.

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Done

src/wallet/rpcwallet.cpp
+ + HelpExampleCli("walletprocesspsbt", "\"psbt\"")
+ );
+
+ LOCK2(cs_main, pwallet->cs_wallet);

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Why lock cs_main? Locking could be in FillPSBT?

@promag

promag Jul 4, 2018

Member

Why lock cs_main? Locking could be in FillPSBT?

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Removed cs_main, moved to FillPSBT.

@achow101

achow101 Jul 5, 2018

Member

Removed cs_main, moved to FillPSBT.

src/wallet/rpcwallet.cpp
+
+ "\nArguments:\n"
+ "1. \"psbt\" (string, required) The transaction base64 string\n"
+ "2. \"sign\", (string, optional, default=true) Also sign the transaction when updating\n"

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

boolean? Drop \" in sign.

@promag

promag Jul 4, 2018

Member

boolean? Drop \" in sign.

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Fixed

@achow101

achow101 Jul 5, 2018

Member

Fixed

src/wallet/rpcwallet.cpp
+
+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
+ throw std::runtime_error(
+ "walletprocesspsbt \"psbt\" ( sign sighashtype )\n"

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

"sighashtype"?

@promag

promag Jul 4, 2018

Member

"sighashtype"?

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Fixed

@achow101

achow101 Jul 5, 2018

Member

Fixed

src/rpc/client.cpp
+ { "walletcreatefundedpsbt", 1, "outputs" },
+ { "walletcreatefundedpsbt", 2, "locktime" },
+ { "walletcreatefundedpsbt", 3, "replaceable" },
+ { "walletcreatefundedpsbt", 4, "options" },
{ "createpsbt", 0, "inputs" },

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

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

@promag

promag Jul 4, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Added

@achow101

achow101 Jul 5, 2018

Member

Added

src/wallet/rpcwallet.cpp
+
+ // If we don't know about this input, skip it and let someone else deal with it
+ const uint256& txhash = txin.prevout.hash;
+ if (pwallet->mapWallet.count(txhash)) {

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Use find to avoid 2nd lookup below.

@promag

promag Jul 4, 2018

Member

Use find to avoid 2nd lookup below.

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Done

+ return redeem_script.empty() && witness_script.empty() && hd_keypaths.empty() && unknown.empty();
+}
+
+void PSBTOutput::Merge(const PSBTOutput& output)

This comment has been minimized.

@araspitzu

araspitzu Jul 5, 2018

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

@araspitzu

araspitzu Jul 5, 2018

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

This comment has been minimized.

@achow101

achow101 Jul 5, 2018

Member

Good catch, fixed

@achow101

achow101 Jul 5, 2018

Member

Good catch, fixed

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 5, 2018

Member

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

Member

instagibbs commented Jul 5, 2018

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

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 5, 2018

Member

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

Member

achow101 commented Jul 5, 2018

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

@laanwj laanwj added this to Blockers in High-priority for review Jul 5, 2018

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 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.

Member

achow101 commented Jul 5, 2018

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.

+
+// Serialize HD keypaths to a stream from a map
+template<typename Stream>
+void SerializeHDKeypaths(Stream& s, const std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths, uint8_t type)

This comment has been minimized.

@araspitzu

araspitzu Jul 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').

@araspitzu

araspitzu Jul 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').

This comment has been minimized.

@achow101

achow101 Jul 6, 2018

Member

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

@achow101

achow101 Jul 6, 2018

Member

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

test/functional/rpc_psbt.py
+ pubkey2 = self.nodes[2].getaddressinfo(self.nodes[2].getnewaddress())['pubkey']
+ p2sh = self.nodes[1].addmultisigaddress(2, [pubkey0, pubkey1, pubkey2], "", "legacy")['address']
+ p2wsh = self.nodes[1].addmultisigaddress(2, [pubkey0, pubkey1, pubkey2], "", "bech32")['address']
+ p2wpkh = self.nodes[1].getnewaddress("", "legacy")

This comment has been minimized.

@sipa

sipa Jul 7, 2018

Member

That doesn't look very witnessy.

@sipa

sipa Jul 7, 2018

Member

That doesn't look very witnessy.

This comment has been minimized.

@achow101

achow101 Jul 7, 2018

Member

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

@achow101

achow101 Jul 7, 2018

Member

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 7, 2018

Member

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

Member

sipa commented Jul 7, 2018

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

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jul 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.

Member

promag commented Jul 7, 2018

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.

src/script/sign.h
+static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
+
+// The separator is 0x00. Reading this in means that the unserializer can interpret it
+// as a 0 length key. which indicates that this is the separator. The separator has no value.

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

micro-nit: period after key seems misplaced

@instagibbs

instagibbs Jul 9, 2018

Member

micro-nit: period after key seems misplaced

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Fixed

@achow101

achow101 Jul 9, 2018

Member

Fixed

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

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

this deserialized a single keypath, right?

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

@instagibbs

instagibbs Jul 9, 2018

Member

this deserialized a single keypath, right?

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

This comment has been minimized.

@sipa

sipa Jul 9, 2018

Member

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

@sipa

sipa Jul 9, 2018

Member

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

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

src/script/sign.h
+ }
+}
+
+/** A structure for PSBTs which contain per input information */

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

micro-nit: per-input

@instagibbs

instagibbs Jul 9, 2018

Member

micro-nit: per-input

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Fixed

@achow101

achow101 Jul 9, 2018

Member

Fixed

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

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

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

@achow101

achow101 Jul 9, 2018

Member

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

src/rpc/rawtransaction.cpp
+ " }\n"
+ " ,...\n"
+ " ]\n"
+ " \"fee\" : fee (numeric, optional) The transaction fee paid if all UTXOs are present\n"

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

This comment has been minimized.

@sipa

sipa Jul 9, 2018

Member

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?

@sipa

sipa Jul 9, 2018

Member

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?

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
+ "\nArguments:\n"
+ "1. \"hexstring\" (string, required) The hex string of a raw transaction\n"
+ "2. permitsigdata (boolean, optional) If true, any signatures in the input will be discarded.\n"

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

what's the default value?

@instagibbs

instagibbs Jul 9, 2018

Member

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

what's the default value?

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Added default value

@achow101

achow101 Jul 9, 2018

Member

Added default value

src/rpc/rawtransaction.cpp
+ "1. \"hexstring\" (string, required) The hex string of a raw transaction\n"
+ "2. permitsigdata (boolean, optional) If true, any signatures in the input will be discarded.\n"
+ " If false, RPC will fail if any signatures are present.\n"
+ "3. iswitness (boolean, optional) Whether the transaction hex is a serialized witness transaction.\n"

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

default value?

@instagibbs

instagibbs Jul 9, 2018

Member

default value?

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

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.

@achow101

achow101 Jul 9, 2018

Member

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.

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

ok, then please say that in the help

@instagibbs

instagibbs Jul 9, 2018

Member

ok, then please say that in the help

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Done

src/wallet/rpcwallet.h
@@ -28,4 +28,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
UniValue getaddressinfo(const JSONRPCRequest& request);
UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);
+bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type = 1, bool sign = true);

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Which?

@achow101

achow101 Jul 9, 2018

Member

Which?

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

was referring to pwallet, which you dispute as an improvement

@instagibbs

instagibbs Jul 9, 2018

Member

was referring to pwallet, which you dispute as an improvement

src/wallet/rpcwallet.cpp
+
+ "\nArguments:\n"
+ "1. \"psbt\" (string, required) The transaction base64 string\n"
+ "2. sign, (boolean, optional, default=true) Also sign the transaction when updating\n"

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

comma after sign?

@instagibbs

instagibbs Jul 9, 2018

Member

comma after sign?

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Fixed

@achow101

achow101 Jul 9, 2018

Member

Fixed

+ return true;
+}
+
+void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

change this to passing wallet by reference as well?

@instagibbs

instagibbs Jul 9, 2018

Member

change this to passing wallet by reference as well?

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

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

@achow101

achow101 Jul 9, 2018

Member

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

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 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.

Member

instagibbs commented Jul 9, 2018

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 9, 2018

Member

might want to fix this warning:

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

laanwj commented Jul 9, 2018

might want to fix this warning:

/.../bitcoin/src/wallet/rpcwallet.h:14:1: warning: class 'PartiallySignedTransaction' was previously declared as a struct [-Wmismatched-tags]
class PartiallySignedTransaction;
^
/.../bitcoin/src/script/sign.h:486:8: note: previous use is here
struct PartiallySignedTransaction
       ^
/.../bitcoin/src/wallet/rpcwallet.h:14:1: note: did you mean struct here?
class PartiallySignedTransaction;
^~~~~
struct
1 warning generated.
src/rpc/rawtransaction.cpp
+
+ "\nResult:\n"
+ "{\n"
+ " \"psbt\" : \"value\", (string) The base64-encoded partially signed transaction\n"

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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

@instagibbs

instagibbs Jul 9, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

Done (kind of)

@achow101

achow101 Jul 9, 2018

Member

Done (kind of)

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

This comment has been minimized.

@instagibbs

instagibbs Jul 9, 2018

Member

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.

@instagibbs

instagibbs Jul 9, 2018

Member

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 9, 2018

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

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 9, 2018

Member

might want to fix this warning

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

Member

achow101 commented Jul 9, 2018

might want to fix this warning

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 10, 2018

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.

@Sjors

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.

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

This comment has been minimized.

@Sjors

Sjors Jul 10, 2018

Member

Nit: 2018

@Sjors

Sjors Jul 10, 2018

Member

Nit: 2018

This comment has been minimized.

@achow101

achow101 Jul 10, 2018

Member

Fixed

test/functional/rpc_psbt.py
+ def run_test(self):
+
+ # Get some money and activate segwit
+ self.nodes[0].generate(500)

This comment has been minimized.

@Sjors

Sjors Jul 10, 2018

Member

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.

@Sjors

Sjors Jul 10, 2018

Member

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.

This comment has been minimized.

@achow101

achow101 Jul 10, 2018

Member

Done

test/functional/rpc_psbt.py
+ new_psbt = self.nodes[0].converttopsbt(rawtx['hex'])
+ self.nodes[0].decodepsbt(new_psbt)
+
+ # BIP 174 Test Vectors

This comment has been minimized.

@Sjors

Sjors Jul 10, 2018

Member

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

@Sjors

Sjors Jul 10, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 10, 2018

Member

Done

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 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.

Member

achow101 commented Jul 10, 2018

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.
Member

sipa commented Jul 11, 2018

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

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 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.

Member

achow101 commented Jul 11, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Jul 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.

Member

Sjors commented Jul 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Jul 13, 2018

Member

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

Member

Sjors commented Jul 13, 2018

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

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Jul 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.

Member

achow101 commented Jul 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Jul 13, 2018

Member

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

Member

Sjors commented Jul 13, 2018

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

src/script/sign.h
+ throw std::ios_base::failure("Invalid PSBT magic bytes");
+ }
+ unsigned char magic_sep;
+ s >> magic_sep;

This comment has been minimized.

@nodar-chkuaselidze

nodar-chkuaselidze Jul 13, 2018

Does not magic_sep need to be 0xff ?

@nodar-chkuaselidze

nodar-chkuaselidze Jul 13, 2018

Does not magic_sep need to be 0xff ?

This comment has been minimized.

@achow101

achow101 Jul 13, 2018

Member

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

@achow101

achow101 Jul 13, 2018

Member

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

achow101 added some commits Jun 27, 2018

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.
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.
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.
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.
@DrahtBot

This comment has been minimized.

Show comment
Hide comment
@DrahtBot

DrahtBot Jul 14, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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.

Note to reviewers: This pull request conflicts with the following ones:
  • #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.

src/rpc/rawtransaction.cpp
+ "createpsbt and walletcreatefundedpsbt should be used for new applications.\n"
+ "\nArguments:\n"
+ "1. \"hexstring\" (string, required) The hex string of a raw transaction\n"
+ "2. permitsigdata (boolean, optional, default=false) If true, any signatures in the input will be discarde and conversion.\n"

This comment has been minimized.

@sipa

sipa Jul 16, 2018

Member

Typo: discarde

@sipa

sipa Jul 16, 2018

Member

Typo: discarde

This comment has been minimized.

@achow101

achow101 Jul 16, 2018

Member

Fixed

src/rpc/rawtransaction.cpp
+ CMutableTransaction tx;
+ bool permitsigdata = request.params[1].isNull() ? false : request.params[1].get_bool();
+ bool iswitness = request.params[2].isNull() ? true : request.params[2].get_bool();
+ bool try_witness = permitsigdata ? iswitness : true;

This comment has been minimized.

@sipa

sipa Jul 16, 2018

Member

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:

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

sipa Jul 16, 2018

Member

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:

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

This comment has been minimized.

@achow101

achow101 Jul 16, 2018

Member

Done

src/rpc/rawtransaction.cpp
+ if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
+ throw std::runtime_error(
+ "createpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable )\n"
+ "\nCreates a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"

This comment has been minimized.

@sipa

sipa Jul 16, 2018

Member

No inputs will be added.

@sipa

sipa Jul 16, 2018

Member

No inputs will be added.

This comment has been minimized.

@achow101

achow101 Jul 16, 2018

Member

Fixed

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

This comment has been minimized.

@sipa

sipa Jul 16, 2018

Member

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

@sipa

sipa Jul 16, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 16, 2018

Member

Fixed

achow101 added some commits Jun 29, 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.
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.
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.
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 17, 2018

Member

utACK 020628e

Member

sipa commented Jul 17, 2018

utACK 020628e

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 18, 2018

Member

utACK 020628e

Member

laanwj commented Jul 18, 2018

utACK 020628e

@laanwj laanwj merged commit 020628e into bitcoin:master Jul 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 18, 2018

Merge #13557: BIP 174 PSBT Serializations and RPCs
020628e Tests for PSBT (Andrew Chow)
a4b06fb Create wallet RPCs for PSBT (Andrew Chow)
c27fe41 Create utility RPCs for PSBT (Andrew Chow)
8b5ef27 SignPSBTInput wrapper function (Andrew Chow)
58a8e28 Refactor transaction creation and transaction funding logic (Andrew Chow)
e9d86a4 Methods for interacting with PSBT structs (Andrew Chow)
12bcc64 Add pubkeys and whether input was witness to SignatureData (Andrew Chow)
41c607f Implement PSBT Structures and un/serialization methods per BIP 174 (Andrew Chow)

Pull request description:

  This Pull Request fully implements the [updated](bitcoin/bips#694) BIP 174 specification. It is based upon #13425 which implements the majority of the signing logic.

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

  This PR contains structs for PSBT, serialization, and deserialzation code. Some changes to `SignatureData` have been made to support detection of UTXO type and storing public keys.

  ***

  Many RPCs have been added to handle PSBTs.

  `walletprocesspsbt` takes a PSBT format transaction, updates the PSBT with any inputs related to this wallet, signs, and finalizes the transaction. There is also an option to not sign and just update.

  `walletcreatefundedpsbt` creates a PSBT from user provided data in the same form as createrawtransaction. It also funds the transaction and takes an options argument in the same form as `fundrawtransaction`. The resulting PSBT is blank with no input or output data filled in. It is analogous to a combination of `createrawtransaction` and `fundrawtransaction`

  `decodepsbt` takes a PSBT and decodes it to JSON. It is analogous to `decoderawtransaction`

  `combinepsbt` takes multiple PSBTs for the same tx and combines them. It is analogous to `combinerawtransaction`

  `finalizepsbt` takes a PSBT and finalizes the inputs. If all inputs are final, it extracts the network serialized transaction and returns that instead of a PSBT unless instructed otherwise.

  `createpsbt` is like `createrawtransaction` but for PSBTs instead of raw transactions.

  `convertpsbt` takes a network serialized transaction and converts it into a psbt. The resulting psbt will lose all signature data and an explicit flag must be set to allow transactions with signature data to be converted.

  ***

  This supersedes #12136

Tree-SHA512: 1ac7a79e5bc669933f0a6fcc93ded55263fdde9e8c144a30266b13ef9f62aacf43edd4cbca1ffbe003090b067e9643c9298c79be69d7c1b10231b32acafb6338

@fanquake fanquake removed this from Blockers in High-priority for review Jul 18, 2018

@Bushstar Bushstar referenced this pull request in FeatherCoin/Feathercoin Jul 19, 2018

Merged

commits from bitcoin/master #356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment