[NOD-225] Finalize nodes below finality point (#335)

* [NOD-225] Finalize nodes below finality point

* [NOD-225] finalizeNodesBelowFinalityPoint only if dag.lastFinalityPoint is changed

* [NOD-225] change comment in validateParents

* [NOD-225] add string to ErrInvalidParentsRelation error

* [NOD-225] Change comment in validateParents

* [NOD-225] Change comment in validateParents

* [NOD-225] change comment in validateParents

* [NOD-225] Delete diff data from db directly from finalizeNodesBelowFinalityPoint

* [NOD-225] Refactor updateFinalityPoint

blockdag/blocknode.go

@@ -101,6 +101,9 @@ type blockNode struct {
 	// only be accessed using the concurrent-safe NodeStatus method on
 	// blockIndex once the node has been added to the global index.
 	status blockStatus
+
+	// isFinalized determines whether the node is below the finality point.
+	isFinalized bool
 }
 
 // initBlockNode initializes a block node from the given header and parent nodes.

blockdag/dag.go

@@ -721,11 +721,11 @@ func (dag *BlockDAG) checkFinalityRules(newNode *blockNode) error {
 // updateFinalityPoint updates the dag's last finality point if necessary.
 func (dag *BlockDAG) updateFinalityPoint() {
 	selectedTip := dag.selectedTip()
-	var newFinalityPoint *blockNode
 	// if the selected tip is the genesis block - it should be the new finality point
 	if selectedTip.isGenesis() {
-		newFinalityPoint = selectedTip
+		dag.lastFinalityPoint = selectedTip
-	} else {
+		return
+	}
 	// We are looking for a new finality point only if the new block's finality score is higher
 	// by 2 than the existing finality point's
 	if selectedTip.finalityScore() < dag.lastFinalityPoint.finalityScore()+2 {
@@ -739,9 +739,42 @@ func (dag *BlockDAG) updateFinalityPoint() {
 			break
 		}
 	}
-		newFinalityPoint = currentNode
+	dag.lastFinalityPoint = currentNode
+	spawn(func() {
+		dag.finalizeNodesBelowFinalityPoint(true)
+	})
+}
+
+func (dag *BlockDAG) finalizeNodesBelowFinalityPoint(deleteDiffData bool) {
+	queue := make([]*blockNode, 0, len(dag.lastFinalityPoint.parents))
+	for _, parent := range dag.lastFinalityPoint.parents {
+		queue = append(queue, parent)
+	}
+	var blockHashesToDelete []*daghash.Hash
+	if deleteDiffData {
+		blockHashesToDelete = make([]*daghash.Hash, 0, FinalityInterval)
+	}
+	for len(queue) > 0 {
+		var current *blockNode
+		current, queue = queue[0], queue[1:]
+		if !current.isFinalized {
+			current.isFinalized = true
+			if deleteDiffData {
+				blockHashesToDelete = append(blockHashesToDelete, current.hash)
+			}
+			for _, parent := range current.parents {
+				queue = append(queue, parent)
+			}
+		}
+	}
+	if deleteDiffData {
+		err := dag.db.Update(func(dbTx database.Tx) error {
+			return dag.utxoDiffStore.removeBlocksDiffData(dbTx, blockHashesToDelete)
+		})
+		if err != nil {
+			panic(fmt.Sprintf("Error removing diff data from utxoDiffStore: %s", err))
+		}
 	}
-	dag.lastFinalityPoint = newFinalityPoint
 }
 
 // NextBlockCoinbaseTransaction prepares the coinbase transaction for the next mined block

blockdag/dag_test.go

@@ -1116,3 +1116,94 @@ func TestAcceptingBlock(t *testing.T) {
 			"Want: nil, got: %s", redChainTipAcceptingBlock.hash)
 	}
 }
+
+func TestFinalizeNodesBelowFinalityPoint(t *testing.T) {
+	testFinalizeNodesBelowFinalityPoint(t, true)
+	testFinalizeNodesBelowFinalityPoint(t, false)
+}
+
+func testFinalizeNodesBelowFinalityPoint(t *testing.T, deleteDiffData bool) {
+	params := dagconfig.SimNetParams
+	params.K = 1
+	dag, teardownFunc, err := DAGSetup("testFinalizeNodesBelowFinalityPoint", Config{
+		DAGParams: &params,
+	})
+	if err != nil {
+		t.Fatalf("Failed to setup DAG instance: %v", err)
+	}
+	defer teardownFunc()
+
+	blockVersion := int32(0x10000000)
+	blockTime := dag.genesis.Header().Timestamp
+
+	flushUTXODiffStore := func() {
+		err := dag.db.Update(func(dbTx database.Tx) error {
+			return dag.utxoDiffStore.flushToDB(dbTx)
+		})
+		if err != nil {
+			t.Fatalf("Error flushing utxoDiffStore data to DB: %s", err)
+		}
+		dag.utxoDiffStore.clearDirtyEntries()
+	}
+
+	addNode := func(parent *blockNode) *blockNode {
+		blockTime = blockTime.Add(time.Second)
+		node := newTestNode(setFromSlice(parent), blockVersion, 0, blockTime, dag.dagParams.K)
+		node.updateParentsChildren()
+		dag.index.AddNode(node)
+
+		// Put dummy diff data in dag.utxoDiffStore
+		err := dag.utxoDiffStore.setBlockDiff(node, NewUTXODiff())
+		if err != nil {
+			t.Fatalf("setBlockDiff: %s", err)
+		}
+		flushUTXODiffStore()
+		return node
+	}
+
+	nodes := make([]*blockNode, 0, FinalityInterval)
+	currentNode := dag.genesis
+	nodes = append(nodes, currentNode)
+	for i := 0; i <= FinalityInterval*2; i++ {
+		currentNode = addNode(currentNode)
+		nodes = append(nodes, currentNode)
+	}
+
+	// Manually set the last finality point
+	dag.lastFinalityPoint = nodes[FinalityInterval-1]
+
+	dag.finalizeNodesBelowFinalityPoint(deleteDiffData)
+	flushUTXODiffStore()
+
+	for _, node := range nodes[:FinalityInterval-1] {
+		if !node.isFinalized {
+			t.Errorf("Node with blue score %d expected to be finalized", node.blueScore)
+		}
+		if _, ok := dag.utxoDiffStore.loaded[*node.hash]; deleteDiffData && ok {
+			t.Errorf("The diff data of node with blue score %d should have been unloaded if deleteDiffData is %T", node.blueScore, deleteDiffData)
+		} else if !deleteDiffData && !ok {
+			t.Errorf("The diff data of node with blue score %d shouldn't have been unloaded if deleteDiffData is %T", node.blueScore, deleteDiffData)
+		}
+		if diffData, err := dag.utxoDiffStore.diffDataFromDB(node.hash); err != nil {
+			t.Errorf("diffDataFromDB: %s", err)
+		} else if deleteDiffData && diffData != nil {
+			t.Errorf("The diff data of node with blue score %d should have been deleted from the database if deleteDiffData is %T", node.blueScore, deleteDiffData)
+		} else if !deleteDiffData && diffData == nil {
+			t.Errorf("The diff data of node with blue score %d shouldn't have been deleted from the database if deleteDiffData is %T", node.blueScore, deleteDiffData)
+		}
+	}
+
+	for _, node := range nodes[FinalityInterval-1:] {
+		if node.isFinalized {
+			t.Errorf("Node with blue score %d wasn't expected to be finalized", node.blueScore)
+		}
+		if _, ok := dag.utxoDiffStore.loaded[*node.hash]; !ok {
+			t.Errorf("The diff data of node with blue score %d shouldn't have been unloaded", node.blueScore)
+		}
+		if diffData, err := dag.utxoDiffStore.diffDataFromDB(node.hash); err != nil {
+			t.Errorf("diffDataFromDB: %s", err)
+		} else if diffData == nil {
+			t.Errorf("The diff data of node with blue score %d shouldn't have been deleted from the database", node.blueScore)
+		}
+	}
+}

blockdag/dagio.go

@@ -554,6 +554,7 @@ func (dag *BlockDAG) initDAGState() error {
 
 		// Set the last finality point
 		dag.lastFinalityPoint = dag.index.LookupNode(state.LastFinalityPoint)
+		dag.finalizeNodesBelowFinalityPoint(false)
 
 		return nil
 	})

