Applying several breaking changes to the WASM interface from backlog #2199

xgreenx · 2024-09-15T09:06:17Z

Closes #1887
Closes FuelLabs/fuel-vm#797
Related #2114
Related #2133

This PR contains several small breaking changes to the WASM interface:

Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the FUEL_ALWAYS_USE_WASM.
Added host_v1 with a new peek_next_txs_size method, that accepts tx_number_limit and size_limit.
Added new variant of the return type to pass the validation result. It removes block serialization and deserialization and should improve performance.
Added a V1 execution result type that uses JSONError instead of postcard serialized error. It adds flexibility of how variants of the error can be managed. More information about it in Add tests to ensure that all error types are backward compatible fuel-vm#797. The change also moves TooManyOutputs error to the top. It shows that JSONError works as expected.
Use a WASM generated bytecode for the local testnet since we always want to use the latest version compatible with native execution.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here
- Add forward compatibility test for fuel-core 0.36.0 #2198
- Remove state_transition_bytecode.wasm from the local testnet folder #2200

rafal-ch

LGTM 👍

crates/services/upgradable-executor/src/executor.rs

acerone85 · 2024-09-16T10:26:46Z

crates/services/upgradable-executor/src/instance.rs

@@ -108,9 +167,23 @@ impl Instance {
 }

 impl<Stage> Instance<Stage> {
-    const LATEST_HOST_MODULE: &'static str = "host_v0";
+    const V0_HOST_MODULE: &'static str = "host_v0";
+    const LATEST_HOST_MODULE: &'static str = "host_v1";


In general I would avoid using a LATEST_HOST_MODULE to refer to the latest module, and have V0_HOST_MODULE and V1_HOST_MODULE instead. Imho using LATEST_HOST_MODULE is more error prone when introducing new versions. E.g. when we introduce V2, a new add_method_v1 function will need to be added and the latest peek_next_txs_size will need to be refactored to be in v1.

acerone85 · 2024-09-16T10:41:03Z

crates/services/upgradable-executor/wasm-executor/src/utils.rs

+#[cfg(not(feature = "std"))]
+#[derive(Debug, serde::Serialize, serde::Deserialize)]
+pub enum ReturnType {
+    ExecutionV0,


question for my own understanding: Why no generic parameter for ExecutionV0 in this case?

WASM executor doesn't use ExecutionV0 variant, so from its perspective it is empty. While from an upgradable perspective it has a type.

acerone85 · 2024-09-16T10:48:37Z

crates/services/upgradable-executor/wasm-executor/src/utils.rs

+    Validation(Result<Uncommitted<ValidationResult, Changes>, JSONError>),
+}
+
+/// Converts the latest execution result to the v1 execution result.


what's the difference between the latest execution result and the v1 execution result? (I thought v1 is the latest version)

It is, but the name of the variant is ExecutionV1, so I tried to highlight that.

MitchTurner

I'll finish reviewing later. I've read through most of it, but I want to give it another pass.

crates/fuel-core/src/service/adapters.rs

MitchTurner · 2024-09-16T16:48:46Z

crates/services/upgradable-executor/src/executor.rs

@@ -383,7 +386,12 @@ where
                    self.native_produce_inner(block, options, dry_run)
                }
                ExecutionStrategy::Wasm { module } => {
-                    self.wasm_produce_inner(module, block, options, dry_run)
+                    let maybe_blocks_module = self.get_module(block_version);


nit: maybe implies Option to me (probably because of this), not Result. blocks_module_res would be what I'd expect. Or call .ok on the res.

MitchTurner · 2024-09-16T16:55:49Z

crates/services/upgradable-executor/src/executor.rs

@@ -383,7 +386,12 @@ where
                    self.native_produce_inner(block, options, dry_run)
                }
                ExecutionStrategy::Wasm { module } => {
-                    self.wasm_produce_inner(module, block, options, dry_run)
+                    let maybe_blocks_module = self.get_module(block_version);


I'm having a hard time understanding what this is gaining us.

Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the FUEL_ALWAYS_USE_WASM.

Is this a backwards compatibility thing?

It is a bug, that we used WASM_BYTECODE from the binary itself instead of using value from the database(if we have any). For example, if you run the node with FUEL_ALWAYS_USE_WASM, it will use the built-in(into fuel-core binary) WASM byte code instead of the byte code that comes with the upgrade of the network. If you compiled your binary with wrong features flag, it can stop you form syncing with the network even with WASM executor

MitchTurner · 2024-09-16T17:00:03Z

crates/services/upgradable-executor/src/executor.rs

@@ -426,7 +434,12 @@ where
            match &self.execution_strategy {
                ExecutionStrategy::Native => self.native_validate_inner(block, options),
                ExecutionStrategy::Wasm { module } => {
-                    self.wasm_validate_inner(module, block, options)
+                    let maybe_blocks_module = self.get_module(block_version);


nit: same thing here

MitchTurner · 2024-09-16T17:22:03Z

version-compatibility/forkless-upgrade/src/tests_helper.rs

@@ -42,7 +44,7 @@ macro_rules! define_core_driver {
            /// This must be before the _db_dir as the drop order matters here.
            pub node: $service,
            pub _db_dir: tempfile::TempDir,
-            #[allow(dead_code)] // Only some of the clients generated by this macro are used
+            // Only some of the clients generated by this macro are used


Do you still need this comment without the allow?

MitchTurner

Okay. LGTM. Reading through everything again today it clicked what everything here is doing.

It felt like a bunch of random changes and I was going to say you should have broken it into separate PRs, but it's all kinda related.

I added a nitty comment, but other than that I'm okay with this.

MitchTurner · 2024-09-17T10:33:56Z

crates/services/upgradable-executor/src/instance.rs

@@ -53,6 +56,17 @@ use wasmtime::{
 trait CallerHelper {
    /// Writes the encoded data to the memory at the provided pointer.
    fn write(&mut self, ptr: u32, encoded: &[u8]) -> anyhow::Result<()>;
+
+    /// Peeks the next transactions from the source and returns the size of the encoded transactions.
+    fn peek_next_txs_size<Source>(


Oh. Can we specify that this is size_in_bytes or maybe just peek_next_txs_bytes? It wasn't obvious that this was bytes and not # of txs.

I guess within the wasm context it would kinda have to be bytes... but it wasn't obvious immediately to me.

…-changes # Conflicts: # bin/fuel-core/chainspec/local-testnet/state_transition_bytecode.wasm

Partial implementation of #2133. ## Linked Issues/PRs (FuelLabs/fuel-vm#821) ## Description It adds handling for the updated data struct `ConsensusParameters` (new field: `block_transaction_size_limit`). No additional logic around this new parameter is implemented in this PR, it's just a stub for further work. The new parameter is exposed via GraphQL, example: Request snippet: ```graphql consensusParameters(version: 0) { blockGasLimit blockTransactionSizeLimit } ``` Response snippet: ```json "consensusParameters": { "blockGasLimit": "30000000", "blockTransactionSizeLimit": "129024" } ``` ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [ ] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: green <[email protected]> Co-authored-by: Hannes Karppila <[email protected]>

rafal-ch

LGTM 👍

acerone85 · 2024-09-17T14:57:40Z

crates/services/upgradable-executor/src/executor.rs

+                    let maybe_blocks_module = self.get_module(block_version).ok();
+                    if let Some(blocks_module) = maybe_blocks_module {
+                        self.wasm_produce_inner(&blocks_module, block, options, dry_run)
+                    } else {
+                        self.wasm_produce_inner(module, block, options, dry_run)
+                    }


question: is this needed in the case that the node does not have the most recent version of the bytecode available, hence it will fallback to the one specified in the ExecutionStrategy::Wasm value?

It is for the case of the FUEL_ALWAYS_USE_WASM=true. We use it in the cases of tests, benchmarks, or if you just want to run WASM executor instead of the native execution.

The ideas of this change is to just use the write state transition instead of WASM version of the native state transition logic, since it can be different if you compiled the binary with different list of feature(it affects the native execution code and the WASM version as well).

@rymnc

## Version v0.36.0 ### Added - [2135](#2135): Added metrics logging for number of blocks served over the p2p req/res protocol. - [2151](#2151): Added limitations on gas used during dry_run in API. - [2188](#2188): Added the new variant `V2` for the `ConsensusParameters` which contains the new `block_transaction_size_limit` parameter. - [2163](#2163): Added runnable task for fetching block committer data. - [2204](#2204): Added `dnsaddr` resolution for TLD without suffixes. ### Changed #### Breaking - [2199](#2199): Applying several breaking changes to the WASM interface from backlog: - Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the `FUEL_ALWAYS_USE_WASM`. - Added `host_v1` with a new `peek_next_txs_size` method, that accepts `tx_number_limit` and `size_limit`. - Added new variant of the return type to pass the validation result. It removes block serialization and deserialization and should improve performance. - Added a V1 execution result type that uses `JSONError` instead of postcard serialized error. It adds flexibility of how variants of the error can be managed. More information about it in FuelLabs/fuel-vm#797. The change also moves `TooManyOutputs` error to the top. It shows that `JSONError` works as expected. - [2145](#2145): feat: Introduce time port in PoA service. - [2155](#2155): Added trait declaration for block committer data - [2142](#2142): Added benchmarks for varied forms of db lookups to assist in optimizations. - [2158](#2158): Log the public address of the signing key, if it is specified - [2188](#2188): Upgraded the `fuel-vm` to `0.57.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.57.0). ## What's Changed * chore(p2p_service): add metrics for number of blocks requested over p2p req/res protocol by @rymnc in #2135 * Weekly `cargo update` by @github-actions in #2149 * Debug V1 algorightm and use more realistic values in gas price analysis by @MitchTurner in #2129 * feat(gas_price_service): include trait declaration for block committer data by @rymnc in #2155 * Convert gas price analysis tool to CLI by @MitchTurner in #2156 * chore: add benchmarks for varied forms of lookups by @rymnc in #2142 * Add label nochangelog on weekly cargo update by @AurelienFT in #2152 * Log consensus-key signer address if specified by @acerone85 in #2158 * chore(rocks_db): move ShallowTempDir to benches crate by @rymnc in #2168 * chore(benches): conditional dropping of databases in benchmarks by @rymnc in #2170 * feat: Introduce time port in PoA service by @netrome in #2145 * Get DA costs from predefined data by @MitchTurner in #2157 * chore(shallow_temp_dir): panic if not panicking by @rymnc in #2172 * chore: Add initial CODEOWNERS file by @netrome in #2179 * Weekly `cargo update` by @github-actions in #2177 * fix(db_lookup_times): rework core logic of benchmark by @rymnc in #2159 * Add verification on transaction dry_run that they don't spend more than block gas limit by @AurelienFT in #2151 * bug: fix algorithm overflow issues by @MitchTurner in #2173 * feat(gas_price_service): create runnable task for expensive background polling for da metadata by @rymnc in #2163 * Weekly `cargo update` by @github-actions in #2197 * Fix bug with gas price factor in V1 algorithm by @MitchTurner in #2201 * Applying several breaking changes to the WASM interface from backlog by @xgreenx in #2199 * chore(p2p): dnsaddr recursive resolution by @rymnc in #2204 ## New Contributors * @acerone85 made their first contribution in #2158 **Full Changelog**: v0.35.0...v0.36.0

Applying several breaking changes to the WASM interface from backlog

c002c0c

xgreenx added the breaking A breaking api change label Sep 15, 2024

xgreenx requested a review from a team September 15, 2024 09:06

xgreenx self-assigned this Sep 15, 2024

xgreenx requested review from Dentosal and MitchTurner as code owners September 15, 2024 09:06

Make CI happy

2cfcaa5

xgreenx requested review from rafal-ch and acerone85 September 15, 2024 09:18

xgreenx and others added 3 commits September 15, 2024 23:07

Merge branch 'master' into feature/wasm-interface-breaking-changes

af91864

Remove timeout error because bytecode is not compiled

8032b7a

Use always compiled WASM bytecode for local testnet

94851b8

This was referenced Sep 16, 2024

Enforce the block size limit #2195

Merged

Add support for ConsensusParametersVersion::V2 #2188

Merged

rafal-ch previously approved these changes Sep 16, 2024

View reviewed changes

crates/services/upgradable-executor/src/executor.rs Outdated Show resolved Hide resolved

acerone85 reviewed Sep 16, 2024

View reviewed changes

Applied comments from the PR

8603fd0

xgreenx dismissed rafal-ch’s stale review via 8603fd0 September 16, 2024 16:01

Use always compiled WASM bytecode for local testnet

38fc4b1

MitchTurner reviewed Sep 16, 2024

View reviewed changes

xgreenx added 3 commits September 16, 2024 19:32

Use always compiled WASM bytecode for local testnet

8b8fbfa

Decrease the number of dry runs

46ae616

Apply comments from PR

d189266

xgreenx requested review from MitchTurner, acerone85, rafal-ch and a team September 17, 2024 08:54

MitchTurner previously approved these changes Sep 17, 2024

View reviewed changes

xgreenx added 2 commits September 17, 2024 13:50

Merge branch 'refs/heads/master' into feature/wasm-interface-breaking…

62aaabb

…-changes # Conflicts: # bin/fuel-core/chainspec/local-testnet/state_transition_bytecode.wasm

Apply suggestion

f345769

xgreenx dismissed MitchTurner’s stale review via f345769 September 17, 2024 11:54

xgreenx requested review from MitchTurner and a team September 17, 2024 11:54

MitchTurner previously approved these changes Sep 17, 2024

View reviewed changes

Do one compilation at the time

b50e75d

xgreenx dismissed MitchTurner’s stale review via b50e75d September 17, 2024 12:44

xgreenx requested a review from MitchTurner September 17, 2024 12:45

MitchTurner previously approved these changes Sep 17, 2024

View reviewed changes

Remove p2p feature since it is not used

d71d375

xgreenx dismissed MitchTurner’s stale review via d71d375 September 17, 2024 13:26

xgreenx requested a review from MitchTurner September 17, 2024 13:26

MitchTurner previously approved these changes Sep 17, 2024

View reviewed changes

Dentosal dismissed MitchTurner’s stale review via 0ae006e September 17, 2024 13:58

xgreenx requested a review from MitchTurner September 17, 2024 14:11

Dentosal approved these changes Sep 17, 2024

View reviewed changes

xgreenx enabled auto-merge (squash) September 17, 2024 14:38

rafal-ch approved these changes Sep 17, 2024

View reviewed changes

xgreenx merged commit 0d084fd into master Sep 17, 2024
33 of 35 checks passed

xgreenx deleted the feature/wasm-interface-breaking-changes branch September 17, 2024 14:50

acerone85 reviewed Sep 17, 2024

View reviewed changes

xgreenx mentioned this pull request Sep 17, 2024

Release v0.36.0 #2213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Applying several breaking changes to the WASM interface from backlog #2199

Applying several breaking changes to the WASM interface from backlog #2199

xgreenx commented Sep 15, 2024 •

edited

Loading

rafal-ch left a comment

acerone85 Sep 16, 2024 •

edited

Loading

acerone85 Sep 16, 2024

xgreenx Sep 16, 2024

acerone85 Sep 16, 2024

xgreenx Sep 16, 2024

MitchTurner left a comment

MitchTurner Sep 16, 2024

MitchTurner Sep 16, 2024

xgreenx Sep 16, 2024

MitchTurner Sep 16, 2024

MitchTurner Sep 16, 2024

MitchTurner left a comment

MitchTurner Sep 17, 2024

rafal-ch left a comment

acerone85 Sep 17, 2024

xgreenx Sep 17, 2024

Applying several breaking changes to the WASM interface from backlog #2199

Applying several breaking changes to the WASM interface from backlog #2199

Conversation

xgreenx commented Sep 15, 2024 • edited Loading

Checklist

Before requesting review

rafal-ch left a comment

Choose a reason for hiding this comment

acerone85 Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx commented Sep 15, 2024 •

edited

Loading

acerone85 Sep 16, 2024 •

edited

Loading