Skip to content

Commit

Permalink
Ensure referenda TracksInfo is sorted (#3325)
Browse files Browse the repository at this point in the history
Changes:
- Add `integrity_check` to trait `TracksInfo` to assert the sorting 
- Use the check in `integrity_test`
- Rely upon sorted property in the `info` for speedup

Note that the pallet already relies upon this (so far) untested
assumption
[here](https://github.com/paritytech/polkadot-sdk/blob/44c7a5eb8c375d77f685abf585961421e5f8b228/substrate/frame/referenda/src/lib.rs#L1280).

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
ggwpez and bkchr authored Feb 17, 2024
1 parent de73dd9 commit 8dc910c
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 3 deletions.
2 changes: 1 addition & 1 deletion prdoc/pr_3308.prdoc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
title: Parachains-Aura: Only produce once per slot
title: "Parachains-Aura: Only produce once per slot"

doc:
- audience: Node Dev
Expand Down
10 changes: 10 additions & 0 deletions prdoc/pr_3325.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
title: "Ensure `TracksInfo` tracks are sorted by ID."

doc:
- audience: Runtime Dev
description: |
Add a `integrity_check` function to trait `TracksInfo` and explicitly state that tracks must
always be sorted by ID. The referenda pallet now also uses this check in its `integrity_test`.

crates:
- name: pallet-referenda
5 changes: 5 additions & 0 deletions substrate/frame/referenda/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,11 @@ pub mod pallet {
Self::do_try_state()?;
Ok(())
}

#[cfg(any(feature = "std", test))]
fn integrity_test() {
T::Tracks::check_integrity().expect("Static tracks configuration is valid.");
}
}

#[pallet::call]
Expand Down
91 changes: 89 additions & 2 deletions substrate/frame/referenda/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,34 @@ pub trait TracksInfo<Balance, Moment> {
/// The origin type from which a track is implied.
type RuntimeOrigin;

/// Return the array of known tracks and their information.
/// Sorted array of known tracks and their information.
///
/// The array MUST be sorted by `Id`. Consumers of this trait are advised to assert
/// [`Self::check_integrity`] prior to any use.
fn tracks() -> &'static [(Self::Id, TrackInfo<Balance, Moment>)];

/// Determine the voting track for the given `origin`.
fn track_for(origin: &Self::RuntimeOrigin) -> Result<Self::Id, ()>;

/// Return the track info for track `id`, by default this just looks it up in `Self::tracks()`.
fn info(id: Self::Id) -> Option<&'static TrackInfo<Balance, Moment>> {
Self::tracks().iter().find(|x| x.0 == id).map(|x| &x.1)
let tracks = Self::tracks();
let maybe_index = tracks.binary_search_by_key(&id, |t| t.0).ok()?;

tracks.get(maybe_index).map(|(_, info)| info)
}

/// Check assumptions about the static data that this trait provides.
fn check_integrity() -> Result<(), &'static str>
where
Balance: 'static,
Moment: 'static,
{
if Self::tracks().windows(2).all(|w| w[0].0 < w[1].0) {
Ok(())
} else {
Err("The tracks that were returned by `tracks` were not sorted by `Id`")
}
}
}

Expand Down Expand Up @@ -670,4 +689,72 @@ mod tests {
assert_eq!(c.delay(pc(30).less_epsilon()), pc(100));
assert_eq!(c.delay(pc(0)), pc(100));
}

#[test]
fn tracks_integrity_check_detects_unsorted() {
use crate::mock::RuntimeOrigin;

pub struct BadTracksInfo;
impl TracksInfo<u64, u64> for BadTracksInfo {
type Id = u8;
type RuntimeOrigin = <RuntimeOrigin as OriginTrait>::PalletsOrigin;
fn tracks() -> &'static [(Self::Id, TrackInfo<u64, u64>)] {
static DATA: [(u8, TrackInfo<u64, u64>); 2] = [
(
1u8,
TrackInfo {
name: "root",
max_deciding: 1,
decision_deposit: 10,
prepare_period: 4,
decision_period: 4,
confirm_period: 2,
min_enactment_period: 4,
min_approval: Curve::LinearDecreasing {
length: Perbill::from_percent(100),
floor: Perbill::from_percent(50),
ceil: Perbill::from_percent(100),
},
min_support: Curve::LinearDecreasing {
length: Perbill::from_percent(100),
floor: Perbill::from_percent(0),
ceil: Perbill::from_percent(100),
},
},
),
(
0u8,
TrackInfo {
name: "none",
max_deciding: 3,
decision_deposit: 1,
prepare_period: 2,
decision_period: 2,
confirm_period: 1,
min_enactment_period: 2,
min_approval: Curve::LinearDecreasing {
length: Perbill::from_percent(100),
floor: Perbill::from_percent(95),
ceil: Perbill::from_percent(100),
},
min_support: Curve::LinearDecreasing {
length: Perbill::from_percent(100),
floor: Perbill::from_percent(90),
ceil: Perbill::from_percent(100),
},
},
),
];
&DATA[..]
}
fn track_for(_: &Self::RuntimeOrigin) -> Result<Self::Id, ()> {
unimplemented!()
}
}

assert_eq!(
BadTracksInfo::check_integrity(),
Err("The tracks that were returned by `tracks` were not sorted by `Id`")
);
}
}

0 comments on commit 8dc910c

Please sign in to comment.