From 597ea9203ad57d48b766fa7fb1fcc1d388118cb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 27 Mar 2024 23:02:37 +0000 Subject: [PATCH 1/2] pallet-scheduler: Unrequest call on failed lookup (#3849) When the scheduler fails to lookup a `call`, it should unrequest it, because it will not be required anymore. --- prdoc/pr_3849.prdoc | 13 +++++++++++++ substrate/frame/scheduler/src/lib.rs | 11 +++++++++++ substrate/frame/scheduler/src/tests.rs | 4 ++++ 3 files changed, 28 insertions(+) create mode 100644 prdoc/pr_3849.prdoc diff --git a/prdoc/pr_3849.prdoc b/prdoc/pr_3849.prdoc new file mode 100644 index 000000000000..a1372b60ffc6 --- /dev/null +++ b/prdoc/pr_3849.prdoc @@ -0,0 +1,13 @@ +title: Unrequest a pre-image when it failed to execute + +doc: + - audience: Runtime User + description: | + When a referenda finished the proposal will be scheduled. When it is scheduled, + the pre-image is requested. The pre-image is unrequested after the proposal + was executed. However, if the proposal failed to execute it wasn't unrequested. + Thus, it could not be removed from the on-chain state. This issue is now solved + by ensuring to unrequest the pre-image when it failed to execute. + +crates: + - name: pallet-scheduler diff --git a/substrate/frame/scheduler/src/lib.rs b/substrate/frame/scheduler/src/lib.rs index 62417b8d2cc2..a53742679e02 100644 --- a/substrate/frame/scheduler/src/lib.rs +++ b/substrate/frame/scheduler/src/lib.rs @@ -1267,6 +1267,17 @@ impl Pallet { id: task.maybe_id, }); + // It was not available when we needed it, so we don't need to have requested it + // anymore. + T::Preimages::drop(&task.call); + + // We don't know why `peek` failed, thus we most account here for the "full weight". + let _ = weight.try_consume(T::WeightInfo::service_task( + task.call.lookup_len().map(|x| x as usize), + task.maybe_id.is_some(), + task.maybe_periodic.is_some(), + )); + return Err((Unavailable, Some(task))) }, }; diff --git a/substrate/frame/scheduler/src/tests.rs b/substrate/frame/scheduler/src/tests.rs index bb02320ad751..440355336396 100644 --- a/substrate/frame/scheduler/src/tests.rs +++ b/substrate/frame/scheduler/src/tests.rs @@ -3008,6 +3008,8 @@ fn unavailable_call_is_detected() { // Ensure the preimage isn't available assert!(!Preimage::have(&bound)); + // But we have requested it + assert!(Preimage::is_requested(&hash)); // Executes in block 4. run_to_block(4); @@ -3016,5 +3018,7 @@ fn unavailable_call_is_detected() { System::events().last().unwrap().event, crate::Event::CallUnavailable { task: (4, 0), id: Some(name) }.into() ); + // It should not be requested anymore. + assert!(!Preimage::is_requested(&hash)); }); } From 5d314eb03ed03d9030bb38b3d2e205f2f5c266ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 27 Mar 2024 23:52:50 +0000 Subject: [PATCH 2/2] pallet-referenda: Detect incorrect pre-image length (#3850) There has been a case that a referenda failed because the length given to `submit` was incorrect. The pallet can actually check the length if the pre-image already exists to ensure that these kind of issues are not happening again. --- prdoc/pr_3850.prdoc | 15 +++++++++++++++ substrate/frame/referenda/src/lib.rs | 12 ++++++++++++ substrate/frame/referenda/src/tests.rs | 16 ++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 prdoc/pr_3850.prdoc diff --git a/prdoc/pr_3850.prdoc b/prdoc/pr_3850.prdoc new file mode 100644 index 000000000000..8f7ce16076e8 --- /dev/null +++ b/prdoc/pr_3850.prdoc @@ -0,0 +1,15 @@ +title: Detect incorrect pre-image length when submitting a referenda + +doc: + - audience: Runtime User + description: | + When submitting a referenda the `proposal` is passed as argument. + The `proposal` is most of the time a reference to a `pre-image` and + which also contains the length of the `pre-image`. This pull request + adds some logic to check that if the `pre-image` already exists and if + it exists, it ensures that the length is passed correctly. This prevents + that the referenda can not be executed because of a mismatch of this + length. + +crates: + - name: pallet-referenda diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index e616056c3022..fbe27e1a4784 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -424,6 +424,8 @@ pub mod pallet { BadStatus, /// The preimage does not exist. PreimageNotExist, + /// The preimage is stored with a different length than the one provided. + PreimageStoredWithDifferentLength, } #[pallet::hooks] @@ -462,6 +464,16 @@ pub mod pallet { let proposal_origin = *proposal_origin; let who = T::SubmitOrigin::ensure_origin(origin, &proposal_origin)?; + // If the pre-image is already stored, ensure that it has the same length as given in + // `proposal`. + if let (Some(preimage_len), Some(proposal_len)) = + (proposal.lookup_hash().and_then(|h| T::Preimages::len(&h)), proposal.lookup_len()) + { + if preimage_len != proposal_len { + return Err(Error::::PreimageStoredWithDifferentLength.into()) + } + } + let track = T::Tracks::track_for(&proposal_origin).map_err(|_| Error::::NoTrack)?; let submission_deposit = Self::take_deposit(who, T::SubmissionDeposit::get())?; diff --git a/substrate/frame/referenda/src/tests.rs b/substrate/frame/referenda/src/tests.rs index 8f51136de0bf..52251fcbdbee 100644 --- a/substrate/frame/referenda/src/tests.rs +++ b/substrate/frame/referenda/src/tests.rs @@ -666,3 +666,19 @@ fn clear_metadata_works() { })); }); } + +#[test] +fn detects_incorrect_len() { + ExtBuilder::default().build_and_execute(|| { + let hash = note_preimage(1); + assert_noop!( + Referenda::submit( + RuntimeOrigin::signed(1), + Box::new(RawOrigin::Root.into()), + frame_support::traits::Bounded::Lookup { hash, len: 3 }, + DispatchTime::At(1), + ), + Error::::PreimageStoredWithDifferentLength + ); + }); +}