From fc841c8047c21443e5ff1f2de8e8e75637b873b0 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Thu, 6 Feb 2020 23:08:56 -0800 Subject: [PATCH] Fix claimpegin and createrawpegin support for multiwallet. Fix multiple issues with claimpegin and createrawpegin support for multiple loaded wallets: - Failure to call EnsureWalletIsAvailable would cause a crash when multiple wallets were loaded, but one was not specified in the RPC call. - Failure to propagate the request URL from claimpegin when making an internal call to another RPC would result in failure when multiple wallets were loaded, by failing to specify one for that call. Add a regression test to the feature_fedpeg.py test: at a critical point, create a second wallet on the sidechaind, and set up the RPC client to use the first one, to check that it still works correctly. --- src/wallet/rpcwallet.cpp | 34 +++++++++++++++++++++++-------- test/functional/feature_fedpeg.py | 8 ++++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b2485651b5..880d339cd3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5554,6 +5554,12 @@ extern UniValue sendrawtransaction(const JSONRPCRequest& request); template static UniValue createrawpegin(const JSONRPCRequest& request, T_tx_ref& txBTCRef, T_tx& tx_aux, T_merkle_block& merkleBlock) { + std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + CWallet* const pwallet = wallet.get(); + + if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) + return NullUniValue; + if (request.fHelp || request.params.size() < 2 || request.params.size() > 3) throw std::runtime_error( RPCHelpMan{"createrawpegin", @@ -5577,9 +5583,6 @@ static UniValue createrawpegin(const JSONRPCRequest& request, T_tx_ref& txBTCRef }, }.ToString()); - std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); - CWallet* const pwallet = wallet.get(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -5695,6 +5698,11 @@ UniValue createrawpegin(const JSONRPCRequest& request) UniValue claimpegin(const JSONRPCRequest& request) { + std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + CWallet* const pwallet = wallet.get(); + + if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) + return NullUniValue; if (request.fHelp || request.params.size() < 2 || request.params.size() > 3) throw std::runtime_error( @@ -5716,8 +5724,6 @@ UniValue claimpegin(const JSONRPCRequest& request) }, }.ToString()); - std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); - CWallet* const pwallet = wallet.get(); CTransactionRef tx_ref; CMutableTransaction mtx; @@ -5728,8 +5734,17 @@ UniValue claimpegin(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Peg-ins cannot be completed during initial sync or reindexing."); } + // NOTE: Making an RPC from within another RPC is not generally a good idea. In particular, it + // is necessary to copy the URI, which contains the wallet if one was given; otherwise + // multi-wallet support will silently break. The resulting request object is still missing a + // bunch of other fields, although they are usually not used by RPC handlers. This is a + // brittle hack, and further examples of this pattern should not be introduced. + // Get raw peg-in transaction - UniValue ret(createrawpegin(request)); + JSONRPCRequest req; + req.URI = request.URI; + req.params = request.params; + UniValue ret(createrawpegin(req)); // See the note above, on why this is a bad idea. // Make sure it can be propagated and confirmed if (!ret["mature"].isNull() && ret["mature"].get_bool() == false) { @@ -5737,11 +5752,12 @@ UniValue claimpegin(const JSONRPCRequest& request) } // Sign it - JSONRPCRequest request2; + JSONRPCRequest req2; + req2.URI = request.URI; UniValue varr(UniValue::VARR); varr.push_back(ret["hex"]); - request2.params = varr; - UniValue result = signrawtransactionwithwallet(request2); + req2.params = varr; + UniValue result = signrawtransactionwithwallet(req2); // See the note above, on why this is a bad idea. if (!DecodeHexTx(mtx, result["hex"].get_str(), false, true)) { throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); diff --git a/test/functional/feature_fedpeg.py b/test/functional/feature_fedpeg.py index d73fb9218f..79a5d176a1 100755 --- a/test/functional/feature_fedpeg.py +++ b/test/functional/feature_fedpeg.py @@ -229,6 +229,14 @@ def run_test(self): raw = parent.gettransaction(txid1)["hex"] + # Create a wallet in order to test that multi-wallet support works correctly for claimpegin + # (Regression test for https://github.com/ElementsProject/elements/issues/812 .) + sidechain.createwallet("throwaway") + # Set up our sidechain RPCs to use the first wallet (with empty name). We do this by + # overriding the RPC object in a hacky way, to avoid breaking a different hack on TestNode + # that enables generate() to work despite the deprecation of the generate RPC. + sidechain.rpc = sidechain.get_wallet_rpc("") + print("Attempting peg-ins") # First attempt fails the consensus check but gives useful result try: