Merged
Conversation
|
We're building your pull request over on Zeet. |
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPServer as HTTP Server<br/>(reorg-api)
participant Redis
participant ClickHouse
participant RPC
participant Kafka
Client->>HTTPServer: POST /v1/reorg/publish<br/>(chain_id, block_numbers)
HTTPServer->>HTTPServer: Validate & deduplicate blocks
HTTPServer->>Redis: Get max processed block<br/>(idempotency cursor)
Redis-->>HTTPServer: cursor value
HTTPServer->>HTTPServer: Filter out processed blocks
HTTPServer->>HTTPServer: Split into batches
loop For each batch
HTTPServer->>ClickHouse: Query existing block data<br/>by block numbers
ClickHouse-->>HTTPServer: Block data with transactions/logs/traces
HTTPServer->>RPC: Fetch canonical blocks<br/>(with retries)
RPC-->>HTTPServer: Block data
HTTPServer->>HTTPServer: Validate & match RPC ordering
HTTPServer->>Kafka: Publish reorg payloads
Kafka-->>HTTPServer: Ack
HTTPServer->>Redis: Update max processed block
Redis-->>HTTPServer: Success
end
HTTPServer-->>Client: JSON response<br/>(status, blocks processed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/config.go`:
- Around line 98-99: After parsing environment into the config struct, validate
that ReorgAPIClickhouseBatchSize is > 0 (since it's a uint64 check for != 0) and
reject the config if it's zero by returning an error (or exiting) with a clear
message like "REORG_API_CLICKHOUSE_BATCH_SIZE must be a positive integer";
update the config-loading function that constructs the config (e.g., the
function that parses env into the struct) to perform this check and propagate
the error so startup fails fast instead of allowing downstream
divide-by-zero/infinite-loop behavior.
- Around line 94-97: The Reorg API is currently defaulting ReorgAPIListenAddr to
":8080" and allowing ReorgAPIKey to be empty which exposes an unauthenticated
mutating endpoint; update the configuration so either the default bind is
loopback (change ReorgAPIListenAddr envDefault to "127.0.0.1:8080") or add a
startup validation that returns an error and aborts if ReorgAPIKey is empty
while ReorgAPIListenAddr is not bound to localhost—implement this check in the
config initialization/validation path that constructs/validates the config
(reference ReorgAPIListenAddr and ReorgAPIKey) and fail fast with a clear error
message when the combination would allow unauthenticated external access.
In `@internal/libs/clickhouse.go`:
- Around line 337-355: Replace the un-cancelable errgroup usage (g :=
new(errgroup.Group)) with errgroup.WithContext so the group is tied to the
request context, and pass that derived ctx into the four concurrent callers (the
goroutines that call queryBlocksByBlockNumbers, queryTransactionsByBlockNumbers,
queryLogsByBlockNumbers, queryTracesByBlockNumbers) as well as into the
underlying query execution function (execQueryV2 / the query helpers) so that
any cancellation or error cancels the remaining ClickHouse scans; ensure each
helper accepts and uses the context for its FINAL scans.
In `@internal/libs/libblockdata/getblockdata.go`:
- Around line 342-355: The code assumes rpcResults preserve the order of
blockNumbers; instead ensure blocks are matched by their returned block number:
after building blockData from rpcResults (symbols: rpcResults, blockData,
common.BlockData), build a map from the block's number (e.g.,
block.Header.Number or whatever field holds the block number) to the
*common.BlockData, then iterate the original blockNumbers slice and for each
expected number pick the corresponding entry from the map (error if missing or
duplicate) before calling Validate; replace the current index-based trust and
return a clear error when an expected block number is absent or when there are
extra/unmatched RPC results.
In `@internal/libs/redis.go`:
- Around line 55-76: The current max-cursor approach in
GetReorgAPIMaxProcessedBlock/SetReorgAPIMaxProcessedBlock/ClearReorgAPIMaxProcessedBlock
is unsafe for non-contiguous or out-of-order block lists; replace the single max
value with per-block idempotency tracking in Redis (e.g., a Redis SET or bitmap
keyed by RedisReorgAPIMaxProcessedBlock:<chainID>) so you can check membership
for an individual block rather than comparing to a max. Update the API
functions: add functions like IsBlockProcessed(chainID, blockNumber) that use
SIsMember or GETBIT, MarkBlockProcessed(chainID, blockNumber) that uses SAdd or
SETBIT, and optionally UnmarkBlock/ResetChain that uses SRem/DEL; keep
RedisClient and the existing RedisReorgAPIMaxProcessedBlock base key name for
locating the keys and migrate or retire the old max-based functions accordingly.
In `@internal/reorgapi/server.go`:
- Around line 123-128: The current filtering that builds work by comparing each
bn in sorted against lastPublishedMaxBlock breaks idempotency for sparse/manual
ranges (e.g., submitting [100,200] then 150 gets dropped); update the logic in
server.go around the work construction (the loop using sorted and
lastPublishedMaxBlock) to either validate incoming ranges are contiguous before
accepting (reject non-contiguous requests in the request handler) or replace the
single lastPublishedMaxBlock cursor with explicit tracking (e.g., a
processedBlocks set or persisted batch IDs) and change the membership check to
consult that set instead of comparing to lastPublishedMaxBlock so
previously-unpublished blocks are not erroneously skipped. Ensure any
persistence/lookup uses the existing functions that read/write publication state
so behavior remains durable.
- Around line 168-172: The ClickHouse query failure handled after calling
libs.GetBlockDataFromClickHouseForBlockNumbers should return a 500 (internal
server error) instead of 400 and must not expose err.Error() to the client;
update the error handling where chunkOld is fetched (the block that logs via
log.Error().Err(err).Msg("manual reorg: clickhouse") and currently calls
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})) to log the detailed
err internally but respond with c.JSON(http.StatusInternalServerError,
gin.H{"error": "internal server error"}) or a short generic message like "failed
to query clickhouse".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a29d6c23-ecee-432b-9ccd-875b7c8969fb
📒 Files selected for processing (9)
cmd/reorgapi.gocmd/root.goconfigs/config.gointernal/libs/clickhouse.gointernal/libs/kafka.gointernal/libs/libblockdata/getblockdata.gointernal/libs/redis.gointernal/reorgapi/server.gointernal/storage/kafka_publisher.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
reorg-apiCLI command for manual reorg batch publishing/healthand/v1/reorg/publishfor reorg operationsConfiguration
REORG_API_LISTEN_ADDR: Server listen address (default::8080)REORG_API_KEY: Optional API key for authorizationREORG_API_CLICKHOUSE_BATCH_SIZE: Batch processing size (default: 10)