Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] Deprecate generate RPC method #14468

Merged
merged 4 commits into from Oct 23, 2018

Conversation

Projects
None yet
@jnewbery
Copy link
Member

commented Oct 12, 2018

Deprecates the generate RPC method.

For concept discussion, see #14299.

Fixes #14299.

@jnewbery jnewbery force-pushed the jnewbery:deprecate_generate branch Oct 12, 2018

@fanquake fanquake added the Wallet label Oct 12, 2018

@jnewbery jnewbery force-pushed the jnewbery:deprecate_generate branch Oct 12, 2018

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

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

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

@jnewbery jnewbery force-pushed the jnewbery:deprecate_generate branch Oct 12, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

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

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Concept ACK

1 similar comment
@gmaxwell

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Concept ACK

@achow101

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

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

@jimmysong
Copy link
Contributor

left a comment

A couple of nits =)

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'])

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 13, 2018

Contributor

Shouldn't the right argument be +63?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 15, 2018

Author Member

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.

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 15, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 15, 2018

Author Member

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

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 15, 2018

Contributor

makes sense.

node.generate(1)
node.generate(101)
node.generatetoaddress(nblocks=1, address=node.getnewaddress(label='coinbase'))
node.generatetoaddress(nblocks=101, address=node.getnewaddress(label='coinbase'))

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 13, 2018

Contributor

maybe put the coinbase_address as a variable?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 15, 2018

Author Member

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

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 15, 2018

Contributor

makes sense.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

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

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

utACK 10c1084d9fc6d4c13ca06bb2ca44088ce9b916ba

try:
self.rpc.importprivkey(privkey=self.get_deterministic_priv_key().key, label='coinbase', rescan=False)
except JSONRPCException as e:
assert str(e).startswith('Method not found') or \

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 16, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 16, 2018

Author Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 18, 2018

Member

@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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 18, 2018

Author Member

Comment added.

@promag
Copy link
Member

left a comment

utACK 10c1084.

test/functional/wallet_multiwallet.py Outdated
@@ -125,7 +125,8 @@ def wallet_file(name):
self.start_node(0, ['-wallet=w4', '-wallet=w5'])
assert_equal(set(node.listwallets()), {"w4", "w5"})
w5 = wallet("w5")
w5.generate(1)
self.nodes[0].generatetoaddress(nblocks=1, address=w5.getnewaddress())

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 16, 2018

Author Member

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.

test/functional/wallet_multiwallet.py Outdated
@@ -125,7 +125,8 @@ def wallet_file(name):
self.start_node(0, ['-wallet=w4', '-wallet=w5'])
assert_equal(set(node.listwallets()), {"w4", "w5"})
w5 = wallet("w5")
w5.generate(1)
self.nodes[0].generatetoaddress(nblocks=1, address=w5.getnewaddress())
# w5.generate(1)

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

IMO drop these comments — because it is deprecated code.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 16, 2018

Author Member

you're right - these should have been removed

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

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

nit, could use same wording as

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 16, 2018

Author Member

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.

try:
self.rpc.importprivkey(privkey=self.get_deterministic_priv_key().key, label='coinbase', rescan=False)
except JSONRPCException as e:
assert str(e).startswith('Method not found') or \

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

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.

[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

@jnewbery jnewbery force-pushed the jnewbery:deprecate_generate branch Oct 16, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

Addressed @promag's comments

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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.

jnewbery added some commits Oct 5, 2018

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

@jnewbery jnewbery force-pushed the jnewbery:deprecate_generate branch to c9f0295 Oct 18, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

utACK c9f0295 👋 generate


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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 21, 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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 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.

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

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Tested. ACK ab9aca2 .

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

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

@DrahtBot DrahtBot referenced this pull request Oct 23, 2018

Closed

Rpc help helper class #14502

@MarcoFalke MarcoFalke merged commit ab9aca2 into bitcoin:master Oct 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Oct 23, 2018

Merge #14468: [wallet] Deprecate generate RPC method
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

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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 jnewbery deleted the jnewbery:deprecate_generate branch Oct 24, 2018

@nopara73

This comment has been minimized.

Copy link

commented Oct 24, 2018

@NicolasDorier you might want to know about this.

MarcoFalke added a commit that referenced this pull request Nov 2, 2018

Merge #14631: [tests] Move deterministic address import to setup_nodes
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

@Nizametdinov Nizametdinov referenced this pull request Nov 14, 2018

Merged

Proposers tests: ledger balance #244

1 of 1 task complete

castarco added a commit to castarco/unit-e that referenced this pull request Nov 14, 2018

Use generatetoaddress instead of generate
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 <andres@thirdhash.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.