-
Notifications
You must be signed in to change notification settings - Fork 735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the host <-> runtime interface machinery #3689
base: master
Are you sure you want to change the base?
Refactor the host <-> runtime interface machinery #3689
Conversation
0b53b3c
to
2745774
Compare
(Sorry, had to force push since the initial commit wasn't properly signed for some reason so I had to re-sign it; no extra changes to the PR.) |
The CI pipeline was cancelled due to failure one of the required jobs. |
…rface_refactoring
@@ -179,7 +219,7 @@ pub trait StatementStore { | |||
} | |||
|
|||
/// Remove a statement from the store by hash. | |||
fn remove(&mut self, hash: &Hash) { | |||
fn remove(&mut self, hash: PassPointerAndRead<&Hash, 32>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe std::mem::size_of::<Hash>()
instead of hard-coded 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was deliberately done to make it explicit how much data is actually being passed.
Normally, yes, I agree, you'd use size_of
to make sure that if the size of the type changes then the code doesn't need to be updated, but notice here that we never want this to change! If the size of Hash
changes then this would be a backwards incompatible change, and any historical block which uses this host function would explode.
So making this explicit is not only there to make it immediately obvious how much data is being passed, but also as an additional safeguard to protect against unintended breakage.
use codec::Codec; | ||
use hash_db::Hasher; | ||
use sp_core::storage::{ChildInfo, StateVersion, Storage}; | ||
use sp_trie::{empty_trie_root, LayoutV1, PrefixedMemoryDB}; | ||
use std::collections::{BTreeMap, HashMap}; | ||
|
||
#[cfg(feature = "std")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it intentional to not use substrate_runtime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because HashMap
is only available under std
.
In general this is still quite messy because I only converted a subset of feature = "std"
into substrate_runtime
. I had to do some extra changes related to this (and this includes changes to the file you've commented on here) in this PR to make the full_crypto
feature work again. I will be continuing with more of this refactoring in future PRs.
(Also, a side note, I think we might be able to actually remove the full_crypto
feature altogether. The whole point of it was to make the code use real crypto libraries as-is instead of calling the host functions when compiled in a no_std
environment. But once we switch to substrate_runtime
to detect whether we're compiling a runtime then compiling in no_std
will by default act like if the full_crypto
is enabled, effectively making the feature flag unnecessary.)
…rface_refactoring
FYI, I'm currently running a full sync Polkadot bult-in on my VPS with this PR to see if anything breaks; it's currently at:
So it looks like there's no breakage as it's almost finished. I'll report back again once it finishes syncing completely. |
|
||
/// Overwrite the native implementation in wasm. The native implementation always returns | ||
/// `false` and the replacement function will return always `true`. | ||
fn overwrite_native_function_implementation() -> bool { | ||
false | ||
} | ||
|
||
/// Gets an `u128` and returns this value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe it is worth keeping some test on how to marshal 128 bit integers in backward compatible way - just for reference purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... well, I got rid of this because marshaling u128
s is not supported anymore, and it's not supported because a) it used the allocator (which was totally silly, as it's a small, constant length type), and b) nothing used it anyway**. (:
If anyone complains I'll add it back in a way that doesn't use the allocator.
** - besides one host function in the benchmarking machinery, which I modified in this PR.
substrate/primitives/io/src/lib.rs
Outdated
) -> MultiRemovalResults { | ||
maybe_prefix: PassFatPointerAndRead<&[u8]>, | ||
maybe_limit: PassByCodec<Option<u32>>, | ||
maybe_cursor: PassByCodec<Option<Vec<u8>>>, //< TODO Make work or just Option<Vec<u8>>? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this TODO comment has been here before, but is it clear what it means and should it stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... no, I have no idea what it means. :P
This host function is register_only
which means that it's not used. I tried to search around, but I couldn't find any docs as to whether there are any concrete plans to actually use it. It's not in the spec and there's no open RFC for it.
It was originally added in this PR, and there's this TODO here to eventually enable it.
So either someone actually pushes it forward, or we delete it.
cc @ggwpez @KiChjang Do you guys perhaps know if there are any plans to do anything (= write an RFC) with this host function?
FYI, the burn-in with this PR applied successfully finished syncing Polkadot and is still running ingesting new blocks as they are built. |
…rface_refactoring
…rface_refactoring
…rface_refactoring
Review required! Latest push from author must always be reviewed |
This PR refactors the way the host functions are defined.
Previously the way a given type was marshalled through the host <-> runtime boundary was hardcoded for every type by the virtue of it implementing the relevant conversion traits.
This had two major consequences:
Option<u32>
was done through the SCALE codec and involved extra memory allocations!)So this PR disentangles this mess and makes the marshalling strategies explicit. This makes it much more clear how a given type in a given host function is marshalled, and also makes it possible to use different marshalling strategies for the same type in different host functions.
Before this PR you'd define a host function like this:
and after this PR you'll define it like this:
In this case the strategy for passing the
&str
is now explicitly specified (PassFatPointerAndRead
). Note that the actual API generated by this macro and the way arguments are accessed is completely unchanged! The#[runtime_interface]
machinery automatically "strips" away the marshalling strategy wrappers, so neither the body of thesay_hello_world
function here nor its callers need to be changed.This is a breaking change only for people who use
#[runtime_interface]
to define their own custom host functions.