From 946a418d138c1b2bee3a5dc14096616a902cc0b7 Mon Sep 17 00:00:00 2001
From: Santiago Palladino <santiago@aztecprotocol.com>
Date: Wed, 15 Jan 2025 16:26:54 -0300
Subject: [PATCH] chore!: Attestation collection times out based on sequencer
 timetable (#11248)

Removes the env var `VALIDATOR_ATTESTATIONS_WAIT_TIMEOUT_MS` and
replaces it with the deadline for publishing the block to L1. This
ensures the sequencer does not get stuck waiting for attestations where
the validators have given up due to a reexecution timeout.

If timetable is not enforced, this value defaults to one aztec slot.
---
 spartan/releases/README.md                    |  1 -
 yarn-project/foundation/src/config/env_var.ts |  1 -
 .../src/sequencer/sequencer.ts                | 10 +++++-
 yarn-project/validator-client/src/config.ts   | 14 +-------
 yarn-project/validator-client/src/factory.ts  |  2 +-
 .../validator-client/src/validator.test.ts    | 19 ++++++----
 .../validator-client/src/validator.ts         | 35 ++++++++++---------
 7 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/spartan/releases/README.md b/spartan/releases/README.md
index b368c4c723f..b1b134e9a93 100644
--- a/spartan/releases/README.md
+++ b/spartan/releases/README.md
@@ -83,7 +83,6 @@ The Sequencer Client is a criticial component that coordinates tx validation, L2
 |Variable| Description|
 |----|-----|
 | VALIDATOR_DISABLED | If this is True, the client won't perform any validator duties. |
-|VALIDATOR_ATTESTATIONS_WAIT_TIMEOUT_MS | Wait for attestations timeout. After this, client throws an error and does not propose a block for that slot. |
 | VALIDATOR_ATTESTATIONS_POLLING_INTERVAL_MS | If not enough attestations, sleep for this long and check again |
 |GOVERNANCE_PROPOSER_PAYLOAD_ADDRESS | To nominate proposals for voting, you must set this variable to the Ethereum address of the `proposal` payload. You must edit this to vote on a governance upgrade.|
 | SEQ_ENFORCE_TIME_TABLE | Whether to enforce strict timeliness requirement when building blocks. Refer [here](#sequencer-timeliness-requirements) for more on the timetable |
diff --git a/yarn-project/foundation/src/config/env_var.ts b/yarn-project/foundation/src/config/env_var.ts
index f33e808bfb6..1862daa6bea 100644
--- a/yarn-project/foundation/src/config/env_var.ts
+++ b/yarn-project/foundation/src/config/env_var.ts
@@ -160,7 +160,6 @@ export type EnvVar =
   | 'TX_GOSSIP_VERSION'
   | 'TXE_PORT'
   | 'VALIDATOR_ATTESTATIONS_POLLING_INTERVAL_MS'
-  | 'VALIDATOR_ATTESTATIONS_WAIT_TIMEOUT_MS'
   | 'VALIDATOR_DISABLED'
   | 'VALIDATOR_PRIVATE_KEY'
   | 'VALIDATOR_REEXECUTE'
diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.ts
index 96cdeef225f..4b1515a01aa 100644
--- a/yarn-project/sequencer-client/src/sequencer/sequencer.ts
+++ b/yarn-project/sequencer-client/src/sequencer/sequencer.ts
@@ -641,7 +641,15 @@ export class Sequencer {
     this.log.debug('Broadcasting block proposal to validators');
     this.validatorClient.broadcastBlockProposal(proposal);
 
-    const attestations = await this.validatorClient.collectAttestations(proposal, numberOfRequiredAttestations);
+    const attestationTimeAllowed = this.enforceTimeTable
+      ? this.timetable.getMaxAllowedTime(SequencerState.PUBLISHING_BLOCK)!
+      : this.aztecSlotDuration;
+    const attestationDeadline = new Date(this.dateProvider.now() + attestationTimeAllowed * 1000);
+    const attestations = await this.validatorClient.collectAttestations(
+      proposal,
+      numberOfRequiredAttestations,
+      attestationDeadline,
+    );
 
     // note: the smart contract requires that the signatures are provided in the order of the committee
     return orderAttestations(attestations, committee);
diff --git a/yarn-project/validator-client/src/config.ts b/yarn-project/validator-client/src/config.ts
index 075f6249920..f083ec499bd 100644
--- a/yarn-project/validator-client/src/config.ts
+++ b/yarn-project/validator-client/src/config.ts
@@ -1,10 +1,9 @@
-import { NULL_KEY, l1ContractsConfigMappings } from '@aztec/ethereum';
+import { NULL_KEY } from '@aztec/ethereum';
 import {
   type ConfigMappingsType,
   booleanConfigHelper,
   getConfigFromMappings,
   numberConfigHelper,
-  pickConfigMappings,
 } from '@aztec/foundation/config';
 
 /**
@@ -20,9 +19,6 @@ export interface ValidatorClientConfig {
   /** Interval between polling for new attestations from peers */
   attestationPollingIntervalMs: number;
 
-  /** Wait for attestations timeout */
-  attestationWaitTimeoutMs: number;
-
   /** Re-execute transactions before attesting */
   validatorReexecute: boolean;
 }
@@ -43,14 +39,6 @@ export const validatorClientConfigMappings: ConfigMappingsType<ValidatorClientCo
     description: 'Interval between polling for new attestations',
     ...numberConfigHelper(200),
   },
-  attestationWaitTimeoutMs: {
-    env: 'VALIDATOR_ATTESTATIONS_WAIT_TIMEOUT_MS',
-    description: 'Wait for attestations timeout',
-    ...numberConfigHelper(
-      getConfigFromMappings(pickConfigMappings(l1ContractsConfigMappings, ['aztecSlotDuration'])).aztecSlotDuration *
-        1000,
-    ),
-  },
   validatorReexecute: {
     env: 'VALIDATOR_REEXECUTE',
     description: 'Re-execute transactions before attesting',
diff --git a/yarn-project/validator-client/src/factory.ts b/yarn-project/validator-client/src/factory.ts
index 3e4bf64acf7..bd29a1cf312 100644
--- a/yarn-project/validator-client/src/factory.ts
+++ b/yarn-project/validator-client/src/factory.ts
@@ -24,5 +24,5 @@ export function createValidatorClient(
     config.validatorPrivateKey = generatePrivateKey();
   }
 
-  return ValidatorClient.new(config, deps.epochCache, deps.p2pClient, deps.telemetry);
+  return ValidatorClient.new(config, deps.epochCache, deps.p2pClient, deps.dateProvider, deps.telemetry);
 }
diff --git a/yarn-project/validator-client/src/validator.test.ts b/yarn-project/validator-client/src/validator.test.ts
index 7b6fdca5a26..341abf1fb01 100644
--- a/yarn-project/validator-client/src/validator.test.ts
+++ b/yarn-project/validator-client/src/validator.test.ts
@@ -7,8 +7,8 @@ import { type EpochCache } from '@aztec/epoch-cache';
 import { Secp256k1Signer } from '@aztec/foundation/crypto';
 import { EthAddress } from '@aztec/foundation/eth-address';
 import { Fr } from '@aztec/foundation/fields';
+import { TestDateProvider } from '@aztec/foundation/timer';
 import { type P2P } from '@aztec/p2p';
-import { NoopTelemetryClient } from '@aztec/telemetry-client/noop';
 
 import { describe, expect, it } from '@jest/globals';
 import { type MockProxy, mock } from 'jest-mock-extended';
@@ -30,11 +30,13 @@ describe('ValidationService', () => {
   let p2pClient: MockProxy<P2P>;
   let epochCache: MockProxy<EpochCache>;
   let validatorAccount: PrivateKeyAccount;
+  let dateProvider: TestDateProvider;
 
   beforeEach(() => {
     p2pClient = mock<P2P>();
     p2pClient.getAttestationsForSlot.mockImplementation(() => Promise.resolve([]));
     epochCache = mock<EpochCache>();
+    dateProvider = new TestDateProvider();
 
     const validatorPrivateKey = generatePrivateKey();
     validatorAccount = privateKeyToAccount(validatorPrivateKey);
@@ -42,16 +44,15 @@ describe('ValidationService', () => {
     config = {
       validatorPrivateKey: validatorPrivateKey,
       attestationPollingIntervalMs: 1000,
-      attestationWaitTimeoutMs: 1000,
       disableValidator: false,
       validatorReexecute: false,
     };
-    validatorClient = ValidatorClient.new(config, epochCache, p2pClient, new NoopTelemetryClient());
+    validatorClient = ValidatorClient.new(config, epochCache, p2pClient, dateProvider);
   });
 
   it('Should throw error if an invalid private key is provided', () => {
     config.validatorPrivateKey = '0x1234567890123456789';
-    expect(() => ValidatorClient.new(config, epochCache, p2pClient, new NoopTelemetryClient())).toThrow(
+    expect(() => ValidatorClient.new(config, epochCache, p2pClient, dateProvider)).toThrow(
       InvalidValidatorPrivateKeyError,
     );
   });
@@ -79,7 +80,9 @@ describe('ValidationService', () => {
   it('Should a timeout if we do not collect enough attestations in time', async () => {
     const proposal = makeBlockProposal();
 
-    await expect(validatorClient.collectAttestations(proposal, 2)).rejects.toThrow(AttestationTimeoutError);
+    await expect(validatorClient.collectAttestations(proposal, 2, new Date(dateProvider.now() + 100))).rejects.toThrow(
+      AttestationTimeoutError,
+    );
   });
 
   it('Should throw an error if the transactions are not available', async () => {
@@ -200,7 +203,11 @@ describe('ValidationService', () => {
 
     // Perform the query
     const numberOfRequiredAttestations = 3;
-    const attestations = await validatorClient.collectAttestations(proposal, numberOfRequiredAttestations);
+    const attestations = await validatorClient.collectAttestations(
+      proposal,
+      numberOfRequiredAttestations,
+      new Date(dateProvider.now() + 5000),
+    );
 
     expect(attestations).toHaveLength(numberOfRequiredAttestations);
   });
diff --git a/yarn-project/validator-client/src/validator.ts b/yarn-project/validator-client/src/validator.ts
index 6d5d55a4553..2dcd79d346b 100644
--- a/yarn-project/validator-client/src/validator.ts
+++ b/yarn-project/validator-client/src/validator.ts
@@ -6,7 +6,7 @@ import { type Fr } from '@aztec/foundation/fields';
 import { createLogger } from '@aztec/foundation/log';
 import { RunningPromise } from '@aztec/foundation/running-promise';
 import { sleep } from '@aztec/foundation/sleep';
-import { type Timer } from '@aztec/foundation/timer';
+import { DateProvider, type Timer } from '@aztec/foundation/timer';
 import { type P2P } from '@aztec/p2p';
 import { BlockProposalValidator } from '@aztec/p2p/msg_validators';
 import { type TelemetryClient, WithTracer } from '@aztec/telemetry-client';
@@ -55,7 +55,7 @@ export interface Validator {
   attestToProposal(proposal: BlockProposal): void;
 
   broadcastBlockProposal(proposal: BlockProposal): void;
-  collectAttestations(proposal: BlockProposal, numberOfRequiredAttestations: number): Promise<BlockAttestation[]>;
+  collectAttestations(proposal: BlockProposal, required: number, deadline: Date): Promise<BlockAttestation[]>;
 }
 
 /**
@@ -80,6 +80,7 @@ export class ValidatorClient extends WithTracer implements Validator {
     private epochCache: EpochCache,
     private p2pClient: P2P,
     private config: ValidatorClientConfig,
+    private dateProvider: DateProvider = new DateProvider(),
     telemetry: TelemetryClient = new NoopTelemetryClient(),
     private log = createLogger('validator'),
   ) {
@@ -119,6 +120,7 @@ export class ValidatorClient extends WithTracer implements Validator {
     config: ValidatorClientConfig,
     epochCache: EpochCache,
     p2pClient: P2P,
+    dateProvider: DateProvider = new DateProvider(),
     telemetry: TelemetryClient = new NoopTelemetryClient(),
   ) {
     if (!config.validatorPrivateKey) {
@@ -128,7 +130,7 @@ export class ValidatorClient extends WithTracer implements Validator {
     const privateKey = validatePrivateKey(config.validatorPrivateKey);
     const localKeyStore = new LocalKeyStore(privateKey);
 
-    const validator = new ValidatorClient(localKeyStore, epochCache, p2pClient, config, telemetry);
+    const validator = new ValidatorClient(localKeyStore, epochCache, p2pClient, config, dateProvider, telemetry);
     validator.registerBlockProposalHandler();
     return validator;
   }
@@ -309,19 +311,21 @@ export class ValidatorClient extends WithTracer implements Validator {
   }
 
   // TODO(https://github.com/AztecProtocol/aztec-packages/issues/7962)
-  async collectAttestations(
-    proposal: BlockProposal,
-    numberOfRequiredAttestations: number,
-  ): Promise<BlockAttestation[]> {
+  async collectAttestations(proposal: BlockProposal, required: number, deadline: Date): Promise<BlockAttestation[]> {
     // Wait and poll the p2pClient's attestation pool for this block until we have enough attestations
     const slot = proposal.payload.header.globalVariables.slotNumber.toBigInt();
-    this.log.debug(`Collecting ${numberOfRequiredAttestations} attestations for slot ${slot}`);
+    this.log.debug(`Collecting ${required} attestations for slot ${slot} with deadline ${deadline.toISOString()}`);
+
+    if (+deadline < this.dateProvider.now()) {
+      this.log.error(
+        `Deadline ${deadline.toISOString()} for collecting ${required} attestations for slot ${slot} is in the past`,
+      );
+      throw new AttestationTimeoutError(required, slot);
+    }
 
     const proposalId = proposal.archive.toString();
     const myAttestation = await this.validationService.attestToProposal(proposal);
 
-    const startTime = Date.now();
-
     let attestations: BlockAttestation[] = [];
     while (true) {
       const collectedAttestations = [myAttestation, ...(await this.p2pClient.getAttestationsForSlot(slot, proposalId))];
@@ -333,15 +337,14 @@ export class ValidatorClient extends WithTracer implements Validator {
       }
       attestations = collectedAttestations;
 
-      if (attestations.length >= numberOfRequiredAttestations) {
-        this.log.verbose(`Collected all ${numberOfRequiredAttestations} attestations for slot ${slot}`);
+      if (attestations.length >= required) {
+        this.log.verbose(`Collected all ${required} attestations for slot ${slot}`);
         return attestations;
       }
 
-      const elapsedTime = Date.now() - startTime;
-      if (elapsedTime > this.config.attestationWaitTimeoutMs) {
-        this.log.error(`Timeout waiting for ${numberOfRequiredAttestations} attestations for slot ${slot}`);
-        throw new AttestationTimeoutError(numberOfRequiredAttestations, slot);
+      if (+deadline < this.dateProvider.now()) {
+        this.log.error(`Timeout ${deadline.toISOString()} waiting for ${required} attestations for slot ${slot}`);
+        throw new AttestationTimeoutError(required, slot);
       }
 
       this.log.debug(`Collected ${attestations.length} attestations so far`);