Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: rate limit peers in request response p2p interactions #8498

Merged
merged 53 commits into from
Sep 12, 2024
Merged
Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
b51e908
feat: cleanup publisher
LHerskind Aug 22, 2024
da2eb7a
refactor: get rid of timetraveler from l1-publisher
LHerskind Aug 28, 2024
3388966
feat: revert if timestamp in future
LHerskind Aug 28, 2024
13a60a3
feat: including txhashes explicitly in the rollup attestations
Maddiaa0 Aug 29, 2024
86026f2
temp
Maddiaa0 Aug 30, 2024
f3eac5b
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Aug 30, 2024
fc7a04a
temp
Maddiaa0 Aug 30, 2024
9eed298
temp
Maddiaa0 Sep 2, 2024
cc09455
temp
Maddiaa0 Sep 2, 2024
06f950f
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 2, 2024
4727cd9
temp: get passing with txhash payloads
Maddiaa0 Sep 3, 2024
b4c2a46
fix: make sure transactions are available in the tx pool
Maddiaa0 Sep 5, 2024
4a8d178
chore: remove logs
Maddiaa0 Sep 5, 2024
b4324fc
fmt
Maddiaa0 Sep 5, 2024
052641a
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 5, 2024
a803a94
🪿
Maddiaa0 Sep 5, 2024
164c117
chore: validator tests
Maddiaa0 Sep 6, 2024
27da59d
Add timeouts to individual reqresp connections
Maddiaa0 Sep 6, 2024
9e7d2d8
fix
Maddiaa0 Sep 6, 2024
3c8e1b9
chore: include tests for specific error messages
Maddiaa0 Sep 6, 2024
9a738e5
fmt
Maddiaa0 Sep 6, 2024
c10260c
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 6, 2024
045af5a
clean
Maddiaa0 Sep 6, 2024
73d26ec
🧹
Maddiaa0 Sep 6, 2024
d358228
chore: fix sequencing tests
Maddiaa0 Sep 7, 2024
4b31953
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 7, 2024
2f82a8f
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 7, 2024
f673593
fmt
Maddiaa0 Sep 7, 2024
a15ab17
fmt
Maddiaa0 Sep 7, 2024
2e3f80b
fmt solidity
Maddiaa0 Sep 7, 2024
ae2a05e
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 7, 2024
5734006
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 7, 2024
1bde1fe
fix: test hash
Maddiaa0 Sep 8, 2024
c775b26
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 8, 2024
7a50a2b
exp: adjust test nodes
Maddiaa0 Sep 8, 2024
72f98bd
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 8, 2024
998f38c
chore: add reqresp configuration values to p2p config
Maddiaa0 Sep 8, 2024
1c2b151
fix: use abi.encode vs encodePacked
Maddiaa0 Sep 11, 2024
e6e7f6b
fix
Maddiaa0 Sep 11, 2024
6f417fc
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 11, 2024
cde6283
fix: merge fix
Maddiaa0 Sep 11, 2024
8290c99
fmt
Maddiaa0 Sep 11, 2024
b13ca93
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 11, 2024
1452017
Merge branch 'master' into md/check-tx-requests-before-signing
Maddiaa0 Sep 11, 2024
120c9a3
Merge branch 'md/check-tx-requests-before-signing' into md/09-06-add_…
Maddiaa0 Sep 11, 2024
b189270
feat: initial rate limiter impl
Maddiaa0 Sep 11, 2024
9b90fe1
Merge branch 'master' into md/09-06-add_timeouts_to_individual_reqres…
Maddiaa0 Sep 11, 2024
b7d815f
Merge branch 'md/09-06-add_timeouts_to_individual_reqresp_connections…
Maddiaa0 Sep 11, 2024
97a6a14
feat: tests + documentation
Maddiaa0 Sep 11, 2024
0e22558
test: add to reqresp.test.ts
Maddiaa0 Sep 11, 2024
b464dcf
Merge branch 'master' into md/add-rate-limits-to-reqresp-peers
Maddiaa0 Sep 11, 2024
d0da214
fix: add todo pr number
Maddiaa0 Sep 11, 2024
ce0bd17
fix: incorrect implementation
Maddiaa0 Sep 12, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: validator tests
Maddiaa0 committed Sep 6, 2024

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
commit 164c117e396d83fa4210db2a0715555fa0395416
25 changes: 14 additions & 11 deletions yarn-project/end-to-end/src/e2e_p2p_network.test.ts
Original file line number Diff line number Diff line change
@@ -172,10 +172,10 @@ describe('e2e_p2p_network', () => {
/**
* Birds eye overview of the test
* 1. We spin up x nodes
* 2. We turn off receiving a tx via gossip from one of the nodes
* 3. We send a transaction and gossip it to other nodes
* 4. This node will receive an attestation that it does not have the data for
* 5. It will request this data over the p2p layer
* 2. We turn off receiving a tx via gossip from two of the nodes
* 3. We send a transactions and gossip it to other nodes
* 4. The disabled nodes will receive an attestation that it does not have the data for
* 5. They will request this data over the p2p layer
* 6. We receive all of the attestations that we need and we produce the block
*
* Note: we do not attempt to let this node produce a block, as it will not have received any transactions
@@ -197,14 +197,17 @@ describe('e2e_p2p_network', () => {
// wait a bit for peers to discover each other
await sleep(4000);

// Replace the p2p node implementation of one of the nodes with a spy such that it does not store transactions that are gossiped to it
// Replace the p2p node implementation of some of the nodes with a spy such that it does not store transactions that are gossiped to it
// Original implementation of `processTxFromPeer` will store received transactions in the tx pool.
const nodeToTurnOffTxGossip = 0;
jest
.spyOn((nodes[nodeToTurnOffTxGossip] as any).p2pClient.p2pService, 'processTxFromPeer')
.mockImplementation((): Promise<void> => {
return Promise.resolve();
});
// We have chosen nodes 0,2 as they do not get chosen to be the sequencer in this test ( node 1 does ).
const nodeToTurnOffTxGossip = [0, 2];
for (const nodeIndex of nodeToTurnOffTxGossip) {
jest
.spyOn((nodes[nodeIndex] as any).p2pClient.p2pService, 'processTxFromPeer')
.mockImplementation((): Promise<void> => {
return Promise.resolve();
});
}

// Only submit transactions to the first two nodes, so that we avoid our sequencer with a mocked p2p layer being picked to produce a block.
// If the shuffling algorithm changes, then this will need to be updated.
1 change: 1 addition & 0 deletions yarn-project/p2p/src/errors/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './reqresp.error.js';
14 changes: 14 additions & 0 deletions yarn-project/p2p/src/errors/reqresp.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { type ReqRespSubProtocol, TX_REQ_PROTOCOL } from '../service/reqresp/interface.js';

export class ReqRespError extends Error {
constructor(protocol: ReqRespSubProtocol, message: string) {
super(message);
}
}

// TODO(md): think about what these errors should ideally be
export class TxHandlerReqRespError extends ReqRespError {
constructor() {
super(TX_REQ_PROTOCOL, 'Could not perform tx handler request response');
}
}
6 changes: 3 additions & 3 deletions yarn-project/p2p/src/service/reqresp/interface.ts
Original file line number Diff line number Diff line change
@@ -3,9 +3,9 @@ import { Tx, TxHash } from '@aztec/circuit-types';
/*
* Request Response Sub Protocols
*/
export const PING_PROTOCOL = '/aztec/ping/0.1.0';
export const STATUS_PROTOCOL = '/aztec/status/0.1.0';
export const TX_REQ_PROTOCOL = '/aztec/tx_req/0.1.0';
export const PING_PROTOCOL = '/aztec/req/ping/0.1.0';
export const STATUS_PROTOCOL = '/aztec/req/status/0.1.0';
export const TX_REQ_PROTOCOL = '/aztec/req/tx/0.1.0';

// Sum type for sub protocols
export type ReqRespSubProtocol = typeof PING_PROTOCOL | typeof STATUS_PROTOCOL | typeof TX_REQ_PROTOCOL;
1 change: 1 addition & 0 deletions yarn-project/validator-client/package.json
Original file line number Diff line number Diff line change
@@ -72,6 +72,7 @@
"@types/jest": "^29.5.0",
"@types/node": "^18.7.23",
"jest": "^29.5.0",
"jest-mock-extended": "^3.0.7",
"ts-node": "^10.9.1",
"typescript": "^5.0.4"
},
1 change: 1 addition & 0 deletions yarn-project/validator-client/src/errors/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './validator.error.js';
19 changes: 19 additions & 0 deletions yarn-project/validator-client/src/errors/validator.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { type TxHash } from '@aztec/circuit-types/tx_hash';

export class ValidatorError extends Error {
constructor(message: string) {
super(message);
}
}

export class AttestationTimeoutError extends ValidatorError {
constructor(numberOfRequiredAttestations: number, slot: bigint) {
super(`Timeout waiting for ${numberOfRequiredAttestations} attestations for slot, ${slot}`);
}
}

export class TransactionsNotAvailableError extends ValidatorError {
constructor(txHashes: TxHash[]) {
super(`Transactions not available: ${txHashes.join(', ')}`);
}
}
71 changes: 71 additions & 0 deletions yarn-project/validator-client/src/validator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/**
* Validation logic unit tests
*/
import { TxHash } from '@aztec/circuit-types';
import { makeHeader } from '@aztec/circuits.js/testing';
import { EthAddress } from '@aztec/foundation/eth-address';
import { Fr } from '@aztec/foundation/fields';
import { type P2P } from '@aztec/p2p';

import { describe, expect, it } from '@jest/globals';
import { type MockProxy, mock } from 'jest-mock-extended';
import { type PrivateKeyAccount, generatePrivateKey, privateKeyToAccount } from 'viem/accounts';

import { makeBlockProposal } from '../../circuit-types/src/p2p/mocks.js';
import { AttestationTimeoutError, TransactionsNotAvailableError } from './errors/validator.error.js';
import { ValidatorClient } from './validator.js';

describe('ValidationService', () => {
let validatorClient: ValidatorClient;
let p2pClient: MockProxy<P2P>;
let validatorAccount: PrivateKeyAccount;

beforeEach(() => {
p2pClient = mock<P2P>();
p2pClient.getAttestationsForSlot.mockImplementation(() => Promise.resolve([]));

const validatorPrivateKey = generatePrivateKey();
validatorAccount = privateKeyToAccount(validatorPrivateKey);

const config = {
validatorPrivateKey: validatorPrivateKey,
attestationPoolingIntervalMs: 1000,
attestationWaitTimeoutMs: 1000,
disableValidator: false,
};
validatorClient = ValidatorClient.new(config, p2pClient);
});

it('Should create a valid block proposal', async () => {
const header = makeHeader();
const archive = Fr.random();
const txs = [1, 2, 3, 4, 5].map(() => TxHash.random());

const blockProposal = await validatorClient.createBlockProposal(header, archive, txs);

expect(blockProposal).toBeDefined();

const validatorAddress = EthAddress.fromString(validatorAccount.address);
expect(await blockProposal.getSender()).toEqual(validatorAddress);
});

// TODO: add a test on the sequencer that it can recover in case that this timeout is hit
it('Should a timeout if we do not collect enough attestations in time', async () => {
const proposal = await makeBlockProposal();

await expect(validatorClient.collectAttestations(proposal, 2)).rejects.toThrow(AttestationTimeoutError);
});

it('Should throw an error if the transactions are not available', async () => {
const proposal = await makeBlockProposal();

// mock the p2pClient.getTxStatus to return undefined for all transactions
p2pClient.getTxStatus.mockImplementation(() => undefined);
// Mock the p2pClient.requestTxs to return undefined for all transactions
p2pClient.requestTxs.mockImplementation(() => Promise.resolve([undefined]));

await expect(validatorClient.ensureTransactionsAreAvailable(proposal)).rejects.toThrow(
TransactionsNotAvailableError,
);
});
});
75 changes: 28 additions & 47 deletions yarn-project/validator-client/src/validator.ts
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ import { type P2P } from '@aztec/p2p';

import { type ValidatorClientConfig } from './config.js';
import { ValidationService } from './duties/validation_service.js';
import { AttestationTimeoutError, TransactionsNotAvailableError } from './errors/validator.error.js';
import { type ValidatorKeyStore } from './key_store/interface.js';
import { LocalKeyStore } from './key_store/local_key_store.js';

@@ -18,7 +19,6 @@ export interface Validator {
createBlockProposal(header: Header, archive: Fr, txs: TxHash[]): Promise<BlockProposal>;
attestToProposal(proposal: BlockProposal): void;

// TODO(md): possible abstraction leak
broadcastBlockProposal(proposal: BlockProposal): void;
collectAttestations(proposal: BlockProposal, numberOfRequiredAttestations: number): Promise<BlockAttestation[]>;
}
@@ -88,33 +88,20 @@ export class ValidatorClient implements Validator {
*/
async ensureTransactionsAreAvailable(proposal: BlockProposal) {
const txHashes: TxHash[] = proposal.txs;
const transactionStatuses = await Promise.all(txHashes.map(txHash => this.p2pClient.getTxStatus(txHash)));

const transactionStatuses = txHashes.map(txHash => this.p2pClient.getTxStatus(txHash));
const haveAllTxs = transactionStatuses.every(tx => tx === 'pending' || tx === 'mined');
const missingTxs = txHashes.filter((_, index) => !['pending', 'mined'].includes(transactionStatuses[index] ?? ''));

// Only in an if statement here for logging purposes
if (!haveAllTxs) {
const missingTxs: TxHash[] = txHashes
.map((_, index) => {
if (!transactionStatuses[index]) {
return txHashes[index];
}
return undefined;
})
.filter(tx => tx !== undefined) as TxHash[];
if (missingTxs.length === 0) {
return; // All transactions are available
}

this.log.verbose(
`Missing ${missingTxs.length} attestations transactions in the tx pool, requesting from the network`,
);
this.log.verbose(`Missing ${missingTxs.length} transactions in the tx pool, requesting from the network`);

if (missingTxs) {
// If transactions are requested successfully, they will be written into the tx pool
const requestedTxs = await this.p2pClient.requestTxs(missingTxs);
const successfullyRetrievedMissingTxs = requestedTxs.every(tx => tx !== undefined);
if (!successfullyRetrievedMissingTxs) {
throw new Error('Failed to retrieve missing transactions');
}
}
const requestedTxs = await this.p2pClient.requestTxs(missingTxs);
if (requestedTxs.some(tx => tx === undefined)) {
this.log.error(`Failed to request transactions from the network: ${missingTxs.join(', ')}`);
throw new TransactionsNotAvailableError(missingTxs);
}
}

@@ -131,38 +118,32 @@ export class ValidatorClient implements Validator {
proposal: BlockProposal,
numberOfRequiredAttestations: number,
): Promise<BlockAttestation[]> {
// Wait and poll the p2pClients attestation pool for this block
// until we have enough attestations

const startTime = Date.now();

// Wait and poll the p2pClient's attestation pool for this block until we have enough attestations
const slot = proposal.header.globalVariables.slotNumber.toBigInt();

this.log.info(`Waiting for ${numberOfRequiredAttestations} attestations for slot: ${slot}`);

const myAttestation = await this.attestToProposal(proposal);
const myAttestation = await this.validationService.attestToProposal(proposal);

const startTime = Date.now();

let attestations: BlockAttestation[] = [];
while (attestations.length < numberOfRequiredAttestations) {
attestations = [myAttestation, ...(await this.p2pClient.getAttestationsForSlot(slot))];
while (true) {
const attestations = [myAttestation, ...(await this.p2pClient.getAttestationsForSlot(slot))];

// Rememebr we can subtract 1 from this if we self sign
if (attestations.length < numberOfRequiredAttestations) {
this.log.verbose(
`SEAN: collected ${attestations.length} attestations so far ${numberOfRequiredAttestations} required`,
);
this.log.verbose(`Waiting ${this.attestationPoolingIntervalMs}ms for more attestations...`);
await sleep(this.attestationPoolingIntervalMs);
if (attestations.length >= numberOfRequiredAttestations) {
this.log.info(`Collected all ${numberOfRequiredAttestations} attestations for slot, ${slot}`);
return attestations;
}

// FIX(md): kinna sad looking code
if (Date.now() - startTime > this.attestationWaitTimeoutMs) {
const elapsedTime = Date.now() - startTime;
if (elapsedTime > this.attestationWaitTimeoutMs) {
this.log.error(`Timeout waiting for ${numberOfRequiredAttestations} attestations for slot, ${slot}`);
throw new Error(`Timeout waiting for ${numberOfRequiredAttestations} attestations for slot, ${slot}`);
throw new AttestationTimeoutError(numberOfRequiredAttestations, slot);
}
}
this.log.info(`Collected all attestations for slot, ${slot}`);

return attestations;
this.log.verbose(
`Collected ${attestations.length} attestations so far, waiting ${this.attestationPoolingIntervalMs}ms for more...`,
);
await sleep(this.attestationPoolingIntervalMs);
}
}
}
3 changes: 2 additions & 1 deletion yarn-project/yarn.lock
Original file line number Diff line number Diff line change
@@ -1219,6 +1219,7 @@ __metadata:
"@types/jest": ^29.5.0
"@types/node": ^18.7.23
jest: ^29.5.0
jest-mock-extended: ^3.0.7
koa: ^2.14.2
koa-router: ^12.0.0
ts-node: ^10.9.1
@@ -10802,7 +10803,7 @@ __metadata:
languageName: node
linkType: hard

"jest-mock-extended@npm:^3.0.3, jest-mock-extended@npm:^3.0.4, jest-mock-extended@npm:^3.0.5":
"jest-mock-extended@npm:^3.0.3, jest-mock-extended@npm:^3.0.4, jest-mock-extended@npm:^3.0.5, jest-mock-extended@npm:^3.0.7":
version: 3.0.7
resolution: "jest-mock-extended@npm:3.0.7"
dependencies: