[wallet] Deprecate generate RPC method #14468

jnewbery commented at 1:03 PM on October 12, 2018: member

Deprecates the generate RPC method.

For concept discussion, see #14299.

jnewbery force-pushed on Oct 12, 2018

fanquake added the label Wallet on Oct 12, 2018

jnewbery force-pushed on Oct 12, 2018

jnewbery renamed this:
~~[WIP] [wallet] Deprecate generate RPC method~~
[wallet] Deprecate generate RPC method
on Oct 12, 2018

meshcollider commented at 2:23 PM on October 12, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/f8c217b3ac4c8ba7e7143ee8cc2212c43132c94a modulo failing test, just needs a check that the wallet is enabled before running it

jnewbery force-pushed on Oct 12, 2018

jnewbery commented at 2:36 PM on October 12, 2018: member

Thanks @MeshCollider. The rpc_deprecated.py test should be fixed now.

meshcollider commented at 2:42 PM on October 12, 2018: contributor

re-utACK, just note that the whole rpc_deprecated test will be skipped now if the wallet isn't enabled, which is ok for now since this is the only test in it I guess

jnewbery commented at 3:34 PM on October 12, 2018: member

re-utACK, just note that the whole rpc_deprecated test will be skipped now if the wallet isn't enabled, which is ok for now since this is the only test in it I guess

Yes. I did consider just silently skipping the individual test case, but I think "mark the whole test as skipped if part of it is skipped" makes most sense. The other option "mark the test as passed even if part of it is skipped" means that test cases can be silently skipped.

sipa commented at 7:06 PM on October 12, 2018: member

Concept ACK

gmaxwell commented at 7:50 PM on October 12, 2018: contributor

Concept ACK

achow101 commented at 8:30 PM on October 12, 2018: member

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

fanquake added this to the milestone 0.18.0 on Oct 13, 2018

in test/functional/wallet_basic.py:102 in 10c1084d9f outdated

  99 |          memory_before = self.nodes[0].getmemoryinfo()
 100 |          self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11)
 101 |          mempool_txid = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10)
 102 |          memory_after = self.nodes[0].getmemoryinfo()
 103 | -        assert(memory_before['locked']['used'] + 64 <= memory_after['locked']['used'])
 104 | +        assert_greater_than(memory_after['locked']['used'], memory_before['locked']['used'])

jimmysong commented at 9:34 PM on October 13, 2018:

Shouldn't the right argument be +63?

jnewbery commented at 2:50 PM on October 15, 2018:

The memory usage change between these two calls is actually 32.

I'm not entirely sure why getmemoryinfo is part of the wallet_basic test. How bitcoin core allocates locked memory pages seems purely an implementation detail and shouldn't cause functional wallet tests to fail.

To minimize the delta in this PR, I changed the equality check to a looser inequality check, but I think this test case should be moved out of wallet_basic.py entirely in a future PR.

jimmysong commented at 3:13 PM on October 15, 2018:

sounds like a good one for someone to do =).

Curious how the test passed before if it was only 32 bytes?

jnewbery commented at 3:15 PM on October 15, 2018:

The difference was 64 bytes before the change to the way the test generates blocks.

jimmysong commented at 8:54 PM on October 15, 2018:

makes sense.

in test/functional/wallet_labels.py:33 in 10c1084d9f outdated

  28 | @@ -29,8 +29,8 @@ def run_test(self):
  29 |  
  30 |          # Note each time we call generate, all generated coins go into
  31 |          # the same address, so we call twice to get two addresses w/50 each
  32 | -        node.generate(1)
  33 | -        node.generate(101)
  34 | +        node.generatetoaddress(nblocks=1, address=node.getnewaddress(label='coinbase'))
  35 | +        node.generatetoaddress(nblocks=101, address=node.getnewaddress(label='coinbase'))

jimmysong commented at 9:37 PM on October 13, 2018:

maybe put the coinbase_address as a variable?

jnewbery commented at 2:50 PM on October 15, 2018:

The intent here is to have two addresses with 50 bitcoin each (see comment immediately above).

jimmysong commented at 3:12 PM on October 15, 2018:

makes sense.

jimmysong commented at 9:38 PM on October 13, 2018: contributor

A couple of nits =)

jnewbery commented at 2:51 PM on October 15, 2018: member

Thanks for the review @jimmysong . I've responded to your nits. Hope my explanations make sense.

sipa commented at 9:25 PM on October 15, 2018: member

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

in test/functional/test_framework/test_node.py:213 in 10c1084d9f outdated

 202 | +        # Try to import the node's deterministic private key. This is a no-op if the private key
 203 | +        # has already been imported.
 204 | +        try:
 205 | +            self.rpc.importprivkey(privkey=self.get_deterministic_priv_key().key, label='coinbase', rescan=False)
 206 | +        except JSONRPCException as e:
 207 | +            assert str(e).startswith('Method not found') or \

laanwj commented at 10:52 AM on October 16, 2018:

I'm not sure I understand this assertion, do we want here to blanket-ignore exceptions matching these strings?

seems to me that continuing with generatetoaddress if this failed is not a good idea

promag commented at 1:39 PM on October 16, 2018:

Only accept importprivkey failure if:

wallet is disabled
there are multiple wallets to import to
wallet is locked

In either case the outcome is the impossibility of spending the coinbase output. And if that's relevant to the test then it would fail for not having enough coins to spend.

jnewbery commented at 3:35 PM on October 16, 2018:

@promag is right. This call to importprivkey can fail for the three reasons listed.

I think that this is a bit too much magic behind the scenes though. I've tidied up the deterministic priv key import behaviour in another branch here: https://github.com/jnewbery/bitcoin/tree/deprecate_generate2. Rather than complicate this PR and add review burden here, are you happy to leave this as is for now and take the final commit in https://github.com/jnewbery/bitcoin/tree/deprecate_generate2 as a follow-up PR?

laanwj commented at 6:53 AM on October 18, 2018:

@jnewbery yes, I'm okay with that! though for this PR, please add a comment w/ the contents of @promag's post or something like that, so that people reading the code and wondering the same understand

jnewbery commented at 9:47 PM on October 18, 2018:

Comment added.

in test/functional/wallet_multiwallet.py:128 in 10c1084d9f outdated

 124 | @@ -125,7 +125,8 @@ def wallet_file(name):
 125 |          self.start_node(0, ['-wallet=w4', '-wallet=w5'])
 126 |          assert_equal(set(node.listwallets()), {"w4", "w5"})
 127 |          w5 = wallet("w5")
 128 | -        w5.generate(1)
 129 | +        self.nodes[0].generatetoaddress(nblocks=1, address=w5.getnewaddress())

promag commented at 1:25 PM on October 16, 2018:

Just noting that could use w5.generatetoaddress despite the fact this RPC doesn't require a wallet.

jnewbery commented at 3:28 PM on October 16, 2018:

