fix: potential e2e-p2p fix

yarn-project/end-to-end/scripts/e2e_test_config.yml

@@ -85,8 +85,8 @@ tests:
   e2e_p2p_upgrade_governance_proposer:
     test_path: 'e2e_p2p/upgrade_governance_proposer.test.ts'
   # https://github.com/AztecProtocol/aztec-packages/issues/9843
-  # e2e_p2p_rediscovery:
-  #   test_path: 'e2e_p2p/rediscovery.test.ts'
+  e2e_p2p_rediscovery:
+    test_path: 'e2e_p2p/rediscovery.test.ts'
   e2e_p2p_reqresp:
     test_path: 'e2e_p2p/reqresp.test.ts'
   flakey_e2e_tests:

yarn-project/end-to-end/src/e2e_p2p/gossip_network.test.ts

@@ -3,7 +3,7 @@ import { sleep } from '@aztec/aztec.js';
 
 import fs from 'fs';
 
-// import { METRICS_PORT } from '../fixtures/fixtures.js';
+import { shouldCollectMetrics } from '../fixtures/fixtures.js';
 import { type NodeContext, createNodes } from '../fixtures/setup_p2p_test.js';
 import { P2PNetworkTest, WAIT_FOR_TX_TIMEOUT } from './p2p_network.js';
 import { createPXEServiceAndSubmitTransactions } from './shared.js';
@@ -24,8 +24,8 @@ describe('e2e_p2p_network', () => {
       testName: 'e2e_p2p_network',
       numberOfNodes: NUM_NODES,
       basePort: BOOT_NODE_UDP_PORT,
-      // Uncomment to collect metrics - run in aztec-packages `docker compose --profile metrics up`
-      // metricsPort: METRICS_PORT,
+      // To collect metrics - run in aztec-packages `docker compose --profile metrics up` and set COLLECT_METRICS=true
+      metricsPort: shouldCollectMetrics(),
     });
     await t.applyBaseSnapshots();
     await t.setup();
@@ -44,6 +44,7 @@ describe('e2e_p2p_network', () => {
     if (!t.bootstrapNodeEnr) {
       throw new Error('Bootstrap node ENR is not available');
     }
+
     // create our network of nodes and submit txs into each of them
     // the number of txs per node and the number of txs per rollup
     // should be set so that the only way for rollups to be built
@@ -57,8 +58,8 @@ describe('e2e_p2p_network', () => {
       NUM_NODES,
       BOOT_NODE_UDP_PORT,
       DATA_DIR,
-      // Uncomment to collect metrics - run in aztec-packages `docker compose --profile metrics up`
-      // METRICS_PORT,
+      // To collect metrics - run in aztec-packages `docker compose --profile metrics up` and set COLLECT_METRICS=true
+      shouldCollectMetrics(),
     );
 
     // wait a bit for peers to discover each other

yarn-project/end-to-end/src/e2e_p2p/p2p_network.ts

@@ -12,6 +12,7 @@ import { getContract } from 'viem';
 import { privateKeyToAccount } from 'viem/accounts';
 
 import {
+  PRIVATE_KEYS_START_INDEX,
   createValidatorConfig,
   generateNodePrivateKeys,
   generatePeerIdPrivateKeys,
@@ -51,7 +52,7 @@ export class P2PNetworkTest {
 
     // Set up the base account and node private keys for the initial network deployment
     this.baseAccount = privateKeyToAccount(`0x${getPrivateKeyFromIndex(0)!.toString('hex')}`);
-    this.nodePrivateKeys = generateNodePrivateKeys(1, numberOfNodes);
+    this.nodePrivateKeys = generateNodePrivateKeys(PRIVATE_KEYS_START_INDEX, numberOfNodes);
     this.peerIdPrivateKeys = generatePeerIdPrivateKeys(numberOfNodes);
 
     this.bootstrapNodeEnr = bootstrapNode.getENR().encodeTxt();
@@ -108,16 +109,11 @@ export class P2PNetworkTest {
       const txHashes: `0x${string}`[] = [];
       for (let i = 0; i < this.numberOfNodes; i++) {
         const account = privateKeyToAccount(this.nodePrivateKeys[i]!);
+        this.logger.debug(`Adding ${account.address} as validator`);
         const txHash = await rollup.write.addValidator([account.address]);
         txHashes.push(txHash);
-        this.logger.debug(`Adding ${account.address} as validator`);
       }
 
-      // Remove the setup validator
-      const initialValidatorAddress = privateKeyToAccount(`0x${getPrivateKeyFromIndex(0)!.toString('hex')}`).address;
-      const txHash = await rollup.write.removeValidator([initialValidatorAddress]);
-      txHashes.push(txHash);
-
       // Wait for all the transactions adding validators to be mined
       await Promise.all(
         txHashes.map(txHash =>
@@ -150,12 +146,52 @@ export class P2PNetworkTest {
     });
   }
 
+  async removeInitialNode() {
+    await this.snapshotManager.snapshot(
+      'remove-inital-validator',
+      async ({ deployL1ContractsValues, aztecNodeConfig }) => {
+        const rollup = getContract({
+          address: deployL1ContractsValues.l1ContractAddresses.rollupAddress.toString(),
+          abi: RollupAbi,
+          client: deployL1ContractsValues.walletClient,
+        });
+
+        // Remove the setup validator
+        const initialValidatorAddress = privateKeyToAccount(`0x${getPrivateKeyFromIndex(0)!.toString('hex')}`).address;
+        const txHash = await rollup.write.removeValidator([initialValidatorAddress]);
+
+        await deployL1ContractsValues.publicClient.waitForTransactionReceipt({
+          hash: txHash,
+        });
+
+        //@note   Now we jump ahead to the next epoch such that the validator committee is picked
+        //        INTERVAL MINING: If we are using anvil interval mining this will NOT progress the time!
+        //        Which means that the validator set will still be empty! So anyone can propose.
+        const slotsInEpoch = await rollup.read.EPOCH_DURATION();
+        const timestamp = await rollup.read.getTimestampForSlot([slotsInEpoch]);
+        const cheatCodes = new EthCheatCodes(aztecNodeConfig.l1RpcUrl);
+        try {
+          await cheatCodes.warp(Number(timestamp));
+        } catch (err) {
+          this.logger.debug('Warp failed, time already satisfied');
+        }
+
+        // Send and await a tx to make sure we mine a block for the warp to correctly progress.
+        await deployL1ContractsValues.publicClient.waitForTransactionReceipt({
+          hash: await deployL1ContractsValues.walletClient.sendTransaction({
+            to: this.baseAccount.address,
+            value: 1n,
+            account: this.baseAccount,
+          }),
+        });
+
+        await this.ctx.aztecNode.stop();
+      },
+    );
+  }
+
   async setup() {
     this.ctx = await this.snapshotManager.setup();
-
-    // TODO(md): make it such that the test can set these up
-    this.ctx.aztecNodeConfig.minTxsPerBlock = 4;
-    this.ctx.aztecNodeConfig.maxTxsPerBlock = 4;
   }
 
   async stopNodes(nodes: AztecNodeService[]) {

yarn-project/end-to-end/src/e2e_p2p/rediscovery.test.ts

@@ -26,6 +26,9 @@ describe('e2e_p2p_rediscovery', () => {
     });
     await t.applyBaseSnapshots();
     await t.setup();
+
+    // We remove the initial node such that it will no longer attempt to build blocks / be in the sequencing set
+    await t.removeInitialNode();
   });
 
   afterEach(async () => {

yarn-project/end-to-end/src/fixtures/fixtures.ts

@@ -1,5 +1,12 @@
 export const METRICS_PORT = 4318;
 
+export const shouldCollectMetrics = () => {
+  if (process.env.COLLECT_METRICS) {
+    return METRICS_PORT;
+  }
+  return undefined;
+};
+
 export const MNEMONIC = 'test test test test test test test test test test test junk';
 export const privateKey = Buffer.from('ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80', 'hex');
 export const privateKey2 = Buffer.from('59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d', 'hex');

yarn-project/end-to-end/src/fixtures/setup_p2p_test.ts

@@ -12,6 +12,10 @@ import { generatePrivateKey } from 'viem/accounts';
 import { getPrivateKeyFromIndex } from './utils.js';
 import { getEndToEndTestTelemetryClient } from './with_telemetry_utils.js';
 
+// Setup snapshots will create a node with index 0, so all of our loops here
+// need to start from 1 to avoid running validators with the same key
+export const PRIVATE_KEYS_START_INDEX = 1;
+
 export interface NodeContext {
   node: AztecNodeService;
   pxeService: PXEService;
@@ -56,7 +60,15 @@ export function createNodes(
     const port = bootNodePort + i + 1;
 
     const dataDir = dataDirectory ? `${dataDirectory}-${i}` : undefined;
-    const nodePromise = createNode(config, peerIdPrivateKeys[i], port, bootstrapNodeEnr, i, dataDir, metricsPort);
+    const nodePromise = createNode(
+      config,
+      peerIdPrivateKeys[i],
+      port,
+      bootstrapNodeEnr,
+      i + PRIVATE_KEYS_START_INDEX,
+      dataDir,
+      metricsPort,
+    );
     nodePromises.push(nodePromise);
   }
   return Promise.all(nodePromises);
@@ -95,7 +107,7 @@ export async function createValidatorConfig(
   bootstrapNodeEnr?: string,
   port?: number,
   peerIdPrivateKey?: string,
-  accountIndex: number = 0,
+  accountIndex: number = 1,
   dataDirectory?: string,
 ) {
   peerIdPrivateKey = peerIdPrivateKey ?? generatePeerIdPrivateKey();
@@ -114,8 +126,6 @@ export async function createValidatorConfig(
     tcpListenAddress: `0.0.0.0:${port}`,
     tcpAnnounceAddress: `127.0.0.1:${port}`,
     udpAnnounceAddress: `127.0.0.1:${port}`,
-    minTxsPerBlock: config.minTxsPerBlock,
-    maxTxsPerBlock: config.maxTxsPerBlock,
     p2pEnabled: true,
     blockCheckIntervalMS: 1000,
     transactionProtocol: '',

yarn-project/end-to-end/src/fixtures/snapshot_manager.ts

@@ -325,8 +325,8 @@ async function setupFromFresh(
   const publisherPrivKeyRaw = hdAccount.getHdKey().privateKey;
   const publisherPrivKey = publisherPrivKeyRaw === null ? null : Buffer.from(publisherPrivKeyRaw);
 
-  const validatorPrivKey = getPrivateKeyFromIndex(1);
-  const proverNodePrivateKey = getPrivateKeyFromIndex(2);
+  const validatorPrivKey = getPrivateKeyFromIndex(0);
+  const proverNodePrivateKey = getPrivateKeyFromIndex(0);
 
   aztecNodeConfig.publisherPrivateKey = `0x${publisherPrivKey!.toString('hex')}`;
   aztecNodeConfig.validatorPrivateKey = `0x${validatorPrivKey!.toString('hex')}`;

yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.test.ts

@@ -1,19 +1,19 @@
-import { type BlockAttestation } from '@aztec/circuit-types';
+import { type BlockAttestation, TxHash } from '@aztec/circuit-types';
+import { Secp256k1Signer } from '@aztec/foundation/crypto';
 import { Fr } from '@aztec/foundation/fields';
 import { NoopTelemetryClient } from '@aztec/telemetry-client/noop';
 
 import { type MockProxy, mock } from 'jest-mock-extended';
-import { type PrivateKeyAccount } from 'viem';
 
 import { type PoolInstrumentation } from '../instrumentation.js';
 import { InMemoryAttestationPool } from './memory_attestation_pool.js';
-import { generateAccount, mockAttestation } from './mocks.js';
+import { mockAttestation } from './mocks.js';
 
 const NUMBER_OF_SIGNERS_PER_TEST = 4;
 
 describe('MemoryAttestationPool', () => {
   let ap: InMemoryAttestationPool;
-  let signers: PrivateKeyAccount[];
+  let signers: Secp256k1Signer[];
   const telemetry = new NoopTelemetryClient();
 
   // Check that metrics are recorded correctly
@@ -23,7 +23,7 @@ describe('MemoryAttestationPool', () => {
     // Use noop telemetry client while testing.
 
     ap = new InMemoryAttestationPool(telemetry);
-    signers = Array.from({ length: NUMBER_OF_SIGNERS_PER_TEST }, generateAccount);
+    signers = Array.from({ length: NUMBER_OF_SIGNERS_PER_TEST }, () => Secp256k1Signer.random());
 
     metricsMock = mock<PoolInstrumentation<BlockAttestation>>();
     // Can i overwrite this like this??
@@ -33,7 +33,7 @@ describe('MemoryAttestationPool', () => {
   it('should add attestations to pool', async () => {
     const slotNumber = 420;
     const archive = Fr.random();
-    const attestations = await Promise.all(signers.map(signer => mockAttestation(signer, slotNumber, archive)));
+    const attestations = signers.map(signer => mockAttestation(signer, slotNumber, archive));
 
     await ap.addAttestations(attestations);
 
@@ -54,9 +54,30 @@ describe('MemoryAttestationPool', () => {
     expect(retreivedAttestationsAfterDelete.length).toBe(0);
   });
 
+  it('Should handle duplicate proposals in a slot', async () => {
+    const slotNumber = 420;
+    const archive = Fr.random();
+    const txs = [0, 1, 2, 3, 4, 5].map(() => TxHash.random());
+
+    // Use the same signer for all attestations
+    const attestations: BlockAttestation[] = [];
+    const signer = signers[0];
+    for (let i = 0; i < NUMBER_OF_SIGNERS_PER_TEST; i++) {
+      attestations.push(mockAttestation(signer, slotNumber, archive, txs));
+    }
+
+    await ap.addAttestations(attestations);
+
+    const retreivedAttestations = await ap.getAttestationsForSlot(BigInt(slotNumber), archive.toString());
+    expect(retreivedAttestations.length).toBe(1);
+    expect(retreivedAttestations[0]).toEqual(attestations[0]);
+    expect(retreivedAttestations[0].payload.txHashes).toEqual(txs);
+    expect(retreivedAttestations[0].getSender().toString()).toEqual(signer.address.toString());
+  });
+
   it('Should store attestations by differing slot', async () => {
     const slotNumbers = [1, 2, 3, 4];
-    const attestations = await Promise.all(signers.map((signer, i) => mockAttestation(signer, slotNumbers[i])));
+    const attestations = signers.map((signer, i) => mockAttestation(signer, slotNumbers[i]));
 
     await ap.addAttestations(attestations);
 
@@ -74,9 +95,7 @@ describe('MemoryAttestationPool', () => {
   it('Should store attestations by differing slot and archive', async () => {
     const slotNumbers = [1, 2, 3, 4];
     const archives = [Fr.random(), Fr.random(), Fr.random(), Fr.random()];
-    const attestations = await Promise.all(
-      signers.map((signer, i) => mockAttestation(signer, slotNumbers[i], archives[i])),
-    );
+    const attestations = signers.map((signer, i) => mockAttestation(signer, slotNumbers[i], archives[i]));
 
     await ap.addAttestations(attestations);
 
@@ -94,7 +113,7 @@ describe('MemoryAttestationPool', () => {
   it('Should delete attestations', async () => {
     const slotNumber = 420;
     const archive = Fr.random();
-    const attestations = await Promise.all(signers.map(signer => mockAttestation(signer, slotNumber, archive)));
+    const attestations = signers.map(signer => mockAttestation(signer, slotNumber, archive));
     const proposalId = attestations[0].archive.toString();
 
     await ap.addAttestations(attestations);
@@ -134,7 +153,7 @@ describe('MemoryAttestationPool', () => {
   it('Should blanket delete attestations per slot and proposal', async () => {
     const slotNumber = 420;
     const archive = Fr.random();
-    const attestations = await Promise.all(signers.map(signer => mockAttestation(signer, slotNumber, archive)));
+    const attestations = signers.map(signer => mockAttestation(signer, slotNumber, archive));
     const proposalId = attestations[0].archive.toString();
 
     await ap.addAttestations(attestations);

yarn-project/p2p/src/mem_pools/attestation_pool/mocks.ts

@@ -1,9 +1,14 @@
-import { BlockAttestation, ConsensusPayload, SignatureDomainSeperator, TxHash } from '@aztec/circuit-types';
+import {
+  BlockAttestation,
+  ConsensusPayload,
+  SignatureDomainSeperator,
+  TxHash,
+  getHashedSignaturePayloadEthSignedMessage,
+} from '@aztec/circuit-types';
 import { makeHeader } from '@aztec/circuits.js/testing';
-import { Signature } from '@aztec/foundation/eth-signature';
+import { type Secp256k1Signer } from '@aztec/foundation/crypto';
 import { Fr } from '@aztec/foundation/fields';
 
-import { type PrivateKeyAccount } from 'viem';
 import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
 
 /** Generate Account
@@ -22,22 +27,18 @@ export const generateAccount = () => {
  * @param slot The slot number the attestation is for
  * @returns A Block Attestation
  */
-export const mockAttestation = async (
-  signer: PrivateKeyAccount,
+export const mockAttestation = (
+  signer: Secp256k1Signer,
   slot: number = 0,
   archive: Fr = Fr.random(),
-): Promise<BlockAttestation> => {
+  txs: TxHash[] = [0, 1, 2, 3, 4, 5].map(() => TxHash.random()),
+): BlockAttestation => {
   // Use arbitrary numbers for all other than slot
   const header = makeHeader(1, 2, slot);
-  const txs = [0, 1, 2, 3, 4, 5].map(() => TxHash.random());
-
   const payload = new ConsensusPayload(header, archive, txs);
 
-  const message: `0x${string}` = `0x${payload
-    .getPayloadToSign(SignatureDomainSeperator.blockAttestation)
-    .toString('hex')}`;
-  const sigString = await signer.signMessage({ message });
+  const hash = getHashedSignaturePayloadEthSignedMessage(payload, SignatureDomainSeperator.blockAttestation);
+  const signature = signer.sign(hash);
 
-  const signature = Signature.from0xString(sigString);
   return new BlockAttestation(payload, signature);
 };