rpc: Move the generate RPC call to rpcwallet
#10683
Conversation
1e09f2c to
8c410a8
Compare
There was a problem hiding this comment.
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
| { | ||
| CWallet * const pwallet = GetWalletForJSONRPCRequest(request); | ||
|
|
||
| if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) |
src/wallet/rpcwallet.cpp
Outdated
| if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) | ||
| return NullUniValue; | ||
|
|
||
| if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) |
src/wallet/rpcwallet.cpp
Outdated
| + HelpExampleCli("generate", "11") | ||
| ); | ||
|
|
||
| int nGenerate = request.params[0].get_int(); |
There was a problem hiding this comment.
nit: variable name should be lowercase, not hungarian
src/wallet/rpcwallet.cpp
Outdated
| ); | ||
|
|
||
| int nGenerate = request.params[0].get_int(); | ||
| uint64_t nMaxTries = 1000000; |
There was a problem hiding this comment.
nit: variable name should be lowercase, not hungarian
src/wallet/rpcwallet.cpp
Outdated
|
|
||
| int nGenerate = request.params[0].get_int(); | ||
| uint64_t nMaxTries = 1000000; | ||
| if (request.params.size() > 1) { |
There was a problem hiding this comment.
nit: should handle case when request.params[1] is Null (for named arguments)
src/wallet/rpcwallet.cpp
Outdated
| pwallet->GetScriptForMining(coinbaseScript); | ||
|
|
||
| // If the keypool is exhausted, no script is returned at all. Catch this. | ||
| if (!coinbaseScript) |
src/wallet/rpcwallet.cpp
Outdated
| throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); | ||
|
|
||
| //throw an error if no script was provided | ||
| if (coinbaseScript->reserveScript.empty()) |
|
utACK 8c410a8 though nit cleanup is great too. |
|
Nice and thanks for doing this: |
|
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. |
3b75faf to
4cdb620
Compare
|
utACK 4cdb6209de2f712cd5847373b6bc2fb6401fb739. No more nits. Thanks!
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. |
src/rpc/mining.h
Outdated
| #ifndef BITCOIN_RPC_MINING_H | ||
| #define BITCOIN_RPC_MINING_H | ||
|
|
||
| #include <script/script.h> |
There was a problem hiding this comment.
Shouldnt this be a "-style include?
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`.
Fix nits by John Newbery.
4cdb620 to
2a96283
Compare
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
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
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
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
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
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
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
ScriptForMiningmethod onCValidationInterface, 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.