client/db: Close missing body gaps for non archive nodes

[lexnv]

This PR closes missing body gaps in the database for non-archive nodes.

Effectively, a missing body gap cannot be closed on the DB side if the node is non-archive. Since execution is already skipped, the node will close the memory gap in the sync engine; however, the gap remains open in the db.

This leads to wasting resources at every startup:

• client info contains a gap that cannot be filled (since we don't have the state around for execution)
• blocks are fetched from the connected peers
• gap is filled by ignoring blocks in the sync engine

Further, for collators on origin master this causes an infinite loop of sync engine restarts that get punished via banning and disconnecting. For more details and root cause check:

• aura/import: Skip block execution when collators have no parent block state #11330

Part of:

• master/regression: Parachain Gap cannot be filled #11299

[bkchr]

How did it come to this? Because someone restarted the node using a different pruning mode?

[lexnv]

Apparently, it was just the collator functioning "normally" and no params were changed other than switching to the master PR (which now has import_existing: true).

I believe I had a look over the client info a few years ago, reporting gaps that should be filled or that should have never existed, but I dismissed that as stopping the node at the wrong time or switching params.

I think this code was silently filling the memory gap but never informed the DB layer about not caring for the block execution:

• polkadot-sdk/substrate/client/service/src/client/client.rs

  Lines 1809 to 1812 in 2b9576c

  	BlockStatus::InChainPruned if !import_existing => {
  	return Ok(ImportResult::AlreadyInChain)
  	},
  	BlockStatus::InChainPruned => {},

And now with the optimization of not even fetching the BODY, its assumed that the DB will never close it:

• polkadot-sdk/substrate/client/service/src/client/client.rs

  Lines 1809 to 1812 in 2b9576c

  	BlockStatus::InChainPruned if !import_existing => {
  	return Ok(ImportResult::AlreadyInChain)
  	},
  	BlockStatus::InChainPruned => {},

Not entirely sure what might have caused the gap in the first place, since the nodes were running without the optimization of stripping BODY initially 🤔 So i would have expected that all bodies were fetched as well

[lrubasze]

LGTM!

Just thinking that maybe it would make sense to add a test (or modify current) that checks if multi-block gaps are closed as well. Seems more real-world scenario.

[skunert]

Still unclear to me what happened. IMO this PR is fixing the symptom, not the issue.

It looks like we write this kind of gap only here:

polkadot-sdk/substrate/client/db/src/lib.rs

Line 1914 in 8604871

gap_type: BlockGapType::MissingBody,

But there we only go if create_gap is true and its above best block:

polkadot-sdk/substrate/client/db/src/lib.rs

Line 1895 in 8604871

} else if operation.create_gap {

So this means that this issue occured at the tip. And at the tip I would expect that we can just request the body? Lukasz changes where only for gap sync below the warp target.

[lexnv]

Versions deployed on the chain:

• t0: stable2512-2 clean sync
• t1: master-14852d21 import_existing: true, which caused the sync loop
• t2: stable260

Before Lukas changes #10373, we checked if the incoming block had a body:

polkadot-sdk/substrate/client/db/src/lib.rs

Line 1520 in 4e02aa2

let existing_body = pending_block.body.is_some();

polkadot-sdk/substrate/client/db/src/lib.rs

Line 1714 in 4e02aa2

let should_check_block_gap = !existing_header || !existing_body;

• a header only import might trick the DB into thinking the body doesn't exist, although the body is already in the DB

After the changes, the two checks are separate (ie we check if the body exists in the DB not the import)

polkadot-sdk/substrate/client/db/src/lib.rs

Lines 1595 to 1599 in d5c5fbe

	// Body in DB (not incoming block) - needed to update gap when adding body to existing
	// header.
	let body_exists_in_db = self.blockchain.body(hash)?.is_some();
	// Incoming block has body - used for fast sync gap handling.
	let incoming_has_body = pending_block.body.is_some();

So, before the Lukas change, we opened a gap that wasn't supposed to get open (most probably via importing a new block as best):

polkadot-sdk/cumulus/client/consensus/common/src/parachain_consensus.rs

Line 513 in d5c5fbe

async fn import_block_as_new_best<Block, P>(hash: Block::Hash, header: Block::Header, parachain: &P)

polkadot-sdk/cumulus/client/consensus/common/src/parachain_consensus.rs

Lines 531 to 534 in d5c5fbe

	// Make it the new best block
	let mut block_import_params = BlockImportParams::new(BlockOrigin::ConsensusBroadcast, header);
	block_import_params.fork_choice = Some(ForkChoiceStrategy::Custom(true));
	block_import_params.import_existing = true;

Then because the import was header-only, the client/db assumes we need to fil the body via gap sync. Which then cascades into a sync loop restart when we switched to Lukas's changes:

• we request the block to fill the gap
• now because import_existing: true we execute the block
• execution fails because there's no parent state
• we go back to step 1 trying to fill the gap

TLDR;

• stable2512-2 had a bug in the DB whcih opened a gap when it shouldnt have
• master-14852d21 is actually fixing the bug, but because we also changed the import_existing to true, and we hit the bug from stable2512-2, we try to fill a gap and enter a sync restart loop
• Since we got data for this happening on 2 parachains (yap polkadot and yap kusama), I would still consider this fix

Would love to double check this with you guys 🙏 @skunert @lrubasze

[skunert]

So you are saying that all nodes that switch from old releases to the new release will have these "accidental gaps" and run into this bug? If yes, then we need to merge this.

[lexnv]

So you are saying that all nodes that switch from old releases to the new release will have these "accidental gaps" and run into this bug? If yes, then we need to merge this.

Yep, this has happened on 6 collators: 3 from polkadot-yap and 3 from kusama-yap parachains

[lexnv]

Have double checked AH-Westend which contains the latest 2603-rc4:

• PR aura/import: Skip block execution when collators have no parent block state #11330 ensures we are not looping during the restart process
• however, without this PR, we'll have the accidental gap live in the DB making the node request it after every reboot to only close it in-memory

[lrubasze]

@lexnv amazing analysis and great explanation.
Pretty twisted scenario :)

[lexnv]

Thanks a lot Lukas for double checking 🙏
Yep, this scenario took me by surprise as well :D

[lexnv]

/cmd prdoc --audience node_dev --bump patch

[bkchr]

@lexnv we need to backport this?

[paritytech-release-backport-bot]

Created backport PR for stable2512:

• [stable2512] Backport #11332 #11451 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11332-to-stable2512
git worktree add --checkout .worktree/backport-11332-to-stable2512 backport-11332-to-stable2512
cd .worktree/backport-11332-to-stable2512
git reset --hard HEAD^
git cherry-pick -x 0f64cfcaf9f1665216d2ea5e5f66a8e632ce423c
git push --force-with-lease

[paritytech-release-backport-bot]

Successfully created backport PR for stable2603:

• [stable2603] Backport #11332 #11452