Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions crates/fuel-core/src/schema/tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use fuel_core_txpool::{
use fuel_core_types::{
fuel_tx::{
Cacheable,
Chargeable,
Transaction as FuelTx,
UniqueIdentifier,
},
Expand Down Expand Up @@ -276,16 +277,43 @@ impl TxMutation {
gas_price: Option<U64>,
) -> async_graphql::Result<Vec<DryRunTransactionExecutionStatus>> {
let block_producer = ctx.data_unchecked::<BlockProducer>();
let params = ctx
let consensus_params = ctx
.data_unchecked::<ConsensusProvider>()
.latest_consensus_params();
let block_gas_limit = consensus_params.block_gas_limit();

let mut max_gas_usable = 0;
let mut transactions = txs
.iter()
.map(|tx| FuelTx::from_bytes(&tx.0))
.collect::<Result<Vec<FuelTx>, _>>()?;
let fee = consensus_params.fee_params();
for transaction in &mut transactions {
transaction.precompute(&params.chain_id())?;
match transaction {
FuelTx::Script(tx) => {
max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
}
FuelTx::Create(tx) => {
max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
}
FuelTx::Mint(_) => {
// Mint transactions doesn't consume gas and so we will add a flat 10% of the block gas limit
max_gas_usable += block_gas_limit / 10;
}
FuelTx::Upgrade(tx) => {
max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
}
FuelTx::Upload(tx) => {
max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
}
FuelTx::Blob(tx) => {
max_gas_usable += tx.max_gas(consensus_params.gas_costs(), fee)
}
};
if max_gas_usable > block_gas_limit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AurelienFT AurelienFT Sep 2, 2024

Choose a reason for hiding this comment

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

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()

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case of dry run we currently ignore remaining_gas_limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return Err(anyhow::anyhow!("The sum of the gas usable by the transactions is greater than the block gas limit").into());
}
transaction.precompute(&consensus_params.chain_id())?;
AurelienFT marked this conversation as resolved.
Show resolved Hide resolved
}

let tx_statuses = block_producer
Expand Down
37 changes: 35 additions & 2 deletions crates/services/producer/src/block_producer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ use fuel_core_types::{
field::{
InputContract,
MintGasPrice,
},
Transaction,
}, Chargeable, Transaction
},
fuel_types::{
BlockHeight,
Expand Down Expand Up @@ -280,6 +279,40 @@ where
gas_price: Option<u64>,
) -> anyhow::Result<Vec<TransactionExecutionStatus>> {
let view = self.view_provider.latest_view()?;

AurelienFT marked this conversation as resolved.
Show resolved Hide resolved
// Check that the sum of the gas usable by the transactions is less than the block gas limit.
let consensus_params = self.consensus_parameters_provider.consensus_params_at_version(&view.latest_consensus_parameters_version()?)?;
let gas_costs = consensus_params.gas_costs();
let fee = consensus_params.fee_params();
let block_gas_limit = consensus_params.block_gas_limit();
let mut max_gas_usable = 0;
for transaction in &transactions {
match transaction {
Transaction::Script(tx) => {
max_gas_usable += tx.max_gas(gas_costs, fee)
}
Transaction::Create(tx) => {
max_gas_usable += tx.max_gas(gas_costs, fee)
}
Transaction::Mint(_) => {
// Mint transactions doesn't consume gas and so we will add a flat 10% of the block gas limit
max_gas_usable += block_gas_limit / 10;
}
Transaction::Upgrade(tx) => {
max_gas_usable += tx.max_gas(gas_costs, fee)
}
Transaction::Upload(tx) => {
max_gas_usable += tx.max_gas(gas_costs, fee)
}
Transaction::Blob(tx) => {
max_gas_usable += tx.max_gas(gas_costs, fee)
}
};
if max_gas_usable > block_gas_limit {
return Err(anyhow!("The sum of the gas usable by the transactions is greater than the block gas limit"));
}
}

let height = height.unwrap_or_else(|| {
view.latest_height()
.unwrap_or_default()
Expand Down
Loading