fix(p2p): guard tx-pool nullifier index deletion by ownership

[rkarabut]

Problem

TxPoolIndices.#removeFromPendingIndices deleted each of a tx's nullifier keys unconditionally. After a same-nullifier conflict is resolved, the key can already point at the surviving tx (e.g. a protected tx A leaves the pending nullifier index, a higher-fee B spending the same nullifier is admitted, then A is restored and evicted). Evicting the loser then deletes the winner's live nullifier entry, so the pool forgets that a pending tx already spends that nullifier — a duplicate spend can be admitted and the fee-replacement rule bypassed. Block building revalidates nullifiers, so this stays a mempool-integrity bug, not a chain-state break.

Fix

Only delete the nullifier key when it still points at the tx being removed:

if (this.#nullifierToTxHash.get(nullifier) === meta.txHash) {
  this.#nullifierToTxHash.delete(nullifier);
}

Removal stays idempotent; an evicted loser can no longer delete a key already reassigned to the survivor.

Test

Adds a regression test in tx_pool_indices.test.ts covering the protect → admit-higher-fee → evict path: after eviction the surviving tx must remain discoverable by the shared nullifier. Verified red→green against the built base (@aztec-labs/p2p jest, 15/15 green; the new test fails without the guard), prettier --check clean, tsgo -b --emitDeclarationOnly exit 0.

Addresses the LabsBox audit finding "[aztec-node] tx_pool_v2 nullifier-conflict eviction deletes the surviving transaction's nullifier index".

🤖 Generated with Claude Code

[greptile-apps]

Confidence Score: 4/5

The behavioral fix appears correct, but the redundant test comments should be removed to satisfy the repository's explicit commenting requirement before merging.

Findings

1.  Redundant setup comments ▶

Summary

This PR prevents removal of one transaction from deleting a shared-nullifier index entry that has already been reassigned to a surviving transaction.

• Guards nullifier deletion by checking that the removed transaction still owns the mapping.
• Adds focused regression coverage for the protect, competing admission, and eviction sequence.
• The implementation preserves idempotent removal and follows the existing string-hash equality convention.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[A owns shared nullifier] --> P[A becomes protected]
    P --> B[B admitted and owns shared nullifier]
    B --> E[A is evicted]
    E --> C{Mapping still points to A?}
    C -- No --> K[Keep B's mapping]
    C -- Yes --> D[Delete mapping]

Loading

_{Reviews (1) · Last reviewed commit: "fix(p2p): guard tx-pool nullifier index ..."}

[greptile-apps]

Redundant setup comments

The comments on lines 94 and 98 only restate the adjacent addPending and updateProtection calls. This violates the repository directive to use inline comments only when they explain a non-obvious reason or constraint. Remove these comments before merging; the later explanation of the conflict scenario remains useful.

Suggested change

	// A is pending and owns the shared nullifier.
	indices.addPending(a);
	expect(indices.getTxHashByNullifier(shared)).toBe(a.txHash);
	// A is included in a proposal, so it leaves the pending nullifier index.
	indices.updateProtection(a.txHash, SlotNumber(1));
	indices.addPending(a);
	expect(indices.getTxHashByNullifier(shared)).toBe(a.txHash);
	indices.updateProtection(a.txHash, SlotNumber(1));

Context Used: yarn-project/CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[rkarabut]

Done — removed both restating comments in 9a8fba7; the conflict-scenario comments that carry the non-obvious reason stay.