backing: move the min votes threshold to the runtime #7577

alindima · 2023-08-03T08:56:15Z

the value is stored in the HostConfiguration
the client will now retrieve this value via a new runtime call (currently staging) and cache it per-session

TODO: test with versi

eskimor · 2023-08-03T09:26:02Z

Yes this should only ever change on session boundaries, but we should not put it into SessionInfo directly because of this. It should be session buffered as SessionInfo right now though, just not packed together in a single struct.

sandreim · 2023-08-08T15:25:35Z

node/subsystem-util/src/runtime/mod.rs

@@ -155,6 +162,34 @@ impl RuntimeInfo {
 		self.get_session_info_by_index(sender, relay_parent, session_index).await
 	}

+	/// Get minimum_backing_votes by relay parent hash.


Technically this is per session, but you use the relay parent hash to query the state at.

The cache is indexed by the session index but if the value is not present in the cache it falls back to requesting it from the runtime (which always needs the relay parent, right?).

Maybe the doc comment is confusing, is that it?

Something like Get the minimum backing votes for the specified session. would be a better description.

The relay parent is needed in case there is a cache miss, because you need to fetch the state of some relay parent. Comment is indeed misleading, as we are fetching by session index, we just use the relay parent to access the state.

sandreim · 2023-08-08T15:28:51Z

statement-table/src/generic.rs

-	/// requisite number of votes for validity from a group.
-	fn requisite_votes(&self, group: &Self::GroupId) -> usize;
+	/// Get a validator group size.
+	fn get_group_size(&self, group: &Self::GroupId) -> Option<usize>;


Why does it need to return Option ?

I don't think it should ever return None under the current usage model but it could if there's a bug.

Still, the method needs to return Option because it queries a Hashmap and we can't know at compile-time whether or not the validator group is present for all the paraIDs.

node/core/backing/src/lib.rs

primitives/src/runtime_api.rs

also cache it per-session in the backing subsystem Signed-off-by: alindima <[email protected]>

alindima · 2023-08-21T11:16:13Z

I had to force-push because of the numerous conflicts after merging the async backing PR, sorry about that.
How are such situations handled normally? (conflicts with the base branch)

paritytech-cicd-pr · 2023-08-21T11:19:35Z

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/3419482

rphmeier · 2023-08-22T01:21:16Z

Yes this should only ever change on session boundaries, but we should not put it into SessionInfo directly because of paritytech/polkadot-sdk#734.

Implementing session buffering for many storage items is probably painful. As suggested in the issue I still believe the best way forward is to have a struct SessionInfo used by the runtime but not exposed elsewhere.

…cking-votes-to-runtime

also enable it for rococo/versi for testing

this dependency has been recently introduced by async backing

this is not needed, as the RuntimeAPISubsystem already takes care of versioning and will return NotSupported if the version is not right.

Companion for paritytech/polkadot#7577 Signed-off-by: alindima <[email protected]>

eskimor

Looks good overall!

eskimor · 2023-08-24T15:11:59Z

node/subsystem-util/src/runtime/mod.rs

@@ -155,6 +162,34 @@ impl RuntimeInfo {
 		self.get_session_info_by_index(sender, relay_parent, session_index).await
 	}

+	/// Get minimum_backing_votes by relay parent hash.


The relay parent is needed in case there is a cache miss, because you need to fetch the state of some relay parent. Comment is indeed misleading, as we are fetching by session index, we just use the relay parent to access the state.

eskimor · 2023-08-24T15:21:53Z

runtime/parachains/src/configuration/migration/v9.rs

+on_demand_fee_variability                : Perbill::from_percent(3),
+on_demand_target_queue_utilization       : Perbill::from_percent(25),
+on_demand_ttl                            : 5u32.into(),
+minimum_backing_votes                    : 2


Wasn't there a constant we can use?

eskimor · 2023-08-24T15:29:46Z

runtime/parachains/src/inclusion/tests.rs

@@ -85,6 +85,10 @@ fn default_allowed_relay_parent_tracker() -> AllowedRelayParentsTracker<Hash, Bl
 	allowed
 }

+fn minimum_backing_votes(group_len: usize) -> usize {


I think we should expose a variant of this function in primitives and use it both on the node side and the runtime side.

eskimor · 2023-08-24T15:50:26Z

node/core/backing/src/lib.rs

@@ -983,10 +996,18 @@ async fn construct_per_relay_parent_state<Context>(

 	let parent = relay_parent;

-	let (validators, groups, session_index, cores) = futures::try_join!(
+	let session_index =
+		try_runtime_api!(runtime_info.get_session_index_for_child(ctx.sender(), parent).await);


Ok, this wasn't introduced by you, but this macro completely defeats the point of our error handling. If we don't want to bring down the node, the correct way of handling this would be to make all runtime errors non fatal, instead of swallowing them in a macro.

The only fatal error we have here is canceled which never happens, except the node is shutting down, so this is fine.

eskimor · 2023-08-24T15:55:40Z

node/core/backing/src/lib.rs

+		try_runtime_api!(runtime_info.get_session_index_for_child(ctx.sender(), parent).await);
+
+	let minimum_backing_votes =
+		request_min_backing_votes(parent, ctx.sender(), |parent, sender| {


Why do we need this wrapper? Can't get_min_backing_votes not already fall back to the constant in case of unsupported?

eskimor · 2023-08-24T16:00:01Z

node/core/runtime-api/src/cache.rs

+		relay_parent: Hash,
+		minimum_backing_votes: u32,
+	) {
+		self.minimum_backing_votes.put(relay_parent, minimum_backing_votes);


No. This should be cached per SessionIndex not per relay parent as it is only supposed to change at session boundaries.

eskimor · 2023-08-24T16:05:58Z

node/network/statement-distribution/src/vstaging/mod.rs

-			state
-				.per_session
-				.insert(session_index, PerSessionState::new(session_info, &state.keystore));
+			let minimum_backing_votes = request_min_backing_votes(


That's why you have this wrapper, because we don't use RuntimeInfo everywhere. One point of RuntimeInfo was developer ergonomics. Therefore it feels kind of bad to make the calls to runtime_info more clunky. I would do the legacy check right in the get function and provide a standalone utility function for subsystems not using RuntimeInfo.

Conceptually we should be using RuntimeInfo everywhere or not at all. So full cleanup would be to port subsystems to the same, but that's a different story not needed to be tackled now. With the SessionInfo split up we might re-consider a few things anyway.

eskimor · 2023-08-24T16:07:01Z

node/subsystem-types/src/messages.rs

@@ -606,6 +606,8 @@ pub enum RuntimeApiRequest {
 	Authorities(RuntimeApiSender<Vec<AuthorityDiscoveryId>>),
 	/// Get the current validator set.
 	Validators(RuntimeApiSender<Vec<ValidatorId>>),
+	/// Get the minimum required backing votes.
+	MinimumBackingVotes(RuntimeApiSender<u32>),


This should contain the SessionIndex.

sandreim

Thanks @alindima , it looks good to me so far.

Also, before any test on Versi you should do a first check with Zombienet.

sandreim · 2023-08-24T13:29:09Z

runtime/parachains/src/configuration/migration/v9.rs

+			StorageVersion::new(9).put::<Pallet<T>>();
+
+			weight_consumed
+		} else {


nit: We could be more descriptive in the warning message, and also account for the case when someone is accidentally skipping a migration and current storage version is 7 when upgrading from an older runtime.

sandreim · 2023-08-24T13:38:14Z

node/subsystem-util/src/runtime/mod.rs

+pub async fn request_min_backing_votes<'a, S, Func, Fut>(
+	parent: Hash,
+	sender: &'a mut S,
+	get_min_backing_votes: Func,


Why not just move the code in the function instead of having to pass the closure ?

sandreim · 2023-08-24T13:41:59Z

node/subsystem-util/src/runtime/mod.rs

@@ -155,6 +162,34 @@ impl RuntimeInfo {
 		self.get_session_info_by_index(sender, relay_parent, session_index).await
 	}

+	/// Get minimum_backing_votes by relay parent hash.


Something like Get the minimum backing votes for the specified session. would be a better description.

sandreim · 2023-08-24T13:55:03Z

statement-table/src/generic.rs

-					total_validity += 1
-				}
+		fn get_group_size(&self, group: &Self::GroupId) -> Option<usize> {
+			let count = self.authorities.values().filter(|g| *g == group).count();


IMO it doesn't make sense to require the backing group size without having a candidate backed first by a group of at least 1 validator.

Can count ever be 0 in any valid scenario ? I think it could only happen if there are more backing groups than number of validators, but that implies some bug that allows candidates to be backed outside the context of our scheduling.

So, we could just return 0 and you wouldn't need the Option.

runtime/parachains/src/runtime_api_impl/vstaging.rs

alindima marked this pull request as draft August 3, 2023 08:56

paritytech-ci requested review from a team August 3, 2023 08:56

alindima requested a review from sandreim August 3, 2023 08:57

sandreim reviewed Aug 8, 2023

View reviewed changes

move min backing votes const to runtime

5feadf4

also cache it per-session in the backing subsystem Signed-off-by: alindima <[email protected]>

alindima force-pushed the alindima-add-min-backing-votes-to-runtime branch from 424f3a4 to 5feadf4 Compare August 21, 2023 11:14

add runtime migration

eb864e0

alindima added 4 commits August 22, 2023 13:33

Merge remote-tracking branch 'origin/master' into alindima-add-min-ba…

8c9a8d8

…cking-votes-to-runtime

introduce api versioning for min_backing votes

09c9f4e

also enable it for rococo/versi for testing

also add min_backing_votes runtime calls to statement-distribution

fe51ef8

this dependency has been recently introduced by async backing

remove explicit version runtime API call

c846d6d

this is not needed, as the RuntimeAPISubsystem already takes care of versioning and will return NotSupported if the version is not right.

alindima added a commit to paritytech/cumulus that referenced this pull request Aug 23, 2023

add minimum_backing_votes staging runtime api

5f1c9ae

Companion for paritytech/polkadot#7577 Signed-off-by: alindima <[email protected]>

alindima mentioned this pull request Aug 23, 2023

[DNM] Companion: add minimum_backing_votes staging runtime api paritytech/cumulus#3050

Open

alindima 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 Aug 24, 2023

eskimor reviewed Aug 24, 2023

View reviewed changes

sandreim reviewed Aug 24, 2023

View reviewed changes

alindima mentioned this pull request Aug 28, 2023

backing: move the min votes threshold to the runtime paritytech/polkadot-sdk#1200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backing: move the min votes threshold to the runtime #7577

backing: move the min votes threshold to the runtime #7577

alindima commented Aug 3, 2023 •

edited

Loading

eskimor commented Aug 3, 2023

sandreim Aug 8, 2023

alindima Aug 21, 2023

sandreim Aug 24, 2023

eskimor Aug 24, 2023

sandreim Aug 8, 2023

alindima Aug 21, 2023

alindima commented Aug 21, 2023

paritytech-cicd-pr commented Aug 21, 2023

rphmeier commented Aug 22, 2023

eskimor left a comment

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

eskimor Aug 24, 2023

sandreim left a comment

sandreim Aug 24, 2023

sandreim Aug 24, 2023

sandreim Aug 24, 2023

sandreim Aug 24, 2023

backing: move the min votes threshold to the runtime #7577

Are you sure you want to change the base?

backing: move the min votes threshold to the runtime #7577

Conversation

alindima commented Aug 3, 2023 • edited Loading

eskimor commented Aug 3, 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

alindima commented Aug 21, 2023

paritytech-cicd-pr commented Aug 21, 2023

rphmeier commented Aug 22, 2023

eskimor left a 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

Choose a reason for hiding this comment

sandreim left a 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

alindima commented Aug 3, 2023 •

edited

Loading