[NOD-376] Fix invalid orphan blocks causing their valid parents to be rejected (#436)

* [NOD-376] Made bad unorphaned blocks not reject the original block.

* [NOD-376] Fix wording in a comment.

* [NOD-376] Add a test to make sure that bad child blocks don't invalidate valid parent blocks.

* [NOD-376] Clarify comments and don't check PoW for child block (it's irrelevant for this test case).

blockdag/process.go

@@ -112,7 +112,12 @@ func (dag *BlockDAG) processOrphans(hash *daghash.Hash, flags BehaviorFlags) err
 			// Potentially accept the block into the block DAG.
 			err = dag.maybeAcceptBlock(orphan.block, flags|BFWasUnorphaned)
 			if err != nil {
-				return err
+				// Since we don't want to reject the original block because of
+				// a bad unorphaned child, only return an error if it's not a RuleError.
+				if _, ok := err.(RuleError); !ok {
+					return err
+				}
+				log.Warnf("Verification failed for orphan block %s: %s", orphanHash, err)
 			}
 
 			// Add this block to the list of blocks to process so

blockdag/process_test.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"github.com/daglabs/btcd/dagconfig"
 	"github.com/daglabs/btcd/util"
+	"github.com/daglabs/btcd/util/daghash"
+	"path/filepath"
 	"testing"
 	"time"
 )
@@ -65,3 +67,65 @@ func TestProcessBlock(t *testing.T) {
 		t.Errorf("ProcessBlock: Expected error \"%s\" but got \"%s\"", expectedErrMsg, err)
 	}
 }
+
+func TestProcessOrphans(t *testing.T) {
+	dag, teardownFunc, err := DAGSetup("TestProcessOrphans", Config{
+		DAGParams: &dagconfig.SimNetParams,
+	})
+	if err != nil {
+		t.Errorf("Failed to setup dag instance: %v", err)
+		return
+	}
+	defer teardownFunc()
+
+	dag.TestSetCoinbaseMaturity(0)
+
+	blocksFile := "blk_0_to_4.dat"
+	blocks, err := LoadBlocks(filepath.Join("testdata/", blocksFile))
+	if err != nil {
+		t.Fatalf("TestProcessOrphans: "+
+			"Error loading file '%s': %s\n", blocksFile, err)
+	}
+
+	// Get a reference to a parent block
+	parentBlock := blocks[1]
+
+	// Get a reference to a child block and mess with it so that:
+	// a. It gets added to the orphan pool
+	// b. It gets rejected once it's unorphaned
+	childBlock := blocks[2]
+	childBlock.MsgBlock().Header.UTXOCommitment = &daghash.ZeroHash
+
+	// Process the child block so that it gets added to the orphan pool
+	isOrphan, delay, err := dag.ProcessBlock(childBlock, BFNoPoWCheck)
+	if err != nil {
+		t.Fatalf("TestProcessOrphans: child block unexpectedly returned an error: %s", err)
+	}
+	if delay != 0 {
+		t.Fatalf("TestProcessOrphans: child block is too far in the future")
+	}
+	if !isOrphan {
+		t.Fatalf("TestProcessOrphans: incorrectly returned that child block is not an orphan")
+	}
+
+	// Process the parent block. Note that this will attempt to unorphan the child block
+	isOrphan, delay, err = dag.ProcessBlock(parentBlock, BFNone)
+	if err != nil {
+		t.Fatalf("TestProcessOrphans: parent block unexpectedly returned an error: %s", err)
+	}
+	if delay != 0 {
+		t.Fatalf("TestProcessOrphans: parent block is too far in the future")
+	}
+	if isOrphan {
+		t.Fatalf("TestProcessOrphans: incorrectly returned that parent block is an orphan")
+	}
+
+	// Make sure that the child block had been rejected
+	node := dag.index.LookupNode(childBlock.Hash())
+	if node == nil {
+		t.Fatalf("TestProcessOrphans: child block missing from block index")
+	}
+	if !dag.index.NodeStatus(node).KnownInvalid() {
+		t.Fatalf("TestProcessOrphans: child block erroneously not marked as invalid")
+	}
+}