tighten up handling of debug mode

soroban-env-host/Cargo.toml

@@ -26,7 +26,7 @@ rand_chacha = "0.3.1"
 num-traits = "0.2.15"
 num-integer = "0.1.45"
 num-derive = "0.4.0"
-backtrace = "0.3"
+backtrace = { version = "0.3", optional = true }
 k256 = {version = "0.13.1", features=["ecdsa", "arithmetic"]}
 # NB: getrandom is a transitive dependency of k256 which we're not using directly
 # but we have to specify it here in order to enable its 'js' feature which
@@ -55,9 +55,10 @@ expect-test = "1.4.0"
 more-asserts = "0.3.1"
 linregress = "0.5.1"
 pretty_assertions = "1.4.0"
+backtrace = "0.3"
 
 [features]
-testutils = ["soroban-env-common/testutils", "soroban-synth-wasm/testutils", "recording_auth"]
+testutils = ["soroban-env-common/testutils", "soroban-synth-wasm/testutils", "recording_auth", "dep:backtrace"]
 next = ["soroban-env-common/next", "soroban-synth-wasm/next", "soroban-bench-utils/next"]
 tracy = ["dep:tracy-client", "soroban-env-common/tracy"]
 recording_auth = []

soroban-env-host/src/auth.rs

@@ -1304,30 +1304,25 @@ impl AuthorizationManager {
         &self,
         host: &Host,
     ) -> Vec<(ScAddress, xdr::SorobanAuthorizedInvocation)> {
-        let inv: Option<Vec<(ScAddress, xdr::SorobanAuthorizedInvocation)>> =
-            host.as_budget().with_shadow_mode(
-                || {
-                    let inv = self
-                        .account_trackers
-                        .borrow()
-                        .iter()
-                        .filter(|t| t.borrow().verified)
-                        .map(|t| {
-                            (
-                                host.scaddress_from_address(t.borrow().address).unwrap(),
-                                t.borrow()
-                                    .invocation_tracker
-                                    .root_authorized_invocation
-                                    .to_xdr(host, true)
-                                    .unwrap(),
-                            )
-                        })
-                        .metered_collect(host)?;
-                    Ok(Some(inv))
-                },
-                || None,
-            );
-        inv.unwrap()
+        host.as_budget()
+            .with_observable_shadow_mode(|| {
+                self.account_trackers
+                    .borrow()
+                    .iter()
+                    .filter(|t| t.borrow().verified)
+                    .map(|t| {
+                        (
+                            host.scaddress_from_address(t.borrow().address).unwrap(),
+                            t.borrow()
+                                .invocation_tracker
+                                .root_authorized_invocation
+                                .to_xdr(host, true)
+                                .unwrap(),
+                        )
+                    })
+                    .metered_collect(host)
+            })
+            .unwrap()
     }
 }
 
@@ -1695,7 +1690,7 @@ impl AccountAuthorizationTracker {
     // metering: free for recording
     #[cfg(any(test, feature = "recording_auth"))]
     fn get_recorded_auth_payload(&self, host: &Host) -> Result<RecordedAuthPayload, HostError> {
-        host.as_budget().with_shadow_mode_fallible(|| {
+        host.as_budget().with_observable_shadow_mode(|| {
             Ok(RecordedAuthPayload {
                 address: if !self.is_invoker {
                     Some(host.visit_obj(self.address, |a: &ScAddress| a.metered_clone(host))?)

soroban-env-host/src/budget.rs

@@ -665,49 +665,36 @@ impl Budget {
         self.0.try_borrow_mut_or_err()?.charge(ty, 1, input)
     }
 
-    /// Runs a user provided closure in shadow mode -- all metering is done through
-    /// the shadow budget.
+    /// Runs a user provided closure in shadow mode -- all metering is done
+    /// through the shadow budget.
     ///
-    /// Because the shadow mode is optional (depending on the configuration,
-    /// nodes may or may not trigger this path), any error occured during execution
-    /// is swallowed by running a fallback closure provided by the caller.
-    ///
-    /// # Arguments:
-    /// * `f` - A fallible closure to be run in shadow mode. If error occurs,
-    ///   fallback closure is run immediately afterwards to replace it
-    ///
-    /// * `fallback` - A fallback closure to be run in case of any error occuring
-    ///
-    /// # Returns:
-    ///
-    /// Returns a value of type `T`. Any errors arising during the execution are
-    /// suppressed.
-    pub(crate) fn with_shadow_mode<T, F, E>(&self, f: F, fallback: E) -> T
+    /// Because shadow mode is _designed not to be observed_ (indeed it exists
+    /// primarily to count actions against the shadow budget that are _optional_
+    /// on a given host, such as debug logging, and that therefore must strictly
+    /// must not be observed), any error that occurs during execution is
+    /// swallowed.
+    pub(crate) fn with_shadow_mode<T, F>(&self, f: F)
     where
         F: FnOnce() -> Result<T, HostError>,
-        E: FnOnce() -> T,
     {
         let mut prev = false;
 
-        let mut res = self
+        if self
             .mut_budget(|mut b| {
                 prev = b.is_in_shadow_mode;
                 b.is_in_shadow_mode = true;
-                b.cpu_insns.check_budget_limit(true)?;
-                b.mem_bytes.check_budget_limit(true)
+                b.cpu_insns.check_budget_limit(IsShadowMode(true))?;
+                b.mem_bytes.check_budget_limit(IsShadowMode(true))
             })
-            .and_then(|_| f());
+            .is_ok()
+        {
+            let _ = f();
+        }
 
-        if let Err(_) = self.mut_budget(|mut b| {
+        let _ = self.mut_budget(|mut b| {
             b.is_in_shadow_mode = prev;
             Ok(())
-        }) {
-            res = Err(
-                Error::from_type_and_code(ScErrorType::Budget, ScErrorCode::InternalError).into(),
-            );
-        }
-
-        res.unwrap_or_else(|_| fallback())
+        });
     }
 
     pub(crate) fn is_in_shadow_mode(&self) -> Result<bool, HostError> {

soroban-env-host/src/budget/dimension.rs

@@ -116,8 +116,8 @@ impl BudgetDimension {
         self.shadow_total_count = 0;
     }
 
-    pub(crate) fn check_budget_limit(&self, is_shadow: bool) -> Result<(), HostError> {
-        let over_limit = if is_shadow {
+    pub(crate) fn check_budget_limit(&self, is_shadow: IsShadowMode) -> Result<(), HostError> {
+        let over_limit = if is_shadow.0 {
             self.shadow_total_count > self.shadow_limit
         } else {
             self.total_count > self.limit
@@ -163,7 +163,7 @@ impl BudgetDimension {
             self.total_count = self.total_count.saturating_add(amount);
         }
 
-        self.check_budget_limit(is_shadow.0)
+        self.check_budget_limit(is_shadow)
     }
 
     // Resets all model parameters to zero (so that we can override and test individual ones later).

soroban-env-host/src/budget/util.rs

@@ -142,26 +142,37 @@ impl Budget {
 
 #[cfg(any(test, feature = "recording_auth"))]
 impl Budget {
-    /// Fallible version of `with_shadow_mode`, enabled only in testing and
-    /// non-production scenarios. The non-fallible `with_shadow_mode` is the
-    /// preferred method and should be used if at all possible.
+    /// Variant of `with_shadow_mode`, enabled only in testing and
+    /// non-production scenarios, that produces a `Result<>` rather than eating
+    /// errors and return values the way `with_shadow_mode` does.
+    ///
+    /// This is undesirable specifically because it has a return value which may
+    /// vary between `Ok` and `Err` depending on whether the shadow budget is
+    /// exhausted, and the shadow budget varies based on debug status --
+    /// something that is not identical from one host to the next.
+    ///
     /// However, in testing and non-production workflows, sometimes we need the
     /// convenience of temporarily "turning off" the budget. This can happen for
-    /// several reasons: we want the some test logic to not affect the production
-    /// budget, or we want to maintain an accurate prediction of production budget
-    /// during preflight. In the latter case, we want to exclude preflight-only
-    /// logic from the budget. By routing metering to the shadow budget instead
-    /// of turning the budget off completely, it offers some DOS-mitigation.
-    pub(crate) fn with_shadow_mode_fallible<T, F>(&self, f: F) -> Result<T, HostError>
+    /// several reasons: we want the some test logic to not affect the
+    /// production budget, or we want to maintain an accurate prediction of
+    /// production budget during preflight. In the latter case, we want to
+    /// exclude preflight-only logic from the budget. By routing metering to the
+    /// shadow budget instead of turning the budget off completely, it offers
+    /// some DOS-mitigation.
+    ///
+    /// If in doubt, do not use this function.
+    pub(crate) fn with_observable_shadow_mode<T, F>(&self, f: F) -> Result<T, HostError>
     where
         F: FnOnce() -> Result<T, HostError>,
     {
         let mut prev = false;
         let should_execute = self.mut_budget(|mut b| {
             prev = b.is_in_shadow_mode;
             b.is_in_shadow_mode = true;
-            b.cpu_insns.check_budget_limit(true)?;
-            b.mem_bytes.check_budget_limit(true)
+            b.cpu_insns
+                .check_budget_limit(super::dimension::IsShadowMode(true))?;
+            b.mem_bytes
+                .check_budget_limit(super::dimension::IsShadowMode(true))
         });
 
         let rt = match should_execute {