Add verification on transaction dry_run that they don't spend more than block gas limit #2151

AurelienFT · 2024-09-02T10:06:52Z

Linked Issues/PRs

Close #1970

Description

Add check that transactions submitted to the graphql endpoint dry_run doesn't spend too much gas. Also add this check in the executor level.

Still todo

Add tests

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

…an max_gas

xgreenx · 2024-09-02T10:57:49Z

crates/fuel-core/src/schema/tx.rs

+                    max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
+                }
+            };
+            if max_gas_usable > block_gas_limit {


I was thinking maybe to add this logic directly into the dry_run method of the executor=)

From my point of view, the issue mentionned API AND in the executor. I placed it in the API and block producer, I have removed the block producer part in order to add it to the executor.
But when looking at the executor code it seems that it already handfle the case correctly in process_l2_txs()

fuel-core/crates/services/executor/src/executor.rs

Lines 566 to 570 in f5240cb

let remaining_gas_limit = block_gas_limit.saturating_sub(data.used_gas);

let mut regular_tx_iter = l2_tx_source

.next(remaining_gas_limit)

.into_iter()

The problem is that it doesn't return an error but just doesn't execute remaning transactions so I think it's great to have the check in the API also to avoid uncessary precompute etc.. and also return a meaningful error.

In the case of dry run we currently ignore remaining_gas_limit

@xgreenx Could you point me to this ? Because I don't see where is the behavior difference

…nto add_limit_dryrun_tx

netrome

Looks good to me overall, but I would like the test case to make a more specific assertion to be able to trust that it is correct.

(My nit comment is not a showstopper, but I'd of course be happy to have that one addressed as well)

crates/fuel-core/src/schema/tx.rs

tests/tests/tx.rs

crates/fuel-core/src/schema/tx.rs

xgreenx · 2024-09-09T08:52:40Z

crates/fuel-core/src/schema/tx.rs

+                    max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
+                }
+            };
+            if max_gas_usable > block_gas_limit {


In the case of dry run we currently ignore remaining_gas_limit

crates/fuel-core/src/schema/tx.rs

crates/services/producer/src/block_producer.rs

tests/tests/tx.rs

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

xgreenx

I still think that we need to handle the gas before match self.execute_transaction_and_commit( since the TxSource can lie/ignore remaining gas.

netrome

LGTM, one nit.

netrome · 2024-09-10T09:13:03Z

crates/services/executor/src/ports.rs

+pub trait TransactionExt {
+    fn max_gas(&self, consensus_params: &ConsensusParameters) -> ExecutorResult<u64>;
+}
+


Nit: Since this is a one-method trait it would be easier to read the intention of it if it was named after the function: pub trait MaxGas.

@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

@rymnc

## Version v0.37.0 ### Added - [1609](#1609): Add DA compression support. Compressed blocks are stored in the offchain database when blocks are produced, and can be fetched using the GraphQL API. - [2290](#2290): Added a new CLI argument `--graphql-max-directives`. The default value is `10`. - [2195](#2195): Added enforcement of the limit on the size of the L2 transactions per block according to the `block_transaction_size_limit` parameter. - [2131](#2131): Add flow in TxPool in order to ask to newly connected peers to share their transaction pool - [2182](#2151): Limit number of transactions that can be fetched via TxSource::next - [2189](#2151): Select next DA height to never include more than u16::MAX -1 transactions from L1. - [2162](#2162): Pool structure with dependencies, etc.. for the next transaction pool module. Also adds insertion/verification process in PoolV2 and tests refactoring - [2265](#2265): Integrate Block Committer API for DA Block Costs. - [2280](#2280): Allow comma separated relayer addresses in cli - [2299](#2299): Support blobs in the predicates. - [2300](#2300): Added new function to `fuel-core-client` for checking whether a blob exists. ### Changed #### Breaking - [2299](#2299): Anyone who wants to participate in the transaction broadcasting via p2p must upgrade to support new predicates on the TxPool level. - [2299](#2299): Upgraded `fuel-vm` to `0.58.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.58.0). - [2276](#2276): Changed how complexity for blocks is calculated. The default complexity now is 80_000. All queries that somehow touch the block header now are more expensive. - [2290](#2290): Added a new GraphQL limit on number of `directives`. The default value is `10`. - [2206](#2206): Use timestamp of last block when dry running transactions. - [2153](#2153): Updated default gas costs for the local testnet configuration to match `fuel-core 0.35.0`. ## What's Changed * fix: use core-test.fuellabs.net for dnsaddr resolution by @rymnc in #2214 * Removed state transition bytecode from the local testnet by @xgreenx in #2215 * Send whole transaction pool upon subscription to gossip by @AurelienFT in #2131 * Update default gas costs based on 0.35.0 benchmarks by @xgreenx in #2153 * feat: Use timestamp of last block when dry running transactions by @netrome in #2206 * fix(dnsaddr_resolution): use fqdn separator to prevent suffixing by dns resolvers by @rymnc in #2222 * TransactionSource: specify maximum number of transactions to be fetched by @acerone85 in #2182 * Implement worst case scenario for price algorithm v1 by @rafal-ch in #2219 * chore(gas_price_service): define port for L2 data by @rymnc in #2224 * Block producer selects da height to never exceed u64::MAX - 1 transactions from L1 by @acerone85 in #2189 * Weekly `cargo update` by @github-actions in #2236 * Use fees to calculate DA reward and avoid issues with Gwei/Wei conversions by @MitchTurner in #2229 * Protect against passing `i128::MIN` to `abs()` which causes overflow by @rafal-ch in #2241 * Acquire `da_finalization_period` from the command line by @rafal-ch in #2240 * Executor: test Tx_count limit with incorrect tx source by @acerone85 in #2242 * Minor updates to docs + a few typos fixed by @rafal-ch in #2250 * chore(gas_price_service): move algorithm_updater to fuel-core-gas-price-service by @rymnc in #2246 * Use single heavy input in the `transaction_throughput.rs` benchmarks by @xgreenx in #2205 * Enforce the block size limit by @rafal-ch in #2195 * feat: build ARM and AMD in parallel by @mchristopher in #2130 * Weekly `cargo update` by @github-actions in #2268 * chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService by @rymnc in #2256 * feat(gas_price_service): update block committer da source with established contract by @rymnc in #2265 * Use bytes from `unrecorded_blocks` rather from the block from DA by @MitchTurner in #2252 * TxPool v2 General architecture by @AurelienFT in #2162 * Add value delimiter and tests args by @AurelienFT in #2280 * fix(da_block_costs): remove Arc<Mutex<>> on shared_state and expose channel by @rymnc in #2278 * fix(combined_database): syncing auxiliary databases on startup with custom behaviour by @rymnc in #2272 * fix: Manually encode Authorization header for eventsource_client by @Br1ght0ne in #2284 * Address `async-graphql` vulnerability by @MitchTurner in #2290 * Update the WASM compatibility tests for `0.36` release by @rafal-ch in #2271 * DA compression by @Dentosal in #1609 * Use different port for every version compatibility test by @rafal-ch in #2301 * Fix block query complexity by @xgreenx in #2297 * Support blobs in predicates by @Voxelot in #2299 **Full Changelog**: v0.36.0...v0.37.0

Add verification on transaction dry_run that they don't spend more th…

9bea61a

…an max_gas

xgreenx reviewed Sep 2, 2024

View reviewed changes

AurelienFT self-assigned this Sep 2, 2024

AurelienFT and others added 5 commits September 2, 2024 15:22

Remove block production check max gas

b3eb785

Merge branch 'master' into add_limit_dryrun_tx

821a50d

Add test dry_run above limit API.

4aadb2e

Merge branch 'add_limit_dryrun_tx' of github.com:FuelLabs/fuel-core i…

e0b3c14

…nto add_limit_dryrun_tx

Update CHANGELOG

103abe9

AurelienFT marked this pull request as ready for review September 2, 2024 14:01

AurelienFT requested review from xgreenx and a team September 2, 2024 14:01

AurelienFT added 3 commits September 2, 2024 16:39

format

e2512fb

Clippy fix.

576ea31

Fix usage of saturating_add

62cd5c5

netrome requested changes Sep 9, 2024

View reviewed changes

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

tests/tests/tx.rs Outdated Show resolved Hide resolved

xgreenx reviewed Sep 9, 2024

View reviewed changes

AurelienFT and others added 2 commits September 9, 2024 15:09

Apply suggestions from code review

d5093b5

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

Reorganise code and better match on tests

462ea8f

AurelienFT requested review from Dentosal and MitchTurner as code owners September 9, 2024 13:28

AurelienFT requested a review from netrome September 9, 2024 13:29

netrome previously approved these changes Sep 9, 2024

View reviewed changes

xgreenx reviewed Sep 9, 2024

View reviewed changes

Merge branch 'master' into add_limit_dryrun_tx

27b466e

AurelienFT dismissed netrome’s stale review via 27b466e September 9, 2024 14:16

Add gas_limit check in executor

2548ebd

AurelienFT requested review from xgreenx and netrome September 9, 2024 16:55

AurelienFT added 2 commits September 9, 2024 23:53

try to gather data on test failing

53aa5ab

temp disable wasm check

8449263

AurelienFT and others added 7 commits September 10, 2024 00:09

readd wasm

7efd477

format

ff0d5e0

fix dbg

3fa8626

remove added test

72ca9b7

Fix test in wasm

8647dc6

Reduce duplication of the code

2904e99

Change order

fb36a9a

xgreenx approved these changes Sep 10, 2024

View reviewed changes

netrome approved these changes Sep 10, 2024

View reviewed changes

AurelienFT merged commit ce857cf into master Sep 10, 2024
35 checks passed

AurelienFT deleted the add_limit_dryrun_tx branch September 10, 2024 20:07

xgreenx mentioned this pull request Sep 17, 2024

Release v0.36.0 #2213

Merged

xgreenx mentioned this pull request Oct 5, 2024

Release v0.37.0 #2302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verification on transaction dry_run that they don't spend more than block gas limit #2151

Add verification on transaction dry_run that they don't spend more than block gas limit #2151

AurelienFT commented Sep 2, 2024

xgreenx Sep 2, 2024

AurelienFT Sep 2, 2024 •

edited

Loading

xgreenx Sep 9, 2024

AurelienFT Sep 9, 2024

AurelienFT Sep 9, 2024

netrome left a comment

xgreenx Sep 9, 2024

xgreenx left a comment

netrome left a comment

netrome Sep 10, 2024

	let remaining_gas_limit = block_gas_limit.saturating_sub(data.used_gas);

	let mut regular_tx_iter = l2_tx_source
	.next(remaining_gas_limit)
	.into_iter()

Add verification on transaction dry_run that they don't spend more than block gas limit #2151

Add verification on transaction dry_run that they don't spend more than block gas limit #2151

Conversation

AurelienFT commented Sep 2, 2024

Linked Issues/PRs

Description

Still todo

Checklist

Before requesting review

xgreenx Sep 2, 2024

Choose a reason for hiding this comment

AurelienFT Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

xgreenx Sep 9, 2024

Choose a reason for hiding this comment

AurelienFT Sep 9, 2024

Choose a reason for hiding this comment

AurelienFT Sep 9, 2024

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

xgreenx Sep 9, 2024

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

netrome Sep 10, 2024

Choose a reason for hiding this comment

AurelienFT Sep 2, 2024 •

edited

Loading