Set hardcoded end block for mainnet27 #945
Conversation
peterargue
requested review from
janezpodhostnik,
m-Peter and
zhangchiqing
as code owners
📝 WalkthroughWalkthroughAdded hardcoded Mainnet 27 spork boundary constants and per-spork range handling; spork clients may override lastHeight for Mainnet 27; latest-height lookups and event subscriptions clamp or skip events beyond the hardcoded Mainnet 27 last height. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sub as EventSubscriber
participant Cross as CrossSporkClient
participant Up as UpstreamNode
participant Ingest as IngestPipeline
Note over Sub: Subscription start
Sub->>Cross: SubscribeEventsByBlockHeight(spork, startHeight)
Cross->>Up: GetLatestBlockHeader()
Up-->>Cross: latestHeader(height, root)
alt root == HardcodedMainnet27SporkRootHeight
note right of Cross `#a8e6cf`: clamp reported/latest height to HardcodedMainnet27LastHeight
else normal
note right of Cross: use upstream reported height
end
Cross-->>Sub: stream events (endHeight possibly clamped)
Note over Sub: For each incoming event
Sub->>Cross: GetLatestHeightForSpork()
Cross-->>Sub: lastHeight (may equal HardcodedMainnet27LastHeight)
Sub->>Sub: set stopAtHardcodedLastHeight?
Sub->>Ingest: OnBlockEvent(event)
alt event.Height > HardcodedMainnet27LastHeight and stopAtHardcodedLastHeight
note right of Sub `#ffd3b6`: skip processing (continue loop)
else
Sub->>Ingest: process event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (3)
services/requester/cross-spork_client.go (3)
18-27: TODO: Confirm the final height before release.The hardcoded constants look appropriate for gating the Mainnet 27 hardfork. Ensure
HardcodedMainnet27LastHeightis confirmed and updated to the correct value before merging.Would you like me to open an issue to track confirmation of the final height value?
214-231: Logic is correct; consider addressing linter suggestion.The routing logic correctly returns cached
lastHeightfor past sporks and fetches live data for the current spork.The static analysis tool flags Line 226: "could remove embedded field 'Client' from selector". Using
c.Client.GetLatestBlockHeaderexplicitly is actually clearer here sinceCrossSporkClientdoesn't override this method, but if you want to silence the linter, you could usec.GetLatestBlockHeaderinstead.🔎 Optional: Simplify embedded field access
- block, err := c.Client.GetLatestBlockHeader(ctx, true) + block, err := c.GetLatestBlockHeader(ctx, true)
267-269: TODO: Subscription boundary handling is critical for hardfork gating.The TODO correctly identifies that subscriptions need to terminate at
lastHeightfor past sporks. Without this,SubscribeEventsByBlockHeightcould continue receiving events beyondHardcodedMainnet27LastHeight, undermining the hardfork gating implemented in other methods.Consider whether this should be addressed before merging, or if a follow-up issue should track it.
Would you like me to open an issue to track implementing the subscription boundary handling?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/requester/cross-spork_client.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T10:14:49.706Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
Applied to files:
services/requester/cross-spork_client.go
🧬 Code graph analysis (1)
services/requester/cross-spork_client.go (1)
models/errors/errors.go (1)
NewHeightOutOfRangeError(51-53)
🪛 GitHub Check: Lint
services/requester/cross-spork_client.go
[failure] 226-226:
QF1008: could remove embedded field "Client" from selector (staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
services/requester/cross-spork_client.go (3)
46-49: LGTM!The guard correctly prevents querying heights beyond the spork's boundary. The
startHeightvalidation is appropriately handled bygetClientForHeightat theCrossSporkClientlevel.
72-83: LGTM!The override logic correctly enforces the hardcoded last height when the spork root matches Mainnet 27. The warning log provides good observability when the override is triggered.
85-96: LGTM!The initialization correctly uses the potentially overridden
lastHeightvalue, ensuring consistency between the logged output and the actual spork client configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/requester/cross-spork_client.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T10:14:49.706Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
Applied to files:
services/requester/cross-spork_client.go
🧬 Code graph analysis (1)
services/requester/cross-spork_client.go (1)
models/errors/errors.go (1)
NewHeightOutOfRangeError(51-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
services/requester/cross-spork_client.go (2)
46-50: LGTM! Proper boundary validation.The early validation correctly enforces the hardcoded end height boundary, ensuring that requests for heights beyond
lastHeightare rejected before reaching the upstream API.
214-223: LGTM! Correct approach for past sporks with hardcoded boundaries.Returning the cached
lastHeightfor past sporks is the right approach, especially when using hardcoded end heights. This ensures deterministic results and avoids unnecessary upstream queries for sporks that have ended.
| const ( | ||
| // The following are hardcoded heights used to ensure gateways ignore blocks after the hard fork | ||
| // height even if they are returned by the upstream access node. | ||
|
|
||
| // HardcodedMainnet27SporkRootHeight is the spork root block height (cadence) for the Mainnet 27 network. | ||
| HardcodedMainnet27SporkRootHeight = 130290659 | ||
|
|
||
| // HardcodedMainnet27LastHeight is the hardcoded last block height (cadence) for the Mainnet 27 network. | ||
| HardcodedMainnet27LastHeight = 137363395 // TODO: confirm final height when ready | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to the hardcoded height
echo "=== Searching for height 137363395 ==="
rg -n "137363395" -A2 -B2
echo ""
echo "=== Searching for Mainnet 27 spork references ==="
rg -n -i "mainnet.?27" -A3 -B3
echo ""
echo "=== Checking for TODO comments related to heights ==="
rg -n "TODO.*height" -iRepository: onflow/flow-evm-gateway
Length of output: 3327
Resolve the TODO before merging—hardcoded last height is unconfirmed.
Line 26 contains an unresolved TODO indicating the final height value 137363395 has not been confirmed. This constant is used to enforce spork boundaries and cap data ingestion; merging with an unconfirmed height could cause incorrect gateway behavior if the actual hardfork height differs. Confirm the height is finalized or remove this PR from consideration until that confirmation is available.
🤖 Prompt for AI Agents
In services/requester/cross-spork_client.go around lines 18 to 27, the constant
HardcodedMainnet27LastHeight (137363395) still has an unresolved TODO and must
not be merged unconfirmed; either replace the hardcoded value with the
officially confirmed final height and remove the TODO and comment, or make this
value configurable (env/config entry) with a clear default and runtime check
that logs/warns and prevents enforcement until a confirmed value is provided;
update tests and remove the TODO comment once the confirmed approach is
implemented.
| // ensure that the last height is the expected block. | ||
| if info.SporkRootBlockHeight == HardcodedMainnet27SporkRootHeight && lastHeight != HardcodedMainnet27LastHeight { | ||
| logger.Warn(). | ||
| Uint64("nodeRootHeight", info.NodeRootBlockHeight). | ||
| Uint64("sporkRootHeight", info.SporkRootBlockHeight). | ||
| Uint64("lastHeight", lastHeight). | ||
| Uint64("hardcodedLastHeight", HardcodedMainnet27LastHeight). | ||
| Msg("access node returned unexpected last height. using hardcoded value.") | ||
| lastHeight = HardcodedMainnet27LastHeight | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if chain ID is available and used elsewhere for network identification
rg -n "chainID|ChainID" services/requester/ -A2 -B2
echo "---"
# Look for other network-specific constants or configuration
rg -n "Mainnet|Testnet" services/requester/ -A2 -B2Repository: onflow/flow-evm-gateway
Length of output: 7151
🏁 Script executed:
#!/bin/bash
# Get more context around the NewCrossSporkClient function definition
head -160 services/requester/cross-spork_client.go | tail -20
echo "---"
# Check the struct definition to see if chainID is stored
rg -n "type CrossSporkClient struct" services/requester/cross-spork_client.go -A15
echo "---"
# Look for other spork-related overrides or network checks in the file
rg -n "Hardcoded|Spork" services/requester/cross-spork_client.goRepository: onflow/flow-evm-gateway
Length of output: 3495
🏁 Script executed:
#!/bin/bash
# Find the addSpork function definition and see its signature
rg -n "func.*addSpork" services/requester/cross-spork_client.go -A20
echo "---"
# Get the full context around lines 74-83 to see which function contains this logic
sed -n '60,90p' services/requester/cross-spork_client.goRepository: onflow/flow-evm-gateway
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Check where add() method is called from NewCrossSporkClient
sed -n '146,182p' services/requester/cross-spork_client.go
echo "---"
# Check if there are any TODO or FIXME comments related to this in the file
rg -n "TODO|FIXME" services/requester/cross-spork_client.goRepository: onflow/flow-evm-gateway
Length of output: 1294
Pass chainID to the spork verification logic and verify the network before applying the Mainnet 27 override.
The add() method applies a hardcoded override for Mainnet 27 based only on SporkRootBlockHeight == HardcodedMainnet27SporkRootHeight, but chainID is available in NewCrossSporkClient and is not passed to this method. This creates a risk where any network sharing the same spork root height value (130290659) would receive the Mainnet 27 override. Additionally, chainID should be used for proper network verification before applying network-specific behavior.
Refactor to:
- Pass
chainIDto theadd()method - Verify
chainID == flowGo.Mainnetbefore applying the override at lines 74-83 - Consider making spork configuration injectable rather than hardcoded (relates to the TODO comment at line 94)
| // TODO: we need to make sure that the AN ends the subscription at the last block | ||
| return client.SubscribeEventsByBlockHeight(ctx, startHeight, filter, opts...) |
There was a problem hiding this comment.
Critical: Subscription boundary handling is unimplemented.
The TODO correctly identifies that subscriptions may not terminate at the hardcoded lastHeight, allowing events beyond the hardfork boundary to be ingested. Unlike single queries that are validated in GetEventsForHeightRange, long-lived subscriptions could continue streaming data indefinitely.
This defeats the primary purpose of this PR: preventing post-hardfork data ingestion.
Consider these options:
- Implement a wrapper that monitors streamed block heights and closes the subscription when
lastHeightis reached - Pass subscription end height as an option if the upstream AN supports it
- Document this as a known limitation if immediate fix is not feasible, and create a tracking issue
Would you like me to help draft an implementation for option 1, or search for whether the Flow SDK supports subscription end bounds?
#!/bin/bash
# Check if SubscribeOption supports end height or similar bounds
rg -n "SubscribeOption" -A10 -B2 --type go
# Look for existing subscription monitoring or termination logic
rg -n "SubscribeEventsByBlockHeight" services/ingestion/ -A10 -B2
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
services/requester/cross-spork_client.go (2)
18-27: Confirm the hardcoded height before merging.The TODO at line 26 indicates the final height value (
137363395) has not been confirmed. This constant enforces spork boundaries and caps data ingestion - merging with an unconfirmed height could cause incorrect gateway behavior.
72-83: ChainID verification still missing.As previously noted, the override is applied based solely on
SporkRootBlockHeightmatching the hardcoded value. Consider also verifyingchainID == flowGo.Mainnetto prevent accidental application on other networks that might coincidentally share the same spork root height.
🧹 Nitpick comments (3)
services/ingestion/event_subscriber.go (1)
199-201: Addresses subscription boundary handling - consider adding a log statement.This implements the subscription boundary handling noted in the cross-spork client TODO. Using
continuerather thanreturnprevents the gateway from crashing.However, if the upstream AN continues streaming events past the hardcoded height, this will silently discard them in a tight loop. Consider adding a debug/trace log (rate-limited or one-time) to indicate events are being skipped, which would aid in troubleshooting.
🔎 Optional: Add one-time log for skipped events
+var loggedSkip bool if stopAtHardcodedLastHeight && blockEvents.Height > requester.HardcodedMainnet27LastHeight { + if !loggedSkip { + r.logger.Debug(). + Uint64("height", blockEvents.Height). + Uint64("hardcodedLastHeight", requester.HardcodedMainnet27LastHeight). + Msg("skipping events beyond hardcoded last height") + loggedSkip = true + } continue // don't exit, otherwise the gateway will crash }services/requester/cross-spork_client.go (2)
273-275: TODO is now addressed by subscriber-side handling.The subscription boundary concern in this TODO is addressed by the changes in
event_subscriber.go(lines 199-201), which skip events beyondHardcodedMainnet27LastHeight. Consider updating or removing this TODO to reflect that the boundary is enforced on the consumer side.🔎 Suggested TODO update
- // TODO: we need to make sure that the AN ends the subscription at the last block + // Note: Subscription boundary for mainnet27 is enforced in the event subscriber + // by skipping events beyond HardcodedMainnet27LastHeight return client.SubscribeEventsByBlockHeight(ctx, startHeight, filter, opts...)
293-299: Clamping logic is correct; consider explicit error for post-hardfork ranges.The clamping works correctly, but if
startHeight > HardcodedMainnet27LastHeight, the caller receives a confusing "invalid range" error from the underlying API. An explicit check could provide clearer feedback:🔎 Optional: Add explicit check for post-hardfork start height
// if the current spork client is the mainnet 27 network and the requested end height is after the hardcoded last height, // fix it to the hardcoded last height. If the requested start height is after the hardcoded last height, the API // will return an error since the range will be invalid. if c.currentSporkFirstHeight == HardcodedMainnet27SporkRootHeight && endHeight > HardcodedMainnet27LastHeight { + if startHeight > HardcodedMainnet27LastHeight { + return nil, errs.NewHeightOutOfRangeError(startHeight) + } endHeight = HardcodedMainnet27LastHeight }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/ingestion/event_subscriber.goservices/requester/cross-spork_client.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
📚 Learning: 2025-10-06T10:14:49.706Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
Applied to files:
services/ingestion/event_subscriber.goservices/requester/cross-spork_client.go
📚 Learning: 2024-10-17T18:04:04.165Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Applied to files:
services/ingestion/event_subscriber.goservices/requester/cross-spork_client.go
📚 Learning: 2025-11-12T13:21:59.080Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: tests/integration_test.go:692-707
Timestamp: 2025-11-12T13:21:59.080Z
Learning: In the flow-evm-gateway integration tests (tests/integration_test.go), the gateway's RPC server continues to operate independently even after the context passed to bootstrap.Run() is cancelled. Context cancellation affects upstream dependencies (like the Emulator acting as an Access Node) but does not immediately shut down the RPC server, allowing tests to verify backup failover scenarios.
Applied to files:
services/ingestion/event_subscriber.go
📚 Learning: 2025-01-24T20:15:10.908Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Applied to files:
services/requester/cross-spork_client.go
🧬 Code graph analysis (2)
services/ingestion/event_subscriber.go (2)
models/events.go (1)
NewBlockEventsError(252-256)services/requester/cross-spork_client.go (1)
HardcodedMainnet27LastHeight(26-26)
services/requester/cross-spork_client.go (1)
models/errors/errors.go (1)
NewHeightOutOfRangeError(51-53)
🔇 Additional comments (3)
services/ingestion/event_subscriber.go (1)
137-148: LGTM - Proper error handling and hardcoded height detection.The error handling for
GetLatestHeightForSporkis correct, and the flag logic properly detects when we're operating on the mainnet27 spork with the hardcoded boundary. This works becauseGetLatestHeightForSporkclamps the returned height toHardcodedMainnet27LastHeightwhen the upstream reports a higher value for mainnet27.services/requester/cross-spork_client.go (2)
36-39: LGTM - Clean helper method for height range checking.Simple and correct bounds check that improves readability where used.
46-49: Good defensive validation.Early rejection of out-of-range requests prevents unnecessary API calls. The comment correctly notes that the underlying API handles the
startHeight > endHeightcase.
| if c.IsPastSpork(height) { | ||
| for _, spork := range c.sporkClients { | ||
| if spork.contains(height) { | ||
| // use the latest block height captured during bootstrapping. | ||
| return spork.lastHeight, nil | ||
| } | ||
| } | ||
| // otherwise, we don't have a spork client for the provided height. | ||
| return 0, errs.NewHeightOutOfRangeError(height) | ||
| } | ||
|
|
||
| block, err := client.GetLatestBlockHeader(ctx, true) | ||
| // otherwise, return the latest height from the current spork. | ||
| block, err := c.GetLatestBlockHeader(ctx, true) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| // if the current spork client is the mainnet 27 network and node reported a higher height, fix it to the hardcoded last height. | ||
| if c.currentSporkFirstHeight == HardcodedMainnet27SporkRootHeight && block.Height > HardcodedMainnet27LastHeight { | ||
| return HardcodedMainnet27LastHeight, nil | ||
| } | ||
|
|
||
| return block.Height, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/requester/cross-spork_client.go | head -250Repository: onflow/flow-evm-gateway
Length of output: 9898
🏁 Script executed:
# Check all usages of SporkRootBlockHeight and NodeRootBlockHeight
rg -n "SporkRootBlockHeight|NodeRootBlockHeight" services/requester/cross-spork_client.go
echo "---"
# Check the node version info struct to understand the fields
rg -n "SporkRootBlockHeight|NodeRootBlockHeight" . -t go --max-count=20 | head -40Repository: onflow/flow-evm-gateway
Length of output: 1180
🏁 Script executed:
# Search for documentation or comments about SporkRootBlockHeight vs NodeRootBlockHeight
rg -i "spork.*root|node.*root" services/ -B2 -A2 --max-count=30
echo "---"
# Check if there's any test or validation that shows these should be equal
rg -n "SporkRootBlockHeight.*NodeRootBlockHeight|NodeRootBlockHeight.*SporkRootBlockHeight" . -t goRepository: onflow/flow-evm-gateway
Length of output: 4523
🏁 Script executed:
# Check if there are any tests that show what the expected relationship is
rg -n "HardcodedMainnet27SporkRootHeight" . -t go -B3 -A3
echo "---"
# Look for documentation or comments that explain the difference between these fields
rg -n "NodeRootBlockHeight|SporkRootBlockHeight" . -t go -B1 -A1 | head -60Repository: onflow/flow-evm-gateway
Length of output: 4517
🏁 Script executed:
# Check the flow-go-sdk to understand the difference between these two fields
web_search "Flow SDK NodeRootBlockHeight SporkRootBlockHeight difference"
echo "---"
# Also check if there's any relationship or documentation in commit history
git log --oneline --all -- services/requester/cross-spork_client.go | head -20Repository: onflow/flow-evm-gateway
Length of output: 850
🏁 Script executed:
# Look for any comments or documentation about the purpose of these fields
rg -n "NodeRootBlockHeight|SporkRootBlockHeight" . -t go --max-count=50 | grep -i "//\|comment\|doc" || echo "No explicit documentation found"
echo "---"
# Check what the actual difference might be in the network
rg -n "NodeRootBlockHeight" . -t go -B2 -A2 | head -40Repository: onflow/flow-evm-gateway
Length of output: 2069
🏁 Script executed:
# Let me check the exact commit that introduced this check to understand the intent
git log --all -p -- services/requester/cross-spork_client.go | grep -A20 -B5 "HardcodedMainnet27SporkRootHeight" | head -60Repository: onflow/flow-evm-gateway
Length of output: 2312
🏁 Script executed:
# Let's check if there are any tests or other code patterns that show when these might differ
rg -n "currentSporkFirstHeight" services/requester/cross-spork_client.go -B2 -A2
echo "---"
# Check if there's usage in GetEventsForHeightRange method that shows the pattern
rg -n "GetEventsForHeightRange" services/requester/cross-spork_client.go -A15 | head -40Repository: onflow/flow-evm-gateway
Length of output: 2800
🏁 Script executed:
# Let's verify the current state by looking at all three locations where we check for Mainnet27
cat -n services/requester/cross-spork_client.go | sed -n '74,76p; 230,234p; 296,299p'Repository: onflow/flow-evm-gateway
Length of output: 847
Use consistent field for detecting Mainnet27 configuration.
Line 75 in add() checks info.SporkRootBlockHeight == HardcodedMainnet27SporkRootHeight, but lines 232 and 297 check c.currentSporkFirstHeight == HardcodedMainnet27SporkRootHeight where currentSporkFirstHeight is derived from info.NodeRootBlockHeight. Since both fields are logged separately (lines 77-78), they can potentially differ. For the current spork, use info.SporkRootBlockHeight to be consistent with past spork detection in add(), or document why the current spork detection uses NodeRootBlockHeight instead.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
services/requester/cross-spork_client.go (4)
238-242: Clamping logic depends on fixing IsMainnet27Client().The clamping at lines 240-241 relies on
IsMainnet27Client()which has a logic inconsistency (see comment on lines 188-193). Until that detection method is corrected, this clamping may apply incorrectly or fail to apply when needed.Additionally, as noted in previous review: ensure consistent use of
SporkRootBlockHeightvsNodeRootBlockHeightfor mainnet27 detection across all methods.
271-283: Subscription boundary enforcement remains unimplemented.As noted in previous review, subscriptions do not terminate at the hardcoded
lastHeightboundary. UnlikeGetEventsForHeightRangewhich validates and clamps height ranges, long-lived subscriptions can continue streaming events beyond the hardfork boundary, potentially ingesting data afterHardcodedMainnet27LastHeight.This undermines the primary purpose of this PR: preventing post-hardfork data ingestion.
18-27: Unconfirmed hardcoded height blocks merge readiness.The TODO on line 26 indicates the final height value has not been confirmed. Merging with an unconfirmed height constant that gates data ingestion could cause incorrect gateway behavior if the actual hardfork height differs.
74-83: Mainnet27 detection lacks network verification and uses inconsistent field.Line 75 checks
info.SporkRootBlockHeight == HardcodedMainnet27SporkRootHeightwithout verifying the network chainID. Additionally, this differs fromIsMainnet27Client()(lines 188-193) which usescurrentSporkFirstHeight(derived fromNodeRootBlockHeight) with a range check rather than an exact spork root match. This inconsistency means the two detection mechanisms can disagree about whether the client is on mainnet27.As noted in previous review: pass and verify chainID in this method to ensure the override only applies to Flow Mainnet.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/requester/cross-spork_client.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
📚 Learning: 2024-10-17T18:04:04.165Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Applied to files:
services/requester/cross-spork_client.go
📚 Learning: 2025-10-06T10:14:49.706Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
Applied to files:
services/requester/cross-spork_client.go
📚 Learning: 2025-01-24T20:15:10.908Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Applied to files:
services/requester/cross-spork_client.go
🧬 Code graph analysis (1)
services/requester/cross-spork_client.go (1)
models/errors/errors.go (1)
NewHeightOutOfRangeError(51-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
services/requester/cross-spork_client.go (2)
46-49: LGTM: Spork boundary validation is correct.The endHeight validation prevents fetching events beyond the spork's lastHeight boundary, which correctly enforces the mainnet27 hardfork limit when lastHeight is overridden.
301-306: Clamping approach is sound but depends on IsMainnet27Client() fix.The endHeight clamping to
HardcodedMainnet27LastHeightcorrectly prevents fetching events beyond the hardfork boundary. The approach is consistent withGetLatestHeightForSporkand provides defense-in-depth with the validation insporkClient.GetEventsForHeightRange(lines 46-49).However, this relies on
IsMainnet27Client()returning correct results. Once the logic inconsistency in that method (lines 188-193) is resolved, this implementation should function as intended.
| // IsMainnet27Client returns true if the current spork client is for the mainnet 27 network. | ||
| func (c *CrossSporkClient) IsMainnet27Client() bool { | ||
| // currentSporkFirstHeight is the node's root block, which is not the same as the spork root block if the node was | ||
| // bootstrapped after the spork. This check works for nodes bootstrapped during or after the mainnet 27 spork. | ||
| return c.currentSporkFirstHeight >= HardcodedMainnet27SporkRootHeight && c.currentSporkFirstHeight <= HardcodedMainnet27LastHeight | ||
| } |
There was a problem hiding this comment.
IsMainnet27Client() uses inconsistent detection logic that may return incorrect results.
This method uses a range check on currentSporkFirstHeight (derived from NodeRootBlockHeight), but the override logic in add() (line 75) uses an exact match on SporkRootBlockHeight. Per the comment on line 190, these fields can differ if the node was bootstrapped after the spork started.
Scenario illustrating the issue:
- Node bootstrapped at height 135000000 (after spork root 130290659 but before last height 137363395)
currentSporkFirstHeight= 135000000 (fromNodeRootBlockHeight)SporkRootBlockHeight= 130290659 (actual spork boundary)IsMainnet27Client()returnstrue(135000000 is in range)- But
add()override at line 75 does not trigger (135000000 ≠ 130290659) - Result: Clamping is applied in
GetLatestHeightForSporkandGetEventsForHeightRangeeven thoughlastHeightwas never overridden
Recommendation: Use SporkRootBlockHeight consistently for mainnet27 detection. Either:
- Pass and store
SporkRootBlockHeightinCrossSporkClientand check it here, or - Change the detection to match spork clients by checking if any spork in
sporkClientshasfirstHeight == HardcodedMainnet27SporkRootHeight
🤖 Prompt for AI Agents
In services/requester/cross-spork_client.go around lines 188-193,
IsMainnet27Client uses currentSporkFirstHeight (NodeRootBlockHeight) for
detection which is inconsistent with add() override that checks
SporkRootBlockHeight; update detection to use the actual spork root value
instead: add a SporkRootBlockHeight field to CrossSporkClient (or ensure it is
passed/stored when clients are constructed), set it where add() currently
determines overrides, and change IsMainnet27Client to compare
SporkRootBlockHeight against HardcodedMainnet27SporkRootHeight/LastHeight (or
alternatively iterate sporkClients to find any with firstHeight ==
HardcodedMainnet27SporkRootHeight); ensure tests/usage updated accordingly so
mainnet27 detection uses the spork root height consistently.
There was a problem hiding this comment.
this is OK. these 2 code paths are for different node types (current vs past).
There was a problem hiding this comment.
@peterargue, thank you for the clarification! You're right—I missed that distinction. The IsMainnet27Client() method checks the current spork using currentSporkFirstHeight, while the add() override applies to past spork clients using SporkRootBlockHeight. That design makes sense given that past sporks are added through the add() method where SporkRootBlockHeight is available, while the current spork detection relies on the NodeRootBlockHeight stored in currentSporkFirstHeight. I appreciate the explanation!
🧠 Learnings used
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/run/cmd.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.706Z
Learning: In services/ingestion/event_subscriber.go, when reconnecting after disconnect errors (DeadlineExceeded, Internal, Unavailable), the subscription should reconnect at lastReceivedHeight rather than lastReceivedHeight+1. This avoids errors when the next height doesn't exist yet, and duplicate event processing is safe because the ingestion engine is explicitly designed to be idempotent (storage uses batch.Set() which overwrites existing entries).
🪛 GitHub Check: Lint
cmd/run/cmd.go
[failure] 301-301:
Error return value of (*github.com/spf13/pflag.FlagSet).MarkHidden is not checked (errcheck)
[failure] 300-300:
Error return value of (*github.com/spf13/pflag.FlagSet).MarkHidden is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
| // soft finality specific flags. include them as disabled to allow operators to run this image without any config changes | ||
| var experimentalSoftFinalityEnabled, experimentalSealingVerificationEnabled bool | ||
| Cmd.Flags().BoolVar(&experimentalSoftFinalityEnabled, "experimental-soft-finality-enabled", false, "feature disabled in this release") | ||
| Cmd.Flags().BoolVar(&experimentalSealingVerificationEnabled, "experimental-sealing-verification-enabled", false, "feature disabled in this release") | ||
| Cmd.Flags().MarkHidden("experimental-soft-finality-enabled") | ||
| Cmd.Flags().MarkHidden("experimental-sealing-verification-enabled") | ||
|
|
There was a problem hiding this comment.
Check error returns from MarkHidden for consistency.
The error returns from MarkHidden on lines 300-301 are not checked, which is inconsistent with the pattern established on line 303 where MarkDeprecated's error is properly handled.
🔎 Proposed fix to check error returns
// soft finality specific flags. include them as disabled to allow operators to run this image without any config changes
var experimentalSoftFinalityEnabled, experimentalSealingVerificationEnabled bool
Cmd.Flags().BoolVar(&experimentalSoftFinalityEnabled, "experimental-soft-finality-enabled", false, "feature disabled in this release")
Cmd.Flags().BoolVar(&experimentalSealingVerificationEnabled, "experimental-sealing-verification-enabled", false, "feature disabled in this release")
- Cmd.Flags().MarkHidden("experimental-soft-finality-enabled")
- Cmd.Flags().MarkHidden("experimental-sealing-verification-enabled")
+ if err := Cmd.Flags().MarkHidden("experimental-soft-finality-enabled"); err != nil {
+ panic(err)
+ }
+ if err := Cmd.Flags().MarkHidden("experimental-sealing-verification-enabled"); err != nil {
+ panic(err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // soft finality specific flags. include them as disabled to allow operators to run this image without any config changes | |
| var experimentalSoftFinalityEnabled, experimentalSealingVerificationEnabled bool | |
| Cmd.Flags().BoolVar(&experimentalSoftFinalityEnabled, "experimental-soft-finality-enabled", false, "feature disabled in this release") | |
| Cmd.Flags().BoolVar(&experimentalSealingVerificationEnabled, "experimental-sealing-verification-enabled", false, "feature disabled in this release") | |
| Cmd.Flags().MarkHidden("experimental-soft-finality-enabled") | |
| Cmd.Flags().MarkHidden("experimental-sealing-verification-enabled") | |
| // soft finality specific flags. include them as disabled to allow operators to run this image without any config changes | |
| var experimentalSoftFinalityEnabled, experimentalSealingVerificationEnabled bool | |
| Cmd.Flags().BoolVar(&experimentalSoftFinalityEnabled, "experimental-soft-finality-enabled", false, "feature disabled in this release") | |
| Cmd.Flags().BoolVar(&experimentalSealingVerificationEnabled, "experimental-sealing-verification-enabled", false, "feature disabled in this release") | |
| if err := Cmd.Flags().MarkHidden("experimental-soft-finality-enabled"); err != nil { | |
| panic(err) | |
| } | |
| if err := Cmd.Flags().MarkHidden("experimental-sealing-verification-enabled"); err != nil { | |
| panic(err) | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 301-301:
Error return value of (*github.com/spf13/pflag.FlagSet).MarkHidden is not checked (errcheck)
[failure] 300-300:
Error return value of (*github.com/spf13/pflag.FlagSet).MarkHidden is not checked (errcheck)
🤖 Prompt for AI Agents
In cmd/run/cmd.go around lines 296 to 302, the two calls to
Cmd.Flags().MarkHidden(...) do not check their returned errors (inconsistent
with the subsequent MarkDeprecated check on line 303); update the two MarkHidden
calls to capture their error returns and handle them the same way as the
MarkDeprecated call (i.e., check if err != nil and return the error up the call
stack or handle via the same error-handling path used for MarkDeprecated) so all
flag metadata calls consistently validate and propagate errors.
|
Is this something we already deployed? do we still need it? |
|
@janezpodhostnik I think this is no longer necessary. It was only needed for the roll-back approach, but since we didn't go down that path, we no longer need this. |
| HardcodedMainnet27LastHeight = 137363395 // TODO: confirm final height when ready | ||
| ) | ||
|
|
||
| type sporkClient struct { |
There was a problem hiding this comment.
// a sporkClient can get data only for heights within the range of the spork defined
// by firstHeight and lastHeight (inclusive)
| getEventsForHeightRangeLimiter ratelimit.Limiter | ||
| } | ||
|
|
||
| // contains checks if the provided height is withing the range of available heights |
There was a problem hiding this comment.
| // contains checks if the provided height is within the range of available heights |
| s.getEventsForHeightRangeLimiter.Take() | ||
|
|
||
| // simplified check since the API will reject requests for startHeight > endHeight. | ||
| if endHeight > s.lastHeight { |
There was a problem hiding this comment.
why not checkingif !(s.contains(startHeight) && s.contains(endHeight))?
I think s.client.GetEventsForHeightRange can only fetch for heights within the range of the spork, no?
| lastHeight := header.Height | ||
|
|
||
| // ensure that the last height is the expected block. | ||
| if info.SporkRootBlockHeight == HardcodedMainnet27SporkRootHeight && lastHeight != HardcodedMainnet27LastHeight { |
There was a problem hiding this comment.
We defined the HardcodedMainnet27SporkRootHeight const, which implies the mainnet27 already exists.
In that case, we need to check two things:
- the crossSporkClient has have mainnet27 spork client
- when mainnet27 spork client is added, the root height and last height must match with the hardcoded const.
It seems we only did for 2, and we didn't do 1. Maybe add 1 as well?
| // HardcodedMainnet27SporkRootHeight is the spork root block height (cadence) for the Mainnet 27 network. | ||
| HardcodedMainnet27SporkRootHeight = 130290659 |
There was a problem hiding this comment.
The mainnet27 root height is always the same, no need to mention hardcoded or anything.
| // HardcodedMainnet27SporkRootHeight is the spork root block height (cadence) for the Mainnet 27 network. | |
| HardcodedMainnet27SporkRootHeight = 130290659 | |
| // Mainnet27SporkRootHeight is the spork root block height (cadence) for the Mainnet 27 network. | |
| Mainnet27SporkRootHeight = 130290659 |
| // The following are hardcoded heights used to ensure gateways ignore blocks after the hard fork | ||
| // height even if they are returned by the upstream access node. |
There was a problem hiding this comment.
| // The following are hardcoded heights used to ensure gateways ignore blocks after the hard fork | |
| // height even if they are returned by the upstream access node. | |
| // The following heights define the canonical Mainnet 27 spork, | |
| // chosen to disambiguate between the 2 Mainnet 27 network variants. |
| // HardcodedMainnet27LastHeight is the hardcoded last block height (cadence) for the Mainnet 27 network. | ||
| HardcodedMainnet27LastHeight = 137363395 // TODO: confirm final height when ready |
There was a problem hiding this comment.
I think canonical or selected would be more clear than hardcoded.
I also updated with the final height.
| // HardcodedMainnet27LastHeight is the hardcoded last block height (cadence) for the Mainnet 27 network. | |
| HardcodedMainnet27LastHeight = 137363395 // TODO: confirm final height when ready | |
| // CanonicalMainnet27LastHeight is the final block height (cadence) | |
| // for the canonical Mainnet 27 network. | |
| CanonicalMainnet27LastHeight = 137390145 |
| HardcodedMainnet27LastHeight = 137363395 // TODO: confirm final height when ready | ||
| ) | ||
|
|
||
| type sporkClient struct { |
There was a problem hiding this comment.
maybe rename it to pastSporkClient to be clear that it should not have current spork, which does not have the last height.
| return s.client.Close() | ||
| } | ||
|
|
||
| type sporkClients []*sporkClient |
There was a problem hiding this comment.
likewise, maybe rename to pastSporkClients
|
|
||
| // if the current spork client is the mainnet 27 network and node reported a higher height, fix it to the hardcoded last height. | ||
| // allow the case where the AN is behind the last height in case it is catching up. | ||
| if c.IsMainnet27Client() && block.Height > HardcodedMainnet27LastHeight { |
There was a problem hiding this comment.
If c.GetLatestBlockHeader might return incorrect block.Height, we could fix at the root by wrapping the mainnet27 client, and correcting GetLatestBlockHeader, eliminating the need for this check and the IsMainnet27Client method.
Closes: #???
Description
Set the last block height for the mainnet 27 network to ensure data from after the end height is ignored.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.