This will interfere with #5993. if this makes sense (conceptual), i’ll rebase this after merging of @sipa’s #5993 or @jtimon’s #4793.
Next steps would be to add a argument pubkeyhash for generate to completely decouple.
generate could check over CMainSignals if a registered “device” can provide a pubkey in case of no given key over the rpc argument.
lightly tested ACK
Thanks for doing this. I coded this a while ago, then set it aside. Your version, with registration, is cleaner.
1207@@ -1208,7 +1208,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
1208 }
1209
1210 CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
1211-{
1212+{
const CChainParams& chainparams as parameter?
CChainParams for the touched functions.
35@@ -35,6 +36,8 @@ class CValidationInterface {
36 virtual void Inventory(const uint256 &hash) {};
37 virtual void ResendWalletTransactions(int64_t nBestBlockTime) {};
38 virtual void BlockChecked(const CBlock&, const CValidationState&) {};
39+ virtual void GetScriptForMining(CScript &script) {};
ScriptForMining the miner will continue with a empty script which should result in a unspendable coinbase? But i didn’t tested that. Will do.
I would not want to create a block with an unspendable coinbase.
I mean the call should return a boolean, or the resulting script should at least be tested for non-emptiness, and the miner should exit if that is the case.
Wouldn’t it be nice if you could use generate() in a non-wallet environment? Regarding RPC test in a post-wallet-split-off world: this could make sense.
If somone uses the internal miner (which has no productive usage IMO) in a wallet-disable mode he probably uses it for testing only.
I agree with adding a warning to the log. But throwing an error would block testing.
Agreed. Maybe it would be most efficient by adding a argument to generate() and setgenerate() RPC calls where one could provide a output script to mine to?
Adding a validation interface implementation for non-wallet-mining could be over the top.
540@@ -558,7 +541,7 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
541
542 minerThreads = new boost::thread_group();
543 for (int i = 0; i < nThreads; i++)
544- minerThreads->create_thread(boost::bind(&BitcoinMiner, pwallet));
545+ minerThreads->create_thread(boost::bind(&BitcoinMiner, boost::cref(chainparams)));
2479@@ -2480,6 +2480,17 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
2480 }
2481 }
2482
2483+void CWallet::GetScriptForMining(CScript &script)
2484+{
2485+ CReserveKey reservekey(this);
2486+ reservekey.KeepKey();
IIRC there was a discussion about this on IRC. But right, i also have a bad feeling with this. Every mining thread reserves and keeps a key straight off.
I think i’m going invest there some time and build a proper way of only keeping keys which where used when a block was created.
-gen or setgenerate, same script/key for all threads) or when calling generate.
GenerateBitcoins() (generate, setgenerate). But this should be okay, at least after we merge HD features (https://github.com/bitcoin/bitcoin/pull/6265).
And this would fix #6268
NACK– reservekey.KeepKey must happen AFTER .GetReservedKey or it is a no-op.
This works:
0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
1index 440459f..933b0ec 100644
2--- a/src/wallet/wallet.cpp
3+++ b/src/wallet/wallet.cpp
4@@ -2586,12 +2586,12 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
5 void CWallet::GetScriptForMining(CScript &script)
6 {
7 CReserveKey reservekey(this);
8- reservekey.KeepKey();
9
10 CPubKey pubkey;
11 if (!reservekey.GetReservedKey(pubkey))
12 return;
13 script = CScript() << ToByteVector(pubkey) << OP_CHECKSIG;
14+ reservekey.KeepKey();
15 }
16
17 void CWallet::LockCoin(COutPoint& output)
To test: generate a block in -regtest mode, get the address of the coinbase transaction (getblock and then gettransaction). Then call getnewaddress; with the code as-is, you’ll get the same pubkey as used by the coinbase transaction.
Updated. Decoupled wallet (over signaling) from miner and rpcmining commands. It still uses signaling to allow different wallet/pubscript-generation implementations, though, mining without pubScript providing listener will generate a error and stops mining.
Now it reserves and keeps a key when calling setgenerate or generating over cmd arg gen (same key for all generated blocks).
Now the miner is completely decoupled from the wallet.
There are no more #ifdef ENABLE_WALLET checks (for mining).
The only check is during runtime if there was a listening device who could provide a CScript over the signal ScriptForMining().
If one tries to use -gen without a wallet compiled in or enabled, it then will throw an exception during init phase. Current master would just ignore the cmd arg -gen if no wallet is present.
The RPC commands setgenerate, generate, getgenerate works in wallet-disabled mode but also throw a exception error: {"code":-32601,"message":"Use the generate method instead of setgenerate on this network"}
548@@ -571,9 +549,14 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
549 if (nThreads == 0 || !fGenerate)
550 return;
551
552+ CScript coinbaseScript;
553+ GetMainSignals().ScriptForMining(coinbaseScript);
554+
555+ //throw an error if no script was provided
//
Edit: Same nit in rpcmining.cpp.
550@@ -573,9 +551,14 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
551 if (nThreads == 0 || !fGenerate)
552 return;
553
554+ CScript coinbaseScript;
555+ GetMainSignals().ScriptForMining(coinbaseScript);
Maybe my backtrace prove (see below) can convince you.
I can also post a linux gdb backtrace or you could add a sleep(10) invoid CWallet::GetScriptForMining(CScript &script)`.
coinbaseScript (check if it’s empty, etc.) and skip the creation/generation of another CScript or do some other clever things. IMO this design is acceptable for the internal miner/wallet decoupling.
559+ throw std::runtime_error("No coinbase script available (mining requires a wallet)");
560+
561 minerThreads = new boost::thread_group();
562 for (int i = 0; i < nThreads; i++)
563- minerThreads->create_thread(boost::bind(&BitcoinMiner, pwallet));
564+ minerThreads->create_thread(boost::bind(&BitcoinMiner, boost::cref(chainparams), coinbaseScript));
boost::cref and i don’t have a strong feeling about it (in both directions). Maybe it’s worth fixing both as soon as we switch to c++11?
34@@ -34,6 +35,8 @@ class CValidationInterface {
35 virtual void Inventory(const uint256 &hash) {}
36 virtual void ResendWalletTransactions(int64_t nBestBlockTime) {}
37 virtual void BlockChecked(const CBlock&, const CValidationState&) {}
38+ virtual void GetScriptForMining(CScript &script) {};
Why not make the return argument explicit:
0CScript GetScriptForMining();
Yes - you’ll need to specify a combiner for the return argument, but right now this issue (“what if there are multiple subscribers that return a value”) is hidden.
I think the current solution is elegant. If multiple subscribers are present, listeners can check the current script value and overwrite or change it. Combining return values (adding a combiner template) would mean to add a logic into the miner to decide, which scripts should be taken if multiple where supplied.
But sure, this have a downside of the fact, that all non-last signal listeners can’t make sure that its script get taken.
I tend to disagree that it’s the responsibility of the listeners to handle what to do when there are multiple listeners - you may not always have control over in which order they are invoked, and that can result in hard to debug problems.
I’m not sure we even want to have multiple subscribers here, ever? If not, a combiner could simply throw an error if multiple return values are returned.
Lets say if we support two wallets, we could define which wallet takes the signaling-lead by setting the signal registration order (last signal connect = last signal call).
Throwing an error because both wallets could provide a script for mining seems to be relatively strict. The current solution would allow the 2nd (or upcoming) wallet(s) to decide if he likes to take the lead and overwrite or allow the first wallets script.
Adding a combiner would mean that we somehow had to check, which wallet provided which script. Also in that case the miner is in charge to decide which script should be taken.
2582@@ -2583,6 +2583,16 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
2583 }
2584 }
2585
2586+void CWallet::GetScriptForMining(CScript &script)
2587+{
2588+ CReserveKey reservekey(this);
Why not make CReserveKey (or e.g. CReserveScript) an abstract base class defined outside the wallet, and return that from CWallet::GetScriptForMining, so that like now the caller can decide to keep or return it
This also interacts better with the case where multiple wallets return a script for mining and some have to be discarded. At least with a CReserveKey-like class, these keys will be recycled.
- use one CReserveScript per mining thread
35@@ -34,6 +36,8 @@ class CValidationInterface {
36 virtual void Inventory(const uint256 &hash) {}
37 virtual void ResendWalletTransactions(int64_t nBestBlockTime) {}
38 virtual void BlockChecked(const CBlock&, const CValidationState&) {}
39+ virtual void GetScriptForMining(boost::shared_ptr<CReserveScript>&) {};
40+ virtual void UpdateRequestCount(const CBlock&) {};
Inventory(const uint256 &hash), connected to g_signals.Inventory does exactly the same thing. As far as I see we can get rid of UpdateRequestCount and replace it with that.
UpdateRequestCount (name does not reflect the purpose of the method) to ResetRequestCount.
608@@ -609,4 +609,13 @@ class CScript : public std::vector<unsigned char>
609 }
610 };
611
612+class CReserveScript
613+{
614+public:
615+ CScript reserveScript;
616+ virtual void KeepScript() {};
;