From 942b2b11eafbe1c42fd16f69d65b12673fc16c45 Mon Sep 17 00:00:00 2001 From: Dmytro Kozhevin Date: Mon, 4 Nov 2024 18:35:42 -0500 Subject: [PATCH] Fix an issue in footprint simulation and add some test coverage for it. The issue occurs when a non-existent entry gets in a failed contract call that is then handled gracefully via `try_call`. The value won't appear in storage and due to how simulation is implemented it also wasn't included in the result footprint due to that. The fix is to just add the footprint-only entries to the recorded ledgers diffs (as a 'never existed' entry). --- soroban-env-host/src/e2e_invoke.rs | 38 ++- soroban-simulation/src/test/simulation.rs | 298 +++++++++++++++++- soroban-test-wasms/src/lib.rs | 3 + soroban-test-wasms/wasm-workspace/Cargo.toml | 3 +- .../opt/20/test_try_call_sac.wasm | Bin 0 -> 578 bytes .../wasm-workspace/try_call_sac/Cargo.toml | 14 + .../wasm-workspace/try_call_sac/src/lib.rs | 18 ++ 7 files changed, 366 insertions(+), 8 deletions(-) create mode 100644 soroban-test-wasms/wasm-workspace/opt/20/test_try_call_sac.wasm create mode 100644 soroban-test-wasms/wasm-workspace/try_call_sac/Cargo.toml create mode 100644 soroban-test-wasms/wasm-workspace/try_call_sac/src/lib.rs diff --git a/soroban-env-host/src/e2e_invoke.rs b/soroban-env-host/src/e2e_invoke.rs index b327d85fe..cae1d7675 100644 --- a/soroban-env-host/src/e2e_invoke.rs +++ b/soroban-env-host/src/e2e_invoke.rs @@ -200,6 +200,35 @@ pub fn get_ledger_changes( Ok(changes) } +/// Creates ledger changes for entries that don't exist in the storage. +/// +/// In recording mode it's possible to have discrepancies between the storage +/// and the footprint. Specifically, if an entry is only accessed from a +/// function that has failed and had its failure handled gracefully (via +/// `try_call`), then the storage map will get rolled back and the access will +/// only be recorded in the footprint. However, we still need to account for +/// these in the ledger entry changes, as downstream consumers (simulation) rely +/// on that to determine the fees. +#[cfg(any(test, feature = "recording_mode"))] +fn add_footprint_only_ledger_changes( + budget: &Budget, + storage: &Storage, + changes: &mut Vec, +) -> Result<(), HostError> { + for (key, access_type) in storage.footprint.0.iter(budget)? { + // We have to check if the entry exists in the internal storage map + // because `has` check on storage affects the footprint. + if storage.map.contains_key(key, budget)? { + continue; + } + let mut entry_change = LedgerEntryChange::default(); + metered_write_xdr(budget, key.as_ref(), &mut entry_change.encoded_key)?; + entry_change.read_only = matches!(*access_type, AccessType::ReadOnly); + changes.push(entry_change); + } + Ok(()) +} + /// Extracts the rent-related changes from the provided ledger changes. /// /// Only meaningful changes are returned (i.e. no-op changes are skipped). @@ -608,8 +637,15 @@ pub fn invoke_host_function_in_recording_mode( } let (ledger_changes, contract_events) = if invoke_result.is_ok() { - let ledger_changes = + let mut ledger_changes = get_ledger_changes(&budget, &storage, &*ledger_snapshot, init_ttl_map)?; + // Add the keys that only exist in the footprint, but not in the + // storage. This doesn't resemble anything in the enforcing mode, so use + // the shadow budget for this. + budget.with_shadow_mode(|| { + add_footprint_only_ledger_changes(budget, &storage, &mut ledger_changes) + }); + let encoded_contract_events = encode_contract_events(budget, &events)?; for e in &encoded_contract_events { contract_events_and_return_value_size = diff --git a/soroban-simulation/src/test/simulation.rs b/soroban-simulation/src/test/simulation.rs index ac1d5c937..58e4f7ac5 100644 --- a/soroban-simulation/src/test/simulation.rs +++ b/soroban-simulation/src/test/simulation.rs @@ -14,14 +14,17 @@ use soroban_env_host::e2e_testutils::{ }; use soroban_env_host::fees::{FeeConfiguration, RentFeeConfiguration}; use soroban_env_host::xdr::{ - ContractCostParamEntry, ContractCostParams, ContractCostType, ContractDataDurability, - ContractDataEntry, ExtensionPoint, LedgerEntry, LedgerEntryData, LedgerFootprint, LedgerKey, - LedgerKeyContractData, ScAddress, ScErrorCode, ScErrorType, ScNonceKey, ScVal, - SorobanAddressCredentials, SorobanAuthorizationEntry, SorobanCredentials, SorobanResources, - SorobanTransactionData, + AccountId, AlphaNum4, AssetCode4, ContractCostParamEntry, ContractCostParams, ContractCostType, + ContractDataDurability, ContractDataEntry, ContractExecutable, ExtensionPoint, Hash, + HostFunction, Int128Parts, InvokeContractArgs, LedgerEntry, LedgerEntryData, LedgerFootprint, + LedgerKey, LedgerKeyContractData, LedgerKeyTrustLine, PublicKey, ScAddress, ScBytes, + ScContractInstance, ScErrorCode, ScErrorType, ScMap, ScNonceKey, ScString, ScSymbol, ScVal, + SorobanAddressCredentials, SorobanAuthorizationEntry, SorobanAuthorizedFunction, + SorobanAuthorizedInvocation, SorobanCredentials, SorobanResources, SorobanTransactionData, + TrustLineAsset, TrustLineEntry, TrustLineEntryExt, TrustLineFlags, Uint256, VecM, }; use soroban_env_host::HostError; -use soroban_test_wasms::{ADD_I32, AUTH_TEST_CONTRACT}; +use soroban_test_wasms::{ADD_I32, AUTH_TEST_CONTRACT, TRY_CALL_SAC}; use std::rc::Rc; use tap::prelude::*; @@ -863,3 +866,286 @@ fn test_simulate_restore_op_returns_error_for_non_existent_entry() { ); assert!(res.is_err()); } + +fn sc_symbol(s: &str) -> ScVal { + ScVal::Symbol(s.try_into().unwrap()) +} + +fn sc_symbol_vec(s: &str) -> ScVal { + ScVal::Vec(Some(vec![sc_symbol(s)].try_into().unwrap())) +} + +fn create_sac_ledger_entry(sac_address: &ScAddress, admin_address: &ScAddress) -> LedgerEntry { + let contract_instance_entry = ContractDataEntry { + ext: ExtensionPoint::V0, + contract: sac_address.clone(), + key: ScVal::LedgerKeyContractInstance, + durability: ContractDataDurability::Persistent, + val: ScVal::ContractInstance(ScContractInstance { + executable: ContractExecutable::StellarAsset, + storage: Some( + ScMap::sorted_from_pairs( + [ + ( + sc_symbol_vec("Admin"), + ScVal::Address(admin_address.clone()), + ), + ( + sc_symbol("METADATA"), + ScVal::Map(Some( + ScMap::sorted_from_pairs( + [ + ( + sc_symbol("name"), + ScVal::String(ScString("aaaa".try_into().unwrap())), + ), + (sc_symbol("decimal"), ScVal::U32(7)), + ( + sc_symbol("symbol"), + ScVal::String(ScString("aaaa".try_into().unwrap())), + ), + ] + .into_iter(), + ) + .unwrap(), + )), + ), + ( + sc_symbol_vec("AssetInfo"), + ScVal::Vec(Some( + vec![ + sc_symbol("AlphaNum4"), + ScVal::Map(Some( + ScMap::sorted_from_pairs( + [ + ( + sc_symbol("asset_code"), + ScVal::String(ScString( + "aaaa".try_into().unwrap(), + )), + ), + ( + sc_symbol("issuer"), + ScVal::Bytes(ScBytes( + vec![0; 32].try_into().unwrap(), + )), + ), + ] + .into_iter(), + ) + .unwrap(), + )), + ] + .try_into() + .unwrap(), + )), + ), + ] + .into_iter(), + ) + .unwrap(), + ), + }), + }; + ledger_entry(LedgerEntryData::ContractData(contract_instance_entry)) +} + +#[test] +fn test_simulate_successful_sac_call() { + let source_account = get_account_id([123; 32]); + let other_account = get_account_id([124; 32]); + let sac_address = ScAddress::Contract(Hash([111; 32])); + let call_args: VecM<_> = vec![ + ScVal::Address(ScAddress::Account(other_account.clone())), + ScVal::I128(Int128Parts { hi: 0, lo: 1 }), + ] + .try_into() + .unwrap(); + let host_fn = HostFunction::InvokeContract(InvokeContractArgs { + contract_address: sac_address.clone(), + function_name: "mint".try_into().unwrap(), + args: call_args.clone(), + }); + let contract_instance_le = + create_sac_ledger_entry(&sac_address, &ScAddress::Account(source_account.clone())); + let trustline = TrustLineEntry { + account_id: other_account.clone(), + asset: TrustLineAsset::CreditAlphanum4(AlphaNum4 { + asset_code: AssetCode4([b'a'; 4]), + issuer: AccountId(PublicKey::PublicKeyTypeEd25519(Uint256([0; 32]))), + }), + balance: 0, + limit: 1_000_000_000, + flags: TrustLineFlags::AuthorizedFlag as u32, + ext: TrustLineEntryExt::V0, + }; + let trustline_le = ledger_entry(LedgerEntryData::Trustline(trustline)); + let ledger_info = default_ledger_info(); + let network_config = default_network_config(); + let snapshot_source = Rc::new( + MockSnapshotSource::from_entries( + vec![ + ( + contract_instance_le.clone(), + Some(ledger_info.sequence_number + 100), + ), + (trustline_le.clone(), None), + (account_entry(&source_account), None), + (account_entry(&other_account), None), + ], + ledger_info.sequence_number, + ) + .unwrap(), + ); + let res = simulate_invoke_host_function_op( + snapshot_source, + &network_config, + &SimulationAdjustmentConfig::no_adjustments(), + &ledger_info, + host_fn, + None, + &source_account, + [1; 32], + true, + ) + .unwrap(); + assert_eq!(res.invoke_result.unwrap(), ScVal::Void); + assert_eq!(res.contract_events.len(), 1); + assert_eq!( + res.auth, + vec![SorobanAuthorizationEntry { + credentials: SorobanCredentials::SourceAccount, + root_invocation: SorobanAuthorizedInvocation { + function: SorobanAuthorizedFunction::ContractFn(InvokeContractArgs { + contract_address: sac_address, + function_name: ScSymbol("mint".try_into().unwrap()), + args: call_args, + },), + sub_invocations: Default::default(), + }, + },] + ); + assert_eq!( + res.transaction_data, + Some(SorobanTransactionData { + ext: ExtensionPoint::V0, + resources: SorobanResources { + footprint: LedgerFootprint { + read_only: vec![ledger_entry_to_ledger_key(&contract_instance_le).unwrap(),] + .try_into() + .unwrap(), + read_write: vec![ledger_entry_to_ledger_key(&trustline_le).unwrap()] + .try_into() + .unwrap() + }, + instructions: 3302139, + read_bytes: 532, + write_bytes: 116, + }, + resource_fee: 28345, + }) + ); +} + +// This test covers an edge-case scenario of a SAC failure due to missing +// trustline handled with `try_call`, which had an issue in recording mode that +// led to incorrect footprint. +// While this doesn't have to be a SAC failure, the issue has been discovered +// in SAC specifically and seems more likely to occur compared to the regular +// contracts (as the regular contracts can normally create their entries, unlike +// the SAC/trustline case). +#[test] +fn test_simulate_unsuccessful_sac_call_with_try_call() { + let source_account = get_account_id([123; 32]); + let other_account = get_account_id([124; 32]); + let sac_address = ScAddress::Contract(Hash([111; 32])); + let contract = CreateContractData::new([1; 32], TRY_CALL_SAC); + let host_fn = HostFunction::InvokeContract(InvokeContractArgs { + contract_address: contract.contract_address.clone(), + function_name: "mint".try_into().unwrap(), + args: vec![ + ScVal::Address(sac_address.clone()), + ScVal::Address(ScAddress::Account(other_account.clone())), + ] + .try_into() + .unwrap(), + }); + let sac_instance_le = create_sac_ledger_entry(&sac_address, &contract.contract_address); + let ledger_info = default_ledger_info(); + let network_config = default_network_config(); + + let snapshot_source = Rc::new( + MockSnapshotSource::from_entries( + vec![ + ( + sac_instance_le.clone(), + Some(ledger_info.sequence_number + 100), + ), + ( + contract.wasm_entry.clone(), + Some(ledger_info.sequence_number + 100), + ), + ( + contract.contract_entry.clone(), + Some(ledger_info.sequence_number + 100), + ), + (account_entry(&source_account), None), + (account_entry(&other_account), None), + ], + ledger_info.sequence_number, + ) + .unwrap(), + ); + + let res = simulate_invoke_host_function_op( + snapshot_source, + &network_config, + &SimulationAdjustmentConfig::no_adjustments(), + &ledger_info, + host_fn, + None, + &source_account, + [1; 32], + true, + ) + .unwrap(); + // The return value indicates the whether the internal `mint` call has + // succeeded. + assert_eq!(res.invoke_result.unwrap(), ScVal::Bool(false)); + assert!(res.contract_events.is_empty()); + assert_eq!(res.auth, vec![]); + let trustline_key = LedgerKey::Trustline(LedgerKeyTrustLine { + account_id: other_account.clone(), + asset: TrustLineAsset::CreditAlphanum4(AlphaNum4 { + asset_code: AssetCode4([b'a'; 4]), + issuer: AccountId(PublicKey::PublicKeyTypeEd25519(Uint256([0; 32]))), + }), + }); + assert_eq!( + res.transaction_data, + Some(SorobanTransactionData { + ext: ExtensionPoint::V0, + resources: SorobanResources { + footprint: LedgerFootprint { + read_only: vec![ + // Trustline key must appear in the footprint, even + // though it's not present in the storage. + trustline_key, + contract.wasm_key.clone(), + contract.contract_key.clone(), + ledger_entry_to_ledger_key(&sac_instance_le).unwrap(), + ] + .tap_mut(|v| v.sort()) + .try_into() + .unwrap(), + // No entries should be actually modified. + read_write: Default::default(), + }, + instructions: 5768570, + read_bytes: 1196, + write_bytes: 0, + }, + resource_fee: 6224, + }) + ); +} diff --git a/soroban-test-wasms/src/lib.rs b/soroban-test-wasms/src/lib.rs index cc3985561..7c7ed4350 100644 --- a/soroban-test-wasms/src/lib.rs +++ b/soroban-test-wasms/src/lib.rs @@ -95,6 +95,9 @@ pub const HOSTILE_LARGE_VALUE: &[u8] = pub const DEPLOYER_TEST_CONTRACT: &[u8] = include_bytes!("../wasm-workspace/opt/20/test_deployer.wasm").as_slice(); +pub const TRY_CALL_SAC: &[u8] = + include_bytes!("../wasm-workspace/opt/20/test_try_call_sac.wasm").as_slice(); + // Protocol 21 Wasms. pub const CONSTRUCTOR_TEST_CONTRACT_P21: &[u8] = include_bytes!("../wasm-workspace/opt/21/test_constructor.wasm").as_slice(); diff --git a/soroban-test-wasms/wasm-workspace/Cargo.toml b/soroban-test-wasms/wasm-workspace/Cargo.toml index c10342a4a..1ffc472dd 100644 --- a/soroban-test-wasms/wasm-workspace/Cargo.toml +++ b/soroban-test-wasms/wasm-workspace/Cargo.toml @@ -49,7 +49,8 @@ members = [ "deployer_with_constructor", "constructor_with_return_value", "constructor_with_result", - "custom_account_context" + "custom_account_context", + "try_call_sac" ] [profile.release] opt-level = "z" diff --git a/soroban-test-wasms/wasm-workspace/opt/20/test_try_call_sac.wasm b/soroban-test-wasms/wasm-workspace/opt/20/test_try_call_sac.wasm new file mode 100644 index 0000000000000000000000000000000000000000..e081fe593b913500ab3616635dcff057c346082c GIT binary patch literal 578 zcmY*W%TC)s6ul3UMwoyRY}lYlVbe`&I|+eZ>;>J^4=`hUL~G;(d8|lml2jlzNc;&a zet;k7rknOV_+DS7?Fru9Ea zzYuA9Ff#$6xco?mwxof8=r7(xKjEwi0^igW-BX_%f&}&%LTE7$44p&wOVhSAVB4NH zQBdgSf^$RW{1bzDV2F*p#T(!_X2rwH)9($-dW^(fw00Zan*GAHCfpx$S1mBO$QQnr zuK4V6%UOv>#?D(=a0ndux|xoKRVmX-U#WELnLDpyaU65x<-pN0jb)aVO6&P(XE~>8 zWUF~^{|WZDSE6b-E>tC7b$2au^t$vNSS$5dl?&Cn-#!l7|LrtNt+UGo{5kO2y|#C7 kqJp&J1wqp7^m<7UbY)*2`(5RC64~?nN4`>t-%S$u2OyYuOaK4? literal 0 HcmV?d00001 diff --git a/soroban-test-wasms/wasm-workspace/try_call_sac/Cargo.toml b/soroban-test-wasms/wasm-workspace/try_call_sac/Cargo.toml new file mode 100644 index 000000000..9ff7a430b --- /dev/null +++ b/soroban-test-wasms/wasm-workspace/try_call_sac/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "test_try_call_sac" +version = "0.0.0" +authors = ["Stellar Development Foundation "] +license = "Apache-2.0" +edition = "2021" +rust-version.workspace = true + +[lib] +crate-type = ["cdylib", "rlib"] +doctest = false + +[dependencies] +soroban-sdk = { workspace = true } diff --git a/soroban-test-wasms/wasm-workspace/try_call_sac/src/lib.rs b/soroban-test-wasms/wasm-workspace/try_call_sac/src/lib.rs new file mode 100644 index 000000000..8ed00ecce --- /dev/null +++ b/soroban-test-wasms/wasm-workspace/try_call_sac/src/lib.rs @@ -0,0 +1,18 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Error, IntoVal}; + +#[contract] +struct TryCallSac; + +#[contractimpl] +impl TryCallSac { + pub fn mint(env: Env, sac_address: Address, to: Address) -> bool { + let res = env.try_invoke_contract::<(), Error>( + &sac_address, + &symbol_short!("mint"), + (to, 1_i128).into_val(&env), + ); + res.is_ok() + } +}