fix(p2p): do not penalize peers that signal a missing block with Fr.ZERO

[mrzeszutko]

Stacked on: [mr/fix-block-txs-validation-archiver-fallback](https://github.com/AztecProtocol/aztec-packages/pull/23624) (the archiver-fallback PR).

---

Summary

A peer that legitimately can't find the requested block in its attestation pool or archiver, but matches the requested hashes against its tx pool and sends those txs back, signals "I don't have the block" by setting archiveRoot = Fr.ZERO in the response (block_txs_handler.ts:54-58). The requester's validation currently treats that response the same as a malicious archive-root mismatch and applies a MidToleranceError peer penalty. After enough such responses from the same helpful peer, that peer is disconnected by the requester for behaviour the protocol explicitly documents as legitimate.

This PR makes the validator recognise Fr.ZERO as the "I don't have the block" signal and stop penalising peers for it.

What's broken

Responder side (correct, intentional)

block_txs_handler.ts:54-58 — when the responder lacks the block (no proposal, no archived block) but the request carried full tx hashes, it tries to serve from its pool by hash and signals the "I don't know the block" condition with archiveRoot = Fr.zero():

if (!txHashes && requestedTxsHashes !== undefined) {
  const responseTxs = (await txPool.getTxsByHash(requestedTxsHashes)).filter(tx => !!tx);
  const response = new BlockTxsResponse(Fr.zero(), new TxArray(...responseTxs), BitVector.init(0, []));
  return response.toBuffer();
}

Requester side (broken)

The validator at libp2p_service.ts:1525-1530 penalizes any archive-root mismatch with MidToleranceError (-10 score points) and throws — including Fr.zero:

if (!response.archiveRoot.equals(request.archiveRoot)) {
  this.peerManager.penalizePeer(peerId, PeerErrorSeverity.MidToleranceError);
  throw new ValidationError(...);
}

After 5 such responses from the same helpful peer, that peer is at -50 in the requester's score book → disconnected by pruneUnhealthyPeers (peer_manager.ts:601-603).

Receiver-side code already documented the correct intent

The wrongful penalty contradicts what the receiver-side code explicitly says should happen. batch_tx_requester.ts:586-600 has handleArchiveRootMismatch, whose docstring spells out exactly the semantic we're restoring:

/**
 * Handles an archive root mismatch between local state and peer response.
 *
 * - Response archive is Fr.ZERO (peer pruned proposal, legitimate): marks peer dumb.
 * - Non-zero archive mismatch (malicious response): penalises + marks dumb.
 */
private handleArchiveRootMismatch(peerId: PeerId, response: BlockTxsResponse): void {
  if (!response.archiveRoot.isZero()) {
    this.peers.penalisePeer(peerId, PeerErrorSeverity.LowToleranceError);
  }
  this.peers.markPeerDumb(peerId);
  this.txsMetadata.clearPeerData(peerId);
}

But this function is only reached from decideIfPeerIsSmart → handleSuccessResponseFromPeer, which only runs when validation returns true. The validator's first check (archive-root equality) rejects every archive-mismatched response — including Fr.zero — so handleArchiveRootMismatch is never actually invoked. The "Fr.zero is legitimate" exemption it encodes has been unreachable since the validator's archive-root check was added.

The flow today:

   reqresp response
        │
        ▼
   validateRequestedBlockTxsConsistency
   ├─ Fr.zero       → reject + Mid penalty (BUG, contradicts the docstring above)
   ├─ non-zero ≠    → reject + Mid penalty
   └─ matches archive root ──► handleSuccessResponseFromPeer
                                  └─ decideIfPeerIsSmart
                                       └─ hasArchiveRootMismatch?
                                            always false (validator filtered them all out)
                                            ──► handleArchiveRootMismatch   ◄── DEAD CODE

So the receiver side already knew Fr.zero should not be penalised; the decision was just being made at the wrong layer. This PR moves it to the validator, where it can actually fire. handleArchiveRootMismatch itself remains dead code (separate cleanup candidate, not in this PR).

Fix

Special-case response.archiveRoot.isZero() at the top of validateRequestedBlockTxsConsistency so the archive-root mismatch path is bypassed for Fr.zero responses. We still return false (the txs are dropped because we can't verify membership/order without the block) but no peer penalty is applied — matching the intent of the Fr.zero exemption in batch_tx_requester.ts:593-600.

// libp2p_service.ts (inside validateRequestedBlockTxsConsistency)
if (response.archiveRoot.isZero()) {
  this.logger.debug(`Peer ${peerId.toString()} signalled missing block with Fr.zero archive root`);
  return false;
}

if (!response.archiveRoot.equals(request.archiveRoot)) {
  this.peerManager.penalizePeer(peerId, PeerErrorSeverity.MidToleranceError);
  ...
}

The validator's other checks are unchanged. The early return prevents the bitvector-length and maxReturnable checks downstream from firing on a zero-length bitvector response, which would otherwise also wrongly penalise the peer.

Tests

Unit — p2p/src/services/libp2p/libp2p_service.test.ts

A new test in the validateRequestedBlockTxsConsistency describe block: "should not penalize a peer that signals lacking the block with Fr.ZERO archive root". Constructs a response with archiveRoot = Fr.ZERO and asserts that peerManager.penalizePeer is not called. Verified red before the fix, green after.

Integration — p2p/src/client/test/p2p_client.integration_block_txs.test.ts

A new test in the p2p client integration block txs protocol describe block: "requester does not penalize peer that returns Fr.zero (peer lacks proposal but matched by hash)". Drives a real reqresp BLOCK_TXS request over libp2p between two clients, lets the responder hit the Fr.zero branch in block_txs_handler, then runs the response through the requester's real validateRequestedBlockTxsConsistency and asserts the requester's peerManager.penalizePeer is not called with MidToleranceError or LowToleranceError. Verified red before the fix, green after.

[AztecBot]

Flakey Tests

🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry.

\033FLAKED\033 (8;;http://ci.aztec-labs.com/504c764c1d368192�504c764c1d3681928;;�):  yarn-project/end-to-end/scripts/run_test.sh simple src/e2e_epochs/epochs_invalidate_block.parallel.test.ts "proposer invalidates multiple checkpoints" (420s) (code: 0) group:e2e-p2p-epoch-flakes

[PhilWindle]

It looks to me like this functions would be better off returning more infomration than simply true/false. Perhaps ReqRespStatus. The case you have added gets translated to INTERNAL_ERROR. It has the same effect as NOT_FOUND but perhaps this is due a cleanup.