diff --git a/yarn-project/ethereum/src/contracts/governance.test.ts b/yarn-project/ethereum/src/contracts/governance.test.ts index cbdd8826cec9..9a3b2e2d317a 100644 --- a/yarn-project/ethereum/src/contracts/governance.test.ts +++ b/yarn-project/ethereum/src/contracts/governance.test.ts @@ -5,6 +5,7 @@ import { createLogger } from '@aztec/foundation/log'; import { TestDateProvider } from '@aztec/foundation/timer'; import { GovernanceAbi } from '@aztec/l1-artifacts/GovernanceAbi'; +import { jest } from '@jest/globals'; import { type Hex, encodeFunctionData, parseEventLogs } from 'viem'; import { type PrivateKeyAccount, privateKeyToAccount } from 'viem/accounts'; import { foundry } from 'viem/chains'; @@ -19,10 +20,31 @@ import { type GovernanceConfiguration, GovernanceContract, MAX_PROPOSAL_LIFETIME_SECONDS, + type Proposal, ProposalState, ReadOnlyGovernanceContract, } from './governance.js'; +describe('ProposalState', () => { + it('matches the on-chain IGovernance.ProposalState enum ordering', () => { + // Mirrors `IGovernance.ProposalState` in l1-contracts. On-chain enum values are decoded by + // numeric index, so any drift here silently misclassifies proposal states (e.g. an `Expired` + // proposal decoding as out-of-range and throwing, or `Dropped` decoding as `Droppable`). + const solidityOrder = [ + 'Pending', + 'Active', + 'Queued', + 'Executable', + 'Rejected', + 'Executed', + 'Droppable', + 'Dropped', + 'Expired', + ]; + expect(solidityOrder.map((_name, index) => ProposalState[index])).toEqual(solidityOrder); + }); +}); + describe('Governance', () => { let anvil: Anvil; let rpcUrl: string; @@ -89,7 +111,7 @@ describe('Governance', () => { expect(governance.getProposal).toBeDefined(); expect(governance.getProposalState).toBeDefined(); expect(governance.getProposalCount).toBeDefined(); - expect(governance.hasActiveProposalWithPayload).toBeDefined(); + expect(governance.getPayloadProposalStatus).toBeDefined(); expect(governance.awaitProposalActive).toBeDefined(); expect(governance.awaitProposalExecutable).toBeDefined(); }); @@ -108,7 +130,7 @@ describe('Governance', () => { expect(config.minimumVotes).toBeGreaterThan(0n); }); - describe('hasActiveProposalWithPayload', () => { + describe('getPayloadProposalStatus', () => { // Runtime bytecode for a contract whose only behavior is: ignore calldata, return `original` // (zero-padded to 32 bytes). This is a stand-in for `IProposerPayload.getOriginalPayload()`. // The point is to faithfully exercise the 'getOriginalPayload' path. @@ -163,6 +185,27 @@ describe('Governance', () => { // Returns the latest L1 block timestamp as a bigint const nowOnChain = () => publicClient.getBlock({ includeTransactions: false }).then(b => b.timestamp); + // Builds a fully-populated synthetic Proposal for spy-driven tests of states that are hard to + // reach on a fresh anvil deployment (Executed / Droppable), where no voting power is available. + const makeProposal = (overrides: Partial): Proposal => ({ + config: { + votingDelay: 0n, + votingDuration: 0n, + executionDelay: 0n, + gracePeriod: 0n, + quorum: 0n, + requiredYeaMargin: 0n, + minimumVotes: 0n, + }, + cachedState: ProposalState.Pending, + state: ProposalState.Pending, + payload: EthAddress.random(), + proposer: EthAddress.random(), + creation: 0n, + summedBallot: { yea: 0n, nay: 0n }, + ...overrides, + }); + beforeAll(async () => { await cheatCodes.startImpersonating(governanceProposerAddress); }); @@ -171,14 +214,14 @@ describe('Governance', () => { await cheatCodes.stopImpersonating(governanceProposerAddress); }); - it('returns false on a fresh governance with no proposals', async () => { + it('reports none on a fresh governance with no proposals', async () => { const proposalCount = await governance.getProposalCount(); expect(proposalCount).toBe(0n); const arbitraryPayload = EthAddress.random().toString(); - await expect(governance.hasActiveProposalWithPayload(arbitraryPayload)).resolves.toBe(false); + await expect(governance.getPayloadProposalStatus(arbitraryPayload)).resolves.toBe('none'); }); - it('returns true when a live proposal unwraps to the queried payload', async () => { + it('reports live when a live proposal unwraps to the queried payload', async () => { const original = EthAddress.random(); const wrapper = await etchCode(buildMockWrapperBytecode(original)); @@ -186,29 +229,29 @@ describe('Governance', () => { // The proposal is freshly created, so it should be in `Pending` state await expect(governance.getProposalState(proposalId)).resolves.toBe(ProposalState.Pending); - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(true); + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); }); - it('returns false when no live proposal references the queried payload', async () => { + it('reports none when no proposal references the queried payload', async () => { // Create a proposal for a different payload than the one we query. const proposalOriginal = EthAddress.random(); const wrapper = await etchCode(buildMockWrapperBytecode(proposalOriginal)); await proposeAsProposer(wrapper); const queriedPayload = EthAddress.random(); - await expect(governance.hasActiveProposalWithPayload(queriedPayload.toString())).resolves.toBe(false); + await expect(governance.getPayloadProposalStatus(queriedPayload.toString())).resolves.toBe('none'); }); - it('returns false once the matching proposal reaches a terminal state', async () => { + it('reports none once the matching proposal reaches a re-signalable terminal state', async () => { // No tokens were ever deposited, so no votes can be cast. Once the active phase ends with no - // yea votes the proposal transitions to `Rejected`, which is terminal -- and at that point - // re-signaling/re-proposing is allowed, so `hasActiveProposalWithPayload` must report false. + // yea votes the proposal transitions to `Rejected`, which allows re-signaling/re-proposing, + // so `getPayloadProposalStatus` must report `none`. const original = EthAddress.random(); const wrapper = await etchCode(buildMockWrapperBytecode(original)); const proposalId = await proposeAsProposer(wrapper); - // Pending while the queried payload is in Pending. - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(true); + // Live while the queried payload's proposal is in Pending. + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); // Warp past the active phase so the proposal becomes terminal. We use the proposal's own // frozen config (creation + votingDelay + votingDuration + 1) rather than the live config, @@ -218,7 +261,7 @@ describe('Governance', () => { await cheatCodes.warp(Number(activeThrough + 1n)); await expect(governance.getProposalState(proposalId)).resolves.toBe(ProposalState.Rejected); - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(false); + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('none'); }); it('skips proposals whose payload reverts on getOriginalPayload (proposeWithLock-style)', async () => { @@ -235,7 +278,17 @@ describe('Governance', () => { // The proposal is freshly created, so it should be in `Pending` state await expect(governance.getProposalState(proposalId)).resolves.toBe(ProposalState.Pending); - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(true); + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); + }); + + it('matches a proposeWithLock proposal by its stored payload address directly', async () => { + // proposeWithLock stores the raw payload (no GSEPayload wrapper), so `getOriginalPayload` + // reverts and there is no unwrapped original to compare. The queried payload must still match + // directly against the proposal's stored payload address. + const rawPayload = await etchCode(REVERTING_WRAPPER_BYTECODE); + await proposeAsProposer(rawPayload); + + await expect(governance.getPayloadProposalStatus(rawPayload)).resolves.toBe('live'); }); it('finds a live proposal among multiple unrelated proposals', async () => { @@ -255,7 +308,7 @@ describe('Governance', () => { const noiseWrapper2 = await etchCode(buildMockWrapperBytecode(irrelevantOriginal2)); await proposeAsProposer(noiseWrapper2); - await expect(governance.hasActiveProposalWithPayload(targetOriginal.toString())).resolves.toBe(true); + await expect(governance.getPayloadProposalStatus(targetOriginal.toString())).resolves.toBe('live'); }); it('matches case-insensitively against the original payload address', async () => { @@ -266,10 +319,84 @@ describe('Governance', () => { await proposeAsProposer(wrapper); const upperHex = ('0x' + original.toString().slice(2).toUpperCase()) as Hex; - await expect(governance.hasActiveProposalWithPayload(upperHex)).resolves.toBe(true); + await expect(governance.getPayloadProposalStatus(upperHex)).resolves.toBe('live'); + }); + + it('reports executed when the matching proposal has been executed', async () => { + // Executed requires a full deposit/vote/execute lifecycle that a fresh anvil deployment has + // no voting power for, so we drive the state via a synthetic proposal read. + const original = EthAddress.random(); + const wrapper = await etchCode(buildMockWrapperBytecode(original)); + const creation = await nowOnChain(); + + jest.spyOn(governance, 'getProposalCount').mockResolvedValue(1n); + jest + .spyOn(governance, 'getProposal') + .mockResolvedValue( + makeProposal({ state: ProposalState.Executed, payload: EthAddress.fromString(wrapper), creation }), + ); + + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('executed'); + }); + + it('prefers live over executed when both reference the payload', async () => { + // A payload re-submitted while a prior execution is still in the lookback window must read as + // `live` so the in-flight proposal is not re-signalled prematurely. + const original = EthAddress.random(); + const executedWrapper = EthAddress.fromString(await etchCode(buildMockWrapperBytecode(original))); + const liveWrapper = EthAddress.fromString(await etchCode(buildMockWrapperBytecode(original))); + const creation = await nowOnChain(); + + jest.spyOn(governance, 'getProposalCount').mockResolvedValue(2n); + jest + .spyOn(governance, 'getProposal') + .mockImplementation((id: bigint) => + Promise.resolve( + id === 1n + ? makeProposal({ state: ProposalState.Executed, payload: executedWrapper, creation }) + : makeProposal({ state: ProposalState.Pending, payload: liveWrapper, creation }), + ), + ); + + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); + }); + + it('reports live for a Droppable proposal (blocks re-signalling)', async () => { + // Droppable is neither live nor terminal; conservatively it blocks re-signalling, since the + // proposal can resume its lifecycle if the governanceProposer is restored. + const original = EthAddress.random(); + const wrapper = await etchCode(buildMockWrapperBytecode(original)); + const creation = await nowOnChain(); + + jest.spyOn(governance, 'getProposalCount').mockResolvedValue(1n); + jest + .spyOn(governance, 'getProposal') + .mockResolvedValue( + makeProposal({ state: ProposalState.Droppable, payload: EthAddress.fromString(wrapper), creation }), + ); + + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); + }); + + it('memoizes the executed verdict so it survives the proposal aging past the lookback', async () => { + const original = EthAddress.random(); + const wrapper = await etchCode(buildMockWrapperBytecode(original)); + const creation = await nowOnChain(); + + // First sweep observes the executed proposal; subsequent sweeps see no proposals at all. + jest.spyOn(governance, 'getProposalCount').mockResolvedValueOnce(1n).mockResolvedValue(0n); + jest + .spyOn(governance, 'getProposal') + .mockResolvedValue( + makeProposal({ state: ProposalState.Executed, payload: EthAddress.fromString(wrapper), creation }), + ); + + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('executed'); + // Even with the executed proposal gone from the sweep, the memoized verdict still reports executed. + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('executed'); }); - it('early-stops on the protocol-wide lifetime cap and returns false for old proposals', async () => { + it('early-stops on the protocol-wide lifetime cap and reports none for old proposals', async () => { // Even if a proposal is "live" in the sense that no terminal-state transition has been // recorded, once its creation timestamp is more than 4 * TIME_UPPER (= 360 days) in the past // it cannot possibly still be in Pending/Active/Queued/Executable, because each phase is @@ -280,7 +407,7 @@ describe('Governance', () => { await proposeAsProposer(wrapper); // Live initially. - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(true); + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('live'); // Warp beyond 4 * 90 days so the proposal's creation falls outside the hard cutoff. const FOUR_TIME_UPPER = 4n * 90n * 24n * 3600n; @@ -288,13 +415,13 @@ describe('Governance', () => { await cheatCodes.warp(Number(target)); // We don't assert on getProposalState here; it would return `Rejected` (no votes cast in the - // active phase), but the early-stop in `hasActiveProposalWithPayload` is meant to fire even - // if it were stuck in some non-terminal state, so we test the boolean directly. - await expect(governance.hasActiveProposalWithPayload(original.toString())).resolves.toBe(false); + // active phase), but the early-stop in `getPayloadProposalStatus` is meant to fire even if it + // were stuck in some non-terminal state, so we test the classification directly. + await expect(governance.getPayloadProposalStatus(original.toString())).resolves.toBe('none'); // Sanity: the older live proposals from earlier tests are also past the cutoff now, so the - // sweep should report false for any payload, including the dummy one. - await expect(governance.hasActiveProposalWithPayload(EthAddress.random().toString())).resolves.toBe(false); + // sweep should report none for any payload, including the dummy one. + await expect(governance.getPayloadProposalStatus(EthAddress.random().toString())).resolves.toBe('none'); }); }); @@ -374,7 +501,7 @@ describe('Governance', () => { expect(governance.getProposal).toBeDefined(); expect(governance.getProposalState).toBeDefined(); expect(governance.getProposalCount).toBeDefined(); - expect(governance.hasActiveProposalWithPayload).toBeDefined(); + expect(governance.getPayloadProposalStatus).toBeDefined(); expect(governance.awaitProposalActive).toBeDefined(); expect(governance.awaitProposalExecutable).toBeDefined(); }); diff --git a/yarn-project/ethereum/src/contracts/governance.ts b/yarn-project/ethereum/src/contracts/governance.ts index 61d82efe0a29..d8be35c296fe 100644 --- a/yarn-project/ethereum/src/contracts/governance.ts +++ b/yarn-project/ethereum/src/contracts/governance.ts @@ -37,7 +37,7 @@ export type L1GovernanceContractAddresses = Pick< 'governanceAddress' | 'rollupAddress' | 'registryAddress' | 'governanceProposerAddress' >; -// NOTE: Must be kept in sync with DataStructures.ProposalState in l1-contracts +// NOTE: Must be kept in sync with IGovernance.ProposalState in l1-contracts export enum ProposalState { Pending, Active, @@ -45,10 +45,21 @@ export enum ProposalState { Executable, Rejected, Executed, + Droppable, Dropped, Expired, } +/** + * Outcome of {@link ReadOnlyGovernanceContract.getPayloadProposalStatus} for a queried payload. + * - `'live'`: a proposal referencing the payload is still progressing (Pending/Active/Queued/ + * Executable) or is `Droppable`, so signalling for it again is redundant or premature. + * - `'executed'`: no live proposal references the payload, but one was already executed within the + * bounded lookback (or a prior sweep observed the execution and memoized it). + * - `'none'`: no live or executed proposal references the payload within the bounded lookback. + */ +export type PayloadProposalStatus = 'live' | 'executed' | 'none'; + /** Vote tallies on a single proposal. Both fields are mutated by `Governance.vote`. */ export interface Ballot { yea: bigint; @@ -119,6 +130,17 @@ const TERMINAL_PROPOSAL_STATES: ReadonlySet = new Set([ ProposalState.Expired, ]); +// Set of `ProposalState` values in which a proposal is still progressing towards execution. +// `Droppable` is deliberately excluded: it is neither live (it cannot progress on its own) nor +// terminal (it can resume its lifecycle if the governanceProposer is restored), so it is handled +// separately and is never memoized as immutable. +const LIVE_PROPOSAL_STATES: ReadonlySet = new Set([ + ProposalState.Pending, + ProposalState.Active, + ProposalState.Queued, + ProposalState.Executable, +]); + // Hard upper bound on the wall-clock lifetime of any Governance proposal, in seconds. // Each proposal stores its own snapshot of `ProposalConfiguration` at creation time and progresses // through Pending -> Active -> Queued -> Executable using those frozen durations @@ -173,6 +195,14 @@ export class ReadOnlyGovernanceContract { */ private readonly originalPayloadCache: Map = new Map(); + /** + * Payloads (lowercased hex) observed as the subject of an `Executed` Governance proposal. Execution + * is immutable on-chain, so this verdict never expires within a process. It restores the + * `'executed'` classification for payloads whose executed proposal has since aged past the bounded + * lookback below. Lost on restart, at which point the bounded-lookback limitation applies again. + */ + private readonly executedPayloads: Set = new Set(); + constructor( address: Hex, public readonly client: ViemClient, @@ -263,30 +293,38 @@ export class ReadOnlyGovernanceContract { } /** - * Checks whether the given original payload is currently the subject of a live (non-terminal) - * Governance proposal. Returns true only if a proposal references this payload and is still in - * Pending, Active, Queued, or Executable state. Terminal proposals (Executed, Rejected, Dropped, - * Expired) are ignored, because once a proposal reaches a terminal state the same original - * payload may legitimately be re-signaled and re-submitted via the GovernanceProposer (each round - * is independent and there is no payload-level uniqueness check on-chain). + * Classifies the given original payload against the Governance proposal history. Distinguishes a + * payload that is still the subject of a live proposal (`'live'`) from one whose proposal was + * already executed (`'executed'`) from one that has no relevant proposal (`'none'`), so callers can + * stop re-signalling an executed payload while still re-signalling one whose proposal was merely + * rejected/dropped/expired (each GovernanceProposer round is independent and there is no + * payload-level uniqueness check on-chain). + * + * A proposal matches the payload either directly (its stored `payload` equals the target, as for + * `proposeWithLock` proposals) or via its `GSEPayload` wrapper unwrapping to the target. `'live'` + * (including `Droppable`) takes precedence over `'executed'`, so a payload re-submitted while a + * prior execution is still in the lookback window reads as `'live'`. * * Implemented as a bounded view-call sweep over `Governance.proposals` rather than an event scan, * because `eth_getLogs` over the full deployment history of a long-lived rollup exceeds typical * RPC block-range caps. The number of proposals (`proposalCount`) is small in practice, and we - * walk newest -> oldest with a hard early-stop on the protocol-wide proposal lifetime cap. + * walk newest -> oldest with a hard early-stop on the protocol-wide proposal lifetime cap. This + * makes the `'executed'` verdict a *bounded lookback* rather than permanent suppression: a proposal + * executed longer ago than the lifetime cap is only reported once it has been observed and + * memoized in-process (memo is lost on restart). */ - public async hasActiveProposalWithPayload(payload: Hex): Promise { + public async getPayloadProposalStatus(payload: Hex): Promise { + const target = payload.toLowerCase() as Hex; + const proposalCount = await this.getProposalCount(); if (proposalCount === 0n) { - return false; + return this.executedPayloads.has(target) ? 'executed' : 'none'; } // Anything created before this cutoff is guaranteed terminal regardless of its frozen config. const block = await this.client.getBlock(); const hardCutoff = block.timestamp - MAX_PROPOSAL_LIFETIME_SECONDS; - const target = payload.toLowerCase() as Hex; - // Proposals are append-only with monotonically non-decreasing creation timestamps, so iterating // from newest -> oldest lets us early-stop as soon as we cross the lifetime cutoff. for (let id = proposalCount - 1n; id >= 0n; id--) { @@ -294,24 +332,29 @@ export class ReadOnlyGovernanceContract { // Hard early-stop: every older proposal is also older than the cutoff and therefore terminal. if (proposal.creation < hardCutoff) { - return false; + break; } + const proposalPayload = proposal.payload.toString().toLowerCase(); const original = await this.getOriginalPayload(proposal.payload); - if (original === undefined || original.toLowerCase() !== target) { + const matches = proposalPayload === target || (original !== undefined && original.toLowerCase() === target); + if (!matches) { continue; } - // The wrapper unwraps to our payload. Only treat this as "already proposed" if the proposal - // is still live -- terminal states allow re-proposing the same payload in a later round. - if (TERMINAL_PROPOSAL_STATES.has(proposal.state)) { - continue; + // A live or Droppable proposal blocks re-signalling and wins over any executed one. + if (LIVE_PROPOSAL_STATES.has(proposal.state) || proposal.state === ProposalState.Droppable) { + return 'live'; + } + + if (proposal.state === ProposalState.Executed) { + this.executedPayloads.add(target); } - return true; + // Rejected/Dropped/Expired proposals allow re-proposing the same payload; keep scanning. } - return false; + return this.executedPayloads.has(target) ? 'executed' : 'none'; } /** diff --git a/yarn-project/ethereum/src/contracts/governance_proposer.ts b/yarn-project/ethereum/src/contracts/governance_proposer.ts index b9e169475854..63eb242bd93c 100644 --- a/yarn-project/ethereum/src/contracts/governance_proposer.ts +++ b/yarn-project/ethereum/src/contracts/governance_proposer.ts @@ -15,7 +15,7 @@ import { import type { L1TxRequest, L1TxUtils } from '../l1_tx_utils/index.js'; import type { ViemClient } from '../types.js'; import { type IEmpireBase, encodeSignal, encodeSignalWithSignature, signSignalWithSig } from './empire_base.js'; -import { ReadOnlyGovernanceContract, extractProposalIdFromLogs } from './governance.js'; +import { type PayloadProposalStatus, ReadOnlyGovernanceContract, extractProposalIdFromLogs } from './governance.js'; export class GovernanceProposerContract implements IEmpireBase { private readonly proposer: GetContractReturnType; @@ -38,16 +38,16 @@ export class GovernanceProposerContract implements IEmpireBase { this.proposer = getContract({ address, abi: GovernanceProposerAbi, client }); } - public get address() { + public get address(): EthAddress { return EthAddress.fromString(this.proposer.address); } - public async getRollupAddress() { + public async getRollupAddress(): Promise { return EthAddress.fromString(await this.proposer.read.getInstance()); } @memoize - public async getRegistryAddress() { + public async getRegistryAddress(): Promise { return EthAddress.fromString(await this.proposer.read.REGISTRY()); } @@ -59,10 +59,6 @@ export class GovernanceProposerContract implements IEmpireBase { return this.proposer.read.ROUND_SIZE(); } - public getInstance() { - return this.proposer.read.getInstance(); - } - public computeRound(slot: SlotNumber): Promise { return this.proposer.read.computeRound([BigInt(slot)]); } @@ -107,7 +103,7 @@ export class GovernanceProposerContract implements IEmpireBase { signer, payload, slot, - await this.getInstance(), + (await this.getRollupAddress()).toString(), this.address.toString(), chainId, ); @@ -130,15 +126,15 @@ export class GovernanceProposerContract implements IEmpireBase { } /** - * Returns true iff the given original payload is currently the subject of a live (non-terminal) - * Governance proposal. Delegates to `ReadOnlyGovernanceContract.hasActiveProposalWithPayload`, which - * implements the actual sweep against the Governance contract -- this method exists only as a - * convenience wrapper so callers that already hold a GovernanceProposer reference don't have to + * Classifies the given original payload against the Governance proposal history (`'live'` / + * `'executed'` / `'none'`). Delegates to `ReadOnlyGovernanceContract.getPayloadProposalStatus`, + * which implements the actual sweep against the Governance contract -- this method exists only as + * a convenience wrapper so callers that already hold a GovernanceProposer reference don't have to * resolve the Governance address themselves. */ - public async hasActiveProposalWithPayload(payload: Hex): Promise { + public async getPayloadProposalStatus(payload: Hex): Promise { const governance = await this.getGovernance(); - return governance.hasActiveProposalWithPayload(payload); + return governance.getPayloadProposalStatus(payload); } /** diff --git a/yarn-project/foundation/src/config/env_var.ts b/yarn-project/foundation/src/config/env_var.ts index 1b1e9cc98f7d..e4d9ae1c535b 100644 --- a/yarn-project/foundation/src/config/env_var.ts +++ b/yarn-project/foundation/src/config/env_var.ts @@ -84,6 +84,7 @@ export type EnvVar = | 'ETHEREUM_ALLOW_NO_DEBUG_HOSTS' | 'FEE_RECIPIENT' | 'FORCE_COLOR' + | 'GOVERNANCE_PROPOSER_FORCE_PAYLOAD_VOTE' | 'GOVERNANCE_PROPOSER_PAYLOAD_ADDRESS' | 'KEY_STORE_DIRECTORY' | 'L1_CHAIN_ID' diff --git a/yarn-project/sequencer-client/src/config.ts b/yarn-project/sequencer-client/src/config.ts index 09c73d6625f2..0477512bb2f3 100644 --- a/yarn-project/sequencer-client/src/config.ts +++ b/yarn-project/sequencer-client/src/config.ts @@ -64,6 +64,7 @@ export const DefaultSequencerConfig = { injectUnrecoverableSignatureAttestation: false, injectYParityAttestation: false, fishermanMode: false, + governanceProposerForcePayloadVote: false, shuffleAttestationOrdering: false, skipPushProposedBlocksToArchiver: false, skipPublishingCheckpointsPercent: 0, @@ -161,6 +162,12 @@ export const sequencerConfigMappings: ConfigMappingsType = { description: 'The address of the payload for the governanceProposer', parseEnv: (val: string) => EthAddress.fromString(val), }, + governanceProposerForcePayloadVote: { + env: 'GOVERNANCE_PROPOSER_FORCE_PAYLOAD_VOTE', + description: + 'Keep signalling the configured governance payload even if a proposal referencing it was already executed.', + ...booleanConfigHelper(DefaultSequencerConfig.governanceProposerForcePayloadVote), + }, l1PublishingTime: { env: 'SEQ_L1_PUBLISHING_TIME_ALLOWANCE_IN_SLOT', description: 'How much time in seconds to allow in the slot for publishing the L1 transaction.', diff --git a/yarn-project/sequencer-client/src/publisher/sequencer-publisher.test.ts b/yarn-project/sequencer-client/src/publisher/sequencer-publisher.test.ts index 363a60540308..b5c62f78c9e3 100644 --- a/yarn-project/sequencer-client/src/publisher/sequencer-publisher.test.ts +++ b/yarn-project/sequencer-client/src/publisher/sequencer-publisher.test.ts @@ -156,6 +156,9 @@ describe('SequencerPublisher', () => { l1Metrics = mock(); governanceProposerContract = mock(); + // By default the configured rollup is the canonical instance, so the canonicality guard passes. + governanceProposerContract.getRollupAddress.mockResolvedValue(EthAddress.fromString(mockRollupAddress)); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('none'); epochCache = mock(); epochCache.getEpochAndSlotNow.mockReturnValue({ epoch: EpochNumber(1), slot: SlotNumber(2), ts: 3n, nowMs: 3000n }); @@ -970,9 +973,9 @@ describe('SequencerPublisher', () => { ).toEqual(false); }); - it('stops signalling when payload was previously proposed', async () => { + it('stops signalling when payload has a live proposal', async () => { const { govPayload } = mockGovernancePayload(); - governanceProposerContract.hasActiveProposalWithPayload.mockResolvedValue(true); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('live'); expect( await publisher.enqueueGovernanceCastSignal( @@ -984,9 +987,9 @@ describe('SequencerPublisher', () => { ).toEqual(false); }); - it('continues signalling when payload was NOT proposed', async () => { + it('continues signalling when payload has no proposal', async () => { const { govPayload } = mockGovernancePayload(); - governanceProposerContract.hasActiveProposalWithPayload.mockResolvedValue(false); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('none'); expect( await publisher.enqueueGovernanceCastSignal( @@ -998,13 +1001,75 @@ describe('SequencerPublisher', () => { ).toEqual(true); }); + it('stops signalling when the payload was already executed by governance', async () => { + // Regression for a mainnet incident: a node kept signalling a payload whose proposal had already + // been executed, wasting gas on a signal the canonical rollup would reject every slot. + const { govPayload } = mockGovernancePayload(); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('executed'); + + expect( + await publisher.enqueueGovernanceCastSignal( + govPayload, + SlotNumber(2), + EthAddress.fromString(testHarnessAttesterAccount.address), + msg => testHarnessAttesterAccount.signTypedData(msg), + ), + ).toEqual(false); + }); + + it('signals an executed payload when the force flag is set', async () => { + // For payloads deliberately designed to be re-executed, operators can opt back into signalling. + const { govPayload } = mockGovernancePayload(); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('executed'); + (publisher as any).config.governanceProposerForcePayloadVote = true; + + expect( + await publisher.enqueueGovernanceCastSignal( + govPayload, + SlotNumber(2), + EthAddress.fromString(testHarnessAttesterAccount.address), + msg => testHarnessAttesterAccount.signTypedData(msg), + ), + ).toEqual(true); + }); + + it('still stops signalling a live payload even with the force flag set', async () => { + // The force flag only overrides the executed verdict; a live proposal must still suppress the signal. + const { govPayload } = mockGovernancePayload(); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValue('live'); + (publisher as any).config.governanceProposerForcePayloadVote = true; + + expect( + await publisher.enqueueGovernanceCastSignal( + govPayload, + SlotNumber(2), + EthAddress.fromString(testHarnessAttesterAccount.address), + msg => testHarnessAttesterAccount.signTypedData(msg), + ), + ).toEqual(false); + }); + + it('does not signal when the configured rollup is not canonical', async () => { + const { govPayload } = mockGovernancePayload(); + governanceProposerContract.getRollupAddress.mockResolvedValue(EthAddress.random()); + + expect( + await publisher.enqueueGovernanceCastSignal( + govPayload, + SlotNumber(2), + EthAddress.fromString(testHarnessAttesterAccount.address), + msg => testHarnessAttesterAccount.signTypedData(msg), + ), + ).toEqual(false); + }); + it('re-checks on every call without caching, so re-signaling resumes if a proposal becomes terminal', async () => { const { govPayload } = mockGovernancePayload(); - // Simulates a payload that has a live proposal in slot 2 but whose proposal becomes terminal - // (Dropped/Rejected/Expired/Executed) by slot 3. The contracts allow re-signaling the same - // payload in a later round once the previous proposal is dead, so the publisher must re-check - // each slot rather than cache the first `true` result indefinitely. - governanceProposerContract.hasActiveProposalWithPayload.mockResolvedValueOnce(true).mockResolvedValueOnce(false); + // Simulates a payload with a live proposal in slot 2 whose proposal becomes re-signalable (e.g. + // Rejected/Dropped/Expired) by slot 3. The contracts allow re-signaling the same payload in a + // later round once the previous proposal is dead, so the publisher must re-check each slot + // rather than cache the first `live` result indefinitely. + governanceProposerContract.getPayloadProposalStatus.mockResolvedValueOnce('live').mockResolvedValueOnce('none'); expect( await publisher.enqueueGovernanceCastSignal( @@ -1024,7 +1089,7 @@ describe('SequencerPublisher', () => { ), ).toEqual(true); - expect(governanceProposerContract.hasActiveProposalWithPayload).toHaveBeenCalledTimes(2); + expect(governanceProposerContract.getPayloadProposalStatus).toHaveBeenCalledTimes(2); }); it('fails open on persistent RPC failure and signals anyway', async () => { @@ -1032,7 +1097,7 @@ describe('SequencerPublisher', () => { // silence governance participation entirely. Failing open at worst produces a duplicate signal // that the contract simply counts alongside others in the round. const { govPayload } = mockGovernancePayload(); - governanceProposerContract.hasActiveProposalWithPayload.mockRejectedValue(new Error('RPC error')); + governanceProposerContract.getPayloadProposalStatus.mockRejectedValue(new Error('RPC error')); expect( await publisher.enqueueGovernanceCastSignal( @@ -1044,11 +1109,11 @@ describe('SequencerPublisher', () => { ).toEqual(true); }); - it('re-checks each call (no caching of false results)', async () => { + it('re-checks each call (no caching of none results)', async () => { const { govPayload } = mockGovernancePayload(); - governanceProposerContract.hasActiveProposalWithPayload.mockResolvedValueOnce(false).mockResolvedValueOnce(true); + governanceProposerContract.getPayloadProposalStatus.mockResolvedValueOnce('none').mockResolvedValueOnce('live'); - // First call: no live proposal, signalling proceeds + // First call: no proposal, signalling proceeds expect( await publisher.enqueueGovernanceCastSignal( govPayload, @@ -1068,7 +1133,7 @@ describe('SequencerPublisher', () => { ), ).toEqual(false); - expect(governanceProposerContract.hasActiveProposalWithPayload).toHaveBeenCalledTimes(2); + expect(governanceProposerContract.getPayloadProposalStatus).toHaveBeenCalledTimes(2); }); describe('enqueuePruneIfPrunable', () => { diff --git a/yarn-project/sequencer-client/src/publisher/sequencer-publisher.ts b/yarn-project/sequencer-client/src/publisher/sequencer-publisher.ts index e57c193407eb..5b9e273df332 100644 --- a/yarn-project/sequencer-client/src/publisher/sequencer-publisher.ts +++ b/yarn-project/sequencer-client/src/publisher/sequencer-publisher.ts @@ -8,6 +8,7 @@ import { MULTI_CALL_3_ADDRESS, Multicall3, MulticallForwarderRevertedError, + type PayloadProposalStatus, type RollupContract, type SimulationOverridesPlan, type SlashingProposerContract, @@ -45,6 +46,7 @@ import { EmpireBaseAbi, ErrorsAbi, RollupAbi, SlashingProposerAbi } from '@aztec import { type ProposerSlashAction, encodeSlashConsensusVotes } from '@aztec/slasher'; import { CommitteeAttestationsAndSigners, type ValidateCheckpointResult } from '@aztec/stdlib/block'; import type { Checkpoint } from '@aztec/stdlib/checkpoint'; +import type { SequencerConfig } from '@aztec/stdlib/config'; import { getLastL1SlotTimestampForL2Slot, getNextL1SlotTimestamp, @@ -215,6 +217,7 @@ export class SequencerPublisher { | 'sequencerPublisherPreviousL1BlockWaitTimeoutMs' | 'sequencerPublisherPreviousL1BlockWaitPollIntervalMs' > & + Pick & Pick & { l1ChainId: number }, deps: { telemetry?: TelemetryClient; @@ -958,6 +961,19 @@ export class SequencerPublisher { this.log.warn(`Cannot enqueue vote cast signal ${signalType} for address zero at slot ${slotNumber}`); return false; } + + const canonicalRollup = await base.getRollupAddress(); + if (!canonicalRollup.equals(EthAddress.fromString(this.rollupContract.address))) { + this.log.warn(`Rollup ${this.rollupContract.address} is not canonical, skipping governance signal`, { + slotNumber, + signalType, + canonicalRollup, + targetRollup: this.rollupContract.address, + payload: payload.toString(), + }); + return false; + } + const round = await base.computeRound(slotNumber); const roundInfo = await base.getRoundInfo(this.rollupContract.address, round); @@ -974,23 +990,38 @@ export class SequencerPublisher { return false; } - // Skip signaling if there is already a live (non-terminal) Governance proposal for this - // payload. This is intentionally not cached: a previously-live proposal may transition to - // a terminal state (Dropped/Rejected/Expired/Executed), at which point we may want to re-signal - // the same payload in a future round. - let proposed = false; + // Classify the payload against the Governance proposal history so we stop signalling once its + // proposal is live or was already executed, while still re-signalling one whose proposal was + // merely rejected/dropped/expired. + let status: PayloadProposalStatus = 'none'; try { - proposed = await base.hasActiveProposalWithPayload(payload.toString()); + status = await base.getPayloadProposalStatus(payload.toString()); } catch (err) { // We deliberately swallow the error and proceed to signal. Failing closed (skipping the // signal) on transient RPC errors would let a flaky L1 endpoint silence governance // participation entirely; failing open at worst produces a duplicate signal that the // contract will simply count alongside others in the round. - this.log.error(`Failed to check if payload ${payload} was already proposed (signalling anyway)`, err); + this.log.error(`Failed to check governance proposal status for payload ${payload} (signalling anyway)`, err, { + slotNumber, + signalType, + }); } - if (proposed) { - this.log.info(`Payload ${payload} has a live governance proposal, stopping signals`); + if (status === 'live') { + this.log.info(`Payload ${payload} has a live governance proposal, stopping signals`, { + slotNumber, + signalType, + payload: payload.toString(), + }); + return false; + } + + if (status === 'executed' && !this.config.governanceProposerForcePayloadVote) { + this.log.info( + `Payload ${payload} was executed by governance within lookback, stopping signals ` + + `(set GOVERNANCE_PROPOSER_FORCE_PAYLOAD_VOTE to re-signal)`, + { slotNumber, signalType, payload: payload.toString() }, + ); return false; } diff --git a/yarn-project/sequencer-client/src/sequencer/checkpoint_voter.ha.integration.test.ts b/yarn-project/sequencer-client/src/sequencer/checkpoint_voter.ha.integration.test.ts index 680c05bef622..199fd34eaa3a 100644 --- a/yarn-project/sequencer-client/src/sequencer/checkpoint_voter.ha.integration.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/checkpoint_voter.ha.integration.test.ts @@ -72,6 +72,8 @@ describe('CheckpointVoter HA Integration', () => { function createMockGovernanceContract(): MockProxy { const contract = mock(); Object.defineProperty(contract, 'address', { value: EthAddress.random(), writable: false }); + contract.getRollupAddress.mockResolvedValue(EthAddress.fromString(rollupContract.address)); + contract.getPayloadProposalStatus.mockResolvedValue('none'); contract.getRoundInfo.mockResolvedValue({ lastSignalSlot: SlotNumber(1), payloadWithMostSignals: EthAddress.ZERO.toString(), @@ -176,7 +178,7 @@ describe('CheckpointVoter HA Integration', () => { // Set up mocks using helper functions rollupContract = mock(); - Object.defineProperty(rollupContract, 'address', { value: EthAddress.random(), writable: false }); + Object.defineProperty(rollupContract, 'address', { value: EthAddress.random().toString(), writable: false }); rollupContract.listenToSlasherChanged.mockReturnValue(undefined as any); rollupContract.getSlashingProposer.mockResolvedValue(undefined); diff --git a/yarn-project/stdlib/src/interfaces/configs.ts b/yarn-project/stdlib/src/interfaces/configs.ts index 14602960be57..67a133f1fb3f 100644 --- a/yarn-project/stdlib/src/interfaces/configs.ts +++ b/yarn-project/stdlib/src/interfaces/configs.ts @@ -49,6 +49,11 @@ export interface SequencerConfig { txPublicSetupAllowListExtend?: AllowedElement[]; /** Payload address to vote for */ governanceProposerPayload?: EthAddress; + /** + * Keep signalling the configured governance payload even if a proposal referencing it was already + * executed within the lookback window. For payloads deliberately designed to be re-executed. + */ + governanceProposerForcePayloadVote?: boolean; /** * Minimum block-building time (`min_block_duration`) still worth allocating if the proposer starts * late, in seconds. @@ -150,6 +155,7 @@ export const SequencerConfigSchema = zodFor()( acvmBinaryPath: z.string().optional(), txPublicSetupAllowListExtend: z.array(AllowedElementSchema).optional(), governanceProposerPayload: schemas.EthAddress.optional(), + governanceProposerForcePayloadVote: z.boolean().optional(), minBlockDuration: z.number().positive().optional(), checkpointProposalPrepareTime: z.number().nonnegative().optional(), l1PublishingTime: z.number().optional(),