From c269209336dfa7e4258cfe7d5b0a3bc07b0da3b2 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 12 Oct 2018 00:04:06 +0900 Subject: [PATCH 1/4] [tests] Small fixups before deprecating generate In advance of deprecating the generate RPC method, make some small changes to a small number of inidividual test cases: - make memory checking less prescriptive in wallet_basic.py - replace calls to generate with generatetoaddress in wallet_keypool.py - replace calls to generate with generatetoaddress and fixup label issues in wallet_labels.py - replace calls to generate with generatetoaddress in wallet_multiwallet.py --- test/functional/wallet_basic.py | 5 +++-- test/functional/wallet_keypool.py | 9 ++++----- test/functional/wallet_labels.py | 9 +++++---- test/functional/wallet_multiwallet.py | 8 ++++---- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 8bbff7f7e..4de3356d7 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 51afa0cb1..ceb970971 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 8d7c77bb9..b71dae9f4 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 189bc2d50..9fa6d0299 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -125,7 +125,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()) @@ -147,7 +147,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) @@ -160,7 +160,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) @@ -169,7 +169,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) From aab81720de237b258ed4e15f1b1831c11abf74f0 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 5 Oct 2018 16:51:10 +0900 Subject: [PATCH 2/4] [tests] Add generate method to TestNode Adds a generate() method to the TestNode class in the test framework. This method intercepts calls to generate, imports a dewterministic private key to the node and then calls generatetoaddress to generate the block to that address. Note that repeated calls to importprivkey for the same private keys are no-ops, so it's fine to call the generate() method many times. --- test/functional/test_framework/test_node.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c05988c66..cc1bfabbf 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)) From c9f02955b2e9062808a9455c4ee7d52cf401eef5 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 11 Oct 2018 23:14:27 +0900 Subject: [PATCH 3/4] [wallet] Deprecate the generate RPC method --- doc/release-notes-14468.md | 15 +++++++++++++++ src/wallet/rpcwallet.cpp | 6 ++++++ test/functional/rpc_deprecated.py | 12 ++++++++++-- 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 doc/release-notes-14468.md diff --git a/doc/release-notes-14468.md b/doc/release-notes-14468.md new file mode 100644 index 000000000..fb0243aba --- /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/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index df10fb48c..9827e025c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3289,6 +3289,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 58074803c..588bfbe08 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() From ab9aca2bdfe68fcd512955ed2c4d706933088528 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 23 Oct 2018 08:32:00 -0400 Subject: [PATCH 4/4] [rpc] add 'getnewaddress' hint to 'generatetoaddress' help text. --- src/rpc/mining.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index d5fb0db75..a47ae34b4 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();