Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[frame/im-online] remove network state from heartbeats #14251

Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0073c30
[frame/im-online] remove `external_addresses` from heartbeats
melekes May 26, 2023
c8218b9
remove unused import
melekes May 29, 2023
a282042
run benchmark
melekes May 29, 2023
b60c5c4
remove external_addresses from offchain NetworkState
melekes May 29, 2023
5c7956e
add missing fn to TestNetwork
melekes May 29, 2023
97b9fd2
Revert "run benchmark"
melekes May 29, 2023
cd589f6
update weights
melekes May 29, 2023
e88d7e2
address @bkchr comments
melekes May 30, 2023
1eb8086
Merge branch 'master' into anton/7181-remove-external-addresses-from-…
melekes May 30, 2023
41bca16
remove duplicate fn
melekes May 30, 2023
619a28e
cleanup benchmarking.rs
melekes May 30, 2023
aaefc66
fix executor tests
melekes May 30, 2023
57246bf
remove peer_id from hearbeat as well
melekes May 31, 2023
8e4061d
remove MaxPeerDataEncodingSize
melekes May 31, 2023
dcc7b38
change storage value type to `()`
melekes Jun 5, 2023
81d9560
scaffold storage migration
melekes Jun 6, 2023
c822c2d
no need to check the type actually
melekes Jun 6, 2023
ace0f6e
remove unnecessary types from v0 mod
melekes Jun 6, 2023
906fa96
add a test for migration
melekes Jun 6, 2023
4092dfb
expose Config types
melekes Jun 6, 2023
9538acd
fix test
melekes Jun 7, 2023
8818c18
replace dummy type with ConstU32
melekes Jun 7, 2023
eedf528
add some comments to migration test
melekes Jun 7, 2023
11c4fd7
fix comment
melekes Jun 7, 2023
aedfdb9
respond to @bkchr comments
melekes Jun 15, 2023
f8970f6
use BoundedOpaqueNetworkState::default
melekes Jun 15, 2023
fbbbd3f
Merge branch 'master' into anton/7181-remove-external-addresses-from-…
melekes Jun 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bin/node/executor/tests/submit_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
network_state: Default::default(),
session_index: 1,
authority_index: 0,
validators_len: 0,
Expand Down
7 changes: 3 additions & 4 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion client/offchain/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@ impl AsyncApi {
#[cfg(test)]
mod tests {
use super::*;
use libp2p::PeerId;
use sc_client_db::offchain::LocalStorage;
use sc_network::{
config::MultiaddrWithPeerId, types::ProtocolName, NetworkPeers, NetworkStateInfo,
Expand Down
17 changes: 3 additions & 14 deletions frame/im-online/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ 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_runtime::{
traits::{ValidateUnsigned, Zero},
transaction_validity::TransactionSource,
Expand All @@ -33,11 +32,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,
Expand All @@ -50,13 +47,8 @@ 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 input_heartbeat = Heartbeat {
block_number: T::BlockNumber::zero(),
network_state,
session_index: 0,
authority_index: k - 1,
validators_len: keys.len() as u32,
Expand All @@ -73,15 +65,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)
Expand All @@ -90,8 +80,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();
}: {
Expand Down
118 changes: 9 additions & 109 deletions frame/im-online/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`]
Expand Down Expand Up @@ -86,15 +85,14 @@ use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{
traits::{
EstimateNextSessionRotation, Get, OneSessionHandler, ValidatorSet,
ValidatorSetWithIdentification, WrapperOpaque,
ValidatorSetWithIdentification,
},
BoundedSlice, WeakBoundedVec,
};
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_runtime::{
offchain::storage::{MutateStorageError, StorageRetrievalError, StorageValueRef},
traits::{AtLeast32BitUnsigned, Convert, Saturating, TrailingZeroInput},
Expand Down Expand Up @@ -190,7 +188,6 @@ enum OffchainErr<BlockNumber> {
AlreadyOnline(u32),
FailedSigning,
FailedToAcquireLock,
NetworkState,
SubmitTransaction,
}

Expand All @@ -206,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"),
}
}
Expand All @@ -222,8 +218,6 @@ 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,
/// Index of the current session.
pub session_index: SessionIndex,
/// An index of the authority on the list of validators.
Expand All @@ -232,64 +226,6 @@ where
pub validators_len: u32,
}

/// 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>
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>
{
fn force_from(ons: &OpaqueNetworkState) -> Self {
let peer_id = WeakBoundedVec::<_, PeerIdEncodingLimit>::force_from(
ons.peer_id.0.clone(),
Some(
"Warning: The size of the encoding of PeerId \
is bigger than expected. A runtime configuration \
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 }
}
}

/// 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,
Expand Down Expand Up @@ -331,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>;

Expand Down Expand Up @@ -408,24 +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<BoundedOpaqueNetworkState>`.
/// 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<
BoundedOpaqueNetworkState<
T::MaxPeerDataEncodingSize,
T::MaxPeerDataEncodingSize,
T::MaxPeerInHeartbeats,
>,
>,
>;
pub(crate) 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.
Expand Down Expand Up @@ -457,16 +376,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>,
Expand All @@ -485,16 +401,7 @@ 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);
ReceivedHeartbeats::<T>::insert(
&current_session,
&heartbeat.authority_index,
WrapperOpaque::from(network_state_bounded),
);
ReceivedHeartbeats::<T>::insert(&current_session, &heartbeat.authority_index, ());

Ok(())
} else if exists {
Expand Down Expand Up @@ -705,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,
network_state,
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)?;

Expand Down
1 change: 0 additions & 1 deletion frame/im-online/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 4 additions & 18 deletions frame/im-online/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -125,10 +122,6 @@ fn heartbeat(

let heartbeat = Heartbeat {
block_number,
network_state: OpaqueNetworkState {
peer_id: OpaquePeerId(vec![1]),
external_addresses: vec![],
},
session_index,
authority_index,
validators_len: validators.len() as u32,
Expand Down Expand Up @@ -252,7 +245,6 @@ fn should_generate_heartbeats() {
heartbeat,
Heartbeat {
block_number: block,
network_state: sp_io::offchain::network_state().unwrap(),
session_index: 2,
authority_index: 2,
validators_len: 3,
Expand Down Expand Up @@ -365,13 +357,7 @@ fn should_not_send_a_report_if_already_online() {

assert_eq!(
heartbeat,
Heartbeat {
block_number: 4,
network_state: sp_io::offchain::network_state().unwrap(),
session_index: 2,
authority_index: 0,
validators_len: 3,
}
Heartbeat { block_number: 4, session_index: 2, authority_index: 0, validators_len: 3 }
);
});
}
Expand Down
Loading