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

gwillen commented at 2:52 am on December 17, 2018: contributor

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.

fanquake added the label Refactoring on Dec 17, 2018

fanquake added the label Wallet on Dec 17, 2018

in src/core_read.cpp:193 in 3c91a3ddf5 outdated

191+    return DecodeRawPSBT(psbt, tx_data, error);
192+}
193+
194+bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
195+{
196+    CDataStream ss_data(tx_data.begin(), tx_data.end(), SER_NETWORK, PROTOCOL_VERSION);

gwillen commented at 6:00 am on December 17, 2018:

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 commented at 6:02 am on December 17, 2018: contributor

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 force-pushed on Dec 17, 2018

gwillen commented at 7:39 am on December 17, 2018: contributor

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

DrahtBot commented at 8:24 am on December 17, 2018: member

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.

in src/psbt.h:128 in 2fe27d6e4d outdated

130+
131+    template <typename Stream>
132+    inline void Unserialize(Stream& s) {
133+        // Read loop
134+        bool found_sep = false;
135+        while(!s.empty()) {

practicalswift commented at 11:57 am on December 17, 2018:

Space before (. Applies throughout this PR.

Please run all new files through clang-format :-)

gwillen commented at 5:01 pm on December 17, 2018:

This is moved code, none of this code is new – I expect I should not actually clang-format it, to preserve move-detection?

promag commented at 5:09 pm on December 17, 2018:

Agree.

practicalswift commented at 5:13 pm on December 17, 2018:

Makes sense!

promag commented at 2:36 pm on December 17, 2018: member

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 force-pushed on Dec 17, 2018

gwillen commented at 10:51 pm on December 17, 2018: contributor

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 commented at 3:02 am on December 18, 2018: member

@gwillen writes:

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

Is it possible to split this commit into one for each of the points here? It makes review harder when everything is squashed into a single commit.

gwillen commented at 4:15 am on December 18, 2018: contributor

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 commented at 4:16 am on December 18, 2018: contributor

@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 commented at 7:49 am on December 18, 2018: member

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

achow101 commented at 4:49 am on December 19, 2018: member

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

promag commented at 2:09 pm on December 19, 2018: member

I definitely agree in splitting it. @gwillen checked out the branch and looks promising.

gwillen force-pushed on Jan 9, 2019

gwillen commented at 11:13 am on January 9, 2019: contributor

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 force-pushed on Jan 9, 2019

jb55 commented at 4:48 pm on January 9, 2019: member

This looks much better now! Thanks!

utACK dd45d3febacb40cf68f58eff08f76db2854fc340

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.

in src/util/strencodings.cpp:191 in 65cb85e625 outdated

187@@ -188,9 +188,9 @@ std::vector<unsigned char> DecodeBase64(const char* p, bool* pfInvalid)
188     return ret;
189 }
190 
191-std::string DecodeBase64(const std::string& str)
192+std::string DecodeBase64(const std::string& str, bool* pfInvalid)

achow101 commented at 6:22 pm on January 9, 2019:

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

gwillen commented at 10:43 pm on January 9, 2019:

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

sipa commented at 0:11 am on January 17, 2019:

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.

Sjors commented at 5:45 pm on January 18, 2019:

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.

gwillen commented at 5:33 am on January 29, 2019:

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

Sjors commented at 3:33 pm on January 29, 2019:

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

in src/psbt.h:42 in 0edb99e681 outdated

35@@ -36,6 +36,14 @@ static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
36 // as a 0 length key which indicates that this is the separator. The separator has no value.
37 static constexpr uint8_t PSBT_SEPARATOR = 0x00;
38 
39+/** An exception in PSBT processing that represents a problem with the input data --
40+    when the input data is user-supplied, it is appropriate to catch() this and use
41+    e.what() to retrieve a human-readable error message. */
42+class PSBTException: public std::runtime_error {

achow101 commented at 6:54 pm on January 9, 2019:

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

gwillen commented at 10:44 pm on January 9, 2019:

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.

promag commented at 10:48 am on January 16, 2019:

Thread https://github.com/bitcoin/bitcoin/pull/14978/commits/617247cbc1622667a493772214fc8416066b3908#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 https://github.com/bitcoin/bitcoin/blob/391a27376b30492f55b398fc220ee96cf3769a32/src/wallet/rpcwallet.cpp#L4194-L4197

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.

sipa commented at 0:21 am on January 17, 2019:

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?

achow101 commented at 6:54 pm on January 9, 2019: member

utACK dd45d3febacb40cf68f58eff08f76db2854fc340

Some comments, nothing that block this IMO

gwillen commented at 10:45 pm on January 9, 2019: contributor

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 force-pushed on Jan 16, 2019

gwillen commented at 0:32 am on January 16, 2019: contributor

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

in src/core_read.cpp:185 in 801333b363 outdated

182-    std::vector<unsigned char> tx_data = DecodeBase64(base64_tx.c_str());
183-    CDataStream ss_data(tx_data, SER_NETWORK, PROTOCOL_VERSION);
184+    bool invalid;
185+    std::string tx_data = DecodeBase64(base64_tx, &invalid);
186+    if (invalid) {
187+        error = "invalid base64";

promag commented at 10:01 am on January 16, 2019:

Commit 801333b

Could add test for this new error?

gwillen commented at 7:12 am on January 29, 2019:

Done.

in src/core_read.cpp:193 in 801333b363 outdated

190+    return DecodeRawPSBT(psbt, tx_data, error);
191+}
192+
193+bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
194+{
195+    CDataStream ss_data(tx_data.data(), tx_data.data() + tx_data.size(), SER_NETWORK, PROTOCOL_VERSION);

promag commented at 10:03 am on January 16, 2019:

Commit 801333b

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

 0--- a/src/streams.h
 1+++ b/src/streams.h
 2@@ -236,17 +236,8 @@ public:
 3         Init(nTypeIn, nVersionIn);
 4     }
 5
 6-    CDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
 7-    {
 8-        Init(nTypeIn, nVersionIn);
 9-    }
10-
11-    CDataStream(const std::vector<char>& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
12-    {
13-        Init(nTypeIn, nVersionIn);
14-    }
15-
16-    CDataStream(const std::vector<unsigned char>& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
17+    template <typename T>
18+    CDataStream(const T& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end())
19     {
20         Init(nTypeIn, nVersionIn);
21     }

gwillen commented at 7:04 am on January 29, 2019:

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?

Sjors commented at 3:33 pm on January 29, 2019:

@achow101 thoughts?

gwillen commented at 5:41 am on January 30, 2019:

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…

in src/core_io.h:40 in 801333b363 outdated

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

promag commented at 10:09 am on January 16, 2019:

Commit 801333b

nit, would be nice see brief comments.

Sjors commented at 5:58 pm on January 18, 2019:

nit: base64_psbt (see next comment)

gwillen commented at 7:11 am on January 29, 2019:

Done.

in src/core_io.h:41 in 801333b363 outdated

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

promag commented at 10:10 am on January 16, 2019:

Commit 801333b

nit, s/tx_data/raw_tx?

Sjors commented at 5:56 pm on January 18, 2019:

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

gwillen commented at 7:11 am on January 29, 2019:

Done.

in src/rpc/rawtransaction.h:17 in 745c540d05 outdated

12@@ -13,6 +13,9 @@ namespace interfaces {
13 class Chain;
14 } // namespace interfaces
15 
16+/** Broadcast a transaction */
17+std::string BroadcastTransaction(CTransactionRef tx, bool allowhighfees = false);

promag commented at 10:16 am on January 16, 2019:

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.

sipa commented at 0:16 am on January 17, 2019:

Returning uint256 seems more natural indeed.

Sjors commented at 6:03 pm on January 18, 2019:

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.

gwillen commented at 7:05 am on January 29, 2019:

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.

Sjors commented at 3:35 pm on January 29, 2019:

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

gwillen commented at 6:28 am on January 30, 2019:

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.

in src/rpc/rawtransaction.cpp:1042 in 745c540d05 outdated

1038     CMutableTransaction mtx;
1039     if (!DecodeHexTx(mtx, request.params[0].get_str()))
1040         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
1041     CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
1042+
1043+    bool allowhighfees = !request.params[1].isNull() && request.params[1].get_bool();

promag commented at 10:20 am on January 16, 2019:

Commit 745c540

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

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

Because it is more clear the default value.

gwillen commented at 5:44 am on January 30, 2019:

Makes sense, done.

in src/psbt.h:401 in 08ef528c04 outdated

405-    }
406-    friend bool operator!=(const PartiallySignedTransaction& a, const PartiallySignedTransaction &b)
407-    {
408-        return !(a == b);
409+    // Checks if they refer to the same underlying CTransaction
410+    bool SameTx(const PartiallySignedTransaction& psbt) {

promag commented at 10:50 am on January 16, 2019:

Commit 08ef528

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

gwillen commented at 6:33 am on January 30, 2019:

Makes sense, done.

in src/psbt.h:390 in 08ef528c04 outdated

390@@ -391,20 +391,15 @@ struct PartiallySignedTransaction
391     std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
392 
393     bool IsNull() const;
394-    void Merge(const PartiallySignedTransaction& psbt);
395+    NODISCARD bool Merge(const PartiallySignedTransaction& psbt);

promag commented at 10:56 am on January 16, 2019:

Commit 08ef528

Add comment explaining the return value.

gwillen commented at 6:38 am on January 30, 2019:

Done.

in src/psbt.cpp:261 in c062d74e11 outdated

256+        ssTx << psbtx;
257+        result = EncodeBase64(ssTx.str());
258+    }
259+}
260+
261+PartiallySignedTransaction CombinePSBTs(std::vector<PartiallySignedTransaction> psbtxs) {

promag commented at 11:04 am on January 16, 2019:

Commit c062d74

Make the argument a const reference.

nit, brace on new line.

gwillen commented at 6:58 am on January 30, 2019:

Done.

promag commented at 11:08 am on January 16, 2019: member

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.

sipa commented at 0:23 am on January 17, 2019: member

Big concept ACK

Sjors commented at 5:35 pm on January 18, 2019: member

Concept ACK.

Funny enough, yesterday I ran into an issue where I needed to reuse sendrawtransaction. @sipa wrote:

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?

I’m also introducing an exception system for ExternalSigner in #14912:

0class ExternalSignerException : public std::runtime_error {
1 public:
2     using std::runtime_error::runtime_error;
3 };
4
5throw ExternalSignerException(strprintf("Some error with detail '%s'", details));

I then map that to an RPC error like so:

0    try {
1        ExternalSigner::DoSomething(...);
2    } catch (const ExternalSignerException& e) {
3        throw JSONRPCError(RPC_WALLET_ERROR, e.what());
4    }

It’s probably a good idea to agree on a single good way to do this.

in src/rpc/rawtransaction.cpp:1044 in 745c540d05 outdated

1029@@ -1030,19 +1030,24 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
1030             + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
1031         );
1032 
1033-    std::promise<void> promise;

Sjors commented at 6:01 pm on January 18, 2019:

Currently in 745c540d05f4a21810eccc5e9450e9b321560560, wrong commit?

gwillen commented at 6:16 am on January 29, 2019:

I think it’s the right commit? It moves into BroadcastTransaction.

Sjors commented at 1:37 pm on January 30, 2019:

You’re right, looks good.

Sjors commented at 6:13 pm on January 18, 2019: member

Reviewed c062d74e1187e481960ca6868a46642b8bbfbd2e, mostly happy.

DrahtBot added the label Needs rebase on Jan 21, 2019

in src/psbt.h:556 in c062d74e11 outdated

551+bool PSBTInputSigned(PSBTInput& input);
552+
553+/** Signs a PSBTInput, verifying that all provided data matches what is being signed. */
554+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash = SIGHASH_ALL);
555+
556+/** Finalizes a PSBT if possible, combining partial signatures and optionally extracting to a signed transaction ready for sending. */

Sjors commented at 1:07 pm on January 24, 2019:

Can you add parameter info here? Try:

0/**
1 * Finalizes a PSBT if possible, combining partial signatures and optionally
2 * extracting to a signed transaction ready for sending.
3 *
4 * [@param](/bitcoin-bitcoin/contributor/param/)[in] &psbtx  reference to PartiallySignedTransaction
5 * [@param](/bitcoin-bitcoin/contributor/param/)[in] extract whether to extract the transaction as a hex string
6 * [@param](/bitcoin-bitcoin/contributor/param/)[in] &result PSBT as base64 or transaction as hex string
7 * [@param](/bitcoin-bitcoin/contributor/param/)[in] &complete  
8 */

Also, in some other places in the code I see a convention of starting with output parameters and moving optional stuff to the end. So in that case maybe switch complete with extract.

Finally I think extract_tx would be more clear. Would also rather leave result empty if extract is false. In that case std::string& result can be renamed to tx_hex.

Though maybe that’s too much refactoring for this PR.

gwillen commented at 7:50 am on January 30, 2019:

Though maybe that’s too much refactoring for this PR.

No, it’s too late, you’ve got me thinking about it now. The “finalize and maybe extract” interface is kind of weird as it stands, and the string-handling stuff belongs in the rpc handler, not in the general-purpose functions. I clearly drew the abstraction boundaries in the wrong place.

I’ve reworked this in a way that feels a bit better to me. It’s not perfect but it no longer obviously feels to me like I can improve it.

in src/rpc/rawtransaction.cpp:1046 in 745c540d05 outdated

1042+
1043+    bool allowhighfees = !request.params[1].isNull() && request.params[1].get_bool();
1044+    return BroadcastTransaction(tx, allowhighfees);
1045+}
1046+
1047+std::string BroadcastTransaction(CTransactionRef tx, bool allowhighfees) {

Sjors commented at 1:46 pm on January 24, 2019:

nit: const params

gwillen commented at 5:47 am on January 30, 2019:

Can you clarify? These are both by-value, so I wouldn’t ordinarily bother marking them const. Am I supposed to?

sipa commented at 5:56 am on January 30, 2019:

There is little point in doing so, as constness of by-value arguments doesn’t even affect the function signature.

You can if you really want to make sure the local variables associated with them don’t change during the function execution, but that’s unusual in our codebase, and not very important.

Sjors commented at 2:24 pm on January 24, 2019: member

Lightly tested c062d74 using #14912, seems to work well.

Needs trivial rebase due because #14906 changed PartiallySignedTransaction::PartiallySignedTransaction(const CTransaction& tx) : tx(tx) to CMutableTransaction in src/script/sign.{h/cpp}

gwillen force-pushed on Jan 29, 2019

DrahtBot removed the label Needs rebase on Jan 29, 2019

gwillen commented at 6:18 am on January 29, 2019: contributor

@promag @sipa @Sjors Can you have an argument, here or elsewhere, about how error handling should work in cases like this, and then tell me what to do? :-) I am ambivalent between “better exceptions” and “switch to return values”. It seems like Sjors and maybe promag favor door number 1, whereas sipa favors door number 2 for reasons I find entirely defensible, but the result is a little harder to work with, though perhaps less hazardous.

gwillen force-pushed on Jan 29, 2019

promag commented at 4:55 pm on January 29, 2019: member

@gwillen I’d avoid using exceptions to control flow. For instance, you have:

0bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false)

And looks like it returns false if it fails, but that’s wrong. In fact, it throws if some error occurs and returns whether the transaction has a complete set of signatures.

Looking the existing calls you see the semantic difference: https://github.com/bitcoin/bitcoin/blob/7275365c9bc7e7ebd6bbf7dcb251946aac44b5de/src/wallet/rpcwallet.cpp#L4035 https://github.com/bitcoin/bitcoin/blob/7275365c9bc7e7ebd6bbf7dcb251946aac44b5de/src/wallet/rpcwallet.cpp#L4149

in src/core_io.h:43 in ed465d8f9b outdated

36@@ -37,7 +37,11 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
37  */
38 bool ParseHashStr(const std::string& strHex, uint256& result);
39 std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
40-NODISCARD bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);
41+
42+// Decode a base64ed PSBT into a PartiallySignedTransaction
43+NODISCARD bool DecodeBase64PSBT(PartiallySignedTransaction& decoded_psbt, const std::string& base64_psbt, std::string& error);
44+// Decode a raw (binary blob) PSBT into a PartiallySignedTransaction

Empact commented at 9:40 pm on January 29, 2019:

nit: use //! or equivalent for doxygen

gwillen commented at 5:32 am on January 30, 2019:

Aha, right. Done.

sipa commented at 5:56 am on January 30, 2019: member

@gwillen My preference is an enum over exceptions (but both are used in the codebase, so if others feel strongly, feel free to ignore me).

gwillen force-pushed on Jan 30, 2019

gwillen commented at 7:57 am on January 30, 2019: contributor

I think I’ve addressed everything except the error-handling / exceptions stuff, which I’m still thinking about and will punt to tomorrow.

I significantly reworked the FinalizePSBT interface, so it deserves another look. Everything else was relatively minor.

Sjors commented at 5:07 pm on January 30, 2019: member

utACK f124a51 modulo the broken bench build on Travis (can’t reproduce that on macOS)

Enum error handling works for me too.

achow101 commented at 8:28 pm on January 30, 2019: member

I agree that using an enum would be better. We already use enums for other errors like script_error, etc. So I think it would make sense to continue that pattern here as well.

gwillen force-pushed on Jan 31, 2019

gwillen commented at 3:44 am on January 31, 2019: contributor

Ok, I ended up doing something that feels a little gross and unorthodox for the error handling.

The problems with a new enum are several-fold: Most notably, it would be at least two new enums, since BroadcastTransaction and the various PSBT functions are not really of the same sort, unless we want something like a general-purpose TransactionError type (which I wouldn’t be totally against.) The ONLY consumer of error codes in any form (as far as I can tell) would be RPC callers, which means the only point of making a bunch of new error constants is to immediately turn around and define a mapping of them to RPC error constants.

(ScriptErrors get handled a little differently, as far as I can tell – they just get converted to strings when they are stuffed into RPC responses, and the numeric codes discarded. As far as I can tell, the only other use for them is in tests. If we were going to go that route here, I would just advocate for pure string errors, since the constants wouldn’t have any interesting producers or consumers at that point.)

Anyway, on the theory that the only consumer of error codes is the RPC subsystem, my approach is this: I retained the use of RPCErrorCode and JSONRPCError. Instead of throwing the resulting UniValue objects, I am using out parameters for them. Then when I am back in the RPC codepath, I take the object and throw it. In the GUI codepath, I expect to extract the error message from it to display to the user, and ignore the code.

This is definitely a little bit crazy, but I was having trouble coming up with any better ideas. Thoughts?

gwillen commented at 4:01 am on January 31, 2019: contributor

Oh, and I think the Travis failure is real – I think the problem is that actually BroadcastTransaction does NOT belong under wallet/ and needs to be linked even when the wallet is disabled. So right now disabling the wallet causes the link to fail.

Suggestions for where to move it? Right now it’s a pair of new files named wallet/transaction.{h,cpp}.

Sjors commented at 9:37 am on January 31, 2019: member

The ONLY consumer of error codes in any form (as far as I can tell) would be RPC callers,

In #14912 I plan to move more code into the ExternalSigner class, which will be used by both RPC and the GUI.

general-purpose TransactionError type

Works for me

I retained the use of RPCErrorCode and JSONRPCError. Instead of throwing the resulting UniValue objects, I am using out parameters for them. Then when I am back in the RPC codepath, I take the object and throw it

I like your previous approach better. I’m also not a fan of using UniValue outside of RPC input & output processing.

that actually BroadcastTransaction does NOT belong under wallet/

Indeed it doesn’t. Nodes without a wallet can also broadcast transactions and do various things with PSBT. Maybe src/node/transaction.{h,c}? @ryanofsky what makes sense given #10973? I’m assuming it doesn’t really belong in src/interfaces?

in src/wallet/psbtwallet.cpp:5 in eb1c459f40 outdated

1@@ -2,14 +2,14 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#include <rpc/protocol.h>
6+#include <rpc/protocol.h> // For JSONRPCError and RPCErrorCode

Sjors commented at 9:41 am on January 31, 2019:

Yuck pseudo-circular dependency? :-)

ryanofsky commented at 2:56 pm on January 31, 2019: member

that actually BroadcastTransaction does NOT belong under wallet/

Indeed it doesn’t. Nodes without a wallet can also broadcast transactions and do various things with PSBT. Maybe src/node/transaction.{h,c}? @ryanofsky what makes sense given #10973? I’m assuming it doesn’t really belong in src/interfaces?

It would be reasonable to make BroadcastTransaction an interfaces::Chain method. (Chain is the object a wallet process uses to communicate with a node process in #10102), and I’ve actually kind of done this already in 9eee26ef6071451ee426f71beb6b77184bc8b073 and 02022184aa98ad27ba4338dd0f7a8e4d20180818 from #15288. It would also be reasonable to move BroadcastTransaction somewhere else if it can’t stay in wallet because it will be used by non-wallet code. Whatever you decide should be fine.

gwillen commented at 1:03 am on February 1, 2019: contributor

@ryanofsky The reason I need something like BroadcastTransaction is that, currently, none of the interfaces:: stuff exports a way to broadcast a transaction that didn’t come from the wallet (and I need the RPC-like ability to do it with a blob I was handed.) So probably these codepaths should eventually merge! :-)

Sjors commented at 12:56 pm on February 1, 2019: member

By the way wallet/transaction.{h,cpp} is still useful, even if not for this PR. E.g. I have a need to move FundTransaction(CWallet* const pwallet, CMutableTransaction& tx... out of rpcwallet.{h,cpp}

I would probably also wrap that inside a FundPSBT method in wallet/psbtwallet.{h,cpp}, so that walletcreatefundedpsbt doesn’t have to deal directly with CMutableTransaction.

gwillen commented at 10:48 pm on February 4, 2019: contributor

@ryanofsky Now that I’m actually trying to do it – why interfaces::Chain and not interfaces::Node? That seems like a better fit. (Especially since the GUI ClientModel object has a Node interface handy, but I don’t see where the GUI easily gets access to a Chain, although I’m not totally sure where to look.)

(Noting that specifically I need to do this in a non-wallet context, for a transaction that’s not necessarily in or from the wallet.)

ryanofsky commented at 11:28 pm on February 4, 2019: member

Why interfaces::Chain and not interfaces::Node? That seems like a better fit.

You’re right. I didn’t look at this PR closely enough and assumed that it was calling BroadcastTransaction from the wallet. In this case, a Chain method would have to be added at some point (like in #15288), because the Chain interface is the only way a separate wallet process can access the node process. After you had a Chain method, you could also allow the GUI to be able to broadcast transactions by adding aNode method which forwarded to m_interfaces.chain->broadcastTransaction(...).

But actually looking at this PR, BroadcastTransaction isn’t called from either the wallet or the gui, only from the sendrawtransaction RPC. So I think it makes most sense for BroadcastTransaction to remain a standalone function so it can be conveniently called from sendrawtransaction.

gwillen commented at 0:10 am on February 5, 2019: contributor

Oh, good point. So I’m going ahead with creating src/node for it to live in, and then later I will forward from interfaces::Node (when I actually add a GUI use of it.)

gwillen commented at 10:42 pm on February 6, 2019: contributor

Ok, I think I have fixed the issue with BroadcastTransaction linking, by moving it out of the wallet stuff.

I have made another attempt at the error stuff. :-) This time we have a TransactionError enum, and a translation table from TransactionError to RPCErrorCode, as well as strings for each TransactionError.

The result is almost faithful, for RPC clients, to how it was before, except:

I have reclassified syntactically-valid but semantically-invalid PSBTs, from RPC_DESERIALIZATION_ERROR to RPC_INVALID_PARAMETER, in one case (we were being inconsistent)
When AcceptToMemoryPool fails, we can extract a string from the failure using FormatStateMessage. But we no longer have an easy way to pass that message back to the RPC caller, so right now we don’t. Probably I should add another out param to fix this or something. Yes? No?

I think we are almost at the finish line. :-)

gwillen force-pushed on Feb 6, 2019

gwillen force-pushed on Feb 7, 2019

gwillen commented at 3:27 am on February 7, 2019: contributor

In fact, it turns out that changing the error message wording of BroadcastTransaction at all breaks a bunch of functional tests. Working on it.

EDIT: Yeah the tests definitely require the failure strings from FormatStateMessage. So I’m going to think about how to plumb those through.

gwillen force-pushed on Feb 7, 2019

gwillen commented at 11:57 am on February 7, 2019: contributor

There we go! Ready for another look.

in src/core_io.h:44 in 6a1570ad97 outdated

36@@ -37,7 +37,11 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
37  */
38 bool ParseHashStr(const std::string& strHex, uint256& result);
39 std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
40-NODISCARD bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error);
41+
42+//! Decode a base64ed PSBT into a PartiallySignedTransaction
43+NODISCARD bool DecodeBase64PSBT(PartiallySignedTransaction& decoded_psbt, const std::string& base64_psbt, std::string& error);
44+//! Decode a raw (binary blob) PSBT into a PartiallySignedTransaction
45+NODISCARD bool DecodeRawPSBT(PartiallySignedTransaction& decoded_psbt, const std::string& raw_psbt, std::string& error);

sipa commented at 3:40 am on February 8, 2019:

It seems a bit strange that raw_psbt is a string and not a std::vector<unsigned char>. The DecodeBase64 call below also returns a std::vector<unsigned char>, so changing this would avoid a conversion (which I’m a bit surprised happens automatically).

gwillen commented at 7:23 am on February 8, 2019:

In fact, DecodeBase64 does not return a vector<uchar>, it does return a std::string when given a std::string, which I imagine is why I ended up doing it this way, to avoid having to convert it. (When given a char*, it returns a vector<uchar>. Your guess why it was done this way is as good as mine.)

I did carefully check that std::string is just a binary-safe as vector<uchar>. I’m fine leaving it or changing it as you prefer.

sipa commented at 0:45 am on February 9, 2019:

Oh, you’re right. That’s weird, but probably out of scope for this PR. I’m fine with leaving this as is.

in src/rpc/util.cpp:142 in acac678ebe outdated

138@@ -139,6 +139,8 @@ RPCErrorCode RPCErrorFromTransactionError(TransactionError terr)
139         case TRANSACTION_ERR_P2P_DISABLED:
140             return RPC_CLIENT_P2P_DISABLED;
141         case TRANSACTION_ERR_INVALID_PSBT:
142+        case TRANSACTION_ERR_PSBT_MISMATCH:

sipa commented at 3:48 am on February 8, 2019:

This changes the error code for TRANSACTION_ERR_INVALID_PSBT from RPC_DESERIALIZATION_ERROR to RPC_INVALID_PARAMETER. Is that intentional?

gwillen commented at 6:28 am on February 8, 2019:

Yes, because there are two cases where we return an RPC error for a PSBT being semantically invalid, and we were returning DESERIALIZATION_ERROR in one case (when the invalid PSBT was given as a parameter) and INVALID_PARAMETER in the other (when the invalid PSBT was the result of a merge), and the I though the latter seemed more reasonable. My sense was that DESERIALIZATION_ERROR is only meant for syntactically invalid arguments? I’d be willing to change it back though. It would be easier if the two cases were treated the same.

sipa commented at 7:03 am on February 8, 2019:

I don’t have any strong opinion on the RPC codes. I just wanted to make sure this was intentional (as it could easily have been a copypaste error or so).

sipa commented at 3:48 am on February 8, 2019: member

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

in src/node/transaction.h:11 in acac678ebe outdated

 6+#define BITCOIN_NODE_TRANSACTION_H
 7+
 8+#include <primitives/transaction.h>
 9+#include <uint256.h>
10+
11+typedef enum TransactionError_t {

sipa commented at 1:08 am on February 9, 2019:

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

gwillen commented at 3:45 am on February 9, 2019:

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

in src/psbt.h:556 in acac678ebe outdated

551+ * return True if the PSBT is now complete, false otherwise
552+ */
553+bool FinalizePSBT(PartiallySignedTransaction& psbtx);
554+
555+/**
556+ * Finalizes a PSBT if possible, and extracts it to a CTransaction if it could be finalized.

sipa commented at 1:10 am on February 9, 2019:

Nit: CMutableTransaction.

gwillen commented at 3:46 am on February 9, 2019:

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

sipa commented at 1:11 am on February 9, 2019: member

utACK acac678ebe6e105e0a775b6eff37b1583139bf21, only nits.

gwillen force-pushed on Feb 9, 2019

in src/node/transaction.h:11 in 9f2297d4de outdated

 6+#define BITCOIN_NODE_TRANSACTION_H
 7+
 8+#include <primitives/transaction.h>
 9+#include <uint256.h>
10+
11+enum TransactionError {

practicalswift commented at 1:05 pm on February 9, 2019:

Should be enum class?

gwillen commented at 4:18 am on February 10, 2019:

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.

gwillen commented at 5:10 am on February 10, 2019:

Updated.

gwillen force-pushed on Feb 10, 2019

Sjors commented at 3:52 pm on February 10, 2019: member

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

DrahtBot added the label Needs rebase on Feb 11, 2019

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.

162ffefd2f

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.

c734aaa15d

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

81cd958848

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

c6c3d42a7d

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.

bd0dbe8763

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

78b9893d02

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.

102faad81e

gwillen force-pushed on Feb 11, 2019

gwillen commented at 10:23 pm on February 11, 2019: contributor

Rebased.

DrahtBot removed the label Needs rebase on Feb 11, 2019

gwillen commented at 2:32 am on February 14, 2019: contributor

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 commented at 3:28 am on February 14, 2019: member

utACK 102faad81efa1cb12c29c466cfe81fc8c7351e1d

meshcollider commented at 8:47 am on February 14, 2019: contributor

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

meshcollider merged this on Feb 14, 2019

meshcollider closed this on Feb 14, 2019

meshcollider referenced this in commit 2452c6cc0a on Feb 14, 2019

gwillen deleted the branch on Feb 15, 2019

MarcoFalke referenced this in commit 169dced9a4 on Feb 22, 2019

MarcoFalke referenced this in commit be0e8b4bff on Aug 2, 2019

deadalnix referenced this in commit 6ed34f9cc7 on Apr 15, 2020

deadalnix referenced this in commit 2f3cc194c8 on Apr 15, 2020

deadalnix referenced this in commit 8e37f88fcb on Apr 16, 2020

deadalnix referenced this in commit 8eef3ca0f3 on Apr 16, 2020

deadalnix referenced this in commit 5918973356 on Apr 16, 2020

deadalnix referenced this in commit 0a01c8cf9d on Apr 16, 2020

deadalnix referenced this in commit 082a077015 on Apr 16, 2020

ftrader referenced this in commit a3a5c178a4 on Aug 17, 2020

ftrader referenced this in commit 53d01afd00 on Aug 17, 2020

ftrader referenced this in commit ca3223e448 on Aug 17, 2020

ShengguangXiao referenced this in commit 517bfc1ac7 on Aug 28, 2020

kittywhiskers referenced this in commit a136254822 on May 19, 2021

kittywhiskers referenced this in commit a20dace3e2 on May 19, 2021

kittywhiskers referenced this in commit e1a48ebcc7 on May 20, 2021

kittywhiskers referenced this in commit 2d3804e68e on May 20, 2021

kittywhiskers referenced this in commit 4476a7e533 on May 20, 2021

kittywhiskers referenced this in commit e3443643f3 on May 20, 2021

PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021

kittywhiskers referenced this in commit 11045e8bc5 on Jul 2, 2021

kittywhiskers referenced this in commit 00b472a2b9 on Jul 4, 2021

kittywhiskers referenced this in commit 90e298004e on Jul 9, 2021

kittywhiskers referenced this in commit 21158a817c on Jul 9, 2021

kittywhiskers referenced this in commit d2e2a21b82 on Jul 15, 2021

kittywhiskers referenced this in commit b88b35790d on Jul 16, 2021

kittywhiskers referenced this in commit eb2bc0887a on Aug 1, 2021

kittywhiskers referenced this in commit a1f386c7ca on Aug 5, 2021

kittywhiskers referenced this in commit 08b203338e on Aug 5, 2021

kittywhiskers referenced this in commit b7185f5720 on Aug 5, 2021

kittywhiskers referenced this in commit 600c9f891b on Aug 5, 2021

kittywhiskers referenced this in commit 784992232f on Aug 5, 2021

kittywhiskers referenced this in commit 4a174359e9 on Aug 5, 2021

kittywhiskers referenced this in commit bf30d9ade7 on Aug 5, 2021

kittywhiskers referenced this in commit 522934703a on Aug 9, 2021

UdjinM6 referenced this in commit 7aebf156e9 on Aug 10, 2021

5tefan referenced this in commit 1d9b4be020 on Aug 12, 2021

5tefan referenced this in commit a2531f3500 on Aug 12, 2021

Munkybooty referenced this in commit 010f5e8a47 on Sep 7, 2021

dzutto referenced this in commit 4a317e2a93 on Sep 24, 2021

dzutto referenced this in commit 5c8485e1bf on Sep 24, 2021

dzutto referenced this in commit 9d84be1dd4 on Sep 24, 2021

dzutto referenced this in commit 46b7464d15 on Sep 27, 2021

dzutto referenced this in commit a9ce72a8bb on Sep 30, 2021

kittywhiskers referenced this in commit 963ceb0d92 on Oct 12, 2021

MarcoFalke locked this on Dec 16, 2021

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

Conflicts