CAP-85: Externally managed contract executables (productionization) - #1704
CAP-85: Externally managed contract executables (productionization)#1704sisuresh wants to merge 5 commits into
Conversation
This implements all the host-side CAP features: a new object type and the respective constructor function for the executable tags, and the host functions for creating/updating the contract executables to external references. (cherry picked from commit 2a82e53)
… + observation regen - Wire per-CAP leaf feature cap_0085_executable_ref (common/host/guest), aggregated by next; gate CAP code + declared_size on the leaf. - Regenerate observations under testutils (benign trace-visibility shift from get_current_contract_id_opt_internal routing through bytes_new_from_slice). WIP checkpoint; metering test value-splits under next still to follow.
Completes the `cap_0085_executable_ref` leaf-feature productionization so every budget-sensitive test passes with the feature both on and off. - cfg-split the expected instruction/resource values in the recording-mode e2e tests, invocation_metering, budget_metering, auth, lifecycle and stellar_asset_contract on `cap_0085_executable_ref`. The enlarged `ScVal` shifts serialization/hashing costs by a few dozen cpu insns / 128 mem bytes per affected invocation. - hostile::excessive_logging: externalizing a full-page diagnostic log now consumes ~1.05MB of shadow memory (vs ~0.95MB), so bump block 3's shadow mem limit 1M -> 2M under the feature and cfg-split the expected budget dump. The production budget is unaffected. - Reorder the cap_85_executable_reference e2e tests' expected ledger_changes to the host's intentional read-only-first ordering (e2e_invoke.rs). Verified: full testutils suite green (778 passed, observations regenerated in the prior commit); full next suite green (801 passed, incl. the cap_85_executable_reference module); cap_0085_executable_ref leaf green for the affected modules. Clippy shows no new findings (the pre-existing lints are identical with and without these changes).
|
Downstream (this release run): rs-soroban-sdk SPIKE pins this rev — stellar/rs-soroban-sdk#1929 |
…host Greens build-and-test (clippy --all-features) and fmt on the p28-cap-0085 branch. The clippy lint is pre-existing VM error-handling code flagged by newer clippy; the rustfmt drift is in CAP-85-touched declared_size/data_helper.
dmkozh
left a comment
There was a problem hiding this comment.
I've left a few comments highlighting the issues I see with feature gating in host. I don't think it's a good idea to maintain a whole lot of semi-dead code and test cartesian products of features in case if we have more than 1 CAP to maintain. I would rather prefer to use good old version control for this and roll back the code on the off chance it's not longer necessary. Surely, one can argue that AI can do the dirty work of adding the gates, but as this PR proves, it's not really perfect, requires human intervention anyways, and makes subsequent work more complex and slow than necessary. Even if the code diverges slightly (e.g. we make a followup change), making an AI-assisted rollback would not be too hard, and it would be much more focused (like if the object + host fns are removed, most of the remaining CAP-specific code will just become unused, which is detectable by compiler)
| // `HostObject::ExecutableTag` is unconditional; without the | ||
| // gated `ScVal::ExecutableTag` there is nothing to convert | ||
| // into (such an object can't be created in this build). | ||
| #[cfg(not(feature = "cap_0085_executable_ref"))] |
There was a problem hiding this comment.
Code like this is precisely the reason for why I don't think we should use feature gates in host. I'm not sure it's a safe to just introduce an unusable host object, and I don't like maintaining the unused code. Not to mention that this messes up with all the observations.
There was a problem hiding this comment.
I'm not sure why it decided to unconditionally include ExecutableTag. That should be gated as well, but I see your point.
| @@ -33,6 +33,14 @@ use crate::{ | |||
| Host, HostError, LedgerInfo, | |||
| }; | |||
| use crate::{ErrorHandler, ModuleCache}; | |||
| // CAP-0085 XDR types used only by the `next`-gated `cap_85_executable_reference` | |||
| // test module (via `use super::*`). | |||
| #[cfg(feature = "next")] | |||
There was a problem hiding this comment.
We shouldn't use next gates in code - if the intention is to gate it (which I don't think we should do, but still), then these should be gated by the XDR feature.
| @@ -882,6 +890,9 @@ fn test_wasm_upload_success_in_recording_mode() { | |||
| }] | |||
| ); | |||
| assert!(res.auth.is_empty()); | |||
| #[cfg(feature = "cap_0085_executable_ref")] | |||
There was a problem hiding this comment.
This pattern is going to become unwieldy if we make just one more change that happens to change the budgets slightly, as we'll need to maintain a cartesian product of all the feature combinations here.
| @@ -1833,6 +1831,90 @@ mod cap_58_constructor { | |||
| params, | |||
| ) | |||
| .unwrap(); | |||
| #[cfg(feature = "cap_0085_executable_ref")] | |||
There was a problem hiding this comment.
I've left a comment re observations in e2e_tests, but this is especially awful to maintain
| @@ -677,6 +692,9 @@ mod tests { | |||
| .filter(|t| { | |||
| // bad tags can't be converted to ScVal | |||
| !matches!(t, Tag::Bad) | |||
| // ExecutableTag <-> ScVal conversion is gated behind CAP-0085. | |||
| && (cfg!(feature = "cap_0085_executable_ref") | |||
There was a problem hiding this comment.
This is quite nasty as well
rs-soroban-env stellar#1698 ("lazily decode ledger entries passed in e2e flow") changed the public e2e_invoke::invoke_host_function and vm::wasm_module_memory_cost signatures. stellar-core's per-protocol host glue mirrors the p27 host and targets the pre-stellar#1698 e2e interface; adopting stellar#1698 needs separate C++ bridge changes out of scope for this WIP CAP-85 host. This resets the tree to the pre-stellar#1698 base (883fd56) plus the CAP-85 commits so core's mirror glue compiles. Added on top of the existing branch (non-destructive) rather than force-pushing a rebase.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
What
Productionizes CAP-85 (externally managed contract executables) behind the
cap_0085_executable_refleaf feature, whichnextaggregates.Building on the CAP-85 implementation, this branch:
cap_0085_executable_refleaf feature (nextaggregates it;
common/nextadds nothing behavioral beyond the leaf).testutils.feature both on and off — the enlarged
ScValshifts metering bya few dozen cpu insns / 128 mem bytes per affected invocation
(
invocation_metering,budget_metering, recording-modee2e_tests,auth,lifecycle,stellar_asset_contract).hostile::excessive_logging:externalizing a full-page diagnostic log now consumes ~1.05MB of shadow
memory (vs ~0.95MB), so block 3's shadow mem limit goes 1M → 2M under the
feature. The production budget is unaffected.
cap_85_executable_referencee2e tests' expectedledger_changesto the host's intentional read-only-first ordering.Related / depends on
Verification
--features testutils(feature off, full suite): 778 passed, 0 failed (observations regenerated)--features next(feature on, full suite): 801 passed, 0 failed (incl. thecap_85_executable_referencemodule)--features cap_0085_executable_ref(leaf): affected modules greenNotes
clippymay surface pre-existing lints from a newer clippy (1.94.x)across untouched test files. These are identical with and without this
branch (verified by diffing the clippy output against the clean tree)
and are unrelated to CAP-85.