merkle-patricia-tree
merkle-patricia-tree copied to clipboard
ethereumjs
Add basic multiproof generation and verification
This PR was based on discussions with @zmitton, with the goal of reducing proof size for https://github.com/ewasm/scout/pull/11.
It generates proofs for multiple keys, and de-duplicates the proof nodes based on their keccak256 hash. In an example test case on average it reduced number of nodes from ~600 to ~200.
This feature should be considered experimental.
Coverage increased (+0.1%) to 93.689% when pulling 893c7248cbb6107b4a5f41e5b6286c9a5e4bdf49 on multiproof into 5a5b1a19597341893ea1908435605339f0cacdd2 on master.
Cool. What I'm seeing looks just like what we had talked about 💯, I support it.
@zmitton Can we regard your comment and some review and then merge or could you respectively go one level deeper and extend this to some review with at least one comment on every file? Would be super-helpful! 😄
@s1na Yes, of course!
I think I get this myself on a second deeper look 😜, so I can also do the review myself (//cc @zmitton). Will directly do, merge (hopefully, seems there is some Travis outage, have updated the branch and the tests broke all under 1 min, we'll see on a second run), and prepare some release notes on this.
Update while writing: jobs still fail, will try a bit later.
@s1na When having a look at the closed PRs I would go on with doing a v3.1.0 release, a bit unsure about #82. Would you go along with this?
Just googled the Travis thing (xfvb refuses to start) and just drop here some possible solution hint thread in case this lasts, would nevertheless assume for now that this is temporary (or alternatively some configuration switch on the Travis side, we'll see).
Thanks for looking into this :)
When having a look at the closed PRs I would go on with doing a v3.1.0 release, a bit unsure about #82. Would you go along with this?
Hmm, it's a hard case because there are some small change of behaviours, even though the API itself (functions and their parameters haven't changed). E.g. when walking the trie, if a node is not present in the DB, the code returns an error. Whereas for example .get used to return null even in this case.
Another option is to merge some other breaking changes that we're planning (e.g. TS migration, or promisified API) and then do a v4.0.0 release. What do you think?
@s1na Hmm, can't judge how urgently you need this or how much it would distract you to just "drop in" some unplanned (though very welcome) TS transition.
I think it can be argued to put this under "bug fixing" the error behavior, so for my part I think I would be ok with a minor release as well.
Hmm, did another test on Travis, still failing, eventually we have to dig a bit deeper. Might be solvable with some slight change on the Travis configuration.
can't judge how urgently you need this
It's not very urgent anymore, I solved my issue with the v3.0.0 release.
It's up to you, if you'd rather do this release now that's fine with me, but I'd be okay with doing the TS/Promise transition because I'm using this library heavily at the moment myself.
Ah okay, then I would take the TS/Promises offer 😋🙂, would be really great if we also get this transition as one of the major libraries.
The code changes and the tests of both look entirely correct. I haven't setup a local environment for it right now, but if the tests are passing it should be good to go