Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP 174 PSBT Serializations and RPCs #13557

Merged
merged 8 commits into from
Jul 18, 2018
Merged

BIP 174 PSBT Serializations and RPCs #13557

merged 8 commits into from
Jul 18, 2018

Conversation

achow101
Copy link
Member

@achow101 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
Copy link
Contributor

DrahtBot commented Jun 28, 2018

Note to reviewers: This pull request conflicts with the following ones:

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.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SignatureData able to store signatures and scripts"

nit, i

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SignatureData able to store signatures and scripts"

class SignatureExtractorChecker final?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
if (checker->CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) {
CPubKey pubkey(vchPubKey);
sigdata->signatures.emplace(pubkey.GetID(), SigPair(pubkey, scriptSig));
Copy link
Contributor

@promag promag Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

class SignatureExtractorChecker : public BaseSignatureChecker
{
private:
SignatureData* sigdata;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SignatureData able to store signatures and scripts"

Use references instead of pointers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -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)
Copy link
Contributor

@promag promag Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return true;
}
// Look for pubkey in all partial sigs
const auto& it = sigdata->signatures.find(address);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Replace CombineSignatures with ProduceSignature"

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@achow101
Copy link
Member Author

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

@promag
Copy link
Contributor

promag commented Jun 28, 2018

Ops @achow101 do you want me to comment there?

@achow101
Copy link
Member Author

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

Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just comments on the serialization commit.

};

/** A structure for PSBTs which contains per output information */
struct PSBTOutput
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent class name (PartiallySignedInput vs PSBTOutput).

Copy link
Member Author

@achow101 achow101 Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Renamed PartiallySignedInput to PSBTInput

static const uint8_t PSBT_SEPARATOR = 0x00;

template<typename Stream, typename... X>
void SerializeToVector(Stream& s, const X&... args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// Read in the signature from value
uint64_t value_len = ReadCompactSize(s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// Do stuff based on type
switch(type) {
case PSBT_IN_NON_WITNESS_UTXO:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

throw std::ios_base::failure("Duplicate Key, key for unknown value already provided");
}
// Read in the value
uint64_t value_len = ReadCompactSize(s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

s >> witness_script;
break;
}
case PSBT_OUT_BIP32_DERIVATION:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

std::vector<PartiallySignedInput> inputs;
std::vector<PSBTOutput> outputs;
std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
uint64_t num_ins = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need num_ins and use_in_index?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


// Read input data
unsigned int i = 0;
while (!s.empty() && i < tx.vin.size()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



// Note: These constants are in reverse byte order because serialization uses LSB
static constexpr uint32_t PSBT_MAGIC_BYTES = 0x74627370;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add template <typename Stream> before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}

// Serialize HD keypaths to a stream from a map
static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add template<typename Stream> before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


// Read input data
unsigned int i = 0;
while (!s.empty() && i < tx.vin.size()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@achow101 achow101 force-pushed the psbt branch 4 times, most recently from 5162bb4 to ed7a484 Compare June 28, 2018 22:22
Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on rawtransaction.cpp RPCs.


RPCTypeCheck(request.params, {UniValue::VSTR});

// Unserialize the transactions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!input.final_script_witness.IsNull()) {
in.pushKV("final_scripWitness", input.final_script_witness.ToString());
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add unknowns to the decode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

" \"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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is clearer to list the fingerprint separately.

}
}

for (const PartiallySignedTransaction& psbtx : psbtxs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"\nResult:\n"
"{\n"
" \"base64\" : \"value\", (string) The base64-encoded network transaction\n"
" \"hex\" : \"value\", (string) The hex-encoded partially signed transaction if extracted\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

continue;
}

// Fill a SignatureData with input info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RPC does not do any funding, I think.

Explain that this implements a Creator role.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/uint256.h Outdated
@@ -91,6 91,15 @@ class base_blob
((uint64_t)ptr[7]) << 56;
}

uint32_t GetUint32LE(int pos) const
Copy link
Member

@sipa sipa Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("x", fingerprint).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sipa
Copy link
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
Copy link
Member Author

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: else on the same line as }.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -4388,6 4391,333 @@ UniValue sethdseed(const JSONRPCRequest& request)
return NullUniValue;
}

