Add pallet-storage-roots #18
Conversation
| /// List of all the keys in `RelayStorageRoot`. | ||
| /// Used to remove the oldest key without having to iterate over all of them. | ||
| #[pallet::storage] | ||
| pub type RelayStorageRootKeys<T: Config> = |
There was a problem hiding this comment.
Please remove this storage item, you don't need it.
To delete old relay roots, you can iterate the RelayStorageRoot map with a drain iterator: https://paritytech.github.io/polkadot-sdk/master/frame_support/storage/trait.IterableStorageMap.html#tymethod.drain
There was a problem hiding this comment.
I also attempted this, the problem is that the drain iterator is not sorted. So if I want to remove the oldest key, I need to iterate over all of them. Removing the first element of the iterator will remove the element whose hash is the lowest. Actually now I see the docs mention this:
Remove all elements from the map and iterate through them in lexicographical order of the encoded key.
There was a problem hiding this comment.
Maybe it will work if we set the hash to identity. Do you think that's a good idea?
There was a problem hiding this comment.
@tmpolaczyk as long as MaxStorageRoots is not too high (less than 1000) I think that the Identity hash is safe.
There was a problem hiding this comment.
Ok we expect this value to be around 10 so I implemented this change. I am not sure if I like it, so I did it in a separate commit that is easy to revert in case we change our minds.
There was a problem hiding this comment.
I don't understand why you see the need for a linked list.
We just want to keep the last K relay hash, don't we?
So you just need to increment OldestRelayNumber to CurrentRelayNumber-K and delete the corrresponding key at each increment.
There was a problem hiding this comment.
In any case, VecDeque is a bad approach because it add more complex storage for nothing. At worst, store a BTreeMap<u32, Hash> directly in one StorageValue and for K ~ 10 it will work.
There was a problem hiding this comment.
Oh I see, one thing I forgot to mention is that the relay number is not necessarily consecutive. In fact currently it increments by 2 for every parachain block, but not always. With async backing it may sometimes increase by 1 and sometimes not. And in the case of parathreads there may be huge gaps. So we cannot use that fact to delete items more efficiently. We could try to iterate starting from OldestRelayNumber up to OldestRelayNumber+10 until we find something, but it may not always work, and it is going to be less efficient than the alternatives.
And the problem with using a single BTreeMap is that it adds proof size. A key is 4 bytes and a value is 32 bytes, so mutating a VecDeque that only has the keys is much more efficient.
There was a problem hiding this comment.
And in the case of parathreads there may be huge gaps.
Sorry, I didn't think about parathreads, it's true that for parathreads the gap can be huge and we can't just increment.
Thanks for the explanation, I understand better the relevance of your initial design now, but at first glance it's not obvious, can you please add a comment in the code that explains the rationale for this design choice?
There was a problem hiding this comment.
Sure, I added some docs. Also changed the storage item to use a BoundedVec, which is converted into a VecDeque after reading it.
…ty hash" This reverts commit 45fe4a1.
| let mut keys: VecDeque<_> = <RelayStorageRootKeys<T>>::get().into_inner().into(); | ||
| keys.push_back(relay_state.number); | ||
| // Delete the oldest stored root if the total number is greater than MaxStorageRoots | ||
| if u32::try_from(keys.len()).unwrap() > T::MaxStorageRoots::get() { |
There was a problem hiding this comment.
Please not use unwrap inside runtime production code, use an explicit saturation to u32::MAX instead
There was a problem hiding this comment.
Agree, thanks for spotting!
| keys.push_back(relay_state.number); | ||
| // Delete the oldest stored root if the total number is greater than MaxStorageRoots | ||
| if u32::try_from(keys.len()).unwrap() > T::MaxStorageRoots::get() { | ||
| let first_key = keys.pop_front().unwrap(); |
There was a problem hiding this comment.
Please never use unwrap inside runtime production code, move the remove instruction inside an if let Some instead
There was a problem hiding this comment.
Very good job, thanks @tmpolaczyk for this awesome contribution!
|
@Agusrodri can you review this PR? |
|
Indeed amazing job @tmpolaczyk ! |
(cherry picked from commit 8406e80)
This pallet stores the latest
MaxStorageRootsrelay storage roots, which can be used to verify state proofs against an old state of the relay chain.