Make orphan ntfns to a return val on ProcessBlock.

This commit changes the way that orphan blocks are identified by adding a
new boolean return value on ProcessBlock and removing the notification for
NTOrphanBlock.

This allows the calling code to identify orphan blocks immediately instead
of having to setup a seperate callback handler and implementing some type
of state tracking.  This, in turn, allows cleaner code for handling them.

In addition, the tests have been updated for the new function signature
and also now check that each block is or is not an orphan as expected
which makes the tests more robust.

ok @jrick

chain_test.go

@@ -48,18 +48,29 @@ func TestHaveBlock(t *testing.T) {
 	btcchain.TstSetCoinbaseMaturity(1)
 
 	for i := 1; i < len(blocks); i++ {
-		err = chain.ProcessBlock(blocks[i], false)
+		isOrphan, err := chain.ProcessBlock(blocks[i], false)
 		if err != nil {
 			t.Errorf("ProcessBlock fail on block %v: %v\n", i, err)
 			return
 		}
+		if isOrphan {
+			t.Errorf("ProcessBlock incorrectly returned block %v "+
+				"is an orphan\n", i)
+			return
+		}
 	}
 
 	// Insert an orphan block.
-	if err := chain.ProcessBlock(btcutil.NewBlock(&Block100000), false); err != nil {
+	isOrphan, err := chain.ProcessBlock(btcutil.NewBlock(&Block100000), false)
+	if err != nil {
 		t.Errorf("Unable to process block: %v", err)
 		return
 	}
+	if !isOrphan {
+		t.Errorf("ProcessBlock indicated block is an not orphan when " +
+			"it should be\n")
+		return
+	}
 
 	tests := []struct {
 		hash string

doc.go

@@ -112,7 +112,7 @@ intentionally causes an error by attempting to process a duplicate block.
 		// Process a block.  For this example, we are going to intentionally
 		// cause an error by trying to process the genesis block which already
 		// exists.
-		err = chain.ProcessBlock(genesisBlock, false)
+		_, err = chain.ProcessBlock(genesisBlock, false)
 		if err != nil {
 			fmt.Printf("Failed to process block: %v\n", err)
 			return

notifications.go

@@ -17,16 +17,10 @@ type NotificationCallback func(*Notification)
 
 // Constants for the type of a notification message.
 const (
-	// NTOrphanBlock indicates an orphan block was processed and the
-	// associated block hash should be passed to the GetOrphanRoot function
-	// to find the root of all known orphans which should then be used to
-	// request the missing blocks.
-	NTOrphanBlock NotificationType = iota
-
 	// NTBlockAccepted indicates the associated block was accepted into
 	// the block chain.  Note that this does not necessarily mean it was
 	// added to the main chain.  For that, use NTBlockConnected.
-	NTBlockAccepted
+	NTBlockAccepted NotificationType = iota
 
 	// NTBlockConnected indicates the associated block was connected to the
 	// main chain.
@@ -40,7 +34,6 @@ const (
 // notificationTypeStrings is a map of notification types back to their constant
 // names for pretty printing.
 var notificationTypeStrings = map[NotificationType]string{
-	NTOrphanBlock:       "NTOrphanBlock",
 	NTBlockAccepted:     "NTBlockAccepted",
 	NTBlockConnected:    "NTBlockConnected",
 	NTBlockDisconnected: "NTBlockDisconnected",
@@ -57,7 +50,6 @@ func (n NotificationType) String() string {
 // Notification defines notification that is sent to the caller via the callback
 // function provided during the call to New and consists of a notification type
 // as well as associated data that depends on the type as follows:
-// 	- NTOrphanBlock:       *btcwire.ShaHash
 // 	- NTBlockAccepted:     *btcutil.Block
 // 	- NTBlockConnected:    *btcutil.Block
 // 	- NTBlockDisconnected: *btcutil.Block

process.go

@@ -81,29 +81,33 @@ func (b *BlockChain) processOrphans(hash *btcwire.ShaHash) error {
 // the block chain.  It includes functionality such as rejecting duplicate
 // blocks, ensuring blocks follow all rules, orphan handling, and insertion into
 // the block chain along with best chain selection and reorganization.
-func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
+//
+// It returns a bool which indicates whether or not the block is an orphan and
+// any errors that occurred during processing.  The returned bool is only valid
+// when the error is nil.
+func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) (bool, error) {
 	blockHash, err := block.Sha()
 	if err != nil {
-		return err
+		return false, err
 	}
 	log.Tracef("Processing block %v", blockHash)
 
 	// The block must not already exist in the main chain or side chains.
 	if b.blockExists(blockHash) {
 		str := fmt.Sprintf("already have block %v", blockHash)
-		return ruleError(ErrDuplicateBlock, str)
+		return false, ruleError(ErrDuplicateBlock, str)
 	}
 
 	// The block must not already exist as an orphan.
 	if _, exists := b.orphans[*blockHash]; exists {
 		str := fmt.Sprintf("already have block (orphan) %v", blockHash)
-		return ruleError(ErrDuplicateBlock, str)
+		return false, ruleError(ErrDuplicateBlock, str)
 	}
 
 	// Perform preliminary sanity checks on the block and its transactions.
 	err = CheckBlockSanity(block, b.netParams.PowLimit)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	// Find the previous checkpoint and perform some additional checks based
@@ -115,7 +119,7 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
 	blockHeader := &block.MsgBlock().Header
 	checkpointBlock, err := b.findPreviousCheckpoint()
 	if err != nil {
-		return err
+		return false, err
 	}
 	if checkpointBlock != nil {
 		// Ensure the block timestamp is after the checkpoint timestamp.
@@ -125,7 +129,7 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
 			str := fmt.Sprintf("block %v has timestamp %v before "+
 				"last checkpoint timestamp %v", blockHash,
 				blockHeader.Timestamp, checkpointTime)
-			return ruleError(ErrTimeTooOld, str)
+			return false, ruleError(ErrTimeTooOld, str)
 		}
 		if !fastAdd {
 			// Even though the checks prior to now have already ensured the
@@ -142,7 +146,7 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
 				str := fmt.Sprintf("block target difficulty of %064x "+
 					"is too low when compared to the previous "+
 					"checkpoint", currentTarget)
-				return ruleError(ErrDifficultyTooLow, str)
+				return false, ruleError(ErrDifficultyTooLow, str)
 			}
 		}
 	}
@@ -155,16 +159,14 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
 			prevHash)
 		b.addOrphanBlock(block)
 
-		// Notify the caller so it can request missing blocks.
+		return true, nil
-		b.sendNotification(NTOrphanBlock, blockHash)
-		return nil
 	}
 
 	// The block has passed all context independent checks and appears sane
 	// enough to potentially accept it into the block chain.
 	err = b.maybeAcceptBlock(block, fastAdd)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	// Accept any orphan blocks that depend on this block (they are no
@@ -172,9 +174,9 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, fastAdd bool) error {
 	// no more.
 	err = b.processOrphans(blockHash)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	log.Debugf("Accepted block %v", blockHash)
-	return nil
+	return false, nil
 }

reorganization_test.go

@@ -57,12 +57,17 @@ func TestReorganization(t *testing.T) {
 	chain.DisableCheckpoints(true)
 	btcchain.TstSetCoinbaseMaturity(1)
 
+	expectedOrphans := map[int]bool{5: true, 6: true}
 	for i := 1; i < len(blocks); i++ {
-		err = chain.ProcessBlock(blocks[i], false)
+		isOrphan, err := chain.ProcessBlock(blocks[i], false)
 		if err != nil {
 			t.Errorf("ProcessBlock fail on block %v: %v\n", i, err)
 			return
 		}
+		if isOrphan && !expectedOrphans[i] {
+			t.Errorf("ProcessBlock incorrectly returned block %v "+
+				"is an orphan\n", i)
+		}
 	}
 
 	return