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

Remove internal miner #7507

Merged
merged 1 commit into from Mar 14, 2016

Conversation

Projects
None yet
10 participants
@Leviathn

Leviathn commented Feb 11, 2016

This code removes the internal miner which is only useful on Testnet.
This leaves the internal miner that is useful on RegTest intact.

Leviathn
Remove internal miner
This code removes the internal miner which is only useful on Testnet.
This leaves the internal miner that is useful on RegTest intact.
@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Feb 11, 2016

Contributor

ACK 8d1de43

Contributor

TheBlueMatt commented Feb 11, 2016

ACK 8d1de43

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

Why? Having a builtin miner was tremendously useful when experimenting with segnet.

Member

sipa commented Feb 11, 2016

Why? Having a builtin miner was tremendously useful when experimenting with segnet.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Feb 11, 2016

Contributor

ACK. But this was useful when testing Segnet.

I guess this marks the official end of an era? Haha.

Contributor

dcousens commented Feb 11, 2016

ACK. But this was useful when testing Segnet.

I guess this marks the official end of an era? Haha.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Feb 11, 2016

Contributor

@sipa Meh. Its really trivial to go git clone some CPU miner and point it to bitcoind...we dont need an entirely-unmaintained CPU miner in the codebase when we have a separate (?!) codepath for mining on regtest. If someone feels like merging this codepath and regtest, I'd be more happy, but having two separate built-in miners seems like overkill.

Contributor

TheBlueMatt commented Feb 11, 2016

@sipa Meh. Its really trivial to go git clone some CPU miner and point it to bitcoind...we dont need an entirely-unmaintained CPU miner in the codebase when we have a separate (?!) codepath for mining on regtest. If someone feels like merging this codepath and regtest, I'd be more happy, but having two separate built-in miners seems like overkill.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

Feel free to go merge the codepaths then, or add a simple python based RPC miner instead. But I disagree with not being able to actually create valid blocks at all.

Member

sipa commented Feb 11, 2016

Feel free to go merge the codepaths then, or add a simple python based RPC miner instead. But I disagree with not being able to actually create valid blocks at all.

@wtogami

This comment has been minimized.

Show comment
Hide comment
@wtogami

wtogami Feb 11, 2016

Contributor

https://github.com/pooler/cpuminer
Much faster than the internal miner. Supports GBT.

Contributor

wtogami commented Feb 11, 2016

https://github.com/pooler/cpuminer
Much faster than the internal miner. Supports GBT.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 11, 2016

Member

BFGMiner also has CPU mining support FWIW.

Member

luke-jr commented Feb 11, 2016

BFGMiner also has CPU mining support FWIW.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

Well at the very least, remove the restriction that generate can't be called on anything but regtest, and make it not stick in an infinite loop if no match is found after 2^32 attempts.

Member

sipa commented Feb 11, 2016

Well at the very least, remove the restriction that generate can't be called on anything but regtest, and make it not stick in an infinite loop if no match is found after 2^32 attempts.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

I agree with removing the duplicated code path, by the way. We wanted to do that when the regtest-specific behaviour of setgenerate was factored out into its own RPC, but we didn't want to make large changes at the time.

Member

sipa commented Feb 11, 2016

I agree with removing the duplicated code path, by the way. We wanted to do that when the regtest-specific behaviour of setgenerate was factored out into its own RPC, but we didn't want to make large changes at the time.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Feb 11, 2016

Contributor

Tested ACK

8d1de43

Contributor

pstratem commented Feb 11, 2016

Tested ACK

8d1de43

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 11, 2016

Contributor

ACK Leviathn@8d1de43
Great diffstat statistics, BTW ;-)

Contributor

paveljanik commented Feb 11, 2016

ACK Leviathn@8d1de43
Great diffstat statistics, BTW ;-)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 11, 2016

Contributor

NITS: please remove some git grep BitcoinMiner occurrences:

doc/developer-notes.md:- BitcoinMiner : Generates bitcoins (if wallet is enabled).
src/miner.cpp:// BitcoinMiner

Few more lines to be deleted...

Contributor

paveljanik commented Feb 11, 2016

NITS: please remove some git grep BitcoinMiner occurrences:

doc/developer-notes.md:- BitcoinMiner : Generates bitcoins (if wallet is enabled).
src/miner.cpp:// BitcoinMiner

Few more lines to be deleted...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

I tend to agree with this on the longer term.

However, before this can be merged, there needs to be an easy to compile and set up CPU miner (let's say a "reference miner"). As well as instructions in doc/ for setting this up instead (as migration path for people using the internal miner).

Last time this was proposed, that was far from the case. I tried various project that were proposed to be as alternative but they were professional miner tools with lots of setup options, or a tangled web of dependencies.

So that's the condition.

Member

laanwj commented Feb 11, 2016

I tend to agree with this on the longer term.

However, before this can be merged, there needs to be an easy to compile and set up CPU miner (let's say a "reference miner"). As well as instructions in doc/ for setting this up instead (as migration path for people using the internal miner).

Last time this was proposed, that was far from the case. I tried various project that were proposed to be as alternative but they were professional miner tools with lots of setup options, or a tangled web of dependencies.

So that's the condition.

@laanwj laanwj added the Mining label Feb 11, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 11, 2016

Member

I argued pretty vigorously against this before in that I feel that Bitcoin Core should be "complete"-- that sending people off to weave together many programs to build a functioning system was not in the sincere spirit of decentralization. Today testnet is not really cpu minable and we have the regtest miner. So I'm increasingly feeling this is a principled distinction with no practical impact.

I think Wladimir's call for an easy to setup cpu miner and clear docs is good; and perhaps the best remediation possible given the facts we can't control.

Member

gmaxwell commented Feb 11, 2016

I argued pretty vigorously against this before in that I feel that Bitcoin Core should be "complete"-- that sending people off to weave together many programs to build a functioning system was not in the sincere spirit of decentralization. Today testnet is not really cpu minable and we have the regtest miner. So I'm increasingly feeling this is a principled distinction with no practical impact.

I think Wladimir's call for an easy to setup cpu miner and clear docs is good; and perhaps the best remediation possible given the facts we can't control.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 11, 2016

Member
./configure --enable-cpumining
make
./bfgminer
Member

luke-jr commented Feb 11, 2016

./configure --enable-cpumining
make
./bfgminer
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

@luke-jr As a reference miner, that's like swatting a fly with a thermonuclear missile. Checking it out is like downloading an operating system. I'd prefer something minimal and self-contained that people can more easily study.

Member

laanwj commented Feb 11, 2016

@luke-jr As a reference miner, that's like swatting a fly with a thermonuclear missile. Checking it out is like downloading an operating system. I'd prefer something minimal and self-contained that people can more easily study.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

So if you would:

  • Keep deleting all code that this PR currently deletes
  • Remove the restriction that the generate RPC only works for regtest
  • Fix the bug that generate will go into an infinite loop when it needs to check more than 2^32 hashes, or even better add a timeout and allow it to fail.

That means that instead of bitcoin-cli setgenerate true, you could use while true; do bitcoin-cli generate 1; done, with nearly no loss of functionality.

Member

sipa commented Feb 11, 2016

So if you would:

  • Keep deleting all code that this PR currently deletes
  • Remove the restriction that the generate RPC only works for regtest
  • Fix the bug that generate will go into an infinite loop when it needs to check more than 2^32 hashes, or even better add a timeout and allow it to fail.

That means that instead of bitcoin-cli setgenerate true, you could use while true; do bitcoin-cli generate 1; done, with nearly no loss of functionality.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Feb 11, 2016

Contributor

@sipa that seems reasonable, but changing the behavior of the generate rpc probably belongs in a new pr

Contributor

pstratem commented Feb 11, 2016

@sipa that seems reasonable, but changing the behavior of the generate rpc probably belongs in a new pr

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 12, 2016

Member

I'm not sure about @sipa's solution. It is elegant because it removes duplicated code, but on the other hand I dislike a RPC call that can hang for a long time. This holds up a (precious) RPC thread instead of a dedicated mining thread. With a timeout (or "max iter count") it would certainly be acceptable to me though.

But as you would effectively still need an external script to mine testnet, to keep calling generate after every timeout, instead of just pass -gen=1 and forget. I'm not convinced that this is better than a completely external reference miner. Easier to implement, sure.

Edit: I've talked with @sipa on IRC and he convinced me that this is the way to go. Our 'reference miner' (for testnet) will just be a python script that calls generate in a loop.

Member

laanwj commented Feb 12, 2016

I'm not sure about @sipa's solution. It is elegant because it removes duplicated code, but on the other hand I dislike a RPC call that can hang for a long time. This holds up a (precious) RPC thread instead of a dedicated mining thread. With a timeout (or "max iter count") it would certainly be acceptable to me though.

But as you would effectively still need an external script to mine testnet, to keep calling generate after every timeout, instead of just pass -gen=1 and forget. I'm not convinced that this is better than a completely external reference miner. Easier to implement, sure.

Edit: I've talked with @sipa on IRC and he convinced me that this is the way to go. Our 'reference miner' (for testnet) will just be a python script that calls generate in a loop.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 6, 2016

Member

@Leviathn are you going to move this forward according to sipa's comment?

Member

laanwj commented Mar 6, 2016

@Leviathn are you going to move this forward according to sipa's comment?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 9, 2016

Member

See #7663.

Member

sipa commented Mar 9, 2016

See #7663.

@laanwj laanwj merged commit 8d1de43 into bitcoin:master Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 14, 2016

Merge #7507: Remove internal miner
8d1de43 Remove internal miner (Leviathn)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#7507: Remove internal miner
8d1de43 Remove internal miner (Leviathn)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#7507: Remove internal miner
8d1de43 Remove internal miner (Leviathn)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge bitcoin#7507: Remove internal miner
8d1de43 Remove internal miner (Leviathn)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge bitcoin#7507: Remove internal miner
8d1de43 Remove internal miner (Leviathn)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment