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

Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. #14978

Merged
merged 7 commits into from Feb 14, 2019

Conversation

Projects
None yet
@gwillen
Copy link
Contributor

commented Dec 17, 2018

  • Move most PSBT definitions into psbt.h.
  • Move most PSBT RPC utilities into psbt.{h,cpp}.
  • Move wallet-touching PSBT RPC utilities (FillPSBT) into
    wallet/psbtwallet.{h,cpp}.
  • Switch exceptions from JSONRPCError() to new PSBTException class.
  • Split DecodePSBT into DecodeBase64PSBT (old behavior) and DecodeRawPSBT.
  • Add one new version of DecodeBase64 utility in strencodings.h (and
    corresponding DecodeBase32 for completeness).
  • Factor BroadcastTransaction utility function out of sendrawtransaction RPC
    handler in rpc/rawtransaction.cpp

Note: For those keeping score at home wondering why refactor, this is in anticipation of (and developed in parallel with) a change to actually introduce GUI use of all this stuff, which is already under development and working-ish.

src/core_read.cpp Outdated

bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
{
CDataStream ss_data(tx_data.begin(), tx_data.end(), SER_NETWORK, PROTOCOL_VERSION);

This comment has been minimized.

Copy link
@gwillen

gwillen Dec 17, 2018

Author Contributor

Ok, I thought I was very clever converting this from a std::vector<unsigned char> to a std::string, but the reality appears to be that Clang is happy with this line and g++ hates it, so I'm going to have to massage this back towards the original version a bit, in order to get it to build anywhere but macOS.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

I cleverly made a small change that apparently g++ does not accept but Clang does, so I will have to fix that before it will build on Linux, oops. (I commented on the offending line in the diff.)

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch Dec 17, 2018

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

Hmm, something is also legitimately broken in the tests, will investigate tomorrow. :-\

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15253 (Net: Consistently log outgoing INV messages by Empact)
  • #15214 (tests: Check for expected return values when calling functions returning a success code by practicalswift)
  • #13932 (Additional utility RPCs for PSBT by achow101)
  • #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
  • #13266 ([moveonly] privatize SignatureExtractorChecker by Empact)
  • #12461 (scripted-diff: Rename key size consts to be relative to their class by Empact)
  • #9152 (Wallet/RPC: sweepprivkeys method to scan UTXO set and send to local wallet by luke-jr)

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.

Show resolved Hide resolved src/psbt.h
@promag

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Concept ACK.

for use in GUI code

Can you detail the GUI part?

which is already under development and working-ish.

Is it available?

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch Dec 17, 2018

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

Can you detail the GUI part?

I'm adding an interface for offline and multisig signing in the GUI, using PSBT as the on-disk format for the intermediate files. See e.g. how Bitcoin Armory handles offline transactions, which I'm basically copying.

Is it available?

I pasted it into #bitcoin-core-dev awhile ago -- I will link it here in the comment thread once I'm sure the tip of my branch is still actually building.

@jb55

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

The GUI work is WIP here: https://github.com/gwillen/bitcoin/tree/feature-offline-v1 . I made that branch at a known-working version, since I'm making significant changes to it right now.

It's not currently based on this PR, which I will have to rebase it onto later; this PR is an extraction of some refactoring that also appears in that PR, plus some other stuff that doesn't yet.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@jb55 Yeah, I can definitely split it out if people prefer. Will take a day or two to get to. I might ask slight forgiveness in some cases where a reasonable intermediate state between two combined changes never existed, and would be hard to recreate.

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Would be helpful to split the commit up, for review now and for intelligibility when looking back in the future.

@achow101

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Concept ACK. Agree with above, split this into multiple commits.

@promag

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

I definitely agree in splitting it.

@gwillen checked out the branch and looks promising.

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch 2 times, most recently Jan 9, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Sorry for the delay. I have split the PR into more reasonable commits. The one that does the bulk of the code movement between files ("Move PSBT definitions and code to separate files") is now only movement (and adjusting headers / Makefile), and no other substantive changes.

The final result is exactly the same as before, except for a single stray blank line I had accidentally introduced in the previous version, which is removed.

Every intermediate step is intended to be sane and self-contained (and buildable), but I haven't actually verified (by compiler rather than by eye) that every intermediate step cleanly builds yet.

(EDIT: Wellll, I was pretty close. All intermediate states now build and pass tests.)

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch to dd45d3f Jan 9, 2019

@jb55

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

This looks much better now! Thanks!

utACK dd45d3f

The refactorings look pretty good in general.

I did a quick skim of the code, I didn't see anything obviously wrong but I'm no expert on this part of the codebase.

@achow101
Copy link
Member

left a comment

utACK dd45d3f

Some comments, nothing that block this IMO

@@ -188,9 +188,9 @@ std::vector<unsigned char> DecodeBase64(const char* p, bool* pfInvalid)
return ret;
}

