-
Notifications
You must be signed in to change notification settings - Fork 827
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
Conversation
blindchaser
commented
Jan 8, 2025
- convert "fromBlock":"0x0","toBlock":"latest" to default subscription behavior
771683c
to
47fba4d
Compare
b087ad4
to
8667e65
Compare
8667e65
to
c5603bd
Compare
e0f85a9
to
e143f0d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
|
evmrpc/subscribe.go
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I created a simple test app that uses the Reminder: The desired behavior is that only new events are streamed, not historic ones. |
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! |
There was a problem hiding this 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.
56dae10
to
2debf7c
Compare