Skip to content
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

Headfirst mining #138

Merged
merged 5 commits into from Mar 9, 2016

Conversation

Projects
None yet
9 participants
@gavinandresen
Copy link

commented Mar 8, 2016

Implement head-first mining: have getblocktemplate build an empty block on the most-work block header while the full block is being fetched/validated.

It will do this for at most 30 seconds, to prevent a 'create a valid proof-of-work block then withhold its data to stop the network processing transactions' attack. If it takes longer than 30 seconds to download/validate the block data, then the previous fully-validated block is mined.

Adds a new command-line option: -blockheadernotify Just like -blocknotify, only notify as soon as a new best-work header is received and validated.

This should be easier to review commit-by-commit.

Tested by running the existing sendheaders.py and getblocktemplate_longpoll.py regression tests, and also running with -blocknotify='/tmp/notify.sh block %s' -blockheadernotify='/tmp/notify.sh header %s'

... where /tmp/notify.sh is this simple little script:

#!/usr/bin/env bash

echo $@ >> /tmp/NOTIFY_MSGS
/Users/gavin/src/classic/src/bitcoin-cli getblocktemplate | head -20 >> /tmp/NOTIFY_MSGS

After running on the main network for a while, /tmp/NOTIFY_MSGS should show getblocktemplate giving an empty-transaction template after a header, then a few seconds later a normal template when the new block is fully validated.

gavinandresen added some commits Mar 8, 2016

Keep track of time block header is first seen
Extends CBlockIndex with a firstSeen member to keep track
of the local time when a block header is first received.
Explicitly pass memory pool to CreateNewBlock
Refactor: have CreateNewBlock take transactions from a passed-in
memory pool object instead of the global mempool object.
Refactor getblocktemplate to prep for mining on best header
This is a do-nothing refactor that just replaces calls to
chainActive.Tip() and direct use of the global mempool object
with build_tip and a reference to either the mempool or an
empty mempool.
getblocktemplate : head-first mining
Mine empty blocks on top of valid proof-of-work block headers
(for at most 30 seconds, to prevent block data withholding attacks)
@ChainQuery

This comment has been minimized.

Copy link

commented Mar 8, 2016

utACK particularly because this helps level the playing field for P2Pool folks :)

@sandakersmann

This comment has been minimized.

Copy link

commented Mar 8, 2016

Concept ACK

@Polve

This comment has been minimized.

Copy link

commented Mar 8, 2016

Since 30s can be a "long" or "short" time depending on the speed of the CPU and the quality of the Internet link, shouldn't maxEmptyTime be a configurable option?

@jameshilliard

This comment has been minimized.

Copy link

commented Mar 8, 2016

You probably want to incorporate a verification flag into this for risk mitigation.

@NanoAkron

This comment has been minimized.

Copy link

commented Mar 9, 2016

Are you combining this with the thin blocks approach?

In which case, you could set a more simple determinant limit that is not dependent on CPU, for example:

"if >= x% of the transactions within the block are not already in my mempool, then fetching would take too long and I should just start building on the previously validated block"

@instagibbs

This comment has been minimized.

Copy link

commented Mar 9, 2016

I agree with @jameshilliard , a verification flag would greatly reduce the risk profile of this.

@zander zander merged this pull request into bitcoinclassic:develop Mar 9, 2016

1 check passed

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

This comment has been minimized.

Copy link

commented Mar 10, 2016

Merged? closed? A bit confusing ...

@shangzhou

This comment has been minimized.

Copy link

commented Mar 10, 2016

"...major bug in the headfirst mining code ,I knew I should've written a unit test first..."
scary! and @gavinandresen why not just comment here ? Shame on you

@zander

This comment has been minimized.

Copy link

commented Mar 10, 2016

why not just comment here ?

The best way to follow the work of the classic team is in slack, as you have noticed.

Github makes for a really bad experience as a social platform, it is meant only for code reviews and this merge request was closed because it wasn't good enough yet. A new one will come when its better.

Please use social platforms like reddit, various forums or slack to discuss non-coding subjects.

Cheers!

@shangzhou

This comment has been minimized.

Copy link

commented Mar 10, 2016

"Github makes for a really bad experience as a social platform"
This is not relevant
I think "# major bug in the headfirst mining code ,I knew I should've written a unit test first" should be stated here first by @gavinandresen himself if he is an professional contributor.

@zander

This comment has been minimized.

Copy link

commented Mar 10, 2016

Bitcoin Classic operates completely in the open, this means that code gets developed in the open and features that are very rough and unfinished get seen by lots of people long long before they are made ready for release.

Please respect the process to show the beautiful end results only after the hard work that goes into it, which indeed includes mistakes, bugs and naturally corrections.

This process has over many years shown to be superior and we will continue to do this.

@bitcoinclassic bitcoinclassic locked and limited conversation to collaborators Mar 10, 2016

@gavinandresen gavinandresen deleted the gavinandresen:headfirst_mining branch Mar 10, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.