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

Emit log on Runtime Code change. #9580

Merged
15 commits merged into from
Sep 15, 2021
Merged

Emit log on Runtime Code change. #9580

15 commits merged into from
Sep 15, 2021

Conversation

tomusdrw
Copy link
Contributor

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.

@tomusdrw 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
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

code looks good to me

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

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

Choose a reason for hiding this comment

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

needs to run cargo fmt.

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

Choose a reason for hiding this comment

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

I personally wouldn't mind the more accurate name RuntimeOrHeapPagesUpdated

Copy link
Contributor

Choose a reason for hiding this comment

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

RuntimeEnvironmentUpdated

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tomusdrw tomusdrw Aug 19, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 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.

@tomusdrw tomusdrw requested a review from bkchr August 24, 2021 08:58
@tomusdrw tomusdrw requested a review from gui1117 August 26, 2021 20:32
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Aug 30, 2021

Merge aborted: Checks failed for 6e99b76

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted back to RuntimeUpdated.

@cheme
Copy link
Contributor

cheme commented Aug 30, 2021

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

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
Copy link
Contributor Author

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
Copy link
Contributor

cheme commented Aug 30, 2021

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

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
Copy link
Contributor

cheme commented Aug 30, 2021

header.digest().logs().len(),
should avoid issue indeed.

@tomaka
Copy link
Contributor

tomaka commented Aug 31, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

tomaka commented Sep 14, 2021

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

@bkchr
Copy link
Member

bkchr commented Sep 14, 2021

None that I'm aware of.

@tomaka
Copy link
Contributor

tomaka commented Sep 15, 2021

bot merge

@ghost
Copy link

ghost commented Sep 15, 2021

Trying merge.

@ghost ghost merged commit 598d74b into master Sep 15, 2021
@ghost ghost deleted the td-runtime-digest branch September 15, 2021 08:07
@apopiak
Copy link
Contributor

apopiak commented Sep 22, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

apopiak commented Sep 23, 2021

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

h4x3rotab pushed a commit to Phala-Network/substrate that referenced this pull request Oct 30, 2021
* 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
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants