- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.5k
 
blockchain, netsync: implement a complete headers-first download during ibd #2428
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
base: master
Are you sure you want to change the base?
blockchain, netsync: implement a complete headers-first download during ibd #2428
Conversation
We add a chainview of bestHeaders so that we'll be able to keep track of headers separately from the bestChain. This is needed as we're getting headers for new block annoucements instead of invs.
Since we may now have blockNodes with just the block header stored without the data, we add a new status to account for this.
          Pull Request Test Coverage Report for Build 18000176133Details
 
 
 
 💛 - Coveralls | 
    
2308a1e    to
    3519720      
    Compare
  
    maybeAcceptHeader performs checks to accept block headers into the header chain. This function allows for a true headers-first download where we only accpet block headers for downloading new blocks.
ProcessBlockHeader performs chain selection and context-free & contextual validation for the given block header. The function allows a header-first downloading of blocks even without checkpoints.
On flushes to the database, we check that the blockNodes we have for the download block headers are not flushed to the disk unless the block data is stored as well for backwards compatibility. With older btcd clients, they rely on the fact that the blockNode is present to check if the block data is also present. Since we now store blockNodes for just the block headers, this is no longer true. Because of this, we don't flush the blockNodes if there's no accompanying block data for it. This results in the downloading and verifying the headers again if the node were to restart but since the header data is small and the verification is quick, it's not a big downside.
In block processing and block downloading, HaveBlock is used to check if the block data already exists. It was ok to just check for the existence of the blockNode but we now we also need to check if the data exists as the blockNode may be present for just the block header.
3519720    to
    f48f8bf      
    Compare
  
    The added exported methods on BlockChain provide access to the block header tip like fetching block hashes and heights from the headers chain.
cf683a9    to
    242fa3b      
    Compare
  
    checkHeadersList takes in a blockhash and returns if it's a checkpointed block and the correct behavior flags for the verification of the block.
fetchHigherPeers provides a convenient function to get peers that are sync candidates and are at a higher advertised height than the passed in height.
isInIBDMode returns if the SyncManager needs to download blocks and sync to the latest chain tip. It determines if it's in ibd mode by checking if the blockchain thinks we're current and if we don't have peers that are at higher advertised blocks.
fetchHeaders picks a random peer at a higher advertised block and requests headers from them.
Instead of the old headerList based header processing, we make use of the new ProcessBlockHeader function.
headers Since we now utilize ProcessBlockHeader, we change the fetchHeaderBlocks to be based off of the block index instead of the headerList in SyncManager.
ince we now utilize ProcessBlockHeaders, we change the startSync function to utilize the block index for downloading blocks/headers instead of using the headerList in SyncManager.
handleBlockMsg is updated to not be based off of the headerList and sm.nextCheckpoint when doing operations related to checkpoints and headers.
Since we no longer utilize the headerList for doing headers-first download and checkpoint tracking, we remove related code.
ibdMode is a more fitting name than headersFirstMode since all blocks are downloaded headers-first during the initial block download.
242fa3b    to
    e6fadc9      
    Compare
  
    | 
           cc: @gijswijs for review  | 
    
| 
           cc: @mohamedawnallah for review  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great set of changes! I found the commit structure very easy to follow as well.
Have performed an initial review pass, but will start to run some active tests on one of my nodes to get a better feel for the changes.
| } | ||
| 
               | 
          ||
| // HaveHeader returns whether the header data is stored in the database. | ||
| func (status blockStatus) HaveHeader() bool { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just the inverse of HaveData?
| MerkleRoot: merkle, | ||
| Bits: chainParams.PowLimitBits, | ||
| Timestamp: ts, | ||
| Nonce: 0, // To be solved. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous comment.
| lastHeader := headers[len(headers)-1] | ||
| lastHeaderHash := lastHeader.BlockHash() | ||
| tipNode := chain.bestHeader.Tip() | ||
| if !tipNode.hash.IsEqual(&lastHeaderHash) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the require package for all these assertions.
| } | ||
| 
               | 
          ||
| // Create invalid header at the checkpoint. | ||
| // | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: blank line, same below.
| lastSidechainHeader := sidechainExtendingHeaders[lastSideChainHeaderIdx] | ||
| lastSidechainHeaderHash := lastSidechainHeader.BlockHash() | ||
| 
               | 
          ||
| // Check that the tip is now different. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| "properly connect to the chain from peer %s "+ | ||
| "-- disconnecting", peer.Addr()) | ||
| _, err := sm.chain.ProcessBlockHeader( | ||
| blockHeader, blockchain.BFNone, false) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: close paren should go on the line below:
		_, err := sm.chain.ProcessBlockHeader(
			blockHeader, blockchain.BFNone, false,
          )| // that is already in the database and is only used to ensure | ||
| // the next header links properly, it must be removed before | ||
| // fetching the blocks. | ||
| sm.headerList.Remove(sm.headerList.Front()) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the header list all together now? Or is it still needed for re-orgs?
| // If we're in the initial block download mode, check if we have | ||
| // peers that we can download headers from. | ||
| _, bestHeaderHeight := sm.chain.BestHeader() | ||
| higherHeaderPeers := sm.fetchHigherPeers(bestHeaderHeight) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though many years have passed at this point, we should retain the logic that we removed to filter out non-segwit peers once segwit is active.
| // If we're on a checkpointed block, check if we still have checkpoints | ||
| // to let the user know if we're switching to normal mode. | ||
| if isCheckpointBlock { | ||
| log.Infof("on checkpoint block %v(%v)", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log suggestion:
Continuing IBD, on check point block %v(%v)
| headersFirstMode bool | ||
| headerList *list.List | ||
| startHeader *list.Element | ||
| nextCheckpoint *chaincfg.Checkpoint | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Change Description
Right now the headers-first download is only based off of the checkpoints and is thus limited to the last checkpoint.
The newly implemented headers-first download will always download headers-first and will validate them to see if the headers connect and have proper proof of work.
Then the block download will be based off of the verified headers. This now eliminates any potential downloading of txs or orphan blocks during ibd. It also makes future parallel block download much better as a parallel block download can only happen for blocks we already have headers for.
It's not yet put into the code yet but this allows the node to also receive block headers instead of invs during block propagation.I'll do this in a follow up later.Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.