Add another condition to the reject-obsolete-parachain-heads extension

[svyatonik]

So currently this extension only rejects transaction if bundled at-relay-chain-block-number is lesser or equal than the existing one. But there's another condition - when parachain head remains the same (i.e. is not updated) in some relay blocks, then the other relay may use different relay chain block to prove it. The extension won't reject the transaction, even though the pallet will ignore the head (since it is the same). This PR handles this situation.

TODOs:

• test it;
• add unit tests;
• cleanup;
• more traces?

[serban300]

Looks good ! I just have 2 comments. If they make sense I would be happy to implement them.

[serban300]

This validation looks very similar with the one here. 2 questions:

1. The equality case for at_relay_block_number seems to be treated differently. Here it is treated as invalid, in update_parachain_head() it's treated as valid. Is this expected ?
2. Would it make sense to do the same validation in both places ? In this case would it make sense to deduplicate the code ?

[svyatonik]

1. Imo in both cases at_relay_block_number leads to error, no (I maybe missing something - sorry)?

fn update_parachain_head() {
    ...
    match stored_best_head {
    	Some(stored_best_head) if stored_best_head.at_relay_block_number <= updated_at_relay_block_number => {
            if updated_head_hash == stored_best_head.head_hash {
                return Err(());
            }

            // else: pass
        }
    ...
}

fn validate() {
	...
	match best_parachain_head {
		Some(best_parachain_head) if best_parachain_head.at_relay_block_number
			>= bundled_relay_block_number => Err(()),
		Some(best_parachain_head) if best_parachain_head.head_hash == *parachain_head_hash => Err(())
	...
}

A few notes on that: the other condition (matching hashes) is actually a separate condition, so maybe in the extension it is expressed better, with a dedicated match branch. In general, we shall ignore tx if it supplies head updated at <= stored_best_head.at_relay_block_number and we shall ignore if it has matching hashes. In practice we can't have two blocks with the same hash (crypto-safe hashing) - there's a lot that will break if we'll see it, so second condition may be checked separately or not.

2. Yeah - if there's a better way to do that, go ahead! Thank you!

[serban300]

1. Imo in both cases at_relay_block_number leads to error, no (I maybe missing something - sorry)?

From what I understand, if stored_best_head.at_relay_block_number == updated_at_relay_block_number and the hashes are different, for update_parachain_head() it will go to // else: pass, while the extension will trigger an error. So if I understand correctly this case is treated differently now, even if in practice we shouldn't have to blocks with the same hash.

Anyway, I just wanted to make sure if we should have the same check in both places.

[svyatonik]

Yeah - correct. It is impossible to have two with blocks the same hashes in practice. And if it is the same block (that is already imported), we shall reject/ignore tx

[serban300]

Nit: I would do something like:

		let parachain = ParaId(P::SOURCE_PARACHAIN_PARA_ID);
		if parachains != &[parachain] {
			return Err(SubstrateError::Custom(format!(
				"Trying to prove unexpected parachains {:?}. Expected {:?}",
				parachains, parachain,
			)))
		}

[serban300]

#1520 was merged. Marking this PR as reviewed.