From 3beae172f019feb2ba9491c75ce7b3a8a2159d5a Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 13:21:30 +0100 Subject: [PATCH 01/14] initial idea --- Cargo.lock | 14 ++++ Cargo.toml | 1 + frame/preimage/Cargo.toml | 38 +++++++++++ frame/preimage/src/lib.rs | 130 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 183 insertions(+) create mode 100644 frame/preimage/Cargo.toml create mode 100644 frame/preimage/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 20499facb5b71..caf3ad09c23df 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5807,6 +5807,20 @@ dependencies = [ "sp-std", ] +[[package]] +name = "pallet-preimage" +version = "4.0.0-dev" +dependencies = [ + "frame-support", + "frame-system", + "parity-scale-codec", + "scale-info", + "sp-core", + "sp-io", + "sp-runtime", + "sp-std", +] + [[package]] name = "pallet-proxy" version = "4.0.0-dev" diff --git a/Cargo.toml b/Cargo.toml index 32d10ca8978dd..d84c87a6b70b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,6 +103,7 @@ members = [ "frame/nicks", "frame/node-authorization", "frame/offences", + "frame/preimage", "frame/proxy", "frame/randomness-collective-flip", "frame/recovery", diff --git a/frame/preimage/Cargo.toml b/frame/preimage/Cargo.toml new file mode 100644 index 0000000000000..35b99c6335b16 --- /dev/null +++ b/frame/preimage/Cargo.toml @@ -0,0 +1,38 @@ +[package] +name = "pallet-preimage" +version = "4.0.0-dev" +authors = ["Parity Technologies "] +edition = "2018" +license = "Apache-2.0" +homepage = "https://substrate.io" +repository = "https://github.com/paritytech/substrate/" +description = "FRAME pallet for storing preimages of hashes" +readme = "README.md" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] } +scale-info = { version = "1.0", default-features = false, features = ["derive"] } +sp-std = { version = "4.0.0-dev", default-features = false, path = "../../primitives/std" } +sp-io = { version = "4.0.0-dev", default-features = false, path = "../../primitives/io" } +sp-runtime = { version = "4.0.0-dev", default-features = false, path = "../../primitives/runtime" } +frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } +frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } + +[dev-dependencies] +sp-core = { version = "4.0.0-dev", path = "../../primitives/core" } + +[features] +default = ["std"] +std = [ + "codec/std", + "scale-info/std", + "sp-std/std", + "sp-io/std", + "sp-runtime/std", + "frame-support/std", + "frame-system/std", +] +try-runtime = ["frame-support/try-runtime"] diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs new file mode 100644 index 0000000000000..4d32d4d3308d8 --- /dev/null +++ b/frame/preimage/src/lib.rs @@ -0,0 +1,130 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! # Preimage Pallet +//! +//! - [`Config`] +//! - [`Call`] +//! +//! ## Overview +//! +//! The Preimage pallet allows for the users and the runtime to store the preimage +//! of a hash on chain. This can be used by other pallets where storing and managing +//! large byte-blobs. + +#![cfg_attr(not(feature = "std"), no_std)] + +use sp_runtime::DispatchResult; +use sp_std::{convert::TryFrom, prelude::*}; + +use codec::{Decode, Encode, MaxEncodedLen}; +use frame_support::{ + pallet_prelude::Get, + traits::{Currency, ReservableCurrency}, + BoundedVec, +}; +use scale_info::TypeInfo; + +pub use pallet::*; + +#[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] +pub struct Preimage +where + MaxSize: Get, +{ + preimage: BoundedVec, + deposit: Option<(Balance, AccountId)>, +} + +type BalanceOf = + <::Currency as Currency<::AccountId>>::Balance; + +#[frame_support::pallet] +pub mod pallet { + use super::{DispatchResult, *}; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + + #[pallet::config] + pub trait Config: frame_system::Config { + /// The overarching event type. + type Event: From> + IsType<::Event>; + + /// Currency type for this pallet. + type Currency: ReservableCurrency; + + /// An origin that can bypass deposits to place a preimage on-chain. + type ForceOrigin: EnsureOrigin; + + /// Max size allowed for a preimage. + type MaxSize: Get + TypeInfo + MaxEncodedLen; + + /// The base deposit for placing a preimage on chain. + type BaseDeposit: Get>; + + /// The per-byte deposit for placing a preimage on chain. + type ByteDeposit: Get>; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + #[pallet::generate_storage_info] + pub struct Pallet(PhantomData); + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// A sudo just took place. \[result\] + Sudid(DispatchResult), + /// The \[sudoer\] just switched identity; the old key is supplied. + KeyChanged(T::AccountId), + /// A sudo just took place. \[result\] + SudoAsDone(DispatchResult), + } + + #[pallet::error] + pub enum Error { + /// Preimage is too large to store on-chain. + TooLarge, + } + + /// The preimages stored by this pallet. + #[pallet::storage] + #[pallet::getter(fn key)] + pub(super) type Key = StorageMap< + _, + Identity, // TODO: Double Check + T::Hash, + Preimage, T::AccountId>, + >; + + #[pallet::call] + impl Pallet { + /// Register a preimage by paying a deposit proportional to the length of the preimage. + #[pallet::weight(0)] //T::WeightInfo::note_preimage(encoded_proposal.len() as u32))] + pub fn note_preimage(origin: OriginFor, bytes: Vec) -> DispatchResult { + // This is a public call, so we ensure that the origin is some signed account. + let sender = ensure_signed(origin)?; + ensure!(bytes.len() as u32 <= T::MaxSize::get(), Error::::TooLarge); + + let bounded_vec = + BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; + + Ok(()) + } + } +} From 3f033482e6adf56278f6eda66e657dc34ca0a93b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 14:47:14 +0100 Subject: [PATCH 02/14] more --- frame/preimage/src/lib.rs | 76 +++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 22 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 4d32d4d3308d8..10ef7ecd2444e 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -28,11 +28,15 @@ #![cfg_attr(not(feature = "std"), no_std)] -use sp_runtime::DispatchResult; +use sp_runtime::{ + traits::{BadOrigin, Hash, Saturating}, + DispatchResult, +}; use sp_std::{convert::TryFrom, prelude::*}; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ + ensure, pallet_prelude::Get, traits::{Currency, ReservableCurrency}, BoundedVec, @@ -42,11 +46,8 @@ use scale_info::TypeInfo; pub use pallet::*; #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] -pub struct Preimage -where - MaxSize: Get, -{ - preimage: BoundedVec, +pub struct Preimage { + preimage: BoundedVec, deposit: Option<(Balance, AccountId)>, } @@ -71,7 +72,7 @@ pub mod pallet { type ForceOrigin: EnsureOrigin; /// Max size allowed for a preimage. - type MaxSize: Get + TypeInfo + MaxEncodedLen; + type MaxSize: Get; /// The base deposit for placing a preimage on chain. type BaseDeposit: Get>; @@ -88,28 +89,26 @@ pub mod pallet { #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { - /// A sudo just took place. \[result\] - Sudid(DispatchResult), - /// The \[sudoer\] just switched identity; the old key is supplied. - KeyChanged(T::AccountId), - /// A sudo just took place. \[result\] - SudoAsDone(DispatchResult), + /// A preimage has been noted. + Noted { hash: T::Hash }, } #[pallet::error] pub enum Error { /// Preimage is too large to store on-chain. TooLarge, + /// Preimage has already been noted on-chain. + AlreadyNoted, } /// The preimages stored by this pallet. #[pallet::storage] #[pallet::getter(fn key)] - pub(super) type Key = StorageMap< + pub(super) type Preimages = StorageMap< _, Identity, // TODO: Double Check T::Hash, - Preimage, T::AccountId>, + Preimage, BalanceOf, T::AccountId>, >; #[pallet::call] @@ -117,14 +116,47 @@ pub mod pallet { /// Register a preimage by paying a deposit proportional to the length of the preimage. #[pallet::weight(0)] //T::WeightInfo::note_preimage(encoded_proposal.len() as u32))] pub fn note_preimage(origin: OriginFor, bytes: Vec) -> DispatchResult { - // This is a public call, so we ensure that the origin is some signed account. - let sender = ensure_signed(origin)?; - ensure!(bytes.len() as u32 <= T::MaxSize::get(), Error::::TooLarge); - - let bounded_vec = - BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; - + // We accept a signed origin which will pay a deposit, or a root origin where a deposit + // is not taken. + let maybe_sender = Self::ensure_signed_or_root(origin)?; + Self::note_bytes(bytes, maybe_sender)?; Ok(()) } } } + +impl Pallet { + /// Ensure that the origin is either root, or `PalletOwner`. + fn ensure_signed_or_root(origin: T::Origin) -> Result, BadOrigin> { + use frame_system::RawOrigin; + match origin.into() { + Ok(RawOrigin::Root) => Ok(None), + Ok(RawOrigin::Signed(signer)) => Ok(Some(signer)), + _ => Err(BadOrigin), + } + } + + pub fn note_bytes(bytes: Vec, maybe_depositor: Option) -> DispatchResult { + let bounded_vec = + BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; + + let hash = T::Hashing::hash(&bounded_vec); + ensure!(!Preimages::::contains_key(hash), Error::::AlreadyNoted); + + let deposit = if let Some(depositor) = maybe_depositor { + let length = bounded_vec.len() as u32; + let deposit = T::BaseDeposit::get() + .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); + T::Currency::reserve(&depositor, deposit)?; + Some((depositor, deposit)) + } else { + None + }; + + let preimage = Preimage { preimage: bounded_vec, deposit }; + + //Preimages::::insert(hash, preimage); + Self::deposit_event(Event::Noted { hash }); + Ok(()) + } +} From 5c23c8ed633387bd3a9b02ec842a16b609a2ed72 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 15:06:18 +0100 Subject: [PATCH 03/14] fix compile --- frame/preimage/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 10ef7ecd2444e..469c26f3fd960 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -48,7 +48,7 @@ pub use pallet::*; #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] pub struct Preimage { preimage: BoundedVec, - deposit: Option<(Balance, AccountId)>, + deposit: Option<(AccountId, Balance)>, } type BalanceOf = @@ -155,7 +155,7 @@ impl Pallet { let preimage = Preimage { preimage: bounded_vec, deposit }; - //Preimages::::insert(hash, preimage); + Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); Ok(()) } From 0f801118e8ab0bb2c3f0100aa550624e9817d563 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 17:02:12 +0100 Subject: [PATCH 04/14] add clear and request logic --- frame/preimage/src/lib.rs | 130 ++++++++++++++++++++++++++++++-------- 1 file changed, 105 insertions(+), 25 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 469c26f3fd960..56f3e1e81a042 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -28,10 +28,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use sp_runtime::{ - traits::{BadOrigin, Hash, Saturating}, - DispatchResult, -}; +use sp_runtime::traits::{BadOrigin, Hash, Saturating}; use sp_std::{convert::TryFrom, prelude::*}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -39,10 +36,14 @@ use frame_support::{ ensure, pallet_prelude::Get, traits::{Currency, ReservableCurrency}, + weights::Pays, BoundedVec, }; use scale_info::TypeInfo; +use frame_support::pallet_prelude::*; +use frame_system::pallet_prelude::*; + pub use pallet::*; #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] @@ -56,9 +57,7 @@ type BalanceOf = #[frame_support::pallet] pub mod pallet { - use super::{DispatchResult, *}; - use frame_support::pallet_prelude::*; - use frame_system::pallet_prelude::*; + use super::*; #[pallet::config] pub trait Config: frame_system::Config { @@ -68,8 +67,9 @@ pub mod pallet { /// Currency type for this pallet. type Currency: ReservableCurrency; - /// An origin that can bypass deposits to place a preimage on-chain. - type ForceOrigin: EnsureOrigin; + /// An origin that can request a preimage be placed on-chain without a deposit or fee, or + /// manage existing preimages. + type ManagerOrigin: EnsureOrigin; /// Max size allowed for a preimage. type MaxSize: Get; @@ -91,6 +91,10 @@ pub mod pallet { pub enum Event { /// A preimage has been noted. Noted { hash: T::Hash }, + /// A preimage has been requested. + Requested { hash: T::Hash }, + /// A preimage has ben cleared. + Cleared { hash: T::Hash }, } #[pallet::error] @@ -102,8 +106,8 @@ pub mod pallet { } /// The preimages stored by this pallet. + // TODO: Maybe store preimage metadata in its own storage. #[pallet::storage] - #[pallet::getter(fn key)] pub(super) type Preimages = StorageMap< _, Identity, // TODO: Double Check @@ -111,39 +115,68 @@ pub mod pallet { Preimage, BalanceOf, T::AccountId>, >; + /// The preimages stored by this pallet. + #[pallet::storage] + pub(super) type Requests = StorageMap< + _, + Identity, // TODO: Double Check + T::Hash, + (), + >; + #[pallet::call] impl Pallet { /// Register a preimage by paying a deposit proportional to the length of the preimage. - #[pallet::weight(0)] //T::WeightInfo::note_preimage(encoded_proposal.len() as u32))] - pub fn note_preimage(origin: OriginFor, bytes: Vec) -> DispatchResult { + #[pallet::weight(0)] + pub fn note_preimage(origin: OriginFor, bytes: Vec) -> DispatchResultWithPostInfo { // We accept a signed origin which will pay a deposit, or a root origin where a deposit // is not taken. - let maybe_sender = Self::ensure_signed_or_root(origin)?; - Self::note_bytes(bytes, maybe_sender)?; + let maybe_sender = Self::ensure_signed_or_manager(origin)?; + Self::note_bytes(bytes, maybe_sender) + } + + #[pallet::weight(0)] + pub fn request_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { + T::ManagerOrigin::ensure_origin(origin)?; + Self::do_request_preimage(hash); + Ok(()) + } + + #[pallet::weight(0)] + pub fn clear_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { + let maybe_sender = Self::ensure_signed_or_manager(origin)?; + Self::do_clear_preimage(hash, maybe_sender); Ok(()) } } } impl Pallet { - /// Ensure that the origin is either root, or `PalletOwner`. - fn ensure_signed_or_root(origin: T::Origin) -> Result, BadOrigin> { - use frame_system::RawOrigin; - match origin.into() { - Ok(RawOrigin::Root) => Ok(None), - Ok(RawOrigin::Signed(signer)) => Ok(Some(signer)), - _ => Err(BadOrigin), + /// Ensure that the origin is either the `ManagerOrigin` or a signed origin. + fn ensure_signed_or_manager(origin: T::Origin) -> Result, BadOrigin> { + if T::ManagerOrigin::ensure_origin(origin.clone()).is_ok() { + return Ok(None) } + let who = ensure_signed(origin)?; + Ok(Some(who)) } - pub fn note_bytes(bytes: Vec, maybe_depositor: Option) -> DispatchResult { + fn note_bytes( + bytes: Vec, + maybe_depositor: Option, + ) -> DispatchResultWithPostInfo { let bounded_vec = BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; let hash = T::Hashing::hash(&bounded_vec); ensure!(!Preimages::::contains_key(hash), Error::::AlreadyNoted); - let deposit = if let Some(depositor) = maybe_depositor { + // We take a deposit only if there is a provided depositor, and the preimage was not + // previously requested. This also allows the tx to pay no fee. + let deposit = if Requests::::contains_key(hash) { + Requests::::remove(hash); + None + } else if let Some(depositor) = maybe_depositor { let length = bounded_vec.len() as u32; let deposit = T::BaseDeposit::get() .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); @@ -153,10 +186,57 @@ impl Pallet { None }; - let preimage = Preimage { preimage: bounded_vec, deposit }; + let preimage = Preimage { preimage: bounded_vec, deposit: deposit.clone() }; Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); - Ok(()) + + // We don't pay a fee if a deposit wasn't taken. + if deposit.is_none() { + Ok(Pays::No.into()) + } else { + Ok(().into()) + } + } + + // This function will add a hash to the list of requested preimages. + // + // If the preimage already exists before the request is made, the deposit for the preimage is + // returned to the user, and removed from their management. + fn do_request_preimage(hash: T::Hash) { + // Preimage already exists. + if let Some(preimage_metadata) = Preimages::::get(hash) { + if let Some((who, amount)) = preimage_metadata.deposit { + T::Currency::unreserve(&who, amount); + } + } else { + Requests::::insert(hash, ()); + Self::deposit_event(Event::Requested { hash }); + } + } + + // Clear a preimage from the storage of the chain, returning any deposit that may be reserved. + // + // If `maybe_owner` is provided, we verify that it is the correct owner before clearing the + // data. + fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) { + Preimages::::mutate_exists(hash, |maybe_value| { + if let Some(preimage_metadata) = maybe_value { + // If there is a deposit on hold, we return it if there is no `maybe_owner` or + // if the owner matches. + if let Some((who, amount)) = &preimage_metadata.deposit { + if let Some(owner) = maybe_owner { + if &owner != who { + // Ownership check did not pass. Return early without mutating anything. + return + } + } + // At this point, we have done all the authorization needed, and we can simply + // unreserve the deposit. + T::Currency::unreserve(&who, *amount); + } + *maybe_value = None; + } + }); } } From 6fd3ccfc5191142315e85b4e5d604e832ac80190 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 17:15:49 +0100 Subject: [PATCH 05/14] improve some docs --- frame/preimage/src/lib.rs | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 56f3e1e81a042..4a42ca0d71bc2 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -115,7 +115,7 @@ pub mod pallet { Preimage, BalanceOf, T::AccountId>, >; - /// The preimages stored by this pallet. + /// Any outstanding preimage requests. #[pallet::storage] pub(super) type Requests = StorageMap< _, @@ -126,7 +126,10 @@ pub mod pallet { #[pallet::call] impl Pallet { - /// Register a preimage by paying a deposit proportional to the length of the preimage. + /// Register a preimage on-chain. + /// + /// If the preimage was previously requested, no fees or deposits are taken for providing + /// the preimage. Otherwise, a deposit is taken proportional to the size of the preimage. #[pallet::weight(0)] pub fn note_preimage(origin: OriginFor, bytes: Vec) -> DispatchResultWithPostInfo { // We accept a signed origin which will pay a deposit, or a root origin where a deposit @@ -135,6 +138,10 @@ pub mod pallet { Self::note_bytes(bytes, maybe_sender) } + /// Request a preimage be uploaded to the chain without paying any fees or deposits. + /// + /// If the preimage requests has already been provided on-chain, we unreserve any deposit + /// a user may have paid, and take the control of the preimage out of their hands. #[pallet::weight(0)] pub fn request_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { T::ManagerOrigin::ensure_origin(origin)?; @@ -142,6 +149,14 @@ pub mod pallet { Ok(()) } + /// Clear a preimage from the runtime storage. + /// + /// If a signed origin is requesting to clear a preimage, they must be managing that + /// preimage by holding a deposit against it. After the preimage is cleared, any held + /// deposit will be returned to the user. + /// + /// Otherwise, the `ManagerOrigin` can clear any preimage, and will correctly handle + /// deposits. #[pallet::weight(0)] pub fn clear_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { let maybe_sender = Self::ensure_signed_or_manager(origin)?; @@ -161,6 +176,11 @@ impl Pallet { Ok(Some(who)) } + /// Store some preimage on chain. + /// + /// We verify that the preimage is within the bounds of what the pallet supports. + /// + /// If the preimage was requested to be uploaded, then the user pays no deposits or tx fees. fn note_bytes( bytes: Vec, maybe_depositor: Option, @@ -186,17 +206,15 @@ impl Pallet { None }; - let preimage = Preimage { preimage: bounded_vec, deposit: deposit.clone() }; + // We don't pay a fee if a deposit wasn't taken. + let dispatch_result = if deposit.is_none() { Ok(Pays::No.into()) } else { Ok(().into()) }; + + let preimage = Preimage { preimage: bounded_vec, deposit }; Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); - // We don't pay a fee if a deposit wasn't taken. - if deposit.is_none() { - Ok(Pays::No.into()) - } else { - Ok(().into()) - } + dispatch_result } // This function will add a hash to the list of requested preimages. @@ -204,8 +222,8 @@ impl Pallet { // If the preimage already exists before the request is made, the deposit for the preimage is // returned to the user, and removed from their management. fn do_request_preimage(hash: T::Hash) { - // Preimage already exists. if let Some(preimage_metadata) = Preimages::::get(hash) { + // Preimage already exists, so we return the deposit of the user who uploaded it. if let Some((who, amount)) = preimage_metadata.deposit { T::Currency::unreserve(&who, amount); } @@ -236,6 +254,7 @@ impl Pallet { T::Currency::unreserve(&who, *amount); } *maybe_value = None; + Self::deposit_event(Event::Cleared { hash }); } }); } From 8847f9f60d4f5c14c328bf86d642a265b082cebf Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 2 Nov 2021 18:00:38 +0100 Subject: [PATCH 06/14] Add and implement trait --- frame/preimage/src/lib.rs | 44 ++++++++++++++++++++++++++++---- frame/support/src/traits.rs | 4 +-- frame/support/src/traits/misc.rs | 20 +++++++++++++++ 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 4a42ca0d71bc2..ca1687a4ee9d9 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -103,6 +103,8 @@ pub mod pallet { TooLarge, /// Preimage has already been noted on-chain. AlreadyNoted, + /// The user is not authorized to perform this action. + NotAuthorized, } /// The preimages stored by this pallet. @@ -160,7 +162,7 @@ pub mod pallet { #[pallet::weight(0)] pub fn clear_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { let maybe_sender = Self::ensure_signed_or_manager(origin)?; - Self::do_clear_preimage(hash, maybe_sender); + Self::do_clear_preimage(hash, maybe_sender)?; Ok(()) } } @@ -237,8 +239,10 @@ impl Pallet { // // If `maybe_owner` is provided, we verify that it is the correct owner before clearing the // data. - fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) { - Preimages::::mutate_exists(hash, |maybe_value| { + // + // If `maybe_owner` is not provided, this function cannot return an error. + fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) -> DispatchResult { + Preimages::::mutate_exists(hash, |maybe_value| -> DispatchResult { if let Some(preimage_metadata) = maybe_value { // If there is a deposit on hold, we return it if there is no `maybe_owner` or // if the owner matches. @@ -246,7 +250,7 @@ impl Pallet { if let Some(owner) = maybe_owner { if &owner != who { // Ownership check did not pass. Return early without mutating anything. - return + return Err(Error::::NotAuthorized.into()) } } // At this point, we have done all the authorization needed, and we can simply @@ -256,6 +260,36 @@ impl Pallet { *maybe_value = None; Self::deposit_event(Event::Cleared { hash }); } - }); + Ok(()) + }) + } +} + +impl frame_support::traits::PreimageHandler for Pallet { + fn preimage_exists(hash: T::Hash) -> bool { + Preimages::::contains_key(hash) + } + + fn preimage_requested(hash: T::Hash) -> bool { + Requests::::contains_key(hash) + } + + fn get_preimage(hash: T::Hash) -> Option> { + Preimages::::get(hash).map(|preimage| preimage.preimage.to_vec()) + } + + fn note_preimage(bytes: Vec) -> Result<(), ()> { + Self::note_bytes(bytes, None).map_err(|_| ())?; + Ok(()) + } + + fn request_preimage(hash: T::Hash) { + Self::do_request_preimage(hash) + } + + fn clear_preimage(hash: T::Hash) { + // Should never fail if authorization check is skipped. + let res = Self::do_clear_preimage(hash, None); + debug_assert!(res.is_ok(), "do_clear_preimage failed when authorization check was skipped"); } } diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index bb990e25646db..dd03ed75d3ccb 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -52,8 +52,8 @@ mod misc; pub use misc::{ Backing, ConstU32, EnsureInherentsAreFirst, EqualPrivilegeOnly, EstimateCallFee, ExecuteBlock, ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime, IsSubType, IsType, Len, - OffchainWorker, OnKilledAccount, OnNewAccount, PrivilegeCmp, SameOrOther, Time, TryDrop, - UnixTime, WrapperKeepOpaque, WrapperOpaque, + OffchainWorker, OnKilledAccount, OnNewAccount, PreimageHandler, PrivilegeCmp, SameOrOther, + Time, TryDrop, UnixTime, WrapperKeepOpaque, WrapperOpaque, }; mod stored_map; diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 0a3fb045d6c1d..aa8d0c6c73d2a 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -564,6 +564,26 @@ impl TypeInfo for WrapperKeepOpaque { } } +/// A interface for managing preimages to hashes on chain. +/// +/// Note that this API does not assume any underlying user is calling, and thus +/// does not handle any preimage ownership or fees. Other system level logic that +/// uses this API should implement that on their own side. +pub trait PreimageHandler { + /// Returns whether a preimage exists for a given hash. + fn preimage_exists(hash: Hash) -> bool; + /// Returns whether a preimage request exists for a given hash. + fn preimage_requested(hash: Hash) -> bool; + /// Returns the preimage for a given hash. + fn get_preimage(hash: Hash) -> Option>; + /// Store the bytes of a preimage on chain. + fn note_preimage(bytes: Vec) -> Result<(), ()>; + /// Request that someone report a preimage.å + fn request_preimage(hash: Hash); + /// Clear an existing preimage. + fn clear_preimage(hash: Hash); +} + #[cfg(test)] mod test { use super::*; From ccd6bb86b2a6c55eac2f2bb8c788b0160274cf56 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 4 Nov 2021 12:31:15 +0100 Subject: [PATCH 07/14] continuing to improve --- frame/preimage/src/lib.rs | 128 ++++++++++++++++++++++++------- frame/support/src/traits/misc.rs | 6 +- 2 files changed, 104 insertions(+), 30 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index ca1687a4ee9d9..15b7db6488f50 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -28,7 +28,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use sp_runtime::traits::{BadOrigin, Hash, Saturating}; +use sp_runtime::traits::{BadOrigin, Hash, Saturating, One}; use sp_std::{convert::TryFrom, prelude::*}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -46,10 +46,19 @@ use frame_system::pallet_prelude::*; pub use pallet::*; +pub enum RefCount { + User(AccountId, Balance), + System(u32), +} + #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] pub struct Preimage { + // The preimage we are storing. preimage: BoundedVec, + // The user who has a deposit for holding this preimage, if any. deposit: Option<(AccountId, Balance)>, + // A reference counter for how many different resources are using this preimage. + ref_count: u32, } type BalanceOf = @@ -123,7 +132,8 @@ pub mod pallet { _, Identity, // TODO: Double Check T::Hash, - (), + u32, + ValueQuery, >; #[pallet::call] @@ -137,18 +147,12 @@ pub mod pallet { // We accept a signed origin which will pay a deposit, or a root origin where a deposit // is not taken. let maybe_sender = Self::ensure_signed_or_manager(origin)?; - Self::note_bytes(bytes, maybe_sender) - } - - /// Request a preimage be uploaded to the chain without paying any fees or deposits. - /// - /// If the preimage requests has already been provided on-chain, we unreserve any deposit - /// a user may have paid, and take the control of the preimage out of their hands. - #[pallet::weight(0)] - pub fn request_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { - T::ManagerOrigin::ensure_origin(origin)?; - Self::do_request_preimage(hash); - Ok(()) + if let Some(sender) = maybe_sender { + Self::note_user_bytes(bytes, sender) + } else { + Self::note_bytes(bytes)?; + Ok(Pays::No.into()) + } } /// Clear a preimage from the runtime storage. @@ -165,6 +169,25 @@ pub mod pallet { Self::do_clear_preimage(hash, maybe_sender)?; Ok(()) } + + /// Request a preimage be uploaded to the chain without paying any fees or deposits. + /// + /// If the preimage requests has already been provided on-chain, we unreserve any deposit + /// a user may have paid, and take the control of the preimage out of their hands. + #[pallet::weight(0)] + pub fn request_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { + T::ManagerOrigin::ensure_origin(origin)?; + Self::do_request_preimage(hash); + Ok(()) + } + + /// Clear the request for a preimage. + #[pallet::weight(0)] + pub fn clear_request(origin: OriginFor, hash: T::Hash) -> DispatchResult { + T::ManagerOrigin::ensure_origin(origin)?; + Self::do_clear_request(hash); + Ok(()) + } } } @@ -178,14 +201,41 @@ impl Pallet { Ok(Some(who)) } + /// Store some preimage on chain from a trusted source. + /// + /// We verify that the preimage is within the bounds of what the pallet supports. + /// + /// If the preimage is already uploaded, we increase the reference counter, ensuring it is + /// not cleared before all uses of this preimage is complete. + fn note_bytes( + bytes: Vec, + ) -> DispatchResult { + let bounded_vec = + BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; + + let hash = T::Hashing::hash(&bounded_vec); + Preimages::::mutate_exists(hash, |maybe_preimage| { + if let Some(preimage) = maybe_preimage { + preimage.ref_count = preimage.ref_count.saturating_add(One::one()); + } else { + *maybe_preimage = Some( + Preimage { preimage: bounded_vec, deposit: None, ref_count: One::one() } + ) + } + }); + + Self::deposit_event(Event::Noted { hash }); + Ok(()) + } + /// Store some preimage on chain. /// /// We verify that the preimage is within the bounds of what the pallet supports. /// /// If the preimage was requested to be uploaded, then the user pays no deposits or tx fees. - fn note_bytes( + fn note_user_bytes( bytes: Vec, - maybe_depositor: Option, + depositor: T::AccountId, ) -> DispatchResultWithPostInfo { let bounded_vec = BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; @@ -195,23 +245,21 @@ impl Pallet { // We take a deposit only if there is a provided depositor, and the preimage was not // previously requested. This also allows the tx to pay no fee. - let deposit = if Requests::::contains_key(hash) { - Requests::::remove(hash); - None - } else if let Some(depositor) = maybe_depositor { + let requests = Requests::::take(hash); + let (deposit, ref_count) = if requests > 0 { + (None, requests) + } else { let length = bounded_vec.len() as u32; let deposit = T::BaseDeposit::get() .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); T::Currency::reserve(&depositor, deposit)?; - Some((depositor, deposit)) - } else { - None + (Some((depositor, deposit)), One::one()) }; // We don't pay a fee if a deposit wasn't taken. let dispatch_result = if deposit.is_none() { Ok(Pays::No.into()) } else { Ok(().into()) }; - let preimage = Preimage { preimage: bounded_vec, deposit }; + let preimage = Preimage { preimage: bounded_vec, deposit, ref_count }; Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); @@ -224,13 +272,17 @@ impl Pallet { // If the preimage already exists before the request is made, the deposit for the preimage is // returned to the user, and removed from their management. fn do_request_preimage(hash: T::Hash) { - if let Some(preimage_metadata) = Preimages::::get(hash) { + if let Some(mut preimage_metadata) = Preimages::::get(hash) { // Preimage already exists, so we return the deposit of the user who uploaded it. - if let Some((who, amount)) = preimage_metadata.deposit { - T::Currency::unreserve(&who, amount); + if let Some((ref who, amount)) = preimage_metadata.deposit { + T::Currency::unreserve(who, amount); } + // Increase the ref_count + preimage_metadata.ref_count.saturating_inc(); + Preimages::::insert(hash, preimage_metadata); } else { - Requests::::insert(hash, ()); + // Increase the number of requests + Requests::::mutate(hash, |requests| requests.saturating_inc()); Self::deposit_event(Event::Requested { hash }); } } @@ -244,6 +296,13 @@ impl Pallet { fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) -> DispatchResult { Preimages::::mutate_exists(hash, |maybe_value| -> DispatchResult { if let Some(preimage_metadata) = maybe_value { + // If this preimage still has reference counters, decrement, and exit early. + if preimage_metadata.ref_count > 1 { + preimage_metadata.ref_count.saturating_dec(); + return Ok(()) + } + // So we are definitely cleaning up this preimage now... + // If there is a deposit on hold, we return it if there is no `maybe_owner` or // if the owner matches. if let Some((who, amount)) = &preimage_metadata.deposit { @@ -263,9 +322,16 @@ impl Pallet { Ok(()) }) } + + /// Clear a preimage request. + fn do_clear_request(hash: T::Hash) { + Requests::::remove(hash); + } } impl frame_support::traits::PreimageHandler for Pallet { + type MaxSize = T::MaxSize; + fn preimage_exists(hash: T::Hash) -> bool { Preimages::::contains_key(hash) } @@ -279,7 +345,7 @@ impl frame_support::traits::PreimageHandler for Pallet { } fn note_preimage(bytes: Vec) -> Result<(), ()> { - Self::note_bytes(bytes, None).map_err(|_| ())?; + Self::note_bytes(bytes).map_err(|_| ())?; Ok(()) } @@ -292,4 +358,8 @@ impl frame_support::traits::PreimageHandler for Pallet { let res = Self::do_clear_preimage(hash, None); debug_assert!(res.is_ok(), "do_clear_preimage failed when authorization check was skipped"); } + + fn clear_request(hash: T::Hash) { + Self::do_clear_request(hash); + } } diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index aa8d0c6c73d2a..5f6a5eb25ad6a 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -570,6 +570,8 @@ impl TypeInfo for WrapperKeepOpaque { /// does not handle any preimage ownership or fees. Other system level logic that /// uses this API should implement that on their own side. pub trait PreimageHandler { + /// Maximum size of a preimage. + type MaxSize: Get; /// Returns whether a preimage exists for a given hash. fn preimage_exists(hash: Hash) -> bool; /// Returns whether a preimage request exists for a given hash. @@ -578,10 +580,12 @@ pub trait PreimageHandler { fn get_preimage(hash: Hash) -> Option>; /// Store the bytes of a preimage on chain. fn note_preimage(bytes: Vec) -> Result<(), ()>; - /// Request that someone report a preimage.å + /// Request that someone report a preimage. fn request_preimage(hash: Hash); /// Clear an existing preimage. fn clear_preimage(hash: Hash); + /// Clear a preimage request. + fn clear_request(hash: Hash); } #[cfg(test)] From ec2b0f0406633a13024a3307a4f8e01124d15570 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 4 Nov 2021 12:58:26 +0100 Subject: [PATCH 08/14] refcount type --- frame/preimage/src/lib.rs | 107 ++++++++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 40 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 15b7db6488f50..a27ac3dde129e 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -46,19 +46,18 @@ use frame_system::pallet_prelude::*; pub use pallet::*; +#[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] pub enum RefCount { User(AccountId, Balance), System(u32), } #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] -pub struct Preimage { +pub struct Preimage { // The preimage we are storing. preimage: BoundedVec, - // The user who has a deposit for holding this preimage, if any. - deposit: Option<(AccountId, Balance)>, - // A reference counter for how many different resources are using this preimage. - ref_count: u32, + // A reference counter for who depends on this resource. + ref_count: RefCount, } type BalanceOf = @@ -123,7 +122,7 @@ pub mod pallet { _, Identity, // TODO: Double Check T::Hash, - Preimage, BalanceOf, T::AccountId>, + Preimage, T::AccountId, BalanceOf>, >; /// Any outstanding preimage requests. @@ -150,7 +149,7 @@ pub mod pallet { if let Some(sender) = maybe_sender { Self::note_user_bytes(bytes, sender) } else { - Self::note_bytes(bytes)?; + Self::note_system_bytes(bytes)?; Ok(Pays::No.into()) } } @@ -207,7 +206,7 @@ impl Pallet { /// /// If the preimage is already uploaded, we increase the reference counter, ensuring it is /// not cleared before all uses of this preimage is complete. - fn note_bytes( + fn note_system_bytes( bytes: Vec, ) -> DispatchResult { let bounded_vec = @@ -216,10 +215,20 @@ impl Pallet { let hash = T::Hashing::hash(&bounded_vec); Preimages::::mutate_exists(hash, |maybe_preimage| { if let Some(preimage) = maybe_preimage { - preimage.ref_count = preimage.ref_count.saturating_add(One::one()); + // If the preimage already exists, it could be owned by a user. + // We have the system take over control of the preimage. + match &preimage.ref_count { + RefCount::User(who, deposit) => { + T::Currency::unreserve(who, *deposit); + preimage.ref_count = RefCount::System(One::one()); + }, + RefCount::System(mut count) => { + count.saturating_inc(); + } + } } else { *maybe_preimage = Some( - Preimage { preimage: bounded_vec, deposit: None, ref_count: One::one() } + Preimage { preimage: bounded_vec, ref_count: RefCount::System(One::one()) } ) } }); @@ -246,20 +255,23 @@ impl Pallet { // We take a deposit only if there is a provided depositor, and the preimage was not // previously requested. This also allows the tx to pay no fee. let requests = Requests::::take(hash); - let (deposit, ref_count) = if requests > 0 { - (None, requests) + let ref_count = if requests > 0 { + RefCount::System(requests) } else { let length = bounded_vec.len() as u32; let deposit = T::BaseDeposit::get() .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); T::Currency::reserve(&depositor, deposit)?; - (Some((depositor, deposit)), One::one()) + RefCount::User(depositor, deposit) }; - // We don't pay a fee if a deposit wasn't taken. - let dispatch_result = if deposit.is_none() { Ok(Pays::No.into()) } else { Ok(().into()) }; + // We don't pay a fee if it is a system preimage. + let dispatch_result = match ref_count { + RefCount::System(_) => Ok(Pays::No.into()), + RefCount::User(_, _) => Ok(().into()) + }; - let preimage = Preimage { preimage: bounded_vec, deposit, ref_count }; + let preimage = Preimage { preimage: bounded_vec, ref_count }; Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); @@ -273,15 +285,22 @@ impl Pallet { // returned to the user, and removed from their management. fn do_request_preimage(hash: T::Hash) { if let Some(mut preimage_metadata) = Preimages::::get(hash) { - // Preimage already exists, so we return the deposit of the user who uploaded it. - if let Some((ref who, amount)) = preimage_metadata.deposit { - T::Currency::unreserve(who, amount); + match preimage_metadata.ref_count { + RefCount::User(who, deposit) => { + // Preimage already exists and owned by a user. + // We return the deposit and change ownership to the system. + T::Currency::unreserve(&who, deposit); + preimage_metadata.ref_count = RefCount::System(One::one()); + }, + RefCount::System(mut count) => { + // Preimage already exists and is owned by the system. + // We simply increase the number of reference counters. + count.saturating_inc(); + } } - // Increase the ref_count - preimage_metadata.ref_count.saturating_inc(); Preimages::::insert(hash, preimage_metadata); } else { - // Increase the number of requests + // Preimage does not exist yet, so increase the number of requests. Requests::::mutate(hash, |requests| requests.saturating_inc()); Self::deposit_event(Event::Requested { hash }); } @@ -296,26 +315,34 @@ impl Pallet { fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) -> DispatchResult { Preimages::::mutate_exists(hash, |maybe_value| -> DispatchResult { if let Some(preimage_metadata) = maybe_value { - // If this preimage still has reference counters, decrement, and exit early. - if preimage_metadata.ref_count > 1 { - preimage_metadata.ref_count.saturating_dec(); - return Ok(()) - } - // So we are definitely cleaning up this preimage now... - - // If there is a deposit on hold, we return it if there is no `maybe_owner` or - // if the owner matches. - if let Some((who, amount)) = &preimage_metadata.deposit { - if let Some(owner) = maybe_owner { - if &owner != who { - // Ownership check did not pass. Return early without mutating anything. + match &preimage_metadata.ref_count { + RefCount::User(who, deposit) => { + // If there is a deposit on hold, we return it if there is no `maybe_owner` or + // if the owner matches. + if let Some(owner) = maybe_owner { + if &owner != who { + // Ownership check did not pass. Return early without mutating anything. + return Err(Error::::NotAuthorized.into()) + } + } + // At this point, we have done all the authorization needed, and we can simply + // unreserve the deposit. + T::Currency::unreserve(who, *deposit); + }, + RefCount::System(mut count) => { + // A regular user cannot clear a system preimage. + if maybe_owner.is_some() { return Err(Error::::NotAuthorized.into()) } + // If this preimage still has reference counters, decrement, and exit early. + if count > 1 { + count.saturating_dec(); + return Ok(()) + } } - // At this point, we have done all the authorization needed, and we can simply - // unreserve the deposit. - T::Currency::unreserve(&who, *amount); - } + }; + + // If we got this far, we are removing the value. *maybe_value = None; Self::deposit_event(Event::Cleared { hash }); } @@ -345,7 +372,7 @@ impl frame_support::traits::PreimageHandler for Pallet { } fn note_preimage(bytes: Vec) -> Result<(), ()> { - Self::note_bytes(bytes).map_err(|_| ())?; + Self::note_system_bytes(bytes).map_err(|_| ())?; Ok(()) } From cb512ef5e69bc8d283eb1db950c2745e9db9c723 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 4 Nov 2021 13:07:34 +0100 Subject: [PATCH 09/14] infallible system preimage upload --- frame/preimage/src/lib.rs | 43 +++++++++++++++----------------- frame/support/src/traits/misc.rs | 2 +- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index a27ac3dde129e..e9c2049081fb7 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -146,10 +146,17 @@ pub mod pallet { // We accept a signed origin which will pay a deposit, or a root origin where a deposit // is not taken. let maybe_sender = Self::ensure_signed_or_manager(origin)?; + let bounded_vec = + BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; if let Some(sender) = maybe_sender { - Self::note_user_bytes(bytes, sender) + let system_requested = Self::note_user_bytes(bounded_vec, sender)?; + if system_requested { + Ok(Pays::No.into()) + } else { + Ok(().into()) + } } else { - Self::note_system_bytes(bytes)?; + Self::note_system_bytes(bounded_vec); Ok(Pays::No.into()) } } @@ -202,16 +209,11 @@ impl Pallet { /// Store some preimage on chain from a trusted source. /// - /// We verify that the preimage is within the bounds of what the pallet supports. - /// /// If the preimage is already uploaded, we increase the reference counter, ensuring it is /// not cleared before all uses of this preimage is complete. fn note_system_bytes( - bytes: Vec, - ) -> DispatchResult { - let bounded_vec = - BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; - + bounded_vec: BoundedVec, + ) { let hash = T::Hashing::hash(&bounded_vec); Preimages::::mutate_exists(hash, |maybe_preimage| { if let Some(preimage) = maybe_preimage { @@ -234,7 +236,6 @@ impl Pallet { }); Self::deposit_event(Event::Noted { hash }); - Ok(()) } /// Store some preimage on chain. @@ -243,12 +244,9 @@ impl Pallet { /// /// If the preimage was requested to be uploaded, then the user pays no deposits or tx fees. fn note_user_bytes( - bytes: Vec, + bounded_vec: BoundedVec, depositor: T::AccountId, - ) -> DispatchResultWithPostInfo { - let bounded_vec = - BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; - + ) -> Result { let hash = T::Hashing::hash(&bounded_vec); ensure!(!Preimages::::contains_key(hash), Error::::AlreadyNoted); @@ -265,10 +263,10 @@ impl Pallet { RefCount::User(depositor, deposit) }; - // We don't pay a fee if it is a system preimage. - let dispatch_result = match ref_count { - RefCount::System(_) => Ok(Pays::No.into()), - RefCount::User(_, _) => Ok(().into()) + // Return whether this was requested by the system. + let system_request = match ref_count { + RefCount::System(_) => true, + RefCount::User(_, _) => false, }; let preimage = Preimage { preimage: bounded_vec, ref_count }; @@ -276,7 +274,7 @@ impl Pallet { Preimages::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); - dispatch_result + Ok(system_request) } // This function will add a hash to the list of requested preimages. @@ -371,9 +369,8 @@ impl frame_support::traits::PreimageHandler for Pallet { Preimages::::get(hash).map(|preimage| preimage.preimage.to_vec()) } - fn note_preimage(bytes: Vec) -> Result<(), ()> { - Self::note_system_bytes(bytes).map_err(|_| ())?; - Ok(()) + fn note_preimage(bytes: BoundedVec) { + Self::note_system_bytes(bytes) } fn request_preimage(hash: T::Hash) { diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 5f6a5eb25ad6a..6198d1be1273d 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -579,7 +579,7 @@ pub trait PreimageHandler { /// Returns the preimage for a given hash. fn get_preimage(hash: Hash) -> Option>; /// Store the bytes of a preimage on chain. - fn note_preimage(bytes: Vec) -> Result<(), ()>; + fn note_preimage(bytes: crate::BoundedVec); /// Request that someone report a preimage. fn request_preimage(hash: Hash); /// Clear an existing preimage. From 7dc3b43449d47c1e560906fc09c35750cb088a57 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 4 Nov 2021 13:09:07 +0100 Subject: [PATCH 10/14] fmt --- frame/preimage/src/lib.rs | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index e9c2049081fb7..049c9e02b4aee 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -28,7 +28,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use sp_runtime::traits::{BadOrigin, Hash, Saturating, One}; +use sp_runtime::traits::{BadOrigin, Hash, One, Saturating}; use sp_std::{convert::TryFrom, prelude::*}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -46,9 +46,12 @@ use frame_system::pallet_prelude::*; pub use pallet::*; +/// A type to note whether a preimage is owned by a user or the system. #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] pub enum RefCount { + /// This preimage is owned by a user who has the following deposit on hold. User(AccountId, Balance), + /// This preimage is managed by the system with a number of reference counters. System(u32), } @@ -211,9 +214,7 @@ impl Pallet { /// /// If the preimage is already uploaded, we increase the reference counter, ensuring it is /// not cleared before all uses of this preimage is complete. - fn note_system_bytes( - bounded_vec: BoundedVec, - ) { + fn note_system_bytes(bounded_vec: BoundedVec) { let hash = T::Hashing::hash(&bounded_vec); Preimages::::mutate_exists(hash, |maybe_preimage| { if let Some(preimage) = maybe_preimage { @@ -226,12 +227,13 @@ impl Pallet { }, RefCount::System(mut count) => { count.saturating_inc(); - } + }, } } else { - *maybe_preimage = Some( - Preimage { preimage: bounded_vec, ref_count: RefCount::System(One::one()) } - ) + *maybe_preimage = Some(Preimage { + preimage: bounded_vec, + ref_count: RefCount::System(One::one()), + }) } }); @@ -294,7 +296,7 @@ impl Pallet { // Preimage already exists and is owned by the system. // We simply increase the number of reference counters. count.saturating_inc(); - } + }, } Preimages::::insert(hash, preimage_metadata); } else { @@ -315,16 +317,17 @@ impl Pallet { if let Some(preimage_metadata) = maybe_value { match &preimage_metadata.ref_count { RefCount::User(who, deposit) => { - // If there is a deposit on hold, we return it if there is no `maybe_owner` or - // if the owner matches. + // If there is a deposit on hold, we return it if there is no `maybe_owner` + // or if the owner matches. if let Some(owner) = maybe_owner { if &owner != who { - // Ownership check did not pass. Return early without mutating anything. + // Ownership check did not pass. Return early without mutating + // anything. return Err(Error::::NotAuthorized.into()) } } - // At this point, we have done all the authorization needed, and we can simply - // unreserve the deposit. + // At this point, we have done all the authorization needed, and we can + // simply unreserve the deposit. T::Currency::unreserve(who, *deposit); }, RefCount::System(mut count) => { @@ -337,7 +340,7 @@ impl Pallet { count.saturating_dec(); return Ok(()) } - } + }, }; // If we got this far, we are removing the value. From d1621fb4c973d39a23f3d31a380525879684c8d4 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 4 Nov 2021 13:13:22 +0100 Subject: [PATCH 11/14] fix requests --- frame/preimage/src/lib.rs | 12 +++++++++--- frame/support/src/traits/misc.rs | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 049c9e02b4aee..b3c7835aaca7c 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -55,12 +55,13 @@ pub enum RefCount { System(u32), } +/// The preimage metadata. #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] pub struct Preimage { // The preimage we are storing. - preimage: BoundedVec, + pub preimage: BoundedVec, // A reference counter for who depends on this resource. - ref_count: RefCount, + pub ref_count: RefCount, } type BalanceOf = @@ -353,7 +354,12 @@ impl Pallet { /// Clear a preimage request. fn do_clear_request(hash: T::Hash) { - Requests::::remove(hash); + let count = Requests::::get(hash); + if count > 1 { + Requests::::insert(hash, count - 1); + } else { + Requests::::remove(hash); + } } } diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 6198d1be1273d..6486eb387ac09 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -580,10 +580,10 @@ pub trait PreimageHandler { fn get_preimage(hash: Hash) -> Option>; /// Store the bytes of a preimage on chain. fn note_preimage(bytes: crate::BoundedVec); - /// Request that someone report a preimage. - fn request_preimage(hash: Hash); /// Clear an existing preimage. fn clear_preimage(hash: Hash); + /// Request that someone report a preimage. + fn request_preimage(hash: Hash); /// Clear a preimage request. fn clear_request(hash: Hash); } From b9f64dafd0d24157ff02975cb8fdc56caec42db3 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 23 Nov 2021 17:47:25 +0100 Subject: [PATCH 12/14] Make it simple --- frame/preimage/src/lib.rs | 270 +++++++++++++++----------------------- 1 file changed, 103 insertions(+), 167 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index b3c7835aaca7c..8d7e4a134411f 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -28,7 +28,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use sp_runtime::traits::{BadOrigin, Hash, One, Saturating}; +use sp_runtime::traits::{BadOrigin, Hash, Saturating}; use sp_std::{convert::TryFrom, prelude::*}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -48,20 +48,13 @@ pub use pallet::*; /// A type to note whether a preimage is owned by a user or the system. #[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] -pub enum RefCount { - /// This preimage is owned by a user who has the following deposit on hold. - User(AccountId, Balance), - /// This preimage is managed by the system with a number of reference counters. - System(u32), -} - -/// The preimage metadata. -#[derive(Clone, Encode, Decode, TypeInfo, MaxEncodedLen)] -pub struct Preimage { - // The preimage we are storing. - pub preimage: BoundedVec, - // A reference counter for who depends on this resource. - pub ref_count: RefCount, +pub enum RequestStatus { + /// The associated preimage has not yet been requested by the system. The given deposit (if + /// some) is being held until either it becomes requested or the user retracts the primage. + Unrequested(Option<(AccountId, Balance)>), + /// There are a non-zero number of outstanding requests for this hash by this chain. If there + /// is a preimage registered, then it may be removed iff this counter becomes zero. + Requested(u32), } type BalanceOf = @@ -117,26 +110,30 @@ pub mod pallet { AlreadyNoted, /// The user is not authorized to perform this action. NotAuthorized, + /// The preimage cannot be removed since it has not yet been noted. + NotNoted, + /// A preimage may not be removed when there are outstanding requests. + Requested, + /// The preimage request cannot be removed since no outstanding requests exist. + NotRequested, } - /// The preimages stored by this pallet. - // TODO: Maybe store preimage metadata in its own storage. + /// The request status of a given hash. #[pallet::storage] - pub(super) type Preimages = StorageMap< + pub(super) type StatusFor = StorageMap< _, - Identity, // TODO: Double Check + Identity, T::Hash, - Preimage, T::AccountId, BalanceOf>, + RequestStatus>, >; - /// Any outstanding preimage requests. + /// The preimages stored by this pallet. #[pallet::storage] - pub(super) type Requests = StorageMap< + pub(super) type PreimageFor = StorageMap< _, - Identity, // TODO: Double Check + Identity, T::Hash, - u32, - ValueQuery, + BoundedVec, >; #[pallet::call] @@ -152,32 +149,19 @@ pub mod pallet { let maybe_sender = Self::ensure_signed_or_manager(origin)?; let bounded_vec = BoundedVec::::try_from(bytes).map_err(|()| Error::::TooLarge)?; - if let Some(sender) = maybe_sender { - let system_requested = Self::note_user_bytes(bounded_vec, sender)?; - if system_requested { - Ok(Pays::No.into()) - } else { - Ok(().into()) - } - } else { - Self::note_system_bytes(bounded_vec); + let system_requested = Self::note_bytes(bounded_vec, maybe_sender.as_ref())?; + if system_requested || maybe_sender.is_none() { Ok(Pays::No.into()) + } else { + Ok(().into()) } } - /// Clear a preimage from the runtime storage. - /// - /// If a signed origin is requesting to clear a preimage, they must be managing that - /// preimage by holding a deposit against it. After the preimage is cleared, any held - /// deposit will be returned to the user. - /// - /// Otherwise, the `ManagerOrigin` can clear any preimage, and will correctly handle - /// deposits. + /// Clear an unrequested preimage from the runtime storage. #[pallet::weight(0)] pub fn clear_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { let maybe_sender = Self::ensure_signed_or_manager(origin)?; - Self::do_clear_preimage(hash, maybe_sender)?; - Ok(()) + Self::do_clear_preimage(hash, maybe_sender) } /// Request a preimage be uploaded to the chain without paying any fees or deposits. @@ -191,12 +175,13 @@ pub mod pallet { Ok(()) } - /// Clear the request for a preimage. + /// Clear a previously made request for a preimage. + /// + /// NOTE: THIS MUST NOT BE CALLED ON `hash` MORE TIMES THAN `request_preimage`. #[pallet::weight(0)] pub fn clear_request(origin: OriginFor, hash: T::Hash) -> DispatchResult { T::ManagerOrigin::ensure_origin(origin)?; - Self::do_clear_request(hash); - Ok(()) + Self::do_clear_request(hash) } } } @@ -211,73 +196,39 @@ impl Pallet { Ok(Some(who)) } - /// Store some preimage on chain from a trusted source. - /// - /// If the preimage is already uploaded, we increase the reference counter, ensuring it is - /// not cleared before all uses of this preimage is complete. - fn note_system_bytes(bounded_vec: BoundedVec) { - let hash = T::Hashing::hash(&bounded_vec); - Preimages::::mutate_exists(hash, |maybe_preimage| { - if let Some(preimage) = maybe_preimage { - // If the preimage already exists, it could be owned by a user. - // We have the system take over control of the preimage. - match &preimage.ref_count { - RefCount::User(who, deposit) => { - T::Currency::unreserve(who, *deposit); - preimage.ref_count = RefCount::System(One::one()); - }, - RefCount::System(mut count) => { - count.saturating_inc(); - }, - } - } else { - *maybe_preimage = Some(Preimage { - preimage: bounded_vec, - ref_count: RefCount::System(One::one()), - }) - } - }); - - Self::deposit_event(Event::Noted { hash }); - } - /// Store some preimage on chain. /// /// We verify that the preimage is within the bounds of what the pallet supports. /// /// If the preimage was requested to be uploaded, then the user pays no deposits or tx fees. - fn note_user_bytes( - bounded_vec: BoundedVec, - depositor: T::AccountId, + fn note_bytes( + preimage: BoundedVec, + maybe_depositor: Option<&T::AccountId>, ) -> Result { - let hash = T::Hashing::hash(&bounded_vec); - ensure!(!Preimages::::contains_key(hash), Error::::AlreadyNoted); + let hash = T::Hashing::hash(&preimage); + ensure!(!PreimageFor::::contains_key(hash), Error::::AlreadyNoted); // We take a deposit only if there is a provided depositor, and the preimage was not // previously requested. This also allows the tx to pay no fee. - let requests = Requests::::take(hash); - let ref_count = if requests > 0 { - RefCount::System(requests) - } else { - let length = bounded_vec.len() as u32; - let deposit = T::BaseDeposit::get() - .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); - T::Currency::reserve(&depositor, deposit)?; - RefCount::User(depositor, deposit) + let was_requested = match (StatusFor::::get(hash), maybe_depositor) { + (Some(RequestStatus::Requested(_)), _) => true, + (Some(RequestStatus::Unrequested(..)), _) => Err(Error::::AlreadyNoted)?, + (_, None) => true, + (None, Some(depositor)) => { + let length = preimage.len() as u32; + let deposit = T::BaseDeposit::get() + .saturating_add(T::ByteDeposit::get().saturating_mul(length.into())); + T::Currency::reserve(depositor, deposit)?; + let status = RequestStatus::Unrequested(Some((depositor.clone(), deposit))); + StatusFor::::insert(hash, status); + false + }, }; - // Return whether this was requested by the system. - let system_request = match ref_count { - RefCount::System(_) => true, - RefCount::User(_, _) => false, - }; - - let preimage = Preimage { preimage: bounded_vec, ref_count }; - - Preimages::::insert(hash, preimage); + PreimageFor::::insert(hash, preimage); Self::deposit_event(Event::Noted { hash }); - Ok(system_request) + Ok(was_requested) } // This function will add a hash to the list of requested preimages. @@ -285,24 +236,20 @@ impl Pallet { // If the preimage already exists before the request is made, the deposit for the preimage is // returned to the user, and removed from their management. fn do_request_preimage(hash: T::Hash) { - if let Some(mut preimage_metadata) = Preimages::::get(hash) { - match preimage_metadata.ref_count { - RefCount::User(who, deposit) => { - // Preimage already exists and owned by a user. - // We return the deposit and change ownership to the system. - T::Currency::unreserve(&who, deposit); - preimage_metadata.ref_count = RefCount::System(One::one()); - }, - RefCount::System(mut count) => { - // Preimage already exists and is owned by the system. - // We simply increase the number of reference counters. - count.saturating_inc(); - }, + let count = StatusFor::::get(hash).map_or(1, |x| match x { + RequestStatus::Requested(mut count) => { + count.saturating_inc(); + count } - Preimages::::insert(hash, preimage_metadata); - } else { - // Preimage does not exist yet, so increase the number of requests. - Requests::::mutate(hash, |requests| requests.saturating_inc()); + RequestStatus::Unrequested(None) => 1, + RequestStatus::Unrequested(Some((owner, deposit))) => { + // Return the deposit - the preimage now has outstanding requests. + T::Currency::unreserve(&owner, deposit); + 1 + }, + }); + StatusFor::::insert(&hash, RequestStatus::Requested(count)); + if count == 1 { Self::deposit_event(Event::Requested { hash }); } } @@ -313,53 +260,39 @@ impl Pallet { // data. // // If `maybe_owner` is not provided, this function cannot return an error. - fn do_clear_preimage(hash: T::Hash, maybe_owner: Option) -> DispatchResult { - Preimages::::mutate_exists(hash, |maybe_value| -> DispatchResult { - if let Some(preimage_metadata) = maybe_value { - match &preimage_metadata.ref_count { - RefCount::User(who, deposit) => { - // If there is a deposit on hold, we return it if there is no `maybe_owner` - // or if the owner matches. - if let Some(owner) = maybe_owner { - if &owner != who { - // Ownership check did not pass. Return early without mutating - // anything. - return Err(Error::::NotAuthorized.into()) - } - } - // At this point, we have done all the authorization needed, and we can - // simply unreserve the deposit. - T::Currency::unreserve(who, *deposit); - }, - RefCount::System(mut count) => { - // A regular user cannot clear a system preimage. - if maybe_owner.is_some() { - return Err(Error::::NotAuthorized.into()) - } - // If this preimage still has reference counters, decrement, and exit early. - if count > 1 { - count.saturating_dec(); - return Ok(()) - } - }, - }; - - // If we got this far, we are removing the value. - *maybe_value = None; - Self::deposit_event(Event::Cleared { hash }); - } - Ok(()) - }) + fn do_clear_preimage(hash: T::Hash, maybe_check_owner: Option) -> DispatchResult { + match StatusFor::::get(&hash).ok_or(Error::::NotNoted)? { + RequestStatus::Unrequested(Some((owner, deposit))) => { + ensure!(maybe_check_owner.map_or(true, |c| &c == &owner), Error::::NotAuthorized); + T::Currency::unreserve(&owner, deposit); + }, + RequestStatus::Unrequested(None) => { + ensure!(maybe_check_owner.is_none(), Error::::NotAuthorized); + }, + RequestStatus::Requested(_) => Err(Error::::Requested)?, + } + StatusFor::::remove(&hash); + PreimageFor::::remove(&hash); + Self::deposit_event(Event::Cleared { hash }); + Ok(()) } /// Clear a preimage request. - fn do_clear_request(hash: T::Hash) { - let count = Requests::::get(hash); - if count > 1 { - Requests::::insert(hash, count - 1); - } else { - Requests::::remove(hash); + fn do_clear_request(hash: T::Hash) -> DispatchResult { + match StatusFor::::get(hash).ok_or(Error::::NotRequested)? { + RequestStatus::Requested(mut count) if count > 1 => { + count.saturating_dec(); + StatusFor::::insert(&hash, RequestStatus::Requested(count)); + } + RequestStatus::Requested(count) => { + debug_assert!(count == 1, "preimage request counter at zero?"); + PreimageFor::::remove(&hash); + StatusFor::::remove(&hash); + Self::deposit_event(Event::Cleared { hash }); + } + RequestStatus::Unrequested(_) => Err(Error::::NotRequested)?, } + Ok(()) } } @@ -367,19 +300,21 @@ impl frame_support::traits::PreimageHandler for Pallet { type MaxSize = T::MaxSize; fn preimage_exists(hash: T::Hash) -> bool { - Preimages::::contains_key(hash) + PreimageFor::::contains_key(hash) } fn preimage_requested(hash: T::Hash) -> bool { - Requests::::contains_key(hash) + matches!(StatusFor::::get(hash), Some(RequestStatus::Requested(..))) } fn get_preimage(hash: T::Hash) -> Option> { - Preimages::::get(hash).map(|preimage| preimage.preimage.to_vec()) + PreimageFor::::get(hash).map(|preimage| preimage.to_vec()) } fn note_preimage(bytes: BoundedVec) { - Self::note_system_bytes(bytes) + // We don't really care if this fails, since that's only the case if someone else has + // already noted it. + let _ = Self::note_bytes(bytes, None); } fn request_preimage(hash: T::Hash) { @@ -389,10 +324,11 @@ impl frame_support::traits::PreimageHandler for Pallet { fn clear_preimage(hash: T::Hash) { // Should never fail if authorization check is skipped. let res = Self::do_clear_preimage(hash, None); - debug_assert!(res.is_ok(), "do_clear_preimage failed when authorization check was skipped"); + debug_assert!(res.is_ok(), "do_clear_preimage failed - request outstanding?"); } fn clear_request(hash: T::Hash) { - Self::do_clear_request(hash); + let res = Self::do_clear_request(hash); + debug_assert!(res.is_ok(), "do_clear_request failed - counter underflow?"); } } From 23485e7fd65d31da64ad83c23ae66c1f248946c0 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 23 Nov 2021 18:01:30 +0100 Subject: [PATCH 13/14] Make it simple --- frame/preimage/src/lib.rs | 39 ++++++++++++++------------ frame/support/src/traits/misc.rs | 41 +++++++++++++++++++--------- frame/support/src/traits/schedule.rs | 1 + 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index 8d7e4a134411f..d0970a5ef3368 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -40,6 +40,7 @@ use frame_support::{ BoundedVec, }; use scale_info::TypeInfo; +use frame_support::traits::{PreimageProvider, PreimageRecipient}; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; @@ -159,9 +160,9 @@ pub mod pallet { /// Clear an unrequested preimage from the runtime storage. #[pallet::weight(0)] - pub fn clear_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { + pub fn unnote_preimage(origin: OriginFor, hash: T::Hash) -> DispatchResult { let maybe_sender = Self::ensure_signed_or_manager(origin)?; - Self::do_clear_preimage(hash, maybe_sender) + Self::unnote_preimage(hash, maybe_sender) } /// Request a preimage be uploaded to the chain without paying any fees or deposits. @@ -260,7 +261,7 @@ impl Pallet { // data. // // If `maybe_owner` is not provided, this function cannot return an error. - fn do_clear_preimage(hash: T::Hash, maybe_check_owner: Option) -> DispatchResult { + fn unnote_preimage(hash: T::Hash, maybe_check_owner: Option) -> DispatchResult { match StatusFor::::get(&hash).ok_or(Error::::NotNoted)? { RequestStatus::Unrequested(Some((owner, deposit))) => { ensure!(maybe_check_owner.map_or(true, |c| &c == &owner), Error::::NotAuthorized); @@ -296,9 +297,7 @@ impl Pallet { } } -impl frame_support::traits::PreimageHandler for Pallet { - type MaxSize = T::MaxSize; - +impl PreimageProvider for Pallet { fn preimage_exists(hash: T::Hash) -> bool { PreimageFor::::contains_key(hash) } @@ -311,24 +310,28 @@ impl frame_support::traits::PreimageHandler for Pallet { PreimageFor::::get(hash).map(|preimage| preimage.to_vec()) } - fn note_preimage(bytes: BoundedVec) { - // We don't really care if this fails, since that's only the case if someone else has - // already noted it. - let _ = Self::note_bytes(bytes, None); - } - fn request_preimage(hash: T::Hash) { Self::do_request_preimage(hash) } - fn clear_preimage(hash: T::Hash) { - // Should never fail if authorization check is skipped. - let res = Self::do_clear_preimage(hash, None); - debug_assert!(res.is_ok(), "do_clear_preimage failed - request outstanding?"); - } - fn clear_request(hash: T::Hash) { let res = Self::do_clear_request(hash); debug_assert!(res.is_ok(), "do_clear_request failed - counter underflow?"); } } + +impl PreimageRecipient for Pallet { + type MaxSize = T::MaxSize; + + fn note_preimage(bytes: BoundedVec) { + // We don't really care if this fails, since that's only the case if someone else has + // already noted it. + let _ = Self::note_bytes(bytes, None); + } + + fn unnote_preimage(hash: T::Hash) { + // Should never fail if authorization check is skipped. + let res = Self::unnote_preimage(hash, None); + debug_assert!(res.is_ok(), "unnote_preimage failed - request outstanding?"); + } +} diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 6486eb387ac09..1beb29eeafa72 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -564,28 +564,43 @@ impl TypeInfo for WrapperKeepOpaque { } } +/// A interface for looking up preimages from their hash on chain. +pub trait PreimageProvider { + /// Returns whether a preimage exists for a given hash. + /// + /// A value of `true` implies that `get_preimage` is `Some`. + fn preimage_exists(hash: Hash) -> bool; + + /// Returns the preimage for a given hash. + fn get_preimage(hash: Hash) -> Option>; + + /// Returns whether a preimage request exists for a given hash. + fn preimage_requested(hash: Hash) -> bool; + + /// Request that someone report a preimage. Providers use this to optimise the economics for + /// preimage reporting. + fn request_preimage(hash: Hash); + + /// Clear a preimage request. + fn clear_request(hash: Hash); +} + /// A interface for managing preimages to hashes on chain. /// /// Note that this API does not assume any underlying user is calling, and thus /// does not handle any preimage ownership or fees. Other system level logic that /// uses this API should implement that on their own side. -pub trait PreimageHandler { +pub trait PreimageRecipient { /// Maximum size of a preimage. type MaxSize: Get; - /// Returns whether a preimage exists for a given hash. - fn preimage_exists(hash: Hash) -> bool; - /// Returns whether a preimage request exists for a given hash. - fn preimage_requested(hash: Hash) -> bool; - /// Returns the preimage for a given hash. - fn get_preimage(hash: Hash) -> Option>; + /// Store the bytes of a preimage on chain. fn note_preimage(bytes: crate::BoundedVec); - /// Clear an existing preimage. - fn clear_preimage(hash: Hash); - /// Request that someone report a preimage. - fn request_preimage(hash: Hash); - /// Clear a preimage request. - fn clear_request(hash: Hash); + + /// Clear a previously noted preimage. This is infallible and should be treated more like a + /// hint - if it was not previously noted or if it is now requested, then this will not do + /// anything. + fn unnote_preimage(hash: Hash); } #[cfg(test)] diff --git a/frame/support/src/traits/schedule.rs b/frame/support/src/traits/schedule.rs index 19f50a93c0681..fad0484bb2b7e 100644 --- a/frame/support/src/traits/schedule.rs +++ b/frame/support/src/traits/schedule.rs @@ -20,6 +20,7 @@ use codec::{Codec, Decode, Encode, EncodeLike}; use scale_info::TypeInfo; use sp_runtime::{DispatchError, RuntimeDebug}; +use frame_support::traits::PreimageProvider; use sp_std::{fmt::Debug, prelude::*}; /// Information relating to the period of a scheduled task. First item is the length of the From 6679e5be233d96bda37ac2190ba7ac1a2ef4b4ff Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 23 Nov 2021 18:55:48 +0100 Subject: [PATCH 14/14] Formatting --- client/consensus/babe/src/verification.rs | 4 +-- .../src/protocol/notifications/behaviour.rs | 4 +-- client/network/src/protocol/sync/blocks.rs | 2 +- client/network/src/service/tests.rs | 2 +- client/network/src/transactions.rs | 4 +-- .../election-provider-multi-phase/src/lib.rs | 2 +- frame/preimage/src/lib.rs | 30 ++++++++----------- .../src/pallet/parse/pallet_struct.rs | 4 +-- frame/support/src/traits/schedule.rs | 2 +- 9 files changed, 24 insertions(+), 30 deletions(-) diff --git a/client/consensus/babe/src/verification.rs b/client/consensus/babe/src/verification.rs index af118312dd07c..1554fa6de31be 100644 --- a/client/consensus/babe/src/verification.rs +++ b/client/consensus/babe/src/verification.rs @@ -114,7 +114,7 @@ where ); check_secondary_plain_header::(pre_hash, secondary, sig, &epoch)?; - } + }, PreDigest::SecondaryVRF(secondary) if epoch.config.allowed_slots.is_secondary_vrf_slots_allowed() => { @@ -125,7 +125,7 @@ where ); check_secondary_vrf_header::(pre_hash, secondary, sig, &epoch)?; - } + }, _ => return Err(babe_err(Error::SecondarySlotAssignmentsDisabled)), } diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 01138e3207570..f66f1fbe9e95a 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -712,7 +712,7 @@ impl Notifications { timer: delay_id, timer_deadline: *backoff, }; - } + }, // Disabled => Enabled PeerState::Disabled { mut connections, backoff_until } => { @@ -2085,7 +2085,7 @@ impl NetworkBehaviour for Notifications { .boxed(), ); } - } + }, // We intentionally never remove elements from `delays`, and it may // thus contain obsolete entries. This is a normal situation. diff --git a/client/network/src/protocol/sync/blocks.rs b/client/network/src/protocol/sync/blocks.rs index 30ba7ffafeffc..ce4535dc0b45f 100644 --- a/client/network/src/protocol/sync/blocks.rs +++ b/client/network/src/protocol/sync/blocks.rs @@ -203,7 +203,7 @@ impl BlockCollection { { *downloading -= 1; false - } + }, Some(&mut BlockRangeState::Downloading { .. }) => true, _ => false, }; diff --git a/client/network/src/service/tests.rs b/client/network/src/service/tests.rs index 69b172d07edfe..87e481dc87f2d 100644 --- a/client/network/src/service/tests.rs +++ b/client/network/src/service/tests.rs @@ -530,7 +530,7 @@ fn fallback_name_working() { { assert_eq!(negotiated_fallback, Some(PROTOCOL_NAME)); break - } + }, _ => {}, }; } diff --git a/client/network/src/transactions.rs b/client/network/src/transactions.rs index 99350f603a375..6d190651160f0 100644 --- a/client/network/src/transactions.rs +++ b/client/network/src/transactions.rs @@ -336,13 +336,13 @@ impl TransactionsHandler { }, ); debug_assert!(_was_in.is_none()); - } + }, Event::NotificationStreamClosed { remote, protocol } if protocol == self.protocol_name => { let _peer = self.peers.remove(&remote); debug_assert!(_peer.is_some()); - } + }, Event::NotificationsReceived { remote, messages } => { for (protocol, message) in messages { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index a7863fafa7747..ab8b1523f0509 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -772,7 +772,7 @@ pub mod pallet { Self::on_initialize_open_unsigned(enabled, now); T::WeightInfo::on_initialize_open_unsigned() } - } + }, _ => T::WeightInfo::on_initialize_nothing(), } } diff --git a/frame/preimage/src/lib.rs b/frame/preimage/src/lib.rs index d0970a5ef3368..aa263ee269972 100644 --- a/frame/preimage/src/lib.rs +++ b/frame/preimage/src/lib.rs @@ -35,12 +35,11 @@ use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ ensure, pallet_prelude::Get, - traits::{Currency, ReservableCurrency}, + traits::{Currency, PreimageProvider, PreimageRecipient, ReservableCurrency}, weights::Pays, BoundedVec, }; use scale_info::TypeInfo; -use frame_support::traits::{PreimageProvider, PreimageRecipient}; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; @@ -121,21 +120,13 @@ pub mod pallet { /// The request status of a given hash. #[pallet::storage] - pub(super) type StatusFor = StorageMap< - _, - Identity, - T::Hash, - RequestStatus>, - >; + pub(super) type StatusFor = + StorageMap<_, Identity, T::Hash, RequestStatus>>; /// The preimages stored by this pallet. #[pallet::storage] - pub(super) type PreimageFor = StorageMap< - _, - Identity, - T::Hash, - BoundedVec, - >; + pub(super) type PreimageFor = + StorageMap<_, Identity, T::Hash, BoundedVec>; #[pallet::call] impl Pallet { @@ -241,7 +232,7 @@ impl Pallet { RequestStatus::Requested(mut count) => { count.saturating_inc(); count - } + }, RequestStatus::Unrequested(None) => 1, RequestStatus::Unrequested(Some((owner, deposit))) => { // Return the deposit - the preimage now has outstanding requests. @@ -264,7 +255,10 @@ impl Pallet { fn unnote_preimage(hash: T::Hash, maybe_check_owner: Option) -> DispatchResult { match StatusFor::::get(&hash).ok_or(Error::::NotNoted)? { RequestStatus::Unrequested(Some((owner, deposit))) => { - ensure!(maybe_check_owner.map_or(true, |c| &c == &owner), Error::::NotAuthorized); + ensure!( + maybe_check_owner.map_or(true, |c| &c == &owner), + Error::::NotAuthorized + ); T::Currency::unreserve(&owner, deposit); }, RequestStatus::Unrequested(None) => { @@ -284,13 +278,13 @@ impl Pallet { RequestStatus::Requested(mut count) if count > 1 => { count.saturating_dec(); StatusFor::::insert(&hash, RequestStatus::Requested(count)); - } + }, RequestStatus::Requested(count) => { debug_assert!(count == 1, "preimage request counter at zero?"); PreimageFor::::remove(&hash); StatusFor::::remove(&hash); Self::deposit_event(Event::Cleared { hash }); - } + }, RequestStatus::Unrequested(_) => Err(Error::::NotRequested)?, } Ok(()) diff --git a/frame/support/procedural/src/pallet/parse/pallet_struct.rs b/frame/support/procedural/src/pallet/parse/pallet_struct.rs index 278f46e13818e..c528faf669ee3 100644 --- a/frame/support/procedural/src/pallet/parse/pallet_struct.rs +++ b/frame/support/procedural/src/pallet/parse/pallet_struct.rs @@ -130,12 +130,12 @@ impl PalletStructDef { if generate_storage_info.is_none() => { generate_storage_info = Some(span); - } + }, PalletStructAttr::StorageVersion { storage_version, .. } if storage_version_found.is_none() => { storage_version_found = Some(storage_version); - } + }, attr => { let msg = "Unexpected duplicated attribute"; return Err(syn::Error::new(attr.span(), msg)) diff --git a/frame/support/src/traits/schedule.rs b/frame/support/src/traits/schedule.rs index fad0484bb2b7e..314e480e1332e 100644 --- a/frame/support/src/traits/schedule.rs +++ b/frame/support/src/traits/schedule.rs @@ -18,9 +18,9 @@ //! Traits and associated utilities for scheduling dispatchables in FRAME. use codec::{Codec, Decode, Encode, EncodeLike}; +use frame_support::traits::PreimageProvider; use scale_info::TypeInfo; use sp_runtime::{DispatchError, RuntimeDebug}; -use frame_support::traits::PreimageProvider; use sp_std::{fmt::Debug, prelude::*}; /// Information relating to the period of a scheduled task. First item is the length of the