Handling of disabled validators in backing subsystem #1259

tdimitrov · 2023-08-29T12:35:12Z

Handles validator disabling in the backing subsystem. The PR introduces two changes:

Don't import statements from disabled validators.
Don't validate and second if the local validator is disabled.

The purpose of this change is first to ignore statements from disabled validators on the node side. This is an optimisation as these statements are supposed to be cleared in the runtime too (still not implemented at the moment). Additionally if the local node is a validator which is disabled - don't process any new candidates.

burdges · 2023-09-06T19:33:22Z

Also discussed in #635 (comment) where I'm not in favor of disabling approval checkers, and definitely not grandpa voters, but yes it's harmless to disable backers, which makes it good to disable backers.

ordian · 2023-09-11T08:42:49Z

Looks sane, we would ideally also ignore dispute statements, or at least dispute initiations.

ordian · 2023-09-11T08:45:31Z

What happens if we revert the block where a validator was disabled to a block where he isn't?

polkadot/node/core/backing/src/lib.rs

sandreim · 2023-09-11T09:49:23Z

polkadot/node/core/backing/src/lib.rs

+			.any(|disabled_val_idx| *disabled_val_idx == *validator_idx)
+	}
+
+	pub fn local_validator_is_disabled(&self) -> Option<bool> {


Why do we need to return Option ?

self.validator is an Option. Afair the code can be executed in cases where the local node is not a validator?

I decided to return an Option here too and handle it at the caller.

sandreim · 2023-09-11T09:53:39Z

We should also look into adding a new zombienet test.

tdimitrov · 2023-09-11T09:54:11Z

Looks sane, we would ideally also ignore dispute statements, or at least dispute initiations.

Yes, but this is part of dispute-coordinator/dispute-distribution right? Or there is something in backing which I am missing?

I'm splitting the work in smaller PRs so that they are easier for review. All the work goes in https://github.com/paritytech/polkadot-sdk/tree/tsv-disabling-node-side

What happens if we revert the block where a validator was disabled to a block where he isn't?

Good question. The first thing that comes to my mind is we should count its vote, but I haven't thought about this in detail yet.

polkadot/node/core/backing/src/lib.rs

rphmeier · 2023-09-11T17:23:03Z

It would be better to handle this on the statement-distribution layer instead, i.e. since there are no changes there in this PR we will currently both accept and distribute statements from disabled validators, even though the backing system does not import them.

What I would suggest is this:

Backing: do not sign, import, or distribute local statements if we are disabled (or kick off validation work at all)
statement-dist: do not accept, request, or distribute statements from disabled validators

A modification to the statement-dist StatementStore would do most of the heavy-lifting here - three parts:

StatementStore: generate request bitmasks which exclude disabled validators
StatementStore reject statements related to disabled validators
Manifest handling: do not count disabled validators towards the seconded & backing thresholds in the advertised bitmask. Likewise in request handling: skip disabled validators' statements in the response

polkadot/node/core/backing/src/tests/mod.rs

Co-authored-by: ordian <write@reusable.software>

polkadot/node/core/backing/src/lib.rs

tdimitrov · 2023-10-19T07:44:33Z

polkadot/node/core/backing/src/lib.rs

+	// TODO: https://github.com/paritytech/polkadot-sdk/issues/1940
+	// Once runtime ver `DISABLED_VALIDATORS_RUNTIME_REQUIREMENT` is released remove this `if`
+	// statement, add `request_from_runtime` call to the `try_join!` call above and use
+	// `try_runtime_api!` to get `disabled_validators`
+	let disabled_validators = if has_required_runtime(
+		ctx.sender(),
+		parent,
+		RuntimeApiRequest::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT,
+	)
+	.await
+	{
+		let disabled_validators = request_from_runtime(parent, ctx.sender(), |tx| {
+			RuntimeApiRequest::DisabledValidators(tx)
+		})
+		.await
+		.await
+		.map_err(Error::JoinMultiple)?;
+
+		let disabled_validators = try_runtime_api!(disabled_validators);
+		disabled_validators
+	} else {
+		gum::debug!(target: LOG_TARGET, "Runtime doesn't support `DisabledValidators` - continuing with an empty disabled validators set");
+		vec![]
+	};
+


This code needs to stay until we release v8: #1940

can we extract that into subsystem-util or somewhere where statement-distribution and dispute-coordinator can reuse it?

Yes, I was thinking about the same and I started working on it but against master.
My idea is to have a wrapper around request_from_runtime which does a version check and optionally returns a fallback version. I'll open a PR against master because I think this will be useful in general. We can backport it here easily after that.

I went into a macro rabbit hole. I'll just extract this implementation here and finish the other version in a more appropriate time.

Sure, sounds good. However, I didn't mean to use a macro for that, just a regular function should be OK.

polkadot/node/subsystem-util/src/lib.rs

polkadot/node/core/backing/src/lib.rs

…so that it can be reused in other subsystems

…stem-util too

* tsv-disabling: (39 commits) Handling of disabled validators in backing subsystem (#1259) Switch trie cache random seed (#1935) Expose prometheus metrics for minimal-relay-chain node in collators (#1942) Do not force collators to update after enabling async backing (#1920) Pin PRDoc image to v0.0.5 until we are ready for v0.0.6 (#1947) [prdoc] Start BEEFY gadget by default for Polkadot nodes (#1945) Update bridges subtree (#1944) bump zombienet version (#1931) [FRAME] Message Queue use proper overweight limit (#1873) Cumulus: Allow aura to use initialized collation request receiver (#1911) Use prebuilt try-runtime binary in CI (#1898) Update kusama/polkadot bootnodes (#1895) Introduce XcmFeesToAccount fee manager (#1234) upgraded review bot to v2.1.0 (#1908) Trading trait and deal with metadata in Mutate trait for nonfungibles_v2 (#1561) Add Runtime Missing Crate Descriptions (#1909) Switch to the release env (#1910) Bump paritytech/review-bot from 2.0.1 to 2.1.0 (#1924) Bump actions/checkout from 4.1.0 to 4.1.1 (#1925) Start BEEFY client by default for Polkadot nodes (#1913) ...

Handles validator disabling in the backing subsystem. The PR introduces two changes: 1. Don't import statements from disabled validators. 2. Don't validate and second if the local validator is disabled. The purpose of this change is first to ignore statements from disabled validators on the node side. This is an optimisation as these statements are supposed to be cleared in the runtime too (still not implemented at the moment). Additionally if the local node is a validator which is disabled - don't process any new candidates. --------- Co-authored-by: ordian <noreply@reusable.software> Co-authored-by: ordian <write@reusable.software>

…tem (#1259) (#2764) #1259 was merged into a feature branch, but we've decided to merge node-side changes for disabling straight into master. This is a dependency of #1841 and #2637. --------- Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>

tdimitrov force-pushed the tsv-disabling-backing branch from b5c6f07 to f52256e Compare August 31, 2023 12:47

tdimitrov added 2 commits September 4, 2023 14:14

Add disabled_validators in staging runtime api

800d8e5

Cumulus changes

936216d

tdimitrov force-pushed the tsv-disabling-node-side branch from 3ee35c8 to 936216d Compare September 4, 2023 11:14

tdimitrov added 2 commits September 4, 2023 14:15

Handling of disabled validators in backing subsystem

7165ef9

Tests

24b5df5

tdimitrov force-pushed the tsv-disabling-backing branch from f52256e to 24b5df5 Compare September 4, 2023 11:16

tdimitrov added 8 commits September 4, 2023 15:45

Merge branch 'master' into tsv-disabling-node-side

eaca5ed

Fix compilation errors

e4dee17

Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing

23023c4

spelling

8db76f5

clippy

e2846b2

fmt

8604b69

clippy

78afa76

Fix tests

9e39e58

tdimitrov added 2 commits September 11, 2023 11:31

Merge branch 'master' into tsv-disabling-node-side

0d0129c

Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing

23fbb41

sandreim reviewed Sep 11, 2023

View reviewed changes

Demote log level of a trace

91d1a8d

rphmeier reviewed Sep 11, 2023

View reviewed changes

polkadot/node/core/backing/src/lib.rs Show resolved Hide resolved

rphmeier reviewed Sep 11, 2023

View reviewed changes

polkadot/node/core/backing/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 11, 2023

View reviewed changes

polkadot/node/core/backing/src/tests/mod.rs Outdated Show resolved Hide resolved

tdimitrov added 2 commits September 13, 2023 14:04

Code review feedback

3d7efab

tests

e68fa6a

Merge branch 'tsv-disabling' into tsv-disabling-backing

1e561e9

tdimitrov changed the base branch from master to tsv-disabling October 16, 2023 11:33

Update polkadot/node/core/backing/src/lib.rs

cf671f4

Co-authored-by: ordian <write@reusable.software>

tdimitrov marked this pull request as ready for review October 17, 2023 09:09

tdimitrov added 3 commits October 17, 2023 22:19

Don't kikoff validation work if we are not a validator

8807981

Merge branch 'tsv-disabling' into tsv-disabling-backing

55f8e01

Merge branch 'tsv-disabling' into tsv-disabling-backing

b5609e1

tdimitrov commented Oct 18, 2023

View reviewed changes

polkadot/node/core/backing/src/lib.rs Outdated Show resolved Hide resolved

tdimitrov added 2 commits October 19, 2023 10:34

Extract has_required_runtime from provisioner to subsystem-util

617e477

Handle old runtimes

d7f4315

tdimitrov commented Oct 19, 2023

View reviewed changes

polkadot/node/subsystem-util/src/lib.rs Outdated Show resolved Hide resolved

Fix tests

718f04b

tdimitrov requested a review from ordian October 19, 2023 10:11

ordian reviewed Oct 19, 2023

View reviewed changes

polkadot/node/core/backing/src/lib.rs Outdated Show resolved Hide resolved

tdimitrov added 7 commits October 19, 2023 22:50

Use the specialized runtime request function for disabled validators

275ce02

Fix log level

f2b17f1

Merge branch 'tsv-disabling' into tsv-disabling-backing

7872f8f

Extract get_disabled_validators_with_fallback in a separate module …

3c89059

…so that it can be reused in other subsystems

Small style fix

15b5069

Use get_disabled_validators_with_fallback in Validator from subsy…

96c1358

…stem-util too

Fix comments

1a6648a

tdimitrov merged commit 913232a into tsv-disabling Oct 20, 2023
109 checks passed

tdimitrov deleted the tsv-disabling-backing branch October 20, 2023 13:13

tdimitrov mentioned this pull request Oct 20, 2023

Disabling - feature PR #1955

Closed

4 tasks

ordian mentioned this pull request Dec 20, 2023

backport to master: Handling of disabled validators in backing subsystem (#1259) #2764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling of disabled validators in backing subsystem #1259

Handling of disabled validators in backing subsystem #1259

tdimitrov commented Aug 29, 2023 •

edited

Loading

burdges commented Sep 6, 2023 •

edited

Loading

ordian commented Sep 11, 2023

ordian commented Sep 11, 2023

sandreim Sep 11, 2023

tdimitrov Sep 11, 2023

sandreim commented Sep 11, 2023

tdimitrov commented Sep 11, 2023

rphmeier commented Sep 11, 2023 •

edited

Loading

tdimitrov Oct 19, 2023

ordian Oct 19, 2023

tdimitrov Oct 19, 2023

tdimitrov Oct 20, 2023

ordian Oct 20, 2023

Handling of disabled validators in backing subsystem #1259

Handling of disabled validators in backing subsystem #1259

Conversation

tdimitrov commented Aug 29, 2023 • edited Loading

burdges commented Sep 6, 2023 • edited Loading

ordian commented Sep 11, 2023

ordian commented Sep 11, 2023

sandreim Sep 11, 2023

Choose a reason for hiding this comment

tdimitrov Sep 11, 2023

Choose a reason for hiding this comment

sandreim commented Sep 11, 2023

tdimitrov commented Sep 11, 2023

rphmeier commented Sep 11, 2023 • edited Loading

tdimitrov Oct 19, 2023

Choose a reason for hiding this comment

ordian Oct 19, 2023

Choose a reason for hiding this comment

tdimitrov Oct 19, 2023

Choose a reason for hiding this comment

tdimitrov Oct 20, 2023

Choose a reason for hiding this comment

ordian Oct 20, 2023

Choose a reason for hiding this comment

tdimitrov commented Aug 29, 2023 •

edited

Loading

burdges commented Sep 6, 2023 •

edited

Loading

rphmeier commented Sep 11, 2023 •

edited

Loading