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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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.33%. Comparing base (ef720f4) to head (e143f0d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2027      +/-   ##
==========================================
- Coverage   61.47%   61.33%   -0.15%     
==========================================
  Files         264      264              
  Lines       24563    24573      +10     
==========================================
- Hits        15101    15072      -29     
- Misses       8333     8373      +40     
+ Partials     1129     1128       -1     
Files with missing lines Coverage Δ
evmrpc/subscribe.go 63.54% <100.00%> (+0.33%) ⬆️

... and 3 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.

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.

3 participants