Create opaque struct for StorageProof.

[jimpo]

Passing around Vec<Vec<u8>> everywhere is gross and confusing and breaks encapsulation.

[cheme]

Seems pretty straight forward, and actually a very good change.
Still a few comments to address (the one on cht may be counter productive to address).

[cheme]

I prefer the old version where we did not cat vecs: here we are eating memory. Maybe an iterator version of merge_storage_proofs will get the best of both world.
Actually merge_storage_proofs takes an iterator as input, so there is maybe something todo, does not seems super easy though since it neends to transform for_each_cht_group to an iterator.

Edit: may not be worth addressing.

[cheme]

vec allocation can be avoided here, especially since merge_storage_proofs takes an iterator as input.

[jimpo]

How exactly do you recommend creating this iterator? I think something like iter::once(init_proof).chain(iter::once(exec_proof)) is a bit clunky.

[cheme]

I do not have better solution than using 'chain', I know some people prefer using Some(init_proof) rather than iter::once, I prefer once.
Clunkier solution (I tend to like fn_iter but I never try to bench it but it should have no advantage over chain): https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8c36252915e606e2fbd845ac2618ffdd

[jimpo]

This really doesn't seem like a good reason to sacrifice readability to me. merge_storage_proofs internally has to create a new HashSet and much larger vector of trie nodes anyway.

[cheme]

Of course it is just an alloc among lot of others, I think crazy iter expression are rather common in the code base, but not abusing them is probably a good thing.

Regarding the other comment where we grow memory, you also can use a different approach, I know some poeple like this kind of api, I never really use it but it may be good here.

struct ProofMerger(Hashset<StorageProof>);

impl ProofMerger {
  fn add(self, proof: StorageProof) {
    self.0.extends(proof);
    self
  }
  fn merge(self) -> StorageProof {
    StorageProof { trie_nodes: self.0.into_iter().collect() }
  }

}

in this case would look like

let total_proof = ProofMerger(init_proof)
  .add(exec_proof)
  .merge();

[cheme]

Note that it is really only nitpicking, should not prevent merging this PR for me.

[Demi-Marie]

LGTM, and definitely cleans things up.

[gui1117]

it seems in all cases you can do merge_storage_proof(Hashet<Proof>) maybe change the signature to get an Hashset directly.

[bkchr]

I think this should be moved to substrate-primitives. State-machine just compiles on native, but the proof stuff is required on no-std as well e.g. in Cumulus.

[jimpo]

OK, I added a TrieProofWithValues struct in substrate-trie which has a no_std option. StorageProof in substrate-state-machine now wraps that.

The reason these need to be separate is because a TrieBackend is an abstration over a single trie which is defined in substrate-state-machine. In particular, it allows storage accesses to child tries which traverse multiple tries, a concept foreign to substrate-trie.

Currently, these structs are both very simple and are basically just a vec of encoded nodes. In the future, though, it would be nice to create trie proofs that omit redundant internal hashes and storage proofs that internally contain multiple separate trie proofs, one for the root trie and one for each child trie.

[bkchr]

Could we please get a method into_inner() or similar?

[jimpo]

OK, yes, I removed the whole TrieProof thing and there's a StorageProof::iter_nodes(self) method which iterates over all trie nodes.

[bkchr]

Some last nitpicks.

[bkchr]

Could we please get a method into_inner() or similar?