Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@montekki
Copy link
Contributor

Exposes proof generation and verification from trie-db.
Bumps trie-db and memory-db dependencies.

@montekki montekki added the A0-please_review Pull request needs code review. label Jan 16, 2020
@montekki montekki requested review from cheme and rphmeier January 16, 2020 14:55
Copy link
Contributor

@NikVolf NikVolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be PR in polkadot ready so that always identical version of trie* dependencies are used

@cheme
Copy link
Contributor

cheme commented Jan 16, 2020

Not sure if it should be part of this PR, but there is also : https://github.com/paritytech/trie/blob/d60e70bdd8c1e62af00459f9301c5002e69aaa21/trie-db/src/lib.rs#L105 that propose a way to have smaller proofs.
The method from this link targets proofs where we do not have content information (extrinsic execution or cumulus witness, or key value proof when we do not know the values).
It is still a processing / memory cost, and I am wondering if we should target support of multiple proof format. The method in this PR are specific enough that it seems very unlikely that someone would like to send a full proof instead.
I just realize that verification could be way faster if we verify against a previously checked proof (no need to recalculate the hash if they were already checked).
Just by curiosity what is the use case here (what I did understand from Jim was that it was mainly to check for change of values)?

@montekki
Copy link
Contributor Author

@cheme For instance it is intended to be used in XCMP

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces everywhere!

@bkchr
Copy link
Member

bkchr commented Jan 16, 2020

Benchmarks need fixing.

@montekki
Copy link
Contributor Author

@bkchr yes, that is in process


/// Create a proof for a subset of keys in a trie.
///
/// The `keys` may contain any set of keys regardless of each one of them is included
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "included into" should be "included in" - here and in other docs.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than docs nit!

@bkchr bkchr merged commit 36a6de0 into paritytech:master Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants