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
29 changes: 21 additions & 8 deletions frame/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,14 @@ pub type ConsumedWeight = PerDispatchClass<Weight>;
pub use pallet::*;

/// Do something when we should be setting the code.
pub trait SetCode {
pub trait SetCode<T: Config> {
/// Set the code to the given blob.
fn set_code(code: Vec<u8>) -> DispatchResult;
}

impl SetCode for () {
impl<T: Config> SetCode<T> for () {
fn set_code(code: Vec<u8>) -> DispatchResult {
storage::unhashed::put_raw(well_known_keys::CODE, &code);
<Pallet<T>>::update_code_in_storage(&code)?;
Ok(())
}
}
Expand Down Expand Up @@ -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.

type OnSetCode: SetCode;
type OnSetCode: SetCode<Self>;
}

#[pallet::pallet]
Expand Down Expand Up @@ -350,21 +350,24 @@ pub mod pallet {
/// - 1 storage write.
/// - Base Weight: 1.405 µs
/// - 1 write to HEAP_PAGES
/// - 1 digest item
/// # </weight>
#[pallet::weight((T::SystemWeightInfo::set_heap_pages(), DispatchClass::Operational))]
pub fn set_heap_pages(origin: OriginFor<T>, pages: u64) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
storage::unhashed::put_raw(well_known_keys::HEAP_PAGES, &pages.encode());
Self::deposit_log(generic::DigestItem::RuntimeCodeOrHeapPagesUpdated);
Ok(().into())
}

/// Set the new runtime code.
///
/// # <weight>
/// - `O(C + S)` where `C` length of `code` and `S` complexity of `can_set_code`
/// - 1 storage write (codec `O(C)`).
/// - 1 call to `can_set_code`: `O(S)` (calls `sp_io::misc::runtime_version` which is
/// expensive).
/// - 1 storage write (codec `O(C)`).
/// - 1 digest item.
/// - 1 event.
/// The weight of this function is dependent on the runtime, but generally this is very
/// expensive. We will treat this as a full block.
Expand All @@ -373,9 +376,7 @@ pub mod pallet {
pub fn set_code(origin: OriginFor<T>, code: Vec<u8>) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
Self::can_set_code(&code)?;

T::OnSetCode::set_code(code)?;
Self::deposit_event(Event::CodeUpdated);
Ok(().into())
}

Expand All @@ -384,6 +385,7 @@ pub mod pallet {
/// # <weight>
/// - `O(C)` where `C` length of `code`
/// - 1 storage write (codec `O(C)`).
/// - 1 digest item.
/// - 1 event.
/// The weight of this function is dependent on the runtime. We will treat this as a full
/// block. # </weight>
Expand All @@ -394,7 +396,6 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
T::OnSetCode::set_code(code)?;
Self::deposit_event(Event::CodeUpdated);
Ok(().into())
}

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

/// Write code to the storage and emit related events and digest items.
///
/// Note this function almost never should be used directly. It is exposed
/// for `OnSetCode` implementations that defer actual code being written to
/// the storage (for instance in case of parachains).
pub fn update_code_in_storage(code: &[u8]) -> DispatchResult {
storage::unhashed::put_raw(well_known_keys::CODE, code);
Self::deposit_log(generic::DigestItem::RuntimeCodeOrHeapPagesUpdated);
Self::deposit_event(Event::CodeUpdated);
Ok(())
}

/// Increment the reference counter on an account.
#[deprecated = "Use `inc_consumers` instead"]
pub fn inc_ref(who: &T::AccountId) {
Expand Down
22 changes: 22 additions & 0 deletions frame/system/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,11 +390,24 @@ fn set_code_checks_works() {
ext.execute_with(|| {
let res = System::set_code(RawOrigin::Root.into(), vec![1, 2, 3, 4]);

assert_runtime_updated_digest(if res.is_ok() { 1 } else { 0 });
assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res);
});
}
}

fn assert_runtime_updated_digest(num: usize) {
assert_eq!(
System::digest()
.logs
.into_iter()
.filter(|item| *item == generic::DigestItem::RuntimeCodeOrHeapPagesUpdated)
.count(),
num,
"Incorrect number of Runtime Updated digest items",
);
}

#[test]
fn set_code_with_real_wasm_blob() {
let executor = substrate_test_runtime_client::new_native_executor();
Expand Down Expand Up @@ -478,3 +491,12 @@ fn extrinsics_root_is_calculated_correctly() {
assert_eq!(ext_root, *header.extrinsics_root());
});
}

#[test]
fn runtime_updated_digest_emitted_when_heap_pages_changed() {
new_test_ext().execute_with(|| {
System::initialize(&1, &[0u8; 32].into(), &Default::default(), InitKind::Full);
System::set_heap_pages(RawOrigin::Root.into(), 5).unwrap();
assert_runtime_updated_digest(1);
});
}
17 changes: 17 additions & 0 deletions primitives/runtime/src/generic/digest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ pub enum DigestItem<Hash> {

/// Some other thing. Unsupported and experimental.
Other(Vec<u8>),

/// An indication for the light clients that the runtime execution
/// environment is updated.
///
/// Currently this is triggered when:
/// 1. Runtime code blob is changed or
/// 2. `heap_pages` value is changed.
RuntimeCodeOrHeapPagesUpdated,
}

/// Available changes trie signals.
Expand Down Expand Up @@ -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.

}

/// Type of the digest item. Used to gain explicit control over `DigestItem` encoding
Expand All @@ -199,6 +209,7 @@ pub enum DigestItemType {
Seal = 5,
PreRuntime = 6,
ChangesTrieSignal = 7,
RuntimeCodeOrHeapPagesUpdated = 8,
}

/// Type of a digest item that contains raw data; this also names the consensus engine ID where
Expand All @@ -225,6 +236,7 @@ impl<Hash> DigestItem<Hash> {
Self::Seal(ref v, ref s) => DigestItemRef::Seal(v, s),
Self::ChangesTrieSignal(ref s) => DigestItemRef::ChangesTrieSignal(s),
Self::Other(ref v) => DigestItemRef::Other(v),
Self::RuntimeCodeOrHeapPagesUpdated => DigestItemRef::RuntimeCodeOrHeapPagesUpdated,
}
}

Expand Down Expand Up @@ -322,6 +334,8 @@ impl<Hash: Decode> Decode for DigestItem<Hash> {
DigestItemType::ChangesTrieSignal =>
Ok(Self::ChangesTrieSignal(Decode::decode(input)?)),
DigestItemType::Other => Ok(Self::Other(Decode::decode(input)?)),
DigestItemType::RuntimeCodeOrHeapPagesUpdated =>
Ok(Self::RuntimeCodeOrHeapPagesUpdated),
}
}
}
Expand Down Expand Up @@ -457,6 +471,9 @@ impl<'a, Hash: Encode> Encode for DigestItemRef<'a, Hash> {
DigestItemType::Other.encode_to(&mut v);
val.encode_to(&mut v);
},
Self::RuntimeCodeOrHeapPagesUpdated => {
DigestItemType::RuntimeCodeOrHeapPagesUpdated.encode_to(&mut v);
},
}

v
Expand Down