Add stale branches heads to finality notifications #10639

davxy · 2022-01-11T18:59:28Z

Includes stale branches heads to finality notifications sent by the client.

Warning

Previous implementation was sending a notification for each block between the previous (explicitly) finalized block and
the new finalized one (with an hardcoded limit of 256). Thus finalization messages were sent also for implicitly finalized blocks.

Now finality notification is sent only for the new finalized head and it contains the hash of the new finalized head hash, new finalized head header, a list of all the implicitly finalized blocks and a list of stale branches heads (i.e. heads that are not part of the canonical chain anymore).

client/service/src/client/client.rs

client/api/src/client.rs

Warning. Previous implementation was sending a notification for each block between the previous (explicitly) finalized block and the new finalized one (with an hardcoded limit of 256). Now finality notification is sent only for the new finalized head and it contains the hash of the new finalized head, new finalized head header, a list of all the implicitly finalized blocks and a list of stale branches heads (i.e. the branches heads that are not part of the canonical chain anymore).

The list contains all the blocks between the previously finalized block up to the parent of the currently finalized one, sorted by block number. `Finalized` messages handler, part of the `MaintainedTransactionPool` implementation for `BasicPool`, still propagate full set of finalized blocks to the txpool by iterating over implicitly finalized blocks list.

client/transaction-pool/api/src/lib.rs

client/api/src/client.rs

davxy · 2022-01-17T17:30:37Z

@bkchr @andresilva

TL;DR

A fairly meticulous inspection of finality_notification_stream consumers;

In particular assumptions about sequential block finalization messages were checked and eventually fixed.

Looks like receiving the last finalized block message was sufficient for almost all consumers with the exception of txpool, where a fix is already contained in this PR.

A review from more experienced people for the single components is still required.

Follows a recap for Substrate, Cumulus and Polkadot with code references to simplify the inspection.

Substrate

Transaction Pool

Stream read by notification_future here
Validated transaction pool assumes to receive a notification message for each finalized block.
Message handler has been adjusted to send the full finalized list to pool.validated_pool().on_block_finalized()

See here

Beefy

Stream read by BeefyWorker here
Message is then processed here
Looks happy with final head notification.

Networking

Stream is read within build_network_future here
A wrapper to find the last finalized block was present here. This is not required anymore.
~~build_network_future forwards finality notification to network.on_block_finalized(hash, number) here~~
~~message arrives to ChainSync::on_block_finalized here~~.
~~message is forwarded to ExtraRequests::on_block_finalized in here~~

RPC

Stream read messages are forwarded to subscribers of subscribe_finalized_heads API here
WARNING: we are now sending notifications only for finalized heads but external applications may rely on previous behaviour?

Cumulus

PovRecovery

Stream is read within PovRecovery::run here
Calls handle_block_finalized(finalized_number) that is used to remove from self.pending_candidates blocks with number <= finalized_number here
Seems happy with final head notification

RelaychainClient

Stream is read within RelaychainClient for RCInterface :: finalized_heads implementation here
The stream result is then used in follow_finalized_head. here
- for each new finalized head the parachain.finalize_block(finalized_hash) is called.
- this is already correctly handled by Substrate Client implementation for Finalizer trait.
Seems happy with final head notification

Polkadot

Overseer

node/overseer/src/lib.rs

forward_events() forwards stream messages to the overseer wrapper using Handle::block_finalized here
Handle::block_finalized sends messages to Overseer task.
The event is catched within the Overseer::run and managed using Overseer::block_finalized here
- This prunes leaves below the finalized head here

The loop was there to prevent sending to `peer.network.on_block_finalized` the full list of finalized blocks. Now only the finalized heads are received.

bkchr · 2022-01-18T12:09:36Z

@davxy ty for the detailed report. Looks good as far as I can see :)

client/transaction-pool/api/src/lib.rs

andresilva

Good work @davxy, LGTM overall. For the RPC I'm not sure whether we should change it to take the summary and emit a notification for each block. We need to add some tests for the creation of stale heads because I am not sure if all cases are covered or whether we might be missing some heads.

@jacogr When multiple blocks are finalized at once, would getting a single finality notification for the latest block, as opposed to now getting one for each finalized block, have any effect on polkadot.js/apps?

client/service/src/lib.rs

client/service/src/client/client.rs

andresilva

LGTM! Nice work @davxy

client/service/src/client/client.rs

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

…ications

bkchr

Looks already very good, but needs some more adjustments.

client/service/src/client/client.rs

bkchr · 2022-01-26T15:26:16Z

Ahh and @davxy please don't use force pushes :)

Co-authored-by: Bastian Köcher <[email protected]>

client/service/src/client/client.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Add stale branches heads to finality notifications Warning. Previous implementation was sending a notification for each block between the previous (explicitly) finalized block and the new finalized one (with an hardcoded limit of 256). Now finality notification is sent only for the new finalized head and it contains the hash of the new finalized head, new finalized head header, a list of all the implicitly finalized blocks and a list of stale branches heads (i.e. the branches heads that are not part of the canonical chain anymore). * Add implicitly finalized blocks list to `ChainEvent::Finalized` message The list contains all the blocks between the previously finalized block up to the parent of the currently finalized one, sorted by block number. `Finalized` messages handler, part of the `MaintainedTransactionPool` implementation for `BasicPool`, still propagate full set of finalized blocks to the txpool by iterating over implicitly finalized blocks list. * Rust fmt * Greedy evaluation of `stale_heads` during finalization * Fix outdated assumption in a comment * Removed a test optimization that is no more relevant The loop was there to prevent sending to `peer.network.on_block_finalized` the full list of finalized blocks. Now only the finalized heads are received. * Last finalized block lookup not required anymore * Tests for block finality notifications payloads * Document a bit tricky condition to avoid duplicate finalization notifications * More idiomatic way to skip an iterator entry Co-authored-by: Bastian Köcher <[email protected]> * Cargo fmt iteration * Typo fix Co-authored-by: Bastian Köcher <[email protected]> * Fix potential failure when a finalized orphan block is imported * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

davxy requested review from andresilva and bkchr January 11, 2022 18:59

davxy assigned bkchr Jan 11, 2022

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 11, 2022

bkchr removed their assignment Jan 11, 2022

bkchr reviewed Jan 11, 2022

View reviewed changes

client/service/src/client/client.rs Outdated Show resolved Hide resolved

bkchr reviewed Jan 11, 2022

View reviewed changes

client/api/src/client.rs Outdated Show resolved Hide resolved

davxy force-pushed the davxy-stale-heads-notify branch from 3320823 to efefe54 Compare January 12, 2022 14:00

davxy requested a review from bkchr January 12, 2022 14:04

davxy marked this pull request as ready for review January 12, 2022 14:15

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 12, 2022

davxy added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 12, 2022

davxy marked this pull request as draft January 12, 2022 15:10

davxy added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 15, 2022

davxy marked this pull request as ready for review January 15, 2022 12:03

Rust fmt

f063fc2

bkchr reviewed Jan 17, 2022

View reviewed changes

client/transaction-pool/api/src/lib.rs Show resolved Hide resolved

client/api/src/client.rs Show resolved Hide resolved

davxy added 2 commits January 17, 2022 13:14

Greedy evaluation of stale_heads during finalization

55e470a

Fix outdated assumption in a comment

83830ff

davxy added 2 commits January 17, 2022 18:41

Removed a test optimization that is no more relevant

3e59449

The loop was there to prevent sending to `peer.network.on_block_finalized` the full list of finalized blocks. Now only the finalized heads are received.

Last finalized block lookup not required anymore

0c4d4e2

davxy force-pushed the davxy-stale-heads-notify branch from dbbe45a to 0c4d4e2 Compare January 18, 2022 11:12

davxy added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jan 18, 2022

bkchr reviewed Jan 18, 2022

View reviewed changes

client/transaction-pool/api/src/lib.rs Show resolved Hide resolved

andresilva reviewed Jan 18, 2022

View reviewed changes

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

client/service/src/client/client.rs Show resolved Hide resolved

client/service/src/client/client.rs Outdated Show resolved Hide resolved

davxy force-pushed the davxy-stale-heads-notify branch from ac4309a to 02944e7 Compare January 20, 2022 19:24

Tests for block finality notifications payloads

1529b5b

davxy force-pushed the davxy-stale-heads-notify branch from 02944e7 to 1529b5b Compare January 21, 2022 08:35

davxy requested review from andresilva and bkchr January 24, 2022 10:40

andresilva approved these changes Jan 25, 2022

View reviewed changes

client/service/src/client/client.rs Outdated Show resolved Hide resolved

client/service/test/src/client/mod.rs Show resolved Hide resolved

Document a bit tricky condition to avoid duplicate finalization notif…

e242b42

…ications

davxy force-pushed the davxy-stale-heads-notify branch from 1795920 to e242b42 Compare January 26, 2022 10:52

bkchr reviewed Jan 26, 2022

View reviewed changes

davxy and others added 5 commits January 26, 2022 16:35

Merge branch 'master' into davxy-stale-heads-notify

cfe97ec

More idiomatic way to skip an iterator entry

8c200fc

Co-authored-by: Bastian Köcher <[email protected]>

Cargo fmt iteration

316f193

Typo fix

8d5ace7

Co-authored-by: Bastian Köcher <[email protected]>

Fix potential failure when a finalized orphan block is imported

f77154c

github-actions bot added the A7-needspolkadotpr label Jan 27, 2022

bkchr approved these changes Jan 27, 2022

View reviewed changes

davxy and others added 2 commits January 27, 2022 16:21

Apply suggestions from code review

7c8d799

Co-authored-by: Bastian Köcher <[email protected]>

Apply suggestions from code review

d4fa931

Co-authored-by: Bastian Köcher <[email protected]>

davxy merged commit 3c1d5b9 into master Jan 27, 2022

davxy deleted the davxy-stale-heads-notify branch January 27, 2022 17:22

andresilva mentioned this pull request Jan 28, 2022

beefy-gadget is not guaranteed to see finality notification for every block. paritytech/grandpa-bridge-gadget#254

Closed

andresilva mentioned this pull request Feb 23, 2022

Implement Lean BEEFY #10882

Merged

paulormart mentioned this pull request Mar 4, 2022

Fix skipped finalized blocks under EventSubscription paritytech/subxt#468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stale branches heads to finality notifications #10639

Add stale branches heads to finality notifications #10639

davxy commented Jan 11, 2022 •

edited

Loading

davxy commented Jan 17, 2022 •

edited

Loading

bkchr commented Jan 18, 2022

andresilva left a comment •

edited

Loading

andresilva left a comment

bkchr left a comment

bkchr commented Jan 26, 2022

Add stale branches heads to finality notifications #10639

Add stale branches heads to finality notifications #10639

Conversation

davxy commented Jan 11, 2022 • edited Loading

davxy commented Jan 17, 2022 • edited Loading

TL;DR

Substrate

Transaction Pool

Beefy

Networking

RPC

Cumulus

PovRecovery

RelaychainClient

Polkadot

Overseer

bkchr commented Jan 18, 2022

andresilva left a comment • edited Loading

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

bkchr commented Jan 26, 2022

davxy commented Jan 11, 2022 •

edited

Loading

davxy commented Jan 17, 2022 •

edited

Loading

andresilva left a comment •

edited

Loading