-
Notifications
You must be signed in to change notification settings - Fork 36.4k
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
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/script/sign.cpp
Outdated
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 ) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
@@ -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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.cpp
Outdated
{ | ||
if (checker->CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) { | ||
CPubKey pubkey(vchPubKey); | ||
sigdata->signatures.emplace(pubkey.GetID(), SigPair(pubkey, scriptSig)); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
class SignatureExtractorChecker : public BaseSignatureChecker | ||
{ | ||
private: | ||
SignatureData* sigdata; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.cpp
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.cpp
Outdated
return true; | ||
} | ||
// Look for pubkey in all partial sigs | ||
const auto& it = sigdata->signatures.find(address); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Ops @achow101 do you want me to comment there? |
No need to comment twice. I will update that PR and then rebase this onto that. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
src/script/sign.h
Outdated
static const uint8_t PSBT_SEPARATOR = 0x00; | ||
|
||
template<typename Stream, typename... X> | ||
void SerializeToVector(Stream& s, const X&... args) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
} | ||
|
||
// Read in the signature from value | ||
uint64_t value_len = ReadCompactSize(s); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
throw std::ios_base::failure("Duplicate Key, key for unknown value already provided"); | ||
} | ||
// Read in the value | ||
uint64_t value_len = ReadCompactSize(s); |
There was a problem hiding this comment.
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;
.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
std::vector<PartiallySignedInput> inputs; | ||
std::vector<PSBTOutput> outputs; | ||
std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown; | ||
uint64_t num_ins = 0; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
src/script/sign.h
Outdated
|
||
// Read input data | ||
unsigned int i = 0; | ||
while (!s.empty() && i < tx.vin.size()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ad1c9ce
to
2eb4f61
Compare
src/script/sign.h
Outdated
|
||
|
||
// Note: These constants are in reverse byte order because serialization uses LSB | ||
static constexpr uint32_t PSBT_MAGIC_BYTES = 0x74627370; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
|
||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constexpr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
} | ||
|
||
// 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/script/sign.h
Outdated
} | ||
|
||
// Serialize HD keypaths to a stream from a map | ||
static void SerializeHDKeypaths(std::vector<uint32_t>>& hd_keypaths, Stream& s) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/script/sign.h
Outdated
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/script/sign.h
Outdated
|
||
// Read input data | ||
unsigned int i = 0; | ||
while (!s.empty() && i < tx.vin.size()) { |
There was a problem hiding this comment.
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.
5162bb4
to
ed7a484
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/rpc/rawtransaction.cpp
Outdated
if (!input.final_script_witness.IsNull()) { | ||
in.pushKV("final_scripWitness", input.final_script_witness.ToString()); | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/rpc/rawtransaction.cpp
Outdated
" \"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" |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
src/rpc/rawtransaction.cpp
Outdated
} | ||
} | ||
|
||
for (const PartiallySignedTransaction& psbtx : psbtxs) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/rpc/rawtransaction.cpp
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/rpc/rawtransaction.cpp
Outdated
"\nResult:\n" | ||
"{\n" | ||
" \"base64\" : \"value\", (string) The base64-encoded network transaction\n" | ||
" \"hex\" : \"value\", (string) The hex-encoded partially signed transaction if extracted\n" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/rpc/rawtransaction.cpp
Outdated
continue; | ||
} | ||
|
||
// Fill a SignatureData with input info |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/rpc/rawtransaction.cpp
Outdated
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/rpc/rawtransaction.cpp
Outdated
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
General comment on naming of RPC field names and input arguments, |
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
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.cpp
Outdated
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()) { |
There was a problem hiding this comment.
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 }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wallet/rpcwallet.cpp
Outdated
@@ -4388,6 4391,333 @@ UniValue sethdseed(const JSONRPCRequest& request) | |||
return NullUniValue; | |||
} | |||
|
|||
bool parse_hd_keypath(std::string keypath_str, std::vector<uint32_t>& keypath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: function naming style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wallet/rpcwallet.cpp
Outdated
return true; | ||
} | ||
|
||
void add_keypath_to_map(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: function naming style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wallet/rpcwallet.cpp
Outdated
hd_keypaths.emplace(vchPubKey, keypath); | ||
} | ||
|
||
bool fill_psbt(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: function naming style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
We will start implementing as soon as this gets merged. |
There was a problem hiding this 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.
src/script/sign.h
Outdated
inline void Unserialize(Stream& s) { | ||
// Read loop | ||
while(!s.empty()) { | ||
// read size of key |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
if (non_witness_utxo) { | ||
throw std::ios_base::failure("Duplicate Key, input non-witness utxo already provided"); | ||
} | ||
if (!witness_utxo.IsNull()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
src/script/sign.h
Outdated
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
s >> sig; | ||
|
||
// Add to list | ||
partial_sigs.emplace(pubkey.GetID(), SigPair(pubkey, sig)); |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
inline void Unserialize(Stream& s) { | ||
// Read loop | ||
while(!s.empty()) { | ||
// read size of key |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/script/sign.h
Outdated
// Read global data | ||
while(!s.empty()) { | ||
// read size of key | ||
uint64_t key_len = ReadCompactSize(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
5f22b5b
to
3a0ca94
Compare
src/core_read.cpp
Outdated
error = "extra data after PSBT"; | ||
return false; | ||
} | ||
} catch (const std::exception& e) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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()) {
There was a problem hiding this comment.
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.
src/core_read.cpp
Outdated
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: variable naming style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/core_read.cpp
Outdated
@@ -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()); |
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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&
.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/rpc/rawtransaction.cpp
Outdated
{ | ||
if (request.fHelp || request.params.size() != 1) | ||
throw std::runtime_error( | ||
"decodepsbt \"base64string\"\n" |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
src/rpc/rawtransaction.cpp
Outdated
"\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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not hex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/rpc/rawtransaction.cpp
Outdated
|
||
PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one | ||
// Merge them | ||
for (const PartiallySignedTransaction& psbtx : psbtxs) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/rpc/rawtransaction.cpp
Outdated
merged_psbt.Merge(psbtx); | ||
} | ||
if (!merged_psbt.IsSane()) { | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Merged PSBT is not sane"); |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 *
.
src/rpc/rawtransaction.cpp
Outdated
"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" |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
fe71185
to
a871194
Compare
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
…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
…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
The fix for CPubKey is a part of `Merge bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
The fix for CPubKey is a part of `Merge bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
backported achow101 psbt implementation (barrystyle)
backported achow101 psbt implementation (barrystyle)
backported achow101 psbt implementation (barrystyle)
…coin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs (PR#4186)
merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: from kittywhiskers:psbt
merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs
Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
[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.
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 asfundrawtransaction
. The resulting PSBT is blank with no input or output data filled in. It is analogous to a combination ofcreaterawtransaction
andfundrawtransaction
decodepsbt
takes a PSBT and decodes it to JSON. It is analogous todecoderawtransaction
combinepsbt
takes multiple PSBTs for the same tx and combines them. It is analogous tocombinerawtransaction
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 likecreaterawtransaction
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