Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
37 changes: 27 additions & 10 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 @@ -296,9 +296,13 @@ pub mod pallet {
#[pallet::constant]
type SS58Prefix: Get<u16>;

/// What to do if the user wants the code set to something. Just use `()` unless you are in
/// cumulus.
type OnSetCode: SetCode;
/// What to do if the runtime wants to change the code to something new.
///
/// The default (`()`) implementation is responsible for setting the correct storage
/// entry and emitting corresponding event and log item. (see [`update_code_in_storage`]).
/// It's unlikely that this needs to be customized, unless you are writing a parachain using
/// `Cumulus`, where the actual code change is deferred.
type OnSetCode: SetCode<Self>;
}

#[pallet::pallet]
Expand Down Expand Up @@ -350,21 +354,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 +380,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 +389,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 +400,6 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
T::OnSetCode::set_code(code)?;
Self::deposit_event(Event::CodeUpdated);
Ok(().into())
}

Expand Down Expand Up @@ -1071,6 +1076,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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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