From 0073c306ed4c0fecb5e9488cfb2c8946717f7cbf Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Fri, 26 May 2023 11:06:54 +0400 Subject: [PATCH 01/25] [frame/im-online] remove `external_addresses` from heartbeats Users should use DHT for discovering new nodes. The reason for adding external addresses was unstable work of authority discovery (see https://github.com/paritytech/substrate/issues/2719), which is now stable. Hence we can safely remove `external_addresses`. Refs https://github.com/paritytech/polkadot/issues/7181 --- client/offchain/src/api.rs | 32 +++---------- frame/im-online/src/benchmarking.rs | 16 ++----- frame/im-online/src/lib.rs | 60 +++++-------------------- frame/im-online/src/tests.rs | 5 +-- frame/im-online/src/weights.rs | 20 +++------ primitives/core/src/offchain/mod.rs | 2 - primitives/core/src/offchain/testing.rs | 2 +- 7 files changed, 26 insertions(+), 111 deletions(-) diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index a15f03bab6f84..06d35d9766513 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -16,7 +16,7 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see <https://www.gnu.org/licenses/>. -use std::{collections::HashSet, str::FromStr, sync::Arc, thread::sleep}; +use std::{collections::HashSet, sync::Arc, thread::sleep}; use crate::NetworkProvider; use codec::{Decode, Encode}; @@ -25,8 +25,8 @@ pub use http::SharedClient; use libp2p::{Multiaddr, PeerId}; use sp_core::{ offchain::{ - self, HttpError, HttpRequestId, HttpRequestStatus, OffchainStorage, OpaqueMultiaddr, - OpaqueNetworkState, StorageKind, Timestamp, + self, HttpError, HttpRequestId, HttpRequestStatus, OffchainStorage, OpaqueNetworkState, + StorageKind, Timestamp, }, OpaquePeerId, }; @@ -252,16 +252,7 @@ impl From<NetworkState> for OpaqueNetworkState { let enc = Encode::encode(&state.peer_id.to_bytes()); let peer_id = OpaquePeerId::new(enc); - let external_addresses: Vec<OpaqueMultiaddr> = state - .external_addresses - .iter() - .map(|multiaddr| { - let e = Encode::encode(&multiaddr.to_string()); - OpaqueMultiaddr::new(e) - }) - .collect(); - - OpaqueNetworkState { peer_id, external_addresses } + OpaqueNetworkState { peer_id } } } @@ -274,20 +265,7 @@ impl TryFrom<OpaqueNetworkState> for NetworkState { let bytes: Vec<u8> = Decode::decode(&mut &inner_vec[..]).map_err(|_| ())?; let peer_id = PeerId::from_bytes(&bytes).map_err(|_| ())?; - let external_addresses: Result<Vec<Multiaddr>, Self::Error> = state - .external_addresses - .iter() - .map(|enc_multiaddr| -> Result<Multiaddr, Self::Error> { - let inner_vec = &enc_multiaddr.0; - let bytes = <Vec<u8>>::decode(&mut &inner_vec[..]).map_err(|_| ())?; - let multiaddr_str = String::from_utf8(bytes).map_err(|_| ())?; - let multiaddr = Multiaddr::from_str(&multiaddr_str).map_err(|_| ())?; - Ok(multiaddr) - }) - .collect(); - let external_addresses = external_addresses?; - - Ok(NetworkState { peer_id, external_addresses }) + Ok(NetworkState { peer_id, external_addresses: vec![] }) } } diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index f90dcd53b3ef9..adbbdad5f4825 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -33,11 +33,9 @@ use sp_runtime::{ use crate::Pallet as ImOnline; const MAX_KEYS: u32 = 1000; -const MAX_EXTERNAL_ADDRESSES: u32 = 100; pub fn create_heartbeat<T: Config>( k: u32, - e: u32, ) -> Result< (crate::Heartbeat<T::BlockNumber>, <T::AuthorityId as RuntimeAppPublic>::Signature), &'static str, @@ -50,10 +48,7 @@ pub fn create_heartbeat<T: Config>( .map_err(|()| "More than the maximum number of keys provided")?; Keys::<T>::put(bounded_keys); - let network_state = OpaqueNetworkState { - peer_id: OpaquePeerId::default(), - external_addresses: vec![OpaqueMultiaddr::new(vec![0; 32]); e as usize], - }; + let network_state = OpaqueNetworkState { peer_id: OpaquePeerId::default() }; let input_heartbeat = Heartbeat { block_number: T::BlockNumber::zero(), network_state, @@ -73,15 +68,13 @@ benchmarks! { #[extra] heartbeat { let k in 1 .. MAX_KEYS; - let e in 1 .. MAX_EXTERNAL_ADDRESSES; - let (input_heartbeat, signature) = create_heartbeat::<T>(k, e)?; + let (input_heartbeat, signature) = create_heartbeat::<T>(k)?; }: _(RawOrigin::None, input_heartbeat, signature) #[extra] validate_unsigned { let k in 1 .. MAX_KEYS; - let e in 1 .. MAX_EXTERNAL_ADDRESSES; - let (input_heartbeat, signature) = create_heartbeat::<T>(k, e)?; + let (input_heartbeat, signature) = create_heartbeat::<T>(k)?; let call = Call::heartbeat { heartbeat: input_heartbeat, signature }; }: { ImOnline::<T>::validate_unsigned(TransactionSource::InBlock, &call) @@ -90,8 +83,7 @@ benchmarks! { validate_unsigned_and_then_heartbeat { let k in 1 .. MAX_KEYS; - let e in 1 .. MAX_EXTERNAL_ADDRESSES; - let (input_heartbeat, signature) = create_heartbeat::<T>(k, e)?; + let (input_heartbeat, signature) = create_heartbeat::<T>(k)?; let call = Call::heartbeat { heartbeat: input_heartbeat, signature }; let call_enc = call.encode(); }: { diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index dd3809f8e9a70..bee341d2484b3 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -235,27 +235,18 @@ where /// A type that is the same as [`OpaqueNetworkState`] but with [`Vec`] replaced with /// [`WeakBoundedVec<Limit>`] where Limit is the respective size limit /// `PeerIdEncodingLimit` represents the size limit of the encoding of `PeerId` -/// `MultiAddrEncodingLimit` represents the size limit of the encoding of `MultiAddr` -/// `AddressesLimit` represents the size limit of the vector of peers connected #[derive(Clone, Eq, PartialEq, Encode, Decode, MaxEncodedLen, TypeInfo)] #[codec(mel_bound())] -#[scale_info(skip_type_params(PeerIdEncodingLimit, MultiAddrEncodingLimit, AddressesLimit))] -pub struct BoundedOpaqueNetworkState<PeerIdEncodingLimit, MultiAddrEncodingLimit, AddressesLimit> +#[scale_info(skip_type_params(PeerIdEncodingLimit))] +pub struct BoundedOpaqueNetworkState<PeerIdEncodingLimit> where PeerIdEncodingLimit: Get<u32>, - MultiAddrEncodingLimit: Get<u32>, - AddressesLimit: Get<u32>, { /// PeerId of the local node in SCALE encoded. pub peer_id: WeakBoundedVec<u8, PeerIdEncodingLimit>, - /// List of addresses the node knows it can be reached as. - pub external_addresses: - WeakBoundedVec<WeakBoundedVec<u8, MultiAddrEncodingLimit>, AddressesLimit>, } -impl<PeerIdEncodingLimit: Get<u32>, MultiAddrEncodingLimit: Get<u32>, AddressesLimit: Get<u32>> - BoundedOpaqueNetworkState<PeerIdEncodingLimit, MultiAddrEncodingLimit, AddressesLimit> -{ +impl<PeerIdEncodingLimit: Get<u32>> BoundedOpaqueNetworkState<PeerIdEncodingLimit> { fn force_from(ons: &OpaqueNetworkState) -> Self { let peer_id = WeakBoundedVec::<_, PeerIdEncodingLimit>::force_from( ons.peer_id.0.clone(), @@ -265,28 +256,7 @@ impl<PeerIdEncodingLimit: Get<u32>, MultiAddrEncodingLimit: Get<u32>, AddressesL adjustment may be needed.", ), ); - - let external_addresses = WeakBoundedVec::<_, AddressesLimit>::force_from( - ons.external_addresses - .iter() - .map(|x| { - WeakBoundedVec::<_, MultiAddrEncodingLimit>::force_from( - x.0.clone(), - Some( - "Warning: The size of the encoding of MultiAddr \ - is bigger than expected. A runtime configuration \ - adjustment may be needed.", - ), - ) - }) - .collect(), - Some( - "Warning: The network has more peers than expected \ - A runtime configuration adjustment may be needed.", - ), - ); - - Self { peer_id, external_addresses } + Self { peer_id } } } @@ -418,13 +388,7 @@ pub mod pallet { SessionIndex, Twox64Concat, AuthIndex, - WrapperOpaque< - BoundedOpaqueNetworkState< - T::MaxPeerDataEncodingSize, - T::MaxPeerDataEncodingSize, - T::MaxPeerInHeartbeats, - >, - >, + WrapperOpaque<BoundedOpaqueNetworkState<T::MaxPeerDataEncodingSize>>, >; /// For each session index, we keep a mapping of `ValidatorId<T>` to the @@ -457,16 +421,13 @@ pub mod pallet { #[pallet::call] impl<T: Config> Pallet<T> { /// ## Complexity: - /// - `O(K + E)` where K is length of `Keys` (heartbeat.validators_len) and E is length of - /// `heartbeat.network_state.external_address` + /// - `O(K)` where K is length of `Keys` (heartbeat.validators_len) /// - `O(K)`: decoding of length `K` - /// - `O(E)`: decoding/encoding of length `E` // NOTE: the weight includes the cost of validate_unsigned as it is part of the cost to // import block with such an extrinsic. #[pallet::call_index(0)] #[pallet::weight(<T as Config>::WeightInfo::validate_unsigned_and_then_heartbeat( heartbeat.validators_len as u32, - heartbeat.network_state.external_addresses.len() as u32, ))] pub fn heartbeat( origin: OriginFor<T>, @@ -485,11 +446,10 @@ pub mod pallet { if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - let network_state_bounded = BoundedOpaqueNetworkState::< - T::MaxPeerDataEncodingSize, - T::MaxPeerDataEncodingSize, - T::MaxPeerInHeartbeats, - >::force_from(&heartbeat.network_state); + let network_state_bounded = + BoundedOpaqueNetworkState::<T::MaxPeerDataEncodingSize>::force_from( + &heartbeat.network_state, + ); ReceivedHeartbeats::<T>::insert( ¤t_session, &heartbeat.authority_index, diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index 80320959c53bd..5fbb0a34d4e2e 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -125,10 +125,7 @@ fn heartbeat( let heartbeat = Heartbeat { block_number, - network_state: OpaqueNetworkState { - peer_id: OpaquePeerId(vec![1]), - external_addresses: vec![], - }, + network_state: OpaqueNetworkState { peer_id: OpaquePeerId(vec![1]) }, session_index, authority_index, validators_len: validators.len() as u32, diff --git a/frame/im-online/src/weights.rs b/frame/im-online/src/weights.rs index 64c1eb5f3a9b0..24301188e54a1 100644 --- a/frame/im-online/src/weights.rs +++ b/frame/im-online/src/weights.rs @@ -48,7 +48,7 @@ use sp_std::marker::PhantomData; /// Weight functions needed for pallet_im_online. pub trait WeightInfo { - fn validate_unsigned_and_then_heartbeat(k: u32, e: u32, ) -> Weight; + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight; } /// Weights for pallet_im_online using the Substrate node and recommended hardware. @@ -65,20 +65,15 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - /// The range of component `e` is `[1, 100]`. - fn validate_unsigned_and_then_heartbeat(k: u32, e: u32, ) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + e * (35 ±0) + k * (32 ±0)` + // Estimated: `10024497 + k * (32 ±0)` // Minimum execution time: 95_573_000 picoseconds. Weight::from_parts(78_856_572, 10024497) // Standard Error: 315 .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) - // Standard Error: 3_181 - .saturating_add(Weight::from_parts(362_093, 0).saturating_mul(e.into())) - .saturating_add(T::DbWeight::get().reads(4_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 35).saturating_mul(e.into())) .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) } } @@ -96,20 +91,15 @@ impl WeightInfo for () { /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - /// The range of component `e` is `[1, 100]`. - fn validate_unsigned_and_then_heartbeat(k: u32, e: u32, ) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + e * (35 ±0) + k * (32 ±0)` + // Estimated: `10024497 + k * (32 ±0)` // Minimum execution time: 95_573_000 picoseconds. Weight::from_parts(78_856_572, 10024497) // Standard Error: 315 .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) - // Standard Error: 3_181 - .saturating_add(Weight::from_parts(362_093, 0).saturating_mul(e.into())) - .saturating_add(RocksDbWeight::get().reads(4_u64)) .saturating_add(RocksDbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 35).saturating_mul(e.into())) .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) } } diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs index a6cef85e6ac1b..0ab17c97150db 100644 --- a/primitives/core/src/offchain/mod.rs +++ b/primitives/core/src/offchain/mod.rs @@ -189,8 +189,6 @@ impl TryFrom<u32> for HttpRequestStatus { pub struct OpaqueNetworkState { /// PeerId of the local node in SCALE encoded. pub peer_id: OpaquePeerId, - /// List of addresses the node knows it can be reached as. - pub external_addresses: Vec<OpaqueMultiaddr>, } /// Simple blob to hold a `Multiaddr` without committing to its format. diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index ee3620e701965..16f80d6427490 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -217,7 +217,7 @@ impl offchain::Externalities for TestOffchainExt { } fn network_state(&self) -> Result<OpaqueNetworkState, ()> { - Ok(OpaqueNetworkState { peer_id: Default::default(), external_addresses: vec![] }) + Ok(OpaqueNetworkState { peer_id: Default::default() }) } fn timestamp(&mut self) -> Timestamp { From c8218b9a57e7f08699a579d197b03a76e1d299ef Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 12:58:37 +0400 Subject: [PATCH 02/25] remove unused import --- frame/im-online/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index adbbdad5f4825..ffa261c196c69 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -24,7 +24,7 @@ use super::*; use frame_benchmarking::v1::benchmarks; use frame_support::{traits::UnfilteredDispatchable, WeakBoundedVec}; use frame_system::RawOrigin; -use sp_core::{offchain::OpaqueMultiaddr, OpaquePeerId}; +use sp_core::OpaquePeerId; use sp_runtime::{ traits::{ValidateUnsigned, Zero}, transaction_validity::TransactionSource, From a282042c2d6bf8bae2c383f6e2699c3fe2970a3d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 13:17:37 +0400 Subject: [PATCH 03/25] run benchmark --- frame/im-online/src/weights.rs | 69 ++++++++++------------------------ 1 file changed, 20 insertions(+), 49 deletions(-) diff --git a/frame/im-online/src/weights.rs b/frame/im-online/src/weights.rs index 24301188e54a1..382ef64c7fb25 100644 --- a/frame/im-online/src/weights.rs +++ b/frame/im-online/src/weights.rs @@ -15,12 +15,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Autogenerated weights for pallet_im_online +//! Autogenerated weights for `pallet_im_online` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-04-06, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2023-05-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `bm2`, CPU: `Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz` +//! HOSTNAME: `build-host`, CPU: `AMD EPYC 7601 32-Core Processor` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: @@ -37,49 +37,18 @@ // --heap-pages=4096 // --output=./frame/im-online/src/weights.rs // --header=./HEADER-APACHE2 -// --template=./.maintain/frame-weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] #![allow(unused_imports)] +#![allow(missing_docs)] -use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; -use sp_std::marker::PhantomData; +use frame_support::{traits::Get, weights::Weight}; +use core::marker::PhantomData; -/// Weight functions needed for pallet_im_online. -pub trait WeightInfo { - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight; -} - -/// Weights for pallet_im_online using the Substrate node and recommended hardware. -pub struct SubstrateWeight<T>(PhantomData<T>); -impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { - /// Storage: Session Validators (r:1 w:0) - /// Proof Skipped: Session Validators (max_values: Some(1), max_size: None, mode: Measured) - /// Storage: Session CurrentIndex (r:1 w:0) - /// Proof Skipped: Session CurrentIndex (max_values: Some(1), max_size: None, mode: Measured) - /// Storage: ImOnline Keys (r:1 w:0) - /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) - /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) - /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) - /// Storage: ImOnline AuthoredBlocks (r:1 w:0) - /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) - /// The range of component `k` is `[1, 1000]`. - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { - // Proof Size summary in bytes: - // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + k * (32 ±0)` - // Minimum execution time: 95_573_000 picoseconds. - Weight::from_parts(78_856_572, 10024497) - // Standard Error: 315 - .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) - .saturating_add(T::DbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) - } -} - -// For backwards compatibility and tests -impl WeightInfo for () { +/// Weight functions for `pallet_im_online`. +pub struct WeightInfo<T>(PhantomData<T>); +impl<T: frame_system::Config> pallet_im_online::WeightInfo for WeightInfo<T> { /// Storage: Session Validators (r:1 w:0) /// Proof Skipped: Session Validators (max_values: Some(1), max_size: None, mode: Measured) /// Storage: Session CurrentIndex (r:1 w:0) @@ -87,19 +56,21 @@ impl WeightInfo for () { /// Storage: ImOnline Keys (r:1 w:0) /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) - /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) + /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(1028), added: 3503, mode: MaxEncodedLen) /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + k * (32 ±0)` - // Minimum execution time: 95_573_000 picoseconds. - Weight::from_parts(78_856_572, 10024497) - // Standard Error: 315 - .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) - .saturating_add(RocksDbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) + // Estimated: `321487 + k * (1761 ±0)` + // Minimum execution time: 122_330_000 picoseconds. + Weight::from_parts(125_399_712, 0) + .saturating_add(Weight::from_parts(0, 321487)) + // Standard Error: 794 + .saturating_add(Weight::from_parts(31_162, 0).saturating_mul(k.into())) + .saturating_add(T::DbWeight::get().reads(4)) + .saturating_add(T::DbWeight::get().writes(1)) + .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) } } From b60c5c436a68248d1099a3c911b2dcde13f6a9a5 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 13:17:50 +0400 Subject: [PATCH 04/25] remove external_addresses from offchain NetworkState --- client/offchain/src/api.rs | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 06d35d9766513..4f14f88da6cd2 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -22,7 +22,7 @@ use crate::NetworkProvider; use codec::{Decode, Encode}; use futures::Future; pub use http::SharedClient; -use libp2p::{Multiaddr, PeerId}; +use libp2p::PeerId; use sp_core::{ offchain::{ self, HttpError, HttpRequestId, HttpRequestStatus, OffchainStorage, OpaqueNetworkState, @@ -159,9 +159,7 @@ impl offchain::Externalities for Api { } fn network_state(&self) -> Result<OpaqueNetworkState, ()> { - let external_addresses = self.network_provider.external_addresses(); - - let state = NetworkState::new(self.network_provider.local_peer_id(), external_addresses); + let state = NetworkState::new(self.network_provider.local_peer_id()); Ok(OpaqueNetworkState::from(state)) } @@ -238,12 +236,11 @@ impl offchain::Externalities for Api { #[derive(Clone, Eq, PartialEq, Debug)] pub struct NetworkState { peer_id: PeerId, - external_addresses: Vec<Multiaddr>, } impl NetworkState { - fn new(peer_id: PeerId, external_addresses: Vec<Multiaddr>) -> Self { - NetworkState { peer_id, external_addresses } + fn new(peer_id: PeerId) -> Self { + NetworkState { peer_id } } } @@ -265,7 +262,7 @@ impl TryFrom<OpaqueNetworkState> for NetworkState { let bytes: Vec<u8> = Decode::decode(&mut &inner_vec[..]).map_err(|_| ())?; let peer_id = PeerId::from_bytes(&bytes).map_err(|_| ())?; - Ok(NetworkState { peer_id, external_addresses: vec![] }) + Ok(NetworkState { peer_id }) } } @@ -388,10 +385,6 @@ mod tests { } impl NetworkStateInfo for TestNetwork { - fn external_addresses(&self) -> Vec<Multiaddr> { - Vec::new() - } - fn local_peer_id(&self) -> PeerId { PeerId::random() } @@ -502,13 +495,7 @@ mod tests { #[test] fn should_convert_network_states() { // given - let state = NetworkState::new( - PeerId::random(), - vec![ - Multiaddr::try_from("/ip4/127.0.0.1/tcp/1234".to_string()).unwrap(), - Multiaddr::try_from("/ip6/2601:9:4f81:9700:803e:ca65:66e8:c21").unwrap(), - ], - ); + let state = NetworkState::new(PeerId::random()); // when let opaque_state = OpaqueNetworkState::from(state.clone()); From 5c7956e12f44a09701683176c63e9e49aa5847dc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 13:29:24 +0400 Subject: [PATCH 05/25] add missing fn to TestNetwork --- client/offchain/src/api.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 4f14f88da6cd2..8a9186255a5d9 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -299,7 +299,7 @@ impl AsyncApi { #[cfg(test)] mod tests { use super::*; - use libp2p::PeerId; + use libp2p::{Multiaddr, PeerId}; use sc_client_db::offchain::LocalStorage; use sc_network::{ config::MultiaddrWithPeerId, types::ProtocolName, NetworkPeers, NetworkStateInfo, @@ -392,6 +392,10 @@ mod tests { fn listen_addresses(&self) -> Vec<Multiaddr> { Vec::new() } + + fn external_addresses(&self) -> Vec<Multiaddr> { + Vec::new() + } } fn offchain_api() -> (Api, AsyncApi) { From 97b9fd279b0861e7932fe20ee0692dbd5b7dc5ee Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 13:55:51 +0400 Subject: [PATCH 06/25] Revert "run benchmark" This reverts commit a282042c2d6bf8bae2c383f6e2699c3fe2970a3d. --- frame/im-online/src/weights.rs | 69 ++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 20 deletions(-) diff --git a/frame/im-online/src/weights.rs b/frame/im-online/src/weights.rs index 382ef64c7fb25..24301188e54a1 100644 --- a/frame/im-online/src/weights.rs +++ b/frame/im-online/src/weights.rs @@ -15,12 +15,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Autogenerated weights for `pallet_im_online` +//! Autogenerated weights for pallet_im_online //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-05-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2023-04-06, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `build-host`, CPU: `AMD EPYC 7601 32-Core Processor` +//! HOSTNAME: `bm2`, CPU: `Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: @@ -37,18 +37,49 @@ // --heap-pages=4096 // --output=./frame/im-online/src/weights.rs // --header=./HEADER-APACHE2 +// --template=./.maintain/frame-weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] #![allow(unused_imports)] -#![allow(missing_docs)] -use frame_support::{traits::Get, weights::Weight}; -use core::marker::PhantomData; +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use sp_std::marker::PhantomData; -/// Weight functions for `pallet_im_online`. -pub struct WeightInfo<T>(PhantomData<T>); -impl<T: frame_system::Config> pallet_im_online::WeightInfo for WeightInfo<T> { +/// Weight functions needed for pallet_im_online. +pub trait WeightInfo { + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight; +} + +/// Weights for pallet_im_online using the Substrate node and recommended hardware. +pub struct SubstrateWeight<T>(PhantomData<T>); +impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { + /// Storage: Session Validators (r:1 w:0) + /// Proof Skipped: Session Validators (max_values: Some(1), max_size: None, mode: Measured) + /// Storage: Session CurrentIndex (r:1 w:0) + /// Proof Skipped: Session CurrentIndex (max_values: Some(1), max_size: None, mode: Measured) + /// Storage: ImOnline Keys (r:1 w:0) + /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) + /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) + /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) + /// Storage: ImOnline AuthoredBlocks (r:1 w:0) + /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) + /// The range of component `k` is `[1, 1000]`. + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { + // Proof Size summary in bytes: + // Measured: `295 + k * (32 ±0)` + // Estimated: `10024497 + k * (32 ±0)` + // Minimum execution time: 95_573_000 picoseconds. + Weight::from_parts(78_856_572, 10024497) + // Standard Error: 315 + .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) + .saturating_add(T::DbWeight::get().writes(1_u64)) + .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) + } +} + +// For backwards compatibility and tests +impl WeightInfo for () { /// Storage: Session Validators (r:1 w:0) /// Proof Skipped: Session Validators (max_values: Some(1), max_size: None, mode: Measured) /// Storage: Session CurrentIndex (r:1 w:0) @@ -56,21 +87,19 @@ impl<T: frame_system::Config> pallet_im_online::WeightInfo for WeightInfo<T> { /// Storage: ImOnline Keys (r:1 w:0) /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) - /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(1028), added: 3503, mode: MaxEncodedLen) + /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `321487 + k * (1761 ±0)` - // Minimum execution time: 122_330_000 picoseconds. - Weight::from_parts(125_399_712, 0) - .saturating_add(Weight::from_parts(0, 321487)) - // Standard Error: 794 - .saturating_add(Weight::from_parts(31_162, 0).saturating_mul(k.into())) - .saturating_add(T::DbWeight::get().reads(4)) - .saturating_add(T::DbWeight::get().writes(1)) - .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) + // Estimated: `10024497 + k * (32 ±0)` + // Minimum execution time: 95_573_000 picoseconds. + Weight::from_parts(78_856_572, 10024497) + // Standard Error: 315 + .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) } } From cd589f61cc87d22d53e777233f1ec0285d5674dc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 29 May 2023 15:32:40 +0400 Subject: [PATCH 07/25] update weights --- frame/im-online/src/weights.rs | 45 ++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/frame/im-online/src/weights.rs b/frame/im-online/src/weights.rs index 24301188e54a1..f22363eecc6e4 100644 --- a/frame/im-online/src/weights.rs +++ b/frame/im-online/src/weights.rs @@ -18,9 +18,9 @@ //! Autogenerated weights for pallet_im_online //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-04-06, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2023-05-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `bm2`, CPU: `Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz` +//! HOSTNAME: `build-host`, CPU: `AMD EPYC 7601 32-Core Processor` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: @@ -37,18 +37,19 @@ // --heap-pages=4096 // --output=./frame/im-online/src/weights.rs // --header=./HEADER-APACHE2 -// --template=./.maintain/frame-weight-template.hbs +// --template=./frame-weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] #![allow(unused_imports)] +#![allow(missing_docs)] use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; -use sp_std::marker::PhantomData; +use core::marker::PhantomData; /// Weight functions needed for pallet_im_online. pub trait WeightInfo { - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight; + fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight; } /// Weights for pallet_im_online using the Substrate node and recommended hardware. @@ -61,20 +62,21 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { /// Storage: ImOnline Keys (r:1 w:0) /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) - /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) + /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(1028), added: 3503, mode: MaxEncodedLen) /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + k * (32 ±0)` - // Minimum execution time: 95_573_000 picoseconds. - Weight::from_parts(78_856_572, 10024497) - // Standard Error: 315 - .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) + // Estimated: `321487 + k * (1761 ±0)` + // Minimum execution time: 120_818_000 picoseconds. + Weight::from_parts(121_396_296, 321487) + // Standard Error: 2_183 + .saturating_add(Weight::from_parts(58_710, 0).saturating_mul(k.into())) + .saturating_add(T::DbWeight::get().reads(4_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) + .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) } } @@ -87,19 +89,20 @@ impl WeightInfo for () { /// Storage: ImOnline Keys (r:1 w:0) /// Proof: ImOnline Keys (max_values: Some(1), max_size: Some(320002), added: 320497, mode: MaxEncodedLen) /// Storage: ImOnline ReceivedHeartbeats (r:1 w:1) - /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(10021032), added: 10023507, mode: MaxEncodedLen) + /// Proof: ImOnline ReceivedHeartbeats (max_values: None, max_size: Some(1028), added: 3503, mode: MaxEncodedLen) /// Storage: ImOnline AuthoredBlocks (r:1 w:0) /// Proof: ImOnline AuthoredBlocks (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) /// The range of component `k` is `[1, 1000]`. - fn validate_unsigned_and_then_heartbeat(k: u32) -> Weight { + fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `295 + k * (32 ±0)` - // Estimated: `10024497 + k * (32 ±0)` - // Minimum execution time: 95_573_000 picoseconds. - Weight::from_parts(78_856_572, 10024497) - // Standard Error: 315 - .saturating_add(Weight::from_parts(22_926, 0).saturating_mul(k.into())) + // Estimated: `321487 + k * (1761 ±0)` + // Minimum execution time: 120_818_000 picoseconds. + Weight::from_parts(121_396_296, 321487) + // Standard Error: 2_183 + .saturating_add(Weight::from_parts(58_710, 0).saturating_mul(k.into())) + .saturating_add(RocksDbWeight::get().reads(4_u64)) .saturating_add(RocksDbWeight::get().writes(1_u64)) - .saturating_add(Weight::from_parts(0, 32).saturating_mul(k.into())) + .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) } } From e88d7e2bcec4f89299275aa21147451306b3cb78 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 30 May 2023 15:55:14 +0400 Subject: [PATCH 08/25] address @bkchr comments --- client/offchain/src/api.rs | 56 ++++++++++++++++++++----- frame/im-online/src/lib.rs | 45 +++++++++----------- frame/im-online/src/tests.rs | 6 +-- primitives/core/src/offchain/mod.rs | 2 + primitives/core/src/offchain/testing.rs | 2 +- 5 files changed, 70 insertions(+), 41 deletions(-) diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 8a9186255a5d9..bd3a08d47d8a1 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -16,17 +16,17 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see <https://www.gnu.org/licenses/>. -use std::{collections::HashSet, sync::Arc, thread::sleep}; +use std::{collections::HashSet, str::FromStr, sync::Arc, thread::sleep}; use crate::NetworkProvider; use codec::{Decode, Encode}; use futures::Future; pub use http::SharedClient; -use libp2p::PeerId; +use libp2p::{Multiaddr, PeerId}; use sp_core::{ offchain::{ - self, HttpError, HttpRequestId, HttpRequestStatus, OffchainStorage, OpaqueNetworkState, - StorageKind, Timestamp, + self, HttpError, HttpRequestId, HttpRequestStatus, OffchainStorage, OpaqueMultiaddr, + OpaqueNetworkState, StorageKind, Timestamp, }, OpaquePeerId, }; @@ -159,7 +159,9 @@ impl offchain::Externalities for Api { } fn network_state(&self) -> Result<OpaqueNetworkState, ()> { - let state = NetworkState::new(self.network_provider.local_peer_id()); + let external_addresses = self.network_provider.external_addresses(); + + let state = NetworkState::new(self.network_provider.local_peer_id(), external_addresses); Ok(OpaqueNetworkState::from(state)) } @@ -236,11 +238,12 @@ impl offchain::Externalities for Api { #[derive(Clone, Eq, PartialEq, Debug)] pub struct NetworkState { peer_id: PeerId, + external_addresses: Vec<Multiaddr>, } impl NetworkState { - fn new(peer_id: PeerId) -> Self { - NetworkState { peer_id } + fn new(peer_id: PeerId, external_addresses: Vec<Multiaddr>) -> Self { + NetworkState { peer_id, external_addresses } } } @@ -249,7 +252,16 @@ impl From<NetworkState> for OpaqueNetworkState { let enc = Encode::encode(&state.peer_id.to_bytes()); let peer_id = OpaquePeerId::new(enc); - OpaqueNetworkState { peer_id } + let external_addresses: Vec<OpaqueMultiaddr> = state + .external_addresses + .iter() + .map(|multiaddr| { + let e = Encode::encode(&multiaddr.to_string()); + OpaqueMultiaddr::new(e) + }) + .collect(); + + OpaqueNetworkState { peer_id, external_addresses } } } @@ -262,7 +274,20 @@ impl TryFrom<OpaqueNetworkState> for NetworkState { let bytes: Vec<u8> = Decode::decode(&mut &inner_vec[..]).map_err(|_| ())?; let peer_id = PeerId::from_bytes(&bytes).map_err(|_| ())?; - Ok(NetworkState { peer_id }) + let external_addresses: Result<Vec<Multiaddr>, Self::Error> = state + .external_addresses + .iter() + .map(|enc_multiaddr| -> Result<Multiaddr, Self::Error> { + let inner_vec = &enc_multiaddr.0; + let bytes = <Vec<u8>>::decode(&mut &inner_vec[..]).map_err(|_| ())?; + let multiaddr_str = String::from_utf8(bytes).map_err(|_| ())?; + let multiaddr = Multiaddr::from_str(&multiaddr_str).map_err(|_| ())?; + Ok(multiaddr) + }) + .collect(); + let external_addresses = external_addresses?; + + Ok(NetworkState { peer_id, external_addresses }) } } @@ -299,7 +324,6 @@ impl AsyncApi { #[cfg(test)] mod tests { use super::*; - use libp2p::{Multiaddr, PeerId}; use sc_client_db::offchain::LocalStorage; use sc_network::{ config::MultiaddrWithPeerId, types::ProtocolName, NetworkPeers, NetworkStateInfo, @@ -385,6 +409,10 @@ mod tests { } impl NetworkStateInfo for TestNetwork { + fn external_addresses(&self) -> Vec<Multiaddr> { + Vec::new() + } + fn local_peer_id(&self) -> PeerId { PeerId::random() } @@ -499,7 +527,13 @@ mod tests { #[test] fn should_convert_network_states() { // given - let state = NetworkState::new(PeerId::random()); + let state = NetworkState::new( + PeerId::random(), + vec![ + Multiaddr::try_from("/ip4/127.0.0.1/tcp/1234".to_string()).unwrap(), + Multiaddr::try_from("/ip6/2601:9:4f81:9700:803e:ca65:66e8:c21").unwrap(), + ], + ); // when let opaque_state = OpaqueNetworkState::from(state.clone()); diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index bee341d2484b3..f4a34cbbc11a6 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -26,8 +26,7 @@ //! in the current era or session. //! //! The heartbeat is a signed transaction, which was signed using the session key -//! and includes the recent best block number of the local validators chain as well -//! as the [NetworkState](../../client/offchain/struct.NetworkState.html). +//! and includes the recent best block number of the local validators chain. //! It is submitted as an Unsigned Transaction via off-chain workers. //! //! - [`Config`] @@ -94,7 +93,7 @@ use frame_system::offchain::{SendTransactionTypes, SubmitTransaction}; pub use pallet::*; use scale_info::TypeInfo; use sp_application_crypto::RuntimeAppPublic; -use sp_core::offchain::OpaqueNetworkState; +use sp_core::OpaquePeerId; use sp_runtime::{ offchain::storage::{MutateStorageError, StorageRetrievalError, StorageValueRef}, traits::{AtLeast32BitUnsigned, Convert, Saturating, TrailingZeroInput}, @@ -222,8 +221,8 @@ where { /// Block number at the time heartbeat is created.. pub block_number: BlockNumber, - /// A state of local network (peer id and external addresses) - pub network_state: OpaqueNetworkState, + /// Peer ID of the local node + pub peer_id: OpaquePeerId, /// Index of the current session. pub session_index: SessionIndex, /// An index of the authority on the list of validators. @@ -232,31 +231,26 @@ where pub validators_len: u32, } -/// A type that is the same as [`OpaqueNetworkState`] but with [`Vec`] replaced with +/// A type that is the same as [`OpaquePeerId`] but with [`Vec`] replaced with /// [`WeakBoundedVec<Limit>`] where Limit is the respective size limit /// `PeerIdEncodingLimit` represents the size limit of the encoding of `PeerId` #[derive(Clone, Eq, PartialEq, Encode, Decode, MaxEncodedLen, TypeInfo)] #[codec(mel_bound())] #[scale_info(skip_type_params(PeerIdEncodingLimit))] -pub struct BoundedOpaqueNetworkState<PeerIdEncodingLimit> +pub struct BoundedOpaquePeerId<PeerIdEncodingLimit>(WeakBoundedVec<u8, PeerIdEncodingLimit>) where - PeerIdEncodingLimit: Get<u32>, -{ - /// PeerId of the local node in SCALE encoded. - pub peer_id: WeakBoundedVec<u8, PeerIdEncodingLimit>, -} + PeerIdEncodingLimit: Get<u32>; -impl<PeerIdEncodingLimit: Get<u32>> BoundedOpaqueNetworkState<PeerIdEncodingLimit> { - fn force_from(ons: &OpaqueNetworkState) -> Self { - let peer_id = WeakBoundedVec::<_, PeerIdEncodingLimit>::force_from( - ons.peer_id.0.clone(), +impl<PeerIdEncodingLimit: Get<u32>> BoundedOpaquePeerId<PeerIdEncodingLimit> { + fn force_from(opaque_peer_id: &OpaquePeerId) -> Self { + Self(WeakBoundedVec::<_, PeerIdEncodingLimit>::force_from( + opaque_peer_id.0.clone(), Some( "Warning: The size of the encoding of PeerId \ is bigger than expected. A runtime configuration \ adjustment may be needed.", ), - ); - Self { peer_id } + )) } } @@ -379,7 +373,7 @@ pub mod pallet { StorageValue<_, WeakBoundedVec<T::AuthorityId, T::MaxKeys>, ValueQuery>; /// For each session index, we keep a mapping of `SessionIndex` and `AuthIndex` to - /// `WrapperOpaque<BoundedOpaqueNetworkState>`. + /// `WrapperOpaque<BoundedOpaquePeerId>`. #[pallet::storage] #[pallet::getter(fn received_heartbeats)] pub(crate) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< @@ -388,7 +382,7 @@ pub mod pallet { SessionIndex, Twox64Concat, AuthIndex, - WrapperOpaque<BoundedOpaqueNetworkState<T::MaxPeerDataEncodingSize>>, + WrapperOpaque<BoundedOpaquePeerId<T::MaxPeerDataEncodingSize>>, >; /// For each session index, we keep a mapping of `ValidatorId<T>` to the @@ -446,14 +440,13 @@ pub mod pallet { if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - let network_state_bounded = - BoundedOpaqueNetworkState::<T::MaxPeerDataEncodingSize>::force_from( - &heartbeat.network_state, - ); + let peer_id = BoundedOpaquePeerId::<T::MaxPeerDataEncodingSize>::force_from( + &heartbeat.peer_id, + ); ReceivedHeartbeats::<T>::insert( ¤t_session, &heartbeat.authority_index, - WrapperOpaque::from(network_state_bounded), + WrapperOpaque::from(peer_id), ); Ok(()) @@ -669,7 +662,7 @@ impl<T: Config> Pallet<T> { sp_io::offchain::network_state().map_err(|_| OffchainErr::NetworkState)?; let heartbeat = Heartbeat { block_number, - network_state, + peer_id: network_state.peer_id, session_index, authority_index, validators_len, diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index 5fbb0a34d4e2e..8ea72a065283c 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -125,7 +125,7 @@ fn heartbeat( let heartbeat = Heartbeat { block_number, - network_state: OpaqueNetworkState { peer_id: OpaquePeerId(vec![1]) }, + peer_id: OpaquePeerId(vec![1]), session_index, authority_index, validators_len: validators.len() as u32, @@ -249,7 +249,7 @@ fn should_generate_heartbeats() { heartbeat, Heartbeat { block_number: block, - network_state: sp_io::offchain::network_state().unwrap(), + peer_id: sp_io::offchain::network_state().unwrap().peer_id, session_index: 2, authority_index: 2, validators_len: 3, @@ -364,7 +364,7 @@ fn should_not_send_a_report_if_already_online() { heartbeat, Heartbeat { block_number: 4, - network_state: sp_io::offchain::network_state().unwrap(), + peer_id: sp_io::offchain::network_state().unwrap().peer_id, session_index: 2, authority_index: 0, validators_len: 3, diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs index 0ab17c97150db..a6cef85e6ac1b 100644 --- a/primitives/core/src/offchain/mod.rs +++ b/primitives/core/src/offchain/mod.rs @@ -189,6 +189,8 @@ impl TryFrom<u32> for HttpRequestStatus { pub struct OpaqueNetworkState { /// PeerId of the local node in SCALE encoded. pub peer_id: OpaquePeerId, + /// List of addresses the node knows it can be reached as. + pub external_addresses: Vec<OpaqueMultiaddr>, } /// Simple blob to hold a `Multiaddr` without committing to its format. diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index 16f80d6427490..ee3620e701965 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -217,7 +217,7 @@ impl offchain::Externalities for TestOffchainExt { } fn network_state(&self) -> Result<OpaqueNetworkState, ()> { - Ok(OpaqueNetworkState { peer_id: Default::default() }) + Ok(OpaqueNetworkState { peer_id: Default::default(), external_addresses: vec![] }) } fn timestamp(&mut self) -> Timestamp { From 41bca16d12cc40f79b7d79c1a8be65528e898154 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 30 May 2023 16:03:26 +0400 Subject: [PATCH 09/25] remove duplicate fn --- client/offchain/src/api.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 504634f8e872e..22be37393e2d3 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -408,10 +408,6 @@ mod tests { fn listen_addresses(&self) -> Vec<Multiaddr> { Vec::new() } - - fn external_addresses(&self) -> Vec<Multiaddr> { - Vec::new() - } } fn offchain_api() -> (Api, AsyncApi) { From 619a28e4fd7c8d71e41a82d0763dcc87182b12d0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 30 May 2023 18:06:22 +0400 Subject: [PATCH 10/25] cleanup benchmarking.rs --- frame/im-online/src/benchmarking.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index ffa261c196c69..ef40fe453db6b 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -48,10 +48,9 @@ pub fn create_heartbeat<T: Config>( .map_err(|()| "More than the maximum number of keys provided")?; Keys::<T>::put(bounded_keys); - let network_state = OpaqueNetworkState { peer_id: OpaquePeerId::default() }; let input_heartbeat = Heartbeat { block_number: T::BlockNumber::zero(), - network_state, + peer_id: OpaquePeerId::default(), session_index: 0, authority_index: k - 1, validators_len: keys.len() as u32, From aaefc66ec434eb6e3291db2dfd1e8e57f0ff9d1d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 30 May 2023 18:23:10 +0400 Subject: [PATCH 11/25] fix executor tests --- bin/node/executor/tests/submit_transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs index b260f90a87466..83ff4c2c1f4fd 100644 --- a/bin/node/executor/tests/submit_transaction.rs +++ b/bin/node/executor/tests/submit_transaction.rs @@ -37,7 +37,7 @@ fn should_submit_unsigned_transaction() { pallet_im_online::sr25519::AuthoritySignature::try_from(vec![0; 64]).unwrap(); let heartbeat_data = pallet_im_online::Heartbeat { block_number: 1, - network_state: Default::default(), + peer_id: Default::default(), session_index: 1, authority_index: 0, validators_len: 0, From 57246bf8ab5952acfcde676f0708ae040a483607 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 31 May 2023 10:42:52 +0400 Subject: [PATCH 12/25] remove peer_id from hearbeat as well https://github.com/paritytech/substrate/pull/14251#discussion_r1210887220 --- bin/node/executor/tests/submit_transaction.rs | 1 - frame/im-online/src/benchmarking.rs | 2 - frame/im-online/src/lib.rs | 67 ++----------------- frame/im-online/src/mock.rs | 1 - frame/im-online/src/tests.rs | 19 ++---- 5 files changed, 11 insertions(+), 79 deletions(-) diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs index 83ff4c2c1f4fd..7678a3c6e5a9f 100644 --- a/bin/node/executor/tests/submit_transaction.rs +++ b/bin/node/executor/tests/submit_transaction.rs @@ -37,7 +37,6 @@ fn should_submit_unsigned_transaction() { pallet_im_online::sr25519::AuthoritySignature::try_from(vec![0; 64]).unwrap(); let heartbeat_data = pallet_im_online::Heartbeat { block_number: 1, - peer_id: Default::default(), session_index: 1, authority_index: 0, validators_len: 0, diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index ef40fe453db6b..4766e5edaadcf 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -24,7 +24,6 @@ use super::*; use frame_benchmarking::v1::benchmarks; use frame_support::{traits::UnfilteredDispatchable, WeakBoundedVec}; use frame_system::RawOrigin; -use sp_core::OpaquePeerId; use sp_runtime::{ traits::{ValidateUnsigned, Zero}, transaction_validity::TransactionSource, @@ -50,7 +49,6 @@ pub fn create_heartbeat<T: Config>( let input_heartbeat = Heartbeat { block_number: T::BlockNumber::zero(), - peer_id: OpaquePeerId::default(), session_index: 0, authority_index: k - 1, validators_len: keys.len() as u32, diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index f4a34cbbc11a6..2d1ff580c3242 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -85,7 +85,7 @@ use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ traits::{ EstimateNextSessionRotation, Get, OneSessionHandler, ValidatorSet, - ValidatorSetWithIdentification, WrapperOpaque, + ValidatorSetWithIdentification, }, BoundedSlice, WeakBoundedVec, }; @@ -93,7 +93,6 @@ use frame_system::offchain::{SendTransactionTypes, SubmitTransaction}; pub use pallet::*; use scale_info::TypeInfo; use sp_application_crypto::RuntimeAppPublic; -use sp_core::OpaquePeerId; use sp_runtime::{ offchain::storage::{MutateStorageError, StorageRetrievalError, StorageValueRef}, traits::{AtLeast32BitUnsigned, Convert, Saturating, TrailingZeroInput}, @@ -189,7 +188,6 @@ enum OffchainErr<BlockNumber> { AlreadyOnline(u32), FailedSigning, FailedToAcquireLock, - NetworkState, SubmitTransaction, } @@ -205,7 +203,6 @@ impl<BlockNumber: sp_std::fmt::Debug> sp_std::fmt::Debug for OffchainErr<BlockNu }, OffchainErr::FailedSigning => write!(fmt, "Failed to sign heartbeat"), OffchainErr::FailedToAcquireLock => write!(fmt, "Failed to acquire lock"), - OffchainErr::NetworkState => write!(fmt, "Failed to fetch network state"), OffchainErr::SubmitTransaction => write!(fmt, "Failed to submit transaction"), } } @@ -221,8 +218,6 @@ where { /// Block number at the time heartbeat is created.. pub block_number: BlockNumber, - /// Peer ID of the local node - pub peer_id: OpaquePeerId, /// Index of the current session. pub session_index: SessionIndex, /// An index of the authority on the list of validators. @@ -231,29 +226,6 @@ where pub validators_len: u32, } -/// A type that is the same as [`OpaquePeerId`] but with [`Vec`] replaced with -/// [`WeakBoundedVec<Limit>`] where Limit is the respective size limit -/// `PeerIdEncodingLimit` represents the size limit of the encoding of `PeerId` -#[derive(Clone, Eq, PartialEq, Encode, Decode, MaxEncodedLen, TypeInfo)] -#[codec(mel_bound())] -#[scale_info(skip_type_params(PeerIdEncodingLimit))] -pub struct BoundedOpaquePeerId<PeerIdEncodingLimit>(WeakBoundedVec<u8, PeerIdEncodingLimit>) -where - PeerIdEncodingLimit: Get<u32>; - -impl<PeerIdEncodingLimit: Get<u32>> BoundedOpaquePeerId<PeerIdEncodingLimit> { - fn force_from(opaque_peer_id: &OpaquePeerId) -> Self { - Self(WeakBoundedVec::<_, PeerIdEncodingLimit>::force_from( - opaque_peer_id.0.clone(), - Some( - "Warning: The size of the encoding of PeerId \ - is bigger than expected. A runtime configuration \ - adjustment may be needed.", - ), - )) - } -} - /// A type for representing the validator id in a session. pub type ValidatorId<T> = <<T as Config>::ValidatorSet as ValidatorSet< <T as frame_system::Config>::AccountId, @@ -295,10 +267,6 @@ pub mod pallet { /// The maximum number of peers to be stored in `ReceivedHeartbeats` type MaxPeerInHeartbeats: Get<u32>; - /// The maximum size of the encoding of `PeerId` and `MultiAddr` that are coming - /// from the hearbeat - type MaxPeerDataEncodingSize: Get<u32>; - /// The overarching event type. type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>; @@ -372,18 +340,11 @@ pub mod pallet { pub(crate) type Keys<T: Config> = StorageValue<_, WeakBoundedVec<T::AuthorityId, T::MaxKeys>, ValueQuery>; - /// For each session index, we keep a mapping of `SessionIndex` and `AuthIndex` to - /// `WrapperOpaque<BoundedOpaquePeerId>`. + /// For each session index, we keep a mapping of `SessionIndex` and `AuthIndex`. #[pallet::storage] #[pallet::getter(fn received_heartbeats)] - pub(crate) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< - _, - Twox64Concat, - SessionIndex, - Twox64Concat, - AuthIndex, - WrapperOpaque<BoundedOpaquePeerId<T::MaxPeerDataEncodingSize>>, - >; + pub(crate) type ReceivedHeartbeats<T: Config> = + StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, bool>; /// For each session index, we keep a mapping of `ValidatorId<T>` to the /// number of blocks authored by the given authority. @@ -440,14 +401,7 @@ pub mod pallet { if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - let peer_id = BoundedOpaquePeerId::<T::MaxPeerDataEncodingSize>::force_from( - &heartbeat.peer_id, - ); - ReceivedHeartbeats::<T>::insert( - ¤t_session, - &heartbeat.authority_index, - WrapperOpaque::from(peer_id), - ); + ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, true); Ok(()) } else if exists { @@ -658,15 +612,8 @@ impl<T: Config> Pallet<T> { ) -> OffchainResult<T, ()> { // A helper function to prepare heartbeat call. let prepare_heartbeat = || -> OffchainResult<T, Call<T>> { - let network_state = - sp_io::offchain::network_state().map_err(|_| OffchainErr::NetworkState)?; - let heartbeat = Heartbeat { - block_number, - peer_id: network_state.peer_id, - session_index, - authority_index, - validators_len, - }; + let heartbeat = + Heartbeat { block_number, session_index, authority_index, validators_len }; let signature = key.sign(&heartbeat.encode()).ok_or(OffchainErr::FailedSigning)?; diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 64e77b24b9b09..78a86edf54598 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -217,7 +217,6 @@ impl Config for Runtime { type WeightInfo = (); type MaxKeys = ConstU32<10_000>; type MaxPeerInHeartbeats = ConstU32<10_000>; - type MaxPeerDataEncodingSize = ConstU32<1_000>; } impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Runtime diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index 8ea72a065283c..a51eff61e78b7 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -22,12 +22,9 @@ use super::*; use crate::mock::*; use frame_support::{assert_noop, dispatch}; -use sp_core::{ - offchain::{ - testing::{TestOffchainExt, TestTransactionPoolExt}, - OffchainDbExt, OffchainWorkerExt, TransactionPoolExt, - }, - OpaquePeerId, +use sp_core::offchain::{ + testing::{TestOffchainExt, TestTransactionPoolExt}, + OffchainDbExt, OffchainWorkerExt, TransactionPoolExt, }; use sp_runtime::{ testing::UintAuthorityId, @@ -125,7 +122,6 @@ fn heartbeat( let heartbeat = Heartbeat { block_number, - peer_id: OpaquePeerId(vec![1]), session_index, authority_index, validators_len: validators.len() as u32, @@ -249,7 +245,6 @@ fn should_generate_heartbeats() { heartbeat, Heartbeat { block_number: block, - peer_id: sp_io::offchain::network_state().unwrap().peer_id, session_index: 2, authority_index: 2, validators_len: 3, @@ -362,13 +357,7 @@ fn should_not_send_a_report_if_already_online() { assert_eq!( heartbeat, - Heartbeat { - block_number: 4, - peer_id: sp_io::offchain::network_state().unwrap().peer_id, - session_index: 2, - authority_index: 0, - validators_len: 3, - } + Heartbeat { block_number: 4, session_index: 2, authority_index: 0, validators_len: 3 } ); }); } From 8e4061dc240e096248193d30095090b857e3bbcc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 31 May 2023 11:05:11 +0400 Subject: [PATCH 13/25] remove MaxPeerDataEncodingSize --- bin/node/runtime/src/lib.rs | 7 +++---- frame/offences/benchmarking/src/mock.rs | 1 - 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 6dc9841f6b44f..dfc4db060ce62 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -326,8 +326,9 @@ impl InstanceFilter<RuntimeCall> for ProxyType { RuntimeCall::Elections(..) | RuntimeCall::Treasury(..) ), - ProxyType::Staking => - matches!(c, RuntimeCall::Staking(..) | RuntimeCall::FastUnstake(..)), + ProxyType::Staking => { + matches!(c, RuntimeCall::Staking(..) | RuntimeCall::FastUnstake(..)) + }, } } fn is_superset(&self, o: &Self) -> bool { @@ -1255,7 +1256,6 @@ parameter_types! { pub const MaxAuthorities: u32 = 100; pub const MaxKeys: u32 = 10_000; pub const MaxPeerInHeartbeats: u32 = 10_000; - pub const MaxPeerDataEncodingSize: u32 = 1_000; } impl<LocalCall> frame_system::offchain::CreateSignedTransaction<LocalCall> for Runtime @@ -1323,7 +1323,6 @@ impl pallet_im_online::Config for Runtime { type WeightInfo = pallet_im_online::weights::SubstrateWeight<Runtime>; type MaxKeys = MaxKeys; type MaxPeerInHeartbeats = MaxPeerInHeartbeats; - type MaxPeerDataEncodingSize = MaxPeerDataEncodingSize; } impl pallet_offences::Config for Runtime { diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index ed0c6c7ea4433..6bb3b9d7f4ee8 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -197,7 +197,6 @@ impl pallet_im_online::Config for Test { type WeightInfo = (); type MaxKeys = ConstU32<10_000>; type MaxPeerInHeartbeats = ConstU32<10_000>; - type MaxPeerDataEncodingSize = ConstU32<1_000>; } impl pallet_offences::Config for Test { From dcc7b38fa3e53876a0e1d5c76b6c3ef64c157c76 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Mon, 5 Jun 2023 16:24:46 +0300 Subject: [PATCH 14/25] change storage value type to `()` https://github.com/paritytech/substrate/pull/14251#discussion_r1214268931 --- frame/im-online/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 2d1ff580c3242..5e594dd182318 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -344,7 +344,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn received_heartbeats)] pub(crate) type ReceivedHeartbeats<T: Config> = - StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, bool>; + StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, ()>; /// For each session index, we keep a mapping of `ValidatorId<T>` to the /// number of blocks authored by the given authority. @@ -401,7 +401,7 @@ pub mod pallet { if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, true); + ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, ()); Ok(()) } else if exists { From 81d95602c695e590654647318ecfd11bb118c16d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 6 Jun 2023 12:16:21 +0300 Subject: [PATCH 15/25] scaffold storage migration --- frame/im-online/src/lib.rs | 10 ++- frame/im-online/src/migration.rs | 140 +++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 frame/im-online/src/migration.rs diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 5e594dd182318..0c357adc57aec 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -77,6 +77,7 @@ #![cfg_attr(not(feature = "std"), no_std)] mod benchmarking; +pub mod migration; mod mock; mod tests; pub mod weights; @@ -105,6 +106,9 @@ use sp_staking::{ use sp_std::prelude::*; pub use weights::WeightInfo; +use frame_support::pallet_prelude::*; +use frame_system::pallet_prelude::*; + pub mod sr25519 { mod app_sr25519 { use sp_application_crypto::{app_crypto, key_types::IM_ONLINE, sr25519}; @@ -245,10 +249,12 @@ type OffchainResult<T, A> = Result<A, OffchainErr<<T as frame_system::Config>::B #[frame_support::pallet] pub mod pallet { use super::*; - use frame_support::pallet_prelude::*; - use frame_system::pallet_prelude::*; + + /// The current storage version. + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); #[pallet::pallet] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet<T>(_); #[pallet::config] diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs new file mode 100644 index 0000000000000..bc3e9cba04d35 --- /dev/null +++ b/frame/im-online/src/migration.rs @@ -0,0 +1,140 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +//! Storage migrations for the im-online pallet. + +use super::*; +use frame_support::{storage_alias, traits::OnRuntimeUpgrade}; + +#[cfg(feature = "try-runtime")] +use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + +/// The log target. +const TARGET: &'static str = "runtime::im-online::migration::v1"; + +/// The original data layout of the im-online pallet without a specific version number. +mod v0 { + use super::*; + use frame_support::traits::WrapperOpaque; + + /// A type that is the same as `OpaqueNetworkState` but with [`Vec`] replaced with + /// [`WeakBoundedVec<Limit>`] where Limit is the respective size limit + /// `PeerIdEncodingLimit` represents the size limit of the encoding of `PeerId` + /// `MultiAddrEncodingLimit` represents the size limit of the encoding of `MultiAddr` + /// `AddressesLimit` represents the size limit of the vector of peers connected + #[derive(Clone, Eq, PartialEq, Encode, Decode, MaxEncodedLen, TypeInfo)] + pub struct BoundedOpaqueNetworkState< + PeerIdEncodingLimit, + MultiAddrEncodingLimit, + AddressesLimit, + > where + PeerIdEncodingLimit: Get<u32>, + MultiAddrEncodingLimit: Get<u32>, + AddressesLimit: Get<u32>, + { + /// PeerId of the local node in SCALE encoded. + pub peer_id: WeakBoundedVec<u8, PeerIdEncodingLimit>, + /// List of addresses the node knows it can be reached as. + pub external_addresses: + WeakBoundedVec<WeakBoundedVec<u8, MultiAddrEncodingLimit>, AddressesLimit>, + } + + #[storage_alias] + pub(crate) type HeartbeatAfter<T: Config> = StorageValue<Pallet<T>, T::BlockNumber, ValueQuery>; + + #[storage_alias] + pub(crate) type Keys<T: Config> = + StorageValue<Pallet<T>, WeakBoundedVec<T::AuthorityId, T::MaxKeys>, ValueQuery>; + + #[storage_alias] + pub(crate) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< + Pallet<T>, + Twox64Concat, + SessionIndex, + Twox64Concat, + AuthIndex, + WrapperOpaque< + BoundedOpaqueNetworkState< + T::MaxPeerDataEncodingSize, + T::MaxPeerDataEncodingSize, + T::MaxPeerInHeartbeats, + >, + >, + >; + + #[storage_alias] + pub(crate) type AuthoredBlocks<T: pallet::Config> = StorageDoubleMap< + Pallet<T>, + Twox64Concat, + SessionIndex, + Twox64Concat, + ValidatorId<T>, + u32, + ValueQuery, + >; +} + +pub mod v1 { + use super::{v0::BoundedOpaqueNetworkState, *}; + + /// Migration for moving im-online from V0 to V1 storage. + pub struct Migration<T>(sp_std::marker::PhantomData<T>); + + impl<T: Config> OnRuntimeUpgrade for Migration<T> { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), TryRuntimeError> { + ensure!(StorageVersion::get::<Pallet<T>>() == 0, "can only upgrade from version 0"); + + log::info!( + target: TARGET, + "Migrating {} received heartbeats", + v0::ReceivedHeartbeats::<T>::iter().count() + ); + + Ok(()) + } + + fn on_runtime_upgrade() -> Weight { + ReceivedHeartbeats::<T>::translate::<BoundedOpaqueNetworkState<T>, _>( + |k: T::SessionIndex, T::AccountId, state: BoundedOpaqueNetworkState<T>| { + log::info!(target: TARGET, "Migrated received heartbeat for {:?}...", k); + Some(()) + }, + ); + + StorageVersion::new(1).put::<Pallet<T>>(); + + let count = ReceivedHeartbeats::<T>::iter().count(); + T::DbWeight::get().reads_writes(count as Weight + 1, count as Weight + 1) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec<u8>) -> DispatchResult { + ensure!(StorageVersion::get::<Pallet<T>>() == 1, "must upgrade"); + + log::info!( + target: TARGET, + "Migrated {} received heartbeats", + crate::ReceivedHeartbeats::<T>::iter().count() + ); + + Ok(()) + } + } +} From c822c2da3755e002737e4b48130c273db69238a0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 6 Jun 2023 12:29:39 +0300 Subject: [PATCH 16/25] no need to check the type actually --- frame/im-online/src/migration.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index bc3e9cba04d35..94ab7841b1c3d 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -69,13 +69,7 @@ mod v0 { SessionIndex, Twox64Concat, AuthIndex, - WrapperOpaque< - BoundedOpaqueNetworkState< - T::MaxPeerDataEncodingSize, - T::MaxPeerDataEncodingSize, - T::MaxPeerInHeartbeats, - >, - >, + WrapperOpaque<BoundedOpaqueNetworkState<u32, u32, T::MaxPeerInHeartbeats>>, >; #[storage_alias] @@ -91,7 +85,7 @@ mod v0 { } pub mod v1 { - use super::{v0::BoundedOpaqueNetworkState, *}; + use super::*; /// Migration for moving im-online from V0 to V1 storage. pub struct Migration<T>(sp_std::marker::PhantomData<T>); @@ -111,8 +105,8 @@ pub mod v1 { } fn on_runtime_upgrade() -> Weight { - ReceivedHeartbeats::<T>::translate::<BoundedOpaqueNetworkState<T>, _>( - |k: T::SessionIndex, T::AccountId, state: BoundedOpaqueNetworkState<T>| { + ReceivedHeartbeats::<T>::translate::<_, _>( + |k: T::SessionIndex, T::AccountId, state: _| { log::info!(target: TARGET, "Migrated received heartbeat for {:?}...", k); Some(()) }, From ace0f6e9e12c4b614f7dce95dfc7b1aeb34b9bf9 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 6 Jun 2023 15:56:02 +0300 Subject: [PATCH 17/25] remove unnecessary types from v0 mod --- frame/im-online/src/lib.rs | 9 +++--- frame/im-online/src/migration.rs | 47 +++++++++++++++----------------- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 0c357adc57aec..198c428c53b98 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -84,13 +84,17 @@ pub mod weights; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ + pallet_prelude::*, traits::{ EstimateNextSessionRotation, Get, OneSessionHandler, ValidatorSet, ValidatorSetWithIdentification, }, BoundedSlice, WeakBoundedVec, }; -use frame_system::offchain::{SendTransactionTypes, SubmitTransaction}; +use frame_system::{ + offchain::{SendTransactionTypes, SubmitTransaction}, + pallet_prelude::*, +}; pub use pallet::*; use scale_info::TypeInfo; use sp_application_crypto::RuntimeAppPublic; @@ -106,9 +110,6 @@ use sp_staking::{ use sp_std::prelude::*; pub use weights::WeightInfo; -use frame_support::pallet_prelude::*; -use frame_system::pallet_prelude::*; - pub mod sr25519 { mod app_sr25519 { use sp_application_crypto::{app_crypto, key_types::IM_ONLINE, sr25519}; diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 94ab7841b1c3d..db6461c618764 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -28,7 +28,7 @@ use sp_runtime::TryRuntimeError; /// The log target. const TARGET: &'static str = "runtime::im-online::migration::v1"; -/// The original data layout of the im-online pallet without a specific version number. +/// The original data layout of the im-online pallet (`ReceivedHeartbeats` storage item). mod v0 { use super::*; use frame_support::traits::WrapperOpaque; @@ -55,13 +55,6 @@ mod v0 { WeakBoundedVec<WeakBoundedVec<u8, MultiAddrEncodingLimit>, AddressesLimit>, } - #[storage_alias] - pub(crate) type HeartbeatAfter<T: Config> = StorageValue<Pallet<T>, T::BlockNumber, ValueQuery>; - - #[storage_alias] - pub(crate) type Keys<T: Config> = - StorageValue<Pallet<T>, WeakBoundedVec<T::AuthorityId, T::MaxKeys>, ValueQuery>; - #[storage_alias] pub(crate) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< Pallet<T>, @@ -71,23 +64,12 @@ mod v0 { AuthIndex, WrapperOpaque<BoundedOpaqueNetworkState<u32, u32, T::MaxPeerInHeartbeats>>, >; - - #[storage_alias] - pub(crate) type AuthoredBlocks<T: pallet::Config> = StorageDoubleMap< - Pallet<T>, - Twox64Concat, - SessionIndex, - Twox64Concat, - ValidatorId<T>, - u32, - ValueQuery, - >; } pub mod v1 { use super::*; - /// Migration for moving im-online from V0 to V1 storage. + /// Simple migration that replaces `ReceivedHeartbeats` values with `()`. pub struct Migration<T>(sp_std::marker::PhantomData<T>); impl<T: Config> OnRuntimeUpgrade for Migration<T> { @@ -105,17 +87,32 @@ pub mod v1 { } fn on_runtime_upgrade() -> Weight { - ReceivedHeartbeats::<T>::translate::<_, _>( + let mut weight = T::DbWeight::get().reads(1); + if StorageVersion::get::<Pallet<T>>() != 0 { + log::warn!( + target: TARGET, + "Skipping migration because current storage version is not 0" + ); + return weight + } + + let count = v0::ReceivedHeartbeats::<T>::iter().count(); + weight.saturating_accrue( + T::DbWeight::get().reads(v0::ReceivedHeartbeats::<T>::iter().count() as u64), + ); + weight.saturating_accrue( + T::DbWeight::get().writes(v0::ReceivedHeartbeats::<T>::iter().count() as u64), + ); + + v0::ReceivedHeartbeats::<T>::translate::<_, _>( |k: T::SessionIndex, T::AccountId, state: _| { - log::info!(target: TARGET, "Migrated received heartbeat for {:?}...", k); + log::trace!(target: TARGET, "Migrated received heartbeat for {:?}...", k); Some(()) }, ); StorageVersion::new(1).put::<Pallet<T>>(); - - let count = ReceivedHeartbeats::<T>::iter().count(); - T::DbWeight::get().reads_writes(count as Weight + 1, count as Weight + 1) + weight.saturating_add(T::DbWeight::get().writes(1)) } #[cfg(feature = "try-runtime")] From 906fa969831f222c5a54b4a3ec8adfb7ade6a8c2 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 6 Jun 2023 16:06:20 +0300 Subject: [PATCH 18/25] add a test for migration --- frame/im-online/src/migration.rs | 39 ++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index db6461c618764..c8a3f04e60405 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -129,3 +129,42 @@ pub mod v1 { } } } + +#[cfg(test)] +#[cfg(feature = "try-runtime")] +mod test { + use super::*; + use crate::mock::{Test as T, *}; + + use frame_support::bounded_vec; + + #[test] + fn migration_works() { + new_test_ext().execute_with(|| { + assert_eq!(StorageVersion::get::<Pallet<T>>(), 0); + + // Insert some received heartbeats into the v0 storage: + let current_session = T::ValidatorSet::session_index(); + v0::ReceivedHeartbeats::<T>::insert(¤t_session, AuthIndex::from(0), ()); + v0::ReceivedHeartbeats::<T>::insert(¤t_session, AuthIndex::from(1), ()); + + assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!( + v1::ReceivedHeartbeats::<T>::iter().count(), + 0, + "V1 storage should be corrupted" + ); + + let state = v1::Migration::<T>::pre_upgrade().unwrap(); + let _w = v1::Migration::<T>::on_runtime_upgrade(); + v1::Migration::<T>::post_upgrade(state).unwrap(); + + assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!(v1::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!(StorageVersion::get::<Pallet<T>>(), 1); + + assert!(v1::ReceivedHeartbeats::<T>::contains(¤t_session, AuthIndex::from(0))); + assert_eq!((), v1::ReceivedHeartbeats::<T>::get(¤t_session, AuthIndex::from(1))); + }); + } +} From 4092dfba2432fbaef14b536fffcbb782e1ee6a5f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Tue, 6 Jun 2023 17:17:39 +0300 Subject: [PATCH 19/25] expose Config types + pre_upgrade and post_upgrade working fn --- frame/im-online/src/lib.rs | 8 ++-- frame/im-online/src/migration.rs | 79 +++++++++++++++++++------------- frame/im-online/src/tests.rs | 4 -- 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 198c428c53b98..177665589cf48 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -339,25 +339,25 @@ pub mod pallet { /// more accurate then the value we calculate for `HeartbeatAfter`. #[pallet::storage] #[pallet::getter(fn heartbeat_after)] - pub(crate) type HeartbeatAfter<T: Config> = StorageValue<_, T::BlockNumber, ValueQuery>; + pub(super) type HeartbeatAfter<T: Config> = StorageValue<_, T::BlockNumber, ValueQuery>; /// The current set of keys that may issue a heartbeat. #[pallet::storage] #[pallet::getter(fn keys)] - pub(crate) type Keys<T: Config> = + pub(super) type Keys<T: Config> = StorageValue<_, WeakBoundedVec<T::AuthorityId, T::MaxKeys>, ValueQuery>; /// For each session index, we keep a mapping of `SessionIndex` and `AuthIndex`. #[pallet::storage] #[pallet::getter(fn received_heartbeats)] - pub(crate) type ReceivedHeartbeats<T: Config> = + pub(super) type ReceivedHeartbeats<T: Config> = StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, ()>; /// For each session index, we keep a mapping of `ValidatorId<T>` to the /// number of blocks authored by the given authority. #[pallet::storage] #[pallet::getter(fn authored_blocks)] - pub(crate) type AuthoredBlocks<T: Config> = StorageDoubleMap< + pub(super) type AuthoredBlocks<T: Config> = StorageDoubleMap< _, Twox64Concat, SessionIndex, diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index c8a3f04e60405..1ea938a81ff3e 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -62,7 +62,17 @@ mod v0 { SessionIndex, Twox64Concat, AuthIndex, - WrapperOpaque<BoundedOpaqueNetworkState<u32, u32, T::MaxPeerInHeartbeats>>, + WrapperOpaque< + BoundedOpaqueNetworkState< + <T as Config>::MaxPeerInHeartbeats, /* XXX: use similar type because + * `MaxPeerDataEncodingSize` was removed in + * v1 */ + <T as Config>::MaxPeerInHeartbeats, /* XXX: use similar type because + * `MaxPeerDataEncodingSize` was removed in + * v1 */ + <T as Config>::MaxPeerInHeartbeats, + >, + >, >; } @@ -74,16 +84,13 @@ pub mod v1 { impl<T: Config> OnRuntimeUpgrade for Migration<T> { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<(), TryRuntimeError> { + fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> { ensure!(StorageVersion::get::<Pallet<T>>() == 0, "can only upgrade from version 0"); - log::info!( - target: TARGET, - "Migrating {} received heartbeats", - v0::ReceivedHeartbeats::<T>::iter().count() - ); + let count = v0::ReceivedHeartbeats::<T>::iter().count(); + log::info!(target: TARGET, "Migrating {} received heartbeats", count); - Ok(()) + Ok((count as u32).encode()) } fn on_runtime_upgrade() -> Weight { @@ -97,19 +104,18 @@ pub mod v1 { } let count = v0::ReceivedHeartbeats::<T>::iter().count(); - weight.saturating_accrue( - T::DbWeight::get().reads(v0::ReceivedHeartbeats::<T>::iter().count() as u64), - ); - weight.saturating_accrue( - T::DbWeight::get().writes(v0::ReceivedHeartbeats::<T>::iter().count() as u64), - ); + weight.saturating_accrue(T::DbWeight::get().reads(count as u64)); + weight.saturating_accrue(T::DbWeight::get().writes(count as u64)); - v0::ReceivedHeartbeats::<T>::translate::<_, _>( - |k: T::SessionIndex, T::AccountId, state: _| { - log::trace!(target: TARGET, "Migrated received heartbeat for {:?}...", k); - Some(()) - }, - ); + let heartbeats = v0::ReceivedHeartbeats::<T>::drain().collect::<Vec<_>>(); + for (session_index, auth_index, _) in heartbeats { + log::trace!( + target: TARGET, + "Migrated received heartbeat for {:?}...", + (session_index, auth_index) + ); + crate::ReceivedHeartbeats::<T>::insert(session_index, auth_index, ()); + } StorageVersion::new(1).put::<Pallet<T>>(); weight.saturating_add(T::DbWeight::get().writes(1)) @@ -117,13 +123,19 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn post_upgrade(state: Vec<u8>) -> DispatchResult { - ensure!(StorageVersion::get::<Pallet<T>>() == 1, "must upgrade"); + let old_received_heartbeats: u32 = + Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); + let new_received_heartbeats = crate::ReceivedHeartbeats::<T>.iter().count(); - log::info!( - target: TARGET, - "Migrated {} received heartbeats", - crate::ReceivedHeartbeats::<T>::iter().count() - ); + if new_received_heartbeats != old_received_heartbeats { + log::error!( + target: TARGET, + "migrated {} received heartbeats, expected {}", + new_received_heartbeats, + old_received_heartbeats + ); + } + ensure!(StorageVersion::get::<Pallet<T>>() == 1, "must upgrade"); Ok(()) } @@ -134,9 +146,7 @@ pub mod v1 { #[cfg(feature = "try-runtime")] mod test { use super::*; - use crate::mock::{Test as T, *}; - - use frame_support::bounded_vec; + use crate::mock::*; #[test] fn migration_works() { @@ -150,7 +160,7 @@ mod test { assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); assert_eq!( - v1::ReceivedHeartbeats::<T>::iter().count(), + crate::ReceivedHeartbeats::<T>::iter().count(), 0, "V1 storage should be corrupted" ); @@ -160,11 +170,14 @@ mod test { v1::Migration::<T>::post_upgrade(state).unwrap(); assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); - assert_eq!(v1::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!(crate::ReceivedHeartbeats::<T>::iter().count(), 2); assert_eq!(StorageVersion::get::<Pallet<T>>(), 1); - assert!(v1::ReceivedHeartbeats::<T>::contains(¤t_session, AuthIndex::from(0))); - assert_eq!((), v1::ReceivedHeartbeats::<T>::get(¤t_session, AuthIndex::from(1))); + assert!(crate::ReceivedHeartbeats::<T>::contains(¤t_session, AuthIndex::from(0))); + assert_eq!( + (), + crate::ReceivedHeartbeats::<T>::get(¤t_session, AuthIndex::from(1)) + ); }); } } diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index a51eff61e78b7..844807bc51264 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -205,8 +205,6 @@ fn late_heartbeat_and_invalid_keys_len_should_fail() { #[test] fn should_generate_heartbeats() { - use frame_support::traits::OffchainWorker; - let mut ext = new_test_ext(); let (offchain, _state) = TestOffchainExt::new(); let (pool, state) = TestTransactionPoolExt::new(); @@ -364,8 +362,6 @@ fn should_not_send_a_report_if_already_online() { #[test] fn should_handle_missing_progress_estimates() { - use frame_support::traits::OffchainWorker; - let mut ext = new_test_ext(); let (offchain, _state) = TestOffchainExt::new(); let (pool, state) = TestTransactionPoolExt::new(); From 9538acdb2e7ede15407ae8d35f3181c77b0ea2a7 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 7 Jun 2023 10:25:57 +0300 Subject: [PATCH 20/25] fix test --- frame/im-online/src/lib.rs | 4 +-- frame/im-online/src/migration.rs | 48 +++++++++++++++++++------------- frame/im-online/src/tests.rs | 2 -- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 177665589cf48..69adcc1c6bac3 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -351,7 +351,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn received_heartbeats)] pub(super) type ReceivedHeartbeats<T: Config> = - StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, ()>; + StorageDoubleMap<_, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, bool>; /// For each session index, we keep a mapping of `ValidatorId<T>` to the /// number of blocks authored by the given authority. @@ -408,7 +408,7 @@ pub mod pallet { if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, ()); + ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, true); Ok(()) } else if exists { diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 1ea938a81ff3e..c8ce098b7c8d0 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -64,10 +64,10 @@ mod v0 { AuthIndex, WrapperOpaque< BoundedOpaqueNetworkState< - <T as Config>::MaxPeerInHeartbeats, /* XXX: use similar type because + <T as Config>::MaxPeerInHeartbeats, /* XXX: used `MaxPeerInHeartbeats` because * `MaxPeerDataEncodingSize` was removed in * v1 */ - <T as Config>::MaxPeerInHeartbeats, /* XXX: use similar type because + <T as Config>::MaxPeerInHeartbeats, /* XXX: used `MaxPeerInHeartbeats` because * `MaxPeerDataEncodingSize` was removed in * v1 */ <T as Config>::MaxPeerInHeartbeats, @@ -114,7 +114,7 @@ pub mod v1 { "Migrated received heartbeat for {:?}...", (session_index, auth_index) ); - crate::ReceivedHeartbeats::<T>::insert(session_index, auth_index, ()); + crate::ReceivedHeartbeats::<T>::insert(session_index, auth_index, true); } StorageVersion::new(1).put::<Pallet<T>>(); @@ -125,9 +125,9 @@ pub mod v1 { fn post_upgrade(state: Vec<u8>) -> DispatchResult { let old_received_heartbeats: u32 = Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); - let new_received_heartbeats = crate::ReceivedHeartbeats::<T>.iter().count(); + let new_received_heartbeats = crate::ReceivedHeartbeats::<T>::iter().count(); - if new_received_heartbeats != old_received_heartbeats { + if new_received_heartbeats != old_received_heartbeats as usize { log::error!( target: TARGET, "migrated {} received heartbeats, expected {}", @@ -146,7 +146,8 @@ pub mod v1 { #[cfg(feature = "try-runtime")] mod test { use super::*; - use crate::mock::*; + use crate::mock::{new_test_ext, Runtime as T}; + use frame_support::traits::WrapperOpaque; #[test] fn migration_works() { @@ -154,30 +155,37 @@ mod test { assert_eq!(StorageVersion::get::<Pallet<T>>(), 0); // Insert some received heartbeats into the v0 storage: - let current_session = T::ValidatorSet::session_index(); - v0::ReceivedHeartbeats::<T>::insert(¤t_session, AuthIndex::from(0), ()); - v0::ReceivedHeartbeats::<T>::insert(¤t_session, AuthIndex::from(1), ()); - - assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); - assert_eq!( - crate::ReceivedHeartbeats::<T>::iter().count(), + let current_session = <T as pallet::Config>::ValidatorSet::session_index(); + v0::ReceivedHeartbeats::<T>::insert( + ¤t_session, 0, - "V1 storage should be corrupted" + WrapperOpaque(v0::BoundedOpaqueNetworkState { + peer_id: WeakBoundedVec::force_from(Default::default(), None), + external_addresses: Default::default(), + }), + ); + v0::ReceivedHeartbeats::<T>::insert( + ¤t_session, + 1, + WrapperOpaque(v0::BoundedOpaqueNetworkState { + peer_id: WeakBoundedVec::force_from(Default::default(), None), + external_addresses: Default::default(), + }), ); + assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!(crate::ReceivedHeartbeats::<T>::iter().count(), 0, "V1 storage corrupted"); + let state = v1::Migration::<T>::pre_upgrade().unwrap(); let _w = v1::Migration::<T>::on_runtime_upgrade(); v1::Migration::<T>::post_upgrade(state).unwrap(); - assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); + assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 0); assert_eq!(crate::ReceivedHeartbeats::<T>::iter().count(), 2); assert_eq!(StorageVersion::get::<Pallet<T>>(), 1); - assert!(crate::ReceivedHeartbeats::<T>::contains(¤t_session, AuthIndex::from(0))); - assert_eq!( - (), - crate::ReceivedHeartbeats::<T>::get(¤t_session, AuthIndex::from(1)) - ); + assert!(crate::ReceivedHeartbeats::<T>::contains_key(¤t_session, 0)); + assert_eq!(Some(true), crate::ReceivedHeartbeats::<T>::get(¤t_session, 1)); }); } } diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index 844807bc51264..79036760c2d42 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -118,8 +118,6 @@ fn heartbeat( id: UintAuthorityId, validators: Vec<u64>, ) -> dispatch::DispatchResult { - use frame_support::unsigned::ValidateUnsigned; - let heartbeat = Heartbeat { block_number, session_index, From 8818c1835f80fc960741357a19aa9f8139d599ef Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 7 Jun 2023 10:34:26 +0300 Subject: [PATCH 21/25] replace dummy type with ConstU32 --- frame/im-online/src/migration.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index c8ce098b7c8d0..742736db1a6eb 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -18,7 +18,10 @@ //! Storage migrations for the im-online pallet. use super::*; -use frame_support::{storage_alias, traits::OnRuntimeUpgrade}; +use frame_support::{ + storage_alias, + traits::{ConstU32, OnRuntimeUpgrade}, +}; #[cfg(feature = "try-runtime")] use frame_support::ensure; @@ -64,12 +67,8 @@ mod v0 { AuthIndex, WrapperOpaque< BoundedOpaqueNetworkState< - <T as Config>::MaxPeerInHeartbeats, /* XXX: used `MaxPeerInHeartbeats` because - * `MaxPeerDataEncodingSize` was removed in - * v1 */ - <T as Config>::MaxPeerInHeartbeats, /* XXX: used `MaxPeerInHeartbeats` because - * `MaxPeerDataEncodingSize` was removed in - * v1 */ + ConstU32<1_000>, + ConstU32<1_000>, <T as Config>::MaxPeerInHeartbeats, >, >, From eedf528720041662ffd07a80f88c9ffd5a8e5af0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 7 Jun 2023 10:42:02 +0300 Subject: [PATCH 22/25] add some comments to migration test --- frame/im-online/src/migration.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 742736db1a6eb..1cabc80f424b3 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -172,19 +172,22 @@ mod test { }), ); + // Check that the v0 storage is populated assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 2); assert_eq!(crate::ReceivedHeartbeats::<T>::iter().count(), 0, "V1 storage corrupted"); + // Perform the migration let state = v1::Migration::<T>::pre_upgrade().unwrap(); let _w = v1::Migration::<T>::on_runtime_upgrade(); v1::Migration::<T>::post_upgrade(state).unwrap(); + // Check that the v1 storage is populated and v0 storage is empty assert_eq!(v0::ReceivedHeartbeats::<T>::iter().count(), 0); assert_eq!(crate::ReceivedHeartbeats::<T>::iter().count(), 2); - assert_eq!(StorageVersion::get::<Pallet<T>>(), 1); - assert!(crate::ReceivedHeartbeats::<T>::contains_key(¤t_session, 0)); assert_eq!(Some(true), crate::ReceivedHeartbeats::<T>::get(¤t_session, 1)); + + assert_eq!(StorageVersion::get::<Pallet<T>>(), 1); }); } } From 11c4fd743a0cd34330689c6227e1c17b0339282c Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Wed, 7 Jun 2023 11:03:20 +0300 Subject: [PATCH 23/25] fix comment --- frame/im-online/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 1cabc80f424b3..31f6defdeae85 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -78,7 +78,7 @@ mod v0 { pub mod v1 { use super::*; - /// Simple migration that replaces `ReceivedHeartbeats` values with `()`. + /// Simple migration that replaces `ReceivedHeartbeats` values with `true`. pub struct Migration<T>(sp_std::marker::PhantomData<T>); impl<T: Config> OnRuntimeUpgrade for Migration<T> { From aedfdb9a0517f6b90192afdb09692494cecd17ea Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Thu, 15 Jun 2023 09:54:01 +0300 Subject: [PATCH 24/25] respond to @bkchr comments --- frame/im-online/src/lib.rs | 18 +++++------ frame/im-online/src/migration.rs | 51 +++++++++----------------------- 2 files changed, 23 insertions(+), 46 deletions(-) diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index 69adcc1c6bac3..1f9557a427293 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -389,7 +389,7 @@ pub mod pallet { // import block with such an extrinsic. #[pallet::call_index(0)] #[pallet::weight(<T as Config>::WeightInfo::validate_unsigned_and_then_heartbeat( - heartbeat.validators_len as u32, + heartbeat.validators_len, ))] pub fn heartbeat( origin: OriginFor<T>, @@ -402,13 +402,13 @@ pub mod pallet { let current_session = T::ValidatorSet::session_index(); let exists = - ReceivedHeartbeats::<T>::contains_key(¤t_session, &heartbeat.authority_index); + ReceivedHeartbeats::<T>::contains_key(current_session, heartbeat.authority_index); let keys = Keys::<T>::get(); let public = keys.get(heartbeat.authority_index as usize); if let (false, Some(public)) = (exists, public) { Self::deposit_event(Event::<T>::HeartbeatReceived { authority_id: public.clone() }); - ReceivedHeartbeats::<T>::insert(¤t_session, &heartbeat.authority_index, true); + ReceivedHeartbeats::<T>::insert(current_session, heartbeat.authority_index, true); Ok(()) } else if exists { @@ -532,22 +532,22 @@ impl<T: Config> Pallet<T> { fn is_online_aux(authority_index: AuthIndex, authority: &ValidatorId<T>) -> bool { let current_session = T::ValidatorSet::session_index(); - ReceivedHeartbeats::<T>::contains_key(¤t_session, &authority_index) || - AuthoredBlocks::<T>::get(¤t_session, authority) != 0 + ReceivedHeartbeats::<T>::contains_key(current_session, authority_index) || + AuthoredBlocks::<T>::get(current_session, authority) != 0 } /// Returns `true` if a heartbeat has been received for the authority at `authority_index` in /// the authorities series, during the current session. Otherwise `false`. pub fn received_heartbeat_in_current_session(authority_index: AuthIndex) -> bool { let current_session = T::ValidatorSet::session_index(); - ReceivedHeartbeats::<T>::contains_key(¤t_session, &authority_index) + ReceivedHeartbeats::<T>::contains_key(current_session, authority_index) } /// Note that the given authority has authored a block in the current session. fn note_authorship(author: ValidatorId<T>) { let current_session = T::ValidatorSet::session_index(); - AuthoredBlocks::<T>::mutate(¤t_session, author, |authored| *authored += 1); + AuthoredBlocks::<T>::mutate(current_session, author, |authored| *authored += 1); } pub(crate) fn send_heartbeats( @@ -794,9 +794,9 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> { // current session, they have already been processed and won't be needed // anymore. #[allow(deprecated)] - ReceivedHeartbeats::<T>::remove_prefix(&T::ValidatorSet::session_index(), None); + ReceivedHeartbeats::<T>::remove_prefix(T::ValidatorSet::session_index(), None); #[allow(deprecated)] - AuthoredBlocks::<T>::remove_prefix(&T::ValidatorSet::session_index(), None); + AuthoredBlocks::<T>::remove_prefix(T::ValidatorSet::session_index(), None); if offenders.is_empty() { Self::deposit_event(Event::<T>::AllGood); diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 31f6defdeae85..035c420f2a00a 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -18,10 +18,7 @@ //! Storage migrations for the im-online pallet. use super::*; -use frame_support::{ - storage_alias, - traits::{ConstU32, OnRuntimeUpgrade}, -}; +use frame_support::{storage_alias, traits::OnRuntimeUpgrade}; #[cfg(feature = "try-runtime")] use frame_support::ensure; @@ -29,49 +26,29 @@ use frame_support::ensure; use sp_runtime::TryRuntimeError; /// The log target. -const TARGET: &'static str = "runtime::im-online::migration::v1"; +const TARGET: &str = "runtime::im-online::migration::v1"; /// The original data layout of the im-online pallet (`ReceivedHeartbeats` storage item). mod v0 { use super::*; use frame_support::traits::WrapperOpaque; - /// A type that is the same as `OpaqueNetworkState` but with [`Vec`] replaced with - /// [`WeakBoundedVec<Limit>`] where Limit is the respective size limit - /// `PeerIdEncodingLimit` represents the size limit of the encoding of `PeerId` - /// `MultiAddrEncodingLimit` represents the size limit of the encoding of `MultiAddr` - /// `AddressesLimit` represents the size limit of the vector of peers connected - #[derive(Clone, Eq, PartialEq, Encode, Decode, MaxEncodedLen, TypeInfo)] - pub struct BoundedOpaqueNetworkState< - PeerIdEncodingLimit, - MultiAddrEncodingLimit, - AddressesLimit, - > where - PeerIdEncodingLimit: Get<u32>, - MultiAddrEncodingLimit: Get<u32>, - AddressesLimit: Get<u32>, - { + #[derive(Encode, Decode)] + pub(super) struct BoundedOpaqueNetworkState { /// PeerId of the local node in SCALE encoded. - pub peer_id: WeakBoundedVec<u8, PeerIdEncodingLimit>, + pub peer_id: Vec<u8>, /// List of addresses the node knows it can be reached as. - pub external_addresses: - WeakBoundedVec<WeakBoundedVec<u8, MultiAddrEncodingLimit>, AddressesLimit>, + pub external_addresses: Vec<Vec<u8>>, } #[storage_alias] - pub(crate) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< + pub(super) type ReceivedHeartbeats<T: Config> = StorageDoubleMap< Pallet<T>, Twox64Concat, SessionIndex, Twox64Concat, AuthIndex, - WrapperOpaque< - BoundedOpaqueNetworkState< - ConstU32<1_000>, - ConstU32<1_000>, - <T as Config>::MaxPeerInHeartbeats, - >, - >, + WrapperOpaque<BoundedOpaqueNetworkState>, >; } @@ -102,11 +79,11 @@ pub mod v1 { return weight } - let count = v0::ReceivedHeartbeats::<T>::iter().count(); - weight.saturating_accrue(T::DbWeight::get().reads(count as u64)); - weight.saturating_accrue(T::DbWeight::get().writes(count as u64)); - let heartbeats = v0::ReceivedHeartbeats::<T>::drain().collect::<Vec<_>>(); + + weight.saturating_accrue(T::DbWeight::get().reads(heartbeats.len() as u64)); + weight.saturating_accrue(T::DbWeight::get().writes(heartbeats.len() as u64)); + for (session_index, auth_index, _) in heartbeats { log::trace!( target: TARGET, @@ -159,7 +136,7 @@ mod test { ¤t_session, 0, WrapperOpaque(v0::BoundedOpaqueNetworkState { - peer_id: WeakBoundedVec::force_from(Default::default(), None), + peer_id: Default::default(), external_addresses: Default::default(), }), ); @@ -167,7 +144,7 @@ mod test { ¤t_session, 1, WrapperOpaque(v0::BoundedOpaqueNetworkState { - peer_id: WeakBoundedVec::force_from(Default::default(), None), + peer_id: Default::default(), external_addresses: Default::default(), }), ); From f8970f68a2a3152dbc5a2cdc6aa5b0bf54dbb726 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev <anton.kalyaev@gmail.com> Date: Thu, 15 Jun 2023 10:02:17 +0300 Subject: [PATCH 25/25] use BoundedOpaqueNetworkState::default intead of using default for each field --- frame/im-online/src/migration.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/frame/im-online/src/migration.rs b/frame/im-online/src/migration.rs index 035c420f2a00a..33a9bd1479d04 100644 --- a/frame/im-online/src/migration.rs +++ b/frame/im-online/src/migration.rs @@ -135,18 +135,12 @@ mod test { v0::ReceivedHeartbeats::<T>::insert( ¤t_session, 0, - WrapperOpaque(v0::BoundedOpaqueNetworkState { - peer_id: Default::default(), - external_addresses: Default::default(), - }), + WrapperOpaque(v0::BoundedOpaqueNetworkState::default()), ); v0::ReceivedHeartbeats::<T>::insert( ¤t_session, 1, - WrapperOpaque(v0::BoundedOpaqueNetworkState { - peer_id: Default::default(), - external_addresses: Default::default(), - }), + WrapperOpaque(v0::BoundedOpaqueNetworkState::default()), ); // Check that the v0 storage is populated