chainHead: Report unique hashes for pruned blocks

[lexnv]

This PR ensures that the reported pruned blocks are unique.

While at it, ensure that the best block event is properly generated when the last best block is a fork that will be pruned in the future.

To achieve this, the chainHead keeps a LRU set of reported pruned blocks to ensure the following are not reported twice:

	 finalized -> block 1 -> block 2 -> block 3
	
	                      -> block 2 -> block 4 -> block 5
	
	           -> block 1 -> block 2_f -> block 6 -> block 7 -> block 8

When block 7 is finalized the branch [block 2; block 3] is reported as pruned.
When block 8 is finalized the branch [block 2; block 4; block 5] should be reported as pruned, however block 2 was already reported as pruned at the previous step.

This is a side-effect of the pruned blocks being reported at level N - 1. For example, if all pruned forks would be reported with the first encounter (when block 6 is finalized we know that block 3 and block 5 are stale), we would not need the LRU cache.

cc @paritytech/subxt-team

Closes #3658

[jsdw]

I had assumed that ensuring pruned hashes were unique was as simple as using a HashSet or something when building the pruned list. That would ensure the blocks in a single pruned list were unique.

Am I right in my understanding that the extra bits in this PR are to also ensure that we don't see the same block hash mentioned again across different pruned lists, too?

[lexnv]

Yep, I initially thought we could get away with a simple HashSet.

However, we might prune a fork that contains another fork:

fork -> A
   |
     -> B -> C

Chain:
        X ... Y ... Z

In this case, when we finalize Y Substrate reports A as pruned. We'll go the path [fork .. A] and report those blocks.
When we finalize Z Substrate reports C as pruned. We'll go again on the path [fork .. B C]. We'd need to ignore the common blocks that we've just reported with [fork .. A].

[skunert]

Logic looks correct, only some nits in the comments 👍

[jsdw]

To check my understanding, this is all abotu addressing this bit of the spec:

The current best block, in other words the last block reported through a bestBlockChanged event, is guaranteed to either be the last item in finalizedBlockHashes, or to not be present in either finalizedBlockHashes or prunedBlockHashes.

Here it looks like when a new last finalized block is seen, we:

• Return happily if the best block was finalized.
• Else, return a new BestBlockChanged event if the current best block has been pruned.
• Else, return a new BestBlockChanged event if the current best block will be pruned (ie is not a descendant of the new finalized chain)

To satisfy the spec, could the whole thing be simplified to pseudocode like this?:

if let Some(current_best_block) = self.best_block_cache {
    // current best block is in pruned list, so need to emit new BestBlock
    let is_in_pruned_list = pruned_block_hashes.iter().any(|hash| *hash == current_best_block);
    // current best block is not the last one we finalized
    let is_not_last_finalized = current_best_block != last_finalized;

    if is_in_pruned_list || is_not_last_finalized {
        let new_best_block = self.client.info().best_hash;
        events.push(BestBlockChanged { best_block_hash })
    }
}

events.push(finalized_event);
Ok(events)

In any case, I wonder whether we could avoid some of the early returns and such, and separate it so that we first write the logic to decide whether we need to push a new BestBlockChanged event and then do the push, with a single return at the end to hand back the events?

[lexnv]

Yep, that makes sense!

I think I was more worried about cases where we've already generated a BestBlockChanged event for a descendant for the finalized block.

However, I think this is safe to regenerate a BestBlockChanged with an older block number, as long as it is the last reported finalized, from this spec statement:

The current best block, in other words the last block reported through a bestBlockChanged event, is guaranteed to either be the last item in finalizedBlockHashes

The suggestion simplifies the code quite a bit, thanks! 🙏

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5843121

[skunert]

Just to be sure I understand this correctly. When we generate the initial events we consider all blocks that are part of a fork that started below the finalized one as pruned, even if they are technically not pruned yet. So these will never be reported?

[lexnv]

Yep. I can't remember exactly, this was introduced quite a while ago to handle the case where the Finalized event would contain a pruned block not reported by a new block event.

This abuses the purpose of self.pruned_blocks, which now holds:

• (previously stored in to_ignore) forks that we did not report previously by the Initialized event
• (the intended purpose of this PR) pruned blocks already reported once

[jsdw]

Nit: This test is quite long, and looking at it, I wonder whether we can extract a couple of helper fns or something so that it's easier to see what's actually beign tested vs all of the staet we're creating.

Like maybe a couple of helpers like this would be handy, but not sure if there are differences tha would make it not-very-useful:

async fn import_block(client, parent_hash, parent_num) -> block {
        let block = BlockBuilderBuilder::new(&*client)
		.on_parent_block(client.chain_info().genesis_hash)
		.with_parent_block_number(0)
		.build()
		.unwrap()
		.build()
		.unwrap()
		.block;
	let block_1_hash = block_1.hash();
	client.import(BlockOrigin::Own, block_1.clone()).await.unwrap();
        block
}

[jsdw]

This looks good to me; good job Alex!