feat: add validation to request / response interactions + adjust scoring appropiately

yarn-project/circuit-types/src/tx/validator/empty_validator.ts

@@ -4,4 +4,8 @@ export class EmptyTxValidator<T extends AnyTx = AnyTx> implements TxValidator<T>
   public validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[]]> {
     return Promise.resolve([txs, []]);
   }
+
+  public validateTx(_tx: T): Promise<boolean> {
+    return Promise.resolve(true);
+  }
 }

yarn-project/circuit-types/src/tx/validator/tx_validator.ts

@@ -4,5 +4,6 @@ import { type Tx } from '../tx.js';
 export type AnyTx = Tx | ProcessedTx;
 
 export interface TxValidator<T extends AnyTx = AnyTx> {
+  validateTx(tx: T): Promise<boolean>;
   validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[]]>;
 }

yarn-project/p2p/src/client/p2p_client.ts

@@ -319,8 +319,6 @@ export class P2PClient implements P2P {
 
     this.log.debug(`Requested ${txHash.toString()} from peer | success = ${!!tx}`);
     if (tx) {
-      // TODO(https://github.com/AztecProtocol/aztec-packages/issues/8485): This check is not sufficient to validate the transaction. We need to validate the entire proof.
-      // TODO(https://github.com/AztecProtocol/aztec-packages/issues/8483): alter peer scoring system for a validator that returns an invalid transcation
       await this.txPool.addTxs([tx]);
     }
 

yarn-project/p2p/src/mocks/index.ts

@@ -1,3 +1,5 @@
+import { type ClientProtocolCircuitVerifier, type Tx } from '@aztec/circuit-types';
+
 import { noise } from '@chainsafe/libp2p-noise';
 import { yamux } from '@chainsafe/libp2p-yamux';
 import { bootstrap } from '@libp2p/bootstrap';
@@ -10,8 +12,10 @@ import { pingHandler, statusHandler } from '../service/reqresp/handlers.js';
 import {
   PING_PROTOCOL,
   type ReqRespSubProtocolHandlers,
+  type ReqRespSubProtocolValidators,
   STATUS_PROTOCOL,
   TX_REQ_PROTOCOL,
+  noopValidator,
 } from '../service/reqresp/interface.js';
 import { ReqResp } from '../service/reqresp/reqresp.js';
 
@@ -57,6 +61,14 @@ export const MOCK_SUB_PROTOCOL_HANDLERS: ReqRespSubProtocolHandlers = {
   [TX_REQ_PROTOCOL]: (_msg: any) => Promise.resolve(Uint8Array.from(Buffer.from('tx'))),
 };
 
+// By default, all requests are valid
+// If you want to test an invalid response, you can override the validator
+export const MOCK_SUB_PROTOCOL_VALIDATORS: ReqRespSubProtocolValidators = {
+  [PING_PROTOCOL]: noopValidator,
+  [STATUS_PROTOCOL]: noopValidator,
+  [TX_REQ_PROTOCOL]: noopValidator,
+};
+
 /**
  * @param numberOfNodes - the number of nodes to create
  * @returns An array of the created nodes
@@ -65,10 +77,13 @@ export const createNodes = async (peerManager: PeerManager, numberOfNodes: numbe
   return await Promise.all(Array.from({ length: numberOfNodes }, () => createReqResp(peerManager)));
 };
 
-// TODO: think about where else this can go
-export const startNodes = async (nodes: ReqRespNode[], subProtocolHandlers = MOCK_SUB_PROTOCOL_HANDLERS) => {
+export const startNodes = async (
+  nodes: ReqRespNode[],
+  subProtocolHandlers = MOCK_SUB_PROTOCOL_HANDLERS,
+  subProtocolValidators = MOCK_SUB_PROTOCOL_VALIDATORS,
+) => {
   for (const node of nodes) {
-    await node.req.start(subProtocolHandlers);
+    await node.req.start(subProtocolHandlers, subProtocolValidators);
   }
 };
 
@@ -105,3 +120,15 @@ export const connectToPeers = async (nodes: ReqRespNode[]): Promise<void> => {
     }
   }
 };
+
+// Mock circuit verifier for testing - reimplementation from bb to avoid dependency
+export class AlwaysTrueCircuitVerifier implements ClientProtocolCircuitVerifier {
+  verifyProof(_tx: Tx): Promise<boolean> {
+    return Promise.resolve(true);
+  }
+}
+export class AlwaysFalseCircuitVerifier implements ClientProtocolCircuitVerifier {
+  verifyProof(_tx: Tx): Promise<boolean> {
+    return Promise.resolve(false);
+  }
+}

yarn-project/p2p/src/service/libp2p_service.ts

@@ -47,13 +47,13 @@ import { PeerErrorSeverity } from './peer_scoring.js';
 import { pingHandler, statusHandler } from './reqresp/handlers.js';
 import {
   DEFAULT_SUB_PROTOCOL_HANDLERS,
+  DEFAULT_SUB_PROTOCOL_VALIDATORS,
   PING_PROTOCOL,
   type ReqRespSubProtocol,
   type ReqRespSubProtocolHandlers,
   STATUS_PROTOCOL,
   type SubProtocolMap,
   TX_REQ_PROTOCOL,
-  subProtocolMap,
 } from './reqresp/interface.js';
 import { ReqResp } from './reqresp/reqresp.js';
 import type { P2PService, PeerDiscoveryService } from './service.js';
@@ -162,7 +162,13 @@ export class LibP2PService implements P2PService {
       this.peerManager.heartbeat();
     }, this.config.peerCheckIntervalMS);
     this.discoveryRunningPromise.start();
-    await this.reqresp.start(this.requestResponseHandlers);
+
+    // Define the sub protocol validators - This is done within this start() method to gain a callback to the existing validateTx function
+    const reqrespSubProtocolValidators = {
+      ...DEFAULT_SUB_PROTOCOL_VALIDATORS,
+      [TX_REQ_PROTOCOL]: this.validateRequestedTx.bind(this),
+    };
+    await this.reqresp.start(this.requestResponseHandlers, reqrespSubProtocolValidators);
   }
 
   /**
@@ -302,18 +308,11 @@ export class LibP2PService implements P2PService {
    * @param request The request type to send
    * @returns
    */
-  async sendRequest<SubProtocol extends ReqRespSubProtocol>(
+  sendRequest<SubProtocol extends ReqRespSubProtocol>(
     protocol: SubProtocol,
     request: InstanceType<SubProtocolMap[SubProtocol]['request']>,
   ): Promise<InstanceType<SubProtocolMap[SubProtocol]['response']> | undefined> {
-    const pair = subProtocolMap[protocol];
-
-    const res = await this.reqresp.sendRequest(protocol, request.toBuffer());
-    if (!res) {
-      return undefined;
-    }
-
-    return pair.response.fromBuffer(res!);
+    return this.reqresp.sendRequest(protocol, request);
   }
 
   /**
@@ -418,28 +417,62 @@ export class LibP2PService implements P2PService {
     const txHashString = txHash.toString();
     this.logger.verbose(`Received tx ${txHashString} from external peer.`);
 
-    const isValidTx = await this.validateTx(tx, peerId);
+    const isValidTx = await this.validatePropagatedTx(tx, peerId);
 
     if (isValidTx) {
       await this.txPool.addTxs([tx]);
     }
   }
 
-  private async validateTx(tx: Tx, peerId: PeerId): Promise<boolean> {
+  /**
+   * Validate a tx that has been requested from a peer.
+   *
+   * The core component of this validator is that the tx hash MUST match the requested tx hash,
+   * In order to perform this check, the tx proof must be verified.
+   *
+   * Note: This function is called from within `ReqResp.sendRequest` as part of the
+   * TX_REQ_PROTOCOL subprotocol validation.
+   *
+   * @param requestedTxHash - The hash of the tx that was requested.
+   * @param responseTx - The tx that was received as a response to the request.
+   * @param peerId - The peer ID of the peer that sent the tx.
+   * @returns True if the tx is valid, false otherwise.
+   */
+  private async validateRequestedTx(requestedTxHash: TxHash, responseTx: Tx, peerId: PeerId): Promise<boolean> {
+    const proofValidator = new TxProofValidator(this.proofVerifier);
+    const validProof = await proofValidator.validateTx(responseTx);
+
+    // If the node returns the wrong data, we penalize it
+    if (!requestedTxHash.equals(responseTx.getTxHash())) {
+      // Returning the wrong data is a low tolerance error
+      this.peerManager.penalizePeer(peerId, PeerErrorSeverity.MidToleranceError);
+      return false;
+    }
+
+    if (!validProof) {
+      // If the proof is invalid, but the txHash is correct, then this is an active attack and we severly punish
+      this.peerManager.penalizePeer(peerId, PeerErrorSeverity.LowToleranceError);
+      return false;
+    }
+
+    return true;
+  }
+
+  private async validatePropagatedTx(tx: Tx, peerId: PeerId): Promise<boolean> {
     const blockNumber = (await this.l2BlockSource.getBlockNumber()) + 1;
     // basic data validation
     const dataValidator = new DataTxValidator();
-    const [_, dataInvalidTxs] = await dataValidator.validateTxs([tx]);
-    if (dataInvalidTxs.length > 0) {
+    const validData = await dataValidator.validateTx(tx);
+    if (!validData) {
       // penalize
       this.node.services.pubsub.score.markInvalidMessageDelivery(peerId.toString(), Tx.p2pTopic);
       return false;
     }
 
     // metadata validation
     const metadataValidator = new MetadataTxValidator(new Fr(this.config.l1ChainId), new Fr(blockNumber));
-    const [__, metaInvalidTxs] = await metadataValidator.validateTxs([tx]);
-    if (metaInvalidTxs.length > 0) {
+    const validMetadata = await metadataValidator.validateTx(tx);
+    if (!validMetadata) {
       // penalize
       this.node.services.pubsub.score.markInvalidMessageDelivery(peerId.toString(), Tx.p2pTopic);
       return false;
@@ -453,8 +486,8 @@ export class LibP2PService implements P2PService {
         return index;
       },
     });
-    const [___, doubleSpendInvalidTxs] = await doubleSpendValidator.validateTxs([tx]);
-    if (doubleSpendInvalidTxs.length > 0) {
+    const validDoubleSpend = await doubleSpendValidator.validateTx(tx);
+    if (!validDoubleSpend) {
       // check if nullifier is older than 20 blocks
       if (blockNumber - this.config.severePeerPenaltyBlockLength > 0) {
         const snapshotValidator = new DoubleSpendTxValidator({
@@ -467,9 +500,9 @@ export class LibP2PService implements P2PService {
           },
         });
 
-        const [____, snapshotInvalidTxs] = await snapshotValidator.validateTxs([tx]);
+        const validSnapshot = await snapshotValidator.validateTx(tx);
         // High penalty if nullifier is older than 20 blocks
-        if (snapshotInvalidTxs.length > 0) {
+        if (!validSnapshot) {
           // penalize
           this.peerManager.penalizePeer(peerId, PeerErrorSeverity.LowToleranceError);
           return false;
@@ -482,8 +515,8 @@ export class LibP2PService implements P2PService {
 
     // proof validation
     const proofValidator = new TxProofValidator(this.proofVerifier);
-    const [_____, proofInvalidTxs] = await proofValidator.validateTxs([tx]);
-    if (proofInvalidTxs.length > 0) {
+    const validProof = await proofValidator.validateTx(tx);
+    if (!validProof) {
       // penalize
       this.peerManager.penalizePeer(peerId, PeerErrorSeverity.MidToleranceError);
       return false;

yarn-project/p2p/src/service/reqresp/interface.ts

@@ -1,5 +1,7 @@
 import { Tx, TxHash } from '@aztec/circuit-types';
 
+import { type PeerId } from '@libp2p/interface';
+
 /*
  * Request Response Sub Protocols
  */
@@ -46,11 +48,29 @@ export interface ProtocolRateLimitQuota {
   globalLimit: RateLimitQuota;
 }
 
+export const noopValidator = () => Promise.resolve(true);
+
 /**
  * A type mapping from supprotocol to it's handling funciton
  */
 export type ReqRespSubProtocolHandlers = Record<ReqRespSubProtocol, ReqRespSubProtocolHandler>;
 
+type ResponseValidator<RequestIdentifier, Response> = (
+  request: RequestIdentifier,
+  response: Response,
+  peerId: PeerId,
+) => Promise<boolean>;
+
+export type ReqRespSubProtocolValidators = {
+  [S in ReqRespSubProtocol]: ResponseValidator<any, any>;
+};
+
+export const DEFAULT_SUB_PROTOCOL_VALIDATORS: ReqRespSubProtocolValidators = {
+  [PING_PROTOCOL]: noopValidator,
+  [STATUS_PROTOCOL]: noopValidator,
+  [TX_REQ_PROTOCOL]: noopValidator,
+};
+
 /**
  * Sub protocol map determines the request and response types for each
  * Req Resp protocol