Split headers-only versions off CheckBlock/AcceptBlock #3884
Conversation
|
This doesn't change the actual behaviour of the pre-existing functions, correct? So, eg, unit test assumptions remain valid.. |
|
A better summary of the changes:
In a later pull request, the orphan storage will be removed entirely, as blocks can be stored with just headers validated but not transactions (potentially resulting in out-of-order block storagre). |
|
Please take your time, and ask for clarification as necessary. |
|
There is a bug in this code, which prevents syncing. Closing until fixed. |
|
\o/ segfault |
|
Bug fixed. @luke-jr I don't think any assumptions that are used in unit tests change. The only real change is that now CBlockIndex entries in the block tree may exist that do not have actual block data associated with them. |
|
I did a review, comments ended up on the origin commit rather than here - oops. Overall looks good, just would appreciate some more comments and helper methods in a few places to make the logic easier to follow and reduce the chances of typos with all the bit twiddling. |
There was a problem hiding this comment.
Shouldn't these be constants perhaps in CChainParams?
There was a problem hiding this comment.
Agree, but let's do that in another PR.
|
There seems to be some bug on the connection logic in this pull. I got a node stuck after it got shutdown during syncing. After launching it again, all the remaining blocks got marked as orphans, and it never resumed syncing. Shutting down and launching again several times didn't help. |
|
@rdponticelli With a recent version of this code (less than 8 days old?) |
|
@sipa Yes, last version. Culprit seems to be some kind of race condition with the low disk space code. Some logs: 2014-04-16 14:18:49 UpdateTip: new best=0000000000000000937bdca5485b8a02da4cbfea74fe982903a2da1ba8537866 height=295933 log2_work=77.988128 tx=36873027 date=2014-04-15 07:18:51 progress=0.993805 After that all the chain is detected as orphan: 2014-04-16 14:58:04 Initialization result: 1 |
|
Running the code in master in this state I got: 2014-04-16 17:10:31 init message: Done loading And after that the new code again: 2014-04-16 17:45:19 init message: Done loading And it's still going like this.... |
|
@rdponticelli Thanks for testing! So the problem was that if the block data failed to write, we ended up with a block header in the index, but no corresponding block data in the block file. At next startup, this would mean it would need to download it again, but the block download logic can't deal with the in-index-but-not-on-disk case yet, which will be added later. I've (hopefully) fixed it by only writing the block header to the index after the block data is present. |
|
Now I'm getting this on another node. It seems that it's breaking when it changes the block file: 2014-04-17 19:02:56 ProcessBlock: ACCEPTED |
|
Just seeing orphans while syncing is perfectly expected (not intentional though, but it will require more to fix that). |
|
Rebased and addressed @mikehearn's comments. |
Also modify some connection logic to deal with non-full blocks in the index.
|
Rebased. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/942b33a19d3aa96326acc044c1834c48beff777c for binaries and test log. |
|
@gavinandresen How does this look to you? |
|
Tested this on the Dogecoin blockchain. Both initial sync and keeping up with the blockchain worked as normal, both restarting the client and waking up from hibernation worked fine and I didn't spot anything unusual in the logs. |
|
I'm currently testing this. It's still incredibly slow due to the peer selection for the block download. I really think it should take the speed of download into consideration for selecting a peer, and I still don't understand why it selects only one peer - why can't it select several? After all, most download tools these days choose to download from multiple sources to reduce the load on any one source. |
|
@rebroad This is an incremental reorganization. All that stuff you are talking about was implemented months ago in sipa's headers first branch but it was too risky a change to take at once and wasn't seeing adequate testing. This pull (and others) are breaking up the steps to move to it incrementally in a way which is easier to verify. |
|
@gmaxwell yes, I know. The stuff I am talking about hasn't been implemented yet. This is why I am mentioning it. |
Pulling from multiple peers and taking speed into account were both implemented in the headers first branch. (actually it does something which is usually better than take speed into account it tracks when peers are stalling the process and kicks out the straggler(s)). |
|
@rebroad Selecting multiple peers to download from would just bring duplicate block downloads back, unless it happens in a coordinated way. No such coordination exists, because we don't know in advance which peers have which blocks - we just download what they tell us they have. Nobody claims we can't download from multiple peers at once - that is one of the goals of headers-first synchronization (which this is incremental work towards, but it will take a few more pull requests before we're actually there). |
|
How do we decide when there's been enough testing and review? Seems like the path forward here isn't clear. |
|
@mikehearn What part is unclear? This is tagged '0.10' so will be merged after the branch-off of 0.9.2. |
|
This may be a discussion for the mailing list, but I'm not sure it's healthy to keep pull requests that are "done" queued up where they have to be constantly rebased, waiting to enter master, missing out on testing that people might do. If you want to have a branch for stabilisation, why not branch for 0.9.2 now and cherry pick fixes into it until you're confident it's ready? |
|
I know that - the plan is to split off 0.9.2 this week. |
|
Okie dokie. |
|
@gmaxwell ah, so it's not in the master branch? Am I able to submit pull requests to the branch you mention? What is the purpose of this branch and will it as some point be merged into master? My main reason for mentioning these concerns is that the current master as it is doesn't work well, and gets 300 or so orphan blocks piling up without catching up on the blockchain, often requiring a restart of bitcoind to get it downloading again, at which point the hundreds of orphan blocks already downloaded are then lost (since they are not saved to disk). It's also frustrating to see this, considering I raised a pull request about 2 years ago which downloaded blocks far faster and more efficiently than it currently does, and which did detect stuck downloads, unlike the current master branch. |
|
@rebroad Gregory talking about my personal headers-first branch (#2964), |
10 participants
There is a small semantic change here, namely that blocks with invalid transaction data (eg duplicate hashes, non-first coinbase, ...) will have their headers accepted into the tree (but never considered for connecting).
That's an inevitable consequence of moving towards headers-first, as we'll do header connectivity checkinng before transaction checking anyway (the transaction data won't be available until later, so we rather do more extensive checking on the headers that are available first).
This has very low DoS risk, as it still requires faking PoW.