fix(dkg): defer dispatch by one block at phase boundaries

rolling-shutter/dkg/manager.go

@@ -326,8 +326,8 @@ func (m *Manager) activeDKGs(ctx context.Context, blockNumber uint64) ([]corekey
 // acceptable — the maybe-function will see the stale snapshot for one block
 // and pick up the new state on the next dispatch.
 //
-// Returns nil for "nothing to do" (no active phase at this block, no initial
-// state for non-Dealing phases). Returns an error for missing per-eon
+// Returns nil for "nothing to do" (no active phase at this block, first block
+// of a phase window, no initial state for non-Dealing phases). Returns an error for missing per-eon
 // configuration (NULL `dkg_contract` or NULL `phase_length`/`lead_length`).
 // The caller logs but does not abort on error.
 func (m *Manager) processDKG(ctx context.Context, eon corekeyperdb.Eon, blockNumber uint64) error {
@@ -354,7 +354,10 @@ func (m *Manager) processDKG(ctx context.Context, eon corekeyperdb.Eon, blockNum
 
 	retry := CurrentRetryCounter(params.activationBlock, params.leadLength, params.phaseLength, blockNumber)
 	retryInt64 := int64(retry) //nolint:gosec // G115: retry counter is bounded by the on-chain contract
-	blockPhase := PhaseAt(params.activationBlock, params.leadLength, params.phaseLength, params.maxRetries, retry, blockNumber)
+	// DispatchPhaseAt (not PhaseAt) so that no action fires on the first block
+	// of a phase window; see its doc comment for the gas-estimation race this
+	// avoids.
+	blockPhase := DispatchPhaseAt(params.activationBlock, params.leadLength, params.phaseLength, params.maxRetries, retry, blockNumber)
 	if blockPhase == PhaseNone {
 		return nil
 	}

rolling-shutter/dkg/manager_test.go

@@ -295,6 +295,103 @@ func TestProcessDKGWritesNothingPastMaxRetries(t *testing.T) {
 	}
 }
 
+// TestHandleBlockDefersDispatchOnPhaseBoundaryBlock is the end-to-end
+// wire-through for the one-block dispatch deferral: on the first block of the
+// Dealing window HandleBlock must write nothing (an RPC node may still serve
+// the previous block's state to eth_estimateGas at that point), and on the
+// window's second block the dealing action must fire as usual.
+func TestHandleBlockDefersDispatchOnPhaseBoundaryBlock(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
+	ctx := context.Background()
+
+	dbpool, dbclose := testsetup.NewTestDBPool(ctx, t, corekeyperdb.Definition)
+	t.Cleanup(dbclose)
+
+	const (
+		keyperConfigIndex int64 = 17
+		activationBlock   int64 = 100
+		phaseLength       int64 = 10
+		leadLength        int64 = 40
+		// Retry 0 dealing window is [60, 70).
+		firstDealingBlock uint64 = 60
+	)
+
+	ownECDSA, err := crypto.GenerateKey()
+	assert.NilError(t, err)
+	ownAddr := crypto.PubkeyToAddress(ownECDSA.PublicKey)
+
+	coreQueries := corekeyperdb.New(dbpool)
+	err = coreQueries.InsertEon(ctx, corekeyperdb.InsertEonParams{
+		Eon:                   keyperConfigIndex,
+		ActivationBlockNumber: activationBlock,
+		KeyperConfigIndex:     keyperConfigIndex,
+		DkgContract:           sql.NullString{String: "0xd0000000000000000000000000000000000000aa", Valid: true},
+		PhaseLength:           sql.NullInt64{Int64: phaseLength, Valid: true},
+		LeadLength:            sql.NullInt64{Int64: leadLength, Valid: true},
+		MaxRetries:            10,
+	})
+	assert.NilError(t, err)
+
+	obsQueries := obskeyperdb.New(dbpool)
+	err = obsQueries.InsertKeyperSet(ctx, obskeyperdb.InsertKeyperSetParams{
+		KeyperConfigIndex:     keyperConfigIndex,
+		ActivationBlockNumber: activationBlock,
+		Keypers:               shdb.EncodeAddresses([]common.Address{ownAddr}),
+		Threshold:             1,
+	})
+	assert.NilError(t, err)
+
+	// The dealing dispatch encrypts the self-eval, which reads the ECIES key
+	// registry.
+	ownECIES := ecies.ImportECDSA(ownECDSA)
+	err = coreQueries.UpsertECIESKey(ctx, corekeyperdb.UpsertECIESKeyParams{
+		KeyperAddress:  shdb.EncodeAddress(ownAddr),
+		EciesPublicKey: shdb.EncodeEciesPublicKey(&ownECIES.PublicKey),
+	})
+	assert.NilError(t, err)
+
+	mgr := New(Config{
+		DBPool:            dbpool,
+		OwnAddress:        ownAddr,
+		ECIESPrivateKey:   ownECIES,
+		ECIESRegistryAddr: common.HexToAddress("0xe0000000000000000000000000000000000000bb"),
+	})
+
+	// First block of the Dealing window: dispatch must be deferred.
+	err = mgr.HandleBlock(ctx, firstDealingBlock)
+	assert.NilError(t, err)
+
+	pending, err := coreQueries.GetPendingTxs(ctx)
+	assert.NilError(t, err)
+	assert.Equal(t, 0, len(pending), "no tx_outbox row may be written on the phase boundary block")
+
+	sent, err := coreQueries.ExistsDKGSentAction(ctx, corekeyperdb.ExistsDKGSentActionParams{
+		KeyperSetIndex: keyperConfigIndex,
+		RetryCounter:   0,
+		Action:         ActionDealing,
+	})
+	assert.NilError(t, err)
+	assert.Assert(t, !sent, "no dkg_sent_actions row may be written on the phase boundary block")
+
+	// Second block of the window: the dealing action fires.
+	err = mgr.HandleBlock(ctx, firstDealingBlock+1)
+	assert.NilError(t, err)
+
+	pending, err = coreQueries.GetPendingTxs(ctx)
+	assert.NilError(t, err)
+	assert.Equal(t, 1, len(pending), "dealing tx_outbox row expected on the window's second block")
+
+	sent, err = coreQueries.ExistsDKGSentAction(ctx, corekeyperdb.ExistsDKGSentActionParams{
+		KeyperSetIndex: keyperConfigIndex,
+		RetryCounter:   0,
+		Action:         ActionDealing,
+	})
+	assert.NilError(t, err)
+	assert.Assert(t, sent, "dealing dkg_sent_actions row expected on the window's second block")
+}
+
 // TestHandleBlockSkipsSupersededKeyperSet is the end-to-end wire-through for
 // the supersession filter (Seam 2 case a): two Keyper Sets with activation
 // blocks 100 and 120; advancing to a block past the successor's activation

rolling-shutter/dkg/phase.go

@@ -90,6 +90,43 @@ func PhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetries, retryCount
 	}
 }
 
+// DispatchPhaseAt returns the phase whose action may be dispatched at
+// `currentBlock`. It matches PhaseAt except on the first block of each phase
+// window, where it returns PhaseNone so that dispatch is deferred to the
+// window's second block. Dispatch decisions go through this function rather
+// than PhaseAt so that the deferral cannot be forgotten at a call site.
+//
+// The deferral exists because gas estimation races the phase boundary: an RPC
+// node can announce block N via newHead while `eth_estimateGas` still executes
+// against the state of N-1. A message enqueued on the boundary block then
+// reverts with WrongPhase during estimation and is permanently marked failed.
+// Waiting one block gives the node's state a full block interval to catch up.
+// The contract window itself is unchanged, so with phaseLength L the remaining
+// L-1 blocks are ample for inclusion.
+//
+// Windows with phaseLength <= 2 are exempt: they have no block that is both
+// past the boundary and still has a successor inside the window, since a
+// transaction triggered by block B is included at B+1 at the earliest. For
+// those, dispatch happens on the window's first block, which is the only
+// choice that can land in-phase at all (L == 2) or the only block there is
+// (L == 1).
+func DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetries, retryCounter, currentBlock uint64) Phase {
+	phase := PhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetries, retryCounter, currentBlock)
+	if phase == PhaseNone || phaseLength <= 2 {
+		return phase
+	}
+	// `phase != PhaseNone` guarantees the offset is inside the four windows,
+	// so it is non-negative and a zero remainder means "first block of a
+	// window". This mirrors `blocksInto` in the contract's DKGState script.
+	start := DKGStart(activationBlock, dkgLeadLength, phaseLength, retryCounter)
+	offset := int64(currentBlock) - start //nolint:gosec // G115: block number fits well within int64
+	pl := int64(phaseLength)              //nolint:gosec // G115: phase length fits well within int64
+	if offset%pl == 0 {
+		return PhaseNone
+	}
+	return phase
+}
+
 // CurrentRetryCounter derives the active retry counter from block arithmetic.
 // Each failed cycle advances the counter by one; the counter is never stored
 // in the database. A block before `DKGStart(..., 0)` returns 0 since the

rolling-shutter/dkg/phase_test.go

@@ -105,6 +105,118 @@ func TestPhaseAtZeroPhaseLength(t *testing.T) {
 	assert.Equal(t, uint64(0), CurrentRetryCounter(1000, 40, 0, 1000))
 }
 
+// TestDispatchPhaseAtDefersFirstBlockOfEachWindow verifies that dispatch is
+// deferred by exactly one block at every phase boundary: the first block of
+// each window returns PhaseNone, the second block returns the window's phase,
+// and all later blocks match PhaseAt unchanged.
+func TestDispatchPhaseAtDefersFirstBlockOfEachWindow(t *testing.T) {
+	const (
+		activationBlock uint64 = 1000
+		dkgLeadLength   uint64 = 40
+		phaseLength     uint64 = 10
+	)
+	// Retry 0: dealing starts at block 960 (1000 - 40).
+	cases := []struct {
+		name  string
+		block uint64
+		want  Phase
+	}{
+		{"before dealing start", 959, PhaseNone},
+		{"dealing first block deferred", 960, PhaseNone},
+		{"dealing second block", 961, PhaseDealing},
+		{"dealing last block", 969, PhaseDealing},
+		{"accusing first block deferred", 970, PhaseNone},
+		{"accusing second block", 971, PhaseAccusing},
+		{"apologizing first block deferred", 980, PhaseNone},
+		{"apologizing second block", 981, PhaseApologizing},
+		{"finalizing first block deferred", 990, PhaseNone},
+		{"finalizing second block", 991, PhaseFinalizing},
+		{"finalizing last block", 999, PhaseFinalizing},
+		{"retry 1 dealing first block deferred", 1000, PhaseNone},
+		{"retry 1 dealing second block", 1001, PhaseDealing},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			retry := CurrentRetryCounter(activationBlock, dkgLeadLength, phaseLength, tc.block)
+			got := DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, retry, tc.block)
+			assert.Equal(t, tc.want, got, "block=%d, retry=%d", tc.block, retry)
+		})
+	}
+}
+
+// TestDispatchPhaseAtPhaseLengthTwo verifies that a phase length of two is
+// exempt from the deferral: the window's second block is its last, and a
+// transaction triggered there is included one block later at the earliest,
+// i.e. never in-phase. Dispatch therefore stays on the first block and
+// DispatchPhaseAt degrades to PhaseAt.
+func TestDispatchPhaseAtPhaseLengthTwo(t *testing.T) {
+	const (
+		activationBlock uint64 = 1000
+		dkgLeadLength   uint64 = 4
+		phaseLength     uint64 = 2
+	)
+	// Retry 0 starts at block 996: dealing 996-997, accusing 998-999,
+	// apologizing 1000-1001, finalizing 1002-1003. Retry 1 deals from 1004.
+	for block, want := range map[uint64]Phase{
+		995:  PhaseNone,
+		996:  PhaseDealing,
+		997:  PhaseDealing,
+		998:  PhaseAccusing,
+		999:  PhaseAccusing,
+		1000: PhaseApologizing,
+		1001: PhaseApologizing,
+		1002: PhaseFinalizing,
+		1003: PhaseFinalizing,
+		1004: PhaseDealing,
+	} {
+		retry := CurrentRetryCounter(activationBlock, dkgLeadLength, phaseLength, block)
+		got := DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, retry, block)
+		assert.Equal(t, want, got, "block=%d, retry=%d", block, retry)
+	}
+}
+
+// TestDispatchPhaseAtPhaseLengthOne verifies that a phase length of one leaves
+// no room to defer, so DispatchPhaseAt degrades to PhaseAt.
+func TestDispatchPhaseAtPhaseLengthOne(t *testing.T) {
+	const (
+		activationBlock uint64 = 1000
+		dkgLeadLength   uint64 = 4
+		phaseLength     uint64 = 1
+	)
+	// Retry 0: dealing at 996, accusing at 997, apologizing at 998,
+	// finalizing at 999.
+	for block, want := range map[uint64]Phase{
+		995: PhaseNone,
+		996: PhaseDealing,
+		997: PhaseAccusing,
+		998: PhaseApologizing,
+		999: PhaseFinalizing,
+	} {
+		retry := CurrentRetryCounter(activationBlock, dkgLeadLength, phaseLength, block)
+		got := DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, retry, block)
+		assert.Equal(t, want, got, "block=%d", block)
+	}
+}
+
+// TestDispatchPhaseAtWindowStartingBeforeGenesis verifies that the boundary
+// test is offset arithmetic, not a lookback on the previous block: a window
+// that already covers block 0 (activation smaller than the lead length) is
+// mid-window at block 0 and dispatches there, while the next window's first
+// block (5) still defers.
+func TestDispatchPhaseAtWindowStartingBeforeGenesis(t *testing.T) {
+	const (
+		activationBlock uint64 = 5
+		dkgLeadLength   uint64 = 10
+		phaseLength     uint64 = 10
+	)
+	// Retry 0 dealing starts at block -5, so dealing covers [-5, 5) and
+	// accusing [5, 15).
+	assert.Equal(t, PhaseDealing, PhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, 0, 0))
+	assert.Equal(t, PhaseDealing, DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, 0, 0))
+	assert.Equal(t, PhaseNone, DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, 0, 5))
+	assert.Equal(t, PhaseAccusing, DispatchPhaseAt(activationBlock, dkgLeadLength, phaseLength, maxRetriesForTests, 0, 6))
+}
+
 // TestPhaseAtRetryCounterAtOrAboveMaxRetries covers the new retry-ceiling
 // gate: PhaseAt returns PhaseNone for any retryCounter >= maxRetries, across
 // representative block numbers inside each of the four sub-windows. This