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

rpc: O(N**2) batch response matching #22805

Closed
bennetyee opened this issue May 3, 2021 · 3 comments · Fixed by #23856
Closed

rpc: O(N**2) batch response matching #22805

bennetyee opened this issue May 3, 2021 · 3 comments · Fixed by #23856
Assignees
Milestone

Comments

@bennetyee
Copy link

go-ethereum/rpc/client.go

Lines 367 to 392 in fc1c1cb

for n := 0; n < len(b) && err == nil; n++ {
var resp *jsonrpcMessage
resp, err = op.wait(ctx, c)
if err != nil {
break
}
// Find the element corresponding to this response.
// The element is guaranteed to be present because dispatch
// only sends valid IDs to our channel.
var elem *BatchElem
for i := range msgs {
if bytes.Equal(msgs[i].ID, resp.ID) {
elem = &b[i]
break
}
}
if resp.Error != nil {
elem.Error = resp.Error
continue
}
if len(resp.Result) == 0 {
elem.Error = ErrNoResult
continue
}
elem.Error = json.Unmarshal(resp.Result, elem.Result)
}

We scan the N=len(b) responses and for each we scan through msgs to look for the one with an ID that matches. For large batches, if the response ID order is in reverse of the batch order, we can get O(N**2) runtime in the matching.

It may be okay to just add a comment to say do not use large batches in the interface, but ideally this shsould be fixed.

@fjl
Copy link
Contributor

fjl commented May 5, 2021

I will fix it.

@fjl fjl self-assigned this May 5, 2021
@fjl fjl changed the title O(N**2) batch response matching rpc: O(N**2) batch response matching May 17, 2021
@muddlebee
Copy link

Hey @fjl 👋 !!
did you get a chance to look at this?

@fjl fjl added this to the 1.10.12 milestone Oct 25, 2021
@fjl
Copy link
Contributor

fjl commented Nov 1, 2021

No, not yet. I consider this low priority because batch requests are not used much. Even if used, I would consider it rare to use hundreds of requests in one batch. That said, I will fix it as soon as I get to it.

@fjl fjl closed this as completed in #23856 Nov 4, 2021
fjl added a commit that referenced this issue Nov 4, 2021
This avoids quadratic time complexity in the lookup of the batch element
corresponding to an RPC response. Unfortunately, the new approach
requires additional memory for the mapping from ID to index.

Fixes #22805
sidhujag pushed a commit to syscoin/go-ethereum that referenced this issue Nov 4, 2021
This avoids quadratic time complexity in the lookup of the batch element
corresponding to an RPC response. Unfortunately, the new approach
requires additional memory for the mapping from ID to index.

Fixes ethereum#22805
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this issue Dec 3, 2022
This avoids quadratic time complexity in the lookup of the batch element
corresponding to an RPC response. Unfortunately, the new approach
requires additional memory for the mapping from ID to index.

Fixes ethereum#22805
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 a pull request may close this issue.

3 participants