From b53e6d4783676fa410a19b3f7b0f7ee8e9ec2de1 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 22 Aug 2023 02:31:23 +0200 Subject: [PATCH] soroban-rpc: simulateTransaction: try to return diagnostic events on failure (#878) --- .../internal/methods/simulate_transaction.go | 7 +- .../internal/preflight/preflight.go | 14 +-- .../internal/preflight/preflight_test.go | 1 - .../test/simulate_transaction_test.go | 3 + cmd/soroban-rpc/lib/preflight/src/fees.rs | 10 +- cmd/soroban-rpc/lib/preflight/src/lib.rs | 7 +- .../lib/preflight/src/preflight.rs | 104 ++++++++++-------- 7 files changed, 84 insertions(+), 62 deletions(-) diff --git a/cmd/soroban-rpc/internal/methods/simulate_transaction.go b/cmd/soroban-rpc/internal/methods/simulate_transaction.go index b0042b880..de19432e8 100644 --- a/cmd/soroban-rpc/internal/methods/simulate_transaction.go +++ b/cmd/soroban-rpc/internal/methods/simulate_transaction.go @@ -35,11 +35,11 @@ type RestorePreamble struct { type SimulateTransactionResponse struct { Error string `json:"error,omitempty"` - TransactionData string `json:"transactionData"` // SorobanTransactionData XDR in base64 - MinResourceFee int64 `json:"minResourceFee,string"` + TransactionData string `json:"transactionData,omitempty"` // SorobanTransactionData XDR in base64 + MinResourceFee int64 `json:"minResourceFee,string,omitempty"` Events []string `json:"events,omitempty"` // DiagnosticEvent XDR in base64 Results []SimulateHostFunctionResult `json:"results,omitempty"` // an array of the individual host function call results - Cost SimulateTransactionCost `json:"cost"` // the effective cpu and memory cost of the invoked transaction execution. + Cost SimulateTransactionCost `json:"cost,omitempty"` // the effective cpu and memory cost of the invoked transaction execution. RestorePreamble RestorePreamble `json:"restorePreamble,omitempty"` // If present, it indicates that a prior RestoreFootprint is required LatestLedger int64 `json:"latestLedger,string"` } @@ -136,6 +136,7 @@ func NewSimulateTransactionHandler(logger *log.Entry, ledgerEntryReader db.Ledge } return SimulateTransactionResponse{ + Error: result.Error, Results: results, Events: result.Events, TransactionData: result.TransactionData, diff --git a/cmd/soroban-rpc/internal/preflight/preflight.go b/cmd/soroban-rpc/internal/preflight/preflight.go index bba7d57cc..b2d2d5106 100644 --- a/cmd/soroban-rpc/internal/preflight/preflight.go +++ b/cmd/soroban-rpc/internal/preflight/preflight.go @@ -77,6 +77,7 @@ type PreflightParameters struct { } type Preflight struct { + Error string Events []string // DiagnosticEvents XDR in base64 TransactionData string // SorobanTransactionData XDR in base64 MinFee int64 @@ -147,7 +148,7 @@ func getFootprintExpirationPreflight(params PreflightParameters) (Preflight, err C.free(unsafe.Pointer(opBodyCString)) C.free(unsafe.Pointer(footprintCString)) - return GoPreflight(res) + return GoPreflight(res), nil } func getSimulationLedgerSeq(readTx db.LedgerEntryReadTx) (uint32, error) { @@ -218,17 +219,14 @@ func getInvokeHostFunctionPreflight(params PreflightParameters) (Preflight, erro C.free(unsafe.Pointer(invokeHostFunctionCString)) C.free(unsafe.Pointer(sourceAccountCString)) - return GoPreflight(res) + return GoPreflight(res), nil } -func GoPreflight(result *C.CPreflightResult) (Preflight, error) { +func GoPreflight(result *C.CPreflightResult) Preflight { defer C.free_preflight_result(result) - if result.error != nil { - return Preflight{}, errors.New(C.GoString(result.error)) - } - preflight := Preflight{ + Error: C.GoString(result.error), Events: GoNullTerminatedStringSlice(result.events), TransactionData: C.GoString(result.transaction_data), MinFee: int64(result.min_fee), @@ -239,5 +237,5 @@ func GoPreflight(result *C.CPreflightResult) (Preflight, error) { PreRestoreTransactionData: C.GoString(result.pre_restore_transaction_data), PreRestoreMinFee: int64(result.pre_restore_min_fee), } - return preflight, nil + return preflight } diff --git a/cmd/soroban-rpc/internal/preflight/preflight_test.go b/cmd/soroban-rpc/internal/preflight/preflight_test.go index 392cba9cd..44bfcee26 100644 --- a/cmd/soroban-rpc/internal/preflight/preflight_test.go +++ b/cmd/soroban-rpc/internal/preflight/preflight_test.go @@ -305,7 +305,6 @@ func getPreflightParameters(t testing.TB, inMemory bool) PreflightParameters { } func TestGetPreflight(t *testing.T) { - params := getPreflightParameters(t, false) _, err := GetPreflight(context.Background(), params) require.NoError(t, err) diff --git a/cmd/soroban-rpc/internal/test/simulate_transaction_test.go b/cmd/soroban-rpc/internal/test/simulate_transaction_test.go index 739b9b1b1..379c962c2 100644 --- a/cmd/soroban-rpc/internal/test/simulate_transaction_test.go +++ b/cmd/soroban-rpc/internal/test/simulate_transaction_test.go @@ -526,6 +526,9 @@ func TestSimulateTransactionError(t *testing.T) { result := simulateTransactionFromTxParams(t, client, params) assert.Greater(t, result.LatestLedger, int64(0)) assert.Contains(t, result.Error, "MissingValue") + require.Len(t, result.Events, 1) + var event xdr.DiagnosticEvent + require.NoError(t, xdr.SafeUnmarshalBase64(result.Events[0], &event)) } func TestSimulateTransactionMultipleOperations(t *testing.T) { diff --git a/cmd/soroban-rpc/lib/preflight/src/fees.rs b/cmd/soroban-rpc/lib/preflight/src/fees.rs index 4ca4e93d2..c0c59fb85 100644 --- a/cmd/soroban-rpc/lib/preflight/src/fees.rs +++ b/cmd/soroban-rpc/lib/preflight/src/fees.rs @@ -364,7 +364,7 @@ fn compute_bump_footprint_rent_changes( for key in (&footprint).read_only.as_vec() { let unmodified_entry = ledger_storage .get(key, false) - .with_context(|| format!("cannot get ledger entry with key {key:?}"))?; + .with_context(|| format!("cannot find bump footprint ledger entry with key {key:?}"))?; let size = (key.to_xdr()?.len() + unmodified_entry.to_xdr()?.len()) as u32; let expirable_entry: Box = (&unmodified_entry).try_into().map_err(|e: String| { @@ -396,7 +396,7 @@ pub(crate) fn compute_restore_footprint_transaction_data_and_min_fee( let ConfigSettingEntry::StateExpiration(state_expiration) = ledger_storage.get_configuration_setting(ConfigSettingId::StateExpiration)? else { - bail!("get_fee_configuration(): unexpected config setting entry for StateExpiration key"); + bail!("unexpected config setting entry for StateExpiration key"); }; let rent_changes = compute_restore_footprint_rent_changes( &footprint, @@ -454,9 +454,9 @@ fn compute_restore_footprint_rent_changes( let mut rent_changes: Vec = Vec::with_capacity(footprint.read_write.len()); for key in footprint.read_write.as_vec() { - let unmodified_entry = ledger_storage - .get(key, true) - .with_context(|| format!("cannot get ledger entry with key {key:?}"))?; + let unmodified_entry = ledger_storage.get(key, true).with_context(|| { + format!("cannot find restore footprint ledger entry with key {key:?}") + })?; let size = (key.to_xdr()?.len() + unmodified_entry.to_xdr()?.len()) as u32; let expirable_entry: Box = (&unmodified_entry).try_into().map_err(|e: String| { diff --git a/cmd/soroban-rpc/lib/preflight/src/lib.rs b/cmd/soroban-rpc/lib/preflight/src/lib.rs index 747a9a8a0..0c461b696 100644 --- a/cmd/soroban-rpc/lib/preflight/src/lib.rs +++ b/cmd/soroban-rpc/lib/preflight/src/lib.rs @@ -69,13 +69,16 @@ pub struct CPreflightResult { impl From for CPreflightResult { fn from(p: PreflightResult) -> Self { let mut result = Self { - error: null_mut(), + error: string_to_c(p.error), auth: xdr_vec_to_base64_c_null_terminated_char_array(p.auth), result: match p.result { None => null_mut(), Some(v) => xdr_to_base64_c(v), }, - transaction_data: xdr_to_base64_c(p.transaction_data), + transaction_data: match p.transaction_data { + None => null_mut(), + Some(v) => xdr_to_base64_c(v), + }, min_fee: p.min_fee, events: xdr_vec_to_base64_c_null_terminated_char_array(p.events), cpu_instructions: p.cpu_instructions, diff --git a/cmd/soroban-rpc/lib/preflight/src/preflight.rs b/cmd/soroban-rpc/lib/preflight/src/preflight.rs index fcd378f1e..e1b64bf29 100644 --- a/cmd/soroban-rpc/lib/preflight/src/preflight.rs +++ b/cmd/soroban-rpc/lib/preflight/src/preflight.rs @@ -11,6 +11,7 @@ use soroban_env_host::xdr::{ SorobanAuthorizationEntry, SorobanCredentials, SorobanTransactionData, VecM, }; use soroban_env_host::{DiagnosticLevel, Host, LedgerInfo}; +use std::collections::HashSet; use std::convert::{TryFrom, TryInto}; use std::iter::FromIterator; use std::rc::Rc; @@ -20,10 +21,12 @@ pub(crate) struct RestorePreamble { pub(crate) min_fee: i64, } +#[derive(Default)] pub(crate) struct PreflightResult { + pub(crate) error: String, pub(crate) auth: Vec, pub(crate) result: Option, - pub(crate) transaction_data: SorobanTransactionData, + pub(crate) transaction_data: Option, pub(crate) min_fee: i64, pub(crate) events: Vec, pub(crate) cpu_instructions: u64, @@ -68,9 +71,9 @@ pub(crate) fn preflight_invoke_hf_op( .context("cannot set ledger info")?; // Run the preflight. - let result = host + let maybe_result = host .invoke_function(invoke_hf_op.host_function.clone()) - .context("host invocation failed")?; + .context("host invocation failed"); let auths: VecM = if needs_auth_recording { let payloads = host.get_recorded_auth_payloads()?; VecM::try_from( @@ -88,6 +91,19 @@ pub(crate) fn preflight_invoke_hf_op( let (storage, events) = host.try_finish().context("cannot finish host invocation")?; let diagnostic_events = host_events_to_diagnostic_events(&events); + let result = match maybe_result { + Ok(r) => r, + // If the invocation failed, try to at least add the diagnostic events + Err(e) => { + return Ok(PreflightResult { + // See https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations + error: format!("{e:?}"), + events: diagnostic_events, + ..Default::default() + }); + } + }; + let invoke_host_function_with_auth = InvokeHostFunctionOp { host_function: invoke_hf_op.host_function, auth: auths.clone(), @@ -103,33 +119,18 @@ pub(crate) fn preflight_invoke_hf_op( ) .context("cannot compute resources and fees")?; - let entries = ledger_storage_rc.get_ledger_keys_requiring_restore(); - let restore_preamble = if entries.is_empty() { - None - } else { - let read_write_vec: Vec = Vec::from_iter(entries); - let restore_footprint = LedgerFootprint { - read_only: VecM::default(), - read_write: read_write_vec.try_into()?, - }; - let (transaction_data, min_fee) = - fees::compute_restore_footprint_transaction_data_and_min_fee( - restore_footprint, - &ledger_storage_rc, - bucket_list_size, - ledger_info.sequence_number, - ) - .context("cannot compute restore preamble")?; - Some(RestorePreamble { - transaction_data, - min_fee, - }) - }; + let restore_preamble = compute_restore_preamble( + ledger_storage_rc.get_ledger_keys_requiring_restore(), + &ledger_storage_rc, + bucket_list_size, + ledger_info.sequence_number, + ) + .context("cannot compute restore preamble")?; Ok(PreflightResult { auth: auths.to_vec(), result: Some(result), - transaction_data, + transaction_data: Some(transaction_data), min_fee, events: diagnostic_events, cpu_instructions: budget @@ -139,6 +140,7 @@ pub(crate) fn preflight_invoke_hf_op( .get_mem_bytes_consumed() .context("cannot get consumed memory")?, restore_preamble, + ..Default::default() }) } @@ -168,6 +170,32 @@ fn recorded_auth_payload_to_xdr( Ok(result) } +fn compute_restore_preamble( + entries: HashSet, + ledger_storage: &LedgerStorage, + bucket_list_size: u64, + current_ledger_seq: u32, +) -> Result> { + if entries.is_empty() { + return Ok(None); + } + let read_write_vec: Vec = Vec::from_iter(entries); + let restore_footprint = LedgerFootprint { + read_only: VecM::default(), + read_write: read_write_vec.try_into()?, + }; + let (transaction_data, min_fee) = fees::compute_restore_footprint_transaction_data_and_min_fee( + restore_footprint, + ledger_storage, + bucket_list_size, + current_ledger_seq, + )?; + Ok(Some(RestorePreamble { + transaction_data, + min_fee, + })) +} + fn host_events_to_diagnostic_events(events: &Events) -> Vec { let mut res: Vec = Vec::with_capacity(events.0.len()); for e in &events.0 { @@ -184,19 +212,19 @@ fn get_budget_from_network_config_params(ledger_storage: &LedgerStorage) -> Resu let ConfigSettingEntry::ContractComputeV0(compute) = ledger_storage.get_configuration_setting(ConfigSettingId::ContractComputeV0)? else { - bail!("get_budget_from_network_config_params(): unexpected config setting entry for ComputeV0 key"); + bail!("unexpected config setting entry for ComputeV0 key"); }; let ConfigSettingEntry::ContractCostParamsCpuInstructions(cost_params_cpu) = ledger_storage .get_configuration_setting(ConfigSettingId::ContractCostParamsCpuInstructions)? else { - bail!("get_budget_from_network_config_params(): unexpected config setting entry for ComputeV0 key"); + bail!("unexpected config setting entry for CostParamsCpuInstructions key"); }; let ConfigSettingEntry::ContractCostParamsMemoryBytes(cost_params_memory) = ledger_storage.get_configuration_setting(ConfigSettingId::ContractCostParamsMemoryBytes)? else { - bail!("get_budget_from_network_config_params(): unexpected config setting entry for ComputeV0 key"); + bail!("unexpected config setting entry for CostParamsMemoryBytes key"); }; let budget = Budget::try_from_configs( @@ -253,14 +281,9 @@ fn preflight_bump_footprint_expiration( current_ledger_seq, )?; Ok(PreflightResult { - auth: vec![], - result: None, - transaction_data, + transaction_data: Some(transaction_data), min_fee, - events: vec![], - cpu_instructions: 0, - memory_bytes: 0, - restore_preamble: None, + ..Default::default() }) } @@ -277,13 +300,8 @@ fn preflight_restore_footprint( current_ledger_seq, )?; Ok(PreflightResult { - auth: vec![], - result: None, - transaction_data, + transaction_data: Some(transaction_data), min_fee, - events: vec![], - cpu_instructions: 0, - memory_bytes: 0, - restore_preamble: None, + ..Default::default() }) }