Use a better data structure for SignedSubmissions instead of Vec

[coriolinus]

Invent a new SignedSubmissions data structure which acts like a BoundedBTreeSet for the signed submissions, but has the property that it only reads a single submission at a time. This gives it the following properties:

• Take advantage of existing code to handle the bounding, instead of re-inventing the wheel.
• Take advantage of O(n log n) sorting properties for BTreeMap instead of the O(n*n) property we had for the vector.
• Substantially improve the worst-case memory use. We have to read more times, but each read is only a fraction as large.

[emostov]

When we drain with our btreemap (e.g. SignedSubmissionsMap::<T>::drain()), do we load/decode all the entries from trie into memory at once? Or does each iteration load/decode a single entry? (Maybe a better question is where could I find the drain impl? 😬)

Also, does the SignedSubmission get dropped from memory once we go to the next loop iteration?

[coriolinus]

This calls the fn drain method defined on line 167. From there, it delegates to SignedSubmissionMap::<T>::drain(), which is an implementation of IterableStorageMap::drain which loads/decodes a single entry at a time.

And yes, each SignedSubmission gets dropped at the end of its loop iteration.

[kianenigma]

Good point, but this is safe since as mentioned we allocate one by one.

Maybe a better question is where could I find the drain impl

https://crates.parity.io/src/frame_support/storage/generator/map.rs.html#135-195

[emostov]

With my level of understanding it looks good, but some Rust related details are still lost on me, so I am going to try and come back.

[kianenigma]

maybe even call it decode_stored_len or similar.

[coriolinus]

I was basing it on StorageValue::decode_len.

[kianenigma]

yeah, and I wanted to precisely not be similar to that given that the semantic is slightly different :p

[kianenigma]

The abstraction itself looks damn good

1. I think this is a serious issue and needs resolving: https://github.com/paritytech/substrate/pull/8933/files#r652639698

2. curious why letting Drop write SignedSubmissions does not work in some tests? I have a hunch finding the reason might unravel another bug.

[coriolinus]

@kianenigma I think this is now ready for another look.

As for the Drop impl, I've realized that we don't want that after all. The whole point of the storage overlays is to ensure that we can return early without writing to storage. If writes were triggered on Drop, then we'd push the overlays to storage each time a SignedSubmissions exited scope, regardless of whether the exit was a success or an error. I haven't proved it (or even really tried to), but I suspect that the test failures given the Drop impl may be due to updating storage when we're not supposed to.

[kianenigma]

but I suspect that the test failures given the Drop impl may be due to updating storage when we're not supposed to.

Yeah, this is the main potential footgun with Drop and I prefer keeping it explicit for now too.

[kianenigma]

WDYT? for me this is much easier to reason about, even if it is a few more lines of code:

match (inserted, maybe_weakest)) => {
  (false, None) => { // Not inserted, none removed }
  (false, Some(weakest) => { unreachable!()??? }
  (true, None) => { // inserted and none was removed } 
  (true, Some(weakest) => { // inserted and someone was ejected }
}

which can be made into

match (inserted, maybe_weakest)) => {
  (false, None) => { // Not inserted, none removed }
  (false, Some(weakest) => { unreachable!()??? }
  (true, maybe_removed) => { // inserted, maybe unreserve if maybe_removed.is_some() }
}

[coriolinus]

Doesn't that break down into what precisely exists now? We handle the (false, None) case with an ensure, don't need to handle a (false, Some(_)) unreachable case, and then handle the (true, maybe_removed) case inline. Agree that those are the cases we need to consider; I just think that the existing formulation is easier to read.

[kianenigma]

I read this in the downstream PR again and it was still confusing to me. Probably our definitions of "easy to read" differs. For me a readable code is one that can be easily understood with the least amount of time spent reading it. This entails being slightly verbose sometimes.

A match statement's basis is that it cannot have any unhandled cases, and the reader can clearly see each case. The sequence of if statements took more time to reason about, at least in my case.

I leave it up to you though, just wanted to share my detailed opinion.

[kianenigma]

LGTM!

[kianenigma]

@coriolinus feel free to merge into the base branch when ready. Then you can proceed with urging the audit team to start their work on the main PR, and get the second approve.

I will re-visit the base PR one last time once this is merged into it.