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

bug: fix estimation issue #1366

Merged
merged 4 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
153 changes: 73 additions & 80 deletions packages/fuels-core/src/types/transaction_builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,72 +393,6 @@ impl ScriptTransactionBuilder {
.collect()
}

fn no_base_asset_input<'a>(
inputs: impl IntoIterator<Item = &'a FuelInput>,
base_asset_id: &AssetId,
) -> bool {
let has_base_asset = inputs.into_iter().any(|i| match i {
FuelInput::CoinSigned(CoinSigned { asset_id, .. })
| FuelInput::CoinPredicate(CoinPredicate { asset_id, .. })
if asset_id == base_asset_id =>
{
true
}
FuelInput::MessageCoinSigned(_) | FuelInput::MessageCoinPredicate(_) => true,
_ => false,
});

!has_base_asset
}

async fn set_script_gas_limit_to_gas_used(
tx: &mut Script,
provider: impl DryRunner,
tolerance: f32,
) -> Result<()> {
let consensus_params = provider.consensus_parameters();
let base_asset_id = provider.consensus_parameters().base_asset_id();

// The dry-run validation will check if there is any base asset input.
// If we are dry-running without inputs we have to add a temporary one.
let no_base_asset_input = Self::no_base_asset_input(tx.inputs(), base_asset_id);
if no_base_asset_input {
tx.inputs_mut().push(FuelInput::coin_signed(
Default::default(),
Default::default(),
1_000_000_000,
Default::default(),
TxPointer::default(),
0,
));

// Add an empty `Witness` for the `coin_signed` we just added
// and increase the witness limit
tx.witnesses_mut().push(Default::default());
tx.set_witness_limit(tx.witness_limit() + WITNESS_STATIC_SIZE as u64);
}

// Get `max_gas` used by everything except the script execution. Add `1` because of rounding.
let max_gas = tx.max_gas(consensus_params.gas_costs(), consensus_params.fee_params()) + 1;
// Increase `script_gas_limit` to the maximum allowed value.
*tx.script_gas_limit_mut() = consensus_params.tx_params().max_gas_per_tx() - max_gas;

let gas_used = provider
.dry_run_and_get_used_gas(tx.clone().into(), tolerance)
.await?;

// Remove dry-run input and witness.
if no_base_asset_input {
tx.inputs_mut().pop();
tx.witnesses_mut().pop();
tx.set_witness_limit(tx.witness_limit() - WITNESS_STATIC_SIZE as u64);
}

*tx.script_gas_limit_mut() = gas_used;

Ok(())
}

async fn resolve_fuel_tx(self, provider: impl DryRunner) -> Result<Script> {
let num_witnesses = self.num_witnesses()?;
let policies = self.generate_fuel_policies()?;
Expand All @@ -475,24 +409,17 @@ impl ScriptTransactionBuilder {
dry_run_witnesses,
);

if has_no_code {
*tx.script_gas_limit_mut() = 0;

// Use the user defined value even if it makes the transaction revert.
let script_gas_limit = if has_no_code {
0
} else if let Some(gas_limit) = self.tx_policies.script_gas_limit() {
*tx.script_gas_limit_mut() = gas_limit;

// If the `script_gas_limit` was not set by the user,
// dry-run the tx to get the `gas_used`
// Use the user defined value even if it makes the transaction revert.
gas_limit
} else {
Self::set_script_gas_limit_to_gas_used(
&mut tx,
&provider,
self.gas_estimation_tolerance,
)
.await?
Self::run_estimation(tx.clone(), &provider, self.gas_estimation_tolerance).await?
};

*tx.script_gas_limit_mut() = script_gas_limit;

Self::set_max_fee_policy(&mut tx, &provider, self.gas_price_estimation_block_horizon)
.await?;

Expand All @@ -506,6 +433,30 @@ impl ScriptTransactionBuilder {
Ok(tx)
}

async fn run_estimation(
mut tx: fuel_tx::Script,
provider: impl DryRunner,
tolerance: f32,
) -> Result<u64> {
let consensus_params = provider.consensus_parameters();
if let Some(fake_input) =
needs_fake_base_input(tx.inputs(), consensus_params.base_asset_id())
{
tx.inputs_mut().push(fake_input);

// Add an empty `Witness` for the `coin_signed` we just added
tx.witnesses_mut().push(Default::default());
tx.set_witness_limit(tx.witness_limit() + WITNESS_STATIC_SIZE as u64);
}

let max_gas = tx.max_gas(consensus_params.gas_costs(), consensus_params.fee_params()) + 1;
*tx.script_gas_limit_mut() = consensus_params.tx_params().max_gas_per_tx() - max_gas;

provider
.dry_run_and_get_used_gas(tx.into(), tolerance)
.await
}

pub fn with_script(mut self, script: Vec<u8>) -> Self {
self.script = script;
self
Expand Down Expand Up @@ -629,6 +580,48 @@ impl ScriptTransactionBuilder {
}
}

fn needs_fake_base_input(inputs: &[FuelInput], base_asset_id: &AssetId) -> Option<fuel_tx::Input> {
let has_base_asset = inputs.iter().any(|i| match i {
FuelInput::CoinSigned(CoinSigned { asset_id, .. })
| FuelInput::CoinPredicate(CoinPredicate { asset_id, .. })
if asset_id == base_asset_id =>
{
true
}
FuelInput::MessageCoinSigned(_) | FuelInput::MessageCoinPredicate(_) => true,
_ => false,
});

if has_base_asset {
return None;
}

let unique_owners = inputs
.iter()
.filter_map(|input| match input {
FuelInput::CoinSigned(CoinSigned { owner, .. })
| FuelInput::CoinPredicate(CoinPredicate { owner, .. }) => Some(owner),
_ => None,
})
.unique()
.collect::<Vec<_>>();

let fake_owner = if let [single_owner] = unique_owners.as_slice() {
**single_owner
} else {
Default::default()
};

Some(FuelInput::coin_signed(
Default::default(),
fake_owner,
1_000_000_000,
Default::default(),
TxPointer::default(),
0,
))
}

impl CreateTransactionBuilder {
pub async fn build(self, provider: impl DryRunner) -> Result<CreateTransaction> {
Ok(CreateTransaction {
Expand Down
2 changes: 1 addition & 1 deletion packages/fuels/Forc.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ members = [
'tests/contracts/library_test',
'tests/contracts/liquidity_pool',
'tests/contracts/low_level_caller',
'tests/contracts/msg_amount',
'tests/contracts/msg_methods',
'tests/contracts/multiple_read_calls',
'tests/contracts/needs_custom_decoder',
'tests/contracts/payable_annotation',
Expand Down
47 changes: 47 additions & 0 deletions packages/fuels/tests/contracts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1825,3 +1825,50 @@ async fn test_reentrant_calls() -> Result<()> {

Ok(())
}

#[tokio::test]
async fn msg_sender_gas_estimation_issue() {
// Gas estimation requires an input of the base asset. If absent, a fake input is
// added. However, if a non-base coin is present and the fake input introduces a
// second owner, it causes the `msg_sender` sway fn to fail. This leads
// to a premature failure in gas estimation, risking transaction failure due to
// a low gas limit.
let mut wallet = WalletUnlocked::new_random(None);

let (coins, ids) =
setup_multiple_assets_coins(wallet.address(), 2, DEFAULT_NUM_COINS, DEFAULT_COIN_AMOUNT);

let provider = setup_test_provider(coins, vec![], None, None)
.await
.unwrap();
wallet.set_provider(provider.clone());

setup_program_test!(
Abigen(Contract(
name = "MyContract",
project = "packages/fuels/tests/contracts/msg_methods"
)),
Deploy(
contract = "MyContract",
name = "contract_instance",
wallet = "wallet"
)
);

let asset_id = ids[0];

// The fake coin won't be added if we add a base asset, so let's not do that
assert!(asset_id != *provider.base_asset_id());
let call_params = CallParameters::default()
.with_amount(100)
.with_asset_id(asset_id);

contract_instance
.methods()
.message_sender()
.call_params(call_params)
.unwrap()
.call()
.await
.unwrap();
}
13 changes: 0 additions & 13 deletions packages/fuels/tests/contracts/msg_amount/src/main.sw

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
authors = ["Fuel Labs <contact@fuel.sh>"]
entry = "main.sw"
license = "Apache-2.0"
name = "msg_amount"
name = "msg_methods"
15 changes: 15 additions & 0 deletions packages/fuels/tests/contracts/msg_methods/src/main.sw
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
contract;

use std::auth::msg_sender;

abi FuelTest {
#[payable]
fn message_sender() -> Identity;
}

impl FuelTest for Contract {
#[payable]
fn message_sender() -> Identity {
msg_sender().unwrap()
}
}
Loading