From 8dc910c8a18ad91a18ff17c492f5cc3977a5133e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 17 Feb 2024 12:34:34 +0100 Subject: [PATCH] Ensure referenda `TracksInfo` is sorted (#3325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Bastian Köcher --- prdoc/pr_3308.prdoc | 2 +- prdoc/pr_3325.prdoc | 10 +++ substrate/frame/referenda/src/lib.rs | 5 ++ substrate/frame/referenda/src/types.rs | 91 +++++++++++++++++++++++++- 4 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 prdoc/pr_3325.prdoc diff --git a/prdoc/pr_3308.prdoc b/prdoc/pr_3308.prdoc index 611461e25fff..386cbd6230b4 100644 --- a/prdoc/pr_3308.prdoc +++ b/prdoc/pr_3308.prdoc @@ -1,4 +1,4 @@ -title: Parachains-Aura: Only produce once per slot +title: "Parachains-Aura: Only produce once per slot" doc: - audience: Node Dev diff --git a/prdoc/pr_3325.prdoc b/prdoc/pr_3325.prdoc new file mode 100644 index 000000000000..eb8126dc9125 --- /dev/null +++ b/prdoc/pr_3325.prdoc @@ -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 diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index 8912f9ad2173..c5bf2266e672 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -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] diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 0a8fb4ff8200..b3c583322cce 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -144,7 +144,10 @@ pub trait TracksInfo { /// 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)]; /// Determine the voting track for the given `origin`. @@ -152,7 +155,23 @@ pub trait TracksInfo { /// 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> { - 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`") + } } } @@ -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 for BadTracksInfo { + type Id = u8; + type RuntimeOrigin = ::PalletsOrigin; + fn tracks() -> &'static [(Self::Id, TrackInfo)] { + static DATA: [(u8, TrackInfo); 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 { + unimplemented!() + } + } + + assert_eq!( + BadTracksInfo::check_integrity(), + Err("The tracks that were returned by `tracks` were not sorted by `Id`") + ); + } }