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

Embed runtime version as a custom section #8688

Merged
17 commits merged into from
May 12, 2021
Merged

Conversation

pepyakin
Copy link
Contributor

This changeset is a step towards #7327

This change consists of three commits (could be reviewed individually)

  • emitting a wasm custom section runtime_apis from the impl_runtime_apis! macro
  • introduction of a new runtime_version proc-macro that parses an existing RuntimeVersion declaration and emits a wasm custom section runtime_version
  • client is taught to look into those custom sections to assemble a RuntimeVersion struct before calling into the runtime.

The client side part is implemented with a built in extension, which is similar to runtime workers machinery. Now preregister_builtin_ext serves as a common point for registering such builtin extensions.

To avoid superfluous deserialization the realm of RuntimeBlob is extended a bit and now it's required for create_wasm_runtime_with_code.

As a follow up work I can highlight that we still do support Core_version runtime API equivalently. I think we should deprecate it and encourage using the new custom wasm section based solution.

This change emits a custom section from the impl_runtime_apis! proc macro.

Each implemented API will result to emitting a link section `runtime_apis`.
During linking all sections with this name will be concatenated and
placed into the final wasm binary under the same name.
This macro takes an existing `RuntimeVersion` const declaration, parses
it and emits the version information in form of a linking section.
Ultimately such a linking section will result into a custom wasm
section.
@pepyakin pepyakin added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix labels Apr 28, 2021
client/executor/common/src/runtime_blob/runtime_blob.rs Outdated Show resolved Hide resolved
client/executor/src/native_executor.rs Outdated Show resolved Hide resolved
client/executor/src/wasm_runtime.rs Outdated Show resolved Hide resolved
@pepyakin pepyakin added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Apr 28, 2021
@pepyakin
Copy link
Contributor Author

pepyakin commented May 5, 2021

Just as a status update: I am struggling to find what's the reason of the test failures in the test-linux-stable-int and cargo-check-benches. I can reproduce both locally.

The both failures have the same error: InvalidTransaction::BadProof. I assume they have the same root cause. I was focused on the test-linux-stable-int workflow, i.e. node-cli's test_sync. I managed to find out that the failure comes from the instantiation of an UncheckedExtrinsic in node-runtime. It seems that the signed payload differs from the actual.

06 00 00 8E AF 04 15 16 87 73 63 26 C9 FE A1 7E 25 FC 52 87 61 36 93 C9 12 90 9C B2 26 AA 47 94 F2 6A 48 0B 00 50 39 27 8C 04 00 00 00 09 01 00 00 01 00 00 00 6E DF 8F 18 54 00 F7 33 C0 44 FF 4B EE 9D 80 25 A9 B2 7B C9 F6 2B 55 7F 36 36 88 91 6F F5 28 3E 6E DF 8F 18 54 00 F7 33 C0 44 FF 4B EE 9D 80 25 A9 B2 7B C9 F6 2B 55 7F 36 36 88 91 6F F5 28 3E

vs.

06 00 00 8E AF 04 15 16 87 73 63 26 C9 FE A1 7E 25 FC 52 87 61 36 93 C9 12 90 9C B2 26 AA 47 94 F2 6A 48 0B 00 50 39 27 8C 04 00 00 00 09 01 00 00 02 00 00 00 6E DF 8F 18 54 00 F7 33 C0 44 FF 4B EE 9D 80 25 A9 B2 7B C9 F6 2B 55 7F 36 36 88 91 6F F5 28 3E 6E DF 8F 18 54 00 F7 33 C0 44 FF 4B EE 9D 80 25 A9 B2 7B C9 F6 2B 55 7F 36 36 88 91 6F F5 28 3E

The difference is only one byte. I haven't figured out what this data stands for yet. I will keep investigating it tomorrow.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looks good, but still some stuff that could be improved :D

bin/node-template/runtime/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 93 to 95
.filter(|cs| cs.name() == section_name)
.next()
.map(|cs| cs.payload())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.filter(|cs| cs.name() == section_name)
.next()
.map(|cs| cs.payload())
.filter_map(|cs| (cs.name() == section_name).then(|| cs.payload()))

Copy link
Contributor Author

@pepyakin pepyakin May 6, 2021

Choose a reason for hiding this comment

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

I am not entirely sure how would that work. Currently, this code takes an iterator of custom sections, leaves only sections that we are interested in, takes the first one which gives us an option and then we narrow the contents of this section to only its payload. But in the proposed version the result seems to be not an option

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.

TIL. This however misses the point, if I understand you correctly. The problem is the expression in the proposed code is typed as Iterator whereas the function returns an Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filter + next is a find though.

We can simplify the code to Iterator::find + Option::map. That will reduce the number of combinators by one.

We could use a Iterator::find_map, but that would use nested bool::then. But I consider nested combis harder to reason about and I would guess bool::then also is not among commonly used functions out there.

primitives/io/src/lib.rs Outdated Show resolved Hide resolved
primitives/io/src/lib.rs Outdated Show resolved Hide resolved
primitives/io/src/lib.rs Show resolved Hide resolved
//! will go into the custom section is parsed from the item declaration. Due to that, the macro is
//! somewhat rigid in terms of the code it accepts. There are the following considerations:
//!
//! - The `spec_name` and `impl_name` must be set by a macro-like expression. The name of the macro
Copy link
Member

Choose a reason for hiding this comment

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

Why do we enforce this?

With this macro we could switch to only supporting a static str and this macro would expand to the correct code that creates the Cow::Borrowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily, this is similar to my initial approach. I found it confusing that the actual type is RuntimeString, but the user passes an &str expression. That's why I ditched it.

This way where we try to recognize the a macro call is very rigid, but at least it seems less magicky.

Copy link
Member

Choose a reason for hiding this comment

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

What is magicky in taking a &str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Rust types are not converted under the hood typically. There are certain exceptions like Deref coercions, but in overwhelming majority of cases you have to be explicit if you want conversion to happen. You cannot even use a string literal to initialize a String!

Applying this logic to the interface in question, if we were to accept a string literal directly and wrap it into RuntimeString::Borrowed then it would look like the string type was transmogrified into RuntimeString.

It may have been OK in other cases, but here we have syntax which is identical to normal Rust. Imagine a reader who is not familiar with the internal workings of the macro. If the macro there, then suddenly rules are different and a string literal can be accepted in place of a RuntimeString defying the standard rules. One may call such a behavior "magical".

primitives/version/proc-macro/src/lib.rs Show resolved Hide resolved
Comment on lines 428 to 434
let mut version = None;

// Use the runtime blob to scan if there is any metadata embedded into the wasm binary pertaining
// to runtime version. We do it before consuming the runtime blob for creating the runtime.
if let Some(embedded_version) = read_embedded_version(&blob)? {
version = Some(embedded_version);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut version = None;
// Use the runtime blob to scan if there is any metadata embedded into the wasm binary pertaining
// to runtime version. We do it before consuming the runtime blob for creating the runtime.
if let Some(embedded_version) = read_embedded_version(&blob)? {
version = Some(embedded_version);
}
// Use the runtime blob to scan if there is any metadata embedded into the wasm binary pertaining
// to runtime version. We do it before consuming the runtime blob for creating the runtime.
let mut version = read_embedded_version(&blob)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you insist? I do realize that it is possible and is more succinct. However, IMO, the verison is there emphasizes that embedded version can be None

Copy link
Member

Choose a reason for hiding this comment

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

I will not insist, but I find it weird :D

In the end the type system is emphasizing that this can be None :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok found a compromise:

let mut version: Option<_> = read_embedded_version(&blob)?;

client/executor/src/wasm_runtime.rs Outdated Show resolved Hide resolved
client/executor/src/wasm_runtime.rs Outdated Show resolved Hide resolved
/// deserializing it.
///
/// See [`sp_maybe_compressed_blob`] for details about decompression.
pub fn uncompress_if_needed(wasm_code: &[u8]) -> Result<Self, WasmError> {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe new should do this? IMHO the name indicates that this is done on an already existing reference, while actually we create an instance with this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean because it's uncompress but returns Self? I don't think it is unheard of, even though that new_*, from_* or with_* are the most common. For example, there is TcpStream::connect which returns Self.

I also had an idea about putting it in new, but in the end I decided to go with uncompress to indicate at the call sites , mainly in sc-executor, that the code may be compressed at that point. If compression were hidden in new, it would be even harder to find out.

I do agree that it is a small thing and perhaps is not that useful, so I can tuck this logic under new if you wish so.

@pepyakin pepyakin added C7-high ❗️ and removed C1-low PR touches the given topic and has a low impact on builders. labels May 12, 2021
@pepyakin
Copy link
Contributor Author

The burn-in looks good. We are interested to get this in rather quickly to be able to upgrade polkadot validation functions with version checks.

@pepyakin
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented May 12, 2021

Trying merge.

@ghost
Copy link

ghost commented May 12, 2021

Bot will approve on the behalf of @pepyakin, since they are a team lead, in an attempt to reach the minimum approval count

@ghost ghost merged commit ef0e22e into master May 12, 2021
@ghost ghost deleted the ser-embed-runtime-version branch May 12, 2021 13:39
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
* emit a custom section from impl_runtime_apis!

This change emits a custom section from the impl_runtime_apis! proc macro.

Each implemented API will result to emitting a link section `runtime_apis`.
During linking all sections with this name will be concatenated and
placed into the final wasm binary under the same name.

* Introduce `runtime_version` proc macro

This macro takes an existing `RuntimeVersion` const declaration, parses
it and emits the version information in form of a linking section.
Ultimately such a linking section will result into a custom wasm
section.

* Parse custom wasm section for runtime version

* Apply suggestions from code review

Co-authored-by: David <dvdplm@gmail.com>

* Fix sc-executor integration tests

* Nits

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Refactor apis section deserialization

* Fix version decoding

* Reuse uncompressed value for CallInWasm

* Log on decompression error

* Simplify if

* Reexport proc-macro from sp_version

* Merge ReadRuntimeVersionExt

* Export `read_embedded_version`

* Fix test

* Simplify searching for custom section

Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@nazar-pc
Copy link
Contributor

I got curious, why sp_version::runtime_version is not used by Polkadot yet?

@bkchr
Copy link
Member

bkchr commented Apr 23, 2022

No real reason, just not yet done.

@pepyakin
Copy link
Contributor Author

Filled an issue ↑

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. A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix 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.

4 participants