bridge: improvements#575
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR optimizes Heimdall’s bridge + vote-extension execution path by reducing redundant L1 RPC calls (batch receipt prefetch + caching), speeding up rootchain event dispatch (topic→event map), and hardening several bridge handlers to fail fast on encoding/decoding errors.
Changes:
- Add L1 receipt prefetching (batch JSON-RPC) and introduce caches for finalized block headers + receipts.
- Replace per-log ABI linear scans with a prebuilt topic→event lookup; stagger task ETAs and increase retry count.
- Simplify clerk mempool checking by switching to the typed CometBFT RPC client and add an event-record existence check before broadcasting.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| helper/receipt.go | Adds PrefetchReceipts batch prefetch + cache-warming for L1 receipts. |
| helper/call.go | Introduces LRU receipt cache + finalized block header cache; adds batch receipt RPC. |
| helper/call_test.go | Adds unit tests for finalized block cache semantics and concurrency. |
| go.mod | Adds github.com/hashicorp/golang-lru/v2 as a direct dependency. |
| bridge/util/common.go | Minor cleanup (whitespace-only changes in diff). |
| bridge/processor/base.go | Replaces raw HTTP mempool fetch with typed CometBFT UnconfirmedTxs. |
| bridge/processor/clerk.go | Uses event-record existence check before broadcasting clerk events. |
| bridge/listener/rootchain.go | Adds event topic map, task staggering, and increases task retry count. |
| bridge/listener/rootchain_selfheal.go | Uses event topic map lookup and guards against empty topics. |
| bridge/listener/rootchain_log.go | Adds early returns on marshal/unpack errors; normalizes log keys. |
| bridge/listener/rootchain_listener_test.go | Adds tests around event map, stagger interval, and early-return behavior. |
| app/abci.go | Adds receipt prefetch step to ExtendVoteHandler and tx-hash extraction helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewFound 2 issues. Checked for bugs and CLAUDE.md compliance. Issue 1: Data race on taskStaggerDelay (bridge/listener/rootchain.go) taskStaggerDelay is written in queryAndBroadcastEvents (runs in the StartHeaderProcess goroutine, line 93) and read in SendTaskWithDelay via rl.taskStaggerDelay (line 269). However, SendTaskWithDelay is also called from the startSelfHealing goroutine (line 102) via processCheckpointAck, processStakeUpdate (which further fans out into goroutines with sync.WaitGroup), and processStateSynced, all of which call handleLog then SendTaskWithDelay. There is no mutex, atomic, or channel protecting this field. The Go race detector will flag this as a data race. Suggested fix: Make taskStaggerDelay a local variable in queryAndBroadcastEvents and pass it as a parameter to handleLog/SendTaskWithDelay, since the self-heal paths do not set it and would use a stale value anyway. Alternatively, use atomic.Int64 or a sync.Mutex. Per security.md: Tests pass with -race flag to detect data races. Issue 2: RetryCount change from 3 to 10 breaks existing tests (bridge/listener/rootchain.go) The change at line 265 updates signature.RetryCount from 3 to 10, but the pre-existing test file bridge/listener/rootchain_test.go still asserts the old value at line 81 (mock matcher sig.RetryCount != 3 returns false, so mock will not match) and at line 611 (require.Equal(t, 3, signature.RetryCount) will fail). Per CLAUDE.md CI Requirements: All tests pass (make test). |
|
test |
|
Inline Review Comment - Data race: |
|
Inline Review Comment -
|
|
@claude review. |
|
Claude finished @kamuikatsurgi's task in 3m 9s —— View job Code Review: bridge improvements
Found 1 issue and 3 suggestions. Previous review issues (data race, test breakage) have been addressed. Issue1.
Consider passing a reasonable limit. The function only checks for clerk limit := 1000 // reasonable upper bound
result, err := bp.httpClient.UnconfirmedTxs(ctx, &limit)Suggestions1. Stagger race between batch processing and self-heal — The data race is fixed with A cleaner approach would be to pass the stagger as a parameter to 2. Hardcoded prefetch timeout —
3. PR description says "retry count from 3 to 10" but the code uses 5 ( What looks good
|
Code reviewFound 1 issue. Checked for bugs and CLAUDE.md compliance. Issue: Stale
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (37.7%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #575 +/- ##
=========================================
+ Coverage 42.1% 42.4% +0.2%
=========================================
Files 172 172
Lines 17722 17899 +177
=========================================
+ Hits 7476 7601 +125
- Misses 9167 9210 +43
- Partials 1079 1088 +9
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review |
|
Claude finished @kamuikatsurgi's task in 5m 1s —— View job Code Review: bridge improvements
Found 2 issues and 2 suggestions. All previously reported issues have been addressed. Issues1. func (c *ContractCaller) getPrefetchMu() *sync.RWMutex {
if c.prefetchMu == nil {
c.prefetchMu = &sync.RWMutex{}
}
return c.prefetchMu
}This check-then-write has a TOCTOU race. If two goroutines call 2. Duplicate import of checkpoint types — "github.com/0xPolygon/heimdall-v2/x/checkpoint/types"
checkpointTypes "github.com/0xPolygon/heimdall-v2/x/checkpoint/types"The same package is imported twice under different aliases. This compiles (Go allows it if both names are used) but is confusing, redundant, and likely unintentional. Use one or the other consistently. Suggestions1.
limit := 1000
result, err := bp.httpClient.UnconfirmedTxs(ctx, &limit)2. Potential deadlock on panic during lock hold — If a panic occurs while Previously reported issues — StatusAll previously reported issues have been addressed:
What looks good
|
|
@claude review |
|
Claude finished @kamuikatsurgi's task in 2m 21s —— View job Code Review: bridge improvements
Found 2 issues and 2 suggestions. All previously reported issues have been addressed. Issues1. Duplicate import in new test file — "github.com/ethereum/go-ethereum/common"
ethCommon "github.com/ethereum/go-ethereum/common"The same package is imported under two names. Go technically allows this (both aliases resolve and are used: 2. caller.prefetchMu.Lock()
caller.prefetchedReceipts = receipts
caller.prefetchMu.Unlock()
However, if caller.prefetchMu.Lock()
for h, r := range receipts {
caller.prefetchedReceipts[h] = r
}
caller.prefetchMu.Unlock()Suggestions1.
2. limit := 100
result, err := bp.httpClient.UnconfirmedTxs(ctx, &limit)Good that you addressed the previous nil-limit issue (which defaulted to 30). CometBFT caps at Previously Reported Issues — StatusAll previously reported issues have been addressed:
What Looks Good
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |



Description