[9177] finality_target returns a hash instead of an option

[tau3]

Hello!

I'd like to submit a fix for #9177
SelectChain::finality_target now returns Result<Block::Hash, ConsensusError>

[cla-bot-2021]

User @tau3, please sign the CLA here.

[andresilva]

Thank you for your contribution @tau3. This will also require a small PR on polkadot to adapt to the interface change, but I can prepare that. Overall LGTM, just some minor changes needed.

[andresilva]

And these as well.

[andresilva]

Let's drop these formatting changes.

[andresilva]

Suggested change

	.map(|maybe_hash| { maybe_hash.unwrap_or(target_hash) })
	.map(|maybe_hash| maybe_hash.unwrap_or(target_hash))

[andresilva]

We should just remove this test now.

[andresilva]

I think this may no longer be true, there is a possibility that SelectChain::finality_target just returned the passed value without checking the DB at all (in practice it shouldn't happen as we can only have previously finalized something that already existed in the database). Either way we should not call expect here and instead handle the case where we get None from this call. Something like this should work:

let best_header = match client.header(BlockId::Hash(best_hash))? {
    Some(header) => header,
    None => {
        debug!(target: "afg", "Couldn't find target block from `finality_target`: {:?}", best_hash);
        return Ok(None);
    },
};

[andresilva]

And sorry for the delay in review! 🙏

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.