Finality notification: Optimize calculation of stale heads #11200

bkchr · 2022-04-11T15:59:00Z

While looking into some problem on Versi where a collator seemed to be stuck. I found out that it
was not stuck but there was a huge gap between last finalized and best block. This lead to a lot
leaves and it was basically trapped inside some loop of reading block headers from the db to find
the stale heads. While looking into this I found out that leaves already supports the feature to
give us the stale heads relative easily. However, the semantics change a little bit. Instead of
returning all stale heads of blocks that are not reachable anymore after finalizing a block, we
currently only return heads with a number lower than the finalized block. This should be no problem,
because these other leaves that are stale will be returned later when a block gets finalized which
number is bigger than the block number of these leaves.

While doing that, I also changed tree_route of the FinalityNotification to include the
old_finalized. Based on the comment I assumed that this was already part of it. However, if
wanted, I can revert this change.

While looking into some problem on Versi where a collator seemed to be stuck. I found out that it was not stuck but there was a huge gap between last finalized and best block. This lead to a lot leaves and it was basically trapped inside some loop of reading block headers from the db to find the stale heads. While looking into this I found out that `leaves` already supports the feature to give us the stale heads relative easily. However, the semantics change a little bit. Instead of returning all stale heads of blocks that are not reachable anymore after finalizing a block, we currently only return heads with a number lower than the finalized block. This should be no problem, because these other leaves that are stale will be returned later when a block gets finalized which number is bigger than the block number of these leaves. While doing that, I also changed `tree_route` of the `FinalityNotification` to include the `old_finalized`. Based on the comment I assumed that this was already part of it. However, if wanted, I can revert this change.

bkchr · 2022-04-11T16:04:00Z

client/api/src/leaves.rs

+	/// This means that no changes are done.
+	///
+	/// Returns the leaves that would be displaced.
+	pub fn simulate_finalize_height(&self, number: N) -> FinalizationDisplaced<H, N> {


I'm happy to accept any kind of better name :D

finalize_height_dry_run ?

displaced_by_finalize_height?

andresilva · 2022-04-11T16:05:29Z

client/api/src/client.rs

 	/// Path from the old finalized to new finalized parent (implicitly finalized blocks).
+	///
+	/// This maps to the range `[old_finalized, new_finalized]`.


FWIW I was also under the impression that this would already include the last finalized block.

andresilva · 2022-04-11T16:17:54Z

client/api/src/leaves.rs

+	/// This means that no changes are done.
+	///
+	/// Returns the leaves that would be displaced.
+	pub fn simulate_finalize_height(&self, number: N) -> FinalizationDisplaced<H, N> {


displaced_by_finalize_height?

client/service/src/client/client.rs

andresilva · 2022-04-11T16:20:03Z

client/service/src/client/client.rs

-				// It is not guaranteed that `backend.blockchain().leaves()` doesn't return
-				// heads that were in a stale state before this finalization and thus already
-				// included in previous notifications. We want to skip such heads.
-				// Given the "route" from the currently finalized block to the head under
-				// analysis, the condition for it to be added to the new stale heads list is:
-				// `!retracted.is_empty() && last_finalized_number <= pivot.number`
-				// 1. "route" has some "retractions".
-				// 2. previously finalized block number is not greater than the "route" pivot:
-				//    - if `last_finalized_number <= pivot.number` then this is a new stale head;
-				//    - else the stale head was already included by some previous finalization.


This way we also don't need to deal with this as we are guaranteed that stuff only gets displaced once. Sorry for making you waste brain cycles @davxy.

No worries. Has been very instructive anyway :-)

davxy · 2022-04-11T17:01:04Z

client/api/src/client.rs

@@ -307,6 +307,8 @@ pub struct FinalityNotification<Block: BlockT> {
 	/// Finalized block header.
 	pub header: Block::Header,
 	/// Path from the old finalized to new finalized parent (implicitly finalized blocks).
+	///
+	/// This maps to the range `[old_finalized, new_finalized]`.


Suggested change

/// This maps to the range `[old_finalized, new_finalized]`.

/// This maps to the range `(old_finalized, new_finalized)`.

The new_finalized hash is not included in the FinalityNotification::tree_route. Is extracted here when mapping the FinalizeSummary to FinalityNotification.

As has been discussed at the time, the last block is included in the FinalityNotification as a separate explicit field to be more "inline" with the design of the import notification.

The tree_route field should contain from old-finalized (excluded) to new finalized (excluded).

(the prev explicitly finalized block is not included here to prevent duplication, see request for change comment below)

It already contained the new_finalized, so this suggestion is not really correct.

davxy · 2022-04-11T17:26:54Z

client/service/src/client/client.rs

+			let finalized = std::iter::once(last_finalized)
+				.chain(route_from_finalized.enacted().iter().map(|elem| elem.hash))
+				.collect::<Vec<_>>();


Suggested change

let finalized = std::iter::once(last_finalized)

.chain(route_from_finalized.enacted().iter().map(|elem| elem.hash))

.collect::<Vec<_>>();

let finalized =

route_from_finalized.enacted().iter().map(|elem| elem.hash).collect::<Vec<_>>();

I didn't included the last_fnalized (i.e. the previous last finalized block) to prevent a double notification for that block:

1st notification when it is the last finalized block (explicitly finalized one)

2nd notification when it is the fist implicitly finalized block

Indeed in the tests now a2 is repeated twice:

substrate/client/service/test/src/client/mod.rs

Lines 1326 to 1328 in 9ce46c3

finality_notification_check(&mut finality_notifications, &[genesis, a1.hash(), a2.hash()], &[]);

finality_notification_check(&mut finality_notifications, &[a2.hash(), a3.hash()], &[b2.hash()]);

Genesis is the only block for which a notification was never sent since it is never explicitly finalized via a block import (implicitly finalized at startup)

In this way if blocks are (as it is usually the "normal" case) finalized one after the other then for each finalized block you'll always receive two finalizations (currently finalized one + prev finalized one)

You convinced me :)

@davxy done :)

davxy · 2022-04-11T17:42:44Z

Excluded the requested modification about removing the previous last finalized block from the notification, LGTM and is indeed faster :-)

davxy · 2022-04-11T17:50:30Z

client/service/test/src/client/mod.rs

 	assert!(finality_notifications.try_next().is_err());
 }

 #[test]
 fn finality_notifications_content() {
+	sp_tracing::try_init_simple();


Maybe this is a leftover

Sort of, but also can stay :)

Co-authored-by: André Silva <[email protected]>

davxy

LGTM 👍

davxy · 2022-04-12T06:58:44Z

Nevermind, you've already fixed it :-)

bkchr · 2022-04-12T07:32:50Z

bot merge

paritytech-processbot · 2022-04-12T07:32:56Z

Waiting for commit status.

paritytech-processbot · 2022-04-12T07:58:15Z

Merge cancelled due to error. Error: Checks failed for 7ed136b

xlc · 2022-04-12T11:25:18Z

Does this help paritytech/cumulus#432?

bkchr · 2022-04-12T11:27:00Z

No

bkchr · 2022-04-12T11:27:22Z

That is something completely different.

…h#11200) * Finality notification: Optimize calculation of stale heads While looking into some problem on Versi where a collator seemed to be stuck. I found out that it was not stuck but there was a huge gap between last finalized and best block. This lead to a lot leaves and it was basically trapped inside some loop of reading block headers from the db to find the stale heads. While looking into this I found out that `leaves` already supports the feature to give us the stale heads relative easily. However, the semantics change a little bit. Instead of returning all stale heads of blocks that are not reachable anymore after finalizing a block, we currently only return heads with a number lower than the finalized block. This should be no problem, because these other leaves that are stale will be returned later when a block gets finalized which number is bigger than the block number of these leaves. While doing that, I also changed `tree_route` of the `FinalityNotification` to include the `old_finalized`. Based on the comment I assumed that this was already part of it. However, if wanted, I can revert this change. * FMT * Update client/service/src/client/client.rs Co-authored-by: André Silva <[email protected]> * Do not include the last finalized block * Rename function * FMT * Fix tests * Update figure Co-authored-by: André Silva <[email protected]>

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. labels Apr 11, 2022

bkchr requested review from andresilva, davxy and arkpar April 11, 2022 15:59

FMT

9ce46c3

bkchr commented Apr 11, 2022

View reviewed changes

andresilva approved these changes Apr 11, 2022

View reviewed changes

davxy reviewed Apr 11, 2022

View reviewed changes

davxy suggested changes Apr 11, 2022

View reviewed changes

davxy reviewed Apr 11, 2022

View reviewed changes

bkchr and others added 2 commits April 11, 2022 20:29

Update client/service/src/client/client.rs

abc07dd

Co-authored-by: André Silva <[email protected]>

Do not include the last finalized block

78fed46

bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Apr 11, 2022

bkchr requested a review from davxy April 11, 2022 18:38

bkchr added 2 commits April 11, 2022 20:41

Rename function

0a30125

FMT

09ef2e9

davxy approved these changes Apr 11, 2022

View reviewed changes

bkchr added 2 commits April 12, 2022 08:07

Fix tests

69ae5e5

Update figure

7ed136b

github-actions bot added the A7-needspolkadotpr label Apr 12, 2022

bkchr merged commit ebef053 into master Apr 12, 2022

bkchr deleted the bkchr-optimize-finality-notification branch April 12, 2022 11:12

Dengjianping mentioned this pull request Jul 7, 2022

Bump deps to v0.9.22 Manta-Network/Manta#571

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finality notification: Optimize calculation of stale heads #11200

Finality notification: Optimize calculation of stale heads #11200

bkchr commented Apr 11, 2022

bkchr Apr 11, 2022

davxy Apr 11, 2022 •

edited

Loading

andresilva Apr 11, 2022

andresilva Apr 11, 2022

andresilva Apr 11, 2022

andresilva Apr 11, 2022

davxy Apr 11, 2022

davxy Apr 11, 2022 •

edited

Loading

bkchr Apr 11, 2022

davxy Apr 11, 2022

davxy Apr 11, 2022 •

edited

Loading

bkchr Apr 11, 2022

bkchr Apr 11, 2022

davxy commented Apr 11, 2022

davxy Apr 11, 2022

bkchr Apr 11, 2022

davxy left a comment

davxy commented Apr 12, 2022

bkchr commented Apr 12, 2022

paritytech-processbot bot commented Apr 12, 2022

paritytech-processbot bot commented Apr 12, 2022

xlc commented Apr 12, 2022

bkchr commented Apr 12, 2022

bkchr commented Apr 12, 2022

	/// This maps to the range `[old_finalized, new_finalized]`.
	/// This maps to the range `(old_finalized, new_finalized)`.

	finality_notification_check(&mut finality_notifications, &[genesis, a1.hash(), a2.hash()], &[]);

	finality_notification_check(&mut finality_notifications, &[a2.hash(), a3.hash()], &[b2.hash()]);

Finality notification: Optimize calculation of stale heads #11200

Finality notification: Optimize calculation of stale heads #11200

Conversation

bkchr commented Apr 11, 2022

Choose a reason for hiding this comment

davxy Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy left a comment

Choose a reason for hiding this comment

davxy commented Apr 12, 2022

bkchr commented Apr 12, 2022

paritytech-processbot bot commented Apr 12, 2022

paritytech-processbot bot commented Apr 12, 2022

xlc commented Apr 12, 2022

bkchr commented Apr 12, 2022

bkchr commented Apr 12, 2022

davxy Apr 11, 2022 •

edited

Loading

davxy Apr 11, 2022 •

edited

Loading

davxy Apr 11, 2022 •

edited

Loading