Refactor runtime `fn enter(...)` to select disputes, bitfields and backing candidates to protect against over length block issues by Lldenaurois · Pull Request #4020 · paritytech/polkadot

Lldenaurois · 2021-10-05T17:16:02Z

TBD

rphmeier · 2021-10-11T22:04:34Z

runtime/parachains/src/disputes.rs

 		included_in: BlockNumber,
 	);

+	fn get_included(session: SessionIndex, candidate_hash: CandidateHash) -> Option<BlockNumber>;


Note #4054 does the same, and it's an antipattern in Rust to call a getter get_.

missing docs

rphmeier · 2021-10-11T22:05:40Z

runtime/parachains/src/paras_inherent.rs

-			MINIMAL_INCLUSION_INHERENT_WEIGHT + data.backed_candidates.len() as Weight * BACKED_CANDIDATE_WEIGHT,
+			MINIMAL_INCLUSION_INHERENT_WEIGHT +
+				count_dispute_statements(&data.disputes) as Weight * DISPUTE_PER_STATEMENT_WEIGHT +
+				count_bitfield_bits(&data.bitfields) as Weight * BITFIELD_WEIGHT +


it really should be based on the number of bitfields, so the amount of signatures checked. That should be the dominant factor

rphmeier · 2021-10-11T22:06:13Z

runtime/parachains/src/paras_inherent.rs

+			MINIMAL_INCLUSION_INHERENT_WEIGHT +
+				count_dispute_statements(&data.disputes) as Weight * DISPUTE_PER_STATEMENT_WEIGHT +
+				count_bitfield_bits(&data.bitfields) as Weight * BITFIELD_WEIGHT +
+				data.backed_candidates.len() as Weight * BACKED_CANDIDATE_WEIGHT,


I could see this being based on the number of signatures checked as well plus a constant for any candidates that do code upgrades.

rphmeier · 2021-10-11T22:06:34Z

runtime/parachains/src/paras_inherent.rs

 				Error::<T>::InvalidParentHeader,
 			);

+			limit_paras_inherent::<T>(&mut disputes, &mut signed_bitfields, &mut backed_candidates);


limiting shouldn't be done within enter, but instead outside of it.

@drahnr - we had further discussions that imply both should do limiting, but the on-chain limiting should be a no-op

No-op as in, filter(filter(x)) should be equal to filter(x) so if on-chain filtering catches anything it was probably not produced by the usual block authoring code or there's a bug.

rphmeier · 2021-10-11T22:06:49Z

runtime/parachains/src/paras_inherent.rs

+			limit_paras_inherent::<T>(&mut disputes, &mut signed_bitfields, &mut backed_candidates);
+			let num_dispute_statements = count_dispute_statements(&disputes) as Weight;
+			let num_bitfield_bits = count_bitfield_bits(&signed_bitfields) as Weight;
+			let backed_candidates_len = backed_candidates.len() as Weight;


Isn't this already done above?

rphmeier · 2021-10-11T22:07:51Z

runtime/parachains/src/paras_inherent.rs

+			let b_local_block = T::DisputesHandler::get_included(b.session, b.candidate_hash);
+			match (a_local_block, b_local_block) {
+				// Prioritize local disputes over remote disputes.
+				(None, Some(_)) => Ordering::Greater,


Doesn't this do the opposite? This says a > b if a not local and b local

rphmeier · 2021-10-11T22:08:44Z

runtime/parachains/src/paras_inherent.rs

+				// Prioritize local disputes over remote disputes.
+				(None, Some(_)) => Ordering::Greater,
+				(Some(_), None) => Ordering::Less,
+				// For local disputes, prioritize those with votes for invalidity.


Disputes have to have 1 vote on either side. I don't get it.

rphmeier · 2021-10-11T22:09:49Z

runtime/parachains/src/paras_inherent.rs

+		return
+	}
+
+	if disputes_weight + bitfields_weight > max_non_inclusion_weight {


We should recalculate disputes weight after filtering.

rphmeier · 2021-10-11T22:11:09Z

runtime/parachains/src/paras_inherent.rs

+
+	if disputes_weight + bitfields_weight > max_non_inclusion_weight {
+		let mut total_weight = disputes_weight;
+		bitfields.retain(|b| {


If we assume that the bitfield weight is constant, this whole block could be replaced by a simple division to check how many bitfields we need to cut off.

rphmeier · 2021-10-11T22:11:37Z

runtime/parachains/src/paras_inherent.rs

+		return
+	}
+
+	if total_non_inclusion_weight > max_non_inclusion_weight {


again, new bitfields weight needs to be recalculated.

rphmeier · 2021-10-11T22:12:53Z

runtime/parachains/src/inclusion/benchmarking.rs

+			&signing_context,
+		));
+
+	}: { Pallet::<T>::process_bitfields(expected_bits(), vec![signed.into()], core_lookup).unwrap() }


This isn't going to be very useful unless we actually set some occupied cores i.e. CandidatePendingAvailability in the inclusion pallet.

rphmeier · 2021-10-11T22:21:58Z

This looks very incomplete and not at the state I'd expect after this time. Still, the direction looks ~ok, although it is extremely minimal. It might help to map out in english, not in code, what you intend for this PR to do.

It's also worth noting that filtering things based on weight will affect the validity of other things that are included. i.e. disputes affect occupied cores, which affects bitfields, which affects backing. This ties in heavily with #3989 and #4028.

eskimor

Quick first review. Another more in depth one to come.

eskimor · 2021-10-13T11:55:34Z

runtime/parachains/src/paras_inherent.rs

+			let backed_candidates_weight =
+				count_backed_candidate_signatures::<T>(&backed_candidates) as Weight *
+					BACKED_CANDIDATE_WEIGHT +
+					count_code_upgrades::<T>(&backed_candidates) as Weight * CODE_UPGRADE_WEIGHT;


CODE_UPGRADE_WEIGHT can hardly be a constant, as it will depend on the size of the runtime.

Also I am missing taking into account messages.

I believe that the messages are accounted for by ensuring we only consider available weight, i.e. other messages in the block are already deducted from the max_block weight.

eskimor · 2021-10-13T12:00:12Z

runtime/parachains/src/paras_inherent.rs

+			.saturating_sub(num_code_upgrades as u64 * CODE_UPGRADE_WEIGHT);
+		let num_candidates_to_retain = min(
+			num_backed_candidate_signatures as u64,
+			block_weight_available / BACKED_CANDIDATE_WEIGHT,


Not sure how this can work with a constant, with runtime upgrades + messages.

I have modified the calculation shortly after you performed the review.

For Code upgrades, I was unaware that this would depend on the size of the runtime, but that makes perfect sense. I assume that will mean that we must unpack the Option on the candidate and determine the weight via NUM_BYTES * PER_BYTE_WEIGHT

rphmeier · 2021-10-16T15:14:45Z

runtime/parachains/src/paras_inherent.rs

 //! as it has no initialization logic and its finalization logic depends only on the details of
 //! this module.

+use no_std_compat::cmp::Ordering;


usually it's imported as sp_std::cmp

rphmeier · 2021-10-16T15:15:58Z

runtime/parachains/src/paras_inherent.rs

+		// Sort the dispute statements according to the following prioritization:
+		//  1. Prioritize local disputes over remote disputes.
+		//  2. Prioritize older disputes over newer disputes.
+		//  3. Prioritize local disputes withan invalid vote over valid votes.


Is this comment outdated?

rphmeier · 2021-10-16T15:17:17Z

runtime/parachains/src/paras_inherent.rs

+			match (a_local_block, b_local_block) {
+				// Prioritize local disputes over remote disputes.
+				(None, Some(_)) => Ordering::Less,
+				(Some(_), None) => Ordering::Greater,


Isn't this and the above backwards? local dispute > remote dispute so sort ascending puts remote before local.

I was quite certain it was correct before but then you added a comment saying it's backward, so I changed it.

Going to write a test to confirm which of the two is desired.

drahnr · 2021-10-18T12:24:48Z

runtime/parachains/src/paras_inherent.rs

+			seed[31] = (index % (1 << 8)) as u8;
+			seed[30] = ((index >> 8) % (1 << 8)) as u8;
+			seed[29] = ((index >> 16) % (1 << 8)) as u8;
+			seed[29] = ((index >> 24) % (1 << 8)) as u8;


Suggested change

seed[31] = (index % (1 << 8)) as u8;

seed[30] = ((index >> 8) % (1 << 8)) as u8;

seed[29] = ((index >> 16) % (1 << 8)) as u8;

seed[29] = ((index >> 24) % (1 << 8)) as u8;

seed[31] = (index & 0xFF) as u8;

seed[30] = ((index >> 8) & 0xFF) as u8;

seed[29] = ((index >> 16) & 0xFF) as u8;

seed[29] = ((index >> 24) & 0xFF) as u8;

rphmeier · 2021-10-18T14:35:47Z

runtime/parachains/src/paras_inherent.rs

+	candidate.validity_votes.len() as Weight * BACKED_CANDIDATE_WEIGHT +
+		match &candidate.candidate.commitments.new_validation_code {
+			Some(v) => v.0.len() as u64 * CODE_UPGRADE_WEIGHT_VARIABLE + CODE_UPGRADE_WEIGHT_FIXED,
+			_ => 0 as Weight,


Suggested change

_ => 0 as Weight,

None => 0 as Weight,

First attempt at a skeleton diff

77a5b7f

Lldenaurois force-pushed the limit_paras_inherent branch from a2d157c to 77a5b7f Compare October 5, 2021 17:45

Lldenaurois changed the title ~~First attempt at a skeleton diff~~ Refactor runtime fn enter(...) to select disputes, bitfields and backing candidates to protect against over length block issues Oct 5, 2021

Lldenaurois added 4 commits October 6, 2021 21:56

Implement first prioritization strategy

e06830b

Apply weights per dispute

ce0551c

Merge remote-tracking branch 'origin/master' into limit_paras_inherent

4bbe738

Add benchmark

5c162f5

emostov mentioned this pull request Oct 9, 2021

Benchmark paras inherent enter #4044

Closed

rphmeier reviewed Oct 11, 2021

View reviewed changes

Lldenaurois added 5 commits October 13, 2021 04:54

Merge remote-tracking branch 'origin/master' into limit_paras_inherent

65d1a89

Address feedback

83c9205

Update limit_paras_inherent_function

326a3f4

Cargo fmt

974e092

Move limit_paras_inherent out of enter function

28f16c5

eskimor reviewed Oct 13, 2021

View reviewed changes

Lldenaurois added 2 commits October 13, 2021 19:14

Modify weight calculations

bff955e

Add tests to demonstrate desired functionality

b1818ef

Lldenaurois force-pushed the limit_paras_inherent branch from 9af6191 to b1818ef Compare October 16, 2021 05:54

Add tests to ensure disputes are prioritized over backed candidates

8fb849e

rphmeier reviewed Oct 16, 2021

View reviewed changes

Fix double counting bug and address feedback

d4ffc5a

drahnr reviewed Oct 18, 2021

View reviewed changes

rphmeier reviewed Oct 18, 2021

View reviewed changes

Lldenaurois closed this Jan 20, 2022

Conversation

Lldenaurois commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Oct 11, 2021

Uh oh!

eskimor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lldenaurois Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rphmeier Oct 11, 2021 •

edited

Loading

Lldenaurois Oct 18, 2021 •

edited

Loading