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

Optimize query on BorReceiptLogs #102

Closed
jdkanani opened this issue Jan 8, 2021 · 8 comments
Closed

Optimize query on BorReceiptLogs #102

jdkanani opened this issue Jan 8, 2021 · 8 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jdkanani
Copy link
Contributor

jdkanani commented Jan 8, 2021

PR #90 introduced BorReceiptLogs, which gives logs from state-sync as well. But, that introduced a delay in response time for the log filter query.

@jdkanani jdkanani added enhancement New feature or request help wanted Extra attention is needed labels Jan 8, 2021
@0xKrishna
Copy link
Contributor

0xKrishna commented Mar 23, 2021

We are only getting that logs timeout related issue only when user doesn’t give startBlock and it starts fetching from 1st block, Right ?
Why don’t we put a check in GetLogs method that if the startBlock is not given in the request, Don’t include borLogs into that ? I know this is not a solution, But I found the code where we are fetching the logs and borLogs separately and then merging them, So just a thought.
https://github.com/maticnetwork/bor/blob/master/eth/filters/api.go#L337-L373

@jdkanani @imyourm8

@jdkanani
Copy link
Contributor Author

That's a nice idea. But the problem is that devs need to understand what is borReceiptLogs and it won't be there when startBlock is not provided or 0. Due to that, data will be inconsistent based on the argument. Not sure if we should do it.

@0xKrishna
Copy link
Contributor

0xKrishna commented Mar 24, 2021

Or maybe we can add a boolean query param like includeBorReceiptLogs in which case the startBlock is compulsory and can't be 0. Again for this the developer must understand what the borReceiptLogs means but if he do he has control over the data he want to request using that param. Also not sure if we can increase the timeout somehow using that param in this specific case.

@ssandeep
Copy link
Contributor

Another thought - can we treat this as a general pagination problem and have a max limit of records that can be queried in a single call. So in this case, let's say we make start and end blocks mandatory and if the difference between start and end is more than 1000 or 10000, we don't allow such calls and throw an error with details about the max no of blocks that can be queried at once. Again, not sure if there would be impact on the consumption from devs or any other restrictions it might place for the use cases we are supporting.

@jdkanani jdkanani linked a pull request Apr 13, 2021 that will close this issue
@hellwolf
Copy link

hellwolf commented Apr 14, 2021

This will force people using external RPC node service to setup their indexers such as thegraph early in their development cycle, even for a just small dapp.

Then what is the need of using that external RPC node at all after one already gone through setting up indexer? Almost none, since the rpc node wouldn't provide any ability to do "scalable range queries".

I found it perplexing that matic BOR node is eliminating the assumption that eth_getLogs should be scalable O(log(N)) at least. And force people to use a indexing layer on top of it.

Please take this one seriously, it affects not only one team, I have validation that there are more teams affected by this.

@hellwolf
Copy link

hellwolf commented Apr 14, 2021

Or maybe we can add a boolean query param like includeBorReceiptLogs in which case the startBlock is compulsory and can't be 0. Again for this the developer must understand what the borReceiptLogs means but if he do he has control over the data he want to request using that param. Also not sure if we can increase the timeout somehow using that param in this specific case.

Something like this could work actually, since logs from "state-sync" might not be interesting to some teams.

OR, even a completely separated rpc function eth_getLogsWithStateSyncs

@ghost
Copy link

ghost commented Jun 8, 2021

Hi guys, just thought I'd cross link this bsc issue here bnb-chain/bsc#113 seems like y'all are facing similar questions. While I think both of your threads together are very valuable, I'd like to point to this users post bnb-chain/bsc#113 (comment) specifically. Seems like a heavy lift, but if it was possible to implement the 10k log response limit with no blockrange limit it will make it so I dont have to re write my application into batched calls of a 1000 blocks per piece. Hope its food for thought.

Cheers,

@0xKrishna
Copy link
Contributor

Re-open if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants