-
Notifications
You must be signed in to change notification settings - Fork 89
Add basic multiproof generation and verification #94
base: master
Are you sure you want to change the base?
Conversation
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! 😄 |
@holgerd77 Do you think we can do a release here after this? |
@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 |
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 :)
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 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. |
It's not very urgent anymore, I solved my issue with the 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 |
This PR was based on discussions with @zmitton, with the goal of reducing proof size for ewasm/scout#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.