Skip to content

rpc: Move the generate RPC call to rpcwallet #10683

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 2 commits into from
Jul 3, 2017
Merged

rpc: Move the generate RPC call to rpcwallet #10683

laanwj merged 2 commits into from
Jul 3, 2017

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 27, 2017 •
edited
Loading

This makes it possible to mine to any wallet when multi-wallet mode is added. Solves the same problem as #10649, but IMO in a cleaner way.

It also gets rid of the circuitous ScriptForMining method on CValidationInterface, which really doesn't belong there.

After this change it's still possible to mine without wallet through generatetoaddress. I first proposed this in #7965.

This was referenced Jun 27, 2017
Closed
Closed
@laanwj laanwj force-pushed the 2017_06_wallet_mining branch from 1e09f2c to 8c410a8 Compare June 27, 2017 15:02
jnewbery
jnewbery reviewed Jun 27, 2017
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK 8c410a8fbb5f1876484a3f10d25cf9c06a8ef875. This is a sensible change. Anything that moves us towards #7965 and away from circular libbitcoin_server.a/libbitcoin_wallet.a dependencies is a good thing in my book.

I realise that this is mostly code-move. There are a bunch of style nits in the moved code - entirely up to you if you want to address them as part of this PR or leave them as they are.

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: braces

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: braces

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: variable name should be lowercase, not hungarian

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: variable name should be lowercase, not hungarian

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should handle case when request.params[1] is Null (for named arguments)

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: braces

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: braces

@instagibbs
Copy link
Member

instagibbs commented Jun 27, 2017

utACK 8c410a8 though nit cleanup is great too.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jun 27, 2017

Nice and thanks for doing this:
utACK 8c410a8fbb5f1876484a3f10d25cf9c06a8ef875

@laanwj
Copy link
Member Author

laanwj commented Jun 28, 2017

Ok ok I'll add a commit to clean up the style nits, don't think it should be done at the time of the move as that just complicates review.

@laanwj laanwj mentioned this pull request Jun 28, 2017
Merged
@laanwj laanwj force-pushed the 2017_06_wallet_mining branch from 3b75faf to 4cdb620 Compare June 28, 2017 06:35
@jnewbery
Copy link
Contributor

jnewbery commented Jun 28, 2017

utACK 4cdb6209de2f712cd5847373b6bc2fb6401fb739. No more nits. Thanks!

don't think it should be done at the time of the move as that just complicates review.

I'd agree if the nit fixes were in the same commit, but if they're done in their own commit either before or after the move it adds almost no review burden (as long as reviewers are reviewing commitwise)

@laanwj
Copy link
Member Author

laanwj commented Jun 28, 2017

I'd agree if the nit fixes were in the same commit, but if they're done in their own commit either before or after the move it adds almost no review burden (as long as reviewers are reviewing commitwise)

Yes that's what I meant, with a separate commit it's fine.

TheBlueMatt
TheBlueMatt reviewed Jun 28, 2017
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4cdb6209de2f712cd5847373b6bc2fb6401fb739, thanks for cleaning up CValidationInterface.

src/rpc/mining.h Outdated
Copy link
Contributor

@TheBlueMatt TheBlueMatt Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt this be a "-style include?

Copy link
Member Author

@laanwj laanwj Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point

laanwj added 2 commits June 29, 2017 12:02
@laanwj
rpc: Move the generate RPC call to rpcwallet
df7e2f0 
This makes it possible to mine to any wallet when multi-wallet mode is added.
Solves the same problem as bitcoin#10649, but IMO in a cleaner way.

It also gets rid of the circuitous `ScriptForMining` method on
`CValidationInterface`, which really doesn't belong there.

After this change it's still possible to mine without wallet through
`generatetoaddress`.
@laanwj
rpc: Update generate for developer notes
2a96283 
Fix nits by John Newbery.
@laanwj laanwj force-pushed the 2017_06_wallet_mining branch from 4cdb620 to 2a96283 Compare June 29, 2017 10:04
@laanwj
Copy link
Member Author

laanwj commented Jun 29, 2017

Updated #include style and squashed this into the first commit, where mining.h was introduced:
8c410a8fbb5f1876484a3f10d25cf9c06a8ef875 → df7e2f0
4cdb6209de2f712cd5847373b6bc2fb6401fb739 → 2a96283

@laanwj laanwj merged commit 2a96283 into bitcoin:master Jul 3, 2017
laanwj added a commit that referenced this pull request Jul 3, 2017
@laanwj
Merge #10683: rpc: Move the generate RPC call to rpcwallet
d81bec7 
2a96283 rpc: Update `generate` for developer notes (Wladimir J. van der Laan)
df7e2f0 rpc: Move the `generate` RPC call to rpcwallet (Wladimir J. van der Laan)

Tree-SHA512: ec658d6178f8435dc54b9d9c6dd59f873055a8ae0c3f177c02049d77b93107dd5fc17a1ff56d50f051810d52fdf306846eaba2ef4fc8d2a6cfa831f57a1045c4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 5, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#10683: rpc: Move the generate RPC call to rpcwallet
6edcf43 
2a96283 rpc: Update `generate` for developer notes (Wladimir J. van der Laan)
df7e2f0 rpc: Move the `generate` RPC call to rpcwallet (Wladimir J. van der Laan)

Tree-SHA512: ec658d6178f8435dc54b9d9c6dd59f873055a8ae0c3f177c02049d77b93107dd5fc17a1ff56d50f051810d52fdf306846eaba2ef4fc8d2a6cfa831f57a1045c4
@PastaPastaPasta PastaPastaPasta mentioned this pull request Aug 5, 2019
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

@jnewbery jnewbery jnewbery left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Mining RPC/REST/ZMQ Tests Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@laanwj @instagibbs @jonasschnelli @jnewbery @TheBlueMatt