Repository navigation
feat: bridge service add block_finalized field to several endpoints 1915 - #1921
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46d647f31d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if header == nil || (blockHash != (common.Hash{}) && header.Hash != blockHash) { | ||
| return false, false, nil | ||
| } | ||
| return isFinal, true, nil |
There was a problem hiding this comment.
When blockHash is zero—such as for settlement rows whose block hash is unavailable—the condition skips hash validation and reports the finality of whichever block currently occupies blockNum. During a reorg, an orphaned row can therefore be labeled block_finalized: true based on its canonical replacement, defeating the identity check this field is intended to provide. Return found=false when no hash was supplied, as the legacy path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. I had left the zero-hash passthrough on purpose (assuming IsFinal alone was enough for the multidownloader path), but it does defeat the identity check. Fixed in 0734796: IsBlockFinalized now returns found=false for a zero hash in both modes, and in the multidownloader path the stored header must have the same hash as the one the caller read. The multidownloader keeps a single block per number in blocks, so without the hash is_final could belong to the block that replaced the one the row came from.
When can a zero hash reach here? For a block with events synced by the multidownloader, never: logs.block_number has a foreign key to blocks and the header is stored with the logs. The only zero-hash block (addLastBlockIfNotIncluded, a fake empty checkpoint) has no events, so no leaf or settlement lives in it. What is left are rows with a NULL hash in the l1infotreesync block table, written by the legacy syncer before that column existed; they now report unknown and block_finalized is omitted. The test "no hash given: unknown" covers it.
| if err = p.fillBlockHashes(ctx, bridges); err != nil { | ||
| return nil, 0, err | ||
| } |
There was a problem hiding this comment.
If a reorg is processed after the bridge rows are scanned but before fillBlockHashes runs, this follow-up query can attach the replacement block's hash to a bridge from the orphaned block at the same height. IsBlockFinalized then validates the replacement rather than the returned bridge and can eventually expose stale data as finalized. Include the block hash in the original query or keep both reads in one consistent transaction; the analogous follow-up enrichment in claims, roots, leaves, and injected GER reads has the same consistency risk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and the same race existed in claims, roots, leaves and the injected GER lookup. Fixed in 500cd10: every one of them now reads the block hash in the same SELECT as the rows, with a LEFT JOIN block (a single statement, so a single snapshot). The public types keep BlockHash as a non-persisted field (meddler:"-"); each package scans into an internal read model with block_hash and converts, following the VerifiedBatchWithBlockHash precedent. I did not use a read transaction because the connection is opened with _txlock=immediate, which would take the write lock.
Covered: GetBridgesPaged, GetBridgeByDepositCount, GetBridgesByContent (bridge and bridge_archive), GetRootByLER, GetClaimsPaged (the join wraps the final select of the compaction CTE, so the compaction cases are untouched), GetClaimsByGER, L1InfoTreeSync.GetInfoByIndex and GetFirstGERAfterL1InfoTreeIndex. A block row without hash gives the zero hash, which IsBlockFinalized reports as unknown. Not covered, and independent of the hash: the count and the page are still two statements, and bridge / bridge_archive are read in two statements.
While doing it I found a related race in IsBlockFinalized itself (it read the stored hash before asking the reorg detector); see the summary comment, item 3.
|
Claude finished @arnaubennassar's task in 1m 2s —— View job Review of PR #1921 (
|
|
Thanks for the thorough review. Status of each point (pushed in 0734796, 500cd10, c2283f2 and 239d814):
Optional ones: the |
Report whether the block of each item is finalized on its network in /bridges, /claims, /injected-l1-info-leaf, /root-by-ler and /settlements (plus the bridge/claim lookups sharing the same response types). The field is a nullable bool omitted when the finalized block cannot be determined, keeping compatibility with previous versions. Fixes #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add L1InfoTreeSync.IsBlockFinalized, backed by the multidownloader's stored IsFinal flag (which is only set once the block hash was validated), and use it in the bridge service for the L1 blocks of /settlements and /injected-l1-info-leaf instead of comparing block numbers against the finalized block. Legacy and read-only syncers report it as unknown, so block_finalized is omitted. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… syncer Add BridgeSync.IsBlockFinalized(num, hash). The legacy syncer has no finality flag, but the reorg detector only stops tracking a block once its hash matched the canonical chain below the finalized block, so a processed block with the given hash that is no longer tracked is finalized. The hash read together with the item is passed so a block replaced after the read is reported as unknown. Bridge and tree.Root now carry the stored BlockHash (not persisted, filled by the read queries). The bridge service uses the syncer finality for /bridges, /bridge-by-deposit-count, /bridges-by-content and /root-by-ler, and drops the block number comparison against the finalized block. Claims and the L2 side of /injected-l1-info-leaf omit block_finalized until their syncers report it. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add ClaimSync.IsBlockFinalized(num, hash), with the same rule as the bridge syncer: a processed block with the given hash that the reorg detector no longer tracks is finalized, since it only stops tracking it after matching its hash against the canonical chain below the finalized block. Claim now carries the stored BlockHash (not persisted, filled by the paged and by-GER queries). The bridge service uses the claim syncer of the network for /claims and /claims-by-ger. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e syncer Add L2GERSync.IsBlockFinalized(num, hash), with the same rule as the bridge and claim syncers (a processed block with the given hash that the reorg detector no longer tracks is finalized). It reports unknown for chains synced in Legacy mode, where the stored block number is the polling head and not the real injection block. GlobalExitRootInfo now carries the stored BlockHash (not persisted, filled by GetFirstGERAfterL1InfoTreeIndex). For an L2 lookup, /injected-l1-info-leaf reports block_finalized only if both the L1 leaf block and the L2 injection block are finalized, false as soon as one is known not to be, and omits it otherwise. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Apply the same pattern as the other legacy syncers to L1InfoTreeSync: a processed block with the given hash that the reorg detector no longer tracks is finalized. IsBlockFinalized now takes the block hash read together with the item: the legacy path requires it, the multidownloader path (IsFinal flag) checks it when given. L1InfoTreeLeaf carries the stored BlockHash (not persisted nor serialized, filled by L1InfoTreeSync.GetInfoByIndex) and the settlements use their joined block hash, so /injected-l1-info-leaf and /settlements report block_finalized in both modes. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add bridgeservice.CurrentAPIRevision (revision.go, starting at 1, with the bump rule and revision history, like the bridge tracker's) and return it as api_revision in GET / and GET /health, in every sync_status case. It is purely informational and independent of the bridge tracker's revision. Update the swagger annotations, the health check section of docs/bridge_service.md and add tests: handler response (every sync_status), client round trip, the revision history staying in sync with the constant, and the finder's health checker ignoring the response body (with and without the new field). Closes #1920 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add bridgeservice/docs/API_REVISIONS.md with how to read api_revision, when to bump it and the changes of every revision. TestCurrentAPIRevisionHasHistoryEntry now also checks that the latest entry of that file (and of revision.go) is the current revision, so the bump can't be forgotten. Refs #1920 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… finality The multidownloader stores one block per number, so without the hash of the block the caller read, IsFinal could belong to the block that replaced it after a reorg. A zero hash (rows written before the block table had a hash, or a missing hash) now reports unknown in both modes, so block_finalized is omitted. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The block_finalized flag is computed from the block hash read together with each item (bridge, claim, L1 info tree leaf, injected GER, exit tree root). Until now that hash was filled by a second query on the block table after reading the rows. A reorg processed between both reads, with the replacement block already inserted, made an orphaned row get the hash of the block that replaced its own, so IsBlockFinalized validated the wrong block. Read the hash with a LEFT JOIN on the block table in the same SELECT (one statement, one snapshot) in GetBridgesPaged, GetBridgeByDepositCount and GetBridgesByContent (bridge and bridge_archive), BridgeSync.GetRootByLER, GetClaimsPaged and GetClaimsByGER, L1InfoTreeSync.GetInfoByIndex and GetFirstGERAfterL1InfoTreeIndex. Rows are scanned into unexported read models with a block_hash column and converted to the public types, which keep BlockHash out of meddler so inserts are unchanged. The read models have the same fields as the public types, so the conversion only compiles while both match, and a test checks their meddler columns. Remove the fillBlockHashes/FillBlockHashes helpers, now unused. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…losed on unknown subscribers IsBlockFinalized read the syncer's stored hash first and asked the reorg detector afterwards. On a reorg the detector notifies the syncer (which deletes the orphaned blocks) and only then stops tracking them, so a full reorg completing between the two reads made an orphaned block look finalized. Add reorgdetector.IsBlockFinalized, shared by BridgeSync, ClaimSync, L2GERSync and L1InfoTreeSync (legacy), which asks the tracker first and compares the stored hash afterwards. GetTrackedBlockByBlockNumber returned db.ErrNotFound both for an untracked block and for an unknown subscriber id, so a wrong id reported every block as finalized. It now returns the new ErrSubscriberNotFound for the latter, and IsBlockFinalized turns it into an error. Refs #1915 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ision bump rule
Reword the BlockFinalized comment on the response types ("omitted when the syncer that
served the item cannot determine it"). In API_REVISIONS.md, drop the exemption for
optional fields from the bump rule (it contradicted the rule of the issue and the bridge
tracker precedent, where adding an optional field bumped the revision), and state in the
revision 1 entry that instances without api_revision predate the contract and have none
of its fields, block_finalized included.
Refs #1915
Refs #1920
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
239d814 to
44e0740
Compare
🔄 Changes Summary
This PR implements two bridge service issues: #1915 (
block_finalized) and #1920 (api_revision).#1915 —
block_finalizedblock_finalized(true|false| omitted) to the bridge service responses tied to a block, so clients know whether a response can still be affected by a reorg.IsBlockFinalized(blockNum, blockHash), using the block hash read together with the item:IsFinalflag stored by the multidownloader, which is only set once the block was validated (the stored header must have the given hash).reorgdetector.IsBlockFinalized, which asks the tracker first and reads the stored hash afterwards: the reorg detector only drops a reorged block from its tracker after the syncer has deleted it, so with that order an orphaned block can never be reported as finalized. An unknown subscriber id is an error (reorgdetector.ErrSubscriberNotFound), never "finalized".Bridge,Claim,GlobalExitRootInfo,L1InfoTreeLeafandtree.Rootcarry the storedBlockHash(meddler:"-", not persisted). It is read in the sameSELECTas the rows (LEFT JOIN block, through internal read models), so it is the hash of the block the item belonged to when it was read, even if a reorg is processed right after.#1920 —
api_revisionin the health endpointGET /andGET /healthreturnapi_revision(HealthCheckResponse.APIRevision), backed bybridgeservice.CurrentAPIRevision(bridgeservice/revision.go), starting at1. Same mechanism as the bridge tracker's, but an independent counter.sync_status(the endpoint still answers 200).bridgeservice/docs/API_REVISIONS.md(also linked fromdocs/bridge_service.md).TestCurrentAPIRevisionHasHistoryEntryfails if the constant, the history inrevision.goand that file get out of sync.bridgeservicefinder's health checker only looks at the status code: a test covers a body with and withoutapi_revision(and with unknown future fields).🔌 API Updates
🔌 Bridge service API
🔌 Bridge service API Update:
api_revisionadded toGET /andGET /health(feat: add "api_revision" to aggkit bridge service #1920), andblock_finalizedadded to (feat: bridge service: add field block_finalized [true|false] into several end-points #1915):/bridge/v1/bridges,/bridge-by-deposit-count,/bridges-by-content(BridgeSync)/bridge/v1/claims,/claims-by-ger(ClaimSync)/bridge/v1/root-by-ler(BridgeSync)/bridge/v1/settlements(L1InfoTreeSync)/bridge/v1/injected-l1-info-leaf: for an L1 lookup the L1 leaf block (L1InfoTreeSync); for an L2 lookup both the L1 leaf block and the L2 injection block (L2GERSync) must be finalized (falseas soon as one is known not to be, omitted if unknown)Not a breaking change.
✅ Testing
reorgdetector.IsBlockFinalizedwith the real reorg detector and a deterministic test of the call order, handler tests ofapi_revisionin everysync_statuscase, the client round trip, the finder's health checker, and unit tests forL1InfoTreeSync,BridgeSync,ClaimSyncandL2GERSyncIsBlockFinalized(tracked / not tracked / hash mismatch / no hash / errors), the block hash filling in the queries, and the bridge service handlers (block_finalized_test.go).🐞 Issues
📝 Notes
L2GERSyncin Legacy mode reports it as unknown: the stored block number there is the polling head, not the real injection block.IsBlockFinalizedresults (one primary key read per row today); a batched API would change five interfaces.GetBridgesPaged/GetClaimsPagedare still two statements, and bridge / bridge_archive are read in two statements (independent of the block hash).L1InfoTreeSync.GetInfoByGlobalExitRootdoesn't fill the leafBlockHash(noctx, and no endpoint reportingblock_finalizeduses it).🤖 Generated with Claude Code