refactor: improve trie prefetching

[pugachAG]

• Fix instances of clippy::result_unit_err warning
• Add explicit assert in tests on timeout while waiting for queue to become empty
• Avoid single-letter variable names
• Avoid code duplication in prefetching.0.lock().expect(POISONED_LOCK_ERR) by introducing helper lock function
• Move reserved_memory to PrefetchSlot
• Add comments to address some confusion I experienced when reading this code.

Related to #8145 and #7723

[akhi3030]

I am not super familiar with the code but trying to get more into it. So some drive-by comments. Feel free to ignore if they do not make sense.

[jakmeier]

Looks great, happy to see the code improving! 😄

I left very few comments, mostly optional ones. Feel free to merge anytime you think it's ready.

---

No action required but some thoughts:

On a more code-philosophical note, I think PrefetchError is a positive change here but there are two reasons why it was like this.

• It is a common pattern in Rust code that a method that are designed to fail often under normal circumstances should return ownership of all arguments that it owns. This would allow to retry, for example. But here it's not so important as we can clone all args. And in the current state of the code, we don't make use of the returned resources anyway, so no harm in removing that. But generally speaking, I would argue this is good practice and a patterns that can be useful to know in Rust code.
• Personally, I only create error enums when there is more than one failure case. I guess it's mostly a matter of taste. But my main argument would be that refactoring from an implicit error to a enum error is trivial when a second variant becomes necessary. But going back can be tricky if the type gets integrated into other types, for example as #[source] of another error. Also, going back to bullet point 1, returning resources is awkward when using a specific error type.

But again, looking at your code changes, it has made things more readable. So in this case I am in favor of making the change. 👍

[akhi3030]

LGTM. It will be best if someone more familiar with the code also did a review though.

[akhi3030]

Definitely not part of this PR but we should improve the API for this function. Currently all callers are calling with small TrieKeys. However, some variants e.g. ContractData can be very large and in such cases, the API should take them by value.

An option might be for this function to accept AccountId instead of TrieKey or for the function to take TrieKey as a reference (but this will introduce a lot of lifetime issues).

[pugachAG]

Just to confirm my understanding: you would like to avoid all .clone() calls involved in creating TrieKey from the main thread, is that right?

[akhi3030]

I feel like an API that encourages one to clone() a large data structure is problematic. So we should think hard about it. In this case, I wonder if changing the function to take AccountId will make the API less "error prone" to use.