std::string DecodeBase64(const std::string& str)
std::string DecodeBase64(const std::string& str, bool* pfInvalid)

This comment has been minimized.

Copy link
@achow101

achow101 Jan 9, 2019

Member

nit: variable name should be snake_case. See doc/developer-notes.md for naming convention.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 9, 2019

Author Contributor

This was copied from surrounding context (it just wraps the c_str version of the same function, which has the same parameter named the same thing, and I'm propagating it outwards to the wrapper.)

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

From https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md:

When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 18, 2019

Member

Nit: we're only calling DecodeBase64 in a handful of places, so you could also change the return type to bool and pass the output string as a reference param. Though it also makes sense to remain consistent with the existing (char*) methods.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

I will fix the argument name, but otherwise leave things alone unless you would strongly prefer the more invasive changes. (I am changing pfInvalid to pf_invalid everywhere in the surrounding context, because it will drive me completely nuts to make it inconsistent. Please don't make me do that.)

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

Yeah, let's not do too much in this PR.

src/psbt.h Outdated
/** An exception in PSBT processing that represents a problem with the input data --
when the input data is user-supplied, it is appropriate to catch() this and use
e.what() to retrieve a human-readable error message. */
class PSBTException: public std::runtime_error {

This comment has been minimized.

Copy link
@achow101

achow101 Jan 9, 2019

Member

This results in RPC errors with a code RPC_MISC_ERROR. I think it would be better if you could somehow make the error code more specific (the message still comes through).

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 9, 2019

Author Contributor

Yeah, I'd love any thoughts on how to accomplish this without gunking up non-RPC code too much when it calls this codepath. I don't really want this to depend on the RPC stuff.

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Thread 617247c#r246575002

One simple way this could be achieved is to add custom error handler to CRPCCommand which would translate this exception to the desired output.

One non intrusive way to add the error handler is to set it in

void RegisterWalletRPCCommands(CRPCTable &t)
{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
t.appendCommand(commands[vcidx].name, &commands[vcidx]);

This way all desired commands would share the same error handling and then we can start removing duplicate error messages in the code by introducing more specific exceptions.

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

I'm slightly worried about the use of exceptions in code that use usable in more places than just RPC, as C++ can't guarantee that all possible exceptions are being dealt with at the right layer.

Would an enum with PSBT-related error codes, plus a enum to string conversion function (like in script/script_error) make sense?

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

FWIW I believe the travis failure is spurious -- it failed a single unrelated test, on a single platform, and the code hasn't changed since the last time it succeeded.

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch from dd45d3f to c062d74 Jan 16, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

(Minor edit to remove a single unnecessary #include line that was bugging me, no significant change.)

@promag
Copy link
Member

left a comment

utACK c062d74 .Verified the move only commit, just adds necessary includes and updates the makefile with the new file. Some nits and other comments for your consideration.

bool invalid;
std::string tx_data = DecodeBase64(base64_tx, &invalid);
if (invalid) {
error = "invalid base64";

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 801333b

Could add test for this new error?

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

Done.


bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
{
CDataStream ss_data(tx_data.data(), tx_data.data() + tx_data.size(), SER_NETWORK, PROTOCOL_VERSION);

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 801333b

To avoid changing this line you could add a commit before with something along:

--- a/src/streams.h
+++ b/src/streams.h
@@ -236,17 +236,8 @@ public:
         Init(nTypeIn, nVersionIn);
     }

-    CDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
-    {
-        Init(nTypeIn, nVersionIn);
-    }
-
-    CDataStream(const std::vector<char>& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
-    {
-        Init(nTypeIn, nVersionIn);
-    }
-
-    CDataStream(const std::vector<unsigned char>& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
+    template <typename T>
+    CDataStream(const T& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
     {
         Init(nTypeIn, nVersionIn);
     }

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

Is this safe (does it not "prove too much")? Certainly I can imagine it might cause worse error messages in some cases. I assume it wasn't done that way for some reason originally?

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

@achow101 thoughts?

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Just for fun, I went and checked -- this code is essentially unchanged since the repo was first imported to github in 2009. So the rationale is probably lost to history...

@@ -37,7 +37,8 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
*/
bool ParseHashStr(const std::string& strHex, uint256& result);
std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
NODISCARD bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);
NODISCARD bool DecodeBase64PSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 801333b

nit, would be nice see brief comments.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 18, 2019

Member

nit: base64_psbt (see next comment)

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

Done.

@@ -37,7 +37,8 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
*/
bool ParseHashStr(const std::string& strHex, uint256& result);
std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
NODISCARD bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);
NODISCARD bool DecodeBase64PSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);
NODISCARD bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error);

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 801333b

nit, s/tx_data/raw_tx?

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 18, 2019

Member

Both tx_data and raw_tx suggest a transaction hex. Something like raw_psbt is probably more clear, and consistent with the function name.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

Done.

@@ -13,6 +13,9 @@ namespace interfaces {
class Chain;
} // namespace interfaces

/** Broadcast a transaction */
std::string BroadcastTransaction(CTransactionRef tx, bool allowhighfees = false);

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 745c540

Shouldn't this be elsewhere if the goal is to use in the GUI?

I think this should return uint256.

nit, could document that the transaction id is returned.

nit, s/allowhighfees/allow_high_fees.

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

Returning uint256 seems more natural indeed.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 18, 2019

Member

I also need access to this function in #14912 from other wallet RPC methods. Not really an issue, because I can always include src/rpc/rawtransaction.h. But perhaps create a new src/wallet/transaction.cpp?

I might add BroadcastTransaction(PartiallySignedTransaction psbt, bool allowhighfees = false) in my own PR.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Author Contributor

Agreed that this should be elsewhere, I will create src/wallet/transaction.cpp for it. (I was reluctant to be too original just to move a single function, but I prefer it that way also.) And agree regarding the return value, will change it.

I don't think there's too much additional value in taking a PartiallySignedTransaction, since finalization always results in a CTransaction.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

(agree with the latter, I ended up with a different approach too, the seperate file is useful enough)

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Done, but this now makes it more obvious that BroadcastTransaction has the same question as the PSBT functions as to how it shall signal an error, since right now it throws UniValues like the other RPC stuff.

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

// parse hex string from parameter
CMutableTransaction mtx;
if (!DecodeHexTx(mtx, request.params[0].get_str()))
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
CTransactionRef tx(MakeTransactionRef(std::move(mtx)));

bool allowhighfees = !request.params[1].isNull() && request.params[1].get_bool();

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 745c540

nit, despite being valid I think it's more readable in the form:

bool allowhighfees = false;
if (!request.params[1].isNull()) allowhighfees = request.params[1].get_bool();

Because it is more clear the default value.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Makes sense, done.

src/psbt.h Outdated
/** An exception in PSBT processing that represents a problem with the input data --
when the input data is user-supplied, it is appropriate to catch() this and use
e.what() to retrieve a human-readable error message. */
class PSBTException: public std::runtime_error {

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Thread 617247c#r246575002

One simple way this could be achieved is to add custom error handler to CRPCCommand which would translate this exception to the desired output.

One non intrusive way to add the error handler is to set it in

void RegisterWalletRPCCommands(CRPCTable &t)
{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
t.appendCommand(commands[vcidx].name, &commands[vcidx]);

This way all desired commands would share the same error handling and then we can start removing duplicate error messages in the code by introducing more specific exceptions.

src/psbt.h Outdated
{
return !(a == b);
// Checks if they refer to the same underlying CTransaction
bool SameTx(const PartiallySignedTransaction& psbt) {

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 08ef528

If this is only used once and you now it's called from Merge then I suggest to inline it there.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Makes sense, done.

@@ -391,20 +391,15 @@ struct PartiallySignedTransaction
std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;

bool IsNull() const;
void Merge(const PartiallySignedTransaction& psbt);
NODISCARD bool Merge(const PartiallySignedTransaction& psbt);

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit 08ef528

Add comment explaining the return value.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Done.

}
}

PartiallySignedTransaction CombinePSBTs(std::vector<PartiallySignedTransaction> psbtxs) {

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit c062d74

Make the argument a const reference.

nit, brace on new line.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2019

Author Contributor

Done.

@sipa
Copy link
Member

left a comment

Big concept ACK

@@ -188,9 +188,9 @@ std::vector<unsigned char> DecodeBase64(const char* p, bool* pfInvalid)
return ret;
}

std::string DecodeBase64(const std::string& str)
std::string DecodeBase64(const std::string& str, bool* pfInvalid)

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

From https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md:

When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.

@@ -13,6 +13,9 @@ namespace interfaces {
class Chain;
} // namespace interfaces

/** Broadcast a transaction */
std::string BroadcastTransaction(CTransactionRef tx, bool allowhighfees = false);

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

Returning uint256 seems more natural indeed.

src/psbt.h Outdated
/** An exception in PSBT processing that represents a problem with the input data --
when the input data is user-supplied, it is appropriate to catch() this and use
e.what() to retrieve a human-readable error message. */
class PSBTException: public std::runtime_error {

This comment has been minimized.

Copy link
@sipa

sipa Jan 17, 2019

Member

I'm slightly worried about the use of exceptions in code that use usable in more places than just RPC, as C++ can't guarantee that all possible exceptions are being dealt with at the right layer.

Would an enum with PSBT-related error codes, plus a enum to string conversion function (like in script/script_error) make sense?

@Sjors Sjors referenced this pull request Jan 18, 2019

Open

[WIP] External signer support (e.g. hardware wallet) #14912

0 of 2 tasks complete
@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

There we go! Ready for another look.

@sipa
Copy link
Member

left a comment

Concept ACK. Also rough code review ACK, though I haven't verified code movement yet.

Show resolved Hide resolved src/core_io.h
Show resolved Hide resolved src/rpc/util.cpp Outdated
@sipa
Copy link
Member

left a comment

utACK acac678, only nits.

#include <primitives/transaction.h>
#include <uint256.h>

typedef enum TransactionError_t {

This comment has been minimized.

Copy link
@sipa

sipa Feb 9, 2019

Member

This looks like a C-ism. You can just use enum TransactionError { ... }; in C++ with (I think) the same effect.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 9, 2019

Author Contributor

In my defense I copied it from ScriptError_t. I'll change it.

src/psbt.h Outdated
bool FinalizePSBT(PartiallySignedTransaction& psbtx);

/**
* Finalizes a PSBT if possible, and extracts it to a CTransaction if it could be finalized.

This comment has been minimized.

Copy link
@sipa

sipa Feb 9, 2019

Member

Nit: CMutableTransaction.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 9, 2019

Author Contributor

Oh, you're right, I think I had to change it for #14906 and forgot to change the comment. Will fix.

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch from acac678 to 9f2297d Feb 9, 2019

#include <primitives/transaction.h>
#include <uint256.h>

enum TransactionError {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 9, 2019

Member

Should be enum class?

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 10, 2019

Author Contributor

Oh, I've never used that before but it looks cool.

It looks like if I made that change, all references to the enum constants would have to be qualified with the name of the enum? So I should remove all the TRANSACTION_ERR prefixes, so e.g. TRANSACTION_ERR_MISSING_INPUTS would become TransactionError::MISSING_INPUTS?

I'm always in favor of adding more compile-time checks when it's easy, so I'm happy to make the change if that's the preferred approach.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 10, 2019

Author Contributor

Updated.

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch from 9f2297d to 6097ae0 Feb 10, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

re-utACK 6097ae0, main changes since my last review:

  • moved BroadcastTransaction to src/node instead of src/wallet/transaction.cpp, because it's not a wallet thing.
  • switched away from exceptions d988f06
  • CombinePSBTs now returns boolean and has an &out and &error param

gwillen added some commits Jan 9, 2019

Factor BroadcastTransaction out of sendrawtransaction
Factor out a new BroadcastTransaction function, performing the core work of the
sendrawtransaction rpc, so that it can be used from the GUI code. Move it from
src/rpc/ to src/node/.
Move PSBT definitions and code to separate files
Move non-wallet PSBT code to src/psbt.{h,cpp}, and PSBT wallet code to
src/wallet/psbtwallet.{h,cpp}. This commit contains only code movement (and
adjustments to includes and Makefile.am.)
Add pf_invalid arg to std::string DecodeBase{32,64}
Add support for the optional "pf_invalid" out parameter (which allows the caller
to detect decoding failures) to the std::string versions of DecodeBase32 and
DecodeBase64. The char* versions already have this feature.

Also, rename all uses of pfInvalid to pf_invalid to match style guidelines.
Split DecodePSBT into Base64 and Raw versions
Split up DecodePSBT, which both decodes base64 and then deserializes a
PartiallySignedTransaction, into two functions: DecodeBase64PSBT, which retains
the old behavior, and DecodeRawPSBT, which only performs the deserialization.

Add a test for base64 decoding failure.
Factor out combine / finalize / extract PSBT helpers
Refactor the new CombinePSBT, FinalizePSBT, and FinalizeAndExtractPSBT
general-purpose functions out of the combinepsbt and finalizepsbt RPCs,
for use in the GUI code.
Remove op== on PSBTs; check compatibility in Merge
Remove the op== on PartiallySignedTransaction, which only checks that the
CTransactions are equal. Instead, check this directly in Merge, and return
false if the CTransactions are not equal (so the PSBTs cannot be merged.)
Switch away from exceptions in refactored tx code
After refactoring general-purpose PSBT and transaction code out of RPC code,
for use in the GUI, it's no longer appropriate to throw exceptions. Instead we
now return bools for success, and take an output parameter for an error object.
We still use JSONRPCError() for the error objects, since only RPC callers
actually care about the error codes.

@gwillen gwillen force-pushed the gwillen:feature-refactor-psbt-rpcs branch from 6097ae0 to 102faad Feb 11, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Feb 11, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Can 1-2 people who already acked, but haven't looked in awhile, take a quick look and give a reack? I do not expect further changes unless I am forced to rebase again.

@achow101

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 102faad

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@gwillen thanks for being so proactive with addressing review comments :)

@meshcollider meshcollider merged commit 102faad into bitcoin:master Feb 14, 2019

2 checks passed

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

meshcollider added a commit that referenced this pull request Feb 14, 2019

Merge #14978: Factor out PSBT utilities from RPCs for use in GUI code…
…; related refactoring.

102faad Factor out combine / finalize / extract PSBT helpers (Glenn Willen)
78b9893 Remove op== on PSBTs; check compatibility in Merge (Glenn Willen)
bd0dbe8 Switch away from exceptions in refactored tx code (Glenn Willen)
c6c3d42 Move PSBT definitions and code to separate files (Glenn Willen)
81cd958 Factor BroadcastTransaction out of sendrawtransaction (Glenn Willen)
c734aaa Split DecodePSBT into Base64 and Raw versions (Glenn Willen)
162ffef Add pf_invalid arg to std::string DecodeBase{32,64} (Glenn Willen)

Pull request description:

  * Move most PSBT definitions into psbt.h.
  * Move most PSBT RPC utilities into psbt.{h,cpp}.
  * Move wallet-touching PSBT RPC utilities (FillPSBT) into
      wallet/psbtwallet.{h,cpp}.
  * Switch exceptions from JSONRPCError() to new PSBTException class.
  * Split DecodePSBT into DecodeBase64PSBT (old behavior) and DecodeRawPSBT.
  * Add one new version of DecodeBase64 utility in strencodings.h (and
      corresponding DecodeBase32 for completeness).
  * Factor BroadcastTransaction utility function out of sendrawtransaction RPC
      handler in rpc/rawtransaction.cpp

  Note: For those keeping score at home wondering why refactor, this is in anticipation of (and developed in parallel with) a change to actually introduce GUI use of all this stuff, which is already under development and working-ish.

Tree-SHA512: 2197c448e657421f430943025357597e7b06c4c377d5d4b2622b9edea52a7193c48843dd731abb3a88ac4023a9c88d211991e0a9b740c22f2e1cbe72adefe390

@gwillen gwillen deleted the gwillen:feature-refactor-psbt-rpcs branch Feb 15, 2019

MarcoFalke added a commit that referenced this pull request Feb 22, 2019

Merge #15408: Remove unused TransactionError constants
fa9b60c Remove unused TransactionError constants (MarcoFalke)

Pull request description:

  Fixup to #14978, which introduced a bunch of unused enum values, such as `UNKNOWN_ERROR`, `ERROR_COUNT` and `TRANSACTION_ERR_LAST`. None of those have a meaning in the context of an `enum class`, where the compiler can infer if all cases have been covered in a switch-case.

  Also, move the global `::maxTxFee` back to the rpc caller, so it can be set on a per call basis (in the future).

Tree-SHA512: 7f1e2d795f1c1278ecd54ddab2b92c2a862f3c637b482d1d008208925befa1c9dd4b3c4bb1bfcbc5ca4b66a41004aaf01ea96ea95236f944250b8a6cf99ff173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.