[unstable2507] Backport #9497

prdoc/pr_9497.prdoc

@@ -0,0 +1,12 @@
+title: 'Society pallet supports non-consecutive block provider'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Society pallet correctly handles situations where on_initialize is invoked with block numbers that:
+    - increase but are not strictly consecutive (e.g., jump from 5 → 10), or
+    - are repeated (e.g., multiple blocks are built at the same Relay Chain parent block, all reporting the same BlockNumberProvider value).
+    This situation may occur when the BlockNumberProvider is not local - for example, on a parachain using the Relay Chain block number provider.
+
+crates:
+- name: pallet-society
+  bump: major

substrate/frame/society/src/lib.rs

@@ -519,13 +519,14 @@ pub mod pallet {
 		#[pallet::constant]
 		type PeriodSpend: Get<BalanceOf<Self, I>>;
 
-		/// The number of blocks on which new candidates should be voted on. Together with
+		/// The number of [Config::BlockNumberProvider] blocks on which new candidates should be
+		/// voted on. Together with
 		/// `ClaimPeriod`, this sums to the number of blocks between candidate intake periods.
 		#[pallet::constant]
 		type VotingPeriod: Get<BlockNumberFor<Self, I>>;
 
-		/// The number of blocks on which new candidates can claim their membership and be the
-		/// named head.
+		/// The number of [Config::BlockNumberProvider] blocks on which new candidates can claim
+		/// their membership and be the named head.
 		#[pallet::constant]
 		type ClaimPeriod: Get<BlockNumberFor<Self, I>>;
 
@@ -536,9 +537,9 @@ pub mod pallet {
 		/// The origin that is allowed to call `found`.
 		type FounderSetOrigin: EnsureOrigin<Self::RuntimeOrigin>;
 
-		/// The number of blocks between membership challenges.
+		/// The number of [Config::BlockNumberProvider] blocks between membership challenges.
 		#[pallet::constant]
-		type ChallengePeriod: Get<SystemBlockNumberFor<Self>>;
+		type ChallengePeriod: Get<BlockNumberFor<Self, I>>;
 
 		/// The maximum number of payouts a member may have waiting unclaimed.
 		#[pallet::constant]
@@ -784,11 +785,20 @@ pub mod pallet {
 	pub type DefenderVotes<T: Config<I>, I: 'static = ()> =
 		StorageDoubleMap<_, Twox64Concat, RoundIndex, Twox64Concat, T::AccountId, Vote>;
 
+	/// Next intake rotation scheduled with [Config::BlockNumberProvider].
+	#[pallet::storage]
+	pub type NextIntakeAt<T: Config<I>, I: 'static = ()> = StorageValue<_, BlockNumberFor<T, I>>;
+
+	/// Next challenge rotation scheduled with [Config::BlockNumberProvider].
+	#[pallet::storage]
+	pub type NextChallengeAt<T: Config<I>, I: 'static = ()> = StorageValue<_, BlockNumberFor<T, I>>;
+
 	#[pallet::hooks]
 	impl<T: Config<I>, I: 'static> Hooks<SystemBlockNumberFor<T>> for Pallet<T, I> {
-		fn on_initialize(n: SystemBlockNumberFor<T>) -> Weight {
+		fn on_initialize(_n: SystemBlockNumberFor<T>) -> Weight {
 			let mut weight = Weight::zero();
 			let weights = T::BlockWeights::get();
+			let now = T::BlockNumberProvider::current_block_number();
 
 			let phrase = b"society_rotation";
 			// we'll need a random seed here.
@@ -801,18 +811,21 @@ pub mod pallet {
 			let mut rng = ChaChaRng::from_seed(seed);
 
 			// Run a candidate/membership rotation
-			match Self::period() {
-				Period::Voting { elapsed, .. } if elapsed.is_zero() => {
-					Self::rotate_intake(&mut rng);
-					weight.saturating_accrue(weights.max_block / 20);
-				},
-				_ => {},
+			let is_intake_moment = match Self::period() {
+				Period::Intake { .. } => true,
+				_ => false,
+			};
+			if is_intake_moment {
+				Self::rotate_intake(&mut rng);
+				weight.saturating_accrue(weights.max_block / 20);
+				Self::set_next_intake_at();
 			}
 
 			// Run a challenge rotation
-			if (n % T::ChallengePeriod::get()).is_zero() {
+			if now >= Self::next_challenge_at() {
 				Self::rotate_challenge(&mut rng);
 				weight.saturating_accrue(weights.max_block / 20);
+				Self::set_next_challenge_at();
 			}
 
 			weight
@@ -1475,9 +1488,11 @@ impl_ensure_origin_with_arg_ignoring_arg! {
 	{}
 }
 
+#[derive(Debug, PartialEq, Eq)]
 pub enum Period<BlockNumber> {
 	Voting { elapsed: BlockNumber, more: BlockNumber },
 	Claim { elapsed: BlockNumber, more: BlockNumber },
+	Intake { elapsed: BlockNumber },
 }
 
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -1488,13 +1503,72 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		let rotation_period = voting_period + claim_period;
 		let now = T::BlockNumberProvider::current_block_number();
 		let phase = now % rotation_period;
-		if phase < voting_period {
+		if now >= Self::next_intake_at() {
+			Period::Intake { elapsed: now - Self::next_intake_at() }
+		} else if phase < voting_period {
 			Period::Voting { elapsed: phase, more: voting_period - phase }
 		} else {
 			Period::Claim { elapsed: phase - voting_period, more: rotation_period - phase }
 		}
 	}
 
+	/// Next intake (candidate/membership) rotation scheduled with [Config::BlockNumberProvider].
+	///
+	/// Rounds the previous block number up to the next rotation period (voting + claim periods).
+	pub fn next_intake_at() -> BlockNumberFor<T, I> {
+		match NextIntakeAt::<T, I>::get() {
+			Some(next) => next,
+			None => {
+				// executed once.
+				let now = T::BlockNumberProvider::current_block_number();
+				let prev_block = now.saturating_sub(BlockNumberFor::<T, I>::one());
+				let rotation_period = T::VotingPeriod::get().saturating_add(T::ClaimPeriod::get());
+				let elapsed = prev_block % rotation_period;
+				let next_intake_at = prev_block + (rotation_period - elapsed);
+				NextIntakeAt::<T, I>::put(next_intake_at);
+				next_intake_at
+			},
+		}
+	}
+
+	/// Set the next intake (candidate/membership) rotation.
+	///
+	/// This supposed to be called once the current intake is executed.
+	fn set_next_intake_at() {
+		let prev_next_intake_at = Self::next_intake_at();
+		let next_intake_at = prev_next_intake_at
+			.saturating_add(T::VotingPeriod::get().saturating_add(T::ClaimPeriod::get()));
+		NextIntakeAt::<T, I>::put(next_intake_at);
+	}
+
+	/// Returns the next challenge rotation scheduled with [Config::BlockNumberProvider].
+	///
+	/// Rounds the previous block number up to the next multiple of the challenge duration.
+	pub fn next_challenge_at() -> BlockNumberFor<T, I> {
+		match NextChallengeAt::<T, I>::get() {
+			Some(next) => next,
+			None => {
+				// executed once.
+				let now = T::BlockNumberProvider::current_block_number();
+				let prev_block = now.saturating_sub(BlockNumberFor::<T, I>::one());
+				let challenge_period = T::ChallengePeriod::get();
+				let elapsed = prev_block % challenge_period;
+				let next_challenge_at = prev_block + (challenge_period - elapsed);
+				NextChallengeAt::<T, I>::put(next_challenge_at);
+				next_challenge_at
+			},
+		}
+	}
+
+	/// Set the next challenge rotation.
+	///
+	/// This supposed to be called once the current challenge is executed.
+	fn set_next_challenge_at() {
+		let prev_next_challenge_at = Self::next_challenge_at();
+		let next_challenge_at = prev_next_challenge_at.saturating_add(T::ChallengePeriod::get());
+		NextChallengeAt::<T, I>::put(next_challenge_at);
+	}
+
 	/// Returns true if the given `target_round` is still in its initial voting phase.
 	fn in_progress(target_round: RoundIndex) -> bool {
 		let round = RoundCount::<T, I>::get();

substrate/frame/society/src/mock.rs

@@ -48,6 +48,7 @@ parameter_types! {
 ord_parameter_types! {
 	pub const ChallengePeriod: u64 = 8;
 	pub const ClaimPeriod: u64 = 1;
+	pub const VotingPeriod: u64 = 3;
 	pub const FounderSetAccount: u128 = 1;
 	pub const SuspensionJudgementSetAccount: u128 = 2;
 	pub const MaxPayouts: u32 = 10;
@@ -75,7 +76,7 @@ impl Config for Test {
 	type Randomness = TestRandomness<Self>;
 	type GraceStrikes = ConstU32<1>;
 	type PeriodSpend = ConstU64<1000>;
-	type VotingPeriod = ConstU64<3>;
+	type VotingPeriod = VotingPeriod;
 	type ClaimPeriod = ClaimPeriod;
 	type MaxLockDuration = ConstU64<100>;
 	type FounderSetOrigin = EnsureSignedBy<FounderSetAccount, u128>;
@@ -231,6 +232,7 @@ pub fn next_intake() {
 		),
 		Period::Claim { more, .. } =>
 			System::run_to_block::<AllPalletsWithSystem>(System::block_number() + more),
+		Period::Intake { .. } => {},
 	}
 }
 

substrate/frame/society/src/tests.rs

@@ -1469,3 +1469,194 @@ fn poke_deposit_handles_insufficient_balance() {
 		);
 	});
 }
+
+#[test]
+fn challenge_with_non_consecutive_blocks_works() {
+	EnvBuilder::new().execute(|| {
+		let challenge_period: u64 = <Test as Config>::ChallengePeriod::get();
+		let now = challenge_period + 1;
+		let next_challenge_at = challenge_period + challenge_period;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+
+		assert_eq!(Society::next_challenge_at(), next_challenge_at);
+
+		Society::on_initialize(0);
+
+		// Add some members
+		place_members([20, 30, 40, 50]);
+		// Votes are empty
+		assert_eq!(DefenderVotes::<Test>::get(0, 20), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 30), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 40), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 50), None);
+		// Check starting point
+		assert_eq!(members(), vec![10, 20, 30, 40, 50]);
+		assert_eq!(Defending::<Test>::get(), None);
+
+		// early for challenge
+		let now = next_challenge_at - 1;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		Society::on_initialize(0);
+		assert_eq!(members(), vec![10, 20, 30, 40, 50]);
+		assert_eq!(Defending::<Test>::get(), None);
+
+		// challenge with delay
+		let now = next_challenge_at + 2;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		Society::on_initialize(0);
+		assert_eq!(Defending::<Test>::get().unwrap().0, 40);
+		// They can always free vote for themselves
+		assert_ok!(Society::defender_vote(Origin::signed(40), false));
+		// everyone votes against 40
+		assert_ok!(Society::defender_vote(Origin::signed(20), false));
+		assert_ok!(Society::defender_vote(Origin::signed(30), false));
+		assert_ok!(Society::defender_vote(Origin::signed(50), false));
+
+		let next_challenge_at = next_challenge_at + challenge_period;
+		assert_eq!(Society::next_challenge_at(), next_challenge_at);
+
+		// early for challenge
+		let now = next_challenge_at - 2;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		Society::on_initialize(0);
+		assert_eq!(members(), vec![10, 20, 30, 40, 50]);
+
+		// challenge with delay
+		let now = next_challenge_at - 1;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		Society::on_initialize(0);
+		assert_eq!(members(), vec![10, 20, 30, 40, 50]);
+
+		// challenge without delay
+		let now = next_challenge_at;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		Society::on_initialize(0);
+		// 40 is suspended
+		assert_eq!(members(), vec![10, 20, 30, 50]);
+		assert_eq!(
+			SuspendedMembers::<Test>::get(40),
+			Some(MemberRecord { rank: 0, strikes: 0, vouching: None, index: 3 })
+		);
+		// Reset votes for last challenge
+		assert_ok!(Society::cleanup_challenge(Origin::signed(0), 0, 10));
+		// New defender is chosen, 30 is challenged
+		assert_eq!(Defending::<Test>::get().unwrap().0, 30);
+		// Votes are reset
+		assert_eq!(DefenderVotes::<Test>::get(0, 20), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 30), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 40), None);
+		assert_eq!(DefenderVotes::<Test>::get(0, 50), None);
+	});
+}
+
+#[test]
+fn intake_with_non_consecutive_blocks_works() {
+	EnvBuilder::new().execute(|| {
+		let voting_period: u64 = <Test as Config>::VotingPeriod::get();
+		let claim_period: u64 = <Test as Config>::ClaimPeriod::get();
+		let rotation_period = voting_period + claim_period;
+		let now = rotation_period + 1;
+		let next_intake_at = rotation_period + rotation_period;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+
+		assert_eq!(Society::next_intake_at(), next_intake_at);
+		assert_eq!(Society::period(), Period::Voting { elapsed: 1, more: 2 });
+
+		Society::on_initialize(0);
+
+		assert_eq!(Balances::free_balance(20), 50);
+		// Bid causes Candidate Deposit to be reserved.
+		assert_ok!(Society::bid(RuntimeOrigin::signed(20), 0));
+		assert_eq!(Balances::free_balance(20), 25);
+
+		// early for intake
+		let now = next_intake_at - 1;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		assert_eq!(Society::period(), Period::Claim { elapsed: 0, more: 1 });
+		Society::on_initialize(0);
+		assert_eq!(candidacies(), vec![]);
+
+		// intake with delay
+		let now = next_intake_at + 1;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		assert_eq!(Society::period(), Period::Intake { elapsed: 1 });
+		Society::on_initialize(0);
+		// 20 is now a candidate
+		assert_eq!(candidacies(), vec![(20, candidacy(1, 0, Deposit(25), 0, 0))]);
+		// 10 (a member) can vote for the candidate
+		assert_ok!(Society::vote(Origin::signed(10), 20, true));
+		conclude_intake(true, None);
+
+		let next_intake_at = next_intake_at + rotation_period;
+		assert_eq!(Society::next_intake_at(), next_intake_at);
+
+		// intake without delay
+		let now = next_intake_at;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		assert_eq!(Society::period(), Period::Intake { elapsed: 0 });
+		Society::on_initialize(0);
+		// 20 is now a member of the society
+		assert_eq!(members(), vec![10, 20]);
+		// Reserved balance is returned
+		assert_eq!(Balances::free_balance(20), 50);
+	});
+}
+
+#[test]
+fn intake_idempotency() {
+	EnvBuilder::new().execute(|| {
+		let voting_period: u64 = <Test as Config>::VotingPeriod::get();
+		let claim_period: u64 = <Test as Config>::ClaimPeriod::get();
+		let rotation_period = voting_period + claim_period;
+		let now = rotation_period + 1;
+		let next_intake_at = rotation_period + rotation_period;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+
+		assert_eq!(Society::next_intake_at(), next_intake_at);
+		assert_eq!(Society::period(), Period::Voting { elapsed: 1, more: 2 });
+
+		// initialize the next intake at
+		Society::on_initialize(0);
+
+		// Bid to become a candidate
+		assert_eq!(Balances::free_balance(20), 50);
+		assert_ok!(Society::bid(RuntimeOrigin::signed(20), 0));
+
+		// intake
+		let now = next_intake_at;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		assert_eq!(Society::period(), Period::Intake { elapsed: 0 });
+		Society::on_initialize(0);
+		// 20 is now a candidate
+		assert_eq!(candidacies(), vec![(20, candidacy(1, 0, Deposit(25), 0, 0))]);
+
+		// Bid one more account to become a candidate
+		assert_eq!(Balances::free_balance(40), 50);
+		assert_ok!(Society::bid(RuntimeOrigin::signed(40), 10));
+
+		// next intake has updated
+		let next_intake_at = next_intake_at + rotation_period;
+		assert_eq!(Society::next_intake_at(), next_intake_at);
+
+		// `on_initialize` at the same block provider block number has not effect
+		assert_eq!(Society::period(), Period::Voting { elapsed: 0, more: 3 });
+		Society::on_initialize(0);
+		// 20 is still the only candidate
+		assert_eq!(candidacies(), vec![(20, candidacy(1, 0, Deposit(25), 0, 0))]);
+
+		// 10 (a member) can vote for the candidate
+		assert_ok!(Society::vote(Origin::signed(10), 20, true));
+		// moves the block to the Claim period
+		conclude_intake(true, None);
+
+		// next intake adds the candidate to the society
+		let now = next_intake_at;
+		<Test as Config>::BlockNumberProvider::set_block_number(now);
+		assert_eq!(Society::period(), Period::Intake { elapsed: 0 });
+		Society::on_initialize(0);
+		// 20 is now a member of the society
+		assert_eq!(members(), vec![10, 20]);
+		// Reserved balance is returned
+		assert_eq!(Balances::free_balance(20), 50);
+	});
+}