bool parse_hd_keypath(std::string keypath_str, std::vector<uint32_t>& keypath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: function naming style

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return true;
}

void add_keypath_to_map(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: function naming style

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

hd_keypaths.emplace(vchPubKey, keypath);
}

bool fill_psbt(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: function naming style

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@nvk
Copy link

nvk commented Jun 29, 2018

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

Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more nits on serialization code.

inline void Unserialize(Stream& s) {
// Read loop
while(!s.empty()) {
// read size of key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (non_witness_utxo) {
throw std::ios_base::failure("Duplicate Key, input non-witness utxo already provided");
}
if (!witness_utxo.IsNull()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

s >> sig;

// Add to list
partial_sigs.emplace(pubkey.GetID(), SigPair(pubkey, sig));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be std::move(sig).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

inline void Unserialize(Stream& s) {
// Read loop
while(!s.empty()) {
// read size of key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same shorter read routine is possible here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Read global data
while(!s.empty()) {
// read size of key
uint64_t key_len = ReadCompactSize(s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And again here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@achow101 achow101 force-pushed the psbt branch 2 times, most recently from 5f22b5b to 3a0ca94 Compare June 30, 2018 01:14
error = "extra data after PSBT";
return false;
}
} catch (const std::exception& e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should learn to read.

@@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: variable naming style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the .c_str().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

{
if (request.fHelp || request.params.size() != 1)
throw std::runtime_error(
"decodepsbt \"base64string\"\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

"\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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not hex.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
// Merge them
for (const PartiallySignedTransaction& psbtx : psbtxs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to merge in the first one again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to erase the first element from the vector.

merged_psbt.Merge(psbtx);
}
if (!merged_psbt.IsSane()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Merged PSBT is not sane");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the cast necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

@achow101 achow101 force-pushed the psbt branch 2 times, most recently from fe71185 to a871194 Compare June 30, 2018 02:46
meshcollider added a commit that referenced this pull request Feb 25, 2020
5bad792 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)

Pull request description:

  In #13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

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

  In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

  More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

ACKs for top commit:
  instagibbs:
    utACK 5bad792
  jonatack:
    ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
  meshcollider:
    utACK 5bad792

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2020
…thods

5bad792 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)

Pull request description:

  In bitcoin#13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

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

  In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

  More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

ACKs for top commit:
  instagibbs:
    utACK bitcoin@5bad792
  jonatack:
    ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
  meshcollider:
    utACK 5bad792

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…thods

5bad792 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)

Pull request description:

  In bitcoin#13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

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

  In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

  More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

ACKs for top commit:
  instagibbs:
    utACK bitcoin@5bad792
  jonatack:
    ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
  meshcollider:
    utACK 5bad792

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request May 19, 2021
The fix for CPubKey is a part of `Merge bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request May 19, 2021
The fix for CPubKey is a part of `Merge bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request May 19, 2021
The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
barrystyle added a commit to pacprotocol/pacprotocol that referenced this pull request May 27, 2021
backported achow101 psbt implementation (barrystyle)
barrystyle added a commit to pacprotocol/pacprotocol that referenced this pull request May 27, 2021
backported achow101 psbt implementation (barrystyle)
barrystyle added a commit to pacprotocol/pacprotocol that referenced this pull request May 31, 2021
backported achow101 psbt implementation (barrystyle)
kwvg pushed a commit to kwvg/dash that referenced this pull request Jun 4, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jun 4, 2021
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 7, 2021
barrystyle added a commit to pacprotocol/pacprotocol that referenced this pull request Jun 8, 2021
barrystyle added a commit to pacprotocol/pacprotocol that referenced this pull request Jun 8, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jun 9, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 9, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Jul 13, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Apr 26, 2022
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Apr 30, 2022
malbit added a commit to malbit/raptoreum that referenced this pull request Aug 7, 2022
[bitcoin#14021](bitcoin/bitcoin#14021) - Import key origin data through descriptors in importmulti
[bitcoin#13557](bitcoin/bitcoin#13557) - BIP174 Serialization and logic only (partial merge)
additional extensions to utilities.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants