Skip to content

fix(bridgetracker): redact error URLs + gate bridgeservicefinder auto-registration - #1862

Merged
arnaubennassar merged 13 commits into
developfrom
fix/proxy-redact-errors-autoregister-flag
Sep 23, 2026
Merged

arnaubennassar merged 13 commits into
developfrom
fix/proxy-redact-errors-autoregister-flag

Conversation

@arnaubennassar

Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • bug: proxy: redact URL from reported errors #1845: The bridge tracker / aggkit-proxy binary was echoing raw backend error strings (URLs, host:port, bare IPs — sometimes with API keys in the path) into client-facing fields (ErrorStep.Description, ErrorData.Message, CertificateData.Error, the activity endpoint's errors/warnings[].message, and proxy's own JSON error body). All of these are now redacted before they leave the process, with a defensive MarshalJSON layer as a last line of defense.
  • feat: proxy / bridgeservicefinder: Add config flag to control automatic registration of newly discovered networks in bridgeservicefinder #1855: bridgeservicefinder used to serve any network discovered after Start immediately, so half-configured rollups could be used prematurely. A new AutoRegisterNewNetworks flag (default true = today's behavior) lets operators freeze the served set at Start; networks discovered afterwards are recorded as pending instead, until a restart or an explicit config change.

⚠️ Breaking Changes

  • No behavior change at default settings for either issue.
  • 🔌 API/CLI: bridgeservicefinder.Finder gains a PendingNetworks() []PendingNetwork method — a compile-time breaking change for any out-of-repo implementer of the interface. Accepted as won't-fix by design (reviewed in S17 finding L16): all in-repo implementers (proxy, tracker, autoclaim, and test doubles) were updated.
  • types.HealthResponse gains an optional pending_networks field (json:"pending_networks,omitempty"), so it is backwards compatible for existing clients.
  • The new AutoRegisterNewNetworks config key defaults to true, i.e. today's behavior, for both the proxy binary and the main aggkit binary's AutoClaim.BridgeServiceFinder.

📋 Config Updates

[BridgeServiceFinder]
AutoRegisterNewNetworks = true

[AutoClaim.BridgeServiceFinder]
AutoRegisterNewNetworks = true

✅ Testing

  • 🤖 Automatic:
    • Unit tests: common, bridgetracker/... (including domain, api, sources, types), bridgeservicefinder, proxy/..., autoclaim/....
    • Two new e2e tests, run in a new dedicated CI matrix group proxy-tracker-stateful (anvil-2chains env) so they don't share a stack/state with the default bridge group:
    • Negative controls were verified during development: with redaction stubbed out, the tracker leaked a real .invalid URL and the redaction test failed as expected; with the flag flipped to true on aggkit-proxy-002, it served the new network live and the auto-register test failed as expected — confirming both tests actually exercise the behavior they claim to.
  • 🖱️ Manual: none beyond the above; go build ./... and golangci-lint run --timeout 5m ./... both clean on this branch.

🐞 Issues

🔗 Related PRs

  • None.

📝 Notes

  • Design decisions validated with the user during planning:
    • AutoRegisterNewNetworks = false freezes the served network set at Start; a network discovered afterwards is recorded as pending (Finder.PendingNetworks(), surfaced on GET /health as pending_networks) rather than served.
    • Activation of a pending network happens only by restarting the process (so the Start enumeration re-runs) or by adding the network explicitly to BridgeURLs/RPCURLs in config.
    • There is intentionally no admin/activation REST endpoint and no allowlist config — activation is restart- or config-driven only.
    • Log lines are deliberately not redacted; operators still need the real endpoint to debug. Only client-facing API/WS fields are redacted.
    • IgnoreNetworkIDs takes precedence over AutoRegisterNewNetworks — an ignored network is never served or reported pending regardless of the flag.
  • Out of scope, called out as explicit follow-ups: no dev-ui changes (its static network list can still drift from the backend) and no kurtosis-cdk changes.

🤖 Generated with Claude Code

arnaubennassar and others added 8 commits September 21, 2026 15:15
The bridge tracker and the aggkit-proxy binary echoed the raw error.Error()
string of failed backend calls (net/http, go-ethereum RPC, bridge-service
HTTP client, agglayer gRPC client) straight into client-visible fields --
ErrorStep.Description, CertificateData.Error, ErrorData.Message (REST/WS),
ActivityItem.Errors / ActivityWarningItem.Message, and proxy's own JSON
error body. Those strings embed operator infrastructure (URLs, host:port,
bare IPs, sometimes API keys in the path).

Add aggkitcommon.RedactSensitive/RedactError/RedactSensitiveSlice
(common/redact.go) and apply them at every construction site (proxy
service, tracker REST/WS handlers, activity endpoint, resolve_steps /
resolve_bridge_tx), plus a defensive MarshalJSON on ErrorData, ErrorStep,
CertificateData, ActivityItem and ActivityWarningItem as a last line of
defense for any site that forgets. Log lines are deliberately left
unredacted -- operators still need the real endpoint to debug.

Closes #1845.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bridgeservicefinder (shared by proxy, tracker and autoclaim) previously
served any rollup discovered after Start the moment it was enumerated --
via CreateNewRollup/CreateNewAggchain/AddExistingRollup events, or via a
Start-enumerated network's first SetTrustedSequencerURL/AggchainMetadataSet
event. Half-configured networks could therefore get used prematurely.

Add Config.AutoRegisterNewNetworks (TOML default true, matching today's
behavior). When false, the served set is frozen at Start: a network
discovered afterwards is recorded as a PendingNetwork instead of served,
logged once at Warn, and surfaced via the new Finder.PendingNetworks()
method -- until the process is restarted or the network is added to
BridgeURLs/RPCURLs (IgnoreNetworkIDs still takes precedence over the flag).

Wire the TOML default through config/default.go, proxy/config/default.go,
the two proxy config-generation scripts and the existing e2e env configs,
update doc.go, and add unit test coverage for the gating and the pending set.

Note: Finder gaining PendingNetworks() is a compile-time breaking change
for any out-of-repo implementer of the interface.

Part of #1855.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Surface bridgeservicefinder.Finder.PendingNetworks() on the tracker's
GET /health response as an optional pending_networks field (network id,
rollup address, first-seen block/time, reason) so operators can observe
gated-but-not-yet-served networks without grepping logs.

Add types.HealthResponse.PendingNetworks ([]PendingNetwork,
json:"pending_networks,omitempty" -- backwards compatible), a
PendingNetworksLister interface in bridgetracker/api decoupling
bridgetracker/types from bridgeservicefinder, and wire it through
bridgetracker.Config.PendingNetworksLister / proxy/cmd/run.go (the finder
satisfies the interface directly). Regenerate the swagger docs.

Part of #1855.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add TestProxyTrackerRedactsURLs (#1845): rewrites a network's
RPCURLs/BridgeURLs to unresolvable .invalid hostnames, restarts
aggkit-proxy-001, and asserts the tracker's client-facing error strings
never contain the raw URL/host while still showing the redaction placeholder,
proving a URL-bearing failure path actually ran.

Add TestProxyAutoRegisterNewNetworks (#1855): runs a second aggkit-proxy-002
instance with AutoRegisterNewNetworks=false alongside aggkit-proxy-001 (left
at the true default), attaches a new rollup on L1, and asserts -001 serves it
live while -002 reports 404 and lists it under health's pending_networks
until it is restarted.

Add the aggkit-proxy-002 compose service and its
aggkit-proxy-noautoreg.toml, loader helpers to restart aggkit-proxy with an
edited config, and a dedicated proxy-tracker-stateful CI matrix group since
these tests mutate config/L1 state and must not share a stack with the
default bridge suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…daction

Document the new [BridgeServiceFinder] AutoRegisterNewNetworks flag (and
its [AutoClaim.BridgeServiceFinder] equivalent), the frozen-set/pending
semantics and activation paths (restart, or adding a network to
BridgeURLs/RPCURLs), the /health pending_networks field, the ErrorStep /
ActivityItem redaction behavior, and the two new e2e tests plus their
CI matrix group.

Closes #1855.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolves the two conflicts caused by #1852 (SQLite-backed bridgetracker
cache), keeping both sides' intent:

- bridgetracker/bridgetracker.go: api.NewAPI now receives upstream's
  ActivityRegisterResolveTimeout and activityPollInterval arguments *and*
  our trailing cfg.PendingNetworksLister.
- bridgetracker/api/activity_command.go: keeps upstream's rewritten
  Execute (validate-before-flush, RegisterAndAwait, 503 + Retry-After)
  while retaining our aggkitcommon.RedactError on the two error sites
  that existed before the rewrite (ParseActivityFilter 400, GetActivity
  500), plus our ActivityItem/ActivityWarningItem MarshalJSON redaction.

Our redaction in bridgetracker/activity.go followed upstream's rewrite to
its new sites (entry.Errors "claim" and "readiness"). Redacting the error
sites the upstream commit newly introduced is deliberately left out of
this merge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The develop merge (9ae85dd) brought in a SQLite-backed activity store
that re-introduced the same class of raw-error leak issue #1845
already fixed for the in-memory activity cache: entry.Errors["claim"]/
["readiness"] and activity_address.last_warnings could persist a raw,
unredacted backend error string.

Extend write-time redaction (aggkitcommon.RedactError) to the two
sqliteActivityStore.refresh sites, and add read-time redaction
(aggkitcommon.RedactSensitive, idempotent) in decodeWarnings and
activityBridgeRow.entry() so a row written by a pre-fix build is
served clean in memory too, not only on the wire via the defensive
MarshalJSON implementations. Add a round-trip test for last_warnings
mirroring the existing claim/readiness ones.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HealthResponse gained an optional pending_networks field without
bumping CurrentAPIRevision, contradicting the constant's own
documented rule that it is bumped whenever a change could break an
existing client parsing the tracker's responses (e.g. a strict
decoder rejecting unknown fields). Bump it to 5 and record the change
in the revision history comment, matching the existing entries' style.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@arnaubennassar

Copy link
Copy Markdown
Collaborator Author

Merged `develop` into this branch (`9ae85dd1`, pulling in the new SQLite-backed activity/tracker cache from #1822/#1852) and pushed the resolution plus two follow-up fixes:

  • Redaction extended to the new SQLite activity store. The merge re-introduced the same class of raw-error leak issue bug: proxy: redact URL from reported errors #1845 fixed: `sqliteActivityStore.refresh`'s two `entry.Errors["claim"]`/`["readiness"]` write sites now go through `aggkitcommon.RedactError`, and `decodeWarnings`/`activityBridgeRow.entry()` now redact on read too (idempotent) — write-time redaction covers new rows going forward, read-time redaction additionally covers any row a pre-fix build already wrote to disk, so an upgrade doesn't serve a raw backend URL from a legacy row into memory even before the defensive `MarshalJSON` layer gets to it. Added a round-trip test for `last_warnings` mirroring the existing claim/readiness ones.
  • Tracker API revision bumped to 5. `HealthResponse` gained the optional `pending_networks` field without bumping `CurrentAPIRevision`, contradicting the constant's own documented rule. Bumped to 5 with a revision-history entry.
  • Known coverage gap: the redaction e2e (`TestProxyTrackerRedactsURLs`) only covers the tx-tracking endpoint. The activity endpoint and the new SQLite-backed activity store are exercised by unit tests only, since `DBPath` is unset (SQLite off) in every e2e env. Not fixing here — wiring a DB path into an e2e env is scope growth for this PR.

`go build`/`go vet`/`golangci-lint` clean, unit suite green, both e2e tests (`TestProxyTrackerRedactsURLs`, `TestProxyAutoRegisterNewNetworks`) pass against `anvil-2chains`.

🤖 Generated with Claude Code

@arnaubennassar

Copy link
Copy Markdown
Collaborator Author

@claude

@claude

claude Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Claude finished @arnaubennassar's task in 6m 43s —— View job


Review: fix(bridgetracker): redact error URLs + gate bridgeservicefinder auto-registration

  • Gather context (diff vs origin/develop)
  • Review redaction layer (common/redact.go + all call sites)
  • Review bridgeservicefinder auto-register gating
  • Review tests / config / docs / e2e infra
  • Post review + summary template

🚀 What's New

Two independent, well-scoped hardening changes to the bridge tracker / aggkit-proxy stack:

  1. Error redaction (bug: proxy: redact URL from reported errors #1845). New common/redact.go exposes RedactSensitive / RedactError / RedactSensitiveSlice: a pure, idempotent, 10-rule regex pipeline that replaces scheme-bearing URLs, host:port, IPv4/IPv6 literals and anchored DNS names with <redacted-url> / <redacted-host>. It is applied at three layers: at construction (resolve_steps.go, resolve_bridge_tx.go, sources/activity.go:warnf, both activity stores, every api/*_command.go + websocket.go error path, proxy/service.go:97), on the SQLite read path (decodeWarnings, activityBridgeRow.entry()) so rows written by a pre-fix build are cleaned on upgrade, and defensively in MarshalJSON (ErrorData, ErrorStep, CertificateData, ActivityItem, ActivityWarningItem). Logs are deliberately left unredacted.
  2. Auto-registration gating (feat: proxy / bridgeservicefinder: Add config flag to control automatic registration of newly discovered networks in bridgeservicefinder #1855). bridgeservicefinder.Config.AutoRegisterNewNetworks (TOML default true) freezes the served network set at Start when set to false. All three post-Start install paths (discoverRollup, applyUpdate, refreshFromChain) record a PendingNetwork instead, logged once at Warn and surfaced on GET /tracker/v1/health as pending_networks (CurrentAPIRevision 4 → 5). IgnoreNetworkIDs correctly takes precedence — I verified ignored networks never enter addrToNetworkID, so applyUpdate/refreshFromChain are unreachable for them, and discoverRollup's ignore check sits before the new gate.

Plus two e2e tests in a new dedicated proxy-tracker-stateful CI matrix group, a second aggkit-proxy-002 compose service, regenerated swagger, and thorough doc updates.

🐛 Bug Fixes

  • Client-facing error strings no longer leak backend URLs / host:port / bare IPs (potentially with API keys in the path) via ErrorStep.Description, ErrorData.Message, CertificateData.Error, the activity endpoint's errors / warnings[].message, or the proxy's own JSON error body.
  • bridgeservicefinder no longer unconditionally serves half-configured rollups discovered after Start.
  • CurrentAPIRevision bumped to 5, fixing the revision-rule violation the merge from develop would otherwise have left.

📋 Config Updates

[BridgeServiceFinder]
AutoRegisterNewNetworks = true   # default; false freezes the served set at Start

[AutoClaim.BridgeServiceFinder]
AutoRegisterNewNetworks = true   # default

⚠️ Breaking Changes

  • bridgeservicefinder.Finder gains PendingNetworks() []PendingNetwork — compile-time break for any out-of-repo implementer. All in-repo implementers/test doubles updated.
  • types.HealthResponse gains pending_networks (omitempty) — wire-compatible; api_revision 4 → 5.
  • No runtime behavior change at default settings.

Overall

This is careful, high-quality work. The layered redaction (construct → persist-read → marshal) is the right shape, the gating is correctly ordered relative to IgnoreNetworkIDs, and the e2e tests include genuine positive assertions plus documented negative controls. Findings below are all non-blocking; the first two are the ones I'd most like to see addressed.

⚠️ I could not run go build / go test / golangci-lint in this environment (command approval denied), so everything below is from static reading of the diff. The author reports all three clean.


🟡 Suggested

1. AutoRegisterNewNetworks's Go zero value is the opposite of its documented default — bridgeservicefinder/config.go:105

The default lives only in the two embedded TOML blobs; applyConfigDefaults (bridgeservicefinder.go:155) cannot distinguish "false" from "unset" for a bool. The code comment acknowledges this, which is good, but a comment is a weak guard for a footgun where the failure mode is silent: any future in-Go Config{...} literal — a new binary, an embedder, a benchmark, a test helper — freezes network discovery with no error and no log, and the symptom (a rollup attached later is never served) surfaces hours away from the cause.

Two robust fixes, either of which removes the hazard entirely:

  • Invert the field: FreezeNetworkSetAtStart bool (TOML false). Zero value then equals the documented default, and applyConfigDefaults never needs to know about it.
  • Or make it *bool and resolve it in applyConfigDefaults alongside the other defaults, so there is exactly one place the default is expressed.

I'd lean towards inverting — it also reads better at the call site (if l.freezeNetworkSet { … record pending … }).

Fix this →

2. aggkit-proxy-002 starts in every anvil-2chains matrix group — test/e2e/envs/anvil-2chains/docker-compose.yml:198

The new service has no profiles: key (no env in this repo uses one), so it is brought up by ensureDockerComposeRunning for the bridge, backward-forward-let and every other anvil-2chains group too — an extra container, an extra L1 event poller, an extra agglayer client and an extra host port binding in jobs that never touch it. Adding profiles: ["proxy-tracker-stateful"] and setting COMPOSE_PROFILES=proxy-tracker-stateful for that one matrix entry confines it to where it's needed.

Fix this →

3. aggkit-proxy-noautoreg.toml is a 114-line byte-copy differing in one line

"Do not otherwise diverge this file; keep the two in sync" is the only thing preventing drift, and drift here produces a silently wrong test (e.g. a stale RollupManagerAddr would make TestProxyAutoRegisterNewNetworks fail for an unrelated reason). --cfg is a cli.StringSliceFlag (proxy/cmd/main.go:15) and LoadFiles merges files in order with later wins (proxy/config/config.go:118-126), so this can collapse to:

volumes:
  - ./config/aggkit-proxy/aggkit-proxy.toml:/etc/aggkit-proxy/config.toml:ro
  - ./config/aggkit-proxy/noautoreg-override.toml:/etc/aggkit-proxy/override.toml:ro
command: ["run", "--cfg=/etc/aggkit-proxy/config.toml", "--cfg=/etc/aggkit-proxy/override.toml", "--components=proxy,tracker"]

with a 3-line override file. ⚠️ Caveat worth weighing: that couples -002 to TestProxyTrackerRedactsURLs's in-place rewrite of aggkit-proxy.toml (both tests run in the same group). Since -002 is only restarted inside the autoregister test, and Go runs proxy_autoregister_test.go before proxy_tracker_redaction_test.go, this is currently benign — but it's real coupling, so the duplicate may be the deliberate cheaper trade. Your call; flagging so it's a decision rather than an accident.

4. bridgetracker (config) → bridgetracker/api → bridgeservicefinder layering — bridgetracker/config.go:281

Declaring PendingNetworksLister in bridgetracker/api and returning []bridgeservicefinder.PendingNetwork makes the config package depend on the api package, and drags the whole finder (plus its go-ethereum contract bindings) into the dependency graph of every importer of bridgetracker. health_command.go's own comment says the goal was "keeping bridgetracker/types free of any dependency on bridgeservicefinder" — but the interface could just live in bridgetracker/types returning []types.PendingNetwork, with proxy/cmd/run.go supplying a tiny adapter over finder. That inverts the dependency the intended way and keeps bridgetracker/config.go importing only types.

5. Residual redaction gap: portless, scheme-less hostnames

Rules 5/7/8 all require a port, and bare hostnames are only redacted in three anchored contexts (lookup X, no such host: X, certificate is valid for X, not Y). That's a deliberate and correct trade against false positives — but it means an error like failed to reach bridge-svc.internal.acme.com or connection refused by aggkit-002 still leaks the endpoint. The URL-with-API-key case #1845 calls out is scheme-bearing so it is covered, and the MarshalJSON layer doesn't help here (it runs the same rules). Worth stating this residual explicitly in RedactSensitive's doc comment (common/redact.go:66-70) so a future reader doesn't assume the function is exhaustive.

🟢 Optional

6. Three doc comments are truncated mid-sentence — bridgetracker/api/activity_command.go:92, :126, bridgetracker/types/status.go:25

All three end with …this is the last line of defense for any caller that does not — the sentence stops there (copy-pasted). Also activity_command.go:88-89 reads …from every Errors value defensively — domain.ActivityEntry. / // Errors is already redacted…, where the line break lands inside the qualified name.

7. Per-marshal cost of RedactSensitive — common/redact.go:71

Ten regex passes run on every ErrorData / ErrorStep / CertificateData / ActivityItem marshal, including the WS update hot path and activity responses that can carry many items each with an Errors map — and in the overwhelmingly common case the string contains nothing to redact. A cheap pre-filter would short-circuit that. Note it can't be the obvious strings.ContainsAny(s, ":.["): rule 6 matches a dotless, colonless lookup myhost, so the guard would need || strings.Contains(s, "lookup") (placeholders themselves contain none of :.[ nor lookup, so idempotency survives either way). Given the strings are short this is likely not worth the added subtlety — raising it only so the trade is conscious. Similarly redactActivityErrors (activity_command.go:99) allocates a fresh map on every marshal even when nothing changes.

8. Health example in the docs is stale — docs/bridgetracker/API.md:410

The example body still shows "api_revision": 2 (now 5) and carries no pending_networks illustration, even though the field table right above it documents both. Since this PR is what bumps the revision and adds the field, it's the natural moment to refresh it.

9. e2e: impersonation is never stopped — test/e2e/proxy_autoregister_test.go:257

anvil_impersonateAccount, the 100 ETH anvil_setBalance and the _ADD_EXISTING_ROLLUP_ROLE grant all persist for the rest of the group. That's fine and intentional given the dedicated matrix group, but a t.Cleanup calling anvil_stopImpersonatingAccount costs nothing and makes local re-runs against a warm stack less surprising.

10. e2e: the redaction test rewrites a git-tracked file — test/e2e/proxy_tracker_redaction_test.go:128

RestartAggkitProxyWithConfig (test/e2e/envs/loader.go:868) mutates test/e2e/envs/anvil-2chains/config/aggkit-proxy/aggkit-proxy.toml in the working tree. The t.Cleanup restore is correct and the comment at :134 already handles the failed-restore case — but not a hard kill (CI job timeout, SIGKILL, ^C), which leaves the checked-in config pointed at .invalid hosts and the repo dirty. Not worth restructuring; a one-line note in the helper's doc comment would be enough to save the next person the debugging.


What I checked and found correct

  • Redaction idempotency holds for every rule: <redacted-url> / <redacted-host> contain no ://, no dotted quad, no […:…], and rules 6/8/9/10 all require their capture group to start with [A-Za-z0-9], which < is not (rule 6 additionally includes </> in its token class, as its comment claims).
  • IgnoreNetworkIDs precedence — verified end to end: buildInitialCache (bridgeservicefinder.go:261) skips ignored networks before resolveNetwork populates addrToNetworkID, and discoverRollup's ignore check (listener.go:475) precedes the new gate, so applyUpdate / refreshFromChain are structurally unreachable for an ignored network and can never record one as pending. The PR description's claim is accurate.
  • refreshFromChain is only reached from processLog (listener.go:326), never from buildInitialCache — so the new !exists && !autoRegister gate cannot break startup enumeration.
  • The listener and the finder share one *cache (bridgeservicefinder.go:214), so PendingNetworks() observes what the listener records; pendingList copies under RLock and PendingNetwork is all value types, so no aliasing escapes.
  • No leak via the reverse proxy: ErrorHandler (proxy/service.go:130-135) already writes a static {"error":"bridge service unreachable"} and only the log line carries the URL.
  • All NewAPI / bridgetracker.New call sites updated (one each); PendingNetworksLister is assigned from a bridgeservicefinder.Finder interface value in proxy/cmd/run.go:244, so the nil-interface check in healthCommand.Execute behaves correctly.
  • lll violations in the new e2e files (e.g. proxy_autoregister_test.go:179) are excluded by .golangci.yml's path: (_test\.go|^test/.*) rule.
  • Swagger regenerated consistently — bridgetracker/api/docs/bridgetracker_swagger.json and docs/assets/swagger/bridge_tracker/swagger.json share a SHA.
  • Env-var override path covered — the new CDK_PROXY_BRIDGESERVICEFINDER_AUTOREGISTERNEWNETWORKS test in proxy/config/config_test.go:79 is a good addition; it pins the one path most likely to regress when the default moves.
    · branch fix/proxy-redact-errors-autoregister-flag

if err := json.Unmarshal(raw, &warnings); err != nil {
return nil, fmt.Errorf("decoding activity last_warnings: %w", err)
}
for i := range warnings {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redacting logs makes more sense at the API layer rather than in the internal objects. As the PR states, URLs/tokens are redacted only at the API level, not in logs. This comment applies to all redacted logs, not just sqlite, but all of them:

“Log lines are deliberately not redacted; operators still need the real endpoint to debug. Only client-facing API/WS fields are redacted.”

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done. Construction-site redaction is removed from every internal/domain object and the activity store (12 call sites); the tracker's internal objects now hold the raw error string in memory, identical to what gets logged. Redaction happens exactly once, at the API/wire layer: the five types.*.MarshalJSON implementations (ErrorStep, ErrorData, CertificateData, ActivityItem, ActivityWarningItem), the ErrorData literals inside bridgetracker/api (load-bearing, not redundant — wsSendError passes errData.Message to wsClose as a bare WebSocket close-frame reason that never reaches ErrorData.MarshalJSON), and proxy/service.go's gin.H error map (also load-bearing: gin.H has no marshaler to fall back on). Commit d536926.

No field lost wire coverage: ErrorStep reaches clients via api.TrackingData.Error and api.BridgeStepPath.Error (both *types.ErrorStep, so MarshalJSON is in the pointer's method set for either), and domain.ActivityEntry.Errors/domain.ActivityWarning.Message reach a client only through newActivityItems/newActivityWarningItems. A row written by a pre-fix build is no longer rewritten on read — it's served raw in memory like any other row, with the wire layer as the single place that redacts, the same as everything else. docs/bridgetracker.md now states this placement explicitly.

Comment thread bridgetracker/api/docs/bridgetracker_swagger.json
"description": "InstanceID is a UUID generated at startup; it changes on every execution, so two\nresponses with different InstanceID come from different instances (or the same\ninstance after a restart)",
"type": "string"
},
"pending_networks": {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment related to start_date or uptime

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer as on the swagger comment: added start_date, not uptime (an absolute instant keeps the response byte-identical between calls and is directly comparable with pending_networks[].first_seen). See the reply on #1862 (comment) for the full reasoning. This file (docs/assets/swagger/bridge_tracker/swagger.json) is regenerated byte-identical to bridgetracker/api/docs/bridgetracker_swagger.json. Commit fddacac.

Comment thread bridgeservicefinder/listener.go Outdated
Comment thread bridgeservicefinder/listener.go Outdated
arnaubennassar and others added 4 commits September 22, 2026 12:38
…verrides and treat SourceConfig as terminal on the discovery path

Two review findings on discoverRollup that are inseparable in the diff
(same function, adjacent hunks), so they land in one commit rather than
the two originally planned:

- The AutoRegisterNewNetworks=false branch was inverted: an already-served
  network (a static Config.BridgeURLs override installed at Start) took
  the early return meant for a genuinely new one, so it never had its
  contract registered/watched and never got its JSON-RPC endpoint resolved
  on-chain. Only the pending-record write is skipped for an already-served
  network now; registration, watching, and JSON-RPC resolution proceed
  exactly as in the AutoRegisterNewNetworks=true case.

- discoverRollup's on-chain resolution could downgrade a SourceConfig
  cache entry the resolver's override map no longer covers. A guard
  (cur.source == SourceConfig && source != SourceConfig) makes it
  terminal on this path too, mirroring applyUpdate's existing rule. The
  overwrite as literally described in review is not reachable today
  (resolver.resolve is config-first, so cache and resolver cannot
  disagree in production), but the guard is a correctness invariant
  worth having locally rather than one that depends on that being true
  forever.

Also fixes a genuine data race the S26 -race run caught in the new R4
regression test: discoverRollup's guard branch returns right after
l.cache.get(rollupID) without ever reaching cache.set, so there was no
lock/unlock pair after the addrToNetworkID/watchedAddresses write for a
concurrent reader to synchronize against. Both write sites now take
cache.mu (reused rather than adding a dedicated lock) and the test reads
addrToNetworkID under the same lock instead of bare.

Extends TestLiveDiscovery_AlreadyServedFromBridgeURLsNeverGoesPending to
cover the JSON-RPC resolution path and adds
TestLiveDiscovery_ConfigSourcedEntryIsNeverOverwrittenByDiscovery for the
terminal-source guard.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…objects

Removes 12 construction-site redaction calls (RedactError/RedactSensitive)
from internal/domain objects and the activity store, so those objects
now hold the raw error string in memory, identical to what gets logged.
Redaction now happens exactly once, at the API/wire layer: the five
types.*.MarshalJSON implementations (ErrorStep, ErrorData,
CertificateData, ActivityItem, ActivityWarningItem), the ErrorData
literals inside bridgetracker/api (load-bearing there, not redundant --
wsSendError passes errData.Message to wsClose as a bare WebSocket
close-frame reason that never reaches ErrorData.MarshalJSON), and
proxy/service.go's gin.H error map (also load-bearing: gin.H has no
marshaler to fall back on).

No field loses wire coverage: ErrorStep reaches clients via
api.TrackingData.Error and api.BridgeStepPath.Error, both *types.ErrorStep,
and MarshalJSON is in the pointer's method set for either;
domain.ActivityEntry.Errors and domain.ActivityWarning.Message reach a
client only via newActivityItems/newActivityWarningItems. A row written
by a pre-fix build is no longer rewritten on read -- it is served raw in
memory like any other row, with the wire layer as the single place that
redacts, same as everything else.

Test migration: assertions that pinned the in-memory value as redacted
now pin the raw value in memory and the redacted value on the wire, so
both the client-facing guarantee and the new "raw in memory" invariant
are asserted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds HealthResponse.StartDate (json:"start_date", RFC3339 UTC, never
omitted), the instant the instance started, stamped in NewAPI alongside
the existing instance_id UUID. Chosen over uptime: an absolute instant
keeps the response byte-identical between calls, and it is directly
comparable with pending_networks[].first_seen (also RFC3339 UTC), which
is the reviewer's underlying point since every pending entry is by
definition discovered after startup. A client can derive uptime as
now - start_date; serving both would be redundant.

Folded into the existing (unshipped) CurrentAPIRevision = 5 bullet
rather than bumping to 6, since revision 5 was introduced by this same
PR for pending_networks and has not shipped -- both additive fields land
in the same contract shape.

Swagger regenerated in both copies (bridgetracker_swagger.json/yaml and
docs/assets/swagger/bridge_tracker/swagger.json); docs/bridgetracker/API.md's
HealthResponse table and example gained the new field.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
32 comments across these 9 files cited artifacts that only exist in an
untracked planning file (e.g. "(S17 M2)", "S21 regression test",
"S1-obs-7", "the plan's..."), which mean nothing outside that file.
Comment-only change: each now states the actual reason inline instead
of the citation; nothing explanatory was removed.

grep -rnE "S[0-9]+ ?(finding|obs|C[0-9]|L[0-9])|the plan's" --include=*.go .
and a wider grep for "\bS[0-9]{1,2}\b|the plan|S3\(" over every .go file
this PR touches now find no matches in files this PR changes.

Out of scope, left alone to avoid unrelated churn: the same class of
reference exists in files this PR does not touch
(test/e2e/removeger_test.go, test/e2e/autoclaim_test.go,
l2gersync/processor_test.go, l2gersync/evm_downloader_sovereign.go) --
present at the merge base, belonging to earlier PRs' plans.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@arnaubennassar

Copy link
Copy Markdown
Collaborator Author

Review round follow-up

Pushed 4 commits addressing all 5 open review threads:

  • 77d9cd0f — R3 + R4 (bridgeservicefinder): the AutoRegisterNewNetworks=false early return for an already-served network no longer skips registering/watching its contract (only the pending-record write is skipped now), and a SourceConfig cache entry is now terminal on the discovery path too, mirroring applyUpdate. Also fixes a data race the required -race run caught in the new R4 regression test (no lock/unlock pair existed after the addrToNetworkID write on the guard-triggering path); both write sites now take cache.mu.
  • d536926f — R1: redaction moved out of internal/domain objects entirely, into the API/wire layer (five MarshalJSON implementations, the ErrorData literals in bridgetracker/api, and proxy/service.go's gin.H). No field lost wire coverage.
  • fddacac0 — R2: added start_date (not uptime) to the tracker health response, folded into the existing unshipped API revision 5.
  • e810bbf0 — cleanup of 32 code comments across 9 files that cited an internal planning file's section numbers (no review thread for this one, flagging it here for visibility).

Replies with full detail are on each thread. Verification: make lint, go build/go vet, full unit suite with -race (green except pre-existing environmental failures in claimsync and l2gersync, both unrelated to this PR — port 8545 contention from an unrelated running stack, and confirmed to pass in isolation), make build-docker, and the anvil-2chains e2e runs for TestProxyTrackerRedactsURLs, TestProxyAutoRegisterNewNetworks, TestJustBridge, TestBridgeL2ToL2, and TestBridgeTrackerL1ToL2 — all PASS.

Second develop move while this PR was in review: fe5add4 -> 967858d
("feat(bridgetracker): step start_date/end_date from on-chain facts, not
now", #1864).

One hard conflict, docs/bridgetracker/API.md, resolved as a union: the
ErrorStep section keeps upstream's three-kinds-of-information intro, the
error_type 3 / "warning" rows and the retry_count note, and keeps this
branch's redaction contract sentence on the description row (extended to
name the warning case too, since a warning's text travels the same
field).

Signature adaptations this merge needed, both in tests this branch added
against the pre-#1864 API and neither changing what they assert:
  - resolve_bridge_tx_test.go: ResolveBridgeTx gained a resolvers map;
    the test exercises the FindBridge-error path, where PendingPath (the
    only consumer) is never reached, so it passes nil.
  - resolve_steps_test.go: UpdateStep gained a trailing StepResolver;
    the four subtests this branch added are all error paths, so they
    pass nil, exactly as upstream's own subtests do.

resolve_steps.go is now byte-identical to develop again: the multi-line
wrap of idxError, left over from the construction-site redaction that
review comment 4069416912 had us remove, is folded back to upstream's
single line.

Redaction verified by hand across the merge, in both directions:
ErrorStep/ErrorData/CertificateData MarshalJSON all survive, and
#1864's new client-facing text (GERData.L2InjectionUnresolvedReason ->
InjectedGERResult.L2InjectionWarning -> the step's own ErrorStep with
ErrorType StepErrorWarning) reaches the wire only through
ErrorStep.MarshalJSON, and is hand-written sanitized text upstream-side
to begin with. No new unredacted path.

Swagger regenerated with the pinned swag and found semantically
identical to the textual merge; the merge result is kept so the only
diff is the genuine change from each side (StepErrorWarning + the error
description text from upstream, start_date + pending_networks from here)
rather than swag's nondeterministic time.Duration enum block.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@arnaubennassar

Copy link
Copy Markdown
Collaborator Author

Merged develop again (fe5add42 → 967858d2, #1864)

develop moved a second time while this round of review was in flight, so the branch carries a
second merge commit (4a123cc9) rather than a rebase — the branch is pushed, and a rebase would
need a force-push.

What the merge required

Redaction re-checked by hand, in both directions. ErrorStep, ErrorData and
CertificateData's MarshalJSON all survive the merge. #1864's new client-facing text
(GERData.L2InjectionUnresolvedReason → InjectedGERResult.L2InjectionWarning → the step's own
ErrorStep with ErrorType StepErrorWarning) reaches the wire only through
ErrorStep.MarshalJSON, and findL2InjectionBlockBackwards builds it as hand-written sanitized
text, deliberately never err.Error() — so there is no new unredacted path to cover.

Two neutral observations for whoever reviews the merged diff

  1. feat(bridgetracker): step start_date/end_date from on-chain facts, not now #1864 changed the per-step start_date/end_date semantics on the wire without bumping
    CurrentAPIRevision (still 4 there). After the merge, revision 5 — introduced by this PR —
    therefore has a history bullet describing only this PR's two additions, not that change.
    Flagging it so nobody reads the bullet as exhaustive for revision 5; whether it warrants a bump
    is feat(bridgetracker): step start_date/end_date from on-chain facts, not now #1864's call, not something changed here.
  2. The start_date name now appears twice in the tracker API, on different objects: feat(bridgetracker): step start_date/end_date from on-chain facts, not now #1864's is
    per bridge step (all_steps[].start_date), this PR's is per instance on /health.
    Incidental name overlap only — not a duplicated field.

Verification on the merged tree: make lint 0 issues; go build ./... and go vet ./...
clean; go test -race -count=1 over everything but ./test/e2e → 94 packages ok, with only the
pre-existing claimsync failures this PR has reported throughout (host port 8545 held by an
unrelated local stack). E2E on anvil-2chains after make build-docker:
TestProxyAutoRegisterNewNetworks, TestProxyTrackerRedactsURLs, TestJustBridge,
TestBridgeL2ToL2, TestBridgeTrackerL1ToL2 — all PASS, no skips.

🤖 Generated with Claude Code

@arnaubennassar arnaubennassar self-assigned this Sep 22, 2026
@arnaubennassar
arnaubennassar merged commit bf2960c into develop Sep 23, 2026
34 checks passed
@arnaubennassar
arnaubennassar deleted the fix/proxy-redact-errors-autoregister-flag branch September 23, 2026 07:52
arnaubennassar added a commit that referenced this pull request Oct 2, 2026
… (#1877)

## 🔄 Changes Summary
- Add three new `/bridge/v1/sync-status` entries: `l1_info_tree_info`,
`claim_l1_info` and `claim_l2_info`, so all six syncers used by a
bridge-service instance (bridgesync L1/L2, l2gersync, l1infotreesync,
claimsync L1/L2) are now reported, not just the first three.
- Add `is_halted` to every sync-status entry (`l1_info`, `l2_info`,
`l2_ger_info` and the three new ones). It is `false` for syncers with no
halt state, and `false` (alongside `is_active:false`) for a syncer that
isn't configured on this instance — the two cases are distinguished by
which entries are present, per the docs.
- Add a per-entry `error` (redacted via the existing
`aggkitcommon.RedactError` helper from #1862: URLs and hosts are
stripped) instead of failing the whole request on the first component
error.
- `/health` gains matching `details` keys `l1_info_tree`, `claim_l1` and
`claim_l2`, plus `is_halted` on every `details.*` entry. `/health` is
derived from the exact same sync-status computation as before (pure
function over the result), so the two endpoints can never drift.
- `computeSyncStatus` now computes all six components concurrently
instead of sequentially, and a panic in a single component's goroutine
is logged with its original stack before being re-raised.
- Add `L1InfoTreeSync.IsActive(ctx) bool` to `l1infotreesync`, mirroring
`bridgesync.BridgeSync.IsActive`.
- Fix a regression introduced (and caught) inside this branch:
`cmd/run_bridgeservice.go` was passing nil concrete syncer pointers into
interface parameters, which produces a non-nil interface holding a typed
nil. On any instance without a claim or l1infotree syncer configured,
both `/bridge/v1/sync-status` and `/health` panicked and returned 500.
Nil concrete pointers are now converted to untyped nil interfaces before
being wired in.
- Fix `bridgesync.GetContractDepositCount`,
`l1infotreesync.GetLastProcessedBlock` and
`l2gersync.GetLastProcessedBlock` to honour the caller's `ctx`: the
bridgesync getter now passes `ctx` via `bind.CallOpts`, and the two
SQLite getters use `QueryRowContext` instead of
`QueryRow`/`meddler.QueryRow`. The `/bridge/v1/sync-status` read timeout
and `/health` compute timeout now actually bound these calls, which
previously ignored `ctx`. Also removes the unused `l2gersync.BlockNum`
helper type.

## ⚠️ Breaking Changes
- 🛠️ **Config**: N/A
- 🔌 **API/CLI**: The JSON response shape is additive only — no existing
field is renamed, removed, or repurposed. The behavioural change: `GET
/bridge/v1/sync-status` now always answers **200**, with a per-entry
`error` string, instead of returning **500** on the first failing
component. `GET /health` `details.*.error` text is now redacted
(hosts/URLs stripped) instead of a raw error. Clients that previously
treated a 500 from `/bridge/v1/sync-status` as their failure signal must
switch to inspecting each entry's `error`/`is_halted` field; a 500 is no
longer produced by this endpoint for component failures.
- 🗑️ **Deprecated Features**: None.

## 📋 Config Updates
- None.

## 🔌 API Updates
### 🔌 Bridge service API
- `GET /bridge/v1/sync-status`: added `l1_info_tree_info`,
`claim_l1_info`, `claim_l2_info`; added `is_halted` to every entry;
added `error` to every entry; the endpoint now always responds 200
instead of 500 on a component failure. Not a breaking interface change
apart from the 200-vs-500 status behaviour described above — no field
was removed or renamed.
- `GET /health`: added `details.l1_info_tree`, `details.claim_l1`,
`details.claim_l2`; added `is_halted` to every `details.*` entry; error
text in `details.*.error` is now redacted. Still always returns HTTP
200. Not a breaking interface change.

### 🔌 Proxy API
- None.

### 🔌 Others API
- None.

## ✅ Testing
- 🤖 **Automatic**: New unit tests in `bridgeservice/bridge_test.go`
(`TestSyncStatusAllSyncers`, `TestHealthFromSyncStatus`,
`TestSyncStatusRedactsErrors`,
`TestComputeSyncStatus_ComponentsRunConcurrently`,
`TestComputeSyncStatusPanicPropagates`), a real halted-processor test in
`l1infotreesync/bridgeservice_halt_test.go`
(`TestBridgeServiceReportsL1InfoTreeHalt`), the
`cmd/run_bridgeservice_test.go` nil-wiring regression tests, and a
`bridgetracker/sources/activity_test.go` regression test
(`TestIsNetworkSynced`) confirming an erroring network still reports
not-synced. New e2e test `TestBridgeServiceSyncStatusAllSyncers` in
`test/e2e/sync_status_all_syncers_test.go`, plus an extended
`TestBridgeServiceHealthSyncStatus`.
- Regression tests for the ctx fix, verified to fail on the pre-fix
code: `TestGetContractDepositCount_HonoursContextDeadline` in
`bridgesync` (a blocked `eth_call`), and
`TestGetLastProcessedBlock_HonoursContext` in both `l1infotreesync` and
`l2gersync` (an already-cancelled ctx).
- 🖱️ **Manual**: `golangci-lint` v2.4.0 reported **0 issues**. `make
test-unit` passed. E2E run (`AGGKIT_E2E_ENV=anvil-2chains make test-e2e
TEST_RUN='^(TestBridgeServiceSyncStatusAllSyncers|TestBridgeServiceHealthSyncStatus)$'`)
passed for both L2A and L2B: `--- PASS:
TestBridgeServiceHealthSyncStatus (3.51s)`, `--- PASS:
TestBridgeServiceSyncStatusAllSyncers (9.03s)`.

**Before (develop today):** `/bridge/v1/sync-status` returns only
`l1_info`, `l2_info` and `l2_ger_info` — no `l1_info_tree_info`,
`claim_l1_info` or `claim_l2_info`, and no `is_halted` anywhere. A
single failing component (e.g. an RPC blip on l1infotreesync) causes the
whole endpoint to answer HTTP 500 instead of returning what it could
compute.

  **After (this branch), real payload from the e2e env:**

  `GET /bridge/v1/sync-status`
  ```json
  {
"l1_info":
{"contract_deposit_count":2,"synchronized_deposit_count":2,"is_synced":true,"is_active":true,"is_halted":false},
"l2_info":
{"contract_deposit_count":0,"synchronized_deposit_count":0,"is_synced":true,"is_active":true,"is_halted":false},
"l2_ger_info":
{"is_active":true,"last_processed_block":100,"is_halted":false},
"l1_info_tree_info":
{"is_active":true,"is_halted":false,"last_processed_block":329},
"claim_l1_info":
{"is_active":true,"is_halted":false,"last_processed_block":329},
"claim_l2_info":
{"is_active":true,"is_halted":false,"last_processed_block":234}
  }
  ```

  `GET /health`
  ```json
  {
    "status": "ok",
    "time": "2026-09-29T07:33:26.425307489Z",
    "version": "v0.11.0-rc10-13-gaf2f650d",
    "sync_status": "done",
    "details": {
      "l1": {"is_active":true,"is_synced":true,"is_halted":false},
      "l2": {"is_active":true,"is_synced":true,"is_halted":false},
      "l2_ger": {"is_active":true,"is_halted":false},
      "l1_info_tree": {"is_active":true,"is_halted":false},
      "claim_l1": {"is_active":true,"is_halted":false},
      "claim_l2": {"is_active":true,"is_halted":false}
    }
  }
  ```

## 🐞 Issues
- Closes #1861

## 🔗 Related PRs
- #1862 (adds the `RedactError` helper this PR reuses for per-entry
error redaction)

## 📝 Notes
- Halt info is exposed as a boolean only (`is_halted`), with no reason
or halted block, because those strings can carry DB/RPC internals — this
follows the same redaction policy as #1862.
- Claimsync, l1infotreesync and l2gersync entries never produce
`pending` in `/health`'s aggregation: they have no in-service "caught
up" signal the way the L1/L2 bridge syncers do (claimsync syncs on
demand via `SetNextRequiredBlock`). They only ever feed the error bucket
or `done`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants