From 30221bdbacfa2bba7609654af1f61a94c9f3b86a Mon Sep 17 00:00:00 2001 From: hal3e Date: Wed, 12 Jun 2024 10:39:49 +0200 Subject: [PATCH] feat!: var output estimation optimization (#1393) (#1418) closes: #1380 The number of variable outputs is now determined by filling the dry-run tx with as much variable outputs as the node will allow and counting the number of `TransferOut` receipts made by the dry run. --------- Co-authored-by: Ahmed Sagdati Co-authored-by: Oleksii Filonenko <12615679+Br1ght0ne@users.noreply.github.com> --- .../tx-dependency-estimation.md | 2 +- .../src/calling-contracts/variable-outputs.md | 12 +- e2e/Forc.toml | 1 + e2e/sway/contracts/var_outputs/Forc.toml | 5 + e2e/sway/contracts/var_outputs/src/main.sw | 15 + e2e/tests/contracts.rs | 96 +++---- e2e/tests/providers.rs | 2 +- e2e/tests/scripts.rs | 3 +- examples/contracts/src/lib.rs | 9 +- examples/cookbook/src/lib.rs | 4 +- packages/fuels-accounts/src/account.rs | 13 +- packages/fuels-accounts/src/provider.rs | 36 ++- .../src/types/transaction_builders.rs | 264 ++++++++++-------- .../types/transaction_builders/dry_runner.rs | 43 +++ .../transaction_builders/script_dry_runner.rs | 136 +++++++++ packages/fuels-programs/src/call_utils.rs | 103 +------ packages/fuels-programs/src/contract.rs | 77 ++--- packages/fuels-programs/src/script_calls.rs | 35 ++- 18 files changed, 517 insertions(+), 339 deletions(-) create mode 100644 e2e/sway/contracts/var_outputs/Forc.toml create mode 100644 e2e/sway/contracts/var_outputs/src/main.sw create mode 100644 packages/fuels-core/src/types/transaction_builders/dry_runner.rs create mode 100644 packages/fuels-core/src/types/transaction_builders/script_dry_runner.rs diff --git a/docs/src/calling-contracts/tx-dependency-estimation.md b/docs/src/calling-contracts/tx-dependency-estimation.md index e545b36b93..76e80b02d3 100644 --- a/docs/src/calling-contracts/tx-dependency-estimation.md +++ b/docs/src/calling-contracts/tx-dependency-estimation.md @@ -8,7 +8,7 @@ The following example uses a contract call that calls an external contract and l {{#include ../../../examples/contracts/src/lib.rs:dependency_estimation_fail}} ``` -As mentioned in previous chapters, you can specify the external contract with `.with_contracts()` and add an output variable with `append_variable_outputs()` to resolve this: +As mentioned in previous chapters, you can specify the external contract and add an output variable to resolve this: ```rust,ignore {{#include ../../../examples/contracts/src/lib.rs:dependency_estimation_manual}} diff --git a/docs/src/calling-contracts/variable-outputs.md b/docs/src/calling-contracts/variable-outputs.md index 93473e6420..7dc42cdcb9 100644 --- a/docs/src/calling-contracts/variable-outputs.md +++ b/docs/src/calling-contracts/variable-outputs.md @@ -11,15 +11,15 @@ Let's say you deployed a contract with the following method: {{#include ../../../e2e/sway/contracts/token_ops/src/main.sw:variable_outputs}} ``` -When calling `transfer_coins_to_output` with the SDK, you can specify the number of variable outputs by chaining `append_variable_outputs(amount)` to your call. Like this: +When calling `transfer_coins_to_output` with the SDK, you can specify the number of variable outputs: ```rust,ignore {{#include ../../../examples/contracts/src/lib.rs:variable_outputs}} ``` - - -`append_variable_outputs` effectively appends a given amount of `Output::Variable`s to the transaction's list of outputs. This output type indicates that the amount and the owner may vary based on transaction execution. - + + +`with_variable_output_policy` sets the policy regarding variable outputs. You can either set the number of variable outputs yourself by providing `VariableOutputPolicy::Exactly(n)` or let the SDK estimate it for you with `VariableOutputPolicy::EstimateMinimum`. A variable output indicates that the amount and the owner may vary based on transaction execution. + -> **Note:** that the Sway `lib-std` function `mint_to_address` calls `transfer_to_address` under the hood, so you need to call `append_variable_outputs` in the Rust SDK tests like you would for `transfer_to_address`. +> **Note:** that the Sway `lib-std` function `mint_to_address` calls `transfer_to_address` under the hood, so you need to call `with_variable_output_policy` in the Rust SDK tests like you would for `transfer_to_address`. diff --git a/e2e/Forc.toml b/e2e/Forc.toml index 785b4a1fad..663157f2e8 100644 --- a/e2e/Forc.toml +++ b/e2e/Forc.toml @@ -27,6 +27,7 @@ members = [ 'sway/contracts/storage', 'sway/contracts/token_ops', 'sway/contracts/transaction_block_height', + 'sway/contracts/var_outputs', 'sway/logs/contract_logs', 'sway/logs/contract_logs_abi', 'sway/logs/contract_with_contract_logs', diff --git a/e2e/sway/contracts/var_outputs/Forc.toml b/e2e/sway/contracts/var_outputs/Forc.toml new file mode 100644 index 0000000000..25a4dcea9d --- /dev/null +++ b/e2e/sway/contracts/var_outputs/Forc.toml @@ -0,0 +1,5 @@ +[project] +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +name = "var_outputs" diff --git a/e2e/sway/contracts/var_outputs/src/main.sw b/e2e/sway/contracts/var_outputs/src/main.sw new file mode 100644 index 0000000000..68a8bd091a --- /dev/null +++ b/e2e/sway/contracts/var_outputs/src/main.sw @@ -0,0 +1,15 @@ +contract; + +abi MyContract { + fn mint(coins: u64, recipient: Identity); +} + +impl MyContract for Contract { + fn mint(coins: u64, recipient: Identity) { + let mut counter = 0; + while counter < coins { + counter += 1; + std::asset::mint_to(recipient, std::constants::ZERO_B256, 1); + } + } +} diff --git a/e2e/tests/contracts.rs b/e2e/tests/contracts.rs index ec78407fc5..23bd38931f 100644 --- a/e2e/tests/contracts.rs +++ b/e2e/tests/contracts.rs @@ -4,6 +4,7 @@ use fuels::{ tx::ContractParameters, types::{errors::transaction::Reason, Bits256, Identity}, }; +use tokio::time::Instant; #[tokio::test] async fn test_multiple_args() -> Result<()> { @@ -699,25 +700,11 @@ async fn test_output_variable_estimation() -> Result<()> { )); } - { - // Should fail due to insufficient attempts (needs at least 3) - let response = contract_methods - .mint_to_addresses(amount, addresses) - .estimate_tx_dependencies(Some(2)) - .await; - - assert!(matches!( - response, - Err(Error::Transaction(Reason::Reverted { .. })) - )); - } - { // Should add 3 output variables automatically let _ = contract_methods .mint_to_addresses(amount, addresses) - .estimate_tx_dependencies(Some(3)) - .await? + .with_variable_output_policy(VariableOutputPolicy::EstimateMinimum) .call() .await?; @@ -730,35 +717,6 @@ async fn test_output_variable_estimation() -> Result<()> { Ok(()) } -#[tokio::test] -async fn test_output_variable_estimation_default_attempts() -> Result<()> { - abigen!(Contract( - name = "MyContract", - abi = "e2e/sway/contracts/token_ops/out/release/token_ops-abi.json" - )); - - let (wallets, addresses, mint_asset_id, contract_id) = - setup_output_variable_estimation_test().await?; - - let contract_instance = MyContract::new(contract_id, wallets[0].clone()); - let contract_methods = contract_instance.methods(); - let amount = 1000; - - let _ = contract_methods - .mint_to_addresses(amount, addresses) - .estimate_tx_dependencies(None) - .await? - .call() - .await?; - - for wallet in wallets.iter() { - let balance = wallet.get_asset_balance(&mint_asset_id).await?; - assert_eq!(balance, amount); - } - - Ok(()) -} - #[tokio::test] async fn test_output_variable_estimation_multicall() -> Result<()> { abigen!(Contract( @@ -796,8 +754,7 @@ async fn test_output_variable_estimation_multicall() -> Result<()> { multi_call_handler.add_call(call_handler); let _ = multi_call_handler - .estimate_tx_dependencies(None) - .await? + .with_variable_output_policy(VariableOutputPolicy::EstimateMinimum) .call::<((), (), ())>() .await?; @@ -932,7 +889,7 @@ async fn test_contract_set_estimation() -> Result<()> { let res = contract_caller_instance .methods() .increment_from_contract(lib_contract_id, 42) - .estimate_tx_dependencies(None) + .determine_missing_contracts(None) .await? .call() .await?; @@ -995,7 +952,7 @@ async fn test_output_variable_contract_id_estimation_multicall() -> Result<()> { multi_call_handler.add_call(call_handler); let call_response = multi_call_handler - .estimate_tx_dependencies(None) + .determine_missing_contracts(None) .await? .call::<(u64, u64, u64, u64)>() .await?; @@ -1262,7 +1219,7 @@ async fn low_level_call() -> Result<()> { Bytes(function_selector), Bytes(call_data), ) - .estimate_tx_dependencies(None) + .determine_missing_contracts(None) .await? .call() .await?; @@ -1290,7 +1247,7 @@ async fn low_level_call() -> Result<()> { Bytes(function_selector), Bytes(call_data), ) - .estimate_tx_dependencies(None) + .determine_missing_contracts(None) .await? .call() .await?; @@ -1872,3 +1829,42 @@ async fn msg_sender_gas_estimation_issue() { .await .unwrap(); } + +#[tokio::test] +async fn variable_output_estimation_is_optimized() -> Result<()> { + setup_program_test!( + Wallets("wallet"), + Abigen(Contract( + name = "MyContract", + project = "e2e/sway/contracts/var_outputs" + )), + Deploy( + contract = "MyContract", + name = "contract_instance", + wallet = "wallet" + ) + ); + + let contract_methods = contract_instance.methods(); + + let coins = 252; + let recipient = Identity::Address(wallet.address().into()); + let start = Instant::now(); + let _ = contract_methods + .mint(coins, recipient) + .with_variable_output_policy(VariableOutputPolicy::EstimateMinimum) + .call() + .await?; + + // using `fuel-core-lib` in debug builds is 20x slower so we won't validate in that case so we + // don't have to maintain two expectations + if !cfg!(all(debug_assertions, feature = "fuel-core-lib")) { + let elapsed = start.elapsed().as_secs(); + let limit = 2; + if elapsed > limit { + panic!("Estimation took too long ({elapsed}). Limit is {limit}"); + } + } + + Ok(()) +} diff --git a/e2e/tests/providers.rs b/e2e/tests/providers.rs index 8727ccf646..792f369b90 100644 --- a/e2e/tests/providers.rs +++ b/e2e/tests/providers.rs @@ -414,7 +414,7 @@ async fn test_amount_and_asset_forwarding() -> Result<()> { // withdraw some tokens to wallet contract_methods .transfer(1_000_000, asset_id, address.into()) - .append_variable_outputs(1) + .with_variable_output_policy(VariableOutputPolicy::Exactly(1)) .call() .await?; diff --git a/e2e/tests/scripts.rs b/e2e/tests/scripts.rs index 56b5971637..5868c5a319 100644 --- a/e2e/tests/scripts.rs +++ b/e2e/tests/scripts.rs @@ -125,8 +125,7 @@ async fn test_output_variable_estimation() -> Result<()> { let inputs = wallet.get_asset_inputs_for_amount(asset_id, amount).await?; let _ = script_call .with_inputs(inputs) - .estimate_tx_dependencies(None) - .await? + .with_variable_output_policy(VariableOutputPolicy::EstimateMinimum) .call() .await?; diff --git a/examples/contracts/src/lib.rs b/examples/contracts/src/lib.rs index 5a1e276895..8251e6c8b5 100644 --- a/examples/contracts/src/lib.rs +++ b/examples/contracts/src/lib.rs @@ -369,7 +369,7 @@ mod tests { // withdraw some tokens to wallet let response = contract_methods .transfer(1_000_000, asset_id, address.into()) - .append_variable_outputs(1) + .with_variable_output_policy(VariableOutputPolicy::Exactly(1)) .call() .await?; // ANCHOR_END: variable_outputs @@ -422,7 +422,7 @@ mod tests { // ANCHOR: dependency_estimation_manual let response = contract_methods .mint_then_increment_from_contract(called_contract_id, amount, address.into()) - .append_variable_outputs(1) + .with_variable_output_policy(VariableOutputPolicy::Exactly(1)) .with_contract_ids(&[called_contract_id.into()]) .call() .await?; @@ -435,7 +435,8 @@ mod tests { // ANCHOR: dependency_estimation let response = contract_methods .mint_then_increment_from_contract(called_contract_id, amount, address.into()) - .estimate_tx_dependencies(Some(2)) + .with_variable_output_policy(VariableOutputPolicy::EstimateMinimum) + .determine_missing_contracts(Some(2)) .await? .call() .await?; @@ -736,7 +737,7 @@ mod tests { Bytes(function_selector), Bytes(call_data), ) - .estimate_tx_dependencies(None) + .determine_missing_contracts(None) .await? .call() .await?; diff --git a/examples/cookbook/src/lib.rs b/examples/cookbook/src/lib.rs index d72bc7221b..c0b64faa6c 100644 --- a/examples/cookbook/src/lib.rs +++ b/examples/cookbook/src/lib.rs @@ -70,7 +70,7 @@ mod tests { contract_methods .deposit(wallet.address().into()) .call_params(call_params)? - .append_variable_outputs(1) + .with_variable_output_policy(VariableOutputPolicy::Exactly(1)) .call() .await?; // ANCHOR_END: liquidity_deposit @@ -86,7 +86,7 @@ mod tests { contract_methods .withdraw(wallet.address().into()) .call_params(call_params)? - .append_variable_outputs(1) + .with_variable_output_policy(VariableOutputPolicy::Exactly(1)) .call() .await?; diff --git a/packages/fuels-accounts/src/account.rs b/packages/fuels-accounts/src/account.rs index 69766306d0..73fa39950e 100644 --- a/packages/fuels-accounts/src/account.rs +++ b/packages/fuels-accounts/src/account.rs @@ -290,7 +290,10 @@ mod tests { use fuel_tx::{Address, ConsensusParameters, Output, Transaction as FuelTransaction}; use fuels_core::{ traits::Signer, - types::{transaction::Transaction, transaction_builders::DryRunner}, + types::{ + transaction::Transaction, + transaction_builders::{DryRun, DryRunner}, + }, }; use rand::{rngs::StdRng, RngCore, SeedableRng}; @@ -334,8 +337,12 @@ mod tests { #[cfg_attr(not(target_arch = "wasm32"), async_trait)] impl DryRunner for MockDryRunner { - async fn dry_run_and_get_used_gas(&self, _: FuelTransaction, _: f32) -> Result { - Ok(0) + async fn dry_run(&self, _: FuelTransaction) -> Result { + Ok(DryRun { + succeeded: true, + script_gas: 0, + variable_outputs: 0, + }) } fn consensus_parameters(&self) -> &ConsensusParameters { diff --git a/packages/fuels-accounts/src/provider.rs b/packages/fuels-accounts/src/provider.rs index a425b610df..dd4e8e0e8e 100644 --- a/packages/fuels-accounts/src/provider.rs +++ b/packages/fuels-accounts/src/provider.rs @@ -17,7 +17,10 @@ use fuel_core_client::client::{ gas_price::{EstimateGasPrice, LatestGasPrice}, }, }; -use fuel_core_types::blockchain::header::LATEST_STATE_TRANSITION_VERSION; +use fuel_core_types::{ + blockchain::header::LATEST_STATE_TRANSITION_VERSION, + services::executor::TransactionExecutionResult, +}; use fuel_tx::{ AssetId, ConsensusParameters, Receipt, Transaction as FuelTransaction, TxId, UtxoId, }; @@ -37,7 +40,7 @@ use fuels_core::{ message_proof::MessageProof, node_info::NodeInfo, transaction::{Transaction, Transactions}, - transaction_builders::DryRunner, + transaction_builders::{DryRun, DryRunner}, transaction_response::TransactionResponse, tx_status::TxStatus, }, @@ -637,12 +640,12 @@ impl Provider { tolerance: f64, ) -> Result { let receipts = self.dry_run_no_validation(tx).await?.take_receipts(); - let gas_used = self.get_gas_used(&receipts); + let gas_used = self.get_script_gas_used(&receipts); Ok((gas_used as f64 * (1.0 + tolerance)) as u64) } - fn get_gas_used(&self, receipts: &[Receipt]) -> u64 { + fn get_script_gas_used(&self, receipts: &[Receipt]) -> u64 { receipts .iter() .rfind(|r| matches!(r, Receipt::ScriptResult { .. })) @@ -701,7 +704,7 @@ impl Provider { #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl DryRunner for Provider { - async fn dry_run_and_get_used_gas(&self, tx: FuelTransaction, tolerance: f32) -> Result { + async fn dry_run(&self, tx: FuelTransaction) -> Result { let [tx_execution_status] = self .client .dry_run_opt(&vec![tx], Some(false)) @@ -709,9 +712,28 @@ impl DryRunner for Provider { .try_into() .expect("should have only one element"); - let gas_used = self.get_gas_used(tx_execution_status.result.receipts()); + let receipts = tx_execution_status.result.receipts(); + let script_gas = self.get_script_gas_used(receipts); + + let variable_outputs = receipts + .iter() + .filter( + |receipt| matches!(receipt, Receipt::TransferOut { amount, .. } if *amount != 0), + ) + .count(); + + let succeeded = matches!( + tx_execution_status.result, + TransactionExecutionResult::Success { .. } + ); + + let dry_run = DryRun { + succeeded, + script_gas, + variable_outputs, + }; - Ok((gas_used as f64 * (1.0 + tolerance as f64)) as u64) + Ok(dry_run) } async fn estimate_gas_price(&self, block_horizon: u32) -> Result { diff --git a/packages/fuels-core/src/types/transaction_builders.rs b/packages/fuels-core/src/types/transaction_builders.rs index 5a54077998..5edd39d813 100644 --- a/packages/fuels-core/src/types/transaction_builders.rs +++ b/packages/fuels-core/src/types/transaction_builders.rs @@ -10,21 +10,19 @@ use async_trait::async_trait; use fuel_asm::{op, GTFArgs, RegId}; use fuel_crypto::{Hasher, Message as CryptoMessage, Signature}; use fuel_tx::{ - field::{Inputs, Policies as PoliciesField, ScriptGasLimit, WitnessLimit, Witnesses}, - input::coin::{CoinPredicate, CoinSigned}, + field::{Outputs, Policies as PoliciesField, ScriptGasLimit, Witnesses}, policies::{Policies, PolicyType}, Chargeable, ConsensusParameters, Create, Input as FuelInput, Output, Script, StorageSlot, Transaction as FuelTransaction, TransactionFee, TxPointer, UniqueIdentifier, Upgrade, Upload, UploadBody, Witness, }; - -pub use fuel_tx::UpgradePurpose; -pub use fuel_tx::UploadSubsection; +pub use fuel_tx::{UpgradePurpose, UploadSubsection}; use fuel_types::{bytes::padded_len_usize, Bytes32, Salt}; use itertools::Itertools; +use script_dry_runner::ScriptDryRunner; use crate::{ - constants::{SIGNATURE_WITNESS_SIZE, WITNESS_STATIC_SIZE, WORD_SIZE}, + constants::{SIGNATURE_WITNESS_SIZE, WORD_SIZE}, traits::Signer, types::{ bech32::Bech32Address, @@ -42,27 +40,10 @@ use crate::{ utils::{calculate_witnesses_size, sealed}, }; -#[cfg_attr(not(target_arch = "wasm32"), async_trait)] -pub trait DryRunner: Send + Sync { - async fn dry_run_and_get_used_gas(&self, tx: FuelTransaction, tolerance: f32) -> Result; - async fn estimate_gas_price(&self, block_horizon: u32) -> Result; - fn consensus_parameters(&self) -> &ConsensusParameters; -} - -#[cfg_attr(not(target_arch = "wasm32"), async_trait)] -impl DryRunner for &T { - async fn dry_run_and_get_used_gas(&self, tx: FuelTransaction, tolerance: f32) -> Result { - (*self).dry_run_and_get_used_gas(tx, tolerance).await - } - - async fn estimate_gas_price(&self, block_horizon: u32) -> Result { - (*self).estimate_gas_price(block_horizon).await - } +mod dry_runner; +mod script_dry_runner; - fn consensus_parameters(&self) -> &ConsensusParameters { - (*self).consensus_parameters() - } -} +pub use dry_runner::*; #[derive(Debug, Clone, Default)] struct UnresolvedWitnessIndexes { @@ -382,6 +363,34 @@ impl Debug for dyn Signer + Send + Sync { } } +/// Controls the SDK behavior regarding variable transaction outputs. +/// +/// # Warning +/// +/// Estimation of variable outputs is performed by saturating the transaction with variable outputs +/// and counting the number of outputs used. This process can be particularly unreliable in cases +/// where the script introspects the number of variable outputs and adjusts its logic accordingly. +/// The script could theoretically mint outputs until all variable outputs are utilized. +/// +/// In such scenarios, estimation of necessary variable outputs becomes nearly impossible. +/// +/// It is advised to avoid relying on automatic estimation of variable outputs if the script +/// contains logic that dynamically adjusts based on the number of outputs. +#[derive(Debug, Clone, Copy)] +pub enum VariableOutputPolicy { + /// Perform a dry run of the transaction estimating the minimum number of variable outputs to + /// add. + EstimateMinimum, + /// Add exactly these many variable outputs to the transaction. + Exactly(usize), +} + +impl Default for VariableOutputPolicy { + fn default() -> Self { + Self::Exactly(0) + } +} + #[derive(Debug, Default)] pub struct ScriptTransactionBuilder { pub script: Vec, @@ -392,6 +401,7 @@ pub struct ScriptTransactionBuilder { pub tx_policies: TxPolicies, pub gas_estimation_tolerance: f32, pub gas_price_estimation_block_horizon: u32, + pub variable_output_policy: VariableOutputPolicy, unresolved_witness_indexes: UnresolvedWitnessIndexes, unresolved_signers: Vec>, } @@ -474,80 +484,111 @@ impl ScriptTransactionBuilder { }) } - // When dry running a tx with `utxo_validation` off, the node will not validate signatures. - // However, the node will check if the right number of witnesses is present. - // This function will create witnesses from a default `Signature` such that the total length matches the expected one. - // Using a `Signature` ensures that the calculated fee includes the fee generated by the witnesses. - fn create_dry_run_witnesses(&self, num_witnesses: u16) -> Vec { - let unresolved_witnesses_len = self.unresolved_witness_indexes.owner_to_idx_offset.len(); - let witness: Witness = Signature::default().as_ref().into(); - repeat(witness) - .take(num_witnesses as usize + unresolved_witnesses_len) - .collect() - } - - async fn resolve_fuel_tx(self, provider: impl DryRunner) -> Result