Codecov Report

❌ Patch coverage is 93.70460% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.57%. Comparing base (62eaa27) to head (a156381).
⚠️ Report is 1 commits behind head on develop.

@@             Coverage Diff             @@
##           develop    #2004      +/-   ##
===========================================
+ Coverage    50.36%   50.57%   +0.21%     
===========================================
  Files          871      875       +4     
  Lines       150924   151690     +766     
===========================================
+ Hits         76009    76717     +708     
- Misses       69852    69902      +50     
- Partials      5063     5071       +8     

Code Review

Found 3 issues during code review.

Issues

1. Resource leak in eth/relay/multiclient.go:52-55 - When rpc.Dial() succeeds but eth_blockNumber call fails, the RPC client is not closed before continuing to the next URL, causing a connection leak.

2. CLAUDE.md violation in eth/relay/multiclient.go:58 - Error from hexutil.DecodeUint64 is ignored with _, violating the CLAUDE.md rule: "Handle Errors: Never ignore errors with _"

3. Missing nil check in eth/relay/service.go:149 - CheckTxPreconfStatus calls s.multiclient.checkTxStatus(hash) without checking if s.multiclient is nil, inconsistent with the defensive programming pattern used in SubmitTransactionForPreconf and SubmitPrivateTx.

Checked for bugs and CLAUDE.md compliance.

A question about a hypothetical scenario - if somehow a node receives a transaction but got killed before it broadcasts the txn, that txn will get lost. It seems like a hard edge case where we need to persist the txn data on disk?

Great point. Yeah, I've kept a note of this to add persistence for storing transactions / preconf tasks. Due to the Amoy rollout next week, I am planning to add this in next iteration.

Although, I've discussed the approach to use when restarting bor w/ Devops. It doesn't require persistence and can still ensure that no tx submission requests are dropped. Thanks!

Code Review

Found 4 issues during code review.

Issue 1: Missing nil check in CheckTxPreconfStatus

File: eth/relay/service.go:184

The CheckTxPreconfStatus method calls s.multiclient.checkTxStatus(hash) without checking if s.multiclient is nil first. This will cause a nil pointer panic when multiclient is nil (which can happen if no block producer URLs are provided or all connections fail).

Other methods like SubmitTransactionForPreconf (line 88) and SubmitPrivateTx (line 118) correctly check for s.multiclient == nil before using it.

Fix: Add a nil check before line 184:

if s.multiclient == nil {
    return false, errRpcClientUnavailable
}

Reference: CLAUDE.md - Common Pitfalls: "Nil Checks: Check interface values and pointers before use"

Issue 2: Inconsistent preconf status check logic

File: eth/relay/multiclient.go:275-276

In checkTxStatus, only TxStatusPending is counted as preconfirmed:

if txStatus == txpool.TxStatusPending {
    preconfOfferedCount.Add(1)
}

However, in internal/ethapi/api.go SendRawTransactionForPreconf (lines 2263-2267), both TxStatusPending and TxStatusIncluded are treated as valid preconfirmations:

if txStatus == txpool.TxStatusPending || txStatus == txpool.TxStatusIncluded {
    txConfirmed = true
}

This inconsistency means that if a transaction is already included in a block, the block producer's endpoint reports it as preconfirmed, but the relay's status check would report it as NOT preconfirmed.

Fix: Update line 275 in multiclient.go to also check for TxStatusIncluded:

if txStatus == txpool.TxStatusPending || txStatus == txpool.TxStatusIncluded {
    preconfOfferedCount.Add(1)
}

Issue 3: Race condition in test - unprotected map access

File: eth/handler_eth_test.go:821-829 and eth/handler_eth_test.go:923

The PrivateTxStore struct uses an unprotected map[common.Hash]struct{}:

type PrivateTxStore struct {
    store map[common.Hash]struct{}
}

This map is:

• Written to from the main goroutine (lines 869-870, 940-941)
• Read from multiple concurrent goroutines (lines 923, 953)

While there is a wg.Wait() between some writes and reads, the Go memory model requires explicit synchronization (not just program order) to establish happens-before relationships between goroutines.

Fix: Add a sync.RWMutex to protect map access:

type PrivateTxStore struct {
    mu    sync.RWMutex
    store map[common.Hash]struct{}
}

func (p *PrivateTxStore) IsTxPrivate(hash common.Hash) bool {
    p.mu.RLock()
    defer p.mu.RUnlock()
    _, ok := p.store[hash]
    return ok
}

Reference: CLAUDE.md - Common Pitfalls: "Race Conditions: Use -race flag, protect shared state"

Issue 4: Resource accumulation due to defer in loop

File: eth/handler_eth_test.go:757-765 and eth/handler_eth_test.go:881-889

Both testDisableTxPropagation and testPrivateTxNotPropagated use defer statements inside a for loop:

for i, sink := range sinks {
    sourcePipe, sinkPipe := p2p.MsgPipe()
    defer sourcePipe.Close()
    defer sinkPipe.Close()
    // ...
    defer sourcePeer.Close()
    defer sinkPeer.Close()
    // ...
}

Deferred functions execute when the function returns, not when the loop iteration ends. With 10 iterations, all 40 resources remain open throughout the entire test until the function exits, which can cause resource exhaustion.

Fix: Either close resources explicitly at the end of each iteration, or wrap the loop body in an anonymous function:

for i, sink := range sinks {
    func() {
        sourcePipe, sinkPipe := p2p.MsgPipe()
        defer sourcePipe.Close()
        defer sinkPipe.Close()
        // ...
    }()
}

Reference: CLAUDE.md - Common Pitfalls: "Close Resources: Channels, files, DB iterators, HTTP bodies"

@claude, can you review this PR, focusing on safety and performance? Please add a comment summarising any concerns about these changes.

Claude finished @kamuikatsurgi's task in 1m 53s —— View job

Safety & Performance Review

• Read core relay service files
• Analyze multiclient implementation
• Review RPC handler changes
• Check test coverage adequacy
• Summarize safety and performance concerns

Summary

This PR introduces a well-architected relay system for preconfirmations and private transactions. I've reviewed the implementation with a focus on safety and performance. Below are my findings.

Safety Concerns

1. time.Sleep in RPC Handler (Medium)

Location: internal/ethapi/api.go:2250

time.Sleep(100 * time.Millisecond)

The SendRawTransactionForPreconf endpoint uses a fixed 100ms sleep to wait for tx pool status. This blocks the RPC handler goroutine and under high load could:

• Exhaust connection pools
• Cause cascading delays for other requests
• Increase RPC response latency significantly

Recommendation: Consider a subscription-based approach or a poll loop with context-aware timeout.

2. Background Retry Goroutines Not Cancellable (Low-Medium)

Location: eth/relay/multiclient.go:215

go mc.retryPrivateTxSubmission(hexTx, hash, failedIndices, txGetter)

The background retry goroutine checks mc.closed.Load() at the start of each retry iteration, but:

• If a retry is sleeping during time.Sleep(privateTxRetryInterval), it won't be interrupted
• During graceful shutdown, these goroutines may continue for up to 2 seconds per retry

The close() method sets mc.closed and closes RPC clients, but active retry goroutines will complete their current iteration before checking the flag.

Mitigations in place: Limited to 5 retries with 2s intervals (max ~10s delay).

Recommendation: Pass a cancellation context from the service that can be cancelled during shutdown.

3. Unbounded Goroutine Spawning in Retry Loop (Low)

Location: eth/relay/multiclient.go:256-279

Each retry iteration spawns N goroutines (one per failed block producer). While bounded by:

• Number of BPs (typically small)
• Max 5 retries
• 2s retry interval

A burst of failed private tx submissions could temporarily accumulate retry goroutines. This is largely mitigated by the constraints above.

4. Map Iteration During Cleanup Holds Full Lock (Low)

Location: eth/relay/service.go:309-317

s.storeMu.Lock()
for hash, task := range s.store {
    if now.Sub(task.insertedAt) > s.config.expiryInterval {
        delete(s.store, hash)
    }
}
s.storeMu.Unlock()

The cleanup routine holds a full lock while iterating the entire store map. With 40k max queued tasks and 10-minute expiry, the map could grow large.

Mitigations in place: Only runs every minute, and the critical section is just delete operations.

Performance Considerations

1. Semaphore Pattern for Rate Limiting (Good ✓)

Location: eth/relay/service.go:142-153

The semaphore pattern for limiting concurrent tasks is well-implemented:

select {
case s.semaphore <- struct{}{}:
case <-s.closeCh:
    return
}
s.wg.Add(1)
go func(task TxTask) {
    defer s.wg.Done()
    defer func() { <-s.semaphore }()
    s.processPreconfTask(task)
}(task)

Default of 1024 concurrent tasks is reasonable.

2. RPC Timeouts (Good ✓)

Location: eth/relay/multiclient.go:19-21

rpcTimeout             = 2 * time.Second
privateTxRetryInterval = 2 * time.Second
privateTxMaxRetries    = 5

2-second RPC timeouts prevent hanging on unresponsive block producers.

3. Graceful Shutdown (Good ✓)

Location: eth/relay/service.go:327-334

func (s *Service) close() {
    close(s.closeCh)
    if s.multiclient != nil {
        s.multiclient.close()
    }
    s.wg.Wait()
}

The service properly waits for all goroutines to finish using sync.WaitGroup.

4. Private Tx Filtering in Broadcast (Good ✓)

Location: eth/handler.go:766-770

if h.privateTxGetter != nil && h.privateTxGetter.IsTxPrivate(tx.Hash()) {
    log.Debug("[tx-relay] skip tx broadcast for private tx", "hash", tx.Hash())
    continue
}

Private transactions are correctly filtered from P2P broadcast with proper nil check.

5. Cache Lock Contention Under High Load (Medium)

Location: eth/relay/service.go:190-191

s.storeMu.Lock()
defer s.storeMu.Unlock()

The updateTaskInCache function holds a full lock for the entire update operation. Under high concurrency, this serializes all cache writes.

Note: Read operations use RLock() correctly, so concurrent reads are not blocked.

Test Coverage

The test suite is comprehensive with good coverage of:

• ✓ Queue overflow scenarios
• ✓ Concurrent task processing
• ✓ Cache update race conditions
• ✓ Private transaction propagation blocking
• ✓ "Already known" error handling
• ✓ Retry logic with TxGetter
• ✓ Nil safety checks

The codecov report shows 93.70% patch coverage, which is solid.

Previously Addressed Issues ✓

Issues from the previous Claude review (2026-01-28) have been addressed:

• Nil check for multiclient in CheckTxPreconfStatus
• Preconf status check logic consistency
• Race condition in test fixed with sync.RWMutex

Verdict

The implementation is well-architected with appropriate safety mechanisms. The main areas to monitor in production:

1. The 100ms sleep in SendRawTransactionForPreconf under high load
2. Cache lock contention if preconf task volume grows significantly
3. Background retry goroutines during shutdown (minor, bounded delay)

These are not blocking issues for the current scope.

@claude can you review this PR, focusing on safety and performance? focus more on commits done after the last review (last few ones). Provide a summary as a comment highlighting any concerns related to these changes.

Claude finished @manav2401's task in 1m 53s —— View job

Safety & Performance Review

• Identify commits since last review (2026-02-03)
• Read changed files from recent commits
• Analyze safety concerns (race conditions, nil checks, error handling)
• Analyze performance concerns (goroutines, allocations, blocking)
• Provide summary with concerns

Summary

I've reviewed the changes since the last review (2026-02-03), focusing on the following key commits:

• f2ea747 - eth/relay: address blocking semaphore
• b60496e - eth/relay: wait for go routines to exit for clean shutdown
• 57b709c - internal/ethapi, eth/relay: address security comments
• 2e48d32 - eth/relay, internal/ethapi: handle private tx via new rpc endpoint
• 950fe67 - eth/relay: add more tests for relay
• 7e605c6, dd790dd, a8e3186 - Test coverage additions

The recent changes demonstrate strong attention to reliability and safety. Below are my findings.

Improvements Since Last Review ✓

1. Graceful Shutdown (Good ✓)

Commits: b60496e, f2ea747

The service now properly tracks all goroutines and waits for them during shutdown:

// eth/relay/service.go:91-94
s.wg.Add(2)
go func() { defer s.wg.Done(); s.processPreconfTasks() }()
go func() { defer s.wg.Done(); s.cleanup() }()

And each spawned task goroutine is tracked:

// eth/relay/service.go:148-152
s.wg.Add(1)
go func(task TxTask) {
    defer s.wg.Done()
    defer func() { <-s.semaphore }()
    s.processPreconfTask(task)
}(task)

Important: The close() method now waits for all goroutines:

// eth/relay/service.go:327-334
func (s *Service) close() {
    close(s.closeCh)
    if s.multiclient != nil {
        s.multiclient.close()
    }
    s.wg.Wait()  // <-- Ensures clean shutdown
}

2. Non-Blocking Semaphore Acquisition (Good ✓)

Commit: f2ea747

The semaphore acquisition is now context-aware and won't block during shutdown:

// eth/relay/service.go:142-147
select {
case s.semaphore <- struct{}{}:
case <-s.closeCh:
    log.Info("[tx-relay] Stopping preconf task processing, service closing")
    return
}

3. Background Retry Cancellation (Good ✓)

The multiClient.closed flag is now checked at the start of each retry iteration:

// eth/relay/multiclient.go:226-229
for retry := 0; retry < privateTxMaxRetries; retry++ {
    if mc.closed.Load() {
        return
    }
    // ...
}

And retry goroutines are not spawned if the client is closing:

// eth/relay/multiclient.go:211
if retry && !mc.closed.Load() {

4. Private Tx Store Cleanup Respects Shutdown (Good ✓)

Location: eth/relay/private_tx_store.go:81-86

The cleanup loop now checks for shutdown signal instead of unconditionally sleeping:

select {
case <-s.closeCh:
    return
case <-time.After(time.Second):
}

5. Error Handling Consistency (Good ✓)

The firstErr pattern with sync.Once is consistently used across submitPreconfTx, submitPrivateTx, and checkTxStatus to capture only the first error:

setError.Do(func() {
    firstErr = err
})

6. Comprehensive Test Coverage (Good ✓)

New tests added:

• TestRelayMethodWiring - verifies API backend delegation
• TestRelayGracefulShutdownOnStop - verifies shutdown completes promptly
• TestSendRawTransactionForPreconf / TestSendRawTransactionPrivate - RPC endpoint tests

Remaining Concerns

1. time.Sleep in RPC Handler (Medium - Unchanged)

Location: internal/ethapi/api.go:2250

time.Sleep(100 * time.Millisecond)

The fixed 100ms sleep in SendRawTransactionForPreconf remains. Under high load, this blocks RPC handler goroutines. The TODO comment acknowledges this needs improvement.

2. Background Retry Sleep Not Interruptible (Low)

Location: eth/relay/multiclient.go:237

time.Sleep(privateTxRetryInterval)

If shutdown occurs during the time.Sleep(), the goroutine will continue sleeping for up to 2 seconds before checking mc.closed. This is bounded and minor, but could be improved with a select on a done channel.

3. Private Tx Endpoint Access Control (Informational)

Location: internal/ethapi/api.go:2268

if !api.b.AcceptPrivateTxs() && !api.b.PrivateTxEnabled() {
    return common.Hash{}, errors.New("private transactions are not accepted")
}

The SendRawTransactionPrivate endpoint is accessible when either AcceptPrivateTxs or PrivateTxEnabled is true. This is intentional (allows relay nodes to also accept private txs), but worth noting for security consideration.

Verdict

The recent commits have significantly improved the implementation, particularly around graceful shutdown and goroutine lifecycle management. The blocking semaphore issue has been addressed, and proper WaitGroup tracking ensures clean shutdown.

The codebase is in good shape for the current scope. The main item to address in future iterations:

1. Replace the 100ms sleep with a subscription-based approach

Quality Gate failed

Failed conditions
14.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Will addresses of "public" relays be annonced anywhere?