[wallet] Deprecate generate RPC method #14468

jnewbery · 2018-10-12T13:03:53Z

Deprecates the generate RPC method.

For concept discussion, see #14299.

Fixes #14299.

meshcollider · 2018-10-12T14:23:45Z

utACK f8c217b modulo failing test, just needs a check that the wallet is enabled before running it

jnewbery · 2018-10-12T14:36:42Z

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

meshcollider · 2018-10-12T14:42:59Z

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 · 2018-10-12T15:34:32Z

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 · 2018-10-12T19:06:56Z

Concept ACK

gmaxwell · 2018-10-12T19:50:26Z

Concept ACK

achow101 · 2018-10-12T20:30:42Z

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

jimmysong

A couple of nits =)

jimmysong · 2018-10-13T21:34:49Z

test/functional/wallet_basic.py

Shouldn't the right argument be +63?

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.

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

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

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

makes sense.

jimmysong · 2018-10-13T21:37:39Z

test/functional/wallet_labels.py

maybe put the coinbase_address as a variable?

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

makes sense.

jnewbery · 2018-10-15T14:51:34Z

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

sipa · 2018-10-15T21:25:15Z

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

laanwj · 2018-10-16T10:52:19Z

test/functional/test_framework/test_node.py

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

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.

@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?

@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

Comment added.

promag

utACK 10c1084.

promag · 2018-10-16T13:25:24Z

test/functional/wallet_multiwallet.py

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

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.

promag · 2018-10-16T13:25:45Z

test/functional/wallet_multiwallet.py

IMO drop these comments — because it is deprecated code.

you're right - these should have been removed

promag · 2018-10-16T13:28:55Z

src/wallet/rpcwallet.cpp

nit, could use same wording as

bitcoin/src/wallet/rpcwallet.cpp

Line 1071 in 2468471

throw JSONRPCError(RPC_METHOD_DEPRECATED, "addwitnessaddress is deprecated and will be fully removed in v0.17. "

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 · 2018-10-16T13:39:21Z

test/functional/test_framework/test_node.py

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.

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

jnewbery · 2018-10-16T15:30:14Z

Addressed @promag's comments

Sjors · 2018-10-17T08:46:19Z

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 · 2018-10-17T19:03:41Z

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 · 2018-10-18T00:52:43Z

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 · 2018-10-18T07:04:07Z

@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 · 2018-10-18T08:59:25Z

bitcoin/src/rpc/mining.cpp

Lines 168 to 169 in 9c5f0d5

    
           "\nGenerate 11 blocks to myaddress\n" 
        
           + HelpExampleCli("generatetoaddress", "11 \"myaddress\"")

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

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.

jnewbery · 2018-10-18T21:48:14Z

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

promag · 2018-10-18T22:42:01Z

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

promag · 2018-10-18T23:24:07Z

utACK c9f0295 👋 generate

maflcko · 2018-10-21T03:14:06Z

test/functional/rpc_deprecated.py

+
+    def skip_test_if_missing_module(self):
+        # The generate RPC method requires the wallet to be compiled
+        self.skip_if_no_wallet()


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.

maflcko · 2018-10-21T03:20:36Z

utACK c9f0295, 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 · 2018-10-22T14:39:23Z

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 · 2018-10-23T07:15:37Z

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 c9f0295

jnewbery · 2018-10-23T12:33:46Z

@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 · 2018-10-23T17:05:49Z

Tested. ACK ab9aca2 .

maflcko · 2018-10-23T18:16:49Z

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 · 2018-10-23T19:23:50Z

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

ab9aca2 [rpc] add 'getnewaddress' hint to 'generatetoaddress' help text. (John Newbery) c9f0295 [wallet] Deprecate the generate RPC method (John Newbery) aab8172 [tests] Add generate method to TestNode (John Newbery) c269209 [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

DrahtBot · 2018-10-23T22:22:59Z

Reviewers, this pull request conflicts with the following ones:

Rpc help helper class #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.

nopara73 · 2018-10-24T07:43:10Z

@NicolasDorier you might want to know about this.

3fd7e76 [tests] Move deterministic address import to setup_nodes (John Newbery) Pull request description: This requires a small changes to a few tests, but means that deterministic addresses will always be imported (unless setup_nodes behaviour is explicitly overridden). Tidies up the way we import deterministic addresses, requested in review comment here: #14468 (comment). Tree-SHA512: 2b32edf500e286c463398487ab1153116a1dc90f64a53614716373311abdc83d8a251fdd8f42d1146b56e308664deaf62952113f66e98bc37f23968096d1a961

Instead of using the wallet's `generate` RPC method ( which is deprecated, see bitcoin/bitcoin#14468 ), use the server's `generatetoaddress` RPC method. Signed-off-by: Andres Correa Casablanca <[email protected]>

Summary: 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 This is a partial backport of Core [[bitcoin/bitcoin#14468 | PR14468]] : bitcoin/bitcoin@c269209 I also added a similar fix for abc-parkedchain.py Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D5969

Summary: 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. This is a partial backport of Core [[bitcoin/bitcoin#14468 | PR14468]] : bitcoin/bitcoin@aab8172 The code was slightly tweaked because we have some tests for which `self.rpc` is `None` because they've been created after [[bitcoin/bitcoin#14631 | PR14631]] which remove the call to importprivkey Depends on D5969 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: majcosta Differential Revision: https://reviews.bitcoinabc.org/D5970

Summary: This is a partial backport of Core [[bitcoin/bitcoin#14468 | PR14468]] : bitcoin/bitcoin@c9f0295 Depends on D5970 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D5971

Summary: Final step of [[bitcoin/bitcoin#14468 | PR14468]] Depends on D5971 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D5972

Summary: 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 This is a partial backport of Core [[bitcoin/bitcoin#14468 | PR14468]] : bitcoin/bitcoin@c269209 I also added a similar fix for abc-parkedchain.py Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D5969

jnewbery force-pushed the deprecate_generate branch from f6a6ba5 to 88ab22b Compare October 12, 2018 13:04

fanquake added the Wallet label Oct 12, 2018

jnewbery force-pushed the deprecate_generate branch from 88ab22b to f8c217b Compare October 12, 2018 13:15

jnewbery changed the title ~~[WIP] [wallet] Deprecate generate RPC method~~ [wallet] Deprecate generate RPC method Oct 12, 2018

jnewbery force-pushed the deprecate_generate branch from f8c217b to 10c1084 Compare October 12, 2018 14:36

fanquake added this to the 0.18.0 milestone Oct 13, 2018

jimmysong reviewed Oct 13, 2018

View reviewed changes

laanwj reviewed Oct 16, 2018

View reviewed changes

promag reviewed Oct 16, 2018

View reviewed changes

jnewbery force-pushed the deprecate_generate branch from 10c1084 to 351faf7 Compare October 16, 2018 15:27

jnewbery added 2 commits October 18, 2018 17:46

[wallet] Deprecate the generate RPC method

c9f0295

jnewbery force-pushed the deprecate_generate branch from 351faf7 to c9f0295 Compare October 18, 2018 21:46

maflcko reviewed Oct 21, 2018

View reviewed changes

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

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

ab9aca2

DrahtBot mentioned this pull request Oct 23, 2018

Rpc help helper class #14502

Closed

maflcko merged commit ab9aca2 into bitcoin:master Oct 23, 2018

jnewbery deleted the deprecate_generate branch October 24, 2018 00:43

jnewbery mentioned this pull request Nov 1, 2018

[tests] Move deterministic address import to setup_nodes #14631

Merged

nzmdn mentioned this pull request Nov 14, 2018

Proposers tests: ledger balance dtr-org/unit-e#244

Merged

1 task

castarco mentioned this pull request Nov 14, 2018

Create snapshot every 150 epochs dtr-org/unit-e#240

Merged

jnewbery mentioned this pull request Feb 12, 2019

Move ParseConfirmTarget from rpc/mining to rpc/util #15373

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[wallet] Deprecate generate RPC method #14468

[wallet] Deprecate generate RPC method #14468

Uh oh!

Conversation

jnewbery commented Oct 12, 2018

Uh oh!

meshcollider commented Oct 12, 2018

Uh oh!

jnewbery commented Oct 12, 2018

Uh oh!

meshcollider commented Oct 12, 2018

Uh oh!

jnewbery commented Oct 12, 2018

Uh oh!

sipa commented Oct 12, 2018

Uh oh!

gmaxwell commented Oct 12, 2018

Uh oh!

achow101 commented Oct 12, 2018

Uh oh!

jimmysong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 15, 2018

Uh oh!

sipa commented Oct 15, 2018

Uh oh!

laanwj Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 16, 2018

Uh oh!

Sjors commented Oct 17, 2018

Uh oh!

jnewbery commented Oct 17, 2018

Uh oh!

promag commented Oct 18, 2018

Uh oh!

laanwj Oct 16, 2018 •

edited

Loading