From a1678e2d3674a246058ddb767fc3d10b95bd52ac Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:13:30 +0100 Subject: [PATCH 01/12] Add integrity check to TracksInfo Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/types.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 0a8fb4ff8200..14dea73b3fe4 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 + /// [`check_integrity`] prior to any use. fn tracks() -> &'static [(Self::Id, TrackInfo)]; /// Determine the voting track for the given `origin`. @@ -153,6 +156,17 @@ 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) + /// Check assumptions about the static data that this trait provides. + fn check_integrity() -> Result<(), ()> + where + Balance: 'static, + Moment: 'static, + { + if Self::tracks().windows(2).all(|w| w[0].0 < w[1].0) { + Ok(()) + } else { + Err(()) + } } } From 8644887602ee27a73cd042ec60ec2a37eb451182 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:13:47 +0100 Subject: [PATCH 02/12] Test integrity_check Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/types.rs | 65 ++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 14dea73b3fe4..786b90747c64 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -684,4 +684,69 @@ 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!(BadTracksInfo::check_integrity().is_err(), "Should detect unsorted tracks."); + } } From 8621faff56ef6a28c3ddf20d2faa67312dcdcd54 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:13:58 +0100 Subject: [PATCH 03/12] Check TracksInfo integrity in referenda pallet Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/lib.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index 8912f9ad2173..8e9d0cb5b746 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -433,6 +433,10 @@ pub mod pallet { Self::do_try_state()?; Ok(()) } + + fn integrity_test() { + Self::do_integrity_test() + } } #[pallet::call] @@ -1304,6 +1308,11 @@ impl, I: 'static> Pallet { } } + #[cfg(any(feature = "std", test))] + fn do_integrity_test() { + T::Tracks::check_integrity().expect("Track configuration is not valid."); + } + /// Ensure the correctness of the state of this pallet. /// /// The following assertions must always apply. From 5939d534fb6f2d4849a37550335da4e1da714c68 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:14:11 +0100 Subject: [PATCH 04/12] Rely on sorted property in the trait Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/types.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 786b90747c64..58fb929c35b4 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -155,7 +155,12 @@ 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<(), ()> where From 7b43c64e88ea585de861eb269db1564154c16834 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:16:16 +0100 Subject: [PATCH 05/12] Re-formulate expect proof Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index 8e9d0cb5b746..a138476bc46a 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -1310,7 +1310,7 @@ impl, I: 'static> Pallet { #[cfg(any(feature = "std", test))] fn do_integrity_test() { - T::Tracks::check_integrity().expect("Track configuration is not valid."); + T::Tracks::check_integrity().expect("Static tracks configuration is valid."); } /// Ensure the correctness of the state of this pallet. From 471475ba3409b68ec85dad6fe0f84e80973b9e29 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:19:28 +0100 Subject: [PATCH 06/12] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_3325.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_3325.prdoc diff --git a/prdoc/pr_3325.prdoc b/prdoc/pr_3325.prdoc new file mode 100644 index 000000000000..07a61ca997b1 --- /dev/null +++ b/prdoc/pr_3325.prdoc @@ -0,0 +1,10 @@ +title: Ensure `TracksInfo` tracks are sorted by ID. + +doc: + - audience: Node 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 From 14c106696beac24259b676a6a940215a8e38a7a1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:20:03 +0100 Subject: [PATCH 07/12] Fix prdoc quotes Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_3308.prdoc | 2 +- prdoc/pr_3325.prdoc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 index 07a61ca997b1..f806b0f83108 100644 --- a/prdoc/pr_3325.prdoc +++ b/prdoc/pr_3325.prdoc @@ -1,4 +1,4 @@ -title: Ensure `TracksInfo` tracks are sorted by ID. +title: "Ensure `TracksInfo` tracks are sorted by ID." doc: - audience: Node Dev From 386fbf40febe0460e6ebe818525963ffc13b29fb Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:20:24 +0100 Subject: [PATCH 08/12] Audience: runtime dev Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_3325.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_3325.prdoc b/prdoc/pr_3325.prdoc index f806b0f83108..eb8126dc9125 100644 --- a/prdoc/pr_3325.prdoc +++ b/prdoc/pr_3325.prdoc @@ -1,7 +1,7 @@ title: "Ensure `TracksInfo` tracks are sorted by ID." doc: - - audience: Node Dev + - 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`. From 6cc77960ffd08b85c95c49ed4ccb7d21edbd4f29 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 15:38:15 +0100 Subject: [PATCH 09/12] Same guards on integrity_test Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index a138476bc46a..733c3fae1cf6 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -434,6 +434,7 @@ pub mod pallet { Ok(()) } + #[cfg(any(feature = "std", test))] fn integrity_test() { Self::do_integrity_test() } From 35eaad69279b333faaf5a0f48c3d64c731a77c59 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 18:39:50 +0100 Subject: [PATCH 10/12] Remove do_ function Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/lib.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index 733c3fae1cf6..c5bf2266e672 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -436,7 +436,7 @@ pub mod pallet { #[cfg(any(feature = "std", test))] fn integrity_test() { - Self::do_integrity_test() + T::Tracks::check_integrity().expect("Static tracks configuration is valid."); } } @@ -1309,11 +1309,6 @@ impl, I: 'static> Pallet { } } - #[cfg(any(feature = "std", test))] - fn do_integrity_test() { - T::Tracks::check_integrity().expect("Static tracks configuration is valid."); - } - /// Ensure the correctness of the state of this pallet. /// /// The following assertions must always apply. From 75fcf0ada600f112c4079992d4155d304e99e7c1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 14 Feb 2024 18:40:56 +0100 Subject: [PATCH 11/12] Better error message Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/referenda/src/types.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 58fb929c35b4..1aa82ddb62e9 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -162,7 +162,7 @@ pub trait TracksInfo { } /// Check assumptions about the static data that this trait provides. - fn check_integrity() -> Result<(), ()> + fn check_integrity() -> Result<(), &'static str> where Balance: 'static, Moment: 'static, @@ -170,7 +170,7 @@ pub trait TracksInfo { if Self::tracks().windows(2).all(|w| w[0].0 < w[1].0) { Ok(()) } else { - Err(()) + Err("The tracks that were returned by `tracks` were not sorted by `Id`") } } } @@ -752,6 +752,9 @@ mod tests { } } - assert!(BadTracksInfo::check_integrity().is_err(), "Should detect unsorted tracks."); + assert_eq!( + BadTracksInfo::check_integrity(), + Err("The tracks that were returned by `tracks` were not sorted by `Id`") + ); } } From ce381a22672b60af715ca81d32283c600cd4d34d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 15 Feb 2024 12:11:51 +0100 Subject: [PATCH 12/12] Update substrate/frame/referenda/src/types.rs --- substrate/frame/referenda/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 1aa82ddb62e9..b3c583322cce 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -147,7 +147,7 @@ pub trait TracksInfo { /// Sorted array of known tracks and their information. /// /// The array MUST be sorted by `Id`. Consumers of this trait are advised to assert - /// [`check_integrity`] prior to any use. + /// [`Self::check_integrity`] prior to any use. fn tracks() -> &'static [(Self::Id, TrackInfo)]; /// Determine the voting track for the given `origin`.