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

eth/filters: eth_getLogs fast exit for invalid block range #28386

Merged
merged 13 commits into from
Nov 7, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Oct 20, 2023

We can quickly exit if the from > to in eth_getLogs request.

BTW, currently for eth_newFilter, if from>to, then the RPC returns an error, but no error for eth_getLogs, should we need to unify their behavior, or let it go?

eth_getLogs request

{
  "jsonrpc": "2.0",
  "method": "eth_getLogs",
  "id": 74,
  "params": [
    {
      "toBlock": "0x1",
      "fromBlock": "0x2000"
    }
  ]
}

eth_getLogs response

{
  "id": 74,
  "jsonrpc": "2.0",
  "result": []
}

eth_newFilter request

{
  "jsonrpc": "2.0",
  "method": "eth_newFilter",
  "id": 73,
  "params": [
    {
      "toBlock": "0x1",
      "fromBlock": "0x2000"
    }
  ]
}

eth_newFilter response

{
  "error": {
    "code": -32000,
    "message": "invalid from and to block combination: from > to"
  },
  "id": 73,
  "jsonrpc": "2.0"
}

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.

Seems reasonable to me, some nits though

eth/filters/api.go Outdated Show resolved Hide resolved
graphql/graphql.go Outdated Show resolved Hide resolved
@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 20, 2023

Seems reasonable to me, some nits though

@holiman fixed it, please take another look.

Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
This reverts commit a15f45c.

Signed-off-by: jsvisa <delweng@gmail.com>
This reverts commit 4bd9908.

Signed-off-by: jsvisa <delweng@gmail.com>
This reverts commit aa50100.

Signed-off-by: jsvisa <delweng@gmail.com>
This reverts commit ed3b1e6.

Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa jsvisa changed the title eth/filters,graphql: fast exit for invalid block range eth/filters: eth_getLogs fast exit for invalid block range Oct 24, 2023
@s1na
Copy link
Contributor

s1na commented Oct 24, 2023

Blocked on ethereum/execution-apis#480 being merged.

@ghost
Copy link

ghost commented Nov 1, 2023

@s1na @jsvisa Could you generalize the error message? Would like to implement block range limit for eth_getLogs call after this PR is merged just like this one paradigmxyz/reth#5243

Something like invalid block range params would be good

@s1na
Copy link
Contributor

s1na commented Nov 2, 2023

Something like invalid block range params would be good

@kaliubuntu0206 I think this error message would work for us too. However generally I'd like if we could standardize error codes instead of error messages. It's perfectly reasonable that clients would return different messages (e.g. some with more context or less).

Signed-off-by: jsvisa <delweng@gmail.com>
@fjl fjl merged commit f20b334 into ethereum:master Nov 7, 2023
2 checks passed
@fjl fjl added this to the 1.13.5 milestone Nov 7, 2023
devopsbo3 pushed 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
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
colinlyguo pushed a commit to scroll-tech/go-ethereum that referenced this pull request Oct 31, 2024
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