Fix message pool common ancestor bug

- Add FindCommonAncestor utility to chain - Add tests for this utility - Update message pool to use this utility
filecoin-project · May 8, 2019 · 45930b2 · 45930b2
1 parent d5e752f
commit 45930b2
Show file tree

Hide file tree

Showing 4 changed files with 208 additions and 53 deletions.
diff --git a/chain/get_ancestors.go b/chain/get_ancestors.go
@@ -12,6 +12,9 @@ import (
 	"github.com/filecoin-project/go-filecoin/types"
 )
 
+// ErrNoCommonAncestor is returned when two chains assumed to have a common ancestor do not.
+var ErrNoCommonAncestor = errors.New("no common ancestor")
+
 // GetRecentAncestorsOfHeaviestChain returns the ancestors of a `TipSet` with
 // height `descendantBlockHeight` in the heaviest chain.
 func GetRecentAncestorsOfHeaviestChain(ctx context.Context, chainReader ReadStore, descendantBlockHeight *types.BlockHeight) ([]types.TipSet, error) {
@@ -124,3 +127,43 @@ func CollectAtMostNTipSets(ctx context.Context, iterator *TipsetIterator, n uint
 	}
 	return ret, nil
 }
+
+// FindCommonAncestor returns the common ancestor of the two tipsets pointed to
+// by the input iterators.  If they share no common ancestor ErrNoCommonAncestor
+// will be returned.
+func FindCommonAncestor(leftIter, rightIter *TipsetIterator) (types.TipSet, error) {
+	for !rightIter.Complete() && !leftIter.Complete() {
+		left := leftIter.Value()
+		right := rightIter.Value()
+
+		leftHeight, err := left.Height()
+		if err != nil {
+			return nil, err
+		}
+		rightHeight, err := right.Height()
+		if err != nil {
+			return nil, err
+		}
+
+		// Found common ancestor.
+		if left.Equals(right) {
+			return left, nil
+		}
+
+		// Update the pointers.  Pointers move back one tipset if they
+		// point to a tipset at the same height or higher than the
+		// other pointer's tipset.
+		if rightHeight >= leftHeight {
+			if err := rightIter.Next(); err != nil {
+				return nil, err
+			}
+		}
+
+		if leftHeight >= rightHeight {
+			if err := leftIter.Next(); err != nil {
+				return nil, err
+			}
+		}
+	}
+	return nil, ErrNoCommonAncestor
+}
diff --git a/chain/get_ancestors_test.go b/chain/get_ancestors_test.go
@@ -268,3 +268,129 @@ func TestGetRecentAncestorsStartingEpochIsNull(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, uint64(25), lastBlockHeight)
 }
+
+func TestFindCommonAncestorSameChain(t *testing.T) {
+	tf.BadUnitTestWithSideEffects(t)
+	ctx, blockSource, chainStore := setupGetAncestorTests(t)
+	// Add 30 tipsets to the head of the chainStore.
+	len1 := 30
+	requireGrowChain(ctx, t, blockSource, chainStore, len1)
+	headTipSet := requireHeadTipset(t, chainStore)
+	headIterOne := chain.IterAncestors(ctx, chainStore, headTipSet)
+	headIterTwo := chain.IterAncestors(ctx, chainStore, headTipSet)
+	commonAncestor, err := chain.FindCommonAncestor(headIterOne, headIterTwo)
+	assert.NoError(t, err)
+	assert.Equal(t, headTipSet, commonAncestor)
+}
+
+func TestFindCommonAncestorFork(t *testing.T) {
+
+	tf.BadUnitTestWithSideEffects(t)
+	ctx, blockSource, chainStore := setupGetAncestorTests(t)
+	// Add 3 tipsets to the head of the chainStore.
+	len1 := 3
+	requireGrowChain(ctx, t, blockSource, chainStore, len1)
+	headTipSetCA := requireHeadTipset(t, chainStore)
+
+	// make the first fork tipset
+	signer, ki := types.NewMockSignersAndKeyInfo(1)
+	mockSignerPubKey := ki[0].PublicKey()
+	fakeChildParams := th.FakeChildParams{
+		Parent:      headTipSetCA,
+		GenesisCid:  genCid,
+		Signer:      signer,
+		MinerPubKey: mockSignerPubKey,
+		StateRoot:   genStateRoot,
+		Nonce:       uint64(4),
+	}
+
+	firstForkBlock := th.RequireMkFakeChild(t, fakeChildParams)
+	requirePutBlocks(t, blockSource, firstForkBlock)
+	firstForkTS := th.RequireNewTipSet(t, firstForkBlock)
+	firstForkTsas := &chain.TipSetAndState{
+		TipSet:          firstForkTS,
+		TipSetStateRoot: genStateRoot,
+	}
+	th.RequirePutTsas(ctx, t, chainStore, firstForkTsas)
+	err := chainStore.SetHead(ctx, firstForkTS)
+	require.NoError(t, err)
+
+	// grow the fork by 10 blocks
+	lenFork := 10
+	requireGrowChain(ctx, t, blockSource, chainStore, lenFork)
+	headTipSetFork := requireHeadTipset(t, chainStore)
+	headIterFork := chain.IterAncestors(ctx, chainStore, headTipSetFork)
+
+	// go back and complete the original chain
+	err = chainStore.SetHead(ctx, headTipSetCA)
+	require.NoError(t, err)
+	lenMainChain := 14
+	requireGrowChain(ctx, t, blockSource, chainStore, lenMainChain)
+	headTipSetMainChain := requireHeadTipset(t, chainStore)
+	headIterMainChain := chain.IterAncestors(ctx, chainStore, headTipSetMainChain)
+
+	commonAncestor, err := chain.FindCommonAncestor(headIterMainChain, headIterFork)
+	assert.NoError(t, err)
+	assert.Equal(t, headTipSetCA, commonAncestor)
+}
+
+func TestFindCommonAncestorNoFork(t *testing.T) {
+	tf.BadUnitTestWithSideEffects(t)
+	ctx, blockSource, chainStore := setupGetAncestorTests(t)
+	// Add 30 tipsets to the head of the chainStore.
+	len1 := 30
+	requireGrowChain(ctx, t, blockSource, chainStore, len1)
+	headTipSet1 := requireHeadTipset(t, chainStore)
+	headIterOne := chain.IterAncestors(ctx, chainStore, headTipSet1)
+	// Now add 19 more tipsets.
+	len2 := 19
+	requireGrowChain(ctx, t, blockSource, chainStore, len2)
+	headTipSet2 := requireHeadTipset(t, chainStore)
+	headIterTwo := chain.IterAncestors(ctx, chainStore, headTipSet2)
+	commonAncestor, err := chain.FindCommonAncestor(headIterOne, headIterTwo)
+	assert.NoError(t, err)
+	assert.Equal(t, headTipSet1, commonAncestor)
+}
+
+// This test exercises an edge case fork that our previous common ancestor
+// utility handled incorrectly.
+func TestFindCommonAncestorNullBlockFork(t *testing.T) {
+	tf.BadUnitTestWithSideEffects(t)
+	ctx, blockSource, chainStore := setupGetAncestorTests(t)
+	// Add 10 tipsets to the head of the chainStore.
+	len1 := 10
+	requireGrowChain(ctx, t, blockSource, chainStore, len1)
+	expectedCA := requireHeadTipset(t, chainStore)
+
+	// add a null block and another block to the head
+	signer, ki := types.NewMockSignersAndKeyInfo(1)
+	mockSignerPubKey := ki[0].PublicKey()
+	fakeChildParams := th.FakeChildParams{
+		Parent:         expectedCA,
+		GenesisCid:     genCid,
+		Signer:         signer,
+		MinerPubKey:    mockSignerPubKey,
+		StateRoot:      genStateRoot,
+		NullBlockCount: uint64(1),
+	}
+
+	afterNullBlock := th.RequireMkFakeChild(t, fakeChildParams)
+	requirePutBlocks(t, blockSource, afterNullBlock)
+	afterNullTS := th.RequireNewTipSet(t, afterNullBlock)
+	afterNullTsas := &chain.TipSetAndState{
+		TipSet:          afterNullTS,
+		TipSetStateRoot: genStateRoot,
+	}
+	th.RequirePutTsas(ctx, t, chainStore, afterNullTsas)
+	afterNullIter := chain.IterAncestors(ctx, chainStore, afterNullTS)
+
+	// grow the fork by 1 block on the other fork
+	len2 := 1
+	requireGrowChain(ctx, t, blockSource, chainStore, len2)
+	mainChainTS := requireHeadTipset(t, chainStore)
+	mainChainIter := chain.IterAncestors(ctx, chainStore, mainChainTS)
+
+	commonAncestor, err := chain.FindCommonAncestor(afterNullIter, mainChainIter)
+	assert.NoError(t, err)
+	assert.Equal(t, expectedCA, commonAncestor)
+}
diff --git a/core/chains.go b/core/chains.go
@@ -12,66 +12,37 @@ import (
 // The resulting lists of blocks are ordered by decreasing height; the ordering of blocks with the same
 // height is undefined until https://github.com/filecoin-project/go-filecoin/issues/2310 is resolved.
 func CollectBlocksToCommonAncestor(ctx context.Context, store chain.BlockProvider, oldHead, newHead types.TipSet) (oldBlocks, newBlocks []*types.Block, err error) {
-	// Strategy: walk head-of-chain pointers old and new back until they are at the same height,
-	// then walk back in lockstep to find the common ancestor.
+	oldIter := chain.IterAncestors(ctx, store, oldHead)
+	newIter := chain.IterAncestors(ctx, store, newHead)
 
-	// If old is higher than new, collect all the messages from the old chain down to the height of new (exclusive).
-	newHeight, err := newHead.Height()
+	commonAncestor, err := chain.FindCommonAncestor(oldIter, newIter)
 	if err != nil {
 		return
 	}
-	oldBlocks, oldItr, err := collectBlocks(ctx, store, oldHead, newHeight)
+	commonHeight, err := commonAncestor.Height()
 	if err != nil {
 		return
 	}
 
-	// If new is higher than old, collect all the messages from new's chain down to the height of old.
-	oldHeight, err := oldHead.Height()
+	// Refresh iterators modified by FindCommonAncestors
+	oldIter = chain.IterAncestors(ctx, store, oldHead)
+	newIter = chain.IterAncestors(ctx, store, newHead)
+
+	// Add 1 to the height argument so that the common ancestor is not
+	// included in the outputs.
+	oldTipsets, err := chain.CollectTipSetsOfHeightAtLeast(ctx, oldIter, types.NewBlockHeight(commonHeight+uint64(1)))
 	if err != nil {
 		return
 	}
-	newBlocks, newItr, err := collectBlocks(ctx, store, newHead, oldHeight)
+	for _, ts := range oldTipsets {
+		oldBlocks = append(oldBlocks, ts.ToSlice()...)
+	}
+	newTipsets, err := chain.CollectTipSetsOfHeightAtLeast(ctx, newIter, types.NewBlockHeight(commonHeight+uint64(1)))
+	for _, ts := range newTipsets {
+		newBlocks = append(newBlocks, ts.ToSlice()...)
+	}
 	if err != nil {
 		return
 	}
-
-	// The tipset iterators are now at the same height.
-	// Continue traversing tipsets in lockstep until they reach the common ancestor.
-	for !(oldItr.Complete() || newItr.Complete() || oldItr.Value().Equals(newItr.Value())) {
-		for _, b := range oldItr.Value() {
-			oldBlocks = append(oldBlocks, b)
-		}
-		for _, b := range newItr.Value() {
-			newBlocks = append(newBlocks, b)
-		}
-
-		// Advance iterators
-		if err = oldItr.Next(); err != nil {
-			return
-		}
-		if err = newItr.Next(); err != nil {
-			return
-		}
-	}
 	return
 }
-
-// collectBlocks collects blocks by traversing the chain from a tipset towards its parents, until some
-// minimum height (excluding the tipset at that height).
-// Returns the blocks collected and a tipset iterator positioned at the tipset at `endHeight`
-func collectBlocks(ctx context.Context, store chain.BlockProvider, head types.TipSet, endHeight uint64) ([]*types.Block, *chain.TipsetIterator, error) {
-	var blocks []*types.Block
-	var err error
-	tsItr := chain.IterAncestors(ctx, store, head)
-	for ; err == nil && !tsItr.Complete(); err = tsItr.Next() {
-		ts := tsItr.Value()
-		height, err := ts.Height()
-		if err != nil || height <= endHeight {
-			break
-		}
-		for _, b := range ts {
-			blocks = append(blocks, b)
-		}
-	}
-	return blocks, tsItr, err
-}
diff --git a/core/message_pool_test.go b/core/message_pool_test.go
@@ -221,6 +221,7 @@ func TestUpdateMessagePool(t *testing.T) {
 		MustAdd(p, m[0], m[1])
 
 		parent := types.TipSet{}
+
 		blk := types.Block{Height: 0}
 		parent[blk.Cid()] = &blk
 
@@ -261,10 +262,14 @@ func TestUpdateMessagePool(t *testing.T) {
 		m := types.NewSignedMsgs(7, mockSigner)
 		MustAdd(p, m[2], m[5])
 
-		oldChain := NewChainWithMessages(store, types.TipSet{}, msgsSet{msgs{m[0], m[1]}})
+		parent := types.TipSet{}
+
+		blk := types.Block{Height: 0}
+		parent[blk.Cid()] = &blk
+		oldChain := NewChainWithMessages(store, parent, msgsSet{msgs{m[0], m[1]}})
 		oldTipSet := headOf(oldChain)
 
-		newChain := NewChainWithMessages(store, types.TipSet{},
+		newChain := NewChainWithMessages(store, parent,
 			msgsSet{msgs{m[2], m[3]}},
 			msgsSet{msgs{m[4]}},
 			msgsSet{msgs{m[0]}},
@@ -287,10 +292,15 @@ func TestUpdateMessagePool(t *testing.T) {
 		m := types.NewSignedMsgs(7, mockSigner)
 		MustAdd(p, m[2], m[5])
 
-		oldChain := NewChainWithMessages(store, types.TipSet{}, msgsSet{msgs{m[0]}, msgs{m[1]}})
+		parent := types.TipSet{}
+
+		blk := types.Block{Height: 0}
+		parent[blk.Cid()] = &blk
+
+		oldChain := NewChainWithMessages(store, parent, msgsSet{msgs{m[0]}, msgs{m[1]}})
 		oldTipSet := headOf(oldChain)
 
-		newChain := NewChainWithMessages(store, types.TipSet{},
+		newChain := NewChainWithMessages(store, parent,
 			msgsSet{msgs{m[2], m[3]}},
 			msgsSet{msgs{m[4]}, msgs{m[0]}, msgs{}, msgs{}},
 			msgsSet{msgs{}, msgs{m[5], m[6]}},
@@ -387,14 +397,19 @@ func TestUpdateMessagePool(t *testing.T) {
 		m := types.NewSignedMsgs(6, mockSigner)
 		MustAdd(p, m[3], m[5])
 
-		oldChain := NewChainWithMessages(store, types.TipSet{},
+		parent := types.TipSet{}
+
+		blk := types.Block{Height: 0}
+		parent[blk.Cid()] = &blk
+
+		oldChain := NewChainWithMessages(store, parent,
 			msgsSet{msgs{m[0]}},
 			msgsSet{msgs{m[1]}},
 			msgsSet{msgs{m[2]}},
 		)
 		oldTipSet := headOf(oldChain)
 
-		newChain := NewChainWithMessages(store, types.TipSet{},
+		newChain := NewChainWithMessages(store, parent,
 			msgsSet{msgs{m[0]}, msgs{m[1]}, msgs{m[2]}},
 		)
 		newTipSet := headOf(newChain)