bug(lib/blocktree): fix blocktree highest common ancestor (#2934)

dot/core/interface.go

@@ -66,12 +66,12 @@ type BlockState interface {
 	FreeImportedBlockNotifierChannel(ch chan *types.Block)
 	GetFinalisedNotifierChannel() chan *types.FinalisationInfo
 	FreeFinalisedNotifierChannel(ch chan *types.FinalisationInfo)
-	HighestCommonAncestor(a, b common.Hash) (common.Hash, error)
 	SubChain(start, end common.Hash) ([]common.Hash, error)
 	GetBlockBody(hash common.Hash) (*types.Body, error)
 	HandleRuntimeChanges(newState *rtstorage.TrieState, in runtime.Instance, bHash common.Hash) error
 	GetRuntime(blockHash common.Hash) (instance runtime.Instance, err error)
 	StoreRuntime(common.Hash, runtime.Instance)
+	LowestCommonAncestor(a, b common.Hash) (common.Hash, error)
 }
 
 // StorageState interface for storage state methods

dot/core/service.go

@@ -327,19 +327,19 @@ func (s *Service) handleBlocksAsync() {
 // handleChainReorg checks if there is a chain re-org (ie. new chain head is on a different chain than the
 // previous chain head). If there is a re-org, it moves the transactions that were included on the previous
 // chain back into the transaction pool.
-func (s *Service) handleChainReorg(prev, curr common.Hash) error {
-	ancestor, err := s.blockState.HighestCommonAncestor(prev, curr)
+func (s *Service) handleChainReorg(best, curr common.Hash) error {
+	ancestor, err := s.blockState.LowestCommonAncestor(best, curr)
 	if err != nil {
 		return err
 	}
 
 	// if the highest common ancestor of the previous chain head and current chain head is the previous chain head,
 	// then the current chain head is the descendant of the previous and thus are on the same chain
-	if ancestor == prev {
+	if ancestor == best || ancestor == curr {
 		return nil
 	}
 
-	subchain, err := s.blockState.SubChain(ancestor, prev)
+	subchain, err := s.blockState.SubChain(ancestor, best)
 	if err != nil {
 		return err
 	}

dot/core/service_test.go

@@ -725,7 +725,7 @@ func Test_Service_handleBlocksAsync(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
 		mockBlockState.EXPECT().BestBlockHash().Return(common.Hash{})
-		mockBlockState.EXPECT().HighestCommonAncestor(common.Hash{}, block.Header.Hash()).
+		mockBlockState.EXPECT().LowestCommonAncestor(common.Hash{}, block.Header.Hash()).
 			Return(common.Hash{}, errTestDummyError)
 
 		blockAddChan := make(chan *types.Block)
@@ -767,7 +767,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(common.Hash{}, errDummyErr)
 
 		service := &Service{
@@ -780,7 +780,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(common.Hash{}, errDummyErr)
 
 		service := &Service{
@@ -793,7 +793,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testPrevHash, nil)
 
 		service := &Service{
@@ -806,7 +806,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testAncestorHash, nil)
 		mockBlockState.EXPECT().SubChain(testAncestorHash, testPrevHash).Return([]common.Hash{}, errDummyErr)
 
@@ -820,7 +820,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testAncestorHash, nil)
 		mockBlockState.EXPECT().SubChain(testAncestorHash, testPrevHash).Return([]common.Hash{}, nil)
 
@@ -834,7 +834,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testAncestorHash, nil)
 		mockBlockState.EXPECT().SubChain(testAncestorHash, testPrevHash).Return(testSubChain, nil)
 		mockBlockState.EXPECT().BestBlockHash().Return(common.Hash{1})
@@ -867,7 +867,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		})
 
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testAncestorHash, nil)
 		mockBlockState.EXPECT().SubChain(testAncestorHash, testPrevHash).Return(testSubChain, nil)
 		mockBlockState.EXPECT().BestBlockHash().Return(common.Hash{1})
@@ -906,7 +906,7 @@ func TestService_handleChainReorg(t *testing.T) {
 		})
 
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().HighestCommonAncestor(testPrevHash, testCurrentHash).
+		mockBlockState.EXPECT().LowestCommonAncestor(testPrevHash, testCurrentHash).
 			Return(testAncestorHash, nil)
 		mockBlockState.EXPECT().SubChain(testAncestorHash, testPrevHash).Return(testSubChain, nil)
 		mockBlockState.EXPECT().BestBlockHash().Return(common.Hash{1})

dot/state/block.go

@@ -561,9 +561,9 @@ func (bs *BlockState) IsDescendantOf(parent, child common.Hash) (bool, error) {
 	return bs.bt.IsDescendantOf(parent, child)
 }
 
-// HighestCommonAncestor returns the block with the highest number that is an ancestor of both a and b
-func (bs *BlockState) HighestCommonAncestor(a, b common.Hash) (common.Hash, error) {
-	return bs.bt.HighestCommonAncestor(a, b)
+// LowestCommonAncestor returns the lowest common ancestor between two blocks in the tree.
+func (bs *BlockState) LowestCommonAncestor(a, b common.Hash) (common.Hash, error) {
+	return bs.bt.LowestCommonAncestor(a, b)
 }
 
 // Leaves returns the leaves of the blocktree as an array

lib/blocktree/blocktree.go

@@ -4,6 +4,7 @@
 package blocktree
 
 import (
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -16,11 +17,14 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promauto"
 )
 
-var leavesGauge = promauto.NewGauge(prometheus.GaugeOpts{
-	Namespace: "gossamer_block",
-	Name:      "leaves_total",
-	Help:      "total number of blocktree leaves",
-})
+var (
+	leavesGauge = promauto.NewGauge(prometheus.GaugeOpts{
+		Namespace: "gossamer_block",
+		Name:      "leaves_total",
+		Help:      "total number of blocktree leaves",
+	})
+	errAncestorOutOfBoundsCheck = errors.New("out of bounds ancestor check")
+)
 
 // Hash common.Hash
 type Hash = common.Hash
@@ -297,29 +301,50 @@ func (bt *BlockTree) Leaves() []Hash {
 	return la
 }
 
-// HighestCommonAncestor returns the highest block that is a Ancestor to both a and b
-func (bt *BlockTree) HighestCommonAncestor(a, b Hash) (Hash, error) {
+// LowestCommonAncestor returns the lowest common ancestor block hash between two blocks in the tree.
+func (bt *BlockTree) LowestCommonAncestor(a, b Hash) (Hash, error) {
 	bt.RLock()
 	defer bt.RUnlock()
 
-	an := bt.getNode(a)
-	if an == nil {
+	aNode := bt.getNode(a)
+	if aNode == nil {
 		return common.Hash{}, ErrNodeNotFound
 	}
 
-	bn := bt.getNode(b)
-	if bn == nil {
+	bNode := bt.getNode(b)
+	if bNode == nil {
 		return common.Hash{}, ErrNodeNotFound
 	}
-
-	ancestor := an.highestCommonAncestor(bn)
-	if ancestor == nil {
-		// this case shouldn't happen - any two nodes in the blocktree must
-		// have a common ancestor, the lowest of which is the root node
-		return common.Hash{}, fmt.Errorf("%w: %s and %s", ErrNoCommonAncestor, a, b)
+	return lowestCommonAncestor(aNode, bNode), nil
+}
+func lowestCommonAncestor(aNode, bNode *node) Hash {
+	higherNode := bNode
+	lowerNode := aNode
+	if aNode.number > bNode.number {
+		higherNode = aNode
+		lowerNode = bNode
+	}
+
+	higherNum := higherNode.number
+	lowerNum := lowerNode.number
+	diff := higherNum - lowerNum
+	for diff > 0 {
+		if higherNode.parent == nil {
+			panic(fmt.Errorf("%w: for block number %v", errAncestorOutOfBoundsCheck, higherNum))
+		}
+		higherNode = higherNode.parent
+		diff--
 	}
 
-	return ancestor.hash, nil
+	for {
+		if higherNode.hash == lowerNode.hash {
+			return higherNode.hash
+		} else if higherNode.parent == nil || lowerNode.parent == nil {
+			panic(fmt.Errorf("%w: for block number %v", errAncestorOutOfBoundsCheck, higherNum))
+		}
+		higherNode = higherNode.parent
+		lowerNode = lowerNode.parent
+	}
 }
 
 // GetAllBlocks returns all the blocks in the tree