detach wallet from miner #5994

Nit: This seems to have sneaked in?

I would say this needs to be able to fail (if no wallet is connected), and have the miner code deal with that case. Otherwise you can't really make it independent of the wallet (if it needs to assume a wallet that can provide a script output for mining is present).

I think it can fail. If no listening module provides a CScript through signal 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.

I think the right solution for that is having a trivial not-really-a-wallet provider that explicitly allows you to set the script output to mine to. It just looks incorrect to have a provider interface for something, and a default implementation that results in the least expected result.

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.

Yes, generate can just be changed to not need the GetScriptForMining interface. And setgenerate could just be deleted IMHO and replaced with a simple proof of concept Python or C miner in the tree.

I just don't like an interface that can clearly fail, but doesn't have a clear way to indicate failure.

I'd rather pass a pointer to chainParams in, and pass that by value to BitcoinMiners. That makes it more obvious to the caller that the argument may be used for a longer time.

Ugh, this means you'll never get a key back, even if no block is found. Would it be possible for the wallet to keep track of which keys are currently being used in a map, and when a block is found, look up the key in the map and mark it as kept?

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.

I think we can leave this as it is now. The only two points where this directly reserves and keeps a key is when starting the internal miner (over -gen or setgenerate, same script/key for all threads) or when calling generate.

Directly keeping the reserve key for a generate command is okay. It can result in unused but kept reserve keys if no block has been found during 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

As the internal miner is meant for testing only, this simple solution is elegant. I'd prefer not to introduce complex micro-management of keys, unless it's necessary for anything besides the miner. Responsibility for not requesting absurd numbers of scripts can be put with the callers (e.g. the mining threads).

NACK-- reservekey.KeepKey must happen AFTER .GetReservedKey or it is a no-op.

This works:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 440459f..933b0ec 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2586,12 +2586,12 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
 void CWallet::GetScriptForMining(CScript &script)
 {
     CReserveKey reservekey(this);
-    reservekey.KeepKey();

     CPubKey pubkey;
     if (!reservekey.GetReservedKey(pubkey))
         return;
     script = CScript() << ToByteVector(pubkey) << OP_CHECKSIG;
+    reservekey.KeepKey();
 }

 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.

Hmmm... Indeed. This was a serious mistake. Thanks for tracking this down. Fixed.

Nit: Missing a space after // Edit: Same nit in rpcmining.cpp.

Are signals guaranteed to be synchronous?

Yes. Boost signals2 are always synchronous and in strict order (first register first call).

Is this guaranteed/by design, or merely an implementation coincidence?

IIRC boost signals2 are by design synchronous. Check http://www.boost.org/doc/libs/1_58_0/doc/html/signals2/thread-safety.html

Nothing on that page suggests they are synchronous...?

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)`.

That's with a current version of Boost. Nothing I can see prevents Boost 1.90 from making it all asynchronous.

What would asynchronous even mean in this context? You can specify how signal's return values are combined. There is no way to make the execution asynchronous without breaking the API (which returns the combined return value directly).

@sipa That's n/a for void return values. Maybe the solution here is to return the CScript then?

I think we can very reasonably assume that boost::signals2 will not fundamentally change their design and API in a way that will only remain compatible for void outputs.

@luke-jr: if the signal listener don't provide a script, a exception get thrown at L559. If there are multiple listeners, one listener can detect this by reading out the passed-by-reference 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.

Maybe I'm alone with this, but I dislike boost::cref... it seems like a hack to make pass-by-reference work when passing a pointer would work just as well.

So a hack for something that's causes by a limitation in Boost itself? Weird...

Passing a pointer would be fine with me, I don't feel strongly about it. I suppose this will be replaced with C++11 constructs (lambda's?) when the time is there.

I saw that the new scheduler also introduced 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?

I don't have a strong opinion either, but I plan to copy whatever is done here for other functions in main.

Why not make the return argument explicit:

CScript 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.

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.

Yes. This current design does not respect the keypool/CReserveKey. Will try a better approach.

Note that 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.

Renamed UpdateRequestCount (name does not reflect the purpose of the method) to ResetRequestCount.

Redundant ;

Fixed.