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