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 1 commit
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
Prev Previous commit
Next Next commit
expose Config types
+ pre_upgrade and post_upgrade working fn
  • Loading branch information
melekes committed Jun 6, 2023
commit 4092dfba2432fbaef14b536fffcbb782e1ee6a5f
8 changes: 4 additions & 4 deletions frame/im-online/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
79 changes: 46 additions & 33 deletions frame/im-online/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
>,
>,
>;
}

Expand All @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed there's a difference between iterating over keys and values. If so, when should I iterate over values? over keys?

log::info!(target: TARGET, "Migrating {} received heartbeats", count);

Ok(())
Ok((count as u32).encode())
}

fn on_runtime_upgrade() -> Weight {
Expand All @@ -97,33 +104,38 @@ pub mod v1 {
}

let count = v0::ReceivedHeartbeats::<T>::iter().count();
melekes marked this conversation as resolved.
Show resolved Hide resolved
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))
}

#[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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been >=. To make it possible that CI checks not directly start to fail when v1 and a potential v2 in the future would be part of runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about line 64?

ensure!(StorageVersion::get::<Pallet<T>>() == 0, "can only upgrade from version 0");

I thought if pre-upgrade fails, post-upgrade won't be executed either, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I also have overseen this 🙈

So, the general idea is that these migrations may stay in the runtime for quite some time and we also have tests on Polkadot that run these migrations using try runtime. Now, when the migration would be applied (which could be at any time) tests will start to fail. TLDR: The check should be removed from pre_upgrade and changed in post as said above.


Ok(())
}
Expand All @@ -134,9 +146,7 @@ pub mod v1 {
#[cfg(feature = "try-runtime")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't set the try-runtime feature in CI, so this will never be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about try-runtime CLI cmd? Or it's not used? I've seen at least 15 migration.rs files containing that flag.

mod test {
use super::*;
use crate::mock::{Test as T, *};

use frame_support::bounded_vec;
use crate::mock::*;

#[test]
fn migration_works() {
Expand All @@ -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"
);
Expand All @@ -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(&current_session, AuthIndex::from(0)));
assert_eq!((), v1::ReceivedHeartbeats::<T>::get(&current_session, AuthIndex::from(1)));
assert!(crate::ReceivedHeartbeats::<T>::contains(&current_session, AuthIndex::from(0)));
assert_eq!(
(),
crate::ReceivedHeartbeats::<T>::get(&current_session, AuthIndex::from(1))
);
});
}
}
4 changes: 0 additions & 4 deletions frame/im-online/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down