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.

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

detach wallet from miner #5994

Merged
merged 4 commits into from Jul 1, 2015

Conversation

Projects
None yet
8 participants
@jonasschnelli
Member

jonasschnelli commented Apr 10, 2015

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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Apr 12, 2015

Contributor

lightly tested ACK

Thanks for doing this. I coded this a while ago, then set it aside. Your version, with registration, is cleaner.

Contributor

jgarzik commented Apr 12, 2015

lightly tested ACK

Thanks for doing this. I coded this a while ago, then set it aside. Your version, with registration, is cleaner.

Show outdated Hide outdated src/wallet/wallet.cpp
@@ -1208,7 +1208,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
}
CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
{
{

This comment has been minimized.

@Diapolo

Diapolo Apr 12, 2015

Nit: This seems to have sneaked in?

@Diapolo

Diapolo Apr 12, 2015

Nit: This seems to have sneaked in?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 13, 2015

Member

fixed @Diapolo's nits.

Member

jonasschnelli commented Apr 13, 2015

fixed @Diapolo's nits.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 14, 2015

Member

Concept ack.
Given that you're touching their interfaces already, could CreateNewBlockWithScript() ProcessBlockFound(), BitcoinMiner() and GenerateBitcoins() take const CChainParams& chainparams as parameter?

Member

jtimon commented Apr 14, 2015

Concept ack.
Given that you're touching their interfaces already, could CreateNewBlockWithScript() ProcessBlockFound(), BitcoinMiner() and GenerateBitcoins() take const CChainParams& chainparams as parameter?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 14, 2015

Member

Added a commit on top to help improve global state independence by passing CChainParams for the touched functions.

Member

jonasschnelli commented Apr 14, 2015

Added a commit on top to help improve global state independence by passing CChainParams for the touched functions.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 14, 2015

Member

ut ACK, it is very nice to decouple miner.o from the wallet.

Member

jtimon commented Apr 14, 2015

ut ACK, it is very nice to decouple miner.o from the wallet.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 16, 2015

Member

Needs rebase

Member

jtimon commented Apr 16, 2015

Needs rebase

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 16, 2015

Member

Rebased and squashed [squashme] commit.

Member

jonasschnelli commented Apr 16, 2015

Rebased and squashed [squashme] commit.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 16, 2015

Member

I would also squash the second commit, leaving the note in the commit descrption. Specially, you never want to create CreateNewBlockWithScript.
In any case, minor nit, re-utAck.

Member

jtimon commented Apr 16, 2015

I would also squash the second commit, leaving the note in the commit descrption. Specially, you never want to create CreateNewBlockWithScript.
In any case, minor nit, re-utAck.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 16, 2015

Member

Agreed. Squashed.

Member

jonasschnelli commented Apr 16, 2015

Agreed. Squashed.

Show outdated Hide outdated src/validationinterface.h
@@ -35,6 +36,8 @@ class CValidationInterface {
virtual void Inventory(const uint256 &hash) {};
virtual void ResendWalletTransactions(int64_t nBestBlockTime) {};
virtual void BlockChecked(const CBlock&, const CValidationState&) {};
virtual void GetScriptForMining(CScript &script) {};

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member

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

@sipa

sipa Apr 24, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member

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.

@sipa

sipa Apr 24, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member
@sipa

sipa via email Apr 24, 2015

Member

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member
@sipa

sipa via email Apr 24, 2015

Member

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member
@sipa

sipa via email Apr 24, 2015

Member
@@ -558,7 +541,7 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
minerThreads = new boost::thread_group();
for (int i = 0; i < nThreads; i++)
minerThreads->create_thread(boost::bind(&BitcoinMiner, pwallet));
minerThreads->create_thread(boost::bind(&BitcoinMiner, boost::cref(chainparams)));

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member

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.

@sipa

sipa Apr 24, 2015

Member

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.

Show outdated Hide outdated src/wallet/wallet.cpp
void CWallet::GetScriptForMining(CScript &script)
{
CReserveKey reservekey(this);
reservekey.KeepKey();

This comment has been minimized.

@sipa

sipa Apr 24, 2015

Member

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?

@sipa

sipa Apr 24, 2015

Member

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?

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

@jonasschnelli

jonasschnelli Apr 24, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 8, 2015

Member

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.

@jonasschnelli

jonasschnelli May 8, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 15, 2015

Member

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 (#6265).
And this would fix #6268

@jonasschnelli

jonasschnelli Jun 15, 2015

Member

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 (#6265).
And this would fix #6268

This comment has been minimized.

@laanwj

laanwj Jun 17, 2015

Member

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

@laanwj

laanwj Jun 17, 2015

Member

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

This comment has been minimized.

@gavinandresen

gavinandresen Jun 24, 2015

Contributor

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.

@gavinandresen

gavinandresen Jun 24, 2015

Contributor

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 24, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 24, 2015

Member

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 29, 2015

Member

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

Member

jonasschnelli commented Apr 29, 2015

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 30, 2015

Member

Travis is failing for the build without wallet: https://travis-ci.org/bitcoin/bitcoin/jobs/60528797#L1557

Member

jtimon commented Apr 30, 2015

Travis is failing for the build without wallet: https://travis-ci.org/bitcoin/bitcoin/jobs/60528797#L1557

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 30, 2015

Member

@jtimon: Thanks for the report. Fixed.

Member

jonasschnelli commented Apr 30, 2015

@jtimon: Thanks for the report. Fixed.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 30, 2015

Member

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"}

Member

jonasschnelli commented Apr 30, 2015

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"}

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

Rebased.
Seeks testers to escape the rebase hamster wheel.

Member

jonasschnelli commented May 19, 2015

Rebased.
Seeks testers to escape the rebase hamster wheel.

Show outdated Hide outdated src/miner.cpp
CScript coinbaseScript;
GetMainSignals().ScriptForMining(coinbaseScript);
//throw an error if no script was provided

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo May 19, 2015

Can you extend or explain in some more details, how this pull achieves, what it sais in the commit-msg title? Perhaps in the commit-msg itself.

Diapolo commented May 19, 2015

Can you extend or explain in some more details, how this pull achieves, what it sais in the commit-msg title? Perhaps in the commit-msg itself.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

@Diapolo: this PR is a step forward in separating the wallet from the core which can lead to multiple benefits. This would be required to allow a flexible interface (basic modularity) for adding/switching bitcoin-core wallets.

Member

jonasschnelli commented May 19, 2015

@Diapolo: this PR is a step forward in separating the wallet from the core which can lead to multiple benefits. This would be required to allow a flexible interface (basic modularity) for adding/switching bitcoin-core wallets.

Show outdated Hide outdated src/miner.cpp
@@ -573,9 +551,14 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
if (nThreads == 0 || !fGenerate)
return;
CScript coinbaseScript;
GetMainSignals().ScriptForMining(coinbaseScript);

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

Are signals guaranteed to be synchronous?

@luke-jr

luke-jr Jun 2, 2015

Member

Are signals guaranteed to be synchronous?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

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

@luke-jr

luke-jr Jun 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli
@jonasschnelli

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

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

@luke-jr

luke-jr Jun 2, 2015

Member

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

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2015

Member

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

@luke-jr

luke-jr Jun 3, 2015

Member

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

This comment has been minimized.

@sipa

sipa Jun 3, 2015

Member

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

sipa Jun 3, 2015

Member

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

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2015

Member

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

@luke-jr

luke-jr Jun 3, 2015

Member

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

This comment has been minimized.

@sipa

sipa Jun 3, 2015

Member

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.

@sipa

sipa Jun 3, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 3, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 3, 2015

Member

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 14, 2015

Member

Needs rebase.

Member

sipa commented Jun 14, 2015

Needs rebase.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 15, 2015

Member

Rebased.

Member

jonasschnelli commented Jun 15, 2015

Rebased.

@sipa

View changes

Show outdated Hide outdated src/miner.cpp
minerThreads = new boost::thread_group();
for (int i = 0; i < nThreads; i++)
minerThreads->create_thread(boost::bind(&BitcoinMiner, pwallet));
minerThreads->create_thread(boost::bind(&BitcoinMiner, boost::cref(chainparams), coinbaseScript));

This comment has been minimized.

@sipa

sipa Jun 15, 2015

Member

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.

@sipa

sipa Jun 15, 2015

Member

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.

This comment has been minimized.

@Diapolo

Diapolo Jun 15, 2015

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

@Diapolo

Diapolo Jun 15, 2015

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

This comment has been minimized.

@laanwj

laanwj Jun 17, 2015

Member

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.

@laanwj

laanwj Jun 17, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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?

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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?

This comment has been minimized.

@jtimon

jtimon Jun 17, 2015

Member

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

@jtimon

jtimon Jun 17, 2015

Member

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 15, 2015

Member

Untested ACK.

Member

sipa commented Jun 15, 2015

Untested ACK.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jun 15, 2015

Member

re-untestedACK

Member

jtimon commented Jun 15, 2015

re-untestedACK

@laanwj

View changes

Show outdated Hide outdated src/validationinterface.h
@@ -34,6 +35,8 @@ class CValidationInterface {
virtual void Inventory(const uint256 &hash) {}
virtual void ResendWalletTransactions(int64_t nBestBlockTime) {}
virtual void BlockChecked(const CBlock&, const CValidationState&) {}
virtual void GetScriptForMining(CScript &script) {};

This comment has been minimized.

@laanwj

laanwj Jun 17, 2015

Member

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.

@laanwj

laanwj Jun 17, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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.

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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.

This comment has been minimized.

@laanwj

laanwj Jun 17, 2015

Member

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.

@laanwj

laanwj Jun 17, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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.

@jonasschnelli

jonasschnelli Jun 17, 2015

Member

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.

@laanwj

View changes

Show outdated Hide outdated src/wallet/wallet.cpp
@@ -2583,6 +2583,16 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
}
}
void CWallet::GetScriptForMining(CScript &script)
{
CReserveKey reservekey(this);

This comment has been minimized.

@laanwj

laanwj Jun 30, 2015

Member

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.

@laanwj

laanwj Jun 30, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 30, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 30, 2015

Member

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

@laanwj laanwj added the Refactoring label Jun 30, 2015

@laanwj

View changes

Show outdated Hide outdated src/validationinterface.h
@@ -34,6 +36,8 @@ class CValidationInterface {
virtual void Inventory(const uint256 &hash) {}
virtual void ResendWalletTransactions(int64_t nBestBlockTime) {}
virtual void BlockChecked(const CBlock&, const CValidationState&) {}
virtual void GetScriptForMining(boost::shared_ptr<CReserveScript>&) {};
virtual void UpdateRequestCount(const CBlock&) {};

This comment has been minimized.

@laanwj

laanwj Jul 1, 2015

Member

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.

@laanwj

laanwj Jul 1, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 1, 2015

Member

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

@jonasschnelli

jonasschnelli Jul 1, 2015

Member

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

@laanwj

View changes

Show outdated Hide outdated src/script/script.h
{
public:
CScript reserveScript;
virtual void KeepScript() {};

This comment has been minimized.

@laanwj

laanwj Jul 1, 2015

Member

Redundant ;

@laanwj

laanwj Jul 1, 2015

Member

Redundant ;

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 1, 2015

Member

Fixed.

@jonasschnelli

@laanwj laanwj merged commit a7b9623 into bitcoin:master Jul 1, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 1, 2015

Merge pull request #5994
a7b9623 miner: rename UpdateRequestCount signal to ResetRequestCount (Jonas Schnelli)
5496253 add CReserveScript to allow modular script keeping/returning (Jonas Schnelli)
087e65d fix GetScriptForMining() CReserveKey::keepKey() issue (Jonas Schnelli)
d0fc10a detach wallet from miner (Jonas Schnelli)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 1, 2015

Member

ACK

Member

laanwj commented Jul 1, 2015

ACK

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Jul 1, 2015

It seems to me storing a CScript in each mining thread as thread-local storage would be easier/cleaner/less code. See http://www.boost.org/doc/libs/1_58_0/doc/html/thread/thread_local_storage.html

gavinandresen commented on 5496253 Jul 1, 2015

It seems to me storing a CScript in each mining thread as thread-local storage would be easier/cleaner/less code. See http://www.boost.org/doc/libs/1_58_0/doc/html/thread/thread_local_storage.html

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 1, 2015

Owner

Hmm... but using TLS would not remove the need for the new boost::signal (get the CScript from the wallet while not knowing that there are something like a "Wallet") and also not remove the CReserveScript. I don't see where it would reduce code... but i have to admit i don't have any experience with tls.

Owner

jonasschnelli replied Jul 1, 2015

Hmm... but using TLS would not remove the need for the new boost::signal (get the CScript from the wallet while not knowing that there are something like a "Wallet") and also not remove the CReserveScript. I don't see where it would reduce code... but i have to admit i don't have any experience with tls.

jtoomim added a commit to jtoomim/bitcoin that referenced this pull request Feb 9, 2016

Testing infrastructure fixes
New, undocumented-on-purpose -mocktime=timestamp command-line
argument to startup with mocktime set. Needed because
time-related blockchain sanity checks are done on startup, before a
test has a chance to make a setmocktime RPC call.

And changed the setmocktime RPC call so calling it will not result in
currently connected peers being disconnected due to inactivity timeouts.

DOES NOT fix a -regest bug, unlike the version of this commit for 0.11.2:

Blocks generated in -regtest mode would pay to a key that was left
in the keypool, which can cause subtle bugs. In particular, I expected
every block generated to be unique, but got duplicate blocks between
calls to setgenerate.

This was Bitcoin Core issue 6268. It was fixed in PR #5994, so is
unnecessary in 0.12 and later.

Conflicts:
	src/init.cpp
	src/rpcmisc.cpp

zkbot added a commit to zcash/zcash that referenced this pull request Mar 9, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request Mar 10, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 10, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request Mar 12, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 12, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request Mar 12, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 12, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request Mar 12, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

str4d added a commit to str4d/zcash that referenced this pull request Mar 15, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

str4d added a commit to str4d/zcash that referenced this pull request Mar 30, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

str4d added a commit to str4d/zcash that referenced this pull request Apr 4, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 6, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request Apr 13, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

str4d added a commit to str4d/zcash that referenced this pull request May 31, 2018

test: Fetch coinbase address from coinbase UTXOs
After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after
startup does not return the address being used by the miner.

zkbot added a commit to zcash/zcash that referenced this pull request May 31, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment