Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Add basic multiproof generation and verification #94

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Jul 15, 2019

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.

@coveralls
Copy link

coveralls commented Jul 15, 2019

Coverage Status

Coverage increased (+0.1%) to 93.689% when pulling 893c724 on multiproof into 5a5b1a1 on master.

@zmitton
Copy link
Contributor

zmitton commented Jul 19, 2019

Cool. What I'm seeing looks just like what we had talked about 💯, I support it.

@holgerd77
Copy link
Member

@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
Copy link
Contributor Author

s1na commented Jul 23, 2019

@holgerd77 Do you think we can do a release here after this?

@holgerd77
Copy link
Member

@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.

@holgerd77
Copy link
Member

@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?

@holgerd77
Copy link
Member

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).

@s1na
Copy link
Contributor Author

s1na commented Jul 25, 2019

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?

@holgerd77
Copy link
Member

@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.

@holgerd77
Copy link
Member

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.

@s1na
Copy link
Contributor Author

s1na commented Jul 25, 2019

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.

@holgerd77
Copy link
Member

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.

@zmitton
Copy link
Contributor

zmitton commented Jul 30, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants