Skip to content

Always re-index latest Cadence block - #776

Merged
m-Peter merged 1 commit into
mainfrom
mpeter/reindex-latest-cadence-block
Oct 9, 2025
Merged

Always re-index latest Cadence block#776
m-Peter merged 1 commit into
mainfrom
mpeter/reindex-latest-cadence-block

Conversation

@m-Peter

@m-Peter m-Peter commented Mar 10, 2025

Copy link
Copy Markdown
Collaborator

Closes: #766

Description

Now that we have merged:

I believe that we can always subscribe to EVM events by using the latest Cadence block. There's been reports of missing EVM blocks, and it seems this behavior manifests after restarts of the EVM GW node.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Summary by CodeRabbit

  • Refactor
    • Event ingestion startup simplified to begin from the latest block height without environment-specific adjustments, leading to a cleaner initialization path and more consistent, predictable event handling across environments.

@coderabbitai

coderabbitai Bot commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Removed the emulator-specific adjustment and the computation of nextCadenceHeight; StartEventIngestion now initializes the RPC event subscriber using latestCadenceHeight directly. No exported/public API declarations were changed.

Changes

Cohort / File(s) Change Summary
Bootstrap: event ingestion init
bootstrap/bootstrap.go
Removed logic that computed nextCadenceHeight (latest+1) and the emulator-specific decrement; now constructs the RPC event subscriber with latestCadenceHeight as the start height.

Sequence Diagram(s)

sequenceDiagram
    participant B as Bootstrap
    participant ES as RPCEventSubscriber
    rect rgba(76,175,80,0.06)
    B->>ES: StartEventIngestion(latestCadenceHeight)
    ES-->>B: Subscription started / events streamed
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • peterargue
  • janezpodhostnik
  • zhangchiqing

Poem

I'm a rabbit in the code, quick on my way,
I nudged one number, let the heights stay.
No emulator tweak, no +1 stride,
A simpler start — I hop with pride. 🐇

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Always re-index latest Cadence block" succinctly captures the primary change of always using the latest Cadence block as the starting point for event subscription, directly reflecting the core diff in bootstrap/bootstrap.go. It is concise, clear, and focused on the main behavioral update without extraneous details. Scanning the title provides an immediate understanding of the PR’s intent.
Linked Issues Check ✅ Passed The diff directly implements the fix proposed in issue #766 by always passing latestCadenceHeight into the event subscriber, ensuring no EVM blocks are missed after node restarts. This change addresses the root cause identified in the investigation and aligns with the linked issue’s coding objectives without omitting any required modifications. All relevant behavior is updated and no additional requirements are left unaddressed.
Out of Scope Changes Check ✅ Passed All modifications are confined to bootstrap/bootstrap.go, where the subscriber initialization is simplified to use latestCadenceHeight, and no unrelated files or modules are altered. This single, focused change aligns fully with the scope of issue #766. There are no additions or refactors outside the intended fix.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mpeter/reindex-latest-cadence-block

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d50ef9e and 0933b2a.

📒 Files selected for processing (1)
  • bootstrap/bootstrap.go (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T10:14:49.676Z
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#890
File: services/ingestion/event_subscriber.go:235-239
Timestamp: 2025-10-06T10:14:49.676Z
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:

  • bootstrap/bootstrap.go
🧬 Code graph analysis (1)
bootstrap/bootstrap.go (1)
services/ingestion/event_subscriber.go (1)
  • NewRPCEventSubscriber (49-65)
⏰ 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 (1)
bootstrap/bootstrap.go (1)

146-155: Verify related PRs addressed atomic updates
No evidence in PR #760 or onflow/flow-go#7050 that non-atomic updates to latestCadenceHeight were fixed. Please confirm those PRs cover the root cause of the atomicity concerns raised previously.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread bootstrap/bootstrap.go
chainID,
b.keystore,
nextCadenceHeight,
latestCadenceHeight,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The latestCadenceHeight is read from storage, which is updated along with all the block data in an atomic batch updates, which means if latestCadenceHeight should always be the latest height that has its data indexed in the storage.

I think we should find the root cause for why nextCadenceHeight doesn't work. It would mean there is some non atomic updates committed after the height is updated, which probably got lost during restarts.

@janezpodhostnik janezpodhostnik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with Leo that we should check where the problem originated, but I'm ok with this being the temporary fix.

Just maybe add a comment somewhere describing what is happening and why.

@m-Peter
m-Peter marked this pull request as draft March 27, 2025 09:05
@m-Peter
m-Peter force-pushed the mpeter/reindex-latest-cadence-block branch from 16b6e99 to 93f4e75 Compare April 7, 2025 08:44
@m-Peter
m-Peter force-pushed the mpeter/reindex-latest-cadence-block branch from 93f4e75 to d50ef9e Compare October 9, 2025 09:56
@m-Peter
m-Peter marked this pull request as ready for review October 9, 2025 09:56
@m-Peter
m-Peter force-pushed the mpeter/reindex-latest-cadence-block branch from d50ef9e to 0933b2a Compare October 9, 2025 11:19
@m-Peter
m-Peter merged commit cae1289 into main Oct 9, 2025
2 checks passed
@m-Peter
m-Peter deleted the mpeter/reindex-latest-cadence-block branch October 9, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate missing EVM block with height 32808661 on testnet

3 participants