diff --git a/prdoc/pr_9497.prdoc b/prdoc/pr_9497.prdoc new file mode 100644 index 0000000000000..c4f0b0b0bb416 --- /dev/null +++ b/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 diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs index 5590ccf7e3e0a..9be673f2e1daa 100644 --- a/substrate/frame/society/src/lib.rs +++ b/substrate/frame/society/src/lib.rs @@ -519,13 +519,14 @@ pub mod pallet { #[pallet::constant] type PeriodSpend: Get>; - /// 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>; - /// 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>; @@ -536,9 +537,9 @@ pub mod pallet { /// The origin that is allowed to call `found`. type FounderSetOrigin: EnsureOrigin; - /// The number of blocks between membership challenges. + /// The number of [Config::BlockNumberProvider] blocks between membership challenges. #[pallet::constant] - type ChallengePeriod: Get>; + type ChallengePeriod: Get>; /// The maximum number of payouts a member may have waiting unclaimed. #[pallet::constant] @@ -784,11 +785,20 @@ pub mod pallet { pub type DefenderVotes, I: 'static = ()> = StorageDoubleMap<_, Twox64Concat, RoundIndex, Twox64Concat, T::AccountId, Vote>; + /// Next intake rotation scheduled with [Config::BlockNumberProvider]. + #[pallet::storage] + pub type NextIntakeAt, I: 'static = ()> = StorageValue<_, BlockNumberFor>; + + /// Next challenge rotation scheduled with [Config::BlockNumberProvider]. + #[pallet::storage] + pub type NextChallengeAt, I: 'static = ()> = StorageValue<_, BlockNumberFor>; + #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { - fn on_initialize(n: SystemBlockNumberFor) -> Weight { + fn on_initialize(_n: SystemBlockNumberFor) -> 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 { Voting { elapsed: BlockNumber, more: BlockNumber }, Claim { elapsed: BlockNumber, more: BlockNumber }, + Intake { elapsed: BlockNumber }, } impl, I: 'static> Pallet { @@ -1488,13 +1503,72 @@ impl, I: 'static> Pallet { 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 { + match NextIntakeAt::::get() { + Some(next) => next, + None => { + // executed once. + let now = T::BlockNumberProvider::current_block_number(); + let prev_block = now.saturating_sub(BlockNumberFor::::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::::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::::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 { + match NextChallengeAt::::get() { + Some(next) => next, + None => { + // executed once. + let now = T::BlockNumberProvider::current_block_number(); + let prev_block = now.saturating_sub(BlockNumberFor::::one()); + let challenge_period = T::ChallengePeriod::get(); + let elapsed = prev_block % challenge_period; + let next_challenge_at = prev_block + (challenge_period - elapsed); + NextChallengeAt::::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::::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::::get(); diff --git a/substrate/frame/society/src/mock.rs b/substrate/frame/society/src/mock.rs index b203dc981769e..de3fb151d8d88 100644 --- a/substrate/frame/society/src/mock.rs +++ b/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; 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; @@ -231,6 +232,7 @@ pub fn next_intake() { ), Period::Claim { more, .. } => System::run_to_block::(System::block_number() + more), + Period::Intake { .. } => {}, } } diff --git a/substrate/frame/society/src/tests.rs b/substrate/frame/society/src/tests.rs index 82ba054501874..d559f6d9c01f7 100644 --- a/substrate/frame/society/src/tests.rs +++ b/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 = ::ChallengePeriod::get(); + let now = challenge_period + 1; + let next_challenge_at = challenge_period + challenge_period; + ::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::::get(0, 20), None); + assert_eq!(DefenderVotes::::get(0, 30), None); + assert_eq!(DefenderVotes::::get(0, 40), None); + assert_eq!(DefenderVotes::::get(0, 50), None); + // Check starting point + assert_eq!(members(), vec![10, 20, 30, 40, 50]); + assert_eq!(Defending::::get(), None); + + // early for challenge + let now = next_challenge_at - 1; + ::BlockNumberProvider::set_block_number(now); + Society::on_initialize(0); + assert_eq!(members(), vec![10, 20, 30, 40, 50]); + assert_eq!(Defending::::get(), None); + + // challenge with delay + let now = next_challenge_at + 2; + ::BlockNumberProvider::set_block_number(now); + Society::on_initialize(0); + assert_eq!(Defending::::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; + ::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; + ::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; + ::BlockNumberProvider::set_block_number(now); + Society::on_initialize(0); + // 40 is suspended + assert_eq!(members(), vec![10, 20, 30, 50]); + assert_eq!( + SuspendedMembers::::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::::get().unwrap().0, 30); + // Votes are reset + assert_eq!(DefenderVotes::::get(0, 20), None); + assert_eq!(DefenderVotes::::get(0, 30), None); + assert_eq!(DefenderVotes::::get(0, 40), None); + assert_eq!(DefenderVotes::::get(0, 50), None); + }); +} + +#[test] +fn intake_with_non_consecutive_blocks_works() { + EnvBuilder::new().execute(|| { + let voting_period: u64 = ::VotingPeriod::get(); + let claim_period: u64 = ::ClaimPeriod::get(); + let rotation_period = voting_period + claim_period; + let now = rotation_period + 1; + let next_intake_at = rotation_period + rotation_period; + ::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; + ::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; + ::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; + ::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 = ::VotingPeriod::get(); + let claim_period: u64 = ::ClaimPeriod::get(); + let rotation_period = voting_period + claim_period; + let now = rotation_period + 1; + let next_intake_at = rotation_period + rotation_period; + ::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; + ::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; + ::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); + }); +}