Handle FTI messages in executor #1787

Voxelot · 2024-03-29T07:12:54Z

Closes #1626

todo:

tests / requirements -

query the status of failed forced transactions
prevent non-chargeable txs from being included
handle duplicate relayed txs ids (what should gql api report if there are duplicate FTIs?)
- ~~NOTE: The current solution is each duplicate gets added to the skipped_transactions for the block. We might want to find a different solution if UX is bad--but it's findable at least currently.~~ Edit: We are now reporting duplicate txs just like any other execution error for relayed txs. Since they will all have unique nonces in practice, all of their ids will be unique.
ensure that skipped fti's due to invalid state (ie. missing utxo inputs) have their status reported
ensure that sentry nodes also include the status of failed FTIs without failing the processing of the block
ensure users aren't double charged for gas payed on l1

crates/fuel-core/src/graphql_api/worker_service/tests.rs

tests/tests/relayer.rs

crates/fuel-core/src/executor.rs

xgreenx · 2024-04-12T08:12:25Z

crates/client/assets/schema.sdl

@@ -850,6 +850,7 @@ type Query {
 	messages(owner: Address, first: Int, after: String, last: Int, before: String): MessageConnection!
 	messageProof(transactionId: TransactionId!, nonce: Nonce!, commitBlockId: BlockId, commitBlockHeight: U32): MessageProof
 	messageStatus(nonce: Nonce!): MessageStatus!
+	status(id: RelayedTransactionId!): RelayedTransactionStatus


The name is still not updated=D

xgreenx · 2024-04-12T08:16:17Z

crates/fuel-core/src/query/relayed_tx.rs

@@ -0,0 +1 @@
+


It seems you can remove this file

xgreenx · 2024-04-12T08:24:17Z

crates/services/executor/src/executor.rs

+                    Err(err) => {
+                        execution_data
+                            .skipped_transactions
+                            .push((tx_id, err.clone()));
+                    }


We only need to add transaction into skipped transaction in production mode. In other cases we need to fail execution of the whole block since it is invalid

xgreenx · 2024-04-12T08:28:04Z

crates/services/executor/src/executor.rs

+        consensus_params: &ConsensusParameters,
+    ) -> Result<CheckedTransaction, ForcedTransactionFailure> {
+        let parsed_tx = Self::parse_tx_bytes(&relayed_tx)?;
+        let checked_tx =


What is the reason to put 2-3 lines of code into a separate function? It creates code duplication like below:

The purpose was to clean up the code. It goes a long way when you see a function called validate_forced_tx to be able to immediately read, line-by-line, each of the validations that are happening and not get distracted with the implementation. I didn't do it to reduce the numbers of lines. And "is a valid variant" is separate from "the max gas is correct" when it comes to requirements, so I wrote them as separate, readable requirements.

xgreenx · 2024-04-12T08:31:33Z

crates/services/executor/src/executor.rs

        Ok(())
    }

+    fn predicates_on_tx_are_valid(


Hmm, into_checked perfromces all checks inside. so we can remove check_predicate

xgreenx · 2024-04-12T08:32:50Z

crates/services/executor/src/executor.rs

+        let checked_tx =
+            Self::get_checked_tx(parsed_tx, *header.height(), consensus_params)?;
+        Self::tx_is_valid_variant(&checked_tx)?;
+        Self::relayed_tx_claimed_enough_max_gas(&checked_tx, &relayed_tx)?;


We need to do this check even before Self::get_checked_tx, because into_checked may be heavy in the case of Create transactions.

xgreenx · 2024-04-12T08:40:12Z

crates/fuel-core/src/executor.rs

+            let expected = "Insufficient max gas.";
+            assert!(actual.starts_with(expected));


Instead of using hardcoded strings you can generate them from corresponding error variant.

assert_eq!(ForcedTransactionFailure::MaxGas.to_string(), actual);

The same applies to all tests

I should have been clearer:

assert!(actual.starts_with(expected));

Is not an assert_eq it's checking that the start of the message is right.

I can rename the expected to something better.

xgreenx · 2024-04-12T08:42:11Z

crates/fuel-core/src/executor.rs

+            // let mut bad_tx = tx.clone();
+            // bad_relayed_tx.set_serialized_transaction(bad_tx_bytes);


I think we can remove it=)

xgreenx

Thank you @MitchTurner for finishing this PR!=) Looks cleaner than the initial variant=)

And thank you one more time for many tests

crates/fuel-core/src/executor.rs

@Dentosal

## Version v0.25.0 ### Fixed - [#1821](#1821): Can handle missing tables in snapshot. - [#1814](#1814): Bugfix: the `iter_all_by_prefix` was not working for all tables. The change adds a `Rust` level filtering. ### Added - [#1831](#1831): Included the total gas and fee used by transaction into `TransactionStatus`. - [#1821](#1821): Propagate shutdown signal to (re)genesis. Also add progress bar for (re)genesis. - [#1813](#1813): Added back support for `/health` endpoint. - [#1799](#1799): Snapshot creation is now concurrent. - [#1811](#1811): Regenesis now preserves old blocks and transactions for GraphQL API. ### Changed - [#1833](#1833): Regenesis of `SpentMessages` and `ProcessedTransactions`. - [#1830](#1830): Use versioning enum for WASM executor input and output. - [#1816](#1816): Updated the upgradable executor to fetch the state transition bytecode from the database when the version doesn't match a native one. This change enables the WASM executor in the "production" build and requires a `wasm32-unknown-unknown` target. - [#1812](#1812): Follow-up PR to simplify the logic around parallel snapshot creation. - [#1809](#1809): Fetch `ConsensusParameters` from the database - [#1808](#1808): Fetch consensus parameters from the provider. #### Breaking - [#1826](#1826): The changes make the state transition bytecode part of the `ChainConfig`. It guarantees the state transition's availability for the network's first blocks. The change has many minor improvements in different areas related to the state transition bytecode: - The state transition bytecode lies in its own file(`state_transition_bytecode.wasm`) along with the chain config file. The `ChainConfig` loads it automatically when `ChainConfig::load` is called and pushes it back when `ChainConfig::write` is called. - The `fuel-core` release bundle also contains the `fuel-core-wasm-executor.wasm` file of the corresponding executor version. - The regenesis process now considers the last block produced by the previous network. When we create a (re)genesis block of a new network, it has the `height = last_block_of_old_netowkr + 1`. It continues the old network and doesn't overlap blocks(before, we had `old_block.height == new_genesis_block.hegiht`). - Along with the new block height, the regenesis process also increases the state transition bytecode and consensus parameters versions. It guarantees that a new network doesn't use values from the previous network and allows us not to migrate `StateTransitionBytecodeVersions` and `ConsensusParametersVersions` tables. - Added a new CLI argument, `native-executor-version,` that allows overriding of the default version of the native executor. It can be useful for side rollups that have their own history of executor upgrades. - Replaced: ```rust let file = std::fs::File::open(path)?; let mut snapshot: Self = serde_json::from_reader(&file)?; ``` with a: ```rust let mut json = String::new(); std::fs::File::open(&path) .with_context(|| format!("Could not open snapshot file: {path:?}"))? .read_to_string(&mut json)?; let mut snapshot: Self = serde_json::from_str(json.as_str())?; ``` because it is 100 times faster for big JSON files. - Updated all tests to use `Config::local_node_*` instead of working with the `SnapshotReader` directly. It is the preparation of the tests for the futures bumps of the `Executor::VERSION`. When we increase the version, all tests continue to use `GenesisBlock.state_transition_bytecode = 0` while the version is different, which forces the usage of the WASM executor, while for tests, we still prefer to test native execution. The `Config::local_node_*` handles it and forces the executor to use the native version. - Reworked the `build.rs` file of the upgradable executor. The script now caches WASM bytecode to avoid recompilation. Also, fixed the issue with outdated WASM bytecode. The script reacts on any modifications of the `fuel-core-wasm-executor` and forces recompilation (it is why we need the cache), so WASM bytecode always is actual now. - [#1822](#1822): Removed support of `Create` transaction from debugger since it doesn't have any script to execute. - [#1822](#1822): Use `fuel-vm 0.49.0` with new transactions types - `Upgrade` and `Upload`. Also added `max_bytecode_subsections` field to the `ConsensusParameters` to limit the number of bytecode subsections in the state transition bytecode. - [#1816](#1816): Updated the upgradable executor to fetch the state transition bytecode from the database when the version doesn't match a native one. This change enables the WASM executor in the "production" build and requires a `wasm32-unknown-unknown` target. ### Before requesting review - [x] I have reviewed the code myself ### After merging, notify other teams - [x] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [x] [Sway compiler](https://github.com/FuelLabs/sway/) - [x] DevOps ## What's Changed * Add PR template by @Dentosal in #1806 * feat: Parallellize snapshot creation by @segfault-magnet in #1799 * Follow-up PR to simplify the logic around parallel snapshot creation by @xgreenx in #1812 * Bugfix: the `iter_all_by_prefix` was not working for all tables by @xgreenx in #1814 * Added back support for `/health` endpoint by @xgreenx in #1813 * Fetch consensus parameters from the provider by @xgreenx in #1808 * Fetch `ConsensusParameters` from the database by @xgreenx in #1809 * Handle FTI messages in executor by @Voxelot in #1787 * Use state transition bytecode from the database by @xgreenx in #1816 * feat: (re)genesis graceful shutdown by @segfault-magnet in #1821 * Use `fuel-vm 0.49.0` with new transactions types by @xgreenx in #1822 * Included the total gas and fee into `TransactionStatus` by @xgreenx in #1831 * Use versioning enum for WASM executor input and output by @xgreenx in #1830 * Support upgradability of the consensus parameters and state transition bytecode in genesis by @xgreenx in #1826 * Store old blocks and txs after regenesis by @Dentosal in #1811 * Regenesis of `SpentMessages` and `ProcessedTransactions` by @xgreenx in #1833 **Full Changelog**: v0.24.2...v0.25.0

Voxelot and others added 2 commits April 2, 2024 11:04

fti processing wip

64e8f3f

update changelog

5dbccbb

MitchTurner force-pushed the feature/process-fti-events branch from c7b1ac9 to 5dbccbb Compare April 2, 2024 18:07

WIP add storage for L1 tx status

4868e5e

MitchTurner assigned Voxelot and MitchTurner Apr 2, 2024

MitchTurner added 11 commits April 2, 2024 16:32

Use types specific to relayed txs

ca2e14b

Add failing test for checking the events are stored in db

2b0fc50

Implement code so test passes :)

40dd37f

Merge branch 'master' into feature/process-fti-events

57760ad

WIP add graphql endpoints + client method

bca0830

Get graphql compiling

f626949

Get failing test compiling

611f9ff

Add actual value to test

2c26842

Use union instead of enum for status

63ed88d

Cleanup

a672cd0

Merge branch 'master' into feature/process-fti-events

a25d9fe

MitchTurner reviewed Apr 4, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/worker_service/tests.rs Show resolved Hide resolved

MitchTurner reviewed Apr 4, 2024

View reviewed changes

tests/tests/relayer.rs Show resolved Hide resolved

MitchTurner added 10 commits April 4, 2024 15:11

Add test for including relayed txs in block

065e93f

Fix test for relayed tx inclusion

21181a1

Add test to show mints are not allowed

bdc6103

Merge branch 'master' into feature/process-fti-events

6542953

Add test for duplicate relayed txs

a96c787

Add test for invalid relayed tx

a2a9576

Add test for verifier to see failed relayed tx events

4b4d343

Add test for not charging for L1 txs on L2

8fbda0f

WIP cleanup

9422061

More cleanup of executor tests

ae4ffce

MitchTurner reviewed Apr 6, 2024

View reviewed changes

crates/fuel-core/src/executor.rs Outdated Show resolved Hide resolved

MitchTurner added 6 commits April 11, 2024 15:52

Remove skips for relayed txs and only skip regular txs

c63a7a4

Simplify code more and fix some compilation stuff

593d244

Refactor validate_forced_tx to be more declarative

ce3bae9

Add test showing relayed tx can spend message from same da block

4c4333c

Appease Clippy-sama

1ef3464

Fix more CI checks

8e28483

MitchTurner requested a review from xgreenx April 12, 2024 00:08

xgreenx reviewed Apr 12, 2024

View reviewed changes

MitchTurner added 4 commits April 12, 2024 01:42

Fix silly bug in code

fa58407

Fix that closure more and do other cleanup

9a7b027

Appease Clippy-san

3fc2085

Use display impls of errors instead of hardcoding

c6c1b2d

MitchTurner requested a review from xgreenx April 12, 2024 17:31

MitchTurner added 5 commits April 12, 2024 15:59

Remove unnecessary check

dab539c

Remove extra semi-colons

a8e714f

Add specific values to low gas test

c83eb18

Use actual tx_id in error for failed execution

d344888

Cleanup

99d21e6

xgreenx previously approved these changes Apr 13, 2024

View reviewed changes

Cleanup max gas calc

9c703f3

MitchTurner dismissed xgreenx’s stale review via 9c703f3 April 13, 2024 00:26

Voxelot commented Apr 13, 2024

View reviewed changes

crates/fuel-core/src/executor.rs Outdated Show resolved Hide resolved

Remove comments

abae5ab

xgreenx approved these changes Apr 13, 2024

View reviewed changes

xgreenx merged commit 376894e into master Apr 13, 2024
33 checks passed

xgreenx deleted the feature/process-fti-events branch April 13, 2024 10:05

xgreenx mentioned this pull request Apr 15, 2024

Process FTI event #1776

Closed

MitchTurner mentioned this pull request Apr 16, 2024

Include forced transactions specification FuelLabs/fuel-specs#572

Open

xgreenx mentioned this pull request Apr 18, 2024

Release v0.25.0 #1839

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle FTI messages in executor #1787

Handle FTI messages in executor #1787

Voxelot commented Mar 29, 2024 •

edited by MitchTurner

xgreenx Apr 12, 2024

xgreenx Apr 12, 2024

xgreenx Apr 12, 2024

xgreenx Apr 12, 2024

MitchTurner Apr 12, 2024

xgreenx Apr 12, 2024

xgreenx Apr 12, 2024

xgreenx Apr 12, 2024

MitchTurner Apr 12, 2024

xgreenx Apr 12, 2024

xgreenx left a comment •

edited

		let expected = "Insufficient max gas.";
		assert!(actual.starts_with(expected));

		// let mut bad_tx = tx.clone();
		// bad_relayed_tx.set_serialized_transaction(bad_tx_bytes);

Handle FTI messages in executor #1787

Handle FTI messages in executor #1787

Conversation

Voxelot commented Mar 29, 2024 • edited by MitchTurner

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment • edited

Choose a reason for hiding this comment

Voxelot commented Mar 29, 2024 •

edited by MitchTurner

xgreenx left a comment •

edited