Reject data columns that does not descend from finalize root instead of ignoring it

[jimmygchen]

Issue Addressed

This issue was identified during the fusaka audit competition.

The verify_parent_block_and_finalized_descendant in data column gossip verification currently load the parent first before checking if the column descends from the finalized root.

However, the fork_choice.get_block(&block_parent_root) function also make the same check internally:

lighthouse/consensus/fork_choice/src/fork_choice.rs

Lines 1242 to 1249 in 8a4f6cf

	/// Returns a `ProtoBlock` if the block is known **and** a descendant of the finalized root.
	pub fn get_block(&self, block_root: &Hash256) -> Option<ProtoBlock> {
	if self.is_finalized_checkpoint_or_descendant(*block_root) {
	self.proto_array.get_block(block_root)
	} else {
	None
	}
	}

Therefore, if the column does not descend from the finalized root, we return an UnknownParent error, before hitting the is_finalized_checkpoint_or_descendant check just below.

Which means we IGNORE the gossip message instead REJECT, and the gossip peer is not immediately penalised. This deviates from the spec.

However, worth noting that lighthouse will currently attempt to request the parent from this peer, and if the peer is not able to serve the parent, it gets penalised with a LowToleranceError, and will get banned after ~5 occurences.

lighthouse/beacon_node/network/src/sync/network_context.rs

Lines 1530 to 1532 in ffa7b2b

	if let Some(Err(RpcResponseError::VerifyError(e))) = &resp {
	self.report_peer(peer_id, PeerAction::LowToleranceError, e.into());
	}

This PR will penalise the bad peer immediately instead of performing block lookups before penalising it.

[michaelsproul]

Nice improvement, makes sense to me.