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

melekes · 2023-05-29T08:40:23Z

Users should use DHT for discovering new nodes. The reason for adding external addresses was unstable work of authority discovery (see #2719), which is now stable. Hence we can safely remove external_addresses.

Refs paritytech/polkadot-sdk#646
polkadot companion: paritytech/polkadot#7309

Users should use DHT for discovering new nodes. The reason for adding external addresses was unstable work of authority discovery (see paritytech#2719), which is now stable. Hence we can safely remove `external_addresses`. Refs https://github.com/paritytech/polkadot/issues/7181

This reverts commit a282042.

dmitry-markin · 2023-05-29T12:23:42Z

They are not used for discovery, but are we sure that they are not used for something else?

dmitry-markin

LGTM code-wise, but I don't know if external addresses in heartbeats are used for anything, and it's safe to remove them.

melekes · 2023-05-29T12:41:09Z

They are not used for discovery, but are we sure that they are not used for something else?

I'm going to refer to this comment: paritytech/polkadot-sdk#646. Not merging this in the closest release is also fine with me (to give developers some time to upgrade, but we should probably notify them somehow...)

altonen

Looks good to me as well but I don't know if these are used for anything else so I'd wait for someone else's approval

primitives/core/src/offchain/mod.rs

client/offchain/src/api.rs

frame/im-online/src/lib.rs

+ pre_upgrade and post_upgrade working fn

melekes · 2023-06-07T07:29:13Z

could @bkchr or @paritytech/frame-coders please take a look at the migration https://github.com/paritytech/substrate/pull/14251/files#diff-e8faeeb1f7c80155f21fe0f315f67a539ce51f8c08bed0d90130aae3cca6627c before we merge this?

melekes · 2023-06-07T07:36:23Z

frame/im-online/src/migration.rs

+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
+			ensure!(StorageVersion::get::<Pallet<T>>() == 0, "can only upgrade from version 0");
+
+			let count = v0::ReceivedHeartbeats::<T>::iter().count();


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

frame/im-online/src/migration.rs

bkchr · 2023-06-14T15:52:12Z

frame/im-online/src/migration.rs

+const TARGET: &'static str = "runtime::im-online::migration::v1";
+
+/// The original data layout of the im-online pallet (`ReceivedHeartbeats` storage item).
+mod v0 {


These types can just be moved to v1.

I like that v0 emphasises previous version, which no longer exists. Also, I copied it from another frame pallet -> better to stay consistent I think

frame/im-online/src/migration.rs

intead of using default for each field

…heartbeats

melekes · 2023-06-15T09:36:41Z

bot merge

paritytech-processbot · 2023-06-15T09:36:49Z

Error: Github API says paritytech/polkadot#7309 is not mergeable

melekes · 2023-06-15T09:42:28Z

bot merge

bkchr · 2023-06-15T10:30:14Z

frame/im-online/src/migration.rs

+					old_received_heartbeats
+				);
+			}
+			ensure!(StorageVersion::get::<Pallet<T>>() == 1, "must upgrade");


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.

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?

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.

bkchr · 2023-06-15T10:30:44Z

frame/im-online/src/migration.rs

+}
+
+#[cfg(test)]
+#[cfg(feature = "try-runtime")]


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

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

modify ensure in post_upgrade to allow for future upgrades refs paritytech#14251 (comment)

* [frame/im-online] remove ensure from pre_upgrade modify ensure in post_upgrade to allow for future upgrades refs #14251 (comment) * cargo fmt

@bkchr

) * [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 paritytech#2719), which is now stable. Hence we can safely remove `external_addresses`. Refs https://github.com/paritytech/polkadot/issues/7181 * remove unused import * run benchmark * remove external_addresses from offchain NetworkState * add missing fn to TestNetwork * Revert "run benchmark" This reverts commit a282042. * update weights * address @bkchr comments * remove duplicate fn * cleanup benchmarking.rs * fix executor tests * remove peer_id from hearbeat as well paritytech#14251 (comment) * remove MaxPeerDataEncodingSize * change storage value type to `()` paritytech#14251 (comment) * scaffold storage migration * no need to check the type actually * remove unnecessary types from v0 mod * add a test for migration * expose Config types + pre_upgrade and post_upgrade working fn * fix test * replace dummy type with ConstU32 * add some comments to migration test * fix comment * respond to @bkchr comments * use BoundedOpaqueNetworkState::default intead of using default for each field

* [frame/im-online] remove ensure from pre_upgrade modify ensure in post_upgrade to allow for future upgrades refs paritytech#14251 (comment) * cargo fmt

melekes marked this pull request as ready for review May 29, 2023 08:40

melekes requested review from a team May 29, 2023 08:40

melekes self-assigned this May 29, 2023

melekes added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes labels May 29, 2023

remove unused import

c8218b9

melekes added the D2-breaksapi label May 29, 2023

melekes added 2 commits May 29, 2023 13:17

run benchmark

a282042

remove external_addresses from offchain NetworkState

b60c5c4

melekes added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 29, 2023

melekes added 3 commits May 29, 2023 13:29

add missing fn to TestNetwork

5c7956e

Revert "run benchmark"

97b9fd2

This reverts commit a282042.

update weights

cd589f6

melekes requested a review from a team May 29, 2023 11:51

dmitry-markin approved these changes May 29, 2023

View reviewed changes

dmitry-markin requested a review from a team May 29, 2023 12:44

altonen reviewed May 29, 2023

View reviewed changes

bkchr suggested changes May 29, 2023

View reviewed changes

primitives/core/src/offchain/mod.rs Outdated Show resolved Hide resolved

client/offchain/src/api.rs Outdated Show resolved Hide resolved

frame/im-online/src/lib.rs Outdated Show resolved Hide resolved

address @bkchr comments

e88d7e2

melekes added 5 commits June 6, 2023 12:29

no need to check the type actually

c822c2d

remove unnecessary types from v0 mod

ace0f6e

add a test for migration

906fa96

expose Config types

4092dfb

+ pre_upgrade and post_upgrade working fn

fix test

9538acd

replace dummy type with ConstU32

8818c18

melekes commented Jun 7, 2023

View reviewed changes

melekes added 2 commits June 7, 2023 10:42

add some comments to migration test

eedf528

fix comment

11c4fd7

bkchr reviewed Jun 14, 2023

View reviewed changes

melekes added 3 commits June 15, 2023 09:54

respond to @bkchr comments

aedfdb9

use BoundedOpaqueNetworkState::default

f8970f6

intead of using default for each field

Merge branch 'master' into anton/7181-remove-external-addresses-from-…

fbbbd3f

…heartbeats

paritytech-processbot bot merged commit 51b2f0e into paritytech:master Jun 15, 2023

melekes deleted the anton/7181-remove-external-addresses-from-heartbeats branch June 15, 2023 09:50

bkchr reviewed Jun 15, 2023

View reviewed changes

melekes added a commit to melekes/substrate that referenced this pull request Jun 16, 2023

[frame/im-online] remove ensure from pre_upgrade

8149804

modify ensure in post_upgrade to allow for future upgrades refs paritytech#14251 (comment)

melekes mentioned this pull request Jun 16, 2023

[frame/im-online] remove ensure from pre_upgrade #14401

Merged

liamaharon mentioned this pull request Jul 24, 2023

ci: check runtime migrations #14618

Merged

vstakhov mentioned this pull request Aug 1, 2023

High rate of dial failures / peers not connected for request/response on Versi paritytech/polkadot-sdk#589

Open

ukint-vs mentioned this pull request Dec 19, 2023

feat: update substrate to v1.0.0 gear-tech/gear#3608

Merged

dnjscksdn98 mentioned this pull request Jan 12, 2024

NODE-65, deps: polkadot sdk migration bifrost-platform/bifrost-node#36

Merged

13 tasks

surangap mentioned this pull request May 29, 2024

Upgrade Substrate to v1.0.0 futureversecom/trn-seed#821

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

melekes commented May 29, 2023 •

edited

Loading

dmitry-markin commented May 29, 2023

dmitry-markin left a comment

melekes commented May 29, 2023

altonen left a comment

melekes commented Jun 7, 2023

melekes Jun 7, 2023

bkchr Jun 14, 2023

melekes Jun 15, 2023 •

edited

Loading

melekes commented Jun 15, 2023

paritytech-processbot bot commented Jun 15, 2023

melekes commented Jun 15, 2023

bkchr Jun 15, 2023

melekes Jun 15, 2023

bkchr Jun 15, 2023

bkchr Jun 15, 2023

melekes Jun 15, 2023

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

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

Conversation

melekes commented May 29, 2023 • edited Loading

dmitry-markin commented May 29, 2023

dmitry-markin left a comment

Choose a reason for hiding this comment

melekes commented May 29, 2023

altonen left a comment

Choose a reason for hiding this comment

melekes commented Jun 7, 2023

melekes Jun 7, 2023

Choose a reason for hiding this comment

bkchr Jun 14, 2023

Choose a reason for hiding this comment

melekes Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

melekes commented Jun 15, 2023

paritytech-processbot bot commented Jun 15, 2023

melekes commented Jun 15, 2023

bkchr Jun 15, 2023

Choose a reason for hiding this comment

melekes Jun 15, 2023

Choose a reason for hiding this comment

bkchr Jun 15, 2023

Choose a reason for hiding this comment

bkchr Jun 15, 2023

Choose a reason for hiding this comment

melekes Jun 15, 2023

Choose a reason for hiding this comment

melekes commented May 29, 2023 •

edited

Loading

melekes Jun 15, 2023 •

edited

Loading