Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decode Merkle proofs entirely at once #3013

Merged
merged 5 commits into from
Nov 21, 2022
Merged

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Nov 17, 2022

cc #2973

This PR rewrites the code that verifies Merkle proofs. Instead of iterating through the proof while decoding it to find one specific element, we now fully decode and verify the proof then later find the elements from that decoding. This means that we no longer need to verify the proof multiple times.

In terms of API, this means that decoding the proof no longer returns any "entry missing" error. The errors about a missing entry have thus been added at a higher level.

The logic of the code keeps track of all the entries found in the proof. Later, in order to find an element, we actually iterate "upwards". So for example to find 0xf00bab, we first try 0xf00bab, then for example 0xf00b and then 0xf00. The vast majority of the time there should be a precise hit, so it seems better to me to try to find 0xf00bab first and then iterate upward in order to prove that the requested key doesn't exist or isn't in the proof.

I've removed the existing code. Despite the fact that it works, I think it's a strictly worse way of doing it than the code in this PR.

As I've reached the end of this PR and my brain is fried, I realized that using a BTreeMap is kind of stupid since the proof is already a tree, and we should instead just keep a parallel Vec adding some information that avoids having to recalculate hashes when decoding the tree. However I don't have the courage to do this anymore and I've just left a TODO instead. Since this is purely an implementation detail and that the complexity would remain the same, it can be changed later without breaking anything.

I also had in mind to immediately add a next_key and a prefix_key function to the proof, but again I don't have the courage to do so now.

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────
       +6668 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h67e45a83a0270f06
       -6552 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h9321c12a0a9e32e8
       +2223 ┊ smoldot::sync::warp_sync::BuildChainInformation<TSrc,TRq>::build::he71aeeb4a8e19549
       +2138 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h8e4e65654e5d6353
       +2133 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h9f8e3f0deb4d58d6
       +2096 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hb9e63801c83cb8e9
       +2094 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hafb856c3e168f443
       +2071 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hc8460297ebb56e76
       +2039 ┊ smoldot::trie::proof_decode::decode_and_verify_proof::h2296938b316dcd09
       +2039 ┊ smoldot::trie::proof_decode::decode_and_verify_proof::h833adbae05bf9768
       +2039 ┊ smoldot::trie::proof_decode::decode_and_verify_proof::h88a8812ccc0f70a7
       +2018 ┊ smoldot::trie::proof_decode::decode_and_verify_proof::h191e66faca1841cb
       +2010 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hcedc31f861e0a76f
       +2010 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hf56f483ce62c33c4
       -1979 ┊ smoldot::sync::warp_sync::BuildRuntime<TSrc,TRq>::build::he9de39c758fcbdbb
       +1979 ┊ smoldot::sync::warp_sync::BuildRuntime<TSrc,TRq>::build::hfd10369ab6509fc7
       +1978 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hbff43cf1ea49b729
       +1864 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h7b776881750092c4
       +1844 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::he60f4b97c8fcc335
       -1823 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h07035515b1b45a7a
       -2884 ┊ ... and 407 more.
      +41531 ┊ Σ [427 Total Rows]

@mergify mergify bot merged commit 1fb2011 into paritytech:main Nov 21, 2022
@tomaka tomaka deleted the proof-decode-all branch November 21, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant