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.
2922 | @@ -2922,6 +2923,47 @@ UniValue bumpfee(const JSONRPCRequest& request) 2923 | return result; 2924 | } 2925 | 2926 | +UniValue generate(const JSONRPCRequest& request) 2927 | +{ 2928 | + CWallet * const pwallet = GetWalletForJSONRPCRequest(request); 2929 | + 2930 | + if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
nit: braces
2928 | + CWallet * const pwallet = GetWalletForJSONRPCRequest(request); 2929 | + 2930 | + if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) 2931 | + return NullUniValue; 2932 | + 2933 | + if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
nit: braces
2942 | + "\nExamples:\n" 2943 | + "\nGenerate 11 blocks\n" 2944 | + + HelpExampleCli("generate", "11") 2945 | + ); 2946 | + 2947 | + int nGenerate = request.params[0].get_int();
nit: variable name should be lowercase, not hungarian
2943 | + "\nGenerate 11 blocks\n" 2944 | + + HelpExampleCli("generate", "11") 2945 | + ); 2946 | + 2947 | + int nGenerate = request.params[0].get_int(); 2948 | + uint64_t nMaxTries = 1000000;
nit: variable name should be lowercase, not hungarian
2944 | + + HelpExampleCli("generate", "11") 2945 | + ); 2946 | + 2947 | + int nGenerate = request.params[0].get_int(); 2948 | + uint64_t nMaxTries = 1000000; 2949 | + if (request.params.size() > 1) {
nit: should handle case when request.params[1] is Null (for named arguments)
2952 | + 2953 | + std::shared_ptr<CReserveScript> coinbaseScript; 2954 | + pwallet->GetScriptForMining(coinbaseScript); 2955 | + 2956 | + // If the keypool is exhausted, no script is returned at all. Catch this. 2957 | + if (!coinbaseScript)
nit: braces
2956 | + // If the keypool is exhausted, no script is returned at all. Catch this. 2957 | + if (!coinbaseScript) 2958 | + throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); 2959 | + 2960 | + //throw an error if no script was provided 2961 | + if (coinbaseScript->reserveScript.empty())
nit: braces
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.
utACK https://github.com/bitcoin/bitcoin/pull/10683/commits/8c410a8fbb5f1876484a3f10d25cf9c06a8ef875 though nit cleanup is great too.
Nice and thanks for doing this: utACK 8c410a8fbb5f1876484a3f10d25cf9c06a8ef875
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.
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)
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.
0 | @@ -0,0 +1,15 @@ 1 | +// Copyright (c) 2017 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | + 5 | +#ifndef BITCOIN_RPC_MINING_H 6 | +#define BITCOIN_RPC_MINING_H 7 | + 8 | +#include <script/script.h>
Shouldnt this be a "-style include?
Yes, good point
utACK 4cdb6209de2f712cd5847373b6bc2fb6401fb739, thanks for cleaning up CValidationInterface.
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`.
Fix nits by John Newbery.
Updated #include style and squashed this into the first commit, where mining.h was introduced:
8c410a8fbb5f1876484a3f10d25cf9c06a8ef875 → df7e2f057b6c9f0f7c950f9077dc63a577f54117
4cdb6209de2f712cd5847373b6bc2fb6401fb739 → 2a962834febc9f56126ef06cf1bd5e1b02370278