Block `ActiveLeavesUpdate` and `BlockFinalized` events in overseer until major sync is complete #6689

tdimitrov · 2023-02-08T13:31:35Z

Adds major sync oracle to overseer. The oracle is used to detect when the node has completed an initial full sync.

The PR also changes the behavior of ActiveLeaves and BlockFinalized events generation. Now they are not sent until the full initial sync is complete.

Partially addresses paritytech/polkadot-sdk#793. Fixes #6694.

node/overseer/src/lib.rs

Overseer won't generate any events to the subsystems until initial full sync is complete.

node/overseer/src/lib.rs

ordian · 2023-02-10T14:11:53Z

node/overseer/src/lib.rs

-							self.handle_external_request(request);
-						}
+						Event::MsgToSubsystem { msg, origin } => self.handle_msg_to_subsystem(msg, origin).await?,
+						Event::Stop => return self.handle_stop().await,


A concern: I think with current impl Ctrl-C before major sync is complete should not stall/deadlock subsystems, but would be nice to double check that.

How should this stall the subsystems?

node/overseer/src/lib.rs

tdimitrov · 2023-02-13T20:43:00Z

I've added some tests to cover the cases we care for + some corner ones. The final step is to refactor the loops so that code duplication is minimal 😟

bkchr · 2023-02-13T21:24:31Z

node/overseer/src/lib.rs

-							self.handle_external_request(request);
-						}
+						Event::MsgToSubsystem { msg, origin } => self.handle_msg_to_subsystem(msg, origin).await?,
+						Event::Stop => return self.handle_stop().await,


How should this stall the subsystems?

node/overseer/src/lib.rs

bkchr · 2023-02-13T21:33:11Z

node/overseer/src/lib.rs

+								// In theory we can receive `BlockImported` during initial major sync. In this case the
+								// update will be empty.
+								let span = match self.span_per_active_leaf.get(&block.hash) {
+									Some(span) => span.clone(),
+									None => {
+										// This should never happen.
+										gum::warn!(
+											target: LOG_TARGET,
+											?block.hash,
+											?block.number,
+											"Span for active leaf not found. This is not expected"
+										);
+										let span = Arc::new(jaeger::Span::new(block.hash, "leaf-activated"));
+										span
+									}
+								};
+								let update = ActiveLeavesUpdate::start_work(ActivatedLeaf {
+									hash: block.hash,
+									number: block.number,
+									status: LeafStatus::Fresh,
+									span,
+								});
+								self.broadcast_signal(OverseerSignal::ActiveLeaves(update)).await?;


I don't get why we need this branch? When we are receving a block import notification while doing major sync, we should not even land in the match block here.

If the finality has stalled and no BlockImported events are generated the subsystems in polkadot won't start because they initiate work on ActiveLeavesUpdate.

Right now this is solved by issuing an ActiveLeavesUpdate on startup with the leaves from DB. This is causing problems though because subsystems start work for old leaves, the state from them is usually pruned and some subsystems generate a bunch of logs on stratup.

Then we decided to send this initial ActiveLeavesUpdate after the major sync is done. But here I see two cases:

Finality is stalled or we don't get BlockImported events for some reason. -> In this case we won't issue ActiveLeavesUpdate and the subsystems will be 'inactive'.

Everything is fine and we get BlockImported at some point after the initial major sync. We don't need any special handling in this case.

So the branch you are referring to tries to solve1. We just got BlockFinalized, the initial major sync is complete and I'm trying to guess if an ActiveLeavesUpdate will be generated.

What I do:

Generate an artificial BlockImported event by calling self.block_imported(..). It is supposed to generate non-empty ActiveLeaves in 99% of the cases but what if it doesn't?

The fat else handles this 'what it doesn't' case with all the stuff that can go wrong.

I wrote tests to cover some of the cases and today I'm thinking about how to simplify this. I don't like it too :)

If the finality has stalled and no BlockImported events are generated the subsystems in polkadot won't start because they initiate work on ActiveLeavesUpdate.

I don't get the connection? When finality stalls, we will still import new blocks?

Finality is stalled or we don't get BlockImported events for some reason. -> In this case we won't issue ActiveLeavesUpdate and the subsystems will be 'inactive'.

This shouldn't happen.

I mean in general you could track when major sync is finished and then send the ActiveLeavesUpdate for all current leaves. However, I'm not really sure that we need this as there will always be some block import (yes with stalled finality it will be slower, but there will be blocks imported.

I don't get the connection? When finality stalls, we will still import new blocks?

For me 'finality has stalled' means 'no new blocks are generated'. I'm misunderstanding something here - can you explain why this is not true?

We are never not producing any new blocks. This is not going to happen, we may slow down block production but not more. There is block production and then there is finality. The block production is for building new blocks and put them on top of the chain. Finality is the process of saying that a certain chain up to a certain point can not be modified anymore, because between the last finalized block and the tip of the chain you can have forks, re-orgs and also go back to older blocks to produce on them. However, the moment we have finalized a block, we can not go before this block.

We are never not producing any new blocks.

I see... so this means after the initial sync we'll always have BlockImported events and all the precautions in my PR make no sense?

Maybe 👀

I mean it can happen that we don't produce any blocks, but then we have much bigger problems as Parachain subsystems not working properly 😬

Ok, what is even the point of acting on a leaf read from DB? Worst thing that can happen is that a single node does not work immediately on a leaf after a quick restart, which should be no problem at all. Hence, I think this can be simplified by simply not triggering ActiveLeavesUpdate on startup on a leaf read from db, but only for regular block import events.

Subsystems which do care about older blocks, do traverse the ancestry already.

It's far more elegant this way!

node/overseer/src/lib.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

…leaves`

paritytech-cicd-pr · 2023-02-14T19:02:27Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2392699

…d put enum instead

… is imported

eskimor

I am confused. Haven't we just agreed that all that is needed is removal of the initial artificial ActiveLeavesUpdate?

eskimor · 2023-02-15T15:47:37Z

node/overseer/src/lib.rs

@@ -746,12 +785,28 @@ where
 						Event::Stop => {
 							self.stop().await;
 							return Ok(());
-						}
+						},
+						Event::BlockImported(block) if self.sync_oracle.is_syncing() => {


Why do we still need the oracle? If I understand correctly, we don't get any BlockImported events anyway during sync.

tdimitrov · 2023-02-16T08:11:34Z

I'm closing this one in favor of #6727

Let's try the simpler solution first.

tdimitrov changed the title ~~[DNM] Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sunc is complete~~ [DNM] Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sync is complete Feb 9, 2023

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from 8b59b56 to 447698d Compare February 9, 2023 08:51

bkchr reviewed Feb 9, 2023

View reviewed changes

node/overseer/src/lib.rs Outdated Show resolved Hide resolved

bkchr mentioned this pull request Feb 9, 2023

overseer: signal completion of major sync #6694

Closed

Add sync oracle to overseer

ae29bb4

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from 447698d to 2244cff Compare February 9, 2023 14:57

Don't send ActiveLeavesUpdate and BlockFinalized until full sync

70d031d

Overseer won't generate any events to the subsystems until initial full sync is complete.

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from 2244cff to 70d031d Compare February 10, 2023 11:47

Comments and indentation

349c89f

tdimitrov changed the title ~~[DNM] Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sync is complete~~ Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sync is complete Feb 10, 2023

Remove unnecessary clone()

5bae570

ordian reviewed Feb 10, 2023

View reviewed changes

tdimitrov added 5 commits February 10, 2023 17:57

Fix BlockFinalized handling in main loop

fe09d12

Pass SyncOracle as a parameter to dummy_overseer_builder

4c4a4db

Some tests

cfcd098

Fix initial sync loop

27b0cf5

More tests

2204966

tdimitrov mentioned this pull request Feb 13, 2023

Prevent dispute-coordinator from doing any work before the initial node sync is complete. #6682

Closed

bkchr reviewed Feb 13, 2023

View reviewed changes

tdimitrov and others added 4 commits February 14, 2023 15:17

Update node/overseer/src/lib.rs

2ba4a97

Co-authored-by: Bastian Köcher <git@kchr.de>

Extract initial ActiveLeaves update logic in `prepare_initial_active_…

969f71a

…leaves`

Update tests

81a64e5

Better comments in tests

6bdb390

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from e680a77 to 6bdb390 Compare February 14, 2023 17:41

Remove unneeded async

f8467d8

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from 6ead261 to f8467d8 Compare February 14, 2023 18:48

tdimitrov added 5 commits February 14, 2023 22:10

Fix dummy_overseer_builder usage

a3840a7

Remove Option<Box<dyn SyncOracle + Send>> from MajorSyncOracle an…

ade3d9d

…d put enum instead

Remove code duplication

ba1167b

Don't send ActiveLeaves on stratup; do it when the first fresh leaf…

809a612

… is imported

Undo some method extractions

2e14f46

tdimitrov force-pushed the tsv-block-overseer-events-during-sync branch from a813802 to 2e14f46 Compare February 15, 2023 14:41

tdimitrov marked this pull request as ready for review February 15, 2023 14:41

tdimitrov requested review from bkchr and eskimor February 15, 2023 14:42

tdimitrov added the B0-silent Changes should not be mentioned in any release notes label Feb 15, 2023

eskimor reviewed Feb 15, 2023

View reviewed changes

tdimitrov mentioned this pull request Feb 16, 2023

Don't send ActiveLeaves from leaves in db on startup in Overseer #6727

Merged

tdimitrov closed this Feb 16, 2023

tdimitrov deleted the tsv-block-overseer-events-during-sync branch February 16, 2023 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block `ActiveLeavesUpdate` and `BlockFinalized` events in overseer until major sync is complete #6689

Block `ActiveLeavesUpdate` and `BlockFinalized` events in overseer until major sync is complete #6689

tdimitrov commented Feb 8, 2023 •

edited

Loading

ordian Feb 10, 2023

bkchr Feb 13, 2023

tdimitrov commented Feb 13, 2023

bkchr Feb 13, 2023

bkchr Feb 13, 2023

tdimitrov Feb 14, 2023

bkchr Feb 14, 2023

tdimitrov Feb 14, 2023

bkchr Feb 14, 2023

tdimitrov Feb 14, 2023

bkchr Feb 14, 2023

eskimor Feb 15, 2023

tdimitrov Feb 15, 2023

paritytech-cicd-pr commented Feb 14, 2023

eskimor left a comment

eskimor Feb 15, 2023

tdimitrov commented Feb 16, 2023

Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sync is complete #6689

Block ActiveLeavesUpdate and BlockFinalized events in overseer until major sync is complete #6689

Conversation

tdimitrov commented Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdimitrov commented Feb 13, 2023

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

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

paritytech-cicd-pr commented Feb 14, 2023

eskimor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdimitrov commented Feb 16, 2023

Block `ActiveLeavesUpdate` and `BlockFinalized` events in overseer until major sync is complete #6689

Block `ActiveLeavesUpdate` and `BlockFinalized` events in overseer until major sync is complete #6689

tdimitrov commented Feb 8, 2023 •

edited

Loading