babe: keep stateless verification in Verifier, move everything else to the import queue

[sistemd]

We agreed to split #8446 into two PRs: one for BABE (this one) and one for AURA. This is the easier one.

[skunert]

Only nits, looking good!

[iulianbarbu]

Other than this LGTM! Will approve after resolving comments above.

[skunert]

LGTM, nice work!

[iulianbarbu]

Very nice, thanks 🙏

[bkchr]

Generally looks good, but some nitpicks that should be solved.

[bkchr]

With removing this there would be some possible panic, because check_primary_header and friends are just accessing the authorities via the index without any bound checks. I would propose we keep this code here and just directly forward the author to these functions below.

[sistemd]

da01d0d - I did it this way since these fns require the entire epoch anyway, so no need to pass the same data in another parameter again. Let me know if you disagree.

[bkchr]

Just realized that these functions already return an error. So, we could just use get() in there. I find it better if we ensure that these functions can not panic, otherwise we may oversee this in the future.

[sistemd]

89acaed

[bkchr]

Suggested change

	NumberFor::<Block>::try_from(block_number - 1)
	.map_err(|_| "parent block exists, hence must be able to decrement it")
	.unwrap(),
	block_number - 1u32.into(),

No idea why you wanted to convert the number first to U256?

[bkchr]

Maybe because you didn't see that BlockNumber implements From<u32>?

[sistemd]

159e2fe

[bkchr]

Suggested change

let block_number: U256 = block_number.into();

[sistemd]

159e2fe

[bkchr]

Suggested change

	let (_epoch_descriptor, viable_epoch) = query_epoch_changes(
	&self.epoch_changes,
	self.client.as_ref(),
	&self.config,
	number,
	slot,
	parent_hash,
	)
	.map_err(|e| ConsensusError::Other(babe_err(e).into()))?;
	let viable_epoch = query_epoch_changes(
	&self.epoch_changes,
	self.client.as_ref(),
	&self.config,
	number,
	slot,
	parent_hash,
	)
	.map_err(|e| ConsensusError::Other(babe_err(e).into()))?.1;

[sistemd]

57a0dfc

[bkchr]

Suggested change

	let epoch = viable_epoch.as_ref();
	match epoch.authorities.get(babe_pre_digest.authority_index() as usize) {
	match viable_epoch.as_ref().authorities.get(babe_pre_digest.authority_index() as usize) {

[sistemd]

57a0dfc

[bkchr]

This should wrap the all the things that are required to check the inherents.

Also as we don't use the inherente data providers etc for anything else in here, we can just move the code directly to check_inherents.

[sistemd]

You mean like this or something else? 5b2c5fd

[bkchr]

Just realized that these functions already return an error. So, we could just use get() in there. I find it better if we ensure that these functions can not panic, otherwise we may oversee this in the future.

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/16291235740
Failed job name: cargo-clippy

[bkchr]

One last nitpick, otherwise great work!

Ty!

[bkchr]

Suggested change

	if let Some(inner_body) = block.body.take() {
	let new_block = Block::new(block.header.clone(), inner_body);
	if !block.state_action.skip_execution_checks() {
	if block.state_action.skip_execution_checks() {
	return Ok(())
	}
	if let Some(inner_body) = block.body.take() {
	let new_block = Block::new(block.header.clone(), inner_body);

[sistemd]

8e4d190

[bkchr]

Suggested change

	let equivocation_proof =
	match check_equivocation(&*self.client, slot_now, slot, header, author)
	.map_err(Error::Client)?
	{
	Some(proof) => proof,
	None => return Ok(()),
	};
	let Some(equivocation_proof) =
	check_equivocation(&*self.client, slot_now, slot, header, author)
	.map_err(Error::Client)?
	else {
	return Ok(())
	};

[sistemd]

8e4d190