blockdag/error.go

@@ -218,6 +218,10 @@ const (
 	// ErrSubnetwork indicates that a block doesn't adhere to the subnetwork
 	// registry rules
 	ErrSubnetworkRegistry
+
+	// ErrInvalidParentsRelation indicates that one of the parents of a block
+	// is also an ancestor of another parent
+	ErrInvalidParentsRelation
 )
 
 // Map of ErrorCode values back to their constant names for pretty printing.
@@ -265,6 +269,7 @@ var errorCodeStrings = map[ErrorCode]string{
 	ErrInvalidGas:                "ErrInvalidGas",
 	ErrInvalidPayload:            "ErrInvalidPayload",
 	ErrInvalidPayloadHash:        "ErrInvalidPayloadHash",
+	ErrInvalidParentsRelation:    "ErrInvalidParentsRelation",
 }
 
 // String returns the ErrorCode as a human-readable name.

blockdag/error_test.go

@@ -59,6 +59,7 @@ func TestErrorCodeStringer(t *testing.T) {
 		{ErrInvalidGas, "ErrInvalidGas"},
 		{ErrInvalidPayload, "ErrInvalidPayload"},
 		{ErrInvalidPayloadHash, "ErrInvalidPayloadHash"},
+		{ErrInvalidParentsRelation, "ErrInvalidParentsRelation"},
 		{0xffff, "Unknown ErrorCode (65535)"},
 	}
 

blockdag/external_dag_test.go

@@ -2,11 +2,10 @@ package blockdag_test
 
 import (
 	"fmt"
+	"github.com/daglabs/btcd/util/subnetworkid"
 	"math"
 	"testing"
 
-	"github.com/daglabs/btcd/util/subnetworkid"
-
 	"github.com/daglabs/btcd/util/daghash"
 	"github.com/daglabs/btcd/util/testtools"
 
@@ -116,7 +115,22 @@ func TestFinality(t *testing.T) {
 
 	// Here we check that a block with lower blue score than the last finality
 	// point will get rejected
-	_, err = buildNodeToDag([]*daghash.Hash{genesis.Hash()})
+	fakeCoinbaseTx, err := dag.NextBlockCoinbaseTransaction(nil, nil)
+	if err != nil {
+		t.Errorf("NextBlockCoinbaseTransaction: %s", err)
+	}
+	merkleRoot := blockdag.BuildHashMerkleTreeStore([]*util.Tx{fakeCoinbaseTx}).Root()
+	beforeFinalityBlock := wire.NewMsgBlock(&wire.BlockHeader{
+		Version:              0x10000000,
+		ParentHashes:         []*daghash.Hash{genesis.Hash()},
+		HashMerkleRoot:       merkleRoot,
+		AcceptedIDMerkleRoot: &daghash.ZeroHash,
+		UTXOCommitment:       &daghash.ZeroHash,
+		Timestamp:            dag.SelectedTipHeader().Timestamp,
+		Bits:                 genesis.MsgBlock().Header.Bits,
+	})
+	beforeFinalityBlock.AddTransaction(fakeCoinbaseTx.MsgTx())
+	_, _, err = dag.ProcessBlock(util.NewBlock(beforeFinalityBlock), blockdag.BFNoPoWCheck)
 	if err == nil {
 		t.Errorf("TestFinality: buildNodeToDag expected an error but got <nil>")
 	}
@@ -126,7 +140,7 @@ func TestFinality(t *testing.T) {
 			t.Errorf("TestFinality: buildNodeToDag expected an error with code %v but instead got %v", blockdag.ErrFinality, rErr.ErrorCode)
 		}
 	} else {
-		t.Errorf("TestFinality: buildNodeToDag got unexpected error: %v", rErr)
+		t.Errorf("TestFinality: buildNodeToDag got unexpected error: %v", err)
 	}
 
 	// Here we check that a block that doesn't have the last finality point in

blockdag/utxodiffstore.go

@@ -20,6 +20,7 @@ type utxoDiffStore struct {
 	dirty     map[daghash.Hash]struct{}
 	loaded    map[daghash.Hash]*blockUTXODiffData
 	mtx       sync.RWMutex
+	removeMtx sync.Mutex
 }
 
 func newUTXODiffStore(dag *BlockDAG) *utxoDiffStore {
@@ -64,6 +65,33 @@ func (diffStore *utxoDiffStore) setBlockDiffChild(node *blockNode, diffChild *bl
 	return nil
 }
 
+func (diffStore *utxoDiffStore) removeBlocksDiffData(dbTx database.Tx, blockHashes []*daghash.Hash) error {
+	diffStore.removeMtx.Lock()
+	defer diffStore.removeMtx.Unlock()
+	for _, hash := range blockHashes {
+		err := diffStore.removeBlockDiffData(dbTx, hash)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (diffStore *utxoDiffStore) removeBlockDiffData(dbTx database.Tx, blockHash *daghash.Hash) error {
+	// We use RLock here not because we aren't about to write into the map,
+	// but because RLock has a lower priority than Lock. In order to prevent
+	// concurrent writes, we employ a second lock around the entirety of
+	// removeBlocksDiffData.
+	diffStore.mtx.RLock()
+	defer diffStore.mtx.RUnlock()
+	delete(diffStore.loaded, *blockHash)
+	err := dbRemoveDiffData(dbTx, blockHash)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (diffStore *utxoDiffStore) setBlockAsDirty(blockHash *daghash.Hash) {
 	diffStore.dirty[*blockHash] = struct{}{}
 }
@@ -164,3 +192,7 @@ func dbStoreDiffData(dbTx database.Tx, hash *daghash.Hash, diffData *blockUTXODi
 
 	return dbTx.Metadata().Bucket(utxoDiffsBucketName).Put(hash[:], serializedDiffData)
 }
+
+func dbRemoveDiffData(dbTx database.Tx, hash *daghash.Hash) error {
+	return dbTx.Metadata().Bucket(utxoDiffsBucketName).Delete(hash[:])
+}

blockdag/utxodiffstore_test.go

@@ -2,13 +2,12 @@ package blockdag
 
 import (
 	"fmt"
-	"reflect"
-	"testing"
-
 	"github.com/daglabs/btcd/dagconfig"
 	"github.com/daglabs/btcd/database"
 	"github.com/daglabs/btcd/util/daghash"
 	"github.com/daglabs/btcd/wire"
+	"reflect"
+	"testing"
 )
 
 func TestUTXODiffStore(t *testing.T) {

blockdag/validate.go

@@ -604,12 +604,18 @@ func (dag *BlockDAG) validateDifficulty(header *wire.BlockHeader, bluestParent *
 	return nil
 }
 
-// validateParents validates that no parent is an ancestor of another parent
+// validateParents validates that no parent is an ancestor of another parent, and no parent is finalized
 func validateParents(blockHeader *wire.BlockHeader, parents blockSet) error {
 	minHeight := uint64(math.MaxUint64)
 	queue := newDownHeap()
 	visited := newSet()
 	for _, parent := range parents {
+		// isFinalized might be false-negative because node finality status is
+		// updated in a separate goroutine. This is why later the block is
+		// checked more thoroughly on the finality rules in dag.checkFinalityRules.
+		if parent.isFinalized {
+			return ruleError(ErrFinality, fmt.Sprintf("block %s is a finalized parent of block %s", parent.hash, blockHeader.BlockHash()))
+		}
 		if parent.height < minHeight {
 			minHeight = parent.height
 		}
@@ -623,10 +629,10 @@ func validateParents(blockHeader *wire.BlockHeader, parents blockSet) error {
 	for queue.Len() > 0 {
 		current := queue.pop()
 		if parents.contains(current) {
-			return fmt.Errorf("block %s is both a parent of %s and an"+
+			return ruleError(ErrInvalidParentsRelation, fmt.Sprintf("block %s is both a parent of %s and an"+
 				" ancestor of another parent",
 				current.hash,
-				blockHeader.BlockHash())
+				blockHeader.BlockHash()))
 		}
 		if current.height > minHeight {
 			for _, parent := range current.parents {