Skip to content

Increase block scanner page size type (uint8 is too small)#1548

Closed
netoneko wants to merge 1 commit into
masterfrom
feature/increase-block-scanner-page-size
Closed

Increase block scanner page size type (uint8 is too small)#1548
netoneko wants to merge 1 commit into
masterfrom
feature/increase-block-scanner-page-size

Conversation

@netoneko
Copy link
Copy Markdown
Contributor

@netoneko netoneko commented Apr 7, 2020

It's simply difficult to process large batches of blocks if the maximum batch size is 255.

This PR updates the batch size type so I can use block scanner is other projects.

@noambergIL
Copy link
Copy Markdown
Contributor

@netoneko are you aware that you changed block scanning from 255 blocks to potentially 2^64 blocks ?
with lines like this ?
page := make([]*protocol.BlockPairContainer, 0, pageSize)

what happens if someone asks for 1billion blocks ?

this pull request don't seem right to me.

@netoneko
Copy link
Copy Markdown
Contributor Author

@noambergIL I think uint16 would be too small (less than 100k blocks), and after that it really does not matter how many blocks are requested from the API - it is internal and can be used for any kind of internal tooling, so if anyone wants to request a billion blocks I don't see a reason to restrict it on the API level.

@netoneko
Copy link
Copy Markdown
Contributor Author

The problem here specifically is that there was a need to process blocks in chunks of more than 255 (which I think is reasonable) - you can propose a different type or a constant that caps the maximum batch size.

@noambergIL
Copy link
Copy Markdown
Contributor

i don't think our code should be written where it fails on memory.
what exactly is the need ?
you cannot request more than 10000k events in infura and that seems to work fine for most.

@netoneko
Copy link
Copy Markdown
Contributor Author

I agree about memory. However, this is an internal API, not external.

@ronnno
Copy link
Copy Markdown
Contributor

ronnno commented Apr 27, 2020

@netoneko where are you planning to use this? I would be worried about creating dependencies on this module as it's not intended to be supported outside this repo. I don't want any reason to retain a module that makes no sense to keep later just because someone else used this from an outside repo....

Generally speaking, i don't know if we will keep the concept of pages here. we used pages in many places where a stream was more suitable. so I would not be surprised if in the future we might want to change that..

If you want to reuse this code maybe extract it to an independent repo as we've done with crypto? what do you think?

@ronnno
Copy link
Copy Markdown
Contributor

ronnno commented Jul 13, 2020

@netoneko see #1595 - the primary problem is that the blocks file codec is private. to work around that I suggest to either make it public or (as the PR demonstrates) to add a public wrapper which allows scanning blocks without pages at all.

that said, i still think creating dependencies on the ONG repo should be done very carefully if at all. usually it's a sign that something should either be inside ONG or that something in ONG should not be in ONG

@netoneko netoneko closed this Jul 21, 2020
@noambergIL noambergIL deleted the feature/increase-block-scanner-page-size branch September 22, 2020 07:31
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.

3 participants