Remove requirement on Hash = H256, make Proposer return StorageChanges and Proof

[bkchr]

So yeah, this pr started as a simple "make Proposer generate a proof on request". However, @rphmeier requested to make it also possible to import blocks without re-executing them. So, here we are, this pull request makes it possible to import self-build blocks without re-executing them.

• Proposer now returns the build block, the StorageChanges and an optional proof.
• The StorageChanges contain all the changes to the storage that are required to build the state for the block.
• While working on it I came across these BlockT<Hash = H256> and it made rustc complain about my code, so I decided to remove this stuff from the whole code base. (Finally we are free to choose any hashing algorithm in the Block!)
• In some circumstances it can happen that we need to calculate the state-root twice, e.g. when we record a proof. That is more a flaw of the current design of state-machine/Backend/ProvingBackend that could be solved with some code refactoring. I tried it, but I failed :(

The process now looks like:

1. Proposer creates a block.
2. We include the storage changes of the block into BlockImportParams.
3. We check if StorageChanges are present and use them or we execute the block to get them (old behavior).

FIxes: #1232 #3624

[rphmeier]

IMO it should not be optional, and it should be more strongly-typed. This is also important work to allow us to avoid re-executing the block proposal when importing a locally authored.

[bkchr]

It will be more strongly typed with this pr #3834

Or what you wanted to be more strongly typed?

How would this allow us avoid re-executing the block when importing a locally build block?

[rphmeier]

This is what I was thinking of: Computing the state root while building a block is producing a list of altered trie nodes to be written into the database. This is discarded. We should pass that through to the import of that block to avoid re-execution because it is an optimization. It's not quite the same thing as this PR, but maybe there is some possible overlap. That's what I wanted to be non-optional. Proving the storage in the parent state probably should be optional.

Vec<Vec<u8>> for storage proofs is really bad and ugly, so I'm glad there's a PR for that underway.

[bkchr]

This is what I was thinking of: Computing the state root while building a block is producing a list of altered trie nodes to be written into the database. This is discarded. We should pass that through to the import of that block to avoid re-execution because it is an optimization.

Yeah okay, that makes sense :) I can do this.

[rphmeier]

Is there some workaround to avoid the unsafe blocks? There is no reason that unsafe code should be introduced in such large modules, since it means that you have to track the invariant in the entire module - in this case, thousands of lines of code. I don't see a good argument for raising the maintenance burden of the client to be so high.

We should avoid unsafe code whenever possible. When it's not (and I'm not convinced it is impossible here...) it needs to be wrapped in a safe API exposed by a small enclosing module.

[bkchr]

The unsafe block is not 1000 of lines. The function is just marked as unsafe to make sure no-one easily breaks the invariant. The ApiRef invariant is that the inner type is bound by the lifetime of ApiRef. Now, when I want to consume the inner, I could also just return inner and break this invariant. To make the user aware that he should know what he is doing, the function is unsafe. You don't need to track anything outside of ApiRef to make sure the invariant holds.
I also would like to get rid of this and as always there is a RFC that would solve my problem here. However, there is maybe also a different way of solving this, but I still did not had the time to test it.

[rphmeier]

The unsafe block is not 1000 of lines.

You are misunderstanding how unsafe code works. It pollutes the entire module containing it, because those are the finest-grained visibility modifiers you can write.

The function is just marked as unsafe to make sure no-one easily breaks the invariant

This is not the purpose of unsafe code. If you can't cause memory unsafety by using it wrongly, then it shouldn't be tagged unsafe. If it can be, then it should be gated off in a safe way, not thrown into a massive audit surface.

I am not saying that no unsafe code should be used. But if we do use it, it should be used correctly and in a way that makes it hard to make mistakes. I perceive this usage as one which is not minimizing possible future mistakes.

[gnunicorn]

Thanks for doing this massive change–took me all day, but finally got through it. Just some minor things. Highly appreciate the decoupling from the h256 hashing!

[gnunicorn]

			(true, ref mut storage_changes @ None, Some(ref body)) => {

What is this wizardry syntax?

[bkchr]

I match on None in column 2 and make the column accessible as ref mut storage_changes in the match arm.

[bkchr]

@rphmeier removed the unsafe code usage :)

[gnunicorn]

YAY

[rphmeier]

@bkchr I will merge when there is an update PR for Polkadot ready!

[gavofyork]

needs resolving and polkadot PR.