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

Client: Implement eth_getLogs RPC endpoint #1520

Closed
holgerd77 opened this issue Oct 11, 2021 · 6 comments
Closed

Client: Implement eth_getLogs RPC endpoint #1520

holgerd77 opened this issue Oct 11, 2021 · 6 comments

Comments

@holgerd77
Copy link
Member

During the Merge interop event it turned out that Eth2 clients need the eth_getLogs JSON RPC endpoint, so we should implement (as some hack when doing work on #1512 we just mocked the call and gave a static response).

For these kind of historical data we would need some database to store along client sync, latest state after some discussion with Ryan was that we likely wouldn't want to store in the configuration database (due to being misplaced) but also do not want to add a new database for every new type of data stored, so we settled a bit on doing one additional database for all now and future historical data additions (e.g. receipts as well if needed) and use some prefixing here for access (log_* or similar). If there are drawbacks which we might have overlooked let us know.

Update: from looking at the API from the RPC description linked above with the different possible filter parameters (block numbers, address, topic), I wonder if we would rather need a small relational database here? 🤔

Logs are produced along VM execution runs in runTx.ts respectively runBlock.ts.

@holgerd77
Copy link
Member Author

Here is a really nice deep-dive-article with a good overview on how eth_getLogs works and valuable information for an implementation (heavily DoS vulnerable RPC method since all parameters are optional, which can result in a huge request load).

@gabrocheleau
Copy link
Contributor

gabrocheleau commented Oct 13, 2021

Would there be any issue for The Merge compatibility if we follow the same rules as Alchemy as outlined in the post shared above (or perhaps make them more/less loose depending on our needs)? Namely:

Here are the safety nets Alchemy has in place for large eth_getLogs requests:

  1. You can make eth_getLogs requests with up to a 2K block range and no limit on the response size.
  2. You can also request any block range with a cap of 10K logs in the response.

EDIT: actually it looks like they've implemented a response size limit since then (from their docs):

Here are the safety nets Alchemy has in place for large eth_getLogs requests:
You can make eth_getLogs requests with up to a 2K block range and 150MB limit on the response size. You can also request any block range with a cap of 10K logs in the response.

@holgerd77
Copy link
Member Author

Yeah, that sounds reasonable.

I guess the most important thing is that we generally have some constants in place to actually set these values, then it will be easy to just adopt later on if we see that some limits are impractical for some reasons.

@gabrocheleau
Copy link
Contributor

After doing a bit of research into how Geth handles logs:

@holgerd77
Copy link
Member Author

Update on this relational DB idea: just had another look at the API. I have to say that I didn't get the whole picture before since I just read "optional" on all the parameters and assumed that there would be a "context-less" search on whatever topic possible (so over the whole range of logs from genesis to present), which would likely not be doable with a key-value store. 😜

I overlooked the default paramters for the block (being set to "latest"). So that makes it a totally different story of course. So I guess one is implementing this by looking at the respective blocks and - from reading the Geth code - the log entries are then stored by txHash and one can just read the txs from the blocks and see if there are matching log entries?

If that's correct I guess the relational DB idea is very much from the table and we can stick to what you guys suggested (one additional level DB for these kind of things).

@acolytec3
Copy link
Contributor

acolytec3 commented Nov 15, 2021

I think we can safely call this one closed via #1185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants