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

les: fix les/1 CHT compatibility issue #15692

Merged
merged 1 commit into from Jan 9, 2018
Merged

Conversation

zsfelfoldi
Copy link
Contributor

This trivial PR fixes an off-by-one error converting to and from LES/1 CHT numbers in compatibility mode.

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.

It's a bit difficult to review this, because it's not terribly well documented (e.g. what is the impact of the bug, and why does it do +1/-1 -- if it's ceiling, it looks like it still may do wrong. ceil(a/b) == (a+b-1)/b, not a/b+1 ).

I also see that we stop for a softLimit post-facto; after the soft limit has been broken. I hope there's not an attack vector in there?

@zsfelfoldi
Copy link
Contributor Author

@holiman CHT tries are generated regularly and are identified by an index. In LES/1 they were generated every 4096 blocks, in LES/2 every 32768 blocks (it was just too much unnecessary overhead, especially because in LES/2 the bloom bit tries were added too). Also (what I forgot to consider at first) LES/1 requests indexed CHTs starting from 1 (so that CHT #1 contained data for blocks up to block 4095, CHT #8 until 32767 (this last block is called the section head). LES/2 indexes CHTs starting from 0 so CHT #0 contains data for blocks up to block 32767. So LES1CHT #8 == LES2CHT #0, LES1CHT #16 == LES2CHT #1, LES1CHT #24 == LES2CHT #2 and so on.
Internally, CHTs are still generated every 4096 blocks to ensure backwards compatibility but according to the new chain indexer mechanism they are numbered from 0 and this is how light.GetChtRoot expects the index. I know it turned out to be a mess but after some time we can remove LES/1 compatibility and simplify this part and always use LES/2 indexing everywhere.

@zsfelfoldi
Copy link
Contributor Author

softResponseLimit was inherited from eth/handler.go and it works exactly the same there too, though I am not 100% sure if we even really need it. The request count limit MaxHelperTrieProofsFetch limits the amount of data anyway, also each individual request can generate a limited amount of response data so I see no attack vector here.
We can discuss whether we should remove softResponseLimit but that is outside the scope of this PR.

@karalabe
Copy link
Member

Could we somehow have a test for this? Would be nice to prevent future regressions.

@fjl fjl merged commit 83d1657 into ethereum:master Jan 9, 2018
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
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.

None yet

4 participants