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

Compact proof. #295

Merged
merged 39 commits into from
Jun 8, 2021
Merged

Compact proof. #295

merged 39 commits into from
Jun 8, 2021

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Jan 18, 2021

This PR depends on :
paritytech/substrate#8574

This PR adds code proof compression (the state proof).

@cheme cheme changed the title Should we compress proof? Should we compress trie storage proof? Jan 18, 2021
@cheme cheme requested a review from bkchr January 18, 2021 15:16
@cheme
Copy link
Contributor Author

cheme commented Jan 28, 2021

I switch this PR to also extract ':code' from proof.

The idea is that, with cumulus setting, validation code is the same as runtime code, so we have access to it while validating.

Therefore the code is removed from proof when compressing and added back when decompressing (in a lazy maner by some validation host function).

Notice that on polka size, code is pretty hacky, probably running
let validation_code = runtime_api_request( ctx, descriptor.relay_parent, RuntimeApiRequest::ValidationCode( descriptor.para_id, assumption, code_tx, ), code_rx, ).await?;
would be less impacting (still need to have these parameters attached to validation data.

Also design could be more generic (maybe this can be applied to other key than ':code'):

  • host function targetting known content: something like 'known_storage_key' returning 'Option<Option<Vec>'.
  • trie decompress could use a lazy trait that take key as input and trigger on ALL encoded 0 length content.
    (currently we need to know skip key when decompressing)
    This requires to escape the compressed 0 length content to some known byte sequence (so it changes the encoding
    in the rather rare case of empty content).

Regarding test done:
At this point only the fact that integration test is running (I did see root mismatch when I inserted &[] as code before linking the host function).

So clearly there is more work to do (polkadot and alternative design), but this seems pretty ok, so this question is as relevant as either.

@cheme cheme changed the title Compact proof and skipped wasm code. Compact proof. Apr 9, 2021
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.

2 small nitpicks

@@ -134,6 +143,7 @@ fn validate_block_no_extra_extrinsics() {
witness,
validation_data,
} = build_block_with_witness(&client, vec![], parent_head.clone());
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this function directly returning the CompactProof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 done

Comment on lines +70 to +74
let root = match sp_trie::decode_compact::<sp_trie::Layout<HashFor<B>>, _, _>(
&mut db,
storage_proof.iter_compact_encoded_nodes(),
Some(parent_head.state_root()),
) {
Copy link
Member

Choose a reason for hiding this comment

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

Ahh and didn't you added a method to the compactproof for uncompressing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would have to rebuild the db afterward, here we directly got the db with the right content.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay

@bkchr
Copy link
Member

bkchr commented Jun 7, 2021

bot merge

@ghost
Copy link

ghost commented Jun 7, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Jun 7, 2021

Merge aborted: Checks failed for 23afc08

@bkchr bkchr merged commit d935b81 into master Jun 8, 2021
@bkchr bkchr deleted the ec-compress-proof branch June 8, 2021 09:51
bkchr added a commit that referenced this pull request Jun 8, 2021
* compact, need to be made optional and look into/compress child trie
roots/state.

* proto with child trie support

* Missing set_offchain_storage overload.

* right name

* Ignore offchain indexing in validation function.

* patch trie-db

* decompress from iter

* use compressed proof

* remove wasm blob from proof (no inject plugged yet)

* change lock

* update trie

* change in toml

* Revert "change in toml"

This reverts commit aa5c568.

* use patch to branches

* i

* i:wq

* switch branch

* ii

* ok, needed to patch the runtime by putting substrate patch in polkadot
project.

* test passing with this conf

* actual lazy code fetch

* patch issue

* Code reorg

* restore commented tests.

* update deps.

* remove polka patch

* fixes

* remove patch

* revert cargo.lock

* cargo update -p sp-trie polkadot-service

* fix collator test (using parent state root).

* Update pallets/parachain-system/src/validate_block/implementation.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Remove encode_witness test function.

* Update pallets/parachain-system/src/validate_block/implementation.rs

* Fix compilation

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
bkchr added a commit that referenced this pull request Jun 23, 2021
bkchr added a commit that referenced this pull request Jun 30, 2021
JoshOrndorff pushed a commit to moonbeam-foundation/cumulus that referenced this pull request Jun 30, 2021
andresilva added a commit that referenced this pull request Jul 15, 2021
bkchr added a commit that referenced this pull request Jul 26, 2021
yrong pushed a commit to yrong/cumulus that referenced this pull request Aug 11, 2021
yrong pushed a commit to yrong/cumulus that referenced this pull request Sep 3, 2021
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.

2 participants