Skip to content

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Aug 26, 2022

As part of #25459 I made a change so that graphql's tx log query goes through the usual filter system so it benefits from the newly added cache. However the filter system only handles logs on the block level, not tx level and I didn't select logs based on txindex which caused a regression.

Fixes #25583

@s1na s1na requested a review from gballet as a code owner August 26, 2022 14:31
@s1na s1na closed this Aug 26, 2022
@s1na s1na reopened this Aug 26, 2022
ret := make([]*Log, 0)
for _, log := range logs {
if uint64(log.TxIndex) != t.index {
continue
Copy link
Contributor

@fjl fjl Aug 27, 2022

Choose a reason for hiding this comment

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

logs is sorted by TxIndex, so this could be improved to skip most iteration:

ix := sort.Search(len(logs), func (i int) bool { return logs[i].TxIndex == t.index })
for ix < len(logs) && logs[ix].TxIndex == t.index {
    ret = append(ret, logs[ix])
    ix++
}

Copy link
Contributor

@fjl fjl Aug 27, 2022

Choose a reason for hiding this comment

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

Also, it's kind of weird to use a filter here. We could just call t.r.backend.GetLogs to get the block logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, just realized that the filter is used here exactly because GetLogs doesn't have access to the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of like the simplicity even tho it's slightly less fast, if you don't have a strong feeling about it.

Choose a reason for hiding this comment

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

Fix tested working on my personal node.
Would deploy 1.10.23 + your_fix on prod if no 1.10.24 official version before the merge (currently using 1.10.21 + TTD set in CLI params)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly in favor of @fjl's suggestion.

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

ret := make([]*Log, 0)
for _, log := range logs {
if uint64(log.TxIndex) != t.index {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly in favor of @fjl's suggestion.

return nil, err
}
ret := make([]*Log, 0, len(logs))
ret := make([]*Log, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ret := make([]*Log, 0)
var ret []*Log

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

@holiman holiman added this to the 1.11.0 milestone Aug 31, 2022
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@gballet gballet merged commit 8cfcb41 into ethereum:master Aug 31, 2022
@holiman holiman modified the milestones: 1.11.0, 1.10.24 Sep 14, 2022
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.

GraphQL/JSON-RPC logs regression between 1.10.21 & 1.10.22
5 participants