Parachains finality pallet

[svyatonik]

I've been considering generic pallet that'll e able to solve both parachains finality + #1002 (comment) . That's because both pallets are simple receiving storage proof => parsing proof => optionally check that the proof is duplicate => doing some actions, based on this proof. But the generic palle will be too generic - imo it is better to have parachains finality logic separated from some generic code.

This is very WIP, although current code already gives general idea of what it'll look like. Main questions that still needs to be solved is keeping old parachain headers (aka heads) as we do with relay chain headers + add similar pruning.

[svyatonik]

Actually it should be supported - parachain may be offboarded and its head may be removed. So there should be 3 possible outcomes from read_parachain_head: Err(_), Ok(Some(_)) and Ok(None). Right now the first (which is relayer error) is replaced with Ok(None), which is handled incorrectly.

[svyatonik]

I'd prefer to deal with it in follow-up PRs, as it requires some more thinking + there are already two PRs on top of that one.

[svyatonik]

All parachain pallets from Polkadot are using polkadot_parachain::primitives::Id to store parachain id. The difference is that it is compact-encoded u32 => it should be fixed

[svyatonik]

Parachains pallet from Polkadot is using polkadot_parachain::primitives::HeadData to store parachain heads. The difference is that they're encoding parachain header twice, i.e. it is prefixed with encoded bytevec length => it should be fixed.

[svyatonik]

@AurevoirXavier @fewensa If you're really keen on this parachain pallets+relay+bridge, then it'll be really helpful if you could add your review to these three dependent PRs - this one, #1199 and #1218 . Please ask any questions directly in PRs (better option) or DM me on riot (you've been already chatting with me there).

The most important design question: there are two options to use parachains finality - one is a separate pallet+relay and another one is simply adding parachain head proof to existing proofs. The latter looks simpler - i.e. right now proof of messages looks like (bridged_header_hash, message_storage_proof, lane, range_of_nonces). We could then simply add a (parachain_id, parachain_head_storage_proof) there and everything should work. But in this case we won't be able to use results of previous parachain_head_storage_proof verification when/if another messages proof is submitted. We may add some cache, but it'll result exactly in a separate pallet. Another thing is that this separate pallet is just another layer on top of our bridge-grandpa (and following bridge-beefy) pallet. Both are sharing (almost) the same API and we're not mixing all possible cases in a single pallet - e.g. we may combine relay<>parachain, relay<>relay and parachain<>parachain bridge by using different set of isolated pallets, while only changing the "glue" code.

[hackfisher]

But in this case we won't be able to use results of previous parachain_head_storage_proof verification when/if another messages proof is submitted.

I don't get this part, do you mean a new parachain_head_storage_proof could be submitted before some message proof get included using relative older parachain header?

If that's the case, bridge grandpa pallet may also have similar issue, and onchain cache looks simple and straight forward to me.

[svyatonik]

But in this case we won't be able to use results of previous parachain_head_storage_proof verification when/if another messages proof is submitted.

I don't get this part, do you mean a new parachain_head_storage_proof could be submitted before some message proof get included using relative older parachain header?

If that's the case, bridge grandpa pallet may also have similar issue, and onchain cache looks simple and straight forward to me.

GRANDPA pallet works exactly like this cache :) Let me explain in details. So message proof = (bridged_header_hash, message_storage_proof, lane, range_of_nonces). To verify message_storage_proof, you'll need to know a storage trie root, which is a part of bridged_header_hash header. And you need to be sure that this header is finalized.

So imagine you don't have a bridge GRANDPA pallet in the runtime (let's omit validator set changes for simplicity). Then the messages pallet may still be used, but the message proof changes to (bridged_header, bridged_header_grandpa_justification, message_storage_proof, lane, range_of_nonces). So it includes finality proof now. And if you need to prove several same-header messages using several transactions, it would mean that you'll need to submit and verify justification multiple times.

That's where GRANDPA pallet helps and where it works like cache - once finality proof is verified, you may just provide header hash in your message proof. and when this proof is verified, you may just read the header from the cache (runtime storage), without reverifying finality proof again.

Now imagine you're bridging with parachain and you don't have both GRANDPA pallet and parachains finality pallet. Then the message proof would be (bridged_relay_header, bridged_relay_header_grandpa_justification, bridged_parachain_header, bridged_parachain_header_storage_proof, message_storage_proof, lane, range_of_nonces). And you would need to verify 3 proofs every time you're reusing the same parachain header in the message proof. So what this separate parachains finality pallet does, is removing the bridged_parachain_header, bridged_parachain_header_storage_proof from this tuple, instead providing a trusted cached map of bridged_parachain_header_hash => bridged_parachain_header. So it also works like cache, but with a parallel API, like bridge GRANDPA pallet does.

Let me know if it makes sense to you.

[eskimor]

Looks pretty good as far as I can tell.

[eskimor]

We usually have the rule that each TODO should reference some ticket.

[svyatonik]

Yep, thanks - I'll open issues for that

[acatangiu]

Looks good, just nits and typos 👍

[serban300]

I tried to ramp-up as a bit on how parachains work and I did multiple iterations on this code. However I still have limited experience with this. But for me, the design looks good. I can't find any issue with it. I just left 2 nits.

[serban300]

Nit: Personally I would change this name to something like ParaState or ParaMetaData since it contains more than just the BestParaHeads.

[serban300]

Personally I would change this structure to something like:

pub struct ParaState {
	pub best_head_hash: BestParaHeadHash,
	pub next_imported_hash_position: u32,
}

in order to reuse the BestParaHeadHash structure instead of having 2 structures that contain at_relay_block_number and head_hash.

[svyatonik]

Yeah, that's a good idea imo