trie: add verifyRangeProof

[samlior]

verifyRangeProof is not currently supported, so I implemented the js version with reference to geth

[codecov]

Codecov Report

Merging #1731 (b0e5fe8) into master (f4f34c8) will increase coverage by 0.52%.
The diff coverage is 64.62%.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.28% <ø> (ø)
client	75.23% <ø> (+3.15%)	⬆️
common	93.90% <ø> (+0.01%)	⬆️
devp2p	82.63% <ø> (+0.26%)	⬆️
ethash	90.76% <ø> (ø)
trie	80.02% <64.62%> (-6.17%)	⬇️
tx	89.94% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[samlior]

The test case failed to run because karma reported that Buffer is undefined. But this project uses Buffer everywhere, is it some environmental problem?

[acolytec3]

The test case failed to run because karma reported that Buffer is undefined. But this project uses Buffer everywhere, is it some environmental problem?

Agreed, this is a strange one because we're already supposed to be polyfilling buffers. Will look into it a bit.

[acolytec3]

@samlior If you delete package-lock.json and reinstall deps, the test will pass. There was something broken somewhere down in the dependency tree.

[s1na]

First thanks for this extensive work. I had a cursory look but didn't check everything with detail, specially the utility methods, e.g. unset, ... and had some comments and clarification questions. Another nitpick is you use invalid proof for all errors :d having more descriptive errors would be more user-friendly.

In general this function is very niche. The only use-case I'm aware of is snap sync. I'm not sure if other users of the library will benefit from this, e.g. compared to a general multiproof (e.g. see ethereumjs/merkle-patricia-tree#94 or ethereumjs/merkle-patricia-tree#101). Do you plan to use it yourself?

[samlior]

You are right, this feature is really niche, I need this feature because I want to add snap sync to my own Ethereum implementation, and this feature is indispensable.

I can go ahead and submit some improvements or continue discussions on this issue, but before that, I'd like to know if you guys think this feature is necessary to be added to this library. @s1na

[holgerd77]

@s1na what was the state of the old PRs on this? I guess it would be ok for @samlior to cherry-pick some (all) of the work done there and integrate eventually if this would make sense? How does these two (three) PRs relate, is your work a strict super-set of what is done here?

[samlior]

@holgerd77 In my understanding, in fact, ethereumjs/merkle-patricia-tree#101 and my work are two completely different things. Turboproof is used to provide proof for stateless ethereum client, and cannot be used to provide proof for snap sync (unless we change the SNAP protocol). And my implementation It is written specifically for snap sync and SNAP protocol.

Is my understanding correct? @s1na

[s1na]

@samlior @holgerd77 yes if the use-case is snap sync then the old PRs won't suffice. This PR is needed. Also if EthereumJS wants to at some point adopt snap sync this PR will be useful.

[holgerd77]

TBH I am not able to review this - I am not deep enough into the trie library - and I wonder if someone else from the team could do a really in-depth review.

Since this is such isolated functionality I would nevertheless be inclined to just merge this in, the side effects if we miss some edge cases here or whatever are extremely limited.

@samlior can you fix (or give a short explaination if you can't/won't) on the remaining issues/comments Sina had? Then I guess we would already be more or less ready to go.

[holgerd77]

(ah, and also wanted to mention: we have also some additional security by the tests)

[holgerd77]

@samlior: also let us know if you need a quick release on this.

[samlior]

@holgerd77 I can and I will continue to work on this PR.
By the way, there's no need for a quick release right now, I've used this part of the code in other ways, thx.

[holgerd77]

@holgerd77 I can and I will continue to work on this PR. By the way, there's no need for a quick release right now, I've used this part of the code in other ways, thx.

Ok, cool, let us know when you consider this ready! 😄

[samlior]

@holgerd77 this PR is ready, and the test fails because karma still reports that Buffer is undefined, but it works on my local machine :D

[ryanio]

looks great, thanks for addressing feedback and everything, I can try to fix that karma issue tomorrow

[holgerd77]

Thanks @samlior, this looks really really great! Will merge this in now, again - super-isolated functionality which cannot do harm to other areas even if we are missing some edge cases here or something.