Merge #14468: [wallet] Deprecate generate RPC method

ab9aca2bdf [rpc] add 'getnewaddress' hint to 'generatetoaddress' help text. (John Newbery)
c9f02955b2 [wallet] Deprecate the generate RPC method (John Newbery)
aab81720de [tests] Add generate method to TestNode (John Newbery)
c269209336 [tests] Small fixups before deprecating generate (John Newbery)

Pull request description:

  Deprecates the `generate` RPC method.

  For concept discussion, see #14299.

  Fixes #14299.

Tree-SHA512: 16a3b8b742932e4f0476c06b23de07a34d9d215b41d9272c1c9d1e39966b0c2406f17c5ab3cc568947620c08171ebe5eb74fd7ed4b62151363e305ee2937cc80

9 files changed, 68 insertions, 17 deletions

diff --git a/doc/release-notes-14468.md b/doc/release-notes-14468.md
new file mode 100644
index 0000000000..fb0243aba8
--- /dev/null
+++ b/doc/release-notes-14468.md
@@ -0,0 +1,15 @@
+Wallet `generate` RPC method deprecated
+---------------------------------------
+
+The wallet's `generate` RPC method has been deprecated and will be fully
+removed in v0.19.
+
+`generate` is only used for testing. The RPC call reaches across multiple
+subsystems (wallet and mining), so is deprecated to simplify the wallet-node
+interface. Projects that are using `generate` for testing purposes should
+transition to using the `generatetoaddress` call, which does not require or use
+the wallet component. Calling `generatetoaddress` with an address returned by
+`getnewaddress` gives the same functionality as the old `generate` method.
+
+To continue using `generate` in v0.18, restart bitcoind with the
+`-deprecatedrpc=generate` configuration.
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index c565b9b4f9..25cfc69da9 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -167,6 +167,8 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
             "\nExamples:\n"
             "\nGenerate 11 blocks to myaddress\n"
             + HelpExampleCli("generatetoaddress", "11 \"myaddress\"")
+            + "If you are running the bitcoin core wallet, you can get a new address to send the newly generated bitcoin to with:\n"
+            + HelpExampleCli("getnewaddress", "")
         );
 
     int nGenerate = request.params[0].get_int();
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index a3f5f8b211..ea5177b086 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3321,6 +3321,12 @@ UniValue generate(const JSONRPCRequest& request)
         );
     }
 
+    if (!IsDeprecatedRPCEnabled("generate")) {
+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "The wallet generate rpc method is deprecated and will be fully removed in v0.19. "
+            "To use generate in v0.18, restart bitcoind with -deprecatedrpc=generate.\n"
+            "Clients should transition to using the node rpc method generatetoaddress\n");
+    }
+
     int num_generate = request.params[0].get_int();
     uint64_t max_tries = 1000000;
     if (!request.params[1].isNull()) {
diff --git a/test/functional/rpc_deprecated.py b/test/functional/rpc_deprecated.py
index 58074803cc..588bfbe083 100755
--- a/test/functional/rpc_deprecated.py
+++ b/test/functional/rpc_deprecated.py
@@ -4,12 +4,17 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_raises_rpc_error
 
 class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.setup_clean_chain = True
-        self.extra_args = [[], ["-deprecatedrpc=validateaddress"]]
+        self.extra_args = [[], ["-deprecatedrpc=generate"]]
+
+    def skip_test_if_missing_module(self):
+        # The generate RPC method requires the wallet to be compiled
+        self.skip_if_no_wallet()
 
     def run_test(self):
         # This test should be used to verify correct behaviour of deprecated
@@ -18,7 +23,10 @@ class DeprecatedRpcTest(BitcoinTestFramework):
         # self.log.info("Make sure that -deprecatedrpc=createmultisig allows it to take addresses")
         # assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, [self.nodes[0].getnewaddress()])
         # self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
-        pass
+
+        self.log.info("Test generate RPC")
+        assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].rpc.generate, 1)
+        self.nodes[1].generate(1)
 
 if __name__ == '__main__':
     DeprecatedRpcTest().main()
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index c05988c661..cc1bfabbfa 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -197,6 +197,25 @@ class TestNode():
             time.sleep(1.0 / poll_per_s)
         self._raise_assertion_error("Unable to connect to bitcoind")
 
+    def generate(self, nblocks, maxtries=1000000):
+        self.log.debug("TestNode.generate() dispatches `generate` call to `generatetoaddress`")
+        # Try to import the node's deterministic private key. This is a no-op if the private key
+        # has already been imported.
+        try:
+            self.rpc.importprivkey(privkey=self.get_deterministic_priv_key().key, label='coinbase', rescan=False)
+        except JSONRPCException as e:
+            # This may fail if:
+            # - wallet is disabled ('Method not found')
+            # - there are multiple wallets to import to ('Wallet file not specified')
+            # - wallet is locked ('Error: Please enter the wallet passphrase with walletpassphrase first')
+            # Just ignore those errors. We can make this tidier by importing the privkey during TestFramework.setup_nodes
+            # TODO: tidy up deterministic privkey import.
+            assert str(e).startswith('Method not found') or \
+                str(e).startswith('Wallet file not specified') or \
+                str(e).startswith('Error: Please enter the wallet passphrase with walletpassphrase first')
+
+        return self.generatetoaddress(nblocks=nblocks, address=self.get_deterministic_priv_key().address, maxtries=maxtries)
+
     def get_wallet_rpc(self, wallet_name):
         if self.use_cli:
             return self.cli("-rpcwallet={}".format(wallet_name))
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 8bbff7f7ef..4de3356d79 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -11,6 +11,7 @@ from test_framework.util import (
     assert_array_result,
     assert_equal,
     assert_fee_amount,
+    assert_greater_than,
     assert_raises_rpc_error,
     connect_nodes_bi,
     sync_blocks,
@@ -92,13 +93,13 @@ class WalletTest(BitcoinTestFramework):
         assert_equal(txout['value'], 50)
 
         # Send 21 BTC from 0 to 2 using sendtoaddress call.
-        # Locked memory should use at least 32 bytes to sign each transaction
+        # Locked memory should increase to sign transactions
         self.log.info("test getmemoryinfo")
         memory_before = self.nodes[0].getmemoryinfo()
         self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11)
         mempool_txid = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10)
         memory_after = self.nodes[0].getmemoryinfo()
-        assert(memory_before['locked']['used'] + 64 <= memory_after['locked']['used'])
+        assert_greater_than(memory_after['locked']['used'], memory_before['locked']['used'])
 
         self.log.info("test gettxout (second part)")
         # utxo spent in mempool should be visible if you exclude mempool
diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py
index 51afa0cb1a..ceb9709712 100755
--- a/test/functional/wallet_keypool.py
+++ b/test/functional/wallet_keypool.py
@@ -73,11 +73,10 @@ class KeyPoolTest(BitcoinTestFramework):
         time.sleep(1.1)
         assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
 
-        # drain them by mining
-        nodes[0].generate(1)
-        nodes[0].generate(1)
-        nodes[0].generate(1)
-        assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].generate, 1)
+        # drain the keypool
+        for _ in range(3):
+            nodes[0].getnewaddress()
+        assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getnewaddress)
 
         nodes[0].walletpassphrase('test', 100)
         nodes[0].keypoolrefill(100)
diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py
index 8d7c77bb96..b71dae9f40 100755
--- a/test/functional/wallet_labels.py
+++ b/test/functional/wallet_labels.py
@@ -29,8 +29,8 @@ class WalletLabelsTest(BitcoinTestFramework):
 
         # Note each time we call generate, all generated coins go into
         # the same address, so we call twice to get two addresses w/50 each
-        node.generate(1)
-        node.generate(101)
+        node.generatetoaddress(nblocks=1, address=node.getnewaddress(label='coinbase'))
+        node.generatetoaddress(nblocks=101, address=node.getnewaddress(label='coinbase'))
         assert_equal(node.getbalance(), 100)
 
         # there should be 2 address groups
@@ -42,8 +42,9 @@ class WalletLabelsTest(BitcoinTestFramework):
         linked_addresses = set()
         for address_group in address_groups:
             assert_equal(len(address_group), 1)
-            assert_equal(len(address_group[0]), 2)
+            assert_equal(len(address_group[0]), 3)
             assert_equal(address_group[0][1], 50)
+            assert_equal(address_group[0][2], 'coinbase')
             linked_addresses.add(address_group[0][0])
 
         # send 50 from each address to a third address not in this wallet
@@ -77,7 +78,7 @@ class WalletLabelsTest(BitcoinTestFramework):
             label.verify(node)
 
         # Check all labels are returned by listlabels.
-        assert_equal(node.listlabels(), [label.name for label in labels])
+        assert_equal(node.listlabels(), sorted(['coinbase'] + [label.name for label in labels]))
 
         # Send a transaction to each label.
         for label in labels:
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index 7d7c77638e..30ab750fac 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -129,7 +129,7 @@ class MultiWalletTest(BitcoinTestFramework):
         self.start_node(0, ['-wallet=w4', '-wallet=w5'])
         assert_equal(set(node.listwallets()), {"w4", "w5"})
         w5 = wallet("w5")
-        w5.generate(1)
+        node.generatetoaddress(nblocks=1, address=w5.getnewaddress())
 
         # now if wallets/ exists again, but the rootdir is specified as the walletdir, w4 and w5 should still be loaded
         os.rename(wallet_dir2, wallet_dir())
@@ -153,7 +153,7 @@ class MultiWalletTest(BitcoinTestFramework):
         wallet_bad = wallet("bad")
 
         # check wallet names and balances
-        wallets[0].generate(1)
+        node.generatetoaddress(nblocks=1, address=wallets[0].getnewaddress())
         for wallet_name, wallet in zip(wallet_names, wallets):
             info = wallet.getwalletinfo()
             assert_equal(info['immature_balance'], 50 if wallet is wallets[0] else 0)
@@ -166,7 +166,7 @@ class MultiWalletTest(BitcoinTestFramework):
         assert_raises_rpc_error(-19, "Wallet file not specified", node.getwalletinfo)
 
         w1, w2, w3, w4, *_ = wallets
-        w1.generate(101)
+        node.generatetoaddress(nblocks=101, address=w1.getnewaddress())
         assert_equal(w1.getbalance(), 100)
         assert_equal(w2.getbalance(), 0)
         assert_equal(w3.getbalance(), 0)
@@ -175,7 +175,7 @@ class MultiWalletTest(BitcoinTestFramework):
         w1.sendtoaddress(w2.getnewaddress(), 1)
         w1.sendtoaddress(w3.getnewaddress(), 2)
         w1.sendtoaddress(w4.getnewaddress(), 3)
-        w1.generate(1)
+        node.generatetoaddress(nblocks=1, address=w1.getnewaddress())
         assert_equal(w2.getbalance(), 1)
         assert_equal(w3.getbalance(), 2)
         assert_equal(w4.getbalance(), 3)