Finalization stalling issues 2024-07

[MOZGIII]

This is a tracking issue for the recurring finalization stalling incidents.

So far, here are the pointers we have.

Commits that are affecting the finalization directly:

• Fix longest chain finalization target lookup paritytech/substrate#13289
  Should not the the cause though, as the issue is with the inability of grandpa to follow a reorg, which is cause by pruned blocks. Or, rather, it could be the cause, but this one is not really where the bug we'd like to fix is.
• Fix block pruning after long re-org paritytech/substrate#13323 - could this be the reason? no, this one works as intended
• grandpa: don't error if best block and finality target are inconsistent paritytech/substrate#13364 - this looks like it directly messes with what's causing the bug, but more like it fixes the aftermath of the one above

Notable historical commits that might've affected this incident:

• Epoch-Changes tree pruning was lagging by one epoch paritytech/substrate#12567
• babe: allow skipping over empty epochs paritytech/substrate#11727 (unlikely though)
• Remove uncles related code paritytech/substrate#13216 (actually looks somewhat interesting)
• Notification-based block pinning paritytech/substrate#13157 (maybe pinning the block that is used as a finality based is a good idea)

Probably irrelevant:

• rpc: Use the blocks pinning API for chainHead methods paritytech/substrate#13233
• rpc/chainHead: Fix pruned blocks events from forks paritytech/substrate#13379
• Reduce consensus spam paritytech/substrate#1658
• BABE: Fix aux data cleaning paritytech/substrate#11263
• grandpa: cleanup stale entries in set id session mapping paritytech/substrate#13237
• BlockId removal: runtime-api refactor paritytech/substrate#13255

Related things from the future:

• Change forks pruning algorithm. paritytech/polkadot-sdk#3962 - new fork pruning algorithm - might work better, but not sure
• Finalization hangs in 1.13 paritytech/polkadot-sdk#4903 - irrelevant for our issue, as this issue is with the new fork pruning algorithm mentioned above being too slow

---

The thoughts we have so far is that the issue is caused by the broken block pruning (which is meaningful when an unfortunate best block selection happens). (UPD: the blocks are not really pruned, we can still request them from the API; this means this is a selection issue) Grandpa tries to finalize the block that is pruned and not the part of the best chain anymore; upon receiving the precommits it fails to recognize them because the block can't be resolved (it's pruned). This is quite odd, since the nodes operate with --blocks-pruning archive, so it must mean they haven't seen the block in question as is it no never to be pruned if seen - but in reality we see a the reorg happening from the block in question to a new one, so the old block is definitely a known one.

---

We are currently on substrate 0.9.40.

---

Mainnet encountered this at least three times lately:

[MOZGIII]

2024-07-26 02:31:13.467  WARN tokio-runtime-worker grandpa: Encountered error finding best chain containing 0xab3e065e87c2c2e3ac34cb07a0e05c1fa7dd7df2d782fee66a8c284bbd430fd0: couldn't find target block: Chain lookup failed: Requested a finality target using a base 0xab3e065e87c2c2e3ac34cb07a0e05c1fa7dd7df2d782fee66a8c284bbd430fd0 not in the best chain 0x325ab807d115074518d0c5bd764de875b0945d0166d0937753968f98b4c2805d    

Explore-logs-2024-07-26 10_41_41.txt
Explore-logs-2024-07-26 10_53_12.txt
The logs from the node to hint what's going on

[MOZGIII]

Current plan for a workaround is to delay the finality for more than 2 blocks. Currently we are planning to delay the finality after by 5 7 blocks beyond the best block. This should help with reducing the chance of the reorg-caused race condition we are facing.

[dmitrylavrenov]

Added the test to reproduce the issue logic - humanode-network/polkadot-sdk@master...humanode-network:polkadot-sdk:grandpa-voting-and-babe-reorg-race-condition.

It shows the root cause that we face.

[MOZGIII]

We have analyzed the code, and it looks like the issue is with those two PRs:

• Fix longest chain finalization target lookup paritytech/substrate#13289
• grandpa: don't error if best block and finality target are inconsistent paritytech/substrate#13364

We think that the fix that is added at paritytech/substrate#13364 does not apply after the code added at paritytech/substrate#13289 returns an error. We'll investigate further.

We are still on course to apply the workaround mentioned above as a hotfix, but the long term solution is also in sight.

[MOZGIII]

To elaborate more on paritytech/substrate#13364:

1. LongestChain::finality_target will error out when the passed target block is not in the best chain (https://github.com/paritytech/substrate/pull/13289/files#diff-97e7e3622ec6f0b455d18c1bf8055ccf5cfd210efd50348df6b07fce4531baeeR115-R119). This also applies to the case where the block number of the target and best-chain block are the same.
2. The fixup code at https://github.com/paritytech/substrate/pull/13364/files#diff-5ecb4cf31ed389a0d12482137c8b234485b199c200b09446cf8876321527446aR1242 tries to use the best block as target block (best block to target block) but in practice that part of the fn is not reached due to how an error from LongestChain::finality_target above is handled.
3. As a result, the intended fix is not applied, and we see the error logged instead - but also the consensus stalls.

[bkchr]

Added the test to reproduce the issue logic - humanode-network/polkadot-sdk@master...humanode-network:polkadot-sdk:grandpa-voting-and-babe-reorg-race-condition.

The test is succeeding?

[dmitrylavrenov]

Added the test to reproduce the issue logic - humanode-network/polkadot-sdk@master...humanode-network:polkadot-sdk:grandpa-voting-and-babe-reorg-race-condition.

The test is succeeding?

Yep, we've implemented it in the way to verify our thoughts about the happened case.

[dmitrylavrenov]

We met the same error

	// `hashof8_a` should be finalized next based on last completed round data,
	// but it's not an valid finality target
	assert_matches!(
		env.select_chain.finality_target(hashof8_a, None).await.unwrap_err(),
		ConsensusError::ChainLookup(_)
	);

[andresilva]

I have only looked into this superficially, but I think @MOZGIII comment on the LongestChain::finality_target issue is correct. The root cause is that we shouldn't error when the given vote target is not in the best chain, otherwise we make it impossible for finality to proceed in case we have a round estimate that gets re-orged out. In principle what should happen is grandpa would finalize the fork that got re-orged out, and then the node would re-org again (after grandpa finalizes). I didn't think through all the consequences of such a change.

[dmitrylavrenov]

I have only looked into this superficially, but I think @MOZGIII comment on the LongestChain::finality_target issue is correct. The root cause is that we shouldn't error when the given vote target is not in the best chain, otherwise we make it impossible for finality to proceed in case we have a round estimate that gets re-orged out. In principle what should happen is grandpa would finalize the fork that got re-orged out, and then the node would re-org again (after grandpa finalizes). I didn't think through all the consequences of such a change.

The same thoughts we discussed with @MOZGIII

[MOZGIII]

@bkchr @andresilva should we expect any hotfix for this implemented at the parity side or rather work on this ourselves?