Skip to content

Conversation

@jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Apr 21, 2023

Try to implement #15063 and discussed with @s1na,
In the first phase, we want to make the filter.Logs() function returns a channel instead of a large array

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

some early comments

@fjl fjl changed the title WIP: eth/filters: retrive logs in async WIP: eth/filters: retrieve logs in async Apr 24, 2023
@jsvisa jsvisa changed the title WIP: eth/filters: retrieve logs in async eth/filters: retrieve logs in async Apr 24, 2023
@jsvisa
Copy link
Contributor Author

jsvisa commented Apr 24, 2023

@s1na please take another look, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't reviewed this in any depth, so maybe I'm missing something... but it looks to me like this spins up two routines that may be stuck forever trying to deliver, in case the other side is no longer reading, for whatever reason.
Wouldn't it make sense it use the ctx to check if it's time to abort these routines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. But seems we have handled this issue inside the indexedLogs https://github.com/ethereum/go-ethereum/blob/101ac683e447a2c23fc0141d33bbb3f2e0e32607/eth/filters/filter.go#L266

I wonder if I've lost something

Copy link
Contributor

@s1na s1na May 8, 2023

Choose a reason for hiding this comment

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

If we want to protect against potential receiver failure, we'd have to wrap every channel send with a select, as in:

select {
  case logchan <- log:
  case ctx.Done():
    return ctx.Error()
}

I guess I'm just wondering if this has an impact on performance or that should be negligible

@s1na
Copy link
Contributor

s1na commented May 8, 2023

This PR is looking very nice overall! I'm gonna do some testing.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsvisa
Copy link
Contributor Author

jsvisa commented May 18, 2023

@s1na @holiman please take another look, thanks

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

One remaining nit, otherwise lgtm

@s1na
Copy link
Contributor

s1na commented May 22, 2023

I saw the timeout testcase I added fail once so I re-adjusted the timeout. It might turn out to be a flaky one. In that case I'll remove it in a future PR.

@s1na
Copy link
Contributor

s1na commented May 23, 2023

Needs rebase

@jsvisa
Copy link
Contributor Author

jsvisa commented May 23, 2023

I'll rebase it right now

s1na and others added 13 commits May 23, 2023 21:36
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Signed-off-by: jsvisa <[email protected]>
@jsvisa
Copy link
Contributor Author

jsvisa commented May 23, 2023

Rebased, and please take another look @s1na @holiman :)

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM. We might be putting out a release pretty shortly, so I'd prefer to wait with merging until afterwards

@s1na s1na added this to the 1.12.1 milestone May 24, 2023
@holiman holiman merged commit db9a178 into ethereum:master May 25, 2023
@jsvisa jsvisa deleted the log-subscribe branch June 3, 2023 12:01
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This change implements async log retrievals via feeding logs in channels, instead of returning slices. This is a first step to implement ethereum#15063.  

---------

Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Sina Mahmoodi <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Co-authored-by: Sina Mahmoodi <[email protected]>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Aug 27, 2025
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants