Bound pallet-offences #11585

ggwpez · 2022-06-02T21:43:51Z

Bound the offences pallet.
Changes:

Introduce global MaxOffenders and MaxReportes constants with both value 100. This is necessary since the staking and offences traits would otherwise all need to be parameterized. Grandpa and BABE both only use at most 1 for these values, so 100 should be enough.
Introduce MaxSameKindReportsEncodedLen and MaxOpaqueTimeSlotEncodedLen to bound the storage of encoded values. There is a test which tests that these constants can encode the respective values, it would be important to run that test against the real runtime config, not just the mock.
Introduce MaxConcurrentReportsPerTime and MaxSameKindReports. These may or may not be necessary, I am not sure. Maybe they can be replaced with instances of MaxOffenders/MaxReporters.
Change the naming of Exposure/ExposureOf and ActiveExposure to avoid a name-clash
Replace a lot of vec![] with Default::default() to make future refactoring easier.

TODO:

There is one expect("todo") where I was unsure about the vector length
What about migrations?

Part of paritytech/polkadot-sdk#323

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

KiChjang · 2022-06-03T17:16:01Z

Would really like @kianenigma to review this when he comes back...

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez · 2022-07-14T10:38:49Z

bot rebase

paritytech-processbot · 2022-07-14T10:38:55Z

Rebased

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

kianenigma · 2022-07-20T12:49:05Z

frame/babe/src/lib.rs

-		};
+		}
+		.try_into()
+		.defensive_proof("MaxReporters must be at least 1;")


do we have defensive_expect? we should :D

kianenigma · 2022-07-20T12:52:06Z

frame/grandpa/src/lib.rs

+			None => vec![],
+		}
+		.try_into()
+		.expect("MaxReporters must be at least 1;");


where is this ensured? Should be in integrity_checks

Its a constant, not a configuration variable, so we only have to ensure it once.
But yea, could add it to the integrity checks.

kianenigma · 2022-07-20T12:55:07Z

frame/offences/src/lib.rs

 		/// A handler called for every offence report.
 		type OnOffenceHandler: OnOffenceHandler<Self::AccountId, Self::IdentificationTuple, Weight>;
+
+		/// Maximum number of reports of the same kind that happened at a specific time slot.
+		#[pallet::constant]


Do all of these really need to be in metadata? harmless, but I wonder if you had thought about each.

kianenigma · 2022-07-20T13:00:25Z

frame/staking/src/lib.rs

+where
+	AccountId: Debug + Encode + Decode + MaxEncodedLen + Clone + Eq + PartialEq,
+	Balance: Debug + HasCompact + Encode + Decode + MaxEncodedLen + Clone + Eq + PartialEq,
+	MaxNominations: Get<u32>,


Not a good name -- we use this term to determine the number of nominations that a single nominator can have. i.e. 16.

I suggest using MaxBackers here instead.

kianenigma · 2022-07-20T13:03:09Z

frame/staking/src/lib.rs

 	/// The total balance backing this validator.
 	#[codec(compact)]
 	pub total: Balance,
 	/// The validator's own stash that is exposed.
 	#[codec(compact)]
 	pub own: Balance,
 	/// The portions of nominators stashes that are exposed.
-	pub others: Vec<IndividualExposure<AccountId, Balance>>,
+	pub others: BoundedVec<IndividualExposure<AccountId, Balance>, MaxNominations>,


This is a great direction that you started here: we certainly want to bound this. Problem is that there is nothing in the ElectionProvider trait explicitly guarantees that this is the case.

What we want to do is:

Staking tells ElectionProvider how many MaxBackers it accepts.

Staking miner respects this when generating solution, and automatically sorts the backings of each validator, and throws away the smaller ones. (currently we area doing onchain, which is horrible)

ElectionProvider checks this for submitted solutions and ensures it is the case.

Staking can just do a defensive truncate when it receives a solution from ElectionProvider.

This new MaxBackers should also replace MaxRewardableNominatorsPerValidator, and entirely replace the notion of oversubscribed validators.

If interested in doing more steps in this path, I will be happy to do a better writeup about it.

Without these steps, we should set MaxBackers to MaxElectingVoters, because in principle it is possible that in an election everyone nominates one validator, and that single validators ends up having the whole e.g. 22500 backers backing them.

kianenigma · 2022-07-20T13:06:03Z

frame/staking/src/lib.rs

 /// A typed conversion from stash account ID to the active exposure of nominators
 /// on that account.
 ///
 /// Active exposure is the exposure of the validator set currently validating, i.e. in
 /// `active_era`. It can differ from the latest planned exposure in `current_era`.
-pub struct ExposureOf<T>(sp_std::marker::PhantomData<T>);
+pub struct ActiveExposure<T>(sp_std::marker::PhantomData<T>);


better name now, but still not quite happy with it.

kianenigma · 2022-07-20T13:06:31Z

frame/staking/src/pallet/impls.rs

@@ -553,10 +552,10 @@ impl<T: Config> Pallet<T> {
 						total = total.saturating_add(stake);
 					});

-				let exposure = Exposure { own, others, total };
+				let exposure = Exposure { own, others: others.try_into().expect("TODO"), total };


This is exactly https://github.com/paritytech/substrate/pull/11585/files#r925580489

stale · 2022-08-31T01:35:58Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2022-09-30T13:48:09Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

kianenigma · 2022-09-30T15:29:40Z

This will someday be done by @Ank4n, but a long chain of other work is needed.

stale · 2022-11-29T18:22:32Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Bound pallet-offences

9d27600

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

github-actions bot added the A0-please_review Pull request needs code review. label Jun 2, 2022

ggwpez 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 Jun 2, 2022

shawntabrizi mentioned this pull request Aug 24, 2023

[FRAME Core] Remove without_storage_info on pallets paritytech/polkadot-sdk#323

Open

40 tasks

KiChjang requested a review from kianenigma June 3, 2022 17:16

ggwpez added 2 commits June 13, 2022 19:39

Merge remote-tracking branch 'origin/master' into oty-bounded-offences

76854e4

Fix traits

2417584

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez requested a review from andresilva as a code owner June 17, 2022 13:05

ggwpez added 7 commits June 17, 2022 16:31

Use correct nightly

8e05856

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Unreproducible compile error...

a7bcc8e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into oty-bounded-offences

1ddfba4

Compile

da88df6

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

WIP

130f796

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix tests

82ab81d

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix tests

6524811

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into oty-bounded-offences

3226339

ggwpez added 6 commits July 18, 2022 10:13

Make compile

b02f13e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into oty-bounded-offences

ad08280

Fix CI

889898a

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix benchmarks

ee29920

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Beauty fixes

4a904d5

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Beauty fixes

a4ad15f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes E1-runtimemigration and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 18, 2022

ggwpez added C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 18, 2022

kianenigma reviewed Jul 20, 2022

View reviewed changes

ggwpez mentioned this pull request Jul 29, 2022

Staking: Introduce MaxBackersPerWinner #11935

Closed

4 tasks

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 31, 2022

ggwpez removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 31, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 30, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 30, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 30, 2022

paritytech deleted a comment from stale bot Oct 30, 2022

ggwpez removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 30, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 29, 2022

stale bot closed this Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bound pallet-offences #11585

Bound pallet-offences #11585

ggwpez commented Jun 2, 2022 •

edited

Loading

KiChjang commented Jun 3, 2022

ggwpez commented Jul 14, 2022

paritytech-processbot bot commented Jul 14, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

ggwpez Jul 20, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

kianenigma Jul 20, 2022

stale bot commented Aug 31, 2022

stale bot commented Sep 30, 2022

kianenigma commented Sep 30, 2022

stale bot commented Nov 29, 2022

Bound pallet-offences #11585

Bound pallet-offences #11585

Conversation

ggwpez commented Jun 2, 2022 • edited Loading

KiChjang commented Jun 3, 2022

ggwpez commented Jul 14, 2022

paritytech-processbot bot commented Jul 14, 2022

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

stale bot commented Aug 31, 2022

stale bot commented Sep 30, 2022

kianenigma commented Sep 30, 2022

stale bot commented Nov 29, 2022

ggwpez commented Jun 2, 2022 •

edited

Loading