Emit log on Runtime Code change. #9580

tomusdrw · 2021-08-18T10:27:18Z

Related: #5047

Currently, we only emit an event in case the runtime code changes. However as described in #5047, for light clients it's difficult to track the code changes, cause reading events is not really feasible.

This PR introduces a RuntimeUpdated digest item, which is triggered on every code update (via frame_system dispatchables) or on heap_pages change (also via frame_system function, not direct state write).

This will allow the light clients to easily update the current code, based just on reading the digest items in the header.

Upgrade considerations

Since DigestItem is used in the client code as well (i.e. we decode the header digest), the nodes MUST be updated before rolling out the runtime upgrade.

gui1117

code looks good to me

gui1117 · 2021-08-18T12:41:41Z

frame/system/src/lib.rs

@@ -1128,18 +1136,18 @@ impl<T: Config> Pallet<T> {

 						Pallet::<T>::on_killed_account(who.clone());
 						Ok(DecRefStatus::Reaped)
-					},


needs to run cargo fmt.

gui1117 · 2021-08-18T12:44:46Z

primitives/runtime/src/generic/digest.rs

+	/// Currently this is triggered when:
+	/// 1. Runtime code blob is changed or
+	/// 2. `heap_pages` value is changed.
+	RuntimeUpdated,


I personally wouldn't mind the more accurate name RuntimeOrHeapPagesUpdated

RuntimeEnvironmentUpdated

bkchr · 2021-08-18T16:59:11Z

frame/system/src/lib.rs

@@ -1068,6 +1069,13 @@ impl<T: Config> Pallet<T> {
 		Account::<T>::contains_key(who)
 	}

+	fn do_set_code(code: Vec<u8>) -> DispatchResult {
+		T::OnSetCode::set_code(code)?;
+		Self::deposit_log(generic::DigestItem::RuntimeUpdated);


This will not work for Parachains.

Parachains have a special set_code implementation that will not update :code here directly. It will first queue the update and update it when it is allowed by the relay chain.

Where can I find the queue implementation? Any chance the actual storage write can go through some function that would emit the log?

Yeah we can do this.

https://github.com/paritytech/cumulus/blob/master/pallets/parachain-system/src/lib.rs#L943

I've exposed a helper function to update the storage and emit event + log. The idea is that this function will be used by parachain code implementation and is used by the default implementation of OnSetCode for unit type.

gui1117 · 2021-08-27T09:26:43Z

frame/system/src/lib.rs

@@ -298,7 +298,7 @@ pub mod pallet {

 		/// What to do if the user wants the code set to something. Just use `()` unless you are in
 		/// cumulus.


I feel like doc here or doc for the SetCode could mention about the log that needs to be deposited.

But it is hard to see where to tell about it.

tomusdrw · 2021-08-30T10:53:38Z

bot merge

ghost · 2021-08-30T10:53:41Z

Merge aborted: Checks failed for 6e99b76

pepyakin

Sorry I am late to party, just discovered this PR.

pepyakin · 2021-08-30T11:16:10Z

primitives/runtime/src/generic/digest.rs

@@ -184,6 +192,8 @@ pub enum DigestItemRef<'a, Hash: 'a> {
 	ChangesTrieSignal(&'a ChangesTrieSignal),
 	/// Any 'non-system' digest item, opaque to the native code.
 	Other(&'a Vec<u8>),
+	/// Runtime code or heap pages updated.
+	RuntimeCodeOrHeapPagesUpdated,


Right now the story about heap pages is in flux. AFAIU, there is an interest to get rid of heap pages and or change the semantics at least.

In that case, maybe it would be better to split those digests in two: runtime code updated and heap pages updated? Then, if needed we deprecate the heap pages. The alternative would be to leave it as is and then change the semantics of RuntimeCodeOrHeapPagesUpdated which seems a bit messier.

I preferred RuntimeUpdated, as it simply means "you might want to check whatever storage item concerns the runtime". The ambiguity is advantageous here.

@thiolliere @bkchr are you guys okay to revert back to RuntimeUpdated so that we can be a bit more flexible regarding what that actually means? My preference would be to have a single enum variant, especially if the heap pages one might get deprecated in the future.

I also think that once we remove HeapPages stuff, RuntimeCodeOrHeapPagesUpdated always mean RuntimeCodeUpdated.
So the transition shouldn't be that messy. Anyway it is ok to me.

Reverted back to RuntimeUpdated.

cheme · 2021-08-30T14:49:01Z

quick question before this is merged, (not sure it is a good idea): should we check at

substrate/client/db/src/lib.rs

Line 2009 in 864d096

or somewhere else after block import processing that when the digest is present in block import header, we indeed have an item for :code in the storage change payload (both are field of block import).
This way we ensure the digest is not added by the block builder without a storage update (same test could be added to cumulus).

tomusdrw · 2021-08-30T14:51:50Z

This way we ensure the digest is not added by the block builder without a storage update (same test could be added to cumulus).

Do we do that for any other digest item? Note it's not an inherent (i.e. not block author's opinion), but rather a digest item and afair the only way to add them is through the runtime execution? That means that a block with an "extra" digest item would be considered invalid, because it wouldn't be generated by the runtime during execute_block.
Or am I missing something?

cheme · 2021-08-30T14:59:29Z

I am not sure, but on a recent dropped branch I did manage some digest directly with the block builder after

substrate/client/block-builder/src/lib.rs

Line 226 in 864d096

let header = self

, and a check on block import.
Yet the check on block import would have need a cumulus implementation too.

Edit: only did work because using substrate-test-runtime, using frame/executive, this wouldn't work.

cheme · 2021-08-30T15:11:36Z

substrate/frame/executive/src/lib.rs

Line 459 in 864d096

header.digest().logs().len(),

should avoid issue indeed.

tomaka · 2021-08-31T12:40:28Z

One short coming with this PR's design is that, in case of a fork at the block height that modifies the runtime, all forks will have that log item but a light client will not be able to know whether the forks did all update to the same runtime or to different ones. Including a hash of the code in the digest item would solve this, but would make the design more restrictive overall. I don't think it's a big deal to leave that unaddressed.

tomusdrw · 2021-08-31T12:54:16Z

Including a hash of the code in the digest item would solve this,

That's actually something I wanted to add initially, but I don't think there is an efficient way to get the hash of the code without actually computing it on-chain. (i.e. no way to read storage item hash after write afaict).

Also this complicates a bit the "heap pages" problem, since in such case the code hash stays the same. Unless we make the code hash optional or hash all of the runtime environment parameters (which might once again be a bit expensive).

tomaka · 2021-09-14T09:12:31Z

Shall we merge this? Is there any reason left to hold off this PR?

bkchr · 2021-09-14T10:17:17Z

None that I'm aware of.

tomaka · 2021-09-15T08:07:00Z

bot merge

ghost · 2021-09-15T08:07:05Z

Trying merge.

apopiak · 2021-09-22T12:39:49Z

Late to the party: What about including the pre- and post-runtime version in the digest?
Edit: Probably wouldn't work well for the heap pages.

tomusdrw · 2021-09-22T21:15:45Z

I was considering including code hash in the digest (see here), but yeah the heap pages complicate that a bit - would probably be easier to have two separate digest items.
Note that the event is only an indication for the light clients, and without any extra data, the light client can query any storage items when indicated to figure out what exactly has changed - I think it's way more flexible than attempting to add any extra data directly to the digest item.

apopiak · 2021-09-23T08:07:47Z

Yeah, unfortunate that heap pages and code were lumped together.

* Emit digest item on Runtime Code changes. * Add tests. * cargo +nightly fmt --all * Rename. * Add comment. * Move generic parameter to the trait. * cargo +nightly fmt --all * Elaborate in doc. * Revert to RuntimeUpdated name * cargo +nightly fmt --all * Rename to RuntimeEnvironmentUpdated

tomusdrw added 3 commits August 18, 2021 11:52

Emit digest item on Runtime Code changes.

467aa71

Merge branch 'master' into td-runtime-digest

7d89a5b

Add tests.

38abfa5

tomusdrw added A0-please_review Pull request needs code review. E5-breaksapi C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 18, 2021

tomaka mentioned this pull request Aug 18, 2021

Backport https://github.com/paritytech/substrate/pull/9580 paritytech/smoldot#1145

Closed

gui1117 approved these changes Aug 18, 2021

View reviewed changes

tomaka mentioned this pull request Aug 18, 2021

Add a RuntimeUpdated digest item paritytech/smoldot#1146

Merged

cargo +nightly fmt --all

7123eec

bkchr suggested changes Aug 18, 2021

View reviewed changes

tomusdrw added 4 commits August 19, 2021 15:59

Rename.

947ed2c

Add comment.

33e440e

Move generic parameter to the trait.

717a680

cargo +nightly fmt --all

ed0943d

tomusdrw requested a review from bkchr August 24, 2021 08:58

bkchr approved these changes Aug 24, 2021

View reviewed changes

tomusdrw requested a review from gui1117 August 26, 2021 20:32

gui1117 reviewed Aug 27, 2021

View reviewed changes

gui1117 approved these changes Aug 27, 2021

View reviewed changes

tomusdrw added 2 commits August 30, 2021 11:53

Merge branch 'master' into td-runtime-digest

8c03efa

Elaborate in doc.

6e99b76

tomusdrw added the B7-runtimenoteworthy label Aug 30, 2021

gui1117 added the B3-apinoteworthy label Aug 30, 2021

pepyakin reviewed Aug 30, 2021

View reviewed changes

tomusdrw added 3 commits September 1, 2021 17:18

Merge branch 'master' into td-runtime-digest

3897dac

Revert to RuntimeUpdated name

4803778

cargo +nightly fmt --all

b3df42a

tomusdrw added 2 commits September 14, 2021 15:02

Merge branch 'master' into td-runtime-digest

dadea43

Rename to RuntimeEnvironmentUpdated

b403705

ghost merged commit 598d74b into master Sep 15, 2021

ghost deleted the td-runtime-digest branch September 15, 2021 08:07

github-actions bot mentioned this pull request Oct 11, 2021

Update substrate/polkadot/cumulus from v0.9.10 to v0.9.11 moonbeam-foundation/moonbeam#892

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit log on Runtime Code change. #9580

Emit log on Runtime Code change. #9580

tomusdrw commented Aug 18, 2021

gui1117 left a comment

gui1117 Aug 18, 2021

gui1117 Aug 18, 2021

kianenigma Sep 14, 2021

bkchr Aug 18, 2021

tomusdrw Aug 19, 2021 •

edited

Loading

bkchr Aug 19, 2021

tomusdrw Aug 24, 2021

gui1117 Aug 27, 2021

tomusdrw commented Aug 30, 2021

ghost commented Aug 30, 2021

pepyakin left a comment

pepyakin Aug 30, 2021

tomaka Aug 30, 2021

tomusdrw Aug 30, 2021

gui1117 Aug 30, 2021

tomusdrw Sep 1, 2021

cheme commented Aug 30, 2021

tomusdrw commented Aug 30, 2021

cheme commented Aug 30, 2021 •

edited

Loading

cheme commented Aug 30, 2021 •

edited

Loading

tomaka commented Aug 31, 2021

tomusdrw commented Aug 31, 2021

tomaka commented Sep 14, 2021

bkchr commented Sep 14, 2021

tomaka commented Sep 15, 2021

ghost commented Sep 15, 2021

apopiak commented Sep 22, 2021 •

edited

Loading

tomusdrw commented Sep 22, 2021

apopiak commented Sep 23, 2021

		@@ -298,7 +298,7 @@ pub mod pallet {

		/// What to do if the user wants the code set to something. Just use `()` unless you are in
		/// cumulus.

Emit log on Runtime Code change. #9580

Emit log on Runtime Code change. #9580

Conversation

tomusdrw commented Aug 18, 2021

Upgrade considerations

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Aug 30, 2021

ghost commented Aug 30, 2021

pepyakin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheme commented Aug 30, 2021

tomusdrw commented Aug 30, 2021

cheme commented Aug 30, 2021 • edited Loading

cheme commented Aug 30, 2021 • edited Loading

tomaka commented Aug 31, 2021

tomusdrw commented Aug 31, 2021

tomaka commented Sep 14, 2021

bkchr commented Sep 14, 2021

tomaka commented Sep 15, 2021

ghost commented Sep 15, 2021

apopiak commented Sep 22, 2021 • edited Loading

tomusdrw commented Sep 22, 2021

apopiak commented Sep 23, 2021

tomusdrw Aug 19, 2021 •

edited

Loading

cheme commented Aug 30, 2021 •

edited

Loading

cheme commented Aug 30, 2021 •

edited

Loading

apopiak commented Sep 22, 2021 •

edited

Loading