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.

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

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

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.

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.

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

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

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

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.

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

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.

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?

Commit 801333b

Could add test for this new error?

Done.

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

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?

@achow101 thoughts?

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

Commit 801333b

nit, would be nice see brief comments.

nit: base64_psbt (see next comment)

Done.

Commit 801333b

nit, s/tx_data/raw_tx?

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

Done.

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.

Returning uint256 seems more natural indeed.

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.

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.

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

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.

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.

Makes sense, done.

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

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.

Commit 08ef528

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

Makes sense, done.

Commit 08ef528

Add comment explaining the return value.

Done.

Commit c062d74

Make the argument a const reference.

nit, brace on new line.

Done.

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.

Returning uint256 seems more natural indeed.

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?