From b850f8c3a7c1d4a3d7da900309f5e25fd5b77ef6 Mon Sep 17 00:00:00 2001 From: Claude Agent Date: Tue, 25 Aug 2026 13:44:47 +0200 Subject: [PATCH] fix(dkg): defer dispatch by one block at phase boundaries 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, which txsender classifies as non-transient and marks failed. Since the dkg_sent_actions marker is written atomically with the outbox row, the keyper never retries the action and sits out the whole phase. Add DispatchPhaseAt, which matches PhaseAt except on the first block of each phase window, and route processDKG through it. The contract window 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 an in-window successor, since a transaction triggered by block B is included at B+1 at the earliest. Co-Authored-By: Claude Opus 5 (1M context) --- rolling-shutter/dkg/manager.go | 9 ++- rolling-shutter/dkg/manager_test.go | 97 ++++++++++++++++++++++++ rolling-shutter/dkg/phase.go | 37 +++++++++ rolling-shutter/dkg/phase_test.go | 112 ++++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 3 deletions(-) diff --git a/rolling-shutter/dkg/manager.go b/rolling-shutter/dkg/manager.go index ec1c2a90..4928ab65 100644 --- a/rolling-shutter/dkg/manager.go +++ b/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 } diff --git a/rolling-shutter/dkg/manager_test.go b/rolling-shutter/dkg/manager_test.go index 6983941a..872406c5 100644 --- a/rolling-shutter/dkg/manager_test.go +++ b/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 diff --git a/rolling-shutter/dkg/phase.go b/rolling-shutter/dkg/phase.go index 354b03d1..ae7b4c6e 100644 --- a/rolling-shutter/dkg/phase.go +++ b/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 diff --git a/rolling-shutter/dkg/phase_test.go b/rolling-shutter/dkg/phase_test.go index b143b251..cda65fe9 100644 --- a/rolling-shutter/dkg/phase_test.go +++ b/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