Yes, that's possible, but I think it's better to call the generatetoaddress method on the node endpoint rather than the wallet endoint (since it's a node method).

However, I have updated these calls to use the node alias rather than self.nodes[0] to be mode compact.

in test/functional/wallet_multiwallet.py:129 in 10c1084d9f outdated

 124 | @@ -125,7 +125,8 @@ def wallet_file(name):
 125 |          self.start_node(0, ['-wallet=w4', '-wallet=w5'])
 126 |          assert_equal(set(node.listwallets()), {"w4", "w5"})
 127 |          w5 = wallet("w5")
 128 | -        w5.generate(1)
 129 | +        self.nodes[0].generatetoaddress(nblocks=1, address=w5.getnewaddress())
 130 | +        # w5.generate(1)

promag commented at 1:25 PM on October 16, 2018:

IMO drop these comments — because it is deprecated code.

jnewbery commented at 3:27 PM on October 16, 2018:

you're right - these should have been removed

in src/wallet/rpcwallet.cpp:3293 in 10c1084d9f outdated

3288 | @@ -3289,6 +3289,12 @@ UniValue generate(const JSONRPCRequest& request)
3289 |          );
3290 |      }
3291 |  
3292 | +    if (!IsDeprecatedRPCEnabled("generate")) {
3293 | +        throw JSONRPCError(RPC_METHOD_DEPRECATED, "The wallet generate rpc method is deprecated and will be fully removed in v0.19. "

promag commented at 1:28 PM on October 16, 2018:

nit, could use same wording as https://github.com/bitcoin/bitcoin/blob/2468471e13987b1be377e1b33fe9c5cdb7a7a3e3/src/wallet/rpcwallet.cpp#L1071

jnewbery commented at 3:29 PM on October 16, 2018:

I wanted to be explicit that it was the wallet method that was being deprecated. Just saying generate is deprecated could be interpreted as the node not being able to generate blocks at all.

promag commented at 1:40 PM on October 16, 2018: member

utACK 10c1084.

[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

c269209336

jnewbery force-pushed on Oct 16, 2018

jnewbery commented at 3:30 PM on October 16, 2018: member

Addressed @promag's comments

Sjors commented at 8:46 AM on October 17, 2018: member

Concept ACK

Can you replace the RPC example bitcoin-cli generatetoaddress 11 "myaddress" with the more useful bitcoin-cli generatetoaddress 1 $(bitcoin-cli getnewaddress)?

jnewbery commented at 7:03 PM on October 17, 2018: member

Can you replace the RPC example bitcoin-cli generatetoaddress 11 "myaddress" with the more useful bitcoin-cli generatetoaddress 1 $(bitcoin-cli getnewaddress)?

That example only works if bitcoind is compiled with a wallet and is running with a single wallet. I don't think it makes sense to make the example so specific.

promag commented at 12:52 AM on October 18, 2018: member

Can you replace the RPC example bitcoin-cli generatetoaddress 11 "myaddress" with the more useful bitcoin-cli generatetoaddress 1 $(bitcoin-cli getnewaddress)? @Sjors sorry, I can't find that, can you link it?

Sjors commented at 7:04 AM on October 18, 2018: member

@jnewbery there can be more than one example in the RPC help, but this is the default config and most likely thing someone is looking for if they try regtest for the first time.

promag commented at 8:59 AM on October 18, 2018: member

https://github.com/bitcoin/bitcoin/blob/9c5f0d542d1db507b3b9c87bd9de6d0d758d51c1/src/rpc/mining.cpp#L168-L169

I also think this is fine. FWIW I would remove all examples.

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

aab81720de

[wallet] Deprecate the generate RPC method c9f02955b2

jnewbery force-pushed on Oct 18, 2018

jnewbery commented at 9:48 PM on October 18, 2018: member

I've added a comment to address #14468 (review).

promag commented at 10:42 PM on October 18, 2018: member

Unrelated failure https://travis-ci.org/bitcoin/bitcoin/jobs/443416363. Restarted job.

promag commented at 11:24 PM on October 18, 2018: member

utACK c9f0295 👋 generate

in test/functional/rpc_deprecated.py:17 in c9f02955b2 outdated

  13 | -        self.extra_args = [[], ["-deprecatedrpc=validateaddress"]]
  14 | +        self.extra_args = [[], ["-deprecatedrpc=generate"]]
  15 | +
  16 | +    def skip_test_if_missing_module(self):
  17 | +        # The generate RPC method requires the wallet to be compiled
  18 | +        self.skip_if_no_wallet()

MarcoFalke commented at 3:14 AM on October 21, 2018:

I don't think the whole test should be skipped if there is not wallet compiled in, but rather only the wallet-specific assert_raises_rpc_error should be skipped. This way other tests in this file that are not wallet-specific would run when the wallet is not compiled.

MarcoFalke commented at 3:20 AM on October 21, 2018: member

utACK c9f02955b2e9062808a9455c4ee7d52cf401eef5, but I'd prefer to not use skip_test_if_missing_module for test suites that are not module-specific (e.g. wallet_.py or interface_zmq_.py suites).

jnewbery commented at 2:39 PM on October 22, 2018: member

I don't think the whole test should be skipped if there is not wallet compiled in, but rather only the wallet-specific assert_raises_rpc_error should be skipped. This way other tests in this file that are not wallet-specific would run when the wallet is not compiled.

See earlier comment:

Yes. I did consider just silently skipping the individual test case, but I think "mark the whole test as skipped if part of it is skipped" makes most sense. The other option "mark the test as passed even if part of it is skipped" means that test cases can be silently skipped.

I'd prefer not to change this PR and invalidate ACKs, but if you feel strongly about this, I could add a wallet_deprecated.py test.

sanket1729 commented at 7:15 AM on October 23, 2018: contributor

I locally tested the changes. Everything works correctly.

Also note that popular tutorials (bitcoin.org and maybe others) start with this RPC as the first example. Giving an additional RPC help output which directly gives new users a copy-paste line might be useful. bitcoin-cli generatetoaddress 1 $(bitcoin-cli getnewaddress).

Not a big deal because websites will update eventually. ACK c9f02955b2e9062808a9455c4ee7d52cf401eef5

[rpc] add 'getnewaddress' hint to 'generatetoaddress' help text. ab9aca2bdf

jnewbery commented at 12:33 PM on October 23, 2018: member

@Sjors @sanket1729 I've added a hint to use getnewaddress to the generatetoaddress help text.

I didn't include the bitcoin-cli generatetoaddress 1 $(bitcoin-cli getnewaddress) text because that's specific to posix shells.

sanket1729 commented at 5:05 PM on October 23, 2018: contributor

Tested. ACK ab9aca2bdfe68fcd512955ed2c4d706933088528 .

MarcoFalke commented at 6:16 PM on October 23, 2018: member

Fine with me to keep it as is in this pull request for now, but if you disagree with the concept of (silently) skipping subtests if the module required to run them is not available, we should revert #14324 and remove the is_compiled helpers, since that seems the only use case of them.

jnewbery commented at 7:23 PM on October 23, 2018: member

...if you disagree with the concept of (silently) skipping subtests...

I can see the merit in both arguments, and don't feel strongly one way or the other.

I'd prefer to not churn this PR any more, but as I said earlier, happy to change if you feel strongly.

MarcoFalke merged this on Oct 23, 2018

MarcoFalke closed this on Oct 23, 2018

MarcoFalke referenced this in commit 3668bb335c on Oct 23, 2018

DrahtBot commented at 10:22 PM on October 23, 2018: member

Reviewers, this pull request conflicts with the following ones:

#14502 (Rpc help helper class by karel-3d)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery deleted the branch on Oct 24, 2018

nopara73 commented at 7:43 AM on October 24, 2018: none

@NicolasDorier you might want to know about this.

MarcoFalke referenced this in commit c34c821e4c on Nov 2, 2018

castarco referenced this in commit eff7532598 on Nov 14, 2018

deadalnix referenced this in commit 2eea6849e3 on May 6, 2020

deadalnix referenced this in commit 59810d76a2 on May 6, 2020

deadalnix referenced this in commit 8aa2e93785 on May 6, 2020

deadalnix referenced this in commit d6f5209927 on May 6, 2020

ftrader referenced this in commit d873f3d7fc on Aug 17, 2020

MarcoFalke locked this on Sep 8, 2021