@claude review this PR. Check for any breaking / risky changes from an implementation / usage point of view.

Claude finished @manav2401's task in 2m 4s —— View job

PR Review: Relay Improvements

• Read all changed files
• Analyze concurrency and lock safety
• Check for breaking changes (metrics, APIs, behavior)
• Analyze error handling and edge cases
• Post detailed review findings

Summary

Solid improvement to private tx lifecycle management. The sweep mechanism with TTL + txpool-awareness directly addresses the unbounded growth problem. The rejection tracker is well-designed with proper cardinality capping. Good test coverage across all new code paths.

I found one concurrency issue worth fixing before merge, a few items worth discussing, and a handful of minor observations.

Findings

1. [HIGH] sweepOnce holds store mutex while calling external txPoolChecker

File: eth/relay/private_tx_store.go:167-186

sweepOnce() holds s.mu.Lock() for the entire iteration, including the call to s.txPoolChecker(hash) at line 179. This checker is wired to eth.txPool.Has() (via eth/backend.go:407), which acquires the txpool's internal lock.

This creates a lock ordering dependency: store.mu → txpool.lock. If any txpool code path ever calls back into the private tx store (e.g., checking IsTxPrivate which acquires store.mu.RLock), you get a deadlock. Even without a full deadlock today, holding the store lock while calling into the txpool blocks all Add/Purge/IsTxPrivate callers for the duration of the iteration — which scales with store size.

Recommended fix: Snapshot entries under lock, release, check txpool without the lock, then re-lock to delete:

func (s *PrivateTxStore) sweepOnce() {
    // Snapshot under read lock
    s.mu.RLock()
    type entry struct {
        hash    common.Hash
        addedAt time.Time
    }
    entries := make([]entry, 0, len(s.txs))
    for h, t := range s.txs {
        entries = append(entries, entry{h, t})
    }
    s.mu.RUnlock()

    // Classify without holding the lock
    now := time.Now()
    toDelete := make([]common.Hash, 0)
    for _, e := range entries {
        age := now.Sub(e.addedAt)
        if age > privateTxTTL {
            toDelete = append(toDelete, e.hash)
        } else if age > privateTxGracePeriod && s.txPoolChecker != nil && !s.txPoolChecker(e.hash) {
            toDelete = append(toDelete, e.hash)
        }
    }

    // Delete under write lock, verifying timestamps haven't changed
    if len(toDelete) == 0 {
        return
    }
    expired := uint64(0)
    s.mu.Lock()
    for _, h := range toDelete {
        // Only delete if the entry wasn't re-added between snapshot and now
        if _, ok := s.txs[h]; ok {
            delete(s.txs, h)
            expired++
        }
    }
    s.mu.Unlock()
    s.txsExpired.Add(expired)
}

Fix this →

2. [MEDIUM] Rejection tracker records "already known" errors before the early return filters them

File: eth/relay/multiclient.go:141-148, 212-214, 300-305

In all three submission paths (submitPreconfTx, submitPrivateTx, retryPrivateTxSubmission), mc.rejectionTracker.record(err) is called before the isAlreadyKnownError(err) check. This means "already known" responses — which are explicitly treated as successes — get counted in the rejection summary.

The tests in TestRejectionTrackerWiredToSubmit (lines 1558-1631) actually verify that already-known is excluded, and they pass. Looking more carefully, this works because the test asserts total == 1 when one BP returns a real error and one returns "already known". But wait — both errors hit record(). Let me re-read...

Actually, after re-examining: the test at line 1577 asserts total == 1, but record() is called for both errors. The "already known" error would normalize to "already known" and be counted. So either:

• The test is wrong (passes by coincidence due to timing), or
• I'm misreading the flow

Looking again at submitPrivateTx line 204-214: record(err) is at line 208, then isAlreadyKnownError check is at line 210. Both run under the same mu.Lock(), so there's no race. The record definitely fires for "already known" errors.

The test at line 1577 should fail with total == 2, not total == 1. If it's passing, the mock might not be triggering the code path as expected. Either way, the intent (per the docstring on record() at line 29: "Callers typically filter out 'already known' error type") doesn't match the implementation.

Fix: Move record(err) after the isAlreadyKnownError check in all three locations, or guard it:

if err != nil {
    privateTxRpcFailureMeter.Mark(1)
    if isAlreadyKnownError(err) {
        privateTxRpcAlreadyKnownMeter.Mark(1)
        successfulIndices = append(successfulIndices, index)
        return
    }
    mc.rejectionTracker.record(err)  // moved after already-known check
    ...
}

Fix this →

Update: I re-read the test more carefully. In TestRejectionTrackerWiredToSubmit "private tx rejection is tracked, already-known is not" (line 1558), server s1 returns a fee cap error and s2 returns "already known". The submitPrivateTx call at line 1574 submits to both. In the goroutine for s2, record(already known) IS called at line 208, then the early return fires at line 210-214. So the tracker receives TWO record() calls: one for the fee error, one for "already known". The assertion at line 1577 (total == 1) should fail. If this test is passing in CI, there may be a timing issue or the test harness isn't capturing both calls. This needs investigation.

3. [MEDIUM-LOW] reportRejections logs even when there are zero rejections

File: eth/relay/multiclient.go:391-407

The reporter logs every minute regardless of whether any rejections occurred:

log.Info("[tx-relay] BP rejection summary", "total", total, "errors", formatRejectionCounts(counts))

On a healthy relay this produces ~1440 empty log lines per day. Consider adding a total > 0 guard:

if total > 0 {
    log.Info(...)
}

4. [LOW] Metric names changed — breaking for existing dashboards

File: eth/relay/service.go, eth/relay/multiclient.go

All metric names have been renamed (e.g., preconfs/submit → relay/preconf/submit/latency, preconfs/rpc/success → relay/bp/rpc/preconf/success, etc.). The old totalPrivateTxsMeter (Meter type) was replaced with privateTxStoreSizeGauge (Gauge type) — also a type change.

This is intentional and an improvement in naming, but any existing Grafana dashboards or alerting rules referencing the old metric names will silently stop receiving data. Not a code bug — just an operational awareness item for the deploy.

5. [LOW] SetTxPoolChecker uses write lock but sweepOnce reads txPoolChecker without synchronizing on the same field

File: eth/relay/private_tx_store.go:141-146 vs 179

SetTxPoolChecker acquires s.mu.Lock() to set s.txPoolChecker. sweepOnce also acquires s.mu.Lock() and reads s.txPoolChecker. Since both hold the same mutex, this is actually safe as written. However, if the sweep is refactored to the snapshot pattern (recommendation #1), the txPoolChecker read would be outside the lock. In that case, you'd want to snapshot the checker reference under the read lock too:

s.mu.RLock()
checker := s.txPoolChecker
// ... snapshot entries ...
s.mu.RUnlock()

6. [LOW] normalizeRejectionMessage treats leading colon as "no useful prefix"

File: eth/relay/rejection_tracker.go:56

if i := strings.IndexByte(msg, ':'); i > 0 {

Using i > 0 means :foo returns :foo unchanged (the test at line 1515 confirms this). This is fine for current error formats, but if a colon-prefixed error ever appears it would bypass normalization. Not a real risk given Go's error conventions — just noting the edge case is covered by tests.

Non-Issues (Verified Safe)

• Map deletion during iteration in sweepOnce: Go spec explicitly allows deleting map entries during range — safe.
• closeOnce added to multiClient.close(): Good addition — prevents double-close panics on reporterDone channel.
• Atomic counters for metrics in PrivateTxStore: Correct use of atomic.Uint64 — no races with the periodic Store(0) reset since the report goroutine is the only writer of resets.
• txPool.Has is safe to call concurrently: The Has method on TxPool acquires its own internal lock — no external synchronization needed at the call site (though the concern is about holding another lock while calling it, per finding MAT-156 Span sync #1).

Breaking / Risky Changes Assessment

Overall: No breaking API changes. The behavioral change (not purging on rejection, relying on TTL/sweep instead) is well-reasoned and properly documented. The main item to fix before merge is the lock-holding pattern in sweepOnce.

Code review

1 issue found. Checked for bugs and CLAUDE.md compliance.

Bug in eth/relay/multiclient.go line 148

mc.rejectionTracker.record(err) is called before the isAlreadyKnownError(err) check in all three call sites (submitPreconfTx line 143, submitPrivateTx line 208, retryPrivateTxSubmission line 302). This means "already known" errors which are intentionally treated as successful submissions are incorrectly counted as rejections in the tracker.

This contradicts the record() docstring: "Callers typically filter out already-known error type while other RPC related errors reach here."

The tests in TestRejectionTrackerWiredToSubmit will fail because total will be 2 (not the expected 1), and counts will contain "already known" despite require.NotContains.

The same ordering issue exists at these two other locations:

• multiclient.go L206-213
• multiclient.go L300-307

Suggested fix (for the first call site at lines 143-148):

				// If the tx is already known, treat it as preconfirmed for this node
				if isAlreadyKnownError(err) {
					preconfRpcAlreadyKnownMeter.Mark(1)
					preconfOfferedCount.Add(1)
					return
				}
				mc.rejectionTracker.record(err)

Accepting the suggestion above fixes only this first call site. The same fix (moving record(err) below the isAlreadyKnownError check + early return) must also be applied at lines 208 and 302.

@claude
Re. your suggestions,

• 1 is addressed
• 2 is kept intentionally - I would like to track "already known" errors as well even though they lead to a successful submission.
• 3 to 6 are not needed and changes are intentional.

Claude finished @manav2401's task in 45s —— View job

Acknowledging Review Feedback

• Read author's response to review findings
• Verify finding MAT-156 Span sync #1 (sweepOnce lock) is addressed
• Check if finding build(deps): bump mixin-deep from 1.3.1 to 1.3.2 in /dashboard/assets #2 tests align with intentional behavior
• Post summary

Thanks for the follow-up. Verified the current state:

Finding #1 — sweepOnce lock: Confirmed fixed. The three-phase pattern (RLock snapshot → filter without lock → Lock to delete) at private_tx_store.go:173-216 is clean. The re-add guard at line 210 (cur.Equal(entry.addedAt)) correctly prevents deleting entries that were re-added between snapshot and delete. The explanatory comment at lines 167-172 is helpful too.

Finding #2 — "already known" in rejection tracker: Understood, intentionally tracked. The tests at lines 1558-1604 now explicitly assert total == 2 and verify counts["already known"] is present, which is consistent with the implementation. No issue here.

Findings #3-#6: Acknowledged as intentional.

LGTM — no remaining concerns.

View job

Codecov Report

❌ Patch coverage is 88.27160% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.23%. Comparing base (ad2d891) to head (6110b9a).
⚠️ Report is 1 commits behind head on master.

❌ Your patch check has failed because the patch coverage (88.27%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

@@            Coverage Diff             @@
##           master    #2205      +/-   ##
==========================================
+ Coverage   52.21%   52.23%   +0.01%     
==========================================
  Files         884      885       +1     
  Lines      155479   155616     +137     
==========================================
+ Hits        81190    81281      +91     
- Misses      69060    69099      +39     
- Partials     5229     5236       +7     

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud