Skip to content

feat: aggkit-proxy bridgeServiceFinder AllowNetworkIDs whitelist 1909 - #1923

Merged
joanestebanr merged 4 commits into
developfrom
feat/1909-bridgeservicefinder-allowlist
Oct 26, 2026
Merged

joanestebanr merged 4 commits into
developfrom
feat/1909-bridgeservicefinder-allowlist

Conversation

@joanestebanr

Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • Add BridgeServiceFinder.AllowNetworkIDs, a whitelist of permitted networks. When non-empty, only the listed network IDs are resolved; every other network is excluded exactly as if it were in IgnoreNetworkIDs (skipped at enumeration and live discovery, GetURL returns ErrNetworkDisabled, never recorded as pending even with AutoRegisterNewNetworks = false).
  • It still combines with AutoRegisterNewNetworks, so a network ID can be listed before the network is attached to the rollup manager.
  • IgnoreNetworkIDs and AllowNetworkIDs are mutually exclusive: New returns ErrConflictingNetworkFilters if both are set, so the proxy fails at startup.
  • Internally, the ignore map is replaced by a small networkFilter shared by the finder and the listener.

⚠️ Breaking Changes

  • 🛠️ Config: None. The new field is optional and defaults to [] (no allowlist). The only new failure is configuring both lists at once.
  • 🔌 API/CLI: None.
  • 🗑️ Deprecated Features: None.

📋 Config Updates

  • 🧾 Diff/Config snippet:
[BridgeServiceFinder]
# Only these networks are served. Mutually exclusive with IgnoreNetworkIDs.
# Include 0 to keep serving the L1 bridge service.
AllowNetworkIDs = [0, 1, 2]

🔌 API Updates

🔌 Bridge service API

  • No changes.

🔌 Proxy API

  • No endpoints modified, added or removed. Not a breaking interface change.

🔌 Others API

  • No changes.

✅ Testing

  • 🤖 Automatic: new unit tests in bridgeservicefinder/allow_network_ids_test.go: conflict with IgnoreNetworkIDs, filter logic, enumeration at Start, and live discovery with AutoRegisterNewNetworks on and off. proxy/config default test extended.
  • 🖱️ Manual: set AllowNetworkIDs in the aggkit-proxy config and check that GET for a non-listed network returns not found, and that setting both lists makes the proxy fail at startup.

🐞 Issues

🔗 Related PRs

  • None

📝 Notes

  • Network 0 (L1) is a regular entry, so it must be listed to keep being served from BridgeURLs; this is consistent with IgnoreNetworkIDs.
  • TestStart_UnreachableService_RequireAllHealthyOnStart{True,False} fail on develop without these changes too.

joanestebanr and others added 2 commits October 26, 2026 08:45
Add BridgeServiceFinder.AllowNetworkIDs: when non-empty, only the listed
networks are resolved and every other one is excluded exactly like an
IgnoreNetworkIDs entry (skipped at enumeration and live discovery,
ErrNetworkDisabled from GetURL, never pending). It combines with
AutoRegisterNewNetworks so a network ID can be listed before the network
is attached to the rollup manager.

IgnoreNetworkIDs and AllowNetworkIDs are mutually exclusive: New returns
ErrConflictingNetworkFilters if both are set.

Fixes #1909

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T09:09:05.240590Z d229d6f PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@joanestebanr joanestebanr self-assigned this Oct 26, 2026
@joanestebanr

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Claude finished @joanestebanr's task in 36s —— View job


Review of PR #1923: AllowNetworkIDs whitelist for bridgeservicefinder

I read the full diff and the new tests. I did not run lint or tests. I used only static analysis, so the lint finding below comes from reading the code and line lengths.

The change is small and clean. Replacing the ignore map with a shared networkFilter is a good refactor. Every former ignoreNetworkIDs lookup now goes through excluded(), and no stale references remain (I grepped for ignoreNetworkIDs and buildIgnoreSet). Excluded networks stay excluded on every path I checked:

  • Enumeration in buildInitialCache.
  • probeAll.
  • GetURL and NetworkIDs.
  • discoverRollup, which runs before the auto-register and pending gate.
  • The metadata-clear re-resolve path in listener.go. It only fires for watched addresses, and excluded networks are never watched.

🔴 Required

None.

🟡 Suggested

  1. lll lint will likely fail. The follow-up commit "wrap lines over 120 chars" missed two lines this PR changed. lll is enabled in .golangci.yml, and CLAUDE.md sets a 120-character limit.

    • bridgeservicefinder/listener.go:472, the discoverRollup doc comment, is about 135 characters: // first and always wins, regardless of Config.AutoRegisterNewNetworks: an excluded network is a deliberate operator decision, not a.
    • bridgeservicefinder/bridgeservicefinder.go:428, the GetURL doc comment, is about 121 characters: // ErrNetworkDisabled if networkID is excluded by Config.IgnoreNetworkIDs / Config.AllowNetworkIDs, or ErrURLNotFound if.

    Please rewrap both. Running make lint will confirm.

  2. The allowed-network case with auto-register off is not asserted in TestLiveDiscovery_AllowNetworkIDs. The autoRegister=false subtest only checks that the blocked network is not pending. It never checks that the allowed network (ID 2) is pending. Add something like require.Contains on the network IDs in PendingNetworks() for that case. That would show the allowlist does not suppress the normal pending flow for permitted networks. It would also catch a regression where excluded and allowed networks behave the same.

  3. Redundant filter validation in newListener (listener.go:127). New already builds and validates the filter, then buildInitialCache calls newListener(..., cfg), which rebuilds it and returns a second, unreachable ErrConflictingNetworkFilters path. Consider passing the already-built networkFilter into newListener. That avoids building it twice and removes the dead error branch. It is minor, but it keeps one source of truth.

🟢 Optional

  • ErrNetworkDisabled's message changed from "listed in IgnoreNetworkIDs" to "excluded by IgnoreNetworkIDs/AllowNetworkIDs". I found no string matches on the old text in the repo, so this is fine. The change is still visible in proxy logs or API error bodies, if any surface it.
  • In TestStart_AllowNetworkIDs_OnlyAllowedAreResolved, the rollups[id-1] indexing relies on rollup IDs being 1-based. A short comment or a map lookup would make that clearer.
  • The live-discovery test relies on sleepPastSeedTick. That matches the existing tests, so it is acceptable. Just be aware of the flakiness risk on slow CI.
  • The PR notes say TestStart_UnreachableService_RequireAllHealthyOnStart{True,False} already fail on develop. A tracking issue would help, so they are not mistaken for regressions from this PR.

PR summary

🚀 What's New

Adds BridgeServiceFinder.AllowNetworkIDs, a whitelist of network IDs. When it is non-empty, only the listed networks are resolved. Every other network is treated as if it were in IgnoreNetworkIDs. That means it is skipped at enumeration and live discovery, GetURL returns ErrNetworkDisabled, and it is never recorded as pending, even with AutoRegisterNewNetworks = false. The allowlist still combines with AutoRegisterNewNetworks, so an ID can be listed before the network is attached to the rollup manager. IgnoreNetworkIDs and AllowNetworkIDs are mutually exclusive: New returns ErrConflictingNetworkFilters if both are set. Internally, the ignore map is replaced by a shared networkFilter used by the finder and the listener. Docs, defaults and tests are updated.

🐛 Bug Fixes

None.

📋 Config Updates

New optional key, default [] (no allowlist):

[BridgeServiceFinder]
# Only these networks are served. Mutually exclusive with IgnoreNetworkIDs.
# Include 0 to keep serving the L1 bridge service.
AllowNetworkIDs = [0, 1, 2]

⚠️ Breaking Changes

None. The only new failure is configuring both IgnoreNetworkIDs and AllowNetworkIDs, and that is a new combination.

Verdict

Looks good. I recommend fixing the two over-length lines before merge, since make lint would likely flag them. The other items are optional.
· branch feat/1909-bridgeservicefinder-allowlist

…chars

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
arnaubennassar
arnaubennassar previously approved these changes Oct 26, 2026
@joanestebanr
joanestebanr enabled auto-merge (squash) October 26, 2026 19:55
@joanestebanr
joanestebanr merged commit 574a9b8 into develop Oct 26, 2026
33 checks passed
@joanestebanr
joanestebanr deleted the feat/1909-bridgeservicefinder-allowlist branch October 26, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: aggkit-proxy: bridgeServiceFinder whitelist

2 participants