Fix missing block number issue on forced canonicalization #12949

bkchr · 2022-12-15T23:07:34Z

There is this issue about missing block numbers on forced canonicalization. I looked over the code now 10000 times and there are possible ways this can be triggered, but I don't really know how this is triggered. So, this pr is going to solve the symptom and not the cause. The block number to hash mapping is set when we import a new best block. Forced canonicalization will now stop at the best block and it will canonicalize the other blocks later when the best block moved. As the error reports indicated that this issue mainly happened on major sync, there should not be any forks, so not doing the canonicalization directly shouldn't be that harmful. All known implementations should import all blocks as best block on major sync anyway (I mean somewhere there is the bug, but I didn't yet found it).

I will also do some changes to Cumulus around some potential culprit for this issue.

Closes: #12613

There is this issue about missing block numbers on forced canonicalization. I looked over the code now 10000 times and there are possible ways this can be triggered, but I don't really know how this is triggered. So, this pr is going to solve the symptom and not the cause. The block number to hash mapping is set when we import a new best block. Forced canonicalization will now stop at the best block and it will canonicalize the other blocks later when the best block moved. As the error reports indicated that this issue mainly happened on major sync, there should not be any forks, so not doing the canonicalization directly shouldn't be that harmful. All known implementations should import all blocks as best block on major sync anyway (I mean somewhere there is the bug, but I didn't yet found it). I will also do some changes to Cumulus around some potential culprit for this issue. Closes: #12613

cheme

calling force canonicalize on the whole range of blocks sounds like a good idea to me, but on this decision I might be missing some subtleties.
a bit out of context, but there is one thing I sometime would find useful when working on these part of code, would be a better fork-tree (storing parent-child relation bidirectional and managed from client db with a good caching layer), not strictly needed but would make some things generally simpler and help debugging. (mentioning it as in the past I remember trying to implement that in some experimental branch)

client/db/src/lib.rs

arkpar · 2022-12-16T09:42:56Z

IMO it would be simpler for force_delayed_canonicalize to not accept a block hash/number at all. Simply call state_db.canonicalize() while state_db.best_canonical() returns a block that's further than canonicalization_delay from the best block.

bkchr · 2022-12-16T10:13:45Z

IMO it would be simpler for force_delayed_canonicalize to not accept a block hash/number at all. Simply call state_db.canonicalize() while state_db.best_canonical() returns a block that's further than canonicalization_delay from the best block.

But then canonicalization_delay == 0 will not work. However, I like the general idea

bkchr · 2022-12-16T10:17:33Z

Okay, I just checked the state_db. There is no canonicalize function?

arkpar · 2022-12-16T10:38:00Z

Sorry, I mean canonicalize_block
canonicalization_delay == 0 makes no sense in real life. It means that any imported block should be canonicalized immediately so no re-orgs are possible. The forced canonicalization is a mechanism to prevent state build-up in memory when finality stalls for whatever reason. The delay value should be high enough to allow a plausible re-org and low enough to prevent out-of-memory situation.

bkchr · 2022-12-16T10:50:55Z

Sorry, I mean canonicalize_block canonicalization_delay == 0 makes no sense in real life.

Yeah, I just wanted to hint this :P

arkpar · 2022-12-16T11:21:47Z

client/db/src/lib.rs

+		let best_canonical = || self.storage.state_db.best_canonical().unwrap_or(0);
+		let info = self.blockchain.info();
+		let best_number: u64 = self.blockchain.info().best_number.saturated_into();
+		let best_hash = info.best_hash;


This could be moved to line 1332 below, or removed entirely

bkchr · 2022-12-16T13:33:53Z

bot merge

paritytech-processbot · 2022-12-16T13:34:00Z

Waiting for commit status.

Backport paritytech/substrate#12949

Polkadot-Forum · 2023-01-20T07:22:42Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1

…#12949) * Fix missing block number issue on forced canonicalization There is this issue about missing block numbers on forced canonicalization. I looked over the code now 10000 times and there are possible ways this can be triggered, but I don't really know how this is triggered. So, this pr is going to solve the symptom and not the cause. The block number to hash mapping is set when we import a new best block. Forced canonicalization will now stop at the best block and it will canonicalize the other blocks later when the best block moved. As the error reports indicated that this issue mainly happened on major sync, there should not be any forks, so not doing the canonicalization directly shouldn't be that harmful. All known implementations should import all blocks as best block on major sync anyway (I mean somewhere there is the bug, but I didn't yet found it). I will also do some changes to Cumulus around some potential culprit for this issue. Closes: paritytech#12613 * Add some docs * Fix fix * Review comments * Review comments

bkchr added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 15, 2022

bkchr requested review from cheme and arkpar December 15, 2022 23:07

bkchr added 2 commits December 16, 2022 00:10

Add some docs

1ffbbb0

Fix fix

dc60786

cheme reviewed Dec 16, 2022

View reviewed changes

client/db/src/lib.rs Show resolved Hide resolved

client/db/src/lib.rs Outdated Show resolved Hide resolved

Review comments

b59a9a0

arkpar reviewed Dec 16, 2022

View reviewed changes

arkpar approved these changes Dec 16, 2022

View reviewed changes

Review comments

71b87a6

cheme approved these changes Dec 16, 2022

View reviewed changes

paritytech-processbot bot merged commit 85f660f into master Dec 16, 2022

paritytech-processbot bot deleted the bkchr-db-fix-canonicalize branch December 16, 2022 14:01

jasl added a commit to Phala-Network/khala-parachain that referenced this pull request Dec 20, 2022

Backport paritytech/substrate#12949

1656e8e

jasl added a commit to Phala-Network/khala-parachain that referenced this pull request Dec 30, 2022

Merge pull request #236 from Phala-Network/backport-substrate-pr-12949

8813f35

Backport paritytech/substrate#12949

librelois mentioned this pull request Jan 23, 2023

Update substrate/polkadot/cumulus from v0.9.32 to v0.9.37 moonbeam-foundation/moonbeam#2059

Closed

AurevoirXavier mentioned this pull request Feb 8, 2023

To polkadot-v0.9.37 darwinia-network/darwinia-2.0#267

Closed

kacperzuk-neti mentioned this pull request Feb 14, 2023

Polkadot v0.9.37 (pallet-nfts) liberland/liberland_substrate#244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing block number issue on forced canonicalization #12949

Fix missing block number issue on forced canonicalization #12949

bkchr commented Dec 15, 2022

cheme left a comment

arkpar commented Dec 16, 2022 •

edited

Loading

bkchr commented Dec 16, 2022

bkchr commented Dec 16, 2022

arkpar commented Dec 16, 2022 •

edited

Loading

bkchr commented Dec 16, 2022

arkpar Dec 16, 2022

bkchr commented Dec 16, 2022

paritytech-processbot bot commented Dec 16, 2022

Polkadot-Forum commented Jan 20, 2023

Fix missing block number issue on forced canonicalization #12949

Fix missing block number issue on forced canonicalization #12949

Conversation

bkchr commented Dec 15, 2022

cheme left a comment

Choose a reason for hiding this comment

arkpar commented Dec 16, 2022 • edited Loading

bkchr commented Dec 16, 2022

bkchr commented Dec 16, 2022

arkpar commented Dec 16, 2022 • edited Loading

bkchr commented Dec 16, 2022

arkpar Dec 16, 2022

Choose a reason for hiding this comment

bkchr commented Dec 16, 2022

paritytech-processbot bot commented Dec 16, 2022

Polkadot-Forum commented Jan 20, 2023

arkpar commented Dec 16, 2022 •

edited

Loading

arkpar commented Dec 16, 2022 •

edited

Loading