Separate production from dry runs in executor & Cleanup all execution paths :) #1873

MitchTurner · 2024-04-29T19:43:49Z

Closes: #1751

Separate Production from DryRun and remove ExecutionKind and ExecutionType.
Refactor code down
Remove thread_block_transaction concept because YAGNI
Remove PartialBlockComponent type
Remove intermediate inner functions
Separate block_storage_tx from BlockExecutor. This was for a couple of reasons, but the big one is that all the &mut selfs of the BlockExecutor can only be handed out one at a time and that was making the code more complicated. We could have just deconstructed self, but then we wouldn't have access to any methods. This was a clean way to separate them, and it makes more sense logically IMO. ExecutorInstance still keeps them together, so maybe there is an argument so separate them higher, but this is a good improvement in the mean time.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification 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
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

crates/services/executor/src/executor.rs

…from-prod-execution

MitchTurner · 2024-04-30T22:53:21Z

crates/services/executor/src/executor.rs

+        Ok(())
+    }
+
+    #[tracing::instrument(skip_all)]


Is this redundant? We also have a skip_all on dry_run_without_commit

Yep, plus I think maybe it is better to remove this lof from dry_run at all

bvrooman · 2024-04-30T23:57:47Z

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

+        let component = {
+            Components {
                header_to_produce: block.header,
                transactions_source: OnceTransactionsSource::new(block.transactions),
                coinbase_recipient,
                gas_price,
-            }),
+            }
        };


Suggested change

let component = {

Components {

header_to_produce: block.header,

transactions_source: OnceTransactionsSource::new(block.transactions),

coinbase_recipient,

gas_price,

}),

}

};

let component = Components {

header_to_produce: block.header,

transactions_source: OnceTransactionsSource::new(block.transactions),

coinbase_recipient,

gas_price,

};

bvrooman · 2024-05-01T00:10:43Z

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

                "The block version({}) is different from the native executor version({}). \
                The WASM executor will be used.", block_version, Self::VERSION
            );


Since this string is now duplicated, it's worth considering if it should be a constant - warning/error message are one of those things that often get overlooked during a change, so we run the risk of having these messages become out of sync.
I would say the same thing about the duplicated error string in the non-wasm error version of dry_run_inner.

bvrooman · 2024-05-01T00:22:29Z

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

+        let block_version = block.header_to_produce.state_transition_bytecode_version;
        let native_executor_version = self.native_executor_version();
        if block_version == native_executor_version {
            match &self.execution_strategy {
-                ExecutionStrategy::Native => self.native_execute_inner(block, options),
+                ExecutionStrategy::Native => self.native_dry_run_inner(block, options),
                ExecutionStrategy::Wasm { module } => {
-                    self.wasm_execute_inner(module, block, options)
+                    self.wasm_dry_run_inner(module, block, options)
                }
            }


Although this introduces some code duplication, the benefit is that it's simple to read and understand. However, I'm curious if you explored any alternative ways to implement these functions that may be a little DRYer. It is understandably challenging given the number of permuations dry run, validation, and Native/Wasm/wasm-executor enabled.

Been looking at this. I think the only part I'd really want to DRY up more would be

let block_version = block.header_to_produce.state_transition_bytecode_version; let native_executor_version = self.native_executor_version(); if block_version == native_executor_version { .. }

But honestly, that would save us like 1 line, especially since validate takes a different Block type than the other two, so to DRY all three, we'd have to get the version as a separate line.

I think it's already flat, and easy to read.

WRT to the functions doing essentially the same thing, I think they can all vary independently, so I don't think we should create an abstraction here.

I was able to find a little place to DRY up the code a bit. Added new_native_executor_instance method.

bvrooman · 2024-05-01T00:42:52Z

crates/services/executor/src/executor.rs

+    fn process_l1_and_l2_txs<T, TxSource>(
+        &self,
+        mut block: PartialFuelBlock,
+        components: &Components<TxSource>,
+        l1_transactions: Vec<CheckedTransaction>,
+        storage_tx: &mut T,
+        data: &mut ExecutionData,
+        skip_failed_txs: bool,
+    ) -> ExecutorResult<PartialFuelBlock>


Would it be feasible to break this up into something like process_l1 and process_l2, and simply call these two functions sequentially?

Yeah. It's worth looking at a bit more. It does seem like a smell to me as well. I included them together because produce and dry_run used the exact same code block, so it was a nice way to group those together. I think there probably exists a cleaner solution.

The l2 code should proabably be able to query the relayed txs from the relayer, but due to some &mut stuff I was dissuaded from doing that in one place, and left the query before generating the block_storage_tx. Now that everything is a bit more stable I'll give it another try.

… remove skipped txs bool and always skip

xgreenx

I've updated one of the tests to show the problem with block validation=)

xgreenx · 2024-05-03T15:21:05Z

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

+    }
+
+    #[cfg(feature = "wasm-executor")]
+    fn dry_run_inner<TxSource>(


I don't like duplicating the whole stack for the dry run when they are the same as production.

I think ExecutionBlockWithSource can still remain a private type of this crate just to avoid unnecessary duplication.

The change causes new 150 unnecessary lines that do exactly the same.

If you want, you can extract the source on the upper level(like dry_run_inner/produce_run_inner), and then you can pass the InputType and source as separate variables.

Okay. I just added a dry_run boolean param to some of the internal functions to branch. I don't think we need a type. I kept the external functions the same, partly because we have a bunch of test functions that won't ever be used for dry_run, so I didn't want to clutter the interfaces with the extra param.

xgreenx · 2024-05-03T15:27:13Z

crates/services/executor/src/executor.rs

+        Ok(())
+    }
+
+    #[tracing::instrument(skip_all)]


Yep, plus I think maybe it is better to remove this lof from dry_run at all

xgreenx · 2024-05-03T15:29:08Z

crates/services/executor/src/executor.rs

+            PartialFuelBlock::new(components.header_to_produce, vec![]);
+        let mut data = ExecutionData::new();
+
+        self.process_l1_txs(


Hmm, maybe it makes sense to remove the processing of L1 for now. Because we don't allow increasing of the da block height during dry runs.

xgreenx · 2024-05-03T16:15:05Z

crates/services/executor/src/executor.rs

-        self.block_st_transaction
-            .commit_changes(block_with_relayer_data_transaction.into_changes())?;
-
-        if execution_kind != ExecutionKind::DryRun && !data.found_mint {


Maybe it would be nice to add a debug assert to verify that found_mint is true.

MitchTurner · 2024-05-03T23:17:04Z

@xgreenx

I've updated one of the tests to show the problem with block validation=)

I've added a solution. The route I went will check that all the L1 transactions are included in the block before executing those txs. Is that what we want though? Does the validator care whether the txs come from L1 or L2? We want to have the producer always include them immediately, at least for now, but IDK if the validator should check that.

What do you think? Instead we could just remove the L1 tx checks completely.

crates/types/src/services/executor.rs

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

xgreenx · 2024-05-07T09:50:18Z

crates/services/executor/src/executor.rs

-
+    pub fn validate_without_commit(
+        self,
+        block: Block,


We need to pass the Block by reference and return the ValidationResult without the Block type inside. It can be follow-up=)

@bvrooman

## Version v0.27.0 ### Added - [#1898](#1898): Enforce increasing of the `Executor::VERSION` on each release. ### Changed - [#1906](#1906): Makes `cli::snapshot::Command` members public such that clients can create and execute snapshot commands programmatically. This enables snapshot execution in external programs, such as the regenesis test suite. - [#1891](#1891): Regenesis now preserves `FuelBlockMerkleData` and `FuelBlockMerkleMetadata` in the off-chain table. These tables are checked when querying message proofs. - [#1886](#1886): Use ref to `Block` in validation code - [#1876](#1876): Updated benchmark to include the worst scenario for `CROO` opcode. Also include consensus parameters in bench output. - [#1879](#1879): Return the old behaviour for the `discovery_works` test. - [#1848](#1848): Added `version` field to the `Block` and `BlockHeader` GraphQL entities. Added corresponding `version` field to the `Block` and `BlockHeader` client types in `fuel-core-client`. - [#1873](#1873): Separate dry runs from block production in executor code, remove `ExecutionKind` and `ExecutionType`, remove `thread_block_transaction` concept, remove `PartialBlockComponent` type, refactor away `inner` functions. - [#1900](#1900): Update the root README as `fuel-core run` no longer has `--chain` as an option. It has been replaced by `--snapshot`. #### Breaking - [#1894](#1894): Use testnet configuration for local testnet. - [#1894](#1894): Removed support for helm chart. - [#1910](#1910): `fuel-vm` upgraded to `0.50.0`. More information in the [changelog](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.50.0). ## What's Changed * feat: Support block and header versions gql by @bvrooman in #1848 * Updated `croo` opcode benchmark to depend on the contract size by @xgreenx in #1876 * Return the old behaviour for the `discovery_works` test by @xgreenx in #1879 * Weekly `cargo update` by @github-actions in #1880 * Separate production from dry runs in executor & Cleanup all execution paths :) by @MitchTurner in #1873 * Use ref instead of owned `Block` in validation by @MitchTurner in #1886 * Weekly `cargo update` by @github-actions in #1893 * ci: fix typos programmatically by @sdankel in #1890 * feat: Preserve message proofs post-regenesis by @bvrooman in #1891 * chore: update README fuel-core run options by @K1-R1 in #1900 * Weekly `cargo update` by @github-actions in #1903 * chore: Make snapshot command members pub accessible by @bvrooman in #1906 * Use testnet configuration for local testnet by @xgreenx in #1894 * Enforce increasing of the `Executor::VERSION` on each release by @xgreenx in #1898 * Bumped the version of the `fuel-vm` to `0.50.0` by @xgreenx in #1910 ## New Contributors * @K1-R1 made their first contribution in #1900 **Full Changelog**: v0.26.0...v0.27.0

MitchTurner added 7 commits April 29, 2024 12:43

Separate production from dry runs in executor

9444cd1

Merge branch 'master' into refactor/separate-dry-run-from-prod-execution

c6be596

Update CHANGELOG

3bac817

Replace missing import

4b4d504

Fix no features check

15a702a

Remove commented code with docs, fix feature specific import

bebc1f4

Fix doc

1fa314c

MitchTurner commented Apr 29, 2024

View reviewed changes

crates/services/executor/src/executor.rs Show resolved Hide resolved

MitchTurner added 13 commits April 29, 2024 16:04

Cleanup

1c74742

Merge remote-tracking branch 'origin' into refactor/separate-dry-run-…

2c9049a

…from-prod-execution

Move mint stuff to helper

d29c43e

Unify tx processing code

db6e8f5

Consolidate interfaces

e620e88

More cleanup

442cd73

Remove YAGNI threaded execution state tx

c69a9a8

Cleanup inner functions

780295f

Flatten code, cleanup

482cf9e

Remove PartialBlockComponent

fa37f08

Remove confusing intermediate function

884c224

Rename, appease Clippy-sama

8b62746

Remove old comments

d0a46a0

MitchTurner commented Apr 30, 2024

View reviewed changes

Fix CHANGELOG, remove comments, rename field to be more consistent

0853e80

MitchTurner marked this pull request as ready for review April 30, 2024 23:00

MitchTurner requested a review from a team April 30, 2024 23:00

bvrooman reviewed May 1, 2024

View reviewed changes

MitchTurner added 4 commits April 30, 2024 19:34

Fix formatting missed by Clippy-chan, DRY up warning code

c3f7d5c

Remove const for tracing fn

6f9b6b1

Separate storage_tx from Executor

d7434d4

Split L1 and L2 tx processing

b8cfa2c

MitchTurner added 3 commits May 1, 2024 13:14

Constrain StorageTx types

6e65e6b

Simplify mint producing function

87ebd3f

Refactor a little bit

1ede334

MitchTurner changed the title ~~Separate production from dry runs in executor~~ Separate production from dry runs in executor & Cleanup all execution paths :) May 2, 2024

MitchTurner requested a review from a team May 2, 2024 16:52

Use refs everywhere instead of consuming the partial block sometimes,…

5054e73

… remove skipped txs bool and always skip

MitchTurner self-assigned this May 2, 2024

Updated test to show the problem

042054f

xgreenx reviewed May 3, 2024

View reviewed changes

Remove redundant 'dry_run` stuff, fix test

c4fa55b

Merge branch 'master' into refactor/separate-dry-run-from-prod-execution

93e8e11

Dentosal reviewed May 6, 2024

View reviewed changes

crates/types/src/services/executor.rs Outdated Show resolved Hide resolved

Dentosal reviewed May 6, 2024

View reviewed changes

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

MitchTurner added 4 commits May 6, 2024 11:46

Improve performance of validate_block

53ed977

Remove commented-out code

5a47095

Merge branch 'master' into refactor/separate-dry-run-from-prod-execution

9d64847

Remove unused error variant

157ae57

Dentosal previously approved these changes May 7, 2024

View reviewed changes

Small improvments during review PR

03395bb

xgreenx dismissed Dentosal’s stale review via 03395bb May 7, 2024 10:09

xgreenx approved these changes May 7, 2024

View reviewed changes

xgreenx mentioned this pull request May 7, 2024

Pass the Block by reference and return the ValidationResult #1882

Closed

xgreenx enabled auto-merge (squash) May 7, 2024 10:14

xgreenx merged commit 9cc93b1 into master May 7, 2024
31 checks passed

xgreenx deleted the refactor/separate-dry-run-from-prod-execution branch May 7, 2024 10:23

xgreenx mentioned this pull request May 29, 2024

Release v0.27.0 #1912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate production from dry runs in executor & Cleanup all execution paths :) #1873

Separate production from dry runs in executor & Cleanup all execution paths :) #1873

