blockchain: Decouple processing and download logic.

[davecgh]

This completely reworks the way block index and processing works to use headers-first semantics and support out of order processing of the associated block data.

This will ultimately provide a wide variety of benefits as it means the entire shape of the block tree can be determined from the headers alone which in turn will allow much more informed decisions to be made, provides better information regarding sync status and warning conditions, and allows future work to add invalidation and reconsideration of arbitrary blocks.

It must be noted that this is a major change to the way the block index and block handling is done and is deeply integrated with the consensus rules, so it will need a significant amount of testing and extremely careful review. To that end, the changes have been split up into several commits to help ease review, but, unfortunately the vast majority of them really couldn't be reasonably split, so the bulk of hardest to review changes are in a single commit.

Also of note is that there are a lot of assumptions made in the calling code in regards to expected error attribution and the state the chain has at the moment notifications are processed, so all of those semantics have intentionally been retained, even though they might seem a bit out of place now, in order to limit the amount of changes introduced at a time and thus better ensures correctness.

For example, in the future it would probably make more sense to make the notifications entirely asynchronous and for validation failures to be reported via those asynchronous notifications. However, before that can happen, all callers would first need to be updated to ensure they do not rely on the chain being in the same state it was at the moment the notification was generated.

Another significant change is that the logic that is used to determine if the chain believes it is current is now more robust and takes advantage of the fact the best known header is now available.

In particular, it introduces two new constraints:

• A latching mechanism that prevents the chain from flipping back to being non current unless no new blocks have been seen for an extended period of time
• The chain only latches to current once it is synced to the header with the most cumulative work that is not known to be invalid in addition to the existing conditions

Finally, in addition to all of the existing full block tests, this introduces a comprehensive set of processing order tests which exercise all of the new logic.

High level overview of the changes:

• Introduce tracking for whether a block is fully linked, meaning it builds on a branch that has block data for all of its ancestors
• Add received order tracking to ensure miners are not able to gain an advantage in terms of chain selection by only advertising a header
• Add several new pieces of information to the block index:
  • Header with the most cumulative work that is not known to be invalid
  • Header with the most cumulative work that is known to be invalid
  • Map of best chain candidates to aid in efficient selection
  • Map of unlinked children for more efficient linking
  • Prunable cached chain tips to significantly reduce potential search space during invalidation
• Introduce a compare function which determines which of two nodes should be considered better for the purposes of best chain selection
• Add chain tip iteration capabilities with potential filtering
• Mark all descendants invalid due to known invalid ancestor when a block is marked invalid
• Add ability to determine if a block can currently be validated
• Rework the chain reorganization func to work with an arbitrary target
• Modify the overall chain reorg func to reorg to the block with the most cumulative work that is valid in the case it is not possible to reorg to the target block
• Add a new MultiError type for keeping track of multiple errors in the same call
• Add a new ErrNoBlockData error kind
• Update all cases that only require all of the ancestor block data to be available to check for that condition instead of the more strict condition of already being fully validated
• Introduce a new processing lock separate from the overall chain lock
• Add new method and ability to independently accept block headers
• Rework the block processing func to accept blocks out of order so long as their header is already known valid
• Keep a cache of blocks that recently passed contextual validation checks
• Cleanup and correct various comments to match reality
• Rework chain current logic to improve robustness and take advantage new best known header availability
  • Adds a latching mechanism that prevents the chain from flipping back to being non current unless no new blocks have been seen for an extended period of time
  • Only latches the chain to current once it is synced to the header with the most cumulative work that is not known to be invalid in addition to the existing conditions
• Add comprehensive tests to exercise the new processing logic and best header tracking (for both invalid and not known invalid)
  • Support query block test name by hash in the testing
  • Improve test harness logging
  • Add separate test block generation and acceptance
• Update README.md and doc.go to accommodate the changes

---

This is work towards #1145.

[davecgh]

Oh, one other thing I forgot to call out is that the NTBlockAccepted notifications are now delivered at the same time for all connected blocks as opposed to interspersed with every block as it's processed. This is acceptable because all of the information the code that uses the notification require is available in the notification.

In order to increase confidence in the notifications in general, I also added some logging for them as they're sent and compared the before and after of the full block tests as follows:

$ cat testfullblocks.txt | grep NT | cut -d' ' -f2 | sort | uniq -c
    204 NTBlockAccepted
    210 NTBlockConnected
     20 NTBlockDisconnected
     12 NTChainReorgDone
     12 NTChainReorgStarted
    179 NTNewTickets
    226 NTNewTipBlockChecked
      7 NTReorganization
    179 NTSpentAndMissedTickets

$ cat testfullblocksnew.txt | grep NT | cut -d' ' -f2 | sort | uniq -c
    204 NTBlockAccepted
    210 NTBlockConnected
     20 NTBlockDisconnected
     12 NTChainReorgDone
     12 NTChainReorgStarted
    179 NTNewTickets
    226 NTNewTipBlockChecked
      7 NTReorganization
    179 NTSpentAndMissedTickets

As can be seen the same number of and type of notifications are being produced. Closer inspection at the logs shows they are produced in the same order as expected as well.

[davecgh]

Comment typos.

[rstaudt2]

Got through the tests so far and called out a few typos. The process tests read very well and are super helpful for visualizing all of the various scenarios this takes into account!

[JoeGruffins]

Awesome.

On testnet there is a lot of output. One warning a few times [WRN] FEES: Trying to process mined transactions at block 587476 when previous best block was at height 587476 at different heights. Also this info [INF] CHAN: FORK: Block 0000002b64225f709a777cbb2acdb011705d8f43bbf32888eeed8476c5c4ecf1 (height 587489) forks the chain at height 587488/block 0000009a0b2f93131a56d24b5151337ebcdd259515b0ef9cf204e44469d01636, but does not cause a reorganize numerous times with different values for the same block. I don't see either of those logs on mainnet, so wondering why such a difference.

All expected?

I also observed a successful reorg happen on testnet.

I am unable to use wallet with the new rpc api version yet, so unable to test sending transactions and such easily.

[davecgh]

On testnet there is a lot of output. One warning a few times [WRN] FEES: Trying to process mined transactions at block 587476 when previous best block was at height 587476 at different heights. Also this info [INF] CHAN: FORK: Block 0000002b64225f709a777cbb2acdb011705d8f43bbf32888eeed8476c5c4ecf1 (height 587489) forks the chain at height 587488/block 0000009a0b2f93131a56d24b5151337ebcdd259515b0ef9cf204e44469d01636, but does not cause a reorganize numerous times with different values for the same block. I don't see either of those logs on mainnet, so wondering why such a difference.

All expected?

Yes, testnet has a lot of competing blocks show up around the time the difficulty drops. Mainnet does not have a difficulty reduction like that, so it doesn't happen there.

The fees warning is also normal. It happens for all forked blocks. It probably doesn't actually need to be a warning, but the behavior is the same on master.

[matheusd]

Finished the first pass review. Aside from the minor issues, looks good. Will do another general pass after the duplicate positional checks issue is addressed. I also ran the sync a bunch of times in mainnet.

Just wanna say, this really mustn't have been fun to write, since it involves such a critical piece of the processing code for the blockchain. Definitely was hard to review, since the individual checks are all interdependent in the critical commit.

OTOH, the resulting decoupled processing is a very nice thing to achieve :P

[matheusd]

Also, just wanna hightlight this is an awesome test :P

[davecgh]

... the "Verified checkpoint at height XXX" is duplicated.

EDIT: I tracked the duplicate print down and it's because the positional block header checks are being run twice. It's not a big deal, but I'll get this updated to avoid doing that.

This has been resolved. I refactored the checks which apply to the block data to a separate function and call it instead when accepting the block data since the headers have already been checked at that point.

View Changes.

[matheusd]

Did a second pass review of the code (as of now), taking note of the final structure for ProcessBlock()/ProcessBlockHeader() for the most important sub-calls and some of the checks performed. Here it is:

chain.ProcessBlockHeader()
  * chain.maybeAcceptBlockHeader(checkHeaderSanity == true)
    * checkBlockHeaderSanity()
      - checks PoW
    * chain.checkBlockHeaderPositional()
      - diff retargeting
      - correct height
    * index.AddNode()
      - create or extend header tip
      - maybe change best header
    
chain.ProcessBlock()
  # (block header check stage)
  * checkBlockSanity()
    * checkBlockHeaderSanity()
      - checks PoW
    * checkProofOfStake
    - checks block size < wire.MaxBlockPayload
    * checkTransactionSanity() 					# (regular then stake txs)
    - checks total ticket and revocation # in header
  * chain.maybeAcceptBlockHeader(checkHeaderSanity == false)
    * chain.checkBlockHeaderPositional()
      - checks diff retargeting
      - checks correct height
    * index.AddNode()
      - create or extend header tip
      - maybe change best header
      
  # (block data check stage)
  * chain.maybeAcceptBlockData()    
    * chain.checkBlockDataPositional()
      - checks for expired txs
    * index.AcceptBlockData()
  * chain.maybeAcceptBlocks()
    * chain.checkBlockContext()
      * chain.checkBlockHeaderContext()
        - checks stakediff in header 
        - checks poolsize in header
      * checkTransactionContext()         			# (repeated for every tx and stx)
      - checks stake tx counts limits
      - checks finalized txs
      - checks count sig ops
      - checks tspends only in TVI blocks
      
  # (best chain selection stage)
  * chain.reorganizeChain()
    * chain.reorganizeChainInternal()     			# (repeated until a valid tip is found)
      * utxos.disconnectBlock()					# (when reorging to a different branch)
      * chain.disconnectBlock()
      * chain.checkBlockContext()         			# (if not done yet for some block - see previous call)
      * chain.checkConnectBlock()         			# (called for every block in the new branch)
        * chain.tspendChecks()
          - checks tspend in correct block
          * chain.checkTSpendHasVotes()
          * chain.checkTSPendsExpenditure()
        * utxos.disconnectDisapprovedBlock() 			# (if block votes to disapprove parent block)
        * chain.checkTransactionsAndConnect() 			# (first stake, then regular txs)
        * checkBlockScripts() 					# (first stake, then regular txs)
        * blockOneCoinbasePaysTokens()
      * chain.connectBlock()              			# (called for every block that passes the above)
        * dbPut****()						# (store best state, utxo set, treasury, gcs filter)
        - update in-memory best state
        - send block connected & tickets ntnfs
      * index.RemoveLessWorkCandidates
    * chain.maybeUpdateIsCurrent()

[matheusd]

Also ran the sync again a few times and executed the test suite on every commit.

Woop!

[davecgh]

I updated this to ensure the index lock is not held during flushing in case pruned ticket information has to be reloaded which itself requires the index lock and to add a couple of trace logging statements for connected and disconnected blocks.

Also, rebased.

[sefbkn]

This PR looks solid overall. Thank you for taking the time to write this along with clear comments and tests. I am still working through it, but have not seen any issues with the latest code thusfar.

—

On a mostly unrelated note with respect to this PR, while syncing I did notice a log message that appears incorrect. It cropped up while testing this PR so I figured it worth mentioning at least even though it's not coming from this set of changes.

[WRN] PEER: Misbehaving peer xxx:9108 (outbound): 0 transactions not found -- ban score increased to 80
...
[INF] SRVR: Banned peer xxx (outbound) for 24h0m0s

In server.go -> OnNotFound, the following line

		reason := fmt.Sprintf("%d %v not found", numBlocks, txStr)

seems like it should be

		reason := fmt.Sprintf("%d %v not found", numTxns, txStr)

[davecgh]

Thanks to everyone for reviewing this. I know it was not a trivial one to review and took a lot of effort.