Wait for block import in parachain consensus #271

bkchr · 2020-12-26T21:13:37Z

There was a bug in the parachain consensus that when importing a relay
chain block that sets a new best parachain block, but the required
parachain block was not yet imported. This pr fixes this by waiting for
the block to be imported.

Closes: #203

There was a bug in the parachain consensus that when importing a relay chain block that sets a new best parachain block, but the required parachain block was not yet imported. This pr fixes this by waiting for the block to be imported.

…rt-new-best-stuff

rphmeier · 2020-12-27T21:18:58Z

consensus/src/lib.rs

-fn finalize_block<T, Block, B>(client: &T, hash: Block::Hash) -> ClientResult<bool>
+/// Follow the finalized head of the given parachain.
+///
+/// For every finalized block of the relay chain,


trailing doc

rphmeier · 2020-12-27T21:21:55Z

consensus/src/lib.rs

+			}
+		};
+
+		let hash = header.hash();


does the header actually need to be decoded for this? blake2(&finalized_head[..]) should give the same hash. We would get an UnknownBlock error if it's an invalid header or we haven't yet seen the header.

As the header is generic, a user could have whatever implementation for hash() they wants.

consensus/src/lib.rs

rphmeier · 2020-12-27T21:25:48Z

consensus/src/lib.rs

+		if parachain.usage_info().chain.finalized_hash != hash {
+			if let Err(e) = parachain.finalize_block(BlockId::hash(hash), None, true) {
+				match e {
+					ClientError::UnknownBlock(_) => tracing::debug!(


I suppose here there is a similar error to the one that the PR fixes. If the relay chain informs us about a finalized header before we've imported that header, then we won't finalize it, even if we import that same header immediately afterwards.

We will correctly finalize probably soon after, when we get a finality notification corresponding to a header we have already imported.

That is right, but also done intentionally by me. I do not expect that we are that much behind the tip that we can not finalize the block, because we did not have this block yet. If that is happening, we probably have other problems.

Maybe should be filed for the "wishlist" but yeah I agree. Even the slight lag in finality should mean that blocks are well propagated before finality.

rphmeier · 2020-12-27T21:26:32Z

consensus/src/lib.rs

+	// The unset best header of the parachain. Will be `Some(_)` when we have imported a relay chain
+	// block before the parachain block it included. In this case we need to wait for this block to
+	// be imported to set it as new best.
+	let mut unset_best_header = None;


Down the line, we may even need to trigger an availability request to recover the PoV. It is possible that the initial collator is withholding the block.

I thought that we don't wanted to prevent to ask the relay chain validators to recover the povblocks?

But yeah, in general you are right. However this is not yet implemented anywhere.

consensus/src/lib.rs

rphmeier · 2020-12-27T21:29:46Z

consensus/src/lib.rs

+		tracing::debug!(
+			target: "cumulus-consensus",
+			block_hash = ?hash,
+			"Skipping set new best block, because block is already the best.",


i'm curious if we do see duplicate notifications like this often.

Anecdotally, I've never noticed it.

Every relay chain block that does not includes a new parachain block, will trigger this log line. So, with rococo v1 at least every second block.

Ah, yeah, it's a debug line. I've never seen it because I never run with that debug target.

rphmeier · 2020-12-27T21:30:30Z

consensus/src/lib.rs

+			Ok(BlockStatus::InChainWithState) => {
+				unset_best_header.take();
+
+				import_block_as_new_best(hash, parachain_head, parachain, announce_block);


why do we import as new best even though it's already InChainWithState? Is it a fork-choice rule thing? is there no other API?

AFAIK there is no other API for this. In the end we just import the header a second time and say that it should be imported as best block.

There once existed an API for this, but it was removed at some point.

rphmeier · 2020-12-27T21:35:07Z

consensus/src/lib.rs

+				tracing::error!(
+					target: "cumulus-collator",
+					block_hash = ?hash,
+					"Trying to set pruned block as new best!",


I guess this might happen as a chain reversion, but probably not since pruning happens after finality

rphmeier

Thanks for tests

rphmeier · 2020-12-27T21:37:20Z

What was the downside of that bug? I suppose collators would still build on the right head because that's determined by the collation generation subsystem. So the RPC would show incorrect data as the "best block" was not updated?

bkchr · 2020-12-27T23:49:42Z

What was the downside of that bug? I suppose collators would still build on the right head because that's determined by the collation generation subsystem. So the RPC would show incorrect data as the "best block" was not updated?

This is not mainly about collators. Collators would still not be able to build a block, if they don't know the parachain block yet. So, we may need a similar fix for the collation as well. We already have something similar for block announcement validation, but there we need to wait for a relay chain block.

This is fix is more for full nodes like rpc or whatever nodes. With rococo v0 we had seen slow nodes like raspberry pi suffering under this problem that one of the blocks was always imported first. Maybe it was a combination of multiple things that lead to this to the seen problems. However, I more like to have this thing fixed to be sure we don't see this.
This could for example also lead to observations of a higher block time with rococo v1, because the rpc parachain rpc node imported the relay chain block before the parachain block.

bkchr · 2021-01-05T21:56:19Z

bot merge

ghost · 2021-01-05T21:56:21Z

Waiting for commit status.

Wait for block import in parachain consensus

529c95b

There was a bug in the parachain consensus that when importing a relay chain block that sets a new best parachain block, but the required parachain block was not yet imported. This pr fixes this by waiting for the block to be imported.

bkchr added the A0-pleasereview label Dec 26, 2020

bkchr requested a review from rphmeier December 26, 2020 21:13

Merge remote-tracking branch 'origin/master' into bkchr-fix-some-impo…

b597da6

…rt-new-best-stuff

rphmeier reviewed Dec 27, 2020

View reviewed changes

consensus/src/lib.rs Show resolved Hide resolved

rphmeier reviewed Dec 27, 2020

View reviewed changes

consensus/src/lib.rs Show resolved Hide resolved

rphmeier reviewed Dec 27, 2020

View reviewed changes

rphmeier approved these changes Dec 27, 2020

View reviewed changes

Finish docs

b26fe97

ghost merged commit 9f0085c into master Jan 5, 2021

ghost deleted the bkchr-fix-some-import-new-best-stuff branch January 5, 2021 22:14

bkchr mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#3

Merged

This pull request was closed.

Wait for block import in parachain consensus #271

Wait for block import in parachain consensus #271

Conversation

bkchr commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier left a comment

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Dec 27, 2020

Uh oh!

bkchr commented Dec 27, 2020

Uh oh!

bkchr commented Jan 5, 2021

Uh oh!

ghost commented Jan 5, 2021

Uh oh!

bkchr commented Dec 26, 2020 •

edited

Loading

rphmeier Dec 27, 2020 •

edited

Loading