fix: correctly handle null blocks when detecting an expensive fork

chain/stmgr/call.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	"github.com/filecoin-project/go-address"
+	"github.com/filecoin-project/go-state-types/abi"
 	"github.com/filecoin-project/go-state-types/crypto"
 	"github.com/ipfs/go-cid"
 	"go.opencensus.io/trace"
@@ -20,41 +21,49 @@ import (
 
 var ErrExpensiveFork = errors.New("refusing explicit call due to state fork at epoch")
 
+// Call applies the given message to the given tipset's parent state, at the epoch following the
+// tipset's parent. In the presence of null blocks, the height at which the message is invoked may
+// be less than the specified tipset.
+//
+// - If no tipset is specified, the first tipset without an expensive migration is used.
+// - If executing a message at a given tipset would trigger an expensive migration, the call will
+//   fail with ErrExpensiveFork.
 func (sm *StateManager) Call(ctx context.Context, msg *types.Message, ts *types.TipSet) (*api.InvocResult, error) {
 	ctx, span := trace.StartSpan(ctx, "statemanager.Call")
 	defer span.End()
 
+	var pheight abi.ChainEpoch = -1
+
 	// If no tipset is provided, try to find one without a fork.
 	if ts == nil {
 		ts = sm.cs.GetHeaviestTipSet()
-
 		// Search back till we find a height with no fork, or we reach the beginning.
-		for ts.Height() > 0 && sm.hasExpensiveFork(ctx, ts.Height()-1) {
-			var err error
-			ts, err = sm.cs.GetTipSetFromKey(ts.Parents())
+		for ts.Height() > 0 {
+			pts, err := sm.cs.GetTipSetFromKey(ts.Parents())
 			if err != nil {
 				return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
 			}
+			if !sm.hasExpensiveFork(pts.Height()) {
+				pheight = pts.Height()
+				break
+			}
+			ts = pts
+		}
+	} else {
+		pts, err := sm.cs.LoadTipSet(ts.Parents())
+		if err != nil {
+			return nil, xerrors.Errorf("failed to load parent tipset: %w", err)
+		}
+		pheight = pts.Height()
+		if sm.hasExpensiveFork(pheight) {
+			return nil, ErrExpensiveFork
 		}
 	}
 
 	bstate := ts.ParentState()
-	pts, err := sm.cs.LoadTipSet(ts.Parents())
-	if err != nil {
-		return nil, xerrors.Errorf("failed to load parent tipset: %w", err)
-	}
-	pheight := pts.Height()
-
-	// If we have to run an expensive migration, and we're not at genesis,
-	// return an error because the migration will take too long.
-	//
-	// We allow this at height 0 for at-genesis migrations (for testing).
-	if pheight > 0 && sm.hasExpensiveFork(ctx, pheight) {
-		return nil, ErrExpensiveFork
-	}
 
 	// Run the (not expensive) migration.
-	bstate, err = sm.handleStateForks(ctx, bstate, pheight, nil, ts)
+	bstate, err := sm.handleStateForks(ctx, bstate, pheight, nil, ts)
 	if err != nil {
 		return nil, fmt.Errorf("failed to handle fork: %w", err)
 	}
@@ -140,25 +149,33 @@ func (sm *StateManager) CallWithGas(ctx context.Context, msg *types.Message, pri
 		// run the fork logic in `sm.TipSetState`. We need the _current_
 		// height to have no fork, because we'll run it inside this
 		// function before executing the given message.
-		for ts.Height() > 0 && (sm.hasExpensiveFork(ctx, ts.Height()) || sm.hasExpensiveFork(ctx, ts.Height()-1)) {
-			var err error
-			ts, err = sm.cs.GetTipSetFromKey(ts.Parents())
+		for ts.Height() > 0 {
+			pts, err := sm.cs.GetTipSetFromKey(ts.Parents())
 			if err != nil {
 				return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
 			}
-		}
-	}
+			if !sm.hasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
+				break
+			}
 
-	// When we're not at the genesis block, make sure we don't have an expensive migration.
-	if ts.Height() > 0 && (sm.hasExpensiveFork(ctx, ts.Height()) || sm.hasExpensiveFork(ctx, ts.Height()-1)) {
-		return nil, ErrExpensiveFork
+			ts = pts
+		}
+	} else if ts.Height() > 0 {
+		pts, err := sm.cs.GetTipSetFromKey(ts.Parents())
+		if err != nil {
+			return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
+		}
+		if sm.hasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
+			return nil, ErrExpensiveFork
+		}
 	}
 
 	state, _, err := sm.TipSetState(ctx, ts)
 	if err != nil {
 		return nil, xerrors.Errorf("computing tipset state: %w", err)
 	}
 
+	// Technically, the tipset we're passing in here should be ts+1, but that may not exist.
 	state, err = sm.handleStateForks(ctx, state, ts.Height(), nil, ts)
 	if err != nil {
 		return nil, fmt.Errorf("failed to handle fork: %w", err)

chain/stmgr/execute.go

@@ -99,7 +99,6 @@ func (sm *StateManager) ApplyBlocks(ctx context.Context, parentEpoch abi.ChainEp
 		}
 
 		// handle state forks
-		// XXX: The state tree
 		newState, err := sm.handleStateForks(ctx, pstate, i, em, ts)
 		if err != nil {
 			return cid.Undef, cid.Undef, xerrors.Errorf("error handling state forks: %w", err)

chain/stmgr/forks.go

@@ -41,8 +41,11 @@ type MigrationCache interface {
 // - The oldState is the state produced by the upgrade epoch.
 // - The returned newState is the new state that will be used by the next epoch.
 // - The height is the upgrade epoch height (already executed).
-// - The tipset is the tipset for the last non-null block before the upgrade. Do
-//   not assume that ts.Height() is the upgrade height.
+// - The tipset is the first non-null tipset after the upgrade height (the tipset in
+//   which the upgrade is executed). Do not assume that ts.Height() is the upgrade height.
+//
+// NOTE: In StateCompute and CallWithGas, the passed tipset is actually the tipset _before_ the
+// upgrade. The tipset should really only be used for referencing the "current chain".
 type MigrationFunc func(
 	ctx context.Context,
 	sm *StateManager, cache MigrationCache,
@@ -208,7 +211,21 @@ func (sm *StateManager) handleStateForks(ctx context.Context, root cid.Cid, heig
 	return retCid, nil
 }
 
-func (sm *StateManager) hasExpensiveFork(ctx context.Context, height abi.ChainEpoch) bool {
+// Returns true if executing the current tipset would trigger an expensive fork.
+//
+// - If the tipset is the genesis, this function always returns false.
+// - If inclusive is true, this function will also return true if applying a message on-top-of the
+//   tipset would trigger a fork.
+func (sm *StateManager) hasExpensiveForkBetween(parent, height abi.ChainEpoch) bool {
+	for h := parent; h < height; h++ {
+		if _, ok := sm.expensiveUpgrades[h]; ok {
+			return true
+		}
+	}
+	return false
+}
+
+func (sm *StateManager) hasExpensiveFork(height abi.ChainEpoch) bool {
 	_, ok := sm.expensiveUpgrades[height]
 	return ok
 }

chain/stmgr/forks_test.go

@@ -242,20 +242,35 @@ func TestForkHeightTriggers(t *testing.T) {
 func TestForkRefuseCall(t *testing.T) {
 	logging.SetAllLoggers(logging.LevelInfo)
 
+	for after := 0; after < 3; after++ {
+		for before := 0; before < 3; before++ {
+			// Makes the lints happy...
+			after := after
+			before := before
+			t.Run(fmt.Sprintf("after:%d,before:%d", after, before), func(t *testing.T) {
+				testForkRefuseCall(t, before, after)
+			})
+		}
+	}
+
+}
+func testForkRefuseCall(t *testing.T, nullsBefore, nullsAfter int) {
 	ctx := context.TODO()
 
 	cg, err := gen.NewGenerator()
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	var migrationCount int
 	sm, err := NewStateManagerWithUpgradeSchedule(
 		cg.ChainStore(), cg.StateManager().VMSys(), UpgradeSchedule{{
 			Network:   network.Version1,
 			Expensive: true,
 			Height:    testForkHeight,
 			Migration: func(ctx context.Context, sm *StateManager, cache MigrationCache, cb ExecMonitor,
 				root cid.Cid, height abi.ChainEpoch, ts *types.TipSet) (cid.Cid, error) {
+				migrationCount++
 				return root, nil
 			}}})
 	if err != nil {
@@ -292,14 +307,20 @@ func TestForkRefuseCall(t *testing.T) {
 		GasFeeCap:  types.NewInt(0),
 	}
 
+	nullStart := abi.ChainEpoch(testForkHeight - nullsBefore)
+	nullLength := abi.ChainEpoch(nullsBefore + nullsAfter)
+
 	for i := 0; i < 50; i++ {
-		ts, err := cg.NextTipSet()
+		pts := cg.CurTipset.TipSet()
+		skip := abi.ChainEpoch(0)
+		if pts.Height() == nullStart {
+			skip = nullLength
+		}
+		ts, err := cg.NextTipSetFromMiners(pts, cg.Miners, skip)
 		if err != nil {
 			t.Fatal(err)
 		}
 
-		pts, err := cg.ChainStore().LoadTipSet(ts.TipSet.TipSet().Parents())
-		require.NoError(t, err)
 		parentHeight := pts.Height()
 		currentHeight := ts.TipSet.TipSet().Height()
 
@@ -321,7 +342,20 @@ func TestForkRefuseCall(t *testing.T) {
 			require.NoError(t, err)
 			require.True(t, ret.MsgRct.ExitCode.IsSuccess())
 		}
+
+		// Calls without a tipset should walk back to the last non-fork tipset.
+		// We _verify_ that the migration wasn't run multiple times at the end of the
+		// test.
+		ret, err = sm.CallWithGas(ctx, m, nil, nil)
+		require.NoError(t, err)
+		require.True(t, ret.MsgRct.ExitCode.IsSuccess())
+
+		ret, err = sm.Call(ctx, m, nil)
+		require.NoError(t, err)
+		require.True(t, ret.MsgRct.ExitCode.IsSuccess())
 	}
+	// Make sure we didn't execute the migration multiple times.
+	require.Equal(t, migrationCount, 1)
 }
 
 func TestForkPreMigration(t *testing.T) {

chain/stmgr/utils.go

@@ -143,13 +143,14 @@ func ComputeState(ctx context.Context, sm *StateManager, height abi.ChainEpoch,
 	}
 
 	for i := ts.Height(); i < height; i++ {
-		// handle state forks
+		// Technically, the tipset we're passing in here should be ts+1, but that may not exist.
 		base, err = sm.handleStateForks(ctx, base, i, &InvocationTracer{trace: &trace}, ts)
 		if err != nil {
 			return cid.Undef, nil, xerrors.Errorf("error handling state forks: %w", err)
 		}
 
-		// TODO: should we also run cron here?
+		// We intentionally don't run cron here, as we may be trying to look into the
+		// future. It's not guaranteed to be accurate... but that's fine.
 	}
 
 	r := store.NewChainRand(sm.cs, ts.Cids())