Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: eth_subscribe with geth open ended range #2027

Merged
merged 8 commits into from
Jan 23, 2025
Merged

Conversation

blindchaser
Copy link
Contributor

  • convert "fromBlock":"0x0","toBlock":"latest" to default subscription behavior

@cordt-sei cordt-sei self-assigned this Jan 9, 2025
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.27%. Comparing base (59d8e48) to head (ffb7a02).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2027      +/-   ##
==========================================
- Coverage   61.45%   61.27%   -0.18%     
==========================================
  Files         264      264              
  Lines       24571    24581      +10     
==========================================
- Hits        15101    15063      -38     
- Misses       8338     8388      +50     
+ Partials     1132     1130       -2     
Files with missing lines Coverage Δ
evmrpc/subscribe.go 65.51% <100.00%> (-1.33%) ⬇️

... and 4 files with indirect coverage changes

if filter.FromBlock != nil && filter.FromBlock.Int64() == 0 &&
filter.ToBlock != nil && filter.ToBlock.Int64() < 0 {
unboundedFilter := &filters.FilterCriteria{
FromBlock: filter.FromBlock, // keep "0x0" to fetch historical logs

Choose a reason for hiding this comment

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

The desired behavior in this scenario is not to fetch historic logs, just stream any new logs.

Copy link
Contributor

@cordt-sei cordt-sei Jan 9, 2025

Choose a reason for hiding this comment

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

Just looking through the relevant geth docs, toBlock and fromBlock are not even valid parameters for this method in any context so it's very confusing as to why they are being passed there in the first place.

@bruce-riley
Copy link

bruce-riley commented Jan 10, 2025

I created a simple test app that uses the go-ethereum library to listen for events from the Wormhole core contract without needing to run the Wormhole guardian. It could be used to verify this fix. See the PR description for how to use it.
wormhole-foundation/wormhole#4214

Reminder: The desired behavior is that only new events are streamed, not historic ones.

@bruce-riley
Copy link

I just confirmed that the Wormhole guardian is now able to see log events using go-ethereum with this fix. Please let us know when this is part of an official release. Thanks!

Copy link
Contributor

@cordt-sei cordt-sei left a comment

Choose a reason for hiding this comment

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

Tested with the aforementioned testing application and confirmed log event receipt.

@blindchaser blindchaser merged commit 18b7597 into main Jan 23, 2025
49 checks passed
@blindchaser blindchaser deleted the yiren/rpc-subscribe branch January 23, 2025 16:59
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.

5 participants