From 8310c3c84b109fb8a6f43c69dae4d531de11bb6a Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Mon, 7 Oct 2024 13:09:05 +0000 Subject: [PATCH 1/8] temp --- .../src/p2p/block_attestation.ts | 4 ++ .../circuit-types/src/p2p/block_proposal.ts | 4 ++ .../src/p2p/consensus_payload.ts | 8 +++ .../circuit-types/src/p2p/gossipable.ts | 7 +++ .../prover_coordination/epoch_proof_quote.ts | 8 +++ .../epoch_proof_quote_payload.ts | 5 ++ .../src/eth-signature/eth_signature.ts | 5 ++ .../memory_attestation_pool.test.ts | 19 ++++++- .../memory_attestation_pool.ts | 43 ++++++++++++--- yarn-project/p2p/src/client/p2p_metrics.ts | 6 +++ .../p2p/src/service/libp2p_service.ts | 11 ++++ .../p2p/src/tx_pool/aztec_kv_tx_pool.ts | 18 +++---- .../p2p/src/tx_pool/instrumentation.ts | 54 +++++++++++++------ .../p2p/src/tx_pool/memory_tx_pool.ts | 18 +++---- 14 files changed, 168 insertions(+), 42 deletions(-) create mode 100644 yarn-project/p2p/src/client/p2p_metrics.ts diff --git a/yarn-project/circuit-types/src/p2p/block_attestation.ts b/yarn-project/circuit-types/src/p2p/block_attestation.ts index c808417baf5..3e7cdfcc13f 100644 --- a/yarn-project/circuit-types/src/p2p/block_attestation.ts +++ b/yarn-project/circuit-types/src/p2p/block_attestation.ts @@ -72,4 +72,8 @@ export class BlockAttestation extends Gossipable { static empty(): BlockAttestation { return new BlockAttestation(ConsensusPayload.empty(), Signature.empty()); } + + getSize(): number { + return (this.payload.getSize() + this.signature.getSize()); + } } diff --git a/yarn-project/circuit-types/src/p2p/block_proposal.ts b/yarn-project/circuit-types/src/p2p/block_proposal.ts index 80cbb80c596..badbf2cfbcd 100644 --- a/yarn-project/circuit-types/src/p2p/block_proposal.ts +++ b/yarn-project/circuit-types/src/p2p/block_proposal.ts @@ -75,4 +75,8 @@ export class BlockProposal extends Gossipable { const reader = BufferReader.asReader(buf); return new BlockProposal(reader.readObject(ConsensusPayload), reader.readObject(Signature)); } + + getSize(): number { + return (this.payload.getSize() + this.signature.getSize()); + } } diff --git a/yarn-project/circuit-types/src/p2p/consensus_payload.ts b/yarn-project/circuit-types/src/p2p/consensus_payload.ts index f9cb6101b06..bf18e1018fb 100644 --- a/yarn-project/circuit-types/src/p2p/consensus_payload.ts +++ b/yarn-project/circuit-types/src/p2p/consensus_payload.ts @@ -50,4 +50,12 @@ export class ConsensusPayload implements Signable { static empty(): ConsensusPayload { return new ConsensusPayload(Header.empty(), Fr.ZERO, []); } + + /** + * Get the size of the consensus payload in bytes. + * @returns The size of the consensus payload. + */ + getSize(): number { + return (this.header.getSize() + this.archive.size + this.txHashes.length * TxHash.SIZE); + } } diff --git a/yarn-project/circuit-types/src/p2p/gossipable.ts b/yarn-project/circuit-types/src/p2p/gossipable.ts index 4b9c407cb03..4de52bce26f 100644 --- a/yarn-project/circuit-types/src/p2p/gossipable.ts +++ b/yarn-project/circuit-types/src/p2p/gossipable.ts @@ -23,4 +23,11 @@ export abstract class Gossipable { * - Serialization method */ abstract toBuffer(): Buffer; + + /** + * Get the size of the gossipable object. + * + * This is used for metrics recording. + */ + abstract getSize(): number; } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts index a26de238aa6..b1e508a24b9 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts @@ -69,4 +69,12 @@ export class EpochProofQuote extends Gossipable { signature: this.signature.toViemSignature(), }; } + + /** + * Get the size of the epoch proof quote in bytes. + * @returns The size of the epoch proof quote in bytes. + */ + getSize(): number { + return (this.payload.getSize() + this.signature.getSize()); + } } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts index 77e75598b73..acd51c2d842 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts @@ -84,6 +84,11 @@ export class EpochProofQuotePayload { }; } + getSize(): number { + // 32 bytes for epochToProve, 32 bytes for validUntilSlot, 32 bytes for bondAmount, 20 bytes for prover, 4 bytes for basisPointFee + return (32 + 32 + 32 + EthAddress.SIZE_IN_BYTES + 4); + } + [inspect.custom](): string { return `EpochProofQuotePayload { epochToProve: ${this.epochToProve}, validUntilSlot: ${this.validUntilSlot}, bondAmount: ${this.bondAmount}, prover: ${this.prover}, basisPointFee: ${this.basisPointFee} }`; } diff --git a/yarn-project/foundation/src/eth-signature/eth_signature.ts b/yarn-project/foundation/src/eth-signature/eth_signature.ts index b985a8ec74c..83a9f780169 100644 --- a/yarn-project/foundation/src/eth-signature/eth_signature.ts +++ b/yarn-project/foundation/src/eth-signature/eth_signature.ts @@ -76,6 +76,11 @@ export class Signature { return serializeToBuffer([this.r, this.s, this.v]); } + getSize(): number { + // 32 bytes for r, 32 bytes for s, 4 bytes for v + return Buffer32.SIZE + Buffer32.SIZE + 4; + } + to0xString(): `0x${string}` { return `0x${this.r.toString()}${this.s.toString()}${this.v.toString(16)}`; } diff --git a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts index a798dedff59..3642a60b069 100644 --- a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts +++ b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts @@ -4,16 +4,31 @@ import { type PrivateKeyAccount } from 'viem'; import { InMemoryAttestationPool } from './memory_attestation_pool.js'; import { generateAccount, mockAttestation } from './mocks.js'; +import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; +import { BlockAttestation } from '@aztec/circuit-types'; +import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { mock, MockProxy } from 'jest-mock-extended'; const NUMBER_OF_SIGNERS_PER_TEST = 4; describe('MemoryAttestationPool', () => { let ap: InMemoryAttestationPool; let signers: PrivateKeyAccount[]; + const telemetry = new NoopTelemetryClient(); + + // Check that metrics are recorded correctly + let metricsMock: MockProxy>; beforeEach(() => { - ap = new InMemoryAttestationPool(); + // Use noop telemetry client while testing. + + ap = new InMemoryAttestationPool(telemetry); signers = Array.from({ length: NUMBER_OF_SIGNERS_PER_TEST }, generateAccount); + + + metricsMock = mock>(); + // Can i overwrite this like this?? + (ap as any).metrics = metricsMock; }); it('should add attestations to pool', async () => { @@ -25,6 +40,8 @@ describe('MemoryAttestationPool', () => { await ap.addAttestations(attestations); + // Check metrics have been updated. + const retreivedAttestations = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(retreivedAttestations.length).toBe(NUMBER_OF_SIGNERS_PER_TEST); diff --git a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts index 34337109afc..f26700508e5 100644 --- a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts +++ b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts @@ -2,12 +2,17 @@ import { type BlockAttestation } from '@aztec/circuit-types'; import { createDebugLogger } from '@aztec/foundation/log'; import { type AttestationPool } from './attestation_pool.js'; +import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { TelemetryClient } from '@aztec/telemetry-client'; export class InMemoryAttestationPool implements AttestationPool { + private metrics: PoolInstrumentation; + private attestations: Map>>; - constructor(private log = createDebugLogger('aztec:attestation_pool')) { + constructor(telemetry: TelemetryClient, private log = createDebugLogger('aztec:attestation_pool')) { this.attestations = new Map(); + this.metrics = new PoolInstrumentation(telemetry, 'InMemoryAttestationPool'); } public getAttestationsForSlot(slot: bigint, proposalId: string): Promise { @@ -35,21 +40,46 @@ export class InMemoryAttestationPool implements AttestationPool { this.log.verbose(`Added attestation for slot ${slotNumber} from ${address}`); } + + // TODO: set these to pending or something ???? + this.metrics.recordAddedObjects(attestations.length); return Promise.resolve(); } + #getNumberOfAttestationsInSlot(slot: bigint): number { + let total = 0; + const slotAttestationMap = getSlotOrDefault(this.attestations, slot); + + if (slotAttestationMap) { + for (const proposalAttestationMap of slotAttestationMap.values() ?? []) { + total += proposalAttestationMap.size; + } + } + return total; + } + public deleteAttestationsForSlot(slot: bigint): Promise { - // TODO(md): check if this will free the memory of the inner hash map + // We count the number of attestations we are removing + const numberOfAttestations = this.#getNumberOfAttestationsInSlot(slot); + this.attestations.delete(slot); - this.log.verbose(`Removed attestation for slot ${slot}`); + this.log.verbose(`Removed ${numberOfAttestations} attestations for slot ${slot}`); + + this.metrics.recordRemovedObjects(numberOfAttestations); return Promise.resolve(); } public deleteAttestationsForSlotAndProposal(slot: bigint, proposalId: string): Promise { - const slotAttestationMap = this.attestations.get(slot); + const slotAttestationMap = getSlotOrDefault(this.attestations, slot); if (slotAttestationMap) { - slotAttestationMap.delete(proposalId); - this.log.verbose(`Removed attestation for slot ${slot}`); + if (slotAttestationMap.has(proposalId)) { + const numberOfAttestations = slotAttestationMap.get(proposalId)?.size ?? 0; + + slotAttestationMap.delete(proposalId); + + this.log.verbose(`Removed ${numberOfAttestations} attestations for slot ${slot} and proposal ${proposalId}`); + this.metrics.recordRemovedObjects(numberOfAttestations); + } } return Promise.resolve(); } @@ -68,6 +98,7 @@ export class InMemoryAttestationPool implements AttestationPool { } } } + this.metrics.recordRemovedObjects(attestations.length); return Promise.resolve(); } } diff --git a/yarn-project/p2p/src/client/p2p_metrics.ts b/yarn-project/p2p/src/client/p2p_metrics.ts new file mode 100644 index 00000000000..5060aa57e81 --- /dev/null +++ b/yarn-project/p2p/src/client/p2p_metrics.ts @@ -0,0 +1,6 @@ +// TODO + +// - Create an interface that can transform our existing Meter interface into something that the libp2p gossipsub metrics +// can absorb! + +// Then we should be able to hook into their existing metrics \ No newline at end of file diff --git a/yarn-project/p2p/src/service/libp2p_service.ts b/yarn-project/p2p/src/service/libp2p_service.ts index 755439b5f2e..275a1e03e60 100644 --- a/yarn-project/p2p/src/service/libp2p_service.ts +++ b/yarn-project/p2p/src/service/libp2p_service.ts @@ -260,6 +260,17 @@ export class LibP2PService implements P2PService { invalidMessageDeliveriesWeight: -20, invalidMessageDeliveriesDecay: 0.5, }), + // TODO(md): Think about adding these back in + // [BlockProposal.p2pTopic]: createTopicScoreParams({ + // topicWeight: 1, + // invalidMessageDeliveriesWeight: -20, + // invalidMessageDeliveriesDecay: 0.5, + // }), + // [BlockAttestation.p2pTopic]: createTopicScoreParams({ + // topicWeight: 1, + // invalidMessageDeliveriesWeight: -20, + // invalidMessageDeliveriesDecay: 0.5, + // }), }, }), }) as (components: GossipSubComponents) => GossipSub, diff --git a/yarn-project/p2p/src/tx_pool/aztec_kv_tx_pool.ts b/yarn-project/p2p/src/tx_pool/aztec_kv_tx_pool.ts index 4037e4cbb15..ffac3d6924b 100644 --- a/yarn-project/p2p/src/tx_pool/aztec_kv_tx_pool.ts +++ b/yarn-project/p2p/src/tx_pool/aztec_kv_tx_pool.ts @@ -4,7 +4,7 @@ import { type Logger, createDebugLogger } from '@aztec/foundation/log'; import { type AztecKVStore, type AztecMap, type AztecSet } from '@aztec/kv-store'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { TxPoolInstrumentation } from './instrumentation.js'; +import { PoolInstrumentation } from './instrumentation.js'; import { type TxPool } from './tx_pool.js'; /** @@ -23,7 +23,7 @@ export class AztecKVTxPool implements TxPool { #log: Logger; - #metrics: TxPoolInstrumentation; + #metrics: PoolInstrumentation; /** * Class constructor for in-memory TxPool. Initiates our transaction pool as a JS Map. @@ -37,7 +37,7 @@ export class AztecKVTxPool implements TxPool { this.#store = store; this.#log = log; - this.#metrics = new TxPoolInstrumentation(telemetry, 'AztecKVTxPool'); + this.#metrics = new PoolInstrumentation(telemetry, 'AztecKVTxPool'); } public markAsMined(txHashes: TxHash[]): Promise { @@ -51,8 +51,8 @@ export class AztecKVTxPool implements TxPool { void this.#pendingTxs.delete(key); } } - this.#metrics.recordRemovedTxs('pending', deleted); - this.#metrics.recordAddedTxs('mined', txHashes.length); + this.#metrics.recordRemovedObjectsWithStatus('pending', deleted); + this.#metrics.recordAddedObjectsWithStatus('mined', txHashes.length); }); } @@ -107,11 +107,11 @@ export class AztecKVTxPool implements TxPool { pendingCount++; // REFACTOR: Use an lmdb conditional write to avoid race conditions with this write tx void this.#pendingTxs.add(key); - this.#metrics.recordTxSize(tx); + this.#metrics.recordSize(tx); } } - this.#metrics.recordAddedTxs('pending', pendingCount); + this.#metrics.recordAddedObjectsWithStatus('pending', pendingCount); }); } @@ -138,8 +138,8 @@ export class AztecKVTxPool implements TxPool { } } - this.#metrics.recordRemovedTxs('pending', pendingDeleted); - this.#metrics.recordRemovedTxs('mined', minedDeleted); + this.#metrics.recordRemovedObjectsWithStatus('pending', pendingDeleted); + this.#metrics.recordRemovedObjectsWithStatus('mined', minedDeleted); }); } diff --git a/yarn-project/p2p/src/tx_pool/instrumentation.ts b/yarn-project/p2p/src/tx_pool/instrumentation.ts index 941a2f46168..96c87efefc4 100644 --- a/yarn-project/p2p/src/tx_pool/instrumentation.ts +++ b/yarn-project/p2p/src/tx_pool/instrumentation.ts @@ -1,24 +1,24 @@ -import { type Tx } from '@aztec/circuit-types'; +import { Gossipable } from '@aztec/circuit-types'; import { Attributes, type Histogram, Metrics, type TelemetryClient, type UpDownCounter } from '@aztec/telemetry-client'; export type TxStatus = 'pending' | 'mined'; /** - * Instrumentation class for the TxPool. + * Instrumentation class for the Pools (TxPool, AttestationPool, etc). */ -export class TxPoolInstrumentation { +export class PoolInstrumentation { /** The number of txs in the mempool */ - private txInMempool: UpDownCounter; + private objectsInMempool: UpDownCounter; /** Tracks tx size */ - private txSize: Histogram; + private objectSize: Histogram; constructor(telemetry: TelemetryClient, name: string) { const meter = telemetry.getMeter(name); - this.txInMempool = meter.createUpDownCounter(Metrics.MEMPOOL_TX_COUNT, { + this.objectsInMempool = meter.createUpDownCounter(Metrics.MEMPOOL_TX_COUNT, { description: 'The current number of transactions in the mempool', }); - this.txSize = meter.createHistogram(Metrics.MEMPOOL_TX_SIZE, { + this.objectSize = meter.createHistogram(Metrics.MEMPOOL_TX_SIZE, { unit: 'By', description: 'The size of transactions in the mempool', advice: { @@ -35,39 +35,59 @@ export class TxPoolInstrumentation { }); } - public recordTxSize(tx: Tx) { - this.txSize.record(tx.getSize()); + public recordSize(poolObject: PoolObject) { + this.objectSize.record(poolObject.getSize()); } /** - * Updates the metrics with the new transactions. - * @param txs - The transactions to record + * Updates the metrics with the new objects. + * @param txs - The objects to record */ - public recordAddedTxs(status: string, count = 1) { + public recordAddedObjectsWithStatus(status: string, count = 1) { if (count < 0) { throw new Error('Count must be positive'); } if (count === 0) { return; } - this.txInMempool.add(count, { + this.objectsInMempool.add(count, { [Attributes.STATUS]: status, }); } + public recordAddedObjects(count = 1) { + if (count < 0) { + throw new Error('Count must be positive'); + } + if (count === 0) { + return; + } + this.objectsInMempool.add(count); + } + /** - * Updates the metrics by removing transactions from the mempool. - * @param count - The number of transactions to remove from the mempool + * Updates the metrics by removing objects from the mempool. + * @param count - The number of objects to remove from the mempool */ - public recordRemovedTxs(status: string, count = 1) { + public recordRemovedObjectsWithStatus(status: string, count = 1) { if (count < 0) { throw new Error('Count must be positive'); } if (count === 0) { return; } - this.txInMempool.add(-1 * count, { + this.objectsInMempool.add(-1 * count, { [Attributes.STATUS]: status, }); } + + public recordRemovedObjects(count = 1) { + if (count < 0) { + throw new Error('Count must be positive'); + } + if (count === 0) { + return; + } + this.objectsInMempool.add(-1 * count); + } } diff --git a/yarn-project/p2p/src/tx_pool/memory_tx_pool.ts b/yarn-project/p2p/src/tx_pool/memory_tx_pool.ts index 6fcf2c263c6..6519a34175f 100644 --- a/yarn-project/p2p/src/tx_pool/memory_tx_pool.ts +++ b/yarn-project/p2p/src/tx_pool/memory_tx_pool.ts @@ -3,7 +3,7 @@ import { type TxAddedToPoolStats } from '@aztec/circuit-types/stats'; import { createDebugLogger } from '@aztec/foundation/log'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { TxPoolInstrumentation } from './instrumentation.js'; +import { PoolInstrumentation } from './instrumentation.js'; import { type TxPool } from './tx_pool.js'; /** @@ -17,7 +17,7 @@ export class InMemoryTxPool implements TxPool { private minedTxs: Set; private pendingTxs: Set; - private metrics: TxPoolInstrumentation; + private metrics: PoolInstrumentation; /** * Class constructor for in-memory TxPool. Initiates our transaction pool as a JS Map. @@ -27,7 +27,7 @@ export class InMemoryTxPool implements TxPool { this.txs = new Map(); this.minedTxs = new Set(); this.pendingTxs = new Set(); - this.metrics = new TxPoolInstrumentation(telemetry, 'InMemoryTxPool'); + this.metrics = new PoolInstrumentation(telemetry, 'InMemoryTxPool'); } public markAsMined(txHashes: TxHash[]): Promise { @@ -36,8 +36,8 @@ export class InMemoryTxPool implements TxPool { this.minedTxs.add(key); this.pendingTxs.delete(key); } - this.metrics.recordRemovedTxs('pending', txHashes.length); - this.metrics.recordAddedTxs('mined', txHashes.length); + this.metrics.recordRemovedObjectsWithStatus('pending', txHashes.length); + this.metrics.recordAddedObjectsWithStatus('mined', txHashes.length); return Promise.resolve(); } @@ -88,12 +88,12 @@ export class InMemoryTxPool implements TxPool { this.txs.set(key, tx); if (!this.minedTxs.has(key)) { pending++; - this.metrics.recordTxSize(tx); + this.metrics.recordSize(tx); this.pendingTxs.add(key); } } - this.metrics.recordAddedTxs('pending', pending); + this.metrics.recordAddedObjectsWithStatus('pending', pending); return Promise.resolve(); } @@ -113,8 +113,8 @@ export class InMemoryTxPool implements TxPool { deletedMined += this.minedTxs.delete(key) ? 1 : 0; } - this.metrics.recordRemovedTxs('pending', deletedPending); - this.metrics.recordRemovedTxs('mined', deletedMined); + this.metrics.recordRemovedObjectsWithStatus('pending', deletedPending); + this.metrics.recordRemovedObjectsWithStatus('mined', deletedMined); return Promise.resolve(); } From bc2f79324ddf276452ce85933b6754dde1464fdf Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Mon, 7 Oct 2024 18:22:57 +0000 Subject: [PATCH 2/8] feat: add metrics collecting tests to pool tests --- .../memory_attestation_pool.test.ts | 11 +++++++++++ yarn-project/p2p/src/client/index.ts | 4 ++-- .../memory_epoch_proof_quote_pool.test.ts | 18 ++++++++++++++++-- .../memory_epoch_proof_quote_pool.ts | 17 ++++++++++++++++- 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts index 3642a60b069..c65cf7681a6 100644 --- a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts +++ b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts @@ -41,6 +41,7 @@ describe('MemoryAttestationPool', () => { await ap.addAttestations(attestations); // Check metrics have been updated. + expect(metricsMock.recordAddedObjects).toHaveBeenCalledWith(attestations.length); const retreivedAttestations = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); @@ -50,6 +51,8 @@ describe('MemoryAttestationPool', () => { // Delete by slot await ap.deleteAttestationsForSlot(BigInt(slotNumber)); + expect(metricsMock.recordRemovedObjects).toHaveBeenCalledWith(attestations.length); + const retreivedAttestationsAfterDelete = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(retreivedAttestationsAfterDelete.length).toBe(0); }); @@ -99,12 +102,16 @@ describe('MemoryAttestationPool', () => { await ap.addAttestations(attestations); + expect(metricsMock.recordAddedObjects).toHaveBeenCalledWith(attestations.length); + const retreivedAttestations = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(retreivedAttestations.length).toBe(NUMBER_OF_SIGNERS_PER_TEST); expect(retreivedAttestations).toEqual(attestations); await ap.deleteAttestations(attestations); + expect(metricsMock.recordRemovedObjects).toHaveBeenCalledWith(attestations.length); + const gottenAfterDelete = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(gottenAfterDelete.length).toBe(0); }); @@ -135,12 +142,16 @@ describe('MemoryAttestationPool', () => { await ap.addAttestations(attestations); + expect(metricsMock.recordAddedObjects).toHaveBeenCalledWith(attestations.length); + const retreivedAttestations = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(retreivedAttestations.length).toBe(NUMBER_OF_SIGNERS_PER_TEST); expect(retreivedAttestations).toEqual(attestations); await ap.deleteAttestationsForSlotAndProposal(BigInt(slotNumber), proposalId); + expect(metricsMock.recordRemovedObjects).toHaveBeenCalledWith(attestations.length); + const retreivedAttestationsAfterDelete = await ap.getAttestationsForSlot(BigInt(slotNumber), proposalId); expect(retreivedAttestationsAfterDelete.length).toBe(0); }); diff --git a/yarn-project/p2p/src/client/index.ts b/yarn-project/p2p/src/client/index.ts index aaafccba9f1..925c2ff69f9 100644 --- a/yarn-project/p2p/src/client/index.ts +++ b/yarn-project/p2p/src/client/index.ts @@ -35,8 +35,8 @@ export const createP2PClient = async ( let config = { ..._config }; const store = deps.store ?? (await createStore('p2p', config, createDebugLogger('aztec:p2p:lmdb'))); const txPool = deps.txPool ?? new AztecKVTxPool(store, telemetry); - const attestationsPool = deps.attestationsPool ?? new InMemoryAttestationPool(); - const epochProofQuotePool = deps.epochProofQuotePool ?? new MemoryEpochProofQuotePool(); + const attestationsPool = deps.attestationsPool ?? new InMemoryAttestationPool(telemetry); + const epochProofQuotePool = deps.epochProofQuotePool ?? new MemoryEpochProofQuotePool(telemetry); let p2pService; diff --git a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts index 16ea4aeec52..e593336a31c 100644 --- a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts +++ b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts @@ -1,12 +1,21 @@ -import { mockEpochProofQuote } from '@aztec/circuit-types'; +import { EpochProofQuote, mockEpochProofQuote } from '@aztec/circuit-types'; import { MemoryEpochProofQuotePool } from './memory_epoch_proof_quote_pool.js'; +import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { mock, MockProxy } from 'jest-mock-extended'; +import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; describe('MemoryEpochProofQuotePool', () => { let pool: MemoryEpochProofQuotePool; + let metricsMock: MockProxy>; + beforeEach(() => { - pool = new MemoryEpochProofQuotePool(); + const telemetry = new NoopTelemetryClient(); + pool = new MemoryEpochProofQuotePool(telemetry); + + metricsMock = mock>(); + (pool as any).metrics = metricsMock; }); it('should add/get quotes to/from pool', () => { @@ -14,6 +23,8 @@ describe('MemoryEpochProofQuotePool', () => { pool.addQuote(quote); + expect(metricsMock.recordAddedObjects).toHaveBeenCalledWith(1); + const quotes = pool.getQuotes(quote.payload.epochToProve); expect(quotes).toHaveLength(1); @@ -36,6 +47,7 @@ describe('MemoryEpochProofQuotePool', () => { const quotes3 = pool.getQuotes(3n); const quotesForEpoch3 = proofQuotes.filter(x => x.payload.epochToProve === 3n); + const quotesForEpoch2 = proofQuotes.filter(x => x.payload.epochToProve === 2n); expect(quotes3).toHaveLength(quotesForEpoch3.length); expect(quotes3).toEqual(quotesForEpoch3); @@ -43,6 +55,8 @@ describe('MemoryEpochProofQuotePool', () => { // should delete all quotes for epochs 2 and 3 pool.deleteQuotesToEpoch(3n); + expect(metricsMock.recordRemovedObjects).toHaveBeenCalledWith(quotesForEpoch2.length + quotesForEpoch3.length); + expect(pool.getQuotes(2n)).toHaveLength(0); expect(pool.getQuotes(3n)).toHaveLength(0); diff --git a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts index a9166838a1b..3280aba66cc 100644 --- a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts +++ b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts @@ -1,26 +1,41 @@ import { type EpochProofQuote } from '@aztec/circuit-types'; import { type EpochProofQuotePool } from './epoch_proof_quote_pool.js'; +import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { TelemetryClient } from '@aztec/telemetry-client'; export class MemoryEpochProofQuotePool implements EpochProofQuotePool { private quotes: Map; - constructor() { + private metrics: PoolInstrumentation; + + constructor(telemetry: TelemetryClient) { this.quotes = new Map(); + this.metrics = new PoolInstrumentation(telemetry, 'MemoryEpochProofQuotePool'); } + addQuote(quote: EpochProofQuote) { const epoch = quote.payload.epochToProve; if (!this.quotes.has(epoch)) { this.quotes.set(epoch, []); } this.quotes.get(epoch)!.push(quote); + + this.metrics.recordAddedObjects(1); } getQuotes(epoch: bigint): EpochProofQuote[] { return this.quotes.get(epoch) || []; } deleteQuotesToEpoch(epoch: bigint): void { const expiredEpochs = Array.from(this.quotes.keys()).filter(k => k <= epoch); + + let removedObjectsCount = 0; for (const expiredEpoch of expiredEpochs) { + // For logging + removedObjectsCount += this.quotes.get(expiredEpoch)?.length || 0; + this.quotes.delete(expiredEpoch); } + + this.metrics.recordRemovedObjects(removedObjectsCount); } } From 8b43c565ea631d23b1b298f36b439166915d41f4 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Tue, 8 Oct 2024 18:01:30 +0000 Subject: [PATCH 3/8] fix: remove unneeded things --- yarn-project/p2p/src/client/p2p_metrics.ts | 6 ------ yarn-project/p2p/src/service/libp2p_service.ts | 11 ----------- 2 files changed, 17 deletions(-) delete mode 100644 yarn-project/p2p/src/client/p2p_metrics.ts diff --git a/yarn-project/p2p/src/client/p2p_metrics.ts b/yarn-project/p2p/src/client/p2p_metrics.ts deleted file mode 100644 index 5060aa57e81..00000000000 --- a/yarn-project/p2p/src/client/p2p_metrics.ts +++ /dev/null @@ -1,6 +0,0 @@ -// TODO - -// - Create an interface that can transform our existing Meter interface into something that the libp2p gossipsub metrics -// can absorb! - -// Then we should be able to hook into their existing metrics \ No newline at end of file diff --git a/yarn-project/p2p/src/service/libp2p_service.ts b/yarn-project/p2p/src/service/libp2p_service.ts index 275a1e03e60..755439b5f2e 100644 --- a/yarn-project/p2p/src/service/libp2p_service.ts +++ b/yarn-project/p2p/src/service/libp2p_service.ts @@ -260,17 +260,6 @@ export class LibP2PService implements P2PService { invalidMessageDeliveriesWeight: -20, invalidMessageDeliveriesDecay: 0.5, }), - // TODO(md): Think about adding these back in - // [BlockProposal.p2pTopic]: createTopicScoreParams({ - // topicWeight: 1, - // invalidMessageDeliveriesWeight: -20, - // invalidMessageDeliveriesDecay: 0.5, - // }), - // [BlockAttestation.p2pTopic]: createTopicScoreParams({ - // topicWeight: 1, - // invalidMessageDeliveriesWeight: -20, - // invalidMessageDeliveriesDecay: 0.5, - // }), }, }), }) as (components: GossipSubComponents) => GossipSub, From 17195763b1bdbce8f211e90bdab9ac6b2716a8c9 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Tue, 8 Oct 2024 18:02:18 +0000 Subject: [PATCH 4/8] fmt --- .../src/attestation_pool/memory_attestation_pool.test.ts | 9 ++++----- .../p2p/src/attestation_pool/memory_attestation_pool.ts | 4 ++-- .../memory_epoch_proof_quote_pool.test.ts | 9 +++++---- .../memory_epoch_proof_quote_pool.ts | 4 ++-- yarn-project/p2p/src/tx_pool/instrumentation.ts | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts index c65cf7681a6..61219b4cd12 100644 --- a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts +++ b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.test.ts @@ -1,13 +1,13 @@ +import { type BlockAttestation } from '@aztec/circuit-types'; 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 '../tx_pool/instrumentation.js'; import { InMemoryAttestationPool } from './memory_attestation_pool.js'; import { generateAccount, mockAttestation } from './mocks.js'; -import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; -import { BlockAttestation } from '@aztec/circuit-types'; -import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; -import { mock, MockProxy } from 'jest-mock-extended'; const NUMBER_OF_SIGNERS_PER_TEST = 4; @@ -25,7 +25,6 @@ describe('MemoryAttestationPool', () => { ap = new InMemoryAttestationPool(telemetry); signers = Array.from({ length: NUMBER_OF_SIGNERS_PER_TEST }, generateAccount); - metricsMock = mock>(); // Can i overwrite this like this?? (ap as any).metrics = metricsMock; diff --git a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts index f26700508e5..e82424915f6 100644 --- a/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts +++ b/yarn-project/p2p/src/attestation_pool/memory_attestation_pool.ts @@ -1,9 +1,9 @@ import { type BlockAttestation } from '@aztec/circuit-types'; import { createDebugLogger } from '@aztec/foundation/log'; +import { type TelemetryClient } from '@aztec/telemetry-client'; -import { type AttestationPool } from './attestation_pool.js'; import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; -import { TelemetryClient } from '@aztec/telemetry-client'; +import { type AttestationPool } from './attestation_pool.js'; export class InMemoryAttestationPool implements AttestationPool { private metrics: PoolInstrumentation; diff --git a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts index e593336a31c..b007ff7feb3 100644 --- a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts +++ b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts @@ -1,9 +1,10 @@ -import { EpochProofQuote, mockEpochProofQuote } from '@aztec/circuit-types'; +import { type EpochProofQuote, mockEpochProofQuote } from '@aztec/circuit-types'; +import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; + +import { type MockProxy, mock } from 'jest-mock-extended'; +import { type PoolInstrumentation } from '../tx_pool/instrumentation.js'; import { MemoryEpochProofQuotePool } from './memory_epoch_proof_quote_pool.js'; -import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; -import { mock, MockProxy } from 'jest-mock-extended'; -import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; describe('MemoryEpochProofQuotePool', () => { let pool: MemoryEpochProofQuotePool; diff --git a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts index 3280aba66cc..da07fd496d2 100644 --- a/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts +++ b/yarn-project/p2p/src/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts @@ -1,8 +1,8 @@ import { type EpochProofQuote } from '@aztec/circuit-types'; +import { type TelemetryClient } from '@aztec/telemetry-client'; -import { type EpochProofQuotePool } from './epoch_proof_quote_pool.js'; import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; -import { TelemetryClient } from '@aztec/telemetry-client'; +import { type EpochProofQuotePool } from './epoch_proof_quote_pool.js'; export class MemoryEpochProofQuotePool implements EpochProofQuotePool { private quotes: Map; diff --git a/yarn-project/p2p/src/tx_pool/instrumentation.ts b/yarn-project/p2p/src/tx_pool/instrumentation.ts index 96c87efefc4..44f8bdc0415 100644 --- a/yarn-project/p2p/src/tx_pool/instrumentation.ts +++ b/yarn-project/p2p/src/tx_pool/instrumentation.ts @@ -1,4 +1,4 @@ -import { Gossipable } from '@aztec/circuit-types'; +import { type Gossipable } from '@aztec/circuit-types'; import { Attributes, type Histogram, Metrics, type TelemetryClient, type UpDownCounter } from '@aztec/telemetry-client'; export type TxStatus = 'pending' | 'mined'; From 548d3cff939010646b11bed70d832e7e723b772b Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Tue, 8 Oct 2024 19:13:45 +0000 Subject: [PATCH 5/8] fmt --- yarn-project/circuit-types/src/p2p/block_attestation.ts | 2 +- yarn-project/circuit-types/src/p2p/block_proposal.ts | 2 +- yarn-project/circuit-types/src/p2p/consensus_payload.ts | 2 +- .../circuit-types/src/prover_coordination/epoch_proof_quote.ts | 2 +- .../src/prover_coordination/epoch_proof_quote_payload.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/yarn-project/circuit-types/src/p2p/block_attestation.ts b/yarn-project/circuit-types/src/p2p/block_attestation.ts index 3e7cdfcc13f..3800fa926df 100644 --- a/yarn-project/circuit-types/src/p2p/block_attestation.ts +++ b/yarn-project/circuit-types/src/p2p/block_attestation.ts @@ -74,6 +74,6 @@ export class BlockAttestation extends Gossipable { } getSize(): number { - return (this.payload.getSize() + this.signature.getSize()); + return this.payload.getSize() + this.signature.getSize(); } } diff --git a/yarn-project/circuit-types/src/p2p/block_proposal.ts b/yarn-project/circuit-types/src/p2p/block_proposal.ts index badbf2cfbcd..0e8dec898e5 100644 --- a/yarn-project/circuit-types/src/p2p/block_proposal.ts +++ b/yarn-project/circuit-types/src/p2p/block_proposal.ts @@ -77,6 +77,6 @@ export class BlockProposal extends Gossipable { } getSize(): number { - return (this.payload.getSize() + this.signature.getSize()); + return this.payload.getSize() + this.signature.getSize(); } } diff --git a/yarn-project/circuit-types/src/p2p/consensus_payload.ts b/yarn-project/circuit-types/src/p2p/consensus_payload.ts index bf18e1018fb..929d3612d98 100644 --- a/yarn-project/circuit-types/src/p2p/consensus_payload.ts +++ b/yarn-project/circuit-types/src/p2p/consensus_payload.ts @@ -56,6 +56,6 @@ export class ConsensusPayload implements Signable { * @returns The size of the consensus payload. */ getSize(): number { - return (this.header.getSize() + this.archive.size + this.txHashes.length * TxHash.SIZE); + return this.header.getSize() + this.archive.size + this.txHashes.length * TxHash.SIZE; } } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts index b1e508a24b9..be551f48f8d 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.ts @@ -75,6 +75,6 @@ export class EpochProofQuote extends Gossipable { * @returns The size of the epoch proof quote in bytes. */ getSize(): number { - return (this.payload.getSize() + this.signature.getSize()); + return this.payload.getSize() + this.signature.getSize(); } } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts index acd51c2d842..500cd2d72f8 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts @@ -86,7 +86,7 @@ export class EpochProofQuotePayload { getSize(): number { // 32 bytes for epochToProve, 32 bytes for validUntilSlot, 32 bytes for bondAmount, 20 bytes for prover, 4 bytes for basisPointFee - return (32 + 32 + 32 + EthAddress.SIZE_IN_BYTES + 4); + return 32 + 32 + 32 + EthAddress.SIZE_IN_BYTES + 4; } [inspect.custom](): string { From 22e826284000e6c54123adc274f9b81bf02ec6a9 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 9 Oct 2024 12:25:57 +0000 Subject: [PATCH 6/8] fix: cache size values --- .../src/p2p/consensus_payload.ts | 18 +++++++++++++-- .../epoch_proof_quote_payload.ts | 21 +++++++++++++++--- .../src/eth-signature/eth_signature.ts | 22 ++++++++++++++++--- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/yarn-project/circuit-types/src/p2p/consensus_payload.ts b/yarn-project/circuit-types/src/p2p/consensus_payload.ts index 929d3612d98..73996317636 100644 --- a/yarn-project/circuit-types/src/p2p/consensus_payload.ts +++ b/yarn-project/circuit-types/src/p2p/consensus_payload.ts @@ -9,6 +9,10 @@ import { TxHash } from '../tx/tx_hash.js'; import { type Signable } from './signature_utils.js'; export class ConsensusPayload implements Signable { + // Cached values + private asBuffer: Buffer | undefined; + private size: number | undefined; + constructor( /** The block header the attestation is made over */ public readonly header: Header, @@ -31,7 +35,12 @@ export class ConsensusPayload implements Signable { } toBuffer(): Buffer { - return serializeToBuffer([this.header, this.archive, this.txHashes.length, this.txHashes]); + if (this.asBuffer) { + return this.asBuffer; + } + this.asBuffer = serializeToBuffer([this.header, this.archive, this.txHashes.length, this.txHashes]); + this.size = this.asBuffer.length; + return this.asBuffer; } static fromBuffer(buf: Buffer | BufferReader): ConsensusPayload { @@ -56,6 +65,11 @@ export class ConsensusPayload implements Signable { * @returns The size of the consensus payload. */ getSize(): number { - return this.header.getSize() + this.archive.size + this.txHashes.length * TxHash.SIZE; + // We cache size to avoid recalculating it + if (this.size) { + return this.size; + } + this.toBuffer(); + return this.size!; } } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts index 500cd2d72f8..15086276c06 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote_payload.ts @@ -5,6 +5,10 @@ import { type FieldsOf } from '@aztec/foundation/types'; import { inspect } from 'util'; export class EpochProofQuotePayload { + // Cached values + private asBuffer: Buffer | undefined; + private size: number | undefined; + constructor( public readonly epochToProve: bigint, public readonly validUntilSlot: bigint, @@ -24,7 +28,13 @@ export class EpochProofQuotePayload { } toBuffer(): Buffer { - return serializeToBuffer(...EpochProofQuotePayload.getFields(this)); + // We cache the buffer to avoid recalculating it + if (this.asBuffer) { + return this.asBuffer; + } + this.asBuffer = serializeToBuffer(...EpochProofQuotePayload.getFields(this)); + this.size = this.asBuffer.length; + return this.asBuffer; } static fromBuffer(buf: Buffer | BufferReader): EpochProofQuotePayload { @@ -85,8 +95,13 @@ export class EpochProofQuotePayload { } getSize(): number { - // 32 bytes for epochToProve, 32 bytes for validUntilSlot, 32 bytes for bondAmount, 20 bytes for prover, 4 bytes for basisPointFee - return 32 + 32 + 32 + EthAddress.SIZE_IN_BYTES + 4; + // We cache size to avoid recalculating it + if (this.size) { + return this.size; + } + // Size is cached when calling toBuffer + this.toBuffer(); + return this.size!; } [inspect.custom](): string { diff --git a/yarn-project/foundation/src/eth-signature/eth_signature.ts b/yarn-project/foundation/src/eth-signature/eth_signature.ts index 83a9f780169..fe817feee3c 100644 --- a/yarn-project/foundation/src/eth-signature/eth_signature.ts +++ b/yarn-project/foundation/src/eth-signature/eth_signature.ts @@ -19,6 +19,10 @@ export type ViemSignature = { * Contains a signature split into it's primary components (r,s,v) */ export class Signature { + // Cached values + private asBuffer: Buffer | undefined; + private size: number | undefined; + constructor( /** The r value of the signature */ public readonly r: Buffer32, @@ -73,12 +77,24 @@ export class Signature { } toBuffer(): Buffer { - return serializeToBuffer([this.r, this.s, this.v]); + // We cache the buffer to avoid recalculating it + if (this.asBuffer) { + return this.asBuffer; + } + this.asBuffer = serializeToBuffer([this.r, this.s, this.v]); + this.size = this.asBuffer.length; + return this.asBuffer; } getSize(): number { - // 32 bytes for r, 32 bytes for s, 4 bytes for v - return Buffer32.SIZE + Buffer32.SIZE + 4; + // We cache size to avoid recalculating it + if (this.size) { + return this.size; + } + + // Size is cached when calling toBuffer + this.toBuffer(); + return this.size!; } to0xString(): `0x${string}` { From 849984a430519e3457e3fe13218dd0e574e7eeb7 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 9 Oct 2024 12:31:27 +0000 Subject: [PATCH 7/8] fix: new interface --- .../memory_attestation_pool.test.ts | 2 +- .../memory_attestation_pool.ts | 2 +- .../memory_epoch_proof_quote_pool.test.ts | 2 +- .../memory_epoch_proof_quote_pool.ts | 2 +- .../{tx_pool => }/instrumentation.ts | 34 ++++--------------- .../src/mem_pools/tx_pool/aztec_kv_tx_pool.ts | 12 +++---- .../src/mem_pools/tx_pool/memory_tx_pool.ts | 12 +++---- 7 files changed, 22 insertions(+), 44 deletions(-) rename yarn-project/p2p/src/mem_pools/{tx_pool => }/instrumentation.ts (69%) diff --git a/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.test.ts b/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.test.ts index 61219b4cd12..695f2699b02 100644 --- a/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.test.ts +++ b/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.test.ts @@ -5,7 +5,7 @@ import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; import { type MockProxy, mock } from 'jest-mock-extended'; import { type PrivateKeyAccount } from 'viem'; -import { type PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { type PoolInstrumentation } from '../instrumentation.js'; import { InMemoryAttestationPool } from './memory_attestation_pool.js'; import { generateAccount, mockAttestation } from './mocks.js'; diff --git a/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.ts b/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.ts index e82424915f6..daf998b3fb5 100644 --- a/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.ts +++ b/yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.ts @@ -2,7 +2,7 @@ import { type BlockAttestation } from '@aztec/circuit-types'; import { createDebugLogger } from '@aztec/foundation/log'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { PoolInstrumentation } from '../instrumentation.js'; import { type AttestationPool } from './attestation_pool.js'; export class InMemoryAttestationPool implements AttestationPool { diff --git a/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts b/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts index b007ff7feb3..2358c833a75 100644 --- a/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts +++ b/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.test.ts @@ -3,7 +3,7 @@ import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; import { type MockProxy, mock } from 'jest-mock-extended'; -import { type PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { type PoolInstrumentation } from '../instrumentation.js'; import { MemoryEpochProofQuotePool } from './memory_epoch_proof_quote_pool.js'; describe('MemoryEpochProofQuotePool', () => { diff --git a/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts b/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts index da07fd496d2..419f1c61463 100644 --- a/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts +++ b/yarn-project/p2p/src/mem_pools/epoch_proof_quote_pool/memory_epoch_proof_quote_pool.ts @@ -1,7 +1,7 @@ import { type EpochProofQuote } from '@aztec/circuit-types'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { PoolInstrumentation } from '../tx_pool/instrumentation.js'; +import { PoolInstrumentation } from '../instrumentation.js'; import { type EpochProofQuotePool } from './epoch_proof_quote_pool.js'; export class MemoryEpochProofQuotePool implements EpochProofQuotePool { diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/instrumentation.ts b/yarn-project/p2p/src/mem_pools/instrumentation.ts similarity index 69% rename from yarn-project/p2p/src/mem_pools/tx_pool/instrumentation.ts rename to yarn-project/p2p/src/mem_pools/instrumentation.ts index 44f8bdc0415..bb3c8480c9c 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/instrumentation.ts +++ b/yarn-project/p2p/src/mem_pools/instrumentation.ts @@ -1,8 +1,6 @@ import { type Gossipable } from '@aztec/circuit-types'; import { Attributes, type Histogram, Metrics, type TelemetryClient, type UpDownCounter } from '@aztec/telemetry-client'; -export type TxStatus = 'pending' | 'mined'; - /** * Instrumentation class for the Pools (TxPool, AttestationPool, etc). */ @@ -43,51 +41,31 @@ export class PoolInstrumentation { * Updates the metrics with the new objects. * @param txs - The objects to record */ - public recordAddedObjectsWithStatus(status: string, count = 1) { + public recordAddedObjects(count = 1, status?: string) { if (count < 0) { throw new Error('Count must be positive'); } if (count === 0) { return; } - this.objectsInMempool.add(count, { - [Attributes.STATUS]: status, - }); - } + const attributes = status ? { [Attributes.STATUS]: status } : {}; - public recordAddedObjects(count = 1) { - if (count < 0) { - throw new Error('Count must be positive'); - } - if (count === 0) { - return; - } - this.objectsInMempool.add(count); + this.objectsInMempool.add(count, attributes); } /** * Updates the metrics by removing objects from the mempool. * @param count - The number of objects to remove from the mempool */ - public recordRemovedObjectsWithStatus(status: string, count = 1) { + public recordRemovedObjects(count = 1, status?: string) { if (count < 0) { throw new Error('Count must be positive'); } if (count === 0) { return; } - this.objectsInMempool.add(-1 * count, { - [Attributes.STATUS]: status, - }); - } - public recordRemovedObjects(count = 1) { - if (count < 0) { - throw new Error('Count must be positive'); - } - if (count === 0) { - return; - } - this.objectsInMempool.add(-1 * count); + const attributes = status ? { [Attributes.STATUS]: status } : {}; + this.objectsInMempool.add(-1 * count, attributes); } } diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts b/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts index ffac3d6924b..432eefd012a 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts +++ b/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts @@ -4,7 +4,7 @@ import { type Logger, createDebugLogger } from '@aztec/foundation/log'; import { type AztecKVStore, type AztecMap, type AztecSet } from '@aztec/kv-store'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { PoolInstrumentation } from './instrumentation.js'; +import { PoolInstrumentation } from '../instrumentation.js'; import { type TxPool } from './tx_pool.js'; /** @@ -51,8 +51,8 @@ export class AztecKVTxPool implements TxPool { void this.#pendingTxs.delete(key); } } - this.#metrics.recordRemovedObjectsWithStatus('pending', deleted); - this.#metrics.recordAddedObjectsWithStatus('mined', txHashes.length); + this.#metrics.recordRemovedObjects(deleted, 'pending'); + this.#metrics.recordAddedObjects(txHashes.length, 'mined'); }); } @@ -111,7 +111,7 @@ export class AztecKVTxPool implements TxPool { } } - this.#metrics.recordAddedObjectsWithStatus('pending', pendingCount); + this.#metrics.recordAddedObjects(pendingCount, 'pending'); }); } @@ -138,8 +138,8 @@ export class AztecKVTxPool implements TxPool { } } - this.#metrics.recordRemovedObjectsWithStatus('pending', pendingDeleted); - this.#metrics.recordRemovedObjectsWithStatus('mined', minedDeleted); + this.#metrics.recordRemovedObjects(pendingDeleted, 'pending'); + this.#metrics.recordRemovedObjects(minedDeleted, 'mined'); }); } diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts b/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts index 6519a34175f..9e6d72ea5a4 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts +++ b/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts @@ -3,7 +3,7 @@ import { type TxAddedToPoolStats } from '@aztec/circuit-types/stats'; import { createDebugLogger } from '@aztec/foundation/log'; import { type TelemetryClient } from '@aztec/telemetry-client'; -import { PoolInstrumentation } from './instrumentation.js'; +import { PoolInstrumentation } from '../instrumentation.js'; import { type TxPool } from './tx_pool.js'; /** @@ -36,8 +36,8 @@ export class InMemoryTxPool implements TxPool { this.minedTxs.add(key); this.pendingTxs.delete(key); } - this.metrics.recordRemovedObjectsWithStatus('pending', txHashes.length); - this.metrics.recordAddedObjectsWithStatus('mined', txHashes.length); + this.metrics.recordRemovedObjects(txHashes.length, 'pending'); + this.metrics.recordAddedObjects(txHashes.length, 'mined'); return Promise.resolve(); } @@ -93,7 +93,7 @@ export class InMemoryTxPool implements TxPool { } } - this.metrics.recordAddedObjectsWithStatus('pending', pending); + this.metrics.recordAddedObjects(pending, 'pending'); return Promise.resolve(); } @@ -113,8 +113,8 @@ export class InMemoryTxPool implements TxPool { deletedMined += this.minedTxs.delete(key) ? 1 : 0; } - this.metrics.recordRemovedObjectsWithStatus('pending', deletedPending); - this.metrics.recordRemovedObjectsWithStatus('mined', deletedMined); + this.metrics.recordRemovedObjects(deletedPending, 'pending'); + this.metrics.recordRemovedObjects(deletedMined, 'mined'); return Promise.resolve(); } From 83f44b09dc33a9c9a37554694637843d12b6cf2e Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 9 Oct 2024 22:16:06 +0000 Subject: [PATCH 8/8] fix: serde tests with cached value --- .../src/p2p/block_attestation.test.ts | 14 +++++++++----- .../circuit-types/src/p2p/block_proposal.test.ts | 10 +++++++--- .../circuit-types/src/p2p/consensus_payload.ts | 15 +++++---------- .../epoch_proof_quote.test.ts | 11 +++++++++-- .../src/eth-signature/eth_signature.test.ts | 13 +++++++++---- .../src/eth-signature/eth_signature.ts | 16 +++++----------- 6 files changed, 44 insertions(+), 35 deletions(-) diff --git a/yarn-project/circuit-types/src/p2p/block_attestation.test.ts b/yarn-project/circuit-types/src/p2p/block_attestation.test.ts index e60fdb09940..908e4551c17 100644 --- a/yarn-project/circuit-types/src/p2p/block_attestation.test.ts +++ b/yarn-project/circuit-types/src/p2p/block_attestation.test.ts @@ -5,23 +5,27 @@ import { BlockAttestation } from './block_attestation.js'; import { makeBlockAttestation } from './mocks.js'; describe('Block Attestation serialization / deserialization', () => { + const checkEquivalence = (serialized: BlockAttestation, deserialized: BlockAttestation) => { + expect(deserialized.getSize()).toEqual(serialized.getSize()); + expect(deserialized).toEqual(serialized); + }; + it('Should serialize / deserialize', () => { const attestation = makeBlockAttestation(); const serialized = attestation.toBuffer(); const deserialized = BlockAttestation.fromBuffer(serialized); - - expect(deserialized).toEqual(attestation); + checkEquivalence(attestation, deserialized); }); it('Should serialize / deserialize + recover sender', () => { const account = Secp256k1Signer.random(); - const proposal = makeBlockAttestation(account); - const serialized = proposal.toBuffer(); + const attestation = makeBlockAttestation(account); + const serialized = attestation.toBuffer(); const deserialized = BlockAttestation.fromBuffer(serialized); - expect(deserialized).toEqual(proposal); + checkEquivalence(attestation, deserialized); // Recover signature const sender = deserialized.getSender(); diff --git a/yarn-project/circuit-types/src/p2p/block_proposal.test.ts b/yarn-project/circuit-types/src/p2p/block_proposal.test.ts index af69c619532..4be10936fec 100644 --- a/yarn-project/circuit-types/src/p2p/block_proposal.test.ts +++ b/yarn-project/circuit-types/src/p2p/block_proposal.test.ts @@ -5,13 +5,17 @@ import { BlockProposal } from './block_proposal.js'; import { makeBlockProposal } from './mocks.js'; describe('Block Proposal serialization / deserialization', () => { + const checkEquivalence = (serialized: BlockProposal, deserialized: BlockProposal) => { + expect(deserialized.getSize()).toEqual(serialized.getSize()); + expect(deserialized).toEqual(serialized); + }; + it('Should serialize / deserialize', () => { const proposal = makeBlockProposal(); const serialized = proposal.toBuffer(); const deserialized = BlockProposal.fromBuffer(serialized); - - expect(deserialized).toEqual(proposal); + checkEquivalence(proposal, deserialized); }); it('Should serialize / deserialize + recover sender', () => { @@ -21,7 +25,7 @@ describe('Block Proposal serialization / deserialization', () => { const serialized = proposal.toBuffer(); const deserialized = BlockProposal.fromBuffer(serialized); - expect(deserialized).toEqual(proposal); + checkEquivalence(proposal, deserialized); // Recover signature const sender = deserialized.getSender(); diff --git a/yarn-project/circuit-types/src/p2p/consensus_payload.ts b/yarn-project/circuit-types/src/p2p/consensus_payload.ts index 73996317636..d3d13f2d2d0 100644 --- a/yarn-project/circuit-types/src/p2p/consensus_payload.ts +++ b/yarn-project/circuit-types/src/p2p/consensus_payload.ts @@ -9,8 +9,6 @@ import { TxHash } from '../tx/tx_hash.js'; import { type Signable } from './signature_utils.js'; export class ConsensusPayload implements Signable { - // Cached values - private asBuffer: Buffer | undefined; private size: number | undefined; constructor( @@ -35,12 +33,9 @@ export class ConsensusPayload implements Signable { } toBuffer(): Buffer { - if (this.asBuffer) { - return this.asBuffer; - } - this.asBuffer = serializeToBuffer([this.header, this.archive, this.txHashes.length, this.txHashes]); - this.size = this.asBuffer.length; - return this.asBuffer; + const buffer = serializeToBuffer([this.header, this.archive, this.txHashes.length, this.txHashes]); + this.size = buffer.length; + return buffer; } static fromBuffer(buf: Buffer | BufferReader): ConsensusPayload { @@ -69,7 +64,7 @@ export class ConsensusPayload implements Signable { if (this.size) { return this.size; } - this.toBuffer(); - return this.size!; + this.size = this.toBuffer().length; + return this.size; } } diff --git a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.test.ts b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.test.ts index 78c2e0572c3..78f68edee04 100644 --- a/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.test.ts +++ b/yarn-project/circuit-types/src/prover_coordination/epoch_proof_quote.test.ts @@ -19,11 +19,18 @@ describe('epoch proof quote', () => { quote = new EpochProofQuote(payload, Signature.random()); }); + const checkEquivalence = (serialized: EpochProofQuote, deserialized: EpochProofQuote) => { + expect(deserialized.getSize()).toEqual(serialized.getSize()); + expect(deserialized).toEqual(serialized); + }; + it('should serialize and deserialize from buffer', () => { - expect(EpochProofQuote.fromBuffer(quote.toBuffer())).toEqual(quote); + const deserialised = EpochProofQuote.fromBuffer(quote.toBuffer()); + checkEquivalence(quote, deserialised); }); it('should serialize and deserialize from JSON', () => { - expect(EpochProofQuote.fromJSON(quote.toJSON())).toEqual(quote); + const deserialised = EpochProofQuote.fromJSON(quote.toJSON()); + checkEquivalence(quote, deserialised); }); }); diff --git a/yarn-project/foundation/src/eth-signature/eth_signature.test.ts b/yarn-project/foundation/src/eth-signature/eth_signature.test.ts index e93c199eacf..aa1c7f93665 100644 --- a/yarn-project/foundation/src/eth-signature/eth_signature.test.ts +++ b/yarn-project/foundation/src/eth-signature/eth_signature.test.ts @@ -20,16 +20,21 @@ describe('eth signature', () => { signature = signer.sign(message); }); + const checkEquivalence = (serialized: Signature, deserialized: Signature) => { + expect(deserialized.getSize()).toEqual(serialized.getSize()); + expect(deserialized).toEqual(serialized); + }; + it('should serialize / deserialize to buffer', () => { const serialized = signature.toBuffer(); const deserialized = Signature.fromBuffer(serialized); - expect(deserialized).toEqual(signature); + checkEquivalence(signature, deserialized); }); it('should serialize / deserialize real signature to hex string', () => { const serialized = signature.to0xString(); const deserialized = Signature.from0xString(serialized); - expect(deserialized).toEqual(signature); + checkEquivalence(signature, deserialized); }); it('should recover signer from signature', () => { @@ -41,13 +46,13 @@ describe('eth signature', () => { const signature = new Signature(Buffer32.random(), Buffer32.random(), 1, false); const serialized = signature.to0xString(); const deserialized = Signature.from0xString(serialized); - expect(deserialized).toEqual(signature); + checkEquivalence(signature, deserialized); }); it('should serialize / deserialize to hex string with 2-digit v', () => { const signature = new Signature(Buffer32.random(), Buffer32.random(), 26, false); const serialized = signature.to0xString(); const deserialized = Signature.from0xString(serialized); - expect(deserialized).toEqual(signature); + checkEquivalence(signature, deserialized); }); }); diff --git a/yarn-project/foundation/src/eth-signature/eth_signature.ts b/yarn-project/foundation/src/eth-signature/eth_signature.ts index fe817feee3c..4226deb801c 100644 --- a/yarn-project/foundation/src/eth-signature/eth_signature.ts +++ b/yarn-project/foundation/src/eth-signature/eth_signature.ts @@ -20,7 +20,6 @@ export type ViemSignature = { */ export class Signature { // Cached values - private asBuffer: Buffer | undefined; private size: number | undefined; constructor( @@ -77,13 +76,9 @@ export class Signature { } toBuffer(): Buffer { - // We cache the buffer to avoid recalculating it - if (this.asBuffer) { - return this.asBuffer; - } - this.asBuffer = serializeToBuffer([this.r, this.s, this.v]); - this.size = this.asBuffer.length; - return this.asBuffer; + const buffer = serializeToBuffer([this.r, this.s, this.v]); + this.size = buffer.length; + return buffer; } getSize(): number { @@ -92,9 +87,8 @@ export class Signature { return this.size; } - // Size is cached when calling toBuffer - this.toBuffer(); - return this.size!; + this.size = this.toBuffer().length; + return this.size; } to0xString(): `0x${string}` {