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

Allow usage of different proof formats #4938

Closed
wants to merge 253 commits into from

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Feb 14, 2020

This PR is integrating paritytech/trie#44 to substrate.

This PR defines some Proof trait and allows choosing them in state machine.

The choice of proof and by extension state-machine backend, would move to client.
For this PR, I do restrict the scope to 'primitives' code.
Complete move to client can be found at https://github.com/cheme/substrate/tree/split_child_proof_master_state_trait .
Cht and change trie proof are also not covered in any branch.

New proof supported in the pr:

The new proofs cannot be merged so new mechanisms for merging proof were added:

  • proof now use an intermediate representation ProofRaw that can get merged and converted later to the final proof target.
  • proof recorder backend needs to keep trace of roots queried, so when it is used multiple tiem as in the call executor we need to inject previous roots ('previous_input' in function 'from_previous_rec_state').

Noticable points:

  • associated statemachine backend for recording and runing proof are not strictly needed as I got a single implementation here.

  • Proofbackend is similar to record backend, but it is not bringing much complexity in the trait definition.

  • Multiple proof implementation is leftover code from the previous version of this pr (single enum for all proofs), this is not really needed at this point and could be removed. I expect though, that we may see some use case for it, especially when two substrate chain want to check each other proofs: usual way is to use a wasm proof checker, but it could be nice to allow native verification over a subset of supported proof through this enum.

  • Full variants for simple and compact proof are not strictly needed.

  • The proof full recorder (see into_partial_full_db) is not strictly needed either.

  • ChildrenProofMap and ChildInfoProof are not strictly needed with current supported child trie and can be replaced with ChildrenMap and ChildInfo.

  • ProofInput is not using a trait and but is kept as an enum.

    • It can be simplified to roots only (the query plan test case use a different proof to register).
    • We could also choose to always register roots when using state machine and remove this input parameter, simplifying things a bit.
  • Proposal from consensus common is storing encoded proof instead of non encoded proof and later encoding.

  • Input for proof is an enum, to avoid complexifying to much the type definitions, but it could be an associated type.

  • FullForMerge is storing by encoded hash, it avoids passing Hasher at some place, but could also use H as parameter (hasher ends up being necessary for compacting proof).

Previously this PR was using an enum for all supported proof type (d659b2c), and additional parameter was added to all api to chose proof kind.
This new version of the pr just define some proof trait and allow associating them to the state-machine backend in a safer way.

allocation whise unless we break all api for a close to nothing perf
change: switching to simple single child info struct.
@cheme
Copy link
Contributor Author

cheme commented Jun 12, 2020

The refactoring seems finish for this, PR, after fixing the oversized line breaks and going through the documentation, I will push the PR back to reviewable.
I already update the description and first feedbacks are welcome (code should not change much).

@cheme
Copy link
Contributor Author

cheme commented Jun 17, 2020

I am pushing this PR back in reviewable state.
The refactoring changes the way proof are handled.
I am not entirely happy with followup code in https://github.com/cheme/substrate/tree/split_child_proof_master_state_trait that allows specifying proof type at client level, due to the number of change and general trait complexity.
But on the other hand it would allow a parachain to run on its own state backend (eg with a different trie or state).
Going back to a pr using globally Multiple (enum) proof in state could be a more straightforward way to allow using compact proof (would need to add a parameter to as_proof_backend), but it still feels like going in a wrong direction.

@cheme cheme added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 17, 2020
@cheme
Copy link
Contributor Author

cheme commented Jul 10, 2020

Pr got updated with master as https://github.com/cheme/substrate/tree/split_child_proof_master_state_trait (generally this branch is rather interesting for its client and service related changes).

@gnunicorn
Copy link
Contributor

closing because of inactivity.

@gnunicorn gnunicorn closed this Sep 9, 2020
@cheme
Copy link
Contributor Author

cheme commented Sep 9, 2020

Well I understand the reason to close pr for inactivity.
Just wanted to mention that I can open this again to anyone interested.
The reduction in size of proof as mentioned in the PR description being between :
trie size | nb value in proof | size compressed / size uncompressed
100 | 1 | 0.83
100 | 10 | 0.23
100 | 100 | 0.05
1000 | 1 | 0.86
1000 | 10 | 0.44
1000 | 100 | 0.20
1000 | 1000 | 0.03
10k | 1 | 0.84
10k | 10 | 0.56
10k | 100 | 0.32
10k | 1000 | 0.12
10k | 10k | 0.02
100k | 1 | 0.87
100k | 10 | 0.63
100k | 100 | 0.45
100k | 1000 | 0.26
100k | 10k | 0.09

So it all relates to the tree coverage for the proof.

Note that those number are from a trie using random 32 bytes keys with 32 byte values.

Another closing note if someone want to use those proofs differently:
this PR did aim at bringing the proof choice and compression for a state at the client level and is only relevant when looking at the follow up branch: https://github.com/cheme/substrate/tree/split_child_proof_master_state_trait .

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. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants