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

[rpc] remove deprecated generate method #15492

Merged
merged 2 commits into from Mar 2, 2019

Conversation

Projects
None yet
8 participants
@Sjors
Copy link
Member

Sjors commented Feb 27, 2019

As announced in v0.18, the wallet generate rpc method is deprecated and will be fully removed in v0.19.

Clients should transition to using the node rpc method generatetoaddress.

@fanquake fanquake added this to the 0.19.0 milestone Feb 27, 2019

@Sjors Sjors force-pushed the Sjors:2019/02/rpc-generate-remove branch 3 times, most recently from 63477e5 to 5e0d4fb Feb 27, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 27, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15150 (gui: Show wallet selector on console window if there are wallets loaded by promag)

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 27, 2019

Nice cleanup. utACK.

@jnewbery
Copy link
Member

jnewbery left a comment

This should be marked as WIP and not merged before the 0.18 branch split.

I agree that this should go in before #15492.

Show resolved Hide resolved src/wallet/rpcwallet.cpp
@@ -4,13 +4,13 @@
# 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
# from test_framework.util import assert_raises_rpc_error

This comment has been minimized.

@jnewbery

jnewbery Feb 27, 2019

Member

Rather than change this file around, can you just revert it to how it was pre-commit c9f0295.

This comment has been minimized.

@Sjors

Sjors Feb 27, 2019

Author Member

Main reason for changing this file around is improving documentation; generate allows a much simpler example than createmultisig.

Update: removed the skip_test_if_missing_module bit, since that's wallet specific.

@Sjors Sjors force-pushed the Sjors:2019/02/rpc-generate-remove branch from 5e0d4fb to 8bb3e4c Feb 27, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Feb 27, 2019

Why WIP? That seems more appropriate for more experimental code. This PR is already tagged 0.19 so that should be clear enough.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Feb 27, 2019

Tagging 0.19 is fine. Just as long as the maintainers are aware that this shouldn't be merged prior to the branch split.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Feb 27, 2019

This PR seems reasonable, but I would like to remove:

Removal helps with separating the wallet from the node in #15288

from the PR description. I don't think this is true in any significant sense. If there really is a need to relate this PR to #15288, you could say:

Removal means that the interfaces::Chain class no longer needs to have a generateBlocks method added in #15288

I think there is some confusion in the #15288 discussion that generateBlocks method requires additional makefile changes that other Chain methods don't require, but this is not true. The only reason makefile changes are part of the generateBlocks commit is because that commit comes first, so all the later commits just piggyback off of it.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Feb 27, 2019

@ryanofsky I removed that comment

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 8bb3e4c, looks good to merge after the branch

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 28, 2019

GetScriptForMining is unused after the removal of generate.

I suggest getting rid of it! :-)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Mar 2, 2019

Merge bitcoin#15492: [rpc] remove deprecated generate method
07cae52 [wallet] remove unused GetScriptForMining (Sjors Provoost)
8bb3e4c [rpc] remove deprecated generate method (Sjors Provoost)

Pull request description:

  As announced in v0.18, the wallet generate rpc method is deprecated and will be fully removed in v0.19.

  Clients should transition to using the node rpc method `generatetoaddress`.

Tree-SHA512: 9e5e913b59f3e18440b2b7b356124c7b87ad19f81a1ab6ada06a6c396b84e734895465f569296f1ba8c12abf74863bab5fd77765c9e806c239713aa83a59485f
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 2, 2019

ACK

@MarcoFalke MarcoFalke merged commit 07cae52 into bitcoin:master Mar 2, 2019

2 checks passed

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

@Sjors Sjors deleted the Sjors:2019/02/rpc-generate-remove branch Mar 2, 2019

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.