Head first mining #152
Head first mining #152
Conversation
|
utACK - Great work Gavin! |
|
Excellent! |
|
nice job Gavin! |
Yes, that is the place where you can find the Classic guys to find out more details. |
|
Travis error is use of map::erase method that is C++11; I'll fix. Also I didn't mention how to test: Run ... and you'll get a /tmp/NOTIFY_MSGS that tells you how many seconds between receiving a header and receiving and validating the full block. On my machine, with a debug build, it is a consistent 5 or 6 seconds. So if I was a miner, using this code should drop my orphan rate by about 0.8 to 1 percent. |
gavinandresen
force-pushed
the
headfirst_mining
branch
from
94683a6
to
11d8b05
Compare
Question here. Does this message open up the possibility of a competing miner relaying |
|
@cpacia : good question, @zander asked the same thing. If a peer lies and sends a valid block in an 'invalidblock' message, the peer is marked as misbehaving (and will be kick-banned if it keeps doing it). The valid block IS added to the best chain, though, and will be sent to peers in 'block' messages. Un-asked-for 'invalidblock' messages are ignored (they must be sent in response to 'getdata' requests). A peer could do this: ... but that is no different from sending a 'block' message with random garbage, and is harmless (either the proof-of-work or the merkle root will fail to validate very quickly). |
|
Thanks, makes sense now. |
|
@cpacia : I'll write a regression test to make sure a miner can't tweak a valid block's transaction data and relay it as 'invalidblock' (invalidblock messages that fail either the PoW or have transaction data that don't match the merkle root in the header should be ignored). |
gavinandresen
force-pushed
the
headfirst_mining
branch
from
11d8b05
to
041e3fe
Compare
|
Two more regression tests added for the 'invalidblock' message. The travis error is a spurious timeout; maybe the travis machine is less busy now and all the tests will succeed... |
|
Pretty interesting |
|
ACK |
|
NACK I dont understand the point of recieving the last block header and wasting time mining an empty block waiting for the last full block data in order to mine a non empty block. Or am I just taking crazy pills here? |
|
NACK. I thought Classic was supposed to be an alternative to Core providing greater capacity... mining empty blocks would completely defeat the main purpose for which I thought Classic exists - who would beneft from this change? the miners perhaps, but not the users. The miners are already benefiting from the smaller block sizes - i.e. Core. Classic is supposed to be an alternative where the users are put before the miners, isn't it? |
|
In the beginning I was thinking the same about empty blocks, but someone Head first, instead, is safer than the already widely done SPV mining and ACK 2016-03-17 0:41 GMT+01:00 R E Broadley notifications@github.com:
|
|
@JohnHanks @rebroad The alternative is the miner mines on top of a previous block (essentially working on a fork) which is just going to be orphaned anyway. The purpose is to reduce orphans and level the playing field for small miners. |
|
Is there something here to prevent mining empty block on top of empty block? So if header transmission included claim of transactions or no transactions, then at least for miners following 'rules' we would avoid having 2 blocks in a row that are empty? So headerImEmpty wold not lead to mining on top of that, but headerIHaveTxs would. |
| CNodeState* s = State(pnode->GetId()); | ||
| if (s && !PeerHasHeader(s, pindexBestHeader)) { | ||
| s->pindexBestHeaderSent = pindexBestHeader; | ||
| pnode->PushMessage(NetMsgType::HEADERS, vHeaders); |
There was a problem hiding this comment.
Should we PushMessage to peers in paralllel in order to decrease delay?
|
@gavinandresen Don't forget to pick a new PROTOCOL_VERSION. |
|
utACK. As I know not matter bitcoind is support header-first mining or not, most pools are doing the work beside bitcoind. They patched bitcoind to get notification like '-blockheadernotify' or listen to other pool's stratum server to get the new height job and make empty block util they received full block. |
|
@gdassori No, it wouldn't be 30 seconds, it would be 10 minutes as the bitcoin network always adjusts to maintain an average of 1 block every 10 minutes. So, for example, ten empty blocks in 2 weeks might look like it only wasted 5 minutes of time, but it will cause the difficulty to rise such that those ten blocks add 100 minutes to the next 2 weeks worth of blocks (assuming no change in mining power), so overall, 100 minutes of network usefulness is lost due to those 10 empty blocks. |
|
@cpacia You are correct-ish... this is assuming the header received has transactions still to arrive. The problem, if I understand correctly, is that we're allowing mining of empty blocks where the previous block was also empty, thereby getting empty blocks in the main chain. Yes, allow mining on headers, but ONLY when the previous block wasn't empty. This would be a far better solution, and would still allow mining on headers, but would stop empty blocks getting into the main chain by miners using this code. |
|
@rebroad: when you do not have the new block, if you don't mine on his header you have to shutdown the miners. |
|
@rebroad keep it about the code here, we asked politely to move the rest of the talk elsewhere. See my previous message. |
|
I don't know if this is correct or not but I remember reading that CGminer and/or BFGminer have/had something that will not allowed them to switch back to block they already mined... |
| @@ -147,6 +148,8 @@ class CBlockIndex | |||
|
|
|||
| //! (memory only) Sequential id assigned to distinguish order in which blocks are received. | |||
| uint32_t nSequenceId; | |||
| //! (memory only) The time this node first heard about blocks | |||
| static std::map<const CBlockIndex*, int64_t> firstSeenTime; | |||
There was a problem hiding this comment.
Not a bad idea. But I think that this means that the destructor of CBlockIndex needs to be introduced that removes itself from this map to avoid a memory leak.
There was a problem hiding this comment.
You may also want to either delete the copy constructor and assignment operator, or implement them. If not, the copies won't have the firstSeenTime information.
|
@zander I'm not sure I understand what you mean; my comments are purely prompted by this pull request and code within. It would make more efficient use of the blockchain if the average time between blocks remains fixed. |
|
I may have misunderstood this pull request.. if it already is coded to avoid empty blocks going into the main chain, then ACK, e.g. if after receiving an empty block (or header) the txs haven't been seen, then I totally approve/ACK dropping back to the previous block. Anyway, it's quite simply a scaling issue IMHO, and given Classic aims to improve scalability, I'd expect the developers to be in favour of not encouraging empty blocks. |
Reading your answers I suggest https://www.reddit.com/r/btc/comments/4aogb9/head_first_mining_by_gavinandresen_pull_request/ Ask your questions there, general information and ideas about approaches and concepts make much more sense to discuss on a forum or similar. Please. |
|
get it all out on the table here. forget reddit @zander if there are issues with codes' pull requests @rebroad has every right to discuss it here. follow up to my previous post: if you can detect using the header that the block is empty or not or check the utxo pool in order to not waste time mining empty blocks when in fact txs need to commit in to the chain then ACK |
gavinandresen
force-pushed
the
headfirst_mining
branch
from
7bed462
to
17fb018
Compare
|
Added a CBlockIndex destructor as suggested by @zander , and squashed it into the first commit. @rebroad @JohnHanks : empty blocks are not a capacity problem, please see my comment here for why: https://www.reddit.com/r/btc/comments/4aogb9/head_first_mining_by_gavinandresen_pull_request/d12gmir |
| @@ -327,13 +327,11 @@ static UniValue BIP22ValidationResult(const CValidationState& state) | |||
| // they are the same, they are different only in the time between receiving a | |||
| // block header and receiving and then fully validating the block with | |||
| // all its transactions. | |||
| static CBlockIndex* GetBuildTip(CBlockIndex* validated_tip, CBlockIndex* header_tip) | |||
| static CBlockIndex* GetBuildTip(int64_t nHeadFirst, CBlockIndex* validated_tip, CBlockIndex* header_tip) | |||
There was a problem hiding this comment.
Change nHeadFirst to nHeadFirstTimeout, a better name?
|
I had some time to do some testing of the networking part of this: Test setup: 5 servers connected in a chain: NJ USA - London UK - Tokyo JP - Sydney AU - LA USA - NJ USA Start and end of the chain are on different bitcoind's running on the same server, with the new -logtimemicros option set to get accurate microsecond timing (all servers are running ntp, but I only report/compare times at the beginning/end of the chain, which are on the same machine so times are directly comparable). $10/month Vultr.com VPS servers (binaries compiled on a $20/month Vultr.com VPS server with 2GB memory). RESULTS for git HEAD April 4 a914968: RESULTS for head-first networking change: That is what I expected; latency is dropped almost in half, because a request-response round-trip is avoided after first networking hop. I'll test with full (1MB and 2MB) blocks next-- hopefully tomorrow, but that might not happen until next week (I'm traveling again). |
Replaces CBlockIndex::nSequenceId with the time a block is first seen, and moves it from the CBlockIndex structure to a separate map (to save memory). Fully validated blocks are still preferred to un- or partially validated blocks.
Refactor: have CreateNewBlock take transactions from a passed-in memory pool object instead of the global mempool object, and build on a passed-in chain tip.
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.
Mine empty blocks on top of valid proof-of-work block headers (for at most 30 seconds, to prevent block data withholding attacks)
When we get block header(s) with more proof-of-work than our current tip, send them immediately to peers that we don't think already have them. And if we receive an invalid block for one of those more-proof-of-work headers, tell peers about it immediately with a new 'invalidblock' message.
This makes head-first mining opt-in. To get empty
blocks built on validated headers, pass:
{ "mode" : "template", "headfirst" : 30 }
... in the getblocktemplate call. The argument
to "headfirst" is the maximum number of seconds
getblocktemplate will return an empty block between
receiving and validating the block header and receiving
and validating the full block data.
A new parameter is added to the getblocktemplate result
object:
"previousfullyvalidated" : true|false
... which will be false if the template returned is an
empty block built on one or more headers.
gavinandresen
force-pushed
the
headfirst_mining
branch
from
d19721e
to
91eefd2
Compare
|
I'm going to close this, reorganize the commits a bit, and re-open as two separate pull requests: One that just optimizes the network relay code, and adds -blockheadernotify And a second that teaches getblocktemplate to mine empty blocks on headers if the miner so desires. |
…atch/remove_obsolete_rpc-tests_shell_script remove rpc-tests.sh which was replaced by Python script
Implement "head first mining" : propagate new 80-byte block headers as quickly as possible across the network, and give miners the opportunity to start mining an empty block as soon as they hear about the block header.
Only valid proof-of-work headers that connect to the most-work chain are relayed, so there are no feasible denial-of-service attacks.
Miners will switch to mining a regular block as soon as the full block is received and validated.
There is a hard-coded 30-second timeout; if the full block data takes longer than 30 seconds to get validated and propagated across the network, or is never sent, miners switch back to mining non-empty blocks on the last fully-validated block.
Details:
New getblocktemplate behavior: getblocktemplate returns an empty (just coinbase transaction) block template in the time between receiving a new block's 'headers' message and receiving and validating the 'block' message. Long-polling getblocktemplate connections are interrupted when a new best-header is received and when blocks are fully validated.
New command-line option, -blockheadernotify. Just like -blocknotify, but notified when a new most-work-header is received.
New p2p message, 'invalidblock'. Just like the 'block' message, but sent to peers when an invalid block that has valid proof-of-work is received, to tell them they should stop mining on the block's header.
Only peers that follow the BIP130 'sendheaders' protocol will be relayed headers immediately, and only they will receive 'invalidblock' messages. SPV clients don't 'sendheaders', so should be unaffected. Older clients that don't understand the 'invalidblock' message will ignore it.
Please limit discussion here on github to code review and testing results. Debate on whether or not head-first mining is a benefit to the network should happen in the bitcoin-classic #debate slack channel and/or the #bitcoin-dev IRC channel.