ballet, txn: reject v1 transactions with duplicate addresses

[topointon-jump]

technically this is unnecessary, these are caught by a runtime/replay check. but it is more consistent with agave's behaviour, which throws these out at sanitization time, and makes it easier to fuzz. it's also more in line with the SIMD.

@ptaffet-jump we should discuss on monday if we actually want to merge this

[github-actions]

 ┌─ ⚡ PERF · bdea53d vs main@de039cd ─────────────────────────────────
 │ SUITE                               BASELINE          NEW         Δ
 │ replay tps, mainnet               27,893 tps   27,799 tps  ·  -0.34%
 │ bench tps, localnet              832,868 tps  832,647 tps  ·  -0.03%
 │ snapshot load, testnet                     …            …         …
 │ mem total, mainnet                170.92 GiB   170.92 GiB  ·   0.00%
 │ mem total, testnet                101.79 GiB   101.79 GiB  ·   0.00%
+│ clean compile, firedancer        265.8 cpu·s  252.7 cpu·s  ▼  -4.91%
 │ binary size, firedancer             85.99 MB     85.99 MB  ·   0.00%
 ├─────────────────────────────────────────────────────────────────────
@@ RUNNING · replay done · snapshot pending · bench done @@
 └─────────────────────────────────────────────────────────────────────

history · 4 pushes

 ┌─ HISTORY · Δ vs main, per push, newest first ─────────────────────────
 │ HEAD         TPS    BENCH     SNAP    MEM·M    MEM·T  COMPILE   BINARY
+│ bdea53d   -0.34%   -0.03%        …    0.00%    0.00%   -4.91%    0.00%
 │ d178057   +0.16%   +0.10%   +1.57%    0.00%    0.00%   +0.16%    0.00%
 │ f8b01ca        …   +0.30%        …    0.00%    0.00%   -0.55%    0.00%
 │ 41b0806   -0.27%   -0.45%   -0.16%    0.00%    0.00%   +1.77%    0.00%
 └───────────────────────────────────────────────────────────────────────

[Copilot]

🟡 Changes recommended

The quadratic validation affects a network-facing hot path and lacks targeted regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Rejects V1 transactions containing duplicate account addresses during parsing, matching Agave sanitization behavior.

Changes:

• Adds pairwise duplicate-address validation for V1 transactions.

File summaries

File	Description
src/ballet/txn/fd_txn_parse.c	Rejects duplicate V1 account addresses.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new rejection behavior lacks a deterministic parser regression test.

Review details

Suppressed comments (1)

src/ballet/txn/fd_txn_parse.c:144

• The new duplicate-address rejection has no deterministic regression test. txn_v1_correctness already covers the other V1 rejection branches, but none constructs two equal addresses, so a reversed memcmp condition or later removal would pass the current suite. Please add a V1 parser case that duplicates two 32-byte address entries and asserts fd_txn_parse returns zero (ideally also retaining a valid distinct-address control).

        CHECK( memcmp( payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*j, payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*k, FD_TXN_ACCT_ADDR_SZ ) );

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The quadratic duplicate scan introduces attacker-controlled overhead on the pre-signature parsing path.

Review details

Suppressed comments (1)

src/ballet/txn/fd_txn_parse.c:145

• This pairwise scan adds a worst-case 2,016 fixed-size comparisons to the pre-signature parsing path for every valid 64-account V1 transaction. That path is directly reachable from untrusted TPU input (src/disco/verify/fd_verify_tile.c:116), so a sender can force this cost repeatedly; the existing duplicate checker is specifically optimized because this check is critical-path (src/disco/pack/fd_chkdup.h:7-19). Please use an O(n) bounded hash/SIMD duplicate check (likely by moving or sharing the existing checker at an appropriate layer) rather than quadratic memcmp scans.

    for( ulong j=0UL; j<(ulong)acct_addr_cnt; j++ ) {
      for( ulong k=j+1UL; k<(ulong)acct_addr_cnt; k++ ) {
        CHECK( memcmp( payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*j, payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*k, FD_TXN_ACCT_ADDR_SZ ) );
      }

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The duplicate check introduces quadratic work in a network-facing transaction parser.

Review details

Suppressed comments (1)

src/ballet/txn/fd_txn_parse.c:144

• This pairwise scan adds 2,016 memcmp calls to every valid 64-account V1 transaction, and fd_txn_parse runs directly on incoming transactions in the verify tile (src/disco/verify/fd_verify_tile.c:116). That makes parsing quadratic in an attacker-controlled account count and can consume substantially more work than the packet size warrants. Please use a bounded hash set with full-key collision checks (the repository already has this pattern in fd_txn_build.h:39-49) or another sub-quadratic duplicate check.

    for( ulong j=0UL; j<(ulong)acct_addr_cnt; j++ ) {
      for( ulong k=j+1UL; k<(ulong)acct_addr_cnt; k++ ) {
        CHECK( memcmp( payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*j, payload+acct_addr_off+FD_TXN_ACCT_ADDR_SZ*k, FD_TXN_ACCT_ADDR_SZ ) );

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced