Merge #813: Fix claimpegin and createrawpegin support for multiwallet.

fc841c804 Fix claimpegin and createrawpegin support for multiwallet. (Glenn Willen)

Pull request description:

  Fixes #812. @stevenroose, take a look when you get back?

  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.

Tree-SHA512: 38d28319d0bc131f530a03e2477b013a982b28d321383a2316372d9aa94f66b19137ba4c1c7e1fe44e3470fe87e80fabbd435836892925b50134c05613581e10

src/wallet/rpcwallet.cpp

@@ -5554,6 +5554,12 @@ extern UniValue sendrawtransaction(const JSONRPCRequest& request);
 template<typename T_tx_ref, typename T_tx, typename T_merkle_block>
 static UniValue createrawpegin(const JSONRPCRequest& request, T_tx_ref& txBTCRef, T_tx& tx_aux, T_merkle_block& merkleBlock)
 {
+    std::shared_ptr<CWallet> 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<CWallet> 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<CWallet> 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<CWallet> 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");

test/functional/feature_fedpeg.py

@@ -229,6 +229,14 @@ class FedPegTest(BitcoinTestFramework):
 
         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: