Allow to disable gap creation during block import

[nazar-pc]

This feature is helpful for us with custom sync protocol that is similar to Warp sync except we do not ever sync the gap and don't want it to exist in the first place (see #5333 and its references for motivation).

Otherwise we had to resort to this: autonomys@d537512

[nazar-pc]

Let me know if no_gap() should have default implementation and require only minor version bump instead

[lexnv]

cc #4984

[nazar-pc]

Not exactly sure how it is related necessarily, it still had to pass through consensus successfully before ending up here IIUC.

I do see those "block has an unknown parent" in some cases though and subscribed to linked issue a while ago.

[bkchr]

I mean this is a hack and I'm fine for now to live with this to make your life easier. My question would be, could we maybe just disable it "globally". So, that you can create the backend and tell it to never create a gap. Would that work for you?

[nazar-pc]

I think it'll work equally well for my use case. I though about it initially and couldn't decide whether there is a use case where there might be a scenarios in such gaps should be created in some cases, but not others in runtime, so I decided to make that possibility open.

In the scenario I'm thinking about one block is inserted as already finalized and I want to disable gap creation for that specific block. There are no other cases where gap would be created, so beyond that it makes no difference to me whether it is global or local decision.

[davxy]

Which block authoring engine are you using? Currently, during warp sync, importing a block within a gap causes Babe to skip certain checks that would otherwise cause the import to fail.

Block verification:

polkadot-sdk/substrate/client/consensus/babe/src/lib.rs

Lines 1149 to 1158 in 0cd577b

	if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) || block.with_state() {
	// Verification for imported blocks is skipped in two cases:
	// 1. When importing blocks below the last finalized block during network initial
	// synchronization.
	// 2. When importing whole state we don't calculate epoch descriptor, but rather read it
	// from the state after import. We also skip all verifications because there's no
	// parent state and we trust the sync module to verify that the state is correct and
	// finalized.
	return Ok(block)
	}

Block import:

polkadot-sdk/substrate/client/consensus/babe/src/lib.rs

Lines 1420 to 1431 in 0cd577b

	// Skip babe logic if block already in chain or importing blocks during initial sync,
	// otherwise the check for epoch changes will error because trying to re-import an
	// epoch change or because of missing epoch data in the tree, respectively.
	if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) ||
	block_status == BlockStatus::InChain
	{
	// When re-importing existing block strip away intermediates.
	// In case of initial sync intermediates should not be present...
	let _ = block.remove_intermediate::<BabeIntermediate<Block>>(INTERMEDIATE_KEY);
	block.fork_choice = Some(ForkChoiceStrategy::Custom(false));
	return self.inner.import_block(block).await.map_err(Into::into)
	}

[nazar-pc]

Which block authoring engine are you using?

We use Dilithium (Subspace v2.3) PoAS consensus and as described in #5333 we already have two custom sync mechanisms that are not part of Substrate either. What BABE consensus does is 100% irrelevant for us. Our consensus is not only ready for the changes in this PR, we already use changes that provide similar effect in production (testnet with ~8000 nodes) during last few months.

We already carry patches to Substrate downstream that allow us to do what we need to do, this is just one of the steps to introduce new capabilities in Substrate that would allow us to drop downstream hacks and workarounds.

As of right now stock Substrate is not flexible enough for our purposes and assumptions like existence of Warp sync are causing issues for us downstream, which I aim to correct over time.

[davxy]

I understand. It’s worth noting that BlockImportParams::create_gap is set to true by default. So Babe will not be affected.

[nazar-pc]

Looks like semver check started using wrong Rust version, so it fails to compile now, merging fresh master didn't help

[nazar-pc]

Anything else I can do in this PR to move it forward?

[davxy]

LGTM

[nazar-pc]

How can I interest more folks in reviewing this? PR is quite simple and was open for about a month now, missing September release cut.

[dmitry-markin]

Looks good to me. I'm not very familiar with backend/db, why this is better than disabling gap sync?

Also, there is #5592 that just touched the same code.

[nazar-pc]

Looks good to me. I'm not very familiar with backend/db, why this is better than disabling gap sync?

Gap sync is the consequence, the database will still have gap recorded, sync status will be misreported by informer, etc. For our chain gap sync is of no interest right now, hence we don't want gap to be created in the first place. At the same time we might want to have it in other cases in the future, which is all still possible with this PR.