Work around declared sizes of replaced ExtensionPoints, horribly.

soroban-env-host/src/host/data_helper.rs

@@ -144,7 +144,9 @@ impl Host {
                 match &e.ext {
                     crate::xdr::ContractCodeEntryExt::V0 => (),
                     crate::xdr::ContractCodeEntryExt::V1(v1) => {
-                        costs = VersionedContractCodeCostInputs::V1(v1.cost_inputs.clone())
+                        costs = VersionedContractCodeCostInputs::V1(
+                            v1.cost_inputs.metered_clone(self.as_budget())?,
+                        )
                     }
                 };
                 Ok((code, costs))

soroban-env-host/src/host/declared_size.rs

@@ -1,3 +1,5 @@
+#[cfg(feature = "next")]
+use crate::xdr::{ContractCodeCostInputs, ContractCodeEntryV1};
 use crate::{
     auth::{
         AccountAuthorizationTracker, AccountAuthorizationTrackerSnapshot, AuthorizedInvocation,
@@ -157,7 +159,11 @@ impl_declared_size_type!(LedgerKeyContractCode, 36);
 impl_declared_size_type!(LedgerEntryExt, 33);
 impl_declared_size_type!(AccountEntry, 216);
 impl_declared_size_type!(TrustLineEntry, 128);
+#[cfg(feature = "next")]
+impl_declared_size_type!(ContractCodeCostInputs, 40);
 impl_declared_size_type!(ContractCodeEntry, 64);
+#[cfg(feature = "next")]
+impl_declared_size_type!(ContractCodeEntryV1, 40);
 // TtlEntry must be declared as it's used in e2e to build
 // The TtlEntryMap, but is not otherwise cloned anywhere.
 impl_declared_size_type!(TtlEntry, 36);
@@ -172,7 +178,80 @@ impl_declared_size_type!(CreateContractArgs, 98);
 impl_declared_size_type!(InvokeContractArgs, 88);
 impl_declared_size_type!(ContractIdPreimage, 65);
 impl_declared_size_type!(ContractDataDurability, 4);
+
+// NB: ExtensionPoint is a 1-variant enum with no payload, which Rust optimizes
+// to take zero bytes of memroy -- but in XDR it's a 4-byte type like any other
+// union.
+//
+// It exists to help allow the protocol to evolve, as a placeholder type in XDR.
+// ExtensionPoints are sprinkled around the XDR definitions where we expect to
+// need to add stuff in the future. Any field with ExtensionPoint as its type
+// can be _replaced_ in subsequent protocols with some _other_ union type that
+// has a variant with a zero discriminant, and some other non-zero-discriminant
+// variants with new payloads. If old data (written when the field was
+// ExtensionPoint) is read by new code (which knows about the new union type),
+// the new code will just interpret the zero-discriminant old value as the zero
+// case in the new type. All well and good!
+//
+// But none of this works in Rust, because Rust doesn't treat unions (enums) as
+// variable size: it allocates space for the largest variant that can occupy the
+// enum, and it allocates zero bytes for ExtensionPoints. So when you upgrade a
+// union field in XDR, if the new variant is bigger than all existing variants,
+// the Rust size of the field will just change (and since ExtensionPoint is
+// zero-sized, this happens _any_ time you replace an ExtensionPoint with some
+// nonempty type). There's no real getting around this.
+//
+// "Luckily" (from the perspective of deterministic replay) we don't charge the
+// cost model based on Rust's "real sizes" of anything; we charge the cost model
+// based on stable _declared_ sizes we write down explicitly here. So what
+// happens when you upgrade a union field in a way that changes the Rust size of
+// a type is that the Rust size _diverges a bit_ from the declared size: in
+// other words the metering based on declared sizes gets a bit inaccurate, and
+// the test below (`test_declared_size`) that checks real sizes are less than or
+// equal to declared sizes needs to have an exception written into it to handle
+// this divergence.
+//
+// This is not ideal. It means in some cases the metering will be off, at least
+// when reading old data into the new enum (and, say, cloning it) we'll treat it
+// as small when really it is a bit bigger.
+//
+// We can recover a degree of correctness at least in the cases where we're
+// writing new data (occupying the new variants of the extended enum) by
+// essentially pretending that the ext field is a sort of magical smart pointer
+// that occupies zero bytes of its own but points to the new variant as "deep
+// substructure" (the same way we do with real pointer fields).
+//
+// In other words, we can use the following idiom:
+//
+//  - Assume the enclosing struct was `S`, and it previously had an
+//    `ExtensionPoint` `S.ext`, and now it has some other type in `S.ext`. Let's
+//    call the new type we want to add `T` and say we added it by replacing the
+//    `ExtensionPoint` with an enum `E` with variants `V0` (covering the old
+//    `ExtensionPoint`) and `V1(T)`.
+//
+//  - Previously we charged nothing to clone `S.ext`, because it was zero-sized.
+//    So we have to continue to charge nothing when cloning the new field when
+//    it is in its `E::V0` state: that's what the old `ExtensionPoint` data will
+//    be interpreted as when it's deserialized, so we have to treat it as we did
+//    before. This is the case where metering has become incorrect, because `E`
+//    is larger than `ExtensionPoint` but we can't acknowledge that fact.
+//
+//  - To charge nothing for `E::V0`, we actually have to _not_ declare `E` as a
+//    `MeteredClone` / `declared_size` type itself. Rather, inside the body of
+//    `S::charge_for_substructure` we match on `S.ext` and do nothing in the
+//    `E::V0` case, only do nonzero work in the `E::V1` case.
+//
+//  - In the `E::V1` case we can call through to `T::charge_for_substructure`,
+//    treating it almost like it was a separate heap allocation. It still gets
+//    charged for, just "as if" it were out-of-line, rather than inline with `S`
+//    where it happens to be located.
+//
+// This is all a bit weird and awful. It's a mistake that we only noticed after
+// we finalized the metering system of Soroban. We're stuck with it at this
+// point. Hopefully most instances will be fairly benign (and if types get
+// upgraded to their new variants, hopefully also transient!)
 impl_declared_size_type!(ExtensionPoint, 0);
+
 impl_declared_size_type!(ScContractInstance, 64);
 impl_declared_size_type!(SorobanAuthorizationEntry, 240);
 impl_declared_size_type!(SorobanAuthorizedInvocation, 128);
@@ -382,7 +461,17 @@ mod test {
         expect!["33"].assert_eq(size_of::<LedgerEntryExt>().to_string().as_str());
         expect!["216"].assert_eq(size_of::<AccountEntry>().to_string().as_str());
         expect!["128"].assert_eq(size_of::<TrustLineEntry>().to_string().as_str());
+        #[cfg(feature = "next")]
+        expect!["40"].assert_eq(size_of::<ContractCodeCostInputs>().to_string().as_str());
+        #[cfg(not(feature = "next"))]
         expect!["56"].assert_eq(size_of::<ContractCodeEntry>().to_string().as_str());
+        // ContractCodeEntry had an ExtensionPoint added to it and is now 40
+        // bytes larger than its original size (and for some reason its declared
+        // size was 64 bytes, even though its original size wasonly 56 bytes)
+        #[cfg(feature = "next")]
+        expect!["104"].assert_eq(size_of::<ContractCodeEntry>().to_string().as_str());
+        #[cfg(feature = "next")]
+        expect!["40"].assert_eq(size_of::<ContractCodeEntryV1>().to_string().as_str());
         expect!["36"].assert_eq(size_of::<TtlEntry>().to_string().as_str());
 
         // NB: a couple structs shrank between rust 1.75 and 1.76 but this is harmless
@@ -448,6 +537,19 @@ mod test {
                     <$t as DeclaredSizeForMetering>::DECLARED_SIZE
                 );
             };
+            // This variant allows accounting for the case where an
+            // ExtensionPoint has been upgraded to some other type, causing the
+            // enclosing type to exceed its declared size: you can include an
+            // extension type as a second argument and it will be added to the
+            // declared size for the sake of retaining _some_ check here.
+            // Unfortunately this is kinda the best we can do. See the long
+            // comment above around the zero declared_size of ExtensionPoint.
+            ($t:ty, $ext:ty) => {
+                ma::assert_le!(
+                    size_of::<$t>() as u64,
+                    <$t as DeclaredSizeForMetering>::DECLARED_SIZE + (size_of::<$ext>() as u64)
+                );
+            };
         }
         // primitive types
         assert_mem_size_le_declared_size!(bool);
@@ -556,7 +658,14 @@ mod test {
         assert_mem_size_le_declared_size!(LedgerEntryExt);
         assert_mem_size_le_declared_size!(AccountEntry);
         assert_mem_size_le_declared_size!(TrustLineEntry);
+        #[cfg(feature = "next")]
+        assert_mem_size_le_declared_size!(ContractCodeCostInputs);
+        #[cfg(not(feature = "next"))]
         assert_mem_size_le_declared_size!(ContractCodeEntry);
+        #[cfg(feature = "next")]
+        assert_mem_size_le_declared_size!(ContractCodeEntry, ContractCodeEntryV1);
+        #[cfg(feature = "next")]
+        assert_mem_size_le_declared_size!(ContractCodeEntryV1);
         assert_mem_size_le_declared_size!(TtlEntry);
         assert_mem_size_le_declared_size!(LedgerKey);
         assert_mem_size_le_declared_size!(LedgerEntry);

soroban-env-host/src/host/metered_clone.rs

@@ -18,6 +18,8 @@
 
 use std::{cell::RefCell, iter::FromIterator, mem, rc::Rc};
 
+#[cfg(feature = "next")]
+use crate::xdr::{ContractCodeCostInputs, ContractCodeEntryExt, ContractCodeEntryV1};
 use crate::{
     budget::{AsBudget, DepthLimiter},
     builtin_contracts::base_types::Address,
@@ -314,6 +316,10 @@ impl MeteredClone for TimePoint {}
 impl MeteredClone for Duration {}
 impl MeteredClone for Hash {}
 impl MeteredClone for Uint256 {}
+#[cfg(feature = "next")]
+impl MeteredClone for ContractCodeCostInputs {}
+#[cfg(feature = "next")]
+impl MeteredClone for ContractCodeEntryV1 {}
 impl MeteredClone for ContractExecutable {}
 impl MeteredClone for AccountId {}
 impl MeteredClone for ScAddress {}
@@ -557,6 +563,12 @@ impl MeteredClone for ContractCodeEntry {
     const IS_SHALLOW: bool = false;
 
     fn charge_for_substructure(&self, budget: impl AsBudget) -> Result<(), HostError> {
+        #[cfg(feature = "next")]
+        // self.ext is a former ExtensionEntry; see note on ExtensionEntry in declared_size.rs
+        match &self.ext {
+            ContractCodeEntryExt::V0 => (),
+            ContractCodeEntryExt::V1(v1) => v1.charge_for_substructure(budget.clone())?,
+        }
         self.code.charge_for_substructure(budget)
     }
 }

soroban-env-host/src/vm/module_cache.rs

@@ -47,9 +47,9 @@ impl ModuleCache {
                             ContractCodeEntryExt::V0 => VersionedContractCodeCostInputs::V0 {
                                 wasm_bytes: code.len(),
                             },
-                            ContractCodeEntryExt::V1(v1) => {
-                                VersionedContractCodeCostInputs::V1(v1.cost_inputs.clone())
-                            }
+                            ContractCodeEntryExt::V1(v1) => VersionedContractCodeCostInputs::V1(
+                                v1.cost_inputs.metered_clone(host.as_budget())?,
+                            ),
                         };
                         self.parse_and_cache_module(host, hash, code, code_cost_inputs)?;
                     }