multi: Decouple orphan handling from blockchain.

This decouples and removes the orphan handling from blockchain in favor
of implementing it in the block manager as part of the overall effort to
decouple the connection code from the download logic.

The change might not make a ton of sense in isolation, since there is no
major functional change, however, decoupling the orphan handling
independently helps make the review process easier and alleviates what
would otherwise result in additional intermediate code to handle cases
that ultimately will no longer exist.

The following is a high level overview of the changes:

- Introduce blockchain function to more easily determine if an error is
  a rule error with a given error code
- Move core orphan handling code from blockchain to block manager
  - Move data structures used to cache and track orphan blocks
  - Move all functions releated to orphans
    - BlockChain.IsKnownOrphan -> blockManager.isKnownOrphan
    - BlockChain.GetOrphanRoot -> blockManager.orphanRoot
    - BlockChain.removeOrphanBlock -> blockManager.removeOrphanBlock
    - BlockChain.addOrphanBlock -> blockManager.addOrphanBlock
- Implement orphan handling in block manager
  - Rework to use the moved functions and data structs
  - Add check for known orphans in addition HaveBlock calls to retain
    the same behavior
  - Modify the semantics of the process block func exposed by the block
    manager so that it no longer stores orphans since all consumers of
    it ultimately reject orphans anyway
- Remove remaining orphan related code from blockchain
  - Update ProcessBlock to return an error when called with an orphan
  - Remove additional orphan processing from ProcessBlock
  - Remove orphan cache check from HaveBlock
  - Adjust example to account for the removed parameter
  - Change chaingen harness tests to detect orphans via returned error
  - Modify fullblock tests to detect orphans via returned error
  - Adapt process order logic tests to cope with lack of orphan
    processing
  - Update all other tests accordingly
  - Update various comments and the README.md and doc.go to properly
    reflect the removal of orphan handling

blockchain/README.md

@@ -37,10 +37,6 @@ is by no means exhaustive:
    transaction amounts, script complexity, and merkle root calculations
  - Compare the block against predetermined checkpoints for expected timestamps
    and difficulty based on elapsed time since the checkpoint
- - Save the most recent orphan blocks for a limited time in case their parent
-   blocks become available
- - Stop processing if the block is an orphan as the rest of the processing
-   depends on the block's position within the block chain
  - Perform a series of more thorough checks that depend on the block's position
    within the block chain such as verifying block difficulties adhere to
    difficulty retarget rules, timestamps are after the median of the last

blockchain/chain.go

@@ -26,10 +26,6 @@ import (
 )
 
 const (
-	// maxOrphanBlocks is the maximum number of orphan blocks that can be
-	// queued.
-	maxOrphanBlocks = 500
-
 	// minMemoryNodes is the minimum number of consecutive nodes needed
 	// in memory in order to perform all necessary validation.  It is used
 	// to determine when it's safe to prune nodes from memory without
@@ -77,14 +73,6 @@ func panicf(format string, args ...interface{}) {
 // [17a 16a 15 14 13 12 11 10 9 8 7 6 4 genesis]
 type BlockLocator []*chainhash.Hash
 
-// orphanBlock represents a block that we don't yet have the parent for.  It
-// is a normal block plus an expiration time to prevent caching the orphan
-// forever.
-type orphanBlock struct {
-	block      *dcrutil.Block
-	expiration time.Time
-}
-
 // BestState houses information about the current best block and other info
 // related to the state of the main chain as it exists from the point of view of
 // the current best block.
@@ -138,10 +126,10 @@ func newBestState(node *blockNode, blockSize, numTxns, totalTxns uint64,
 	}
 }
 
-// BlockChain provides functions for working with the Decred block chain.
-// It includes functionality such as rejecting duplicate blocks, ensuring blocks
-// follow all rules, orphan handling, checkpoint handling, and best chain
-// selection with reorganization.
+// BlockChain provides functions for working with the Decred block chain.  It
+// includes functionality such as rejecting duplicate blocks, ensuring blocks
+// follow all rules, checkpoint handling, and best chain selection with
+// reorganization.
 type BlockChain struct {
 	// The following fields are set when the instance is created and can't
 	// be changed afterwards, so there is no need to protect them with a
@@ -181,13 +169,6 @@ type BlockChain struct {
 	index     *blockIndex
 	bestChain *chainView
 
-	// These fields are related to handling of orphan blocks.  They are
-	// protected by a combination of the chain lock and the orphan lock.
-	orphanLock   sync.RWMutex
-	orphans      map[chainhash.Hash]*orphanBlock
-	prevOrphans  map[chainhash.Hash][]*orphanBlock
-	oldestOrphan *orphanBlock
-
 	// The block cache for main chain blocks to facilitate faster chain reorgs
 	// and more efficient recent block serving.
 	mainChainBlockCacheLock sync.RWMutex
@@ -360,11 +341,11 @@ func (b *BlockChain) DisableVerify(disable bool) {
 
 // HaveBlock returns whether or not the chain instance has the block represented
 // by the passed hash.  This includes checking the various places a block can
-// be like part of the main chain, on a side chain, or in the orphan pool.
+// be like part of the main chain or on a side chain.
 //
 // This function is safe for concurrent access.
 func (b *BlockChain) HaveBlock(hash *chainhash.Hash) bool {
-	return b.index.HaveBlock(hash) || b.IsKnownOrphan(hash)
+	return b.index.HaveBlock(hash)
 }
 
 // ChainWork returns the total work up to and including the block of the
@@ -378,136 +359,6 @@ func (b *BlockChain) ChainWork(hash *chainhash.Hash) (*big.Int, error) {
 	return node.workSum, nil
 }
 
-// IsKnownOrphan returns whether the passed hash is currently a known orphan.
-// Keep in mind that only a limited number of orphans are held onto for a
-// limited amount of time, so this function must not be used as an absolute
-// way to test if a block is an orphan block.  A full block (as opposed to just
-// its hash) must be passed to ProcessBlock for that purpose.  However, calling
-// ProcessBlock with an orphan that already exists results in an error, so this
-// function provides a mechanism for a caller to intelligently detect *recent*
-// duplicate orphans and react accordingly.
-//
-// This function is safe for concurrent access.
-func (b *BlockChain) IsKnownOrphan(hash *chainhash.Hash) bool {
-	// Protect concurrent access.  Using a read lock only so multiple
-	// readers can query without blocking each other.
-	b.orphanLock.RLock()
-	_, exists := b.orphans[*hash]
-	b.orphanLock.RUnlock()
-
-	return exists
-}
-
-// GetOrphanRoot returns the head of the chain for the provided hash from the
-// map of orphan blocks.
-//
-// This function is safe for concurrent access.
-func (b *BlockChain) GetOrphanRoot(hash *chainhash.Hash) *chainhash.Hash {
-	// Protect concurrent access.  Using a read lock only so multiple
-	// readers can query without blocking each other.
-	b.orphanLock.RLock()
-	defer b.orphanLock.RUnlock()
-
-	// Keep looping while the parent of each orphaned block is
-	// known and is an orphan itself.
-	orphanRoot := hash
-	prevHash := hash
-	for {
-		orphan, exists := b.orphans[*prevHash]
-		if !exists {
-			break
-		}
-		orphanRoot = prevHash
-		prevHash = &orphan.block.MsgBlock().Header.PrevBlock
-	}
-
-	return orphanRoot
-}
-
-// removeOrphanBlock removes the passed orphan block from the orphan pool and
-// previous orphan index.
-func (b *BlockChain) removeOrphanBlock(orphan *orphanBlock) {
-	// Protect concurrent access.
-	b.orphanLock.Lock()
-	defer b.orphanLock.Unlock()
-
-	// Remove the orphan block from the orphan pool.
-	orphanHash := orphan.block.Hash()
-	delete(b.orphans, *orphanHash)
-
-	// Remove the reference from the previous orphan index too.  An indexing
-	// for loop is intentionally used over a range here as range does not
-	// reevaluate the slice on each iteration nor does it adjust the index
-	// for the modified slice.
-	prevHash := &orphan.block.MsgBlock().Header.PrevBlock
-	orphans := b.prevOrphans[*prevHash]
-	for i := 0; i < len(orphans); i++ {
-		hash := orphans[i].block.Hash()
-		if hash.IsEqual(orphanHash) {
-			copy(orphans[i:], orphans[i+1:])
-			orphans[len(orphans)-1] = nil
-			orphans = orphans[:len(orphans)-1]
-			i--
-		}
-	}
-	b.prevOrphans[*prevHash] = orphans
-
-	// Remove the map entry altogether if there are no longer any orphans
-	// which depend on the parent hash.
-	if len(b.prevOrphans[*prevHash]) == 0 {
-		delete(b.prevOrphans, *prevHash)
-	}
-}
-
-// addOrphanBlock adds the passed block (which is already determined to be
-// an orphan prior calling this function) to the orphan pool.  It lazily cleans
-// up any expired blocks so a separate cleanup poller doesn't need to be run.
-// It also imposes a maximum limit on the number of outstanding orphan
-// blocks and will remove the oldest received orphan block if the limit is
-// exceeded.
-func (b *BlockChain) addOrphanBlock(block *dcrutil.Block) {
-	// Remove expired orphan blocks.
-	for _, oBlock := range b.orphans {
-		if time.Now().After(oBlock.expiration) {
-			b.removeOrphanBlock(oBlock)
-			continue
-		}
-
-		// Update the oldest orphan block pointer so it can be discarded
-		// in case the orphan pool fills up.
-		if b.oldestOrphan == nil ||
-			oBlock.expiration.Before(b.oldestOrphan.expiration) {
-			b.oldestOrphan = oBlock
-		}
-	}
-
-	// Limit orphan blocks to prevent memory exhaustion.
-	if len(b.orphans)+1 > maxOrphanBlocks {
-		// Remove the oldest orphan to make room for the new one.
-		b.removeOrphanBlock(b.oldestOrphan)
-		b.oldestOrphan = nil
-	}
-
-	// Protect concurrent access.  This is intentionally done here instead
-	// of near the top since removeOrphanBlock does its own locking and
-	// the range iterator is not invalidated by removing map entries.
-	b.orphanLock.Lock()
-	defer b.orphanLock.Unlock()
-
-	// Insert the block into the orphan map with an expiration time
-	// 1 hour from now.
-	expiration := time.Now().Add(time.Hour)
-	oBlock := &orphanBlock{
-		block:      block,
-		expiration: expiration,
-	}
-	b.orphans[*block.Hash()] = oBlock
-
-	// Add to previous hash lookup index for faster dependency lookups.
-	prevHash := &block.MsgBlock().Header.PrevBlock
-	b.prevOrphans[*prevHash] = append(b.prevOrphans[*prevHash], oBlock)
-}
-
 // TipGeneration returns the entire generation of blocks stemming from the
 // parent of the current tip.
 //
@@ -570,14 +421,6 @@ func (b *BlockChain) fetchBlockByNode(node *blockNode) (*dcrutil.Block, error) {
 		return block, nil
 	}
 
-	// Check orphan cache.
-	b.orphanLock.RLock()
-	orphan, existsOrphans := b.orphans[node.hash]
-	b.orphanLock.RUnlock()
-	if existsOrphans {
-		return orphan.block, nil
-	}
-
 	// Load the block from the database.
 	err := b.db.View(func(dbTx database.Tx) error {
 		var err error
@@ -2163,8 +2006,6 @@ func New(ctx context.Context, config *Config) (*BlockChain, error) {
 		subsidyCache:                  subsidyCache,
 		index:                         newBlockIndex(config.DB),
 		bestChain:                     newChainView(nil),
-		orphans:                       make(map[chainhash.Hash]*orphanBlock),
-		prevOrphans:                   make(map[chainhash.Hash][]*orphanBlock),
 		mainChainBlockCache:           make(map[chainhash.Hash]*dcrutil.Block),
 		deploymentCaches:              newThresholdCaches(params),
 		isVoterMajorityVersionCache:   make(map[[stakeMajorityCacheKeySize]byte]bool),

blockchain/chain_test.go

@@ -101,7 +101,7 @@ func TestBlockchainFunctions(t *testing.T) {
 			t.Errorf("NewBlockFromBytes error: %v", err.Error())
 		}
 
-		_, _, err = chain.ProcessBlock(bl, BFNone)
+		_, err = chain.ProcessBlock(bl, BFNone)
 		if err != nil {
 			t.Fatalf("ProcessBlock error at height %v: %v", i, err.Error())
 		}

blockchain/common_test.go

@@ -343,25 +343,20 @@ func (g *chaingenHarness) AcceptBlock(blockName string) {
 	g.t.Logf("Testing block %s (hash %s, height %d)", blockName, block.Hash(),
 		blockHeight)
 
-	forkLen, isOrphan, err := g.chain.ProcessBlock(block, BFNone)
+	forkLen, err := g.chain.ProcessBlock(block, BFNone)
 	if err != nil {
 		g.t.Fatalf("block %q (hash %s, height %d) should have been accepted: %v",
 			blockName, block.Hash(), blockHeight, err)
 	}
 
-	// Ensure the main chain and orphan flags match the values specified in the
-	// test.
-	isMainChain := !isOrphan && forkLen == 0
+	// Ensure the block was accepted to the main chain as indicated by a fork
+	// length of zero.
+	isMainChain := forkLen == 0
 	if !isMainChain {
 		g.t.Fatalf("block %q (hash %s, height %d) unexpected main chain flag "+
 			"-- got %v, want true", blockName, block.Hash(), blockHeight,
 			isMainChain)
 	}
-	if isOrphan {
-		g.t.Fatalf("block %q (hash %s, height %d) unexpected orphan flag -- "+
-			"got %v, want false", blockName, block.Hash(), blockHeight,
-			isOrphan)
-	}
 }
 
 // AcceptTipBlock processes the current tip block associated with the harness
@@ -383,7 +378,7 @@ func (g *chaingenHarness) RejectBlock(blockName string, code ErrorCode) {
 	g.t.Logf("Testing block %s (hash %s, height %d)", blockName, block.Hash(),
 		blockHeight)
 
-	_, _, err := g.chain.ProcessBlock(block, BFNone)
+	_, err := g.chain.ProcessBlock(block, BFNone)
 	if err == nil {
 		g.t.Fatalf("block %q (hash %s, height %d) should not have been accepted",
 			blockName, block.Hash(), blockHeight)
@@ -440,25 +435,20 @@ func (g *chaingenHarness) AcceptedToSideChainWithExpectedTip(tipName string) {
 	g.t.Logf("Testing block %s (hash %s, height %d)", g.TipName(), block.Hash(),
 		blockHeight)
 
-	forkLen, isOrphan, err := g.chain.ProcessBlock(block, BFNone)
+	forkLen, err := g.chain.ProcessBlock(block, BFNone)
 	if err != nil {
 		g.t.Fatalf("block %q (hash %s, height %d) should have been accepted: %v",
 			g.TipName(), block.Hash(), blockHeight, err)
 	}
 
-	// Ensure the main chain and orphan flags match the values specified in
-	// the test.
-	isMainChain := !isOrphan && forkLen == 0
+	// Ensure the block was accepted to a side chain as indicated by a non-zero
+	// fork length.
+	isMainChain := forkLen == 0
 	if isMainChain {
 		g.t.Fatalf("block %q (hash %s, height %d) unexpected main chain flag "+
 			"-- got %v, want false", g.TipName(), block.Hash(), blockHeight,
 			isMainChain)
 	}
-	if isOrphan {
-		g.t.Fatalf("block %q (hash %s, height %d) unexpected orphan flag -- "+
-			"got %v, want false", g.TipName(), block.Hash(), blockHeight,
-			isOrphan)
-	}
 
 	g.ExpectTip(tipName)
 }

blockchain/doc.go

@@ -15,13 +15,12 @@ extremely important that fully validating nodes agree on all rules.
 At a high level, this package provides support for inserting new blocks into the
 block chain according to the aforementioned rules.  It includes functionality
 such as rejecting duplicate blocks, ensuring blocks and transactions follow all
-rules, orphan handling, and best chain selection along with reorganization.
+rules, and best chain selection along with reorganization.
 
 Since this package does not deal with other Decred specifics such as network
 communication or wallets, it provides a notification system which gives the
 caller a high level of flexibility in how they want to react to certain events
-such as orphan blocks which need their parents requested and newly connected
-main chain blocks which might result in wallet updates.
+such as newly connected main chain blocks which might result in wallet updates.
 
 Decred Chain Processing Overview
 
@@ -36,10 +35,6 @@ is by no means exhaustive:
    transaction amounts, script complexity, and merkle root calculations
  - Compare the block against predetermined checkpoints for expected timestamps
    and difficulty based on elapsed time since the checkpoint
- - Save the most recent orphan blocks for a limited time in case their parent
-   blocks become available
- - Stop processing if the block is an orphan as the rest of the processing
-   depends on the block's position within the block chain
  - Perform a series of more thorough checks that depend on the block's position
    within the block chain such as verifying block difficulties adhere to
    difficulty retarget rules, timestamps are after the median of the last

blockchain/error.go

@@ -634,3 +634,10 @@ func (e RuleError) Error() string {
 func ruleError(c ErrorCode, desc string) RuleError {
 	return RuleError{ErrorCode: c, Description: desc}
 }
+
+// IsErrorCode returns whether or not the provided error is a rule error with
+// the provided error code.
+func IsErrorCode(err error, c ErrorCode) bool {
+	e, ok := err.(RuleError)
+	return ok && e.ErrorCode == c
+}