MitchTurner commented Apr 29, 2024 •

edited

Loading

MitchTurner Apr 30, 2024

xgreenx May 3, 2024

bvrooman Apr 30, 2024

bvrooman May 1, 2024

bvrooman May 1, 2024

MitchTurner May 1, 2024

MitchTurner May 1, 2024

bvrooman May 1, 2024

MitchTurner May 1, 2024 •

edited

Loading

xgreenx left a comment •

edited

Loading

xgreenx May 3, 2024

MitchTurner May 3, 2024

xgreenx May 3, 2024

xgreenx May 3, 2024

xgreenx May 3, 2024

MitchTurner commented May 3, 2024

xgreenx May 7, 2024

Separate production from dry runs in executor & Cleanup all execution paths :) #1873

Separate production from dry runs in executor & Cleanup all execution paths :) #1873

Conversation

MitchTurner commented Apr 29, 2024 • edited Loading

Checklist

Before requesting review

After merging, notify other teams

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner May 1, 2024 • edited Loading

Choose a reason for hiding this comment

xgreenx left a comment • 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

Choose a reason for hiding this comment

MitchTurner commented May 3, 2024

Choose a reason for hiding this comment

MitchTurner commented Apr 29, 2024 •

edited

Loading

MitchTurner May 1, 2024 •

edited

Loading

xgreenx left a comment •

edited

Loading