Skip to content

Society pallet supports non-consecutive block provider#9497

Merged
muharem merged 10 commits intomasterfrom
muharem-society-block-provider
Aug 25, 2025
Merged

Society pallet supports non-consecutive block provider#9497
muharem merged 10 commits intomasterfrom
muharem-society-block-provider

Conversation

@muharem
Copy link
Copy Markdown
Contributor

@muharem muharem commented Aug 15, 2025

Society pallet supports non-consecutive block provider

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.

@muharem muharem requested a review from a team as a code owner August 15, 2025 14:04
Comment thread substrate/frame/society/src/lib.rs Outdated
let phase = now % rotation_period;
if phase < voting_period {
if now > Self::next_intake_at() {
Period::IntakeDelay { elapsed: now - Self::next_intake_at() }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if delayed for too long we wanna make sure we do not inter the Voting period again

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment, if delayed for too long I don't see any special case using the variant IntakeDelay, the inner field elapsed is never used.

I would expect the comment to say: "if some block haven't been seen, we still trigger the rotation on the first block after the next_intake_at."

@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Aug 15, 2025
@muharem muharem requested review from ggwpez and gui1117 August 15, 2025 14:09
Comment thread substrate/frame/society/src/lib.rs Outdated
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;
prev_block + (rotation_period - elapsed)
Copy link
Copy Markdown
Member

@ggwpez ggwpez Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the subtraction here cannot underflow since we do the % first. Otherwise if it could underflow it would result in a very big value since its using u32.

Copy link
Copy Markdown

@eugypalu eugypalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks alright but would be good to have a test for it with delayed / skipped blocks 😅

Comment thread substrate/frame/society/src/lib.rs
Comment thread substrate/frame/society/src/lib.rs Outdated
///
/// 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();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to call the function here instead of directly using NextIntakeAt::get() because it could be that it is not yet migrated?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the current flow here it cannot be unset. but I think its safer and more correct to get it with that function since we have a lazy initialization for it

Copy link
Copy Markdown
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from this comment looks good to me

Comment thread substrate/frame/society/src/lib.rs
gui1117
gui1117 previously approved these changes Aug 21, 2025
@gui1117 gui1117 self-requested a review August 21, 2025 23:23
@gui1117 gui1117 dismissed their stale review August 21, 2025 23:36

outdated review

@muharem muharem added this pull request to the merge queue Aug 25, 2025
Merged via the queue into master with commit 1a51257 Aug 25, 2025
278 of 283 checks passed
@muharem muharem deleted the muharem-society-block-provider branch August 25, 2025 11:51
@EgorPopelyaev EgorPopelyaev added the A4-backport-unstable2507 Pull request must be backported to the unstable2507 release branch label Aug 25, 2025
@paritytech-release-backport-bot
Copy link
Copy Markdown

Successfully created backport PR for unstable2507:

paritytech-release-backport-bot Bot pushed a commit that referenced this pull request Aug 25, 2025
Society pallet supports non-consecutive block provider

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.

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
(cherry picked from commit 1a51257)
pepoviola pushed a commit that referenced this pull request Aug 26, 2025
Society pallet supports non-consecutive block provider

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.

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
EgorPopelyaev added a commit that referenced this pull request Sep 5, 2025
Backport #9497 into `unstable2507` from muharem.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: muharem <ismailov.m.h@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Egor_P <egor@parity.io>
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
Society pallet supports non-consecutive block provider

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.

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A4-backport-unstable2507 Pull request must be backported to the unstable2507 release branch T1-FRAME This PR/Issue is related to core FRAME, the framework.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants