Repository navigation
VM hooks return error, version 2 - #994
Open
andrei-marinica wants to merge 24 commits into
Open
andrei-marinica wants to merge 24 commits into
andrei-marinica wants to merge 24 commits into
Conversation
Introduces vmhost.EarlyExit (and ExitUserError/ExitOutOfGas constructors), executor.VMHooksErrorHandler, and vmhooks.HandleHookError/resolveEarlyExit/ applyEarlyExit. Nothing references these yet; this is step 1 of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- eiGenModel: EIFunction gains ReturnsError. - eiGenParse: extractEIFunctionResults strips a trailing error result; tolerant of hooks that don't return one yet (parser stays permissive until every hook is migrated). - eiGenWriteCgo: goResultSignature renders the Go result list for both migrated and not-yet-migrated hooks; writeGoExports emits the HandleHookError call on the error path while leaving the C ABI (extern decls, //export signatures) untouched. - eiGenWriteGoVMHooks: VMHooks interface embeds VMHooksErrorHandler. - eiGenWriteVMHooksWrapper: WrapperVMHooks methods pass the error through and gain a HandleHookError passthrough. - Deleted WriteWasmer1Cgo and writePopulateImports/cgoImportName, dead code with no wasmer1 package left to target. Step 2 of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No hook bodies migrated yet. Diff is exactly the VMHooksErrorHandler embed in the VMHooks interface and the HandleHookError passthrough on WrapperVMHooks. All Rust outputs in mx-vm-executor-rs are byte-identical (git status --short there is empty), wasmer2ImportsCgo.go and every other generated Go file are unchanged. go build and make test-short pass. Step 3 of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Calibration run for the hook error migration: all 5 ManagedMap* hooks now return (int32, error) instead of calling FailExecution internally. No external call sites needed fixing. Rust outputs in mx-vm-executor-rs remain byte-identical. Step 4 (file 1/8) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 12 SmallInt*/Int64* hooks now return error (plus their value result where applicable), preserving the exact value returned alongside each failure. The two backwards-compatibility hooks (Int64getArgument, Int64storageStore, Int64storageLoad, Int64finish) forward to their SmallInt* counterparts, whose shapes now line up directly. Fixed one external call site in managedei_test.go's hand-written mock contract, which calls VMHooksImpl methods directly (bypassing the generated wrapper): it now applies HandleHookError itself, same as a real cgo export would, keeping errcheck clean. Step 4 (file 2/8) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 22 BigFloat* hooks now return error (plus their value result where applicable). The unexported setResultIfNotInfinity helper, called from several hook tails (Add/Sub/Mul/Div/SetBigInt/Pow), now returns error instead of calling the package-level FailExecution itself; callers propagate it with a plain `return setResultIfNotInfinity(...)`. context.pow was already error-returning and is untouched. Fixed two direct-call test sites in bigFloats_test.go (TestBigFloats_Pow and TestBigFloats_Pow_WithBarnardOpcodesFlag) that call BigFloatPow directly on VMHooksImpl, bypassing the generated wrapper; they now check the returned error like the other assertions in those tests. Step 4 (file 3/8) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 27 MBuffer* hooks now return error (plus their value result where applicable), preserving the exact value returned alongside each failure — including the several places where the sentinel value differs between error sites in the same hook (e.g. MBufferEq returns -1 on a GetBytes failure but 1 on a gas failure; MBufferStorageLoad returns 0 on a GetStorage failure but -1 on a gas-for-load failure). Deliberate "silent" bounds-exceeded returns (no failure, see the existing "does not fail execution if slice exceeds bounds" comments) keep returning their status value with a nil error. Migrated the three WithHost/WithTypedArgs helpers that call FailExecution internally: ManagedBufferCopyByteSliceWithHost, ManagedBufferSetByteSliceWithHost, ManagedBufferSetByteSliceWithTypedArgs now return (int32, error) and the hooks that front them (MBufferCopyByteSlice, MBufferSetByteSlice) simply forward the tuple. Fixed external call sites that call these methods/helpers directly, bypassing the generated wrapper: managedops_test.go (3 sites, applying FailExecution(host, err) explicitly, matching the existing idiom there) and manBuffers_test.go (4 sites, asserting the returned error is nil). Step 4 (file 4/8) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 41 crypto hooks (hashes, BLS/Ed25519/Secp256k1/Secp256r1 verify, elliptic curve ops, key generation) now return error (plus their value result where applicable), preserving every sentinel value exactly as before. All WithHost helpers that performed failure side effects (ManagedRipemd160WithHost, ManagedVerifyBLSWithHost, ManagedVerifyEd25519WithHost, ManagedVerifyCustomSecp256k1WithHost, ManagedVerifySecp256k1WithHost, ManagedEncodeSecp256k1DerSignatureWithHost, ManagedScalarBaseMultECWithHost, ManagedScalarMultECWithHost, ManagedMarshalECWithHost, ManagedMarshalCompressedECWithHost, ManagedUnmarshalECWithHost, ManagedUnmarshalCompressedECWithHost, ManagedGenerateKeyECWithHost, ManagedCreateECWithHost) and the unexported commonScalarBaseMultEC/commonScalarMultEC/commonUnmarshalEC/ commonUnmarshalCompressedEC helpers now return (int32, error) too. Already error-returning helpers (commonMarshalEC, commonMarshalCompressedEC, commonGenerateEC, useGasForCryptoVerify) are untouched. Notable preserved quirks (verified against the original control flow, not just pattern-matched): - AddEC/DoubleEC: the original called FailExecution(err1) on a failed GetEllipticCurve without returning, then kept using the (would-be nil) curve. Proved this is unreachable dead code — Get100xCurveGasCostMultiplier, checked earlier in the same hook with an unconditional early return, reads the same ecValues map entry and already guarantees the handle exists by the time GetEllipticCurve runs — so migrating this spot to a normal early `return err1` is an exact behavioural no-op. - ManagedUnmarshalECWithHost silently returns 1 (no error) when GetBytes(dataHandle) fails, unlike its sibling hooks which do fail execution on that same error - preserved as `return 1, nil`. - CreateEC vs ManagedCreateECWithHost: pre-existing asymmetry on an unrecognized curve name (CreateEC falls through silently, the WithHost variant fails execution with ErrBadBounds) is preserved. Fixed 17 direct call sites in managedei_test.go that call the *WithHost helpers directly, bypassing the generated wrapper, applying FailExecution(host, err) explicitly per the established idiom. Step 4 (file 5/8) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 41 hooks now return error (plus their value result where
applicable). Migrated 11 WithHost helpers that performed failure side
effects: ManagedGetESDTTokenDataWithHost, ManagedGetESDTTokenTypeWithHost,
ManagedAsyncCallWithHost, GetCallbackClosureWithHost,
ManagedDeleteContractWithHost, ManagedIsESDTFrozenWithHost,
ManagedIsESDTLimitedTransferWithHost, ManagedIsESDTPausedWithHost,
ManagedBufferToHexWithHost, ManagedGetCodeMetadataWithHost,
ManagedIsBuiltinFunctionWithHost, ManagedExecuteOnDestContextWithErrorReturnWithHost.
Already error-returning helpers (chargeGasForExtraLength,
ManagedGetAllTransfersCallValueTyped, ManagedGetCodeHashTyped,
setReturnDataIfExists) are untouched.
R6 case: ManagedAsyncCallWithHost's `errors.Is(err, ErrNotEnoughGas)`
tail (the census's one SetRuntimeBreakpointValue site in this file)
becomes `return vmhost.ExitOutOfGas()`, matching the plan's §6.3
AsyncCall worked example. R7 case: ManagedSignalError's tail becomes
`return vmhost.ExitUserError(...)`.
Preserved two subtle quirks verbatim rather than "fixing" them:
- ManagedGetCodeMetadataWithHost: `if err != nil || check.IfNil(contract)
{ FailExecution(host, err); return }` silently no-ops when contract is
nil but err is nil (FailExecution(host, nil) was already a documented
no-op) — migrated to `return err`, which is nil in that branch and
thus still a no-op once routed through HandleHookError's nil guard.
- ManagedUpgradeContract: a pre-existing redundant/dead duplicate error
check is preserved as-is rather than removed.
Hooks that tail-call baseOps.go helpers not yet migrated (CreateAsyncCallWithTypedArgs,
UpgradeFromSourceContractWithTypedArgs, upgradeContract, deleteContract,
ExecuteOn{Same,Dest}ContextWithTypedArgs, ExecuteReadOnlyWithTypedArguments,
TransferESDTNFTExecute*WithTypedArgs, TransferValueExecuteWithTypedArgs)
just forward the value with `, nil` — those helpers still apply their
own failure side effects internally until baseOps.go is migrated last.
Fixed external call sites that call the migrated WithHost helpers
directly, bypassing the generated wrapper: managedei_test.go (9 sites),
execution_test.go (2 sites), mock/contracts/asyncParentSC.go (1 site)
use the established `vmhooks.FailExecution(host, err)` idiom. A fourth,
mock/contracts/gasMismatchParent.go's GasMismatchAsyncCallParentMock,
specifically exercises ManagedAsyncCallWithHost's out-of-gas path, so a
plain FailExecution(host, err) would have been wrong (it doesn't know
about *vmhost.EarlyExit and would unconditionally burn gas instead of
setting the breakpoint) — it routes through
vmhooks.NewVMHooksImpl(host).HandleHookError(err) instead, replicating
exactly what the generated wrapper does. Verified via
TestGasUsed_AsyncCallManaged_Mocks.
Step 4 (file 6/8) of PLAN-go-hooks-error-migration.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 42 BigInt* hooks now return error (plus their value result where applicable), preserving the exact value returned alongside each failure. BigIntToStringWithHost, the one WithHost-style helper in this file, now returns error too. No fall-through-after-FailExecution, R5, R6, R7, or direct runtime.FailExecution sites in this file (confirmed by grep against the original). Hooks that tail-call not-yet-migrated baseOps.go helpers still relying on the helper's own internal FailExecution (failIfMoreThanOneESDTTransfer, getESDTTransferFromInputFailIfWrongIndex) forward with `, nil`, unchanged behavior until baseOps.go is migrated. Fixed two direct call sites in managedei_test.go that bypass the generated wrapper: Test_BigIntToString (calls BigIntToStringWithHost directly) applies FailExecution(host, err); bigIntToInt64MockContract (calls BigIntGetSignedArgument/BigIntGetInt64 directly) routes through HandleHookError, consistent with how SmallIntFinishSigned was already handled there from an earlier migration step. Step 4 (file 7/8) of PLAN-go-hooks-error-migration.md. Only baseOps.go remains. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 87 hooks in baseOps.go now return error (plus their value result where applicable), completing the per-file migration started in PLAN-go-hooks-error-migration.md step 4. Also removes the duplicate @AutoGenerate(VMHooks) annotation on UpgradeContract (plan step 6): the generator now reports 277 hooks, not 278. Migrated ~30 WithHost/WithTypedArgs/internal helpers that performed failure side effects, including all 5 remaining R6 (SetRuntimeBreakpointValue) sites and the last R7 (SignalUserError) site: - SignalError's tail becomes `return vmhost.ExitUserError(...)`, verbatim per the plan's §6.2 worked example. - AsyncCall's tail, and the two gas-limit prechecks plus the two RegisterLegacyAsyncCall-error checks inside upgradeContract and deleteContract, all become `return vmhost.ExitOutOfGas()` (or `return ..., vmhost.ExitOutOfGas()`), verbatim per §6.3. - getESDTTransferFromInputFailIfWrongIndex, failIfMoreThanOneESDTTransfer, TransferValueExecuteWithHost/WithTypedArgs, TransferESDTNFTExecute* (With/WithTypedArgs/WithTypedArgsWithFailure/ByUserWithTypedArgs), CreateAsyncCallWithHost/WithTypedArgs, upgradeContract, UpgradeFromSourceContractWithTypedArgs, deleteContract, StorageStoreWithHost/WithTypedArgs, StorageLoadFromAddressWithHost, StorageLoadWithHost, SetStorageLockWithHost/WithTypedArgs, ExecuteOnSameContextWithHost/WithTypedArgs, ExecuteOnDestContextWithHost/WithTypedArgs, ExecuteReadOnlyWithHost/WithTypedArguments, createContractWithHost, GetReturnDataWithHostAndTypedArgs, CleanReturnDataWithHost, DeleteFromReturnDataWithHost all now return (value, error) / error instead of calling FailExecution themselves. Already error-returning helpers (StorageLoadFromAddressWithTypedArgs, getESDTDataFromBlockchainHook, createContract, getArgumentsFromMemory, the extractIndirectContractCallArguments* helpers) are untouched. Fixed every forwarding call site in the already-migrated managedei.go and bigIntOps.go that used to do `<value>, nil` (or a bare `return nil`) because the baseOps.go helper it called wasn't migrated yet and was already applying its own failure side effects internally — these now properly propagate the helper's real error instead of silently discarding it: ManagedCreateAsyncCall, ManagedUpgradeFromSourceContract, ManagedUpgradeContract, ManagedDeleteContractWithHost, ManagedExecuteReadOnly, ManagedExecuteOnSameContext, ManagedExecuteOnDestContext(WithErrorReturnWithHost), ManagedMultiTransferESDTNFTExecute(WithReturn)(ByUser), ManagedTransferValueExecute, BigIntGetESDTCallValue, BigIntGetESDTCallValueByIndex. Notable preserved quirks, each verified against the original control flow rather than pattern-matched: - Several void-hook tails (GetBlockRandomSeed, GetStateRootHash, GetPrevBlockRandomSeed, GetOriginalTxHash, GetCurrentTxHash, GetPrevTxHash) called FailExecution as their last statement with no explicit trailing return, relying on Go's implicit end-of-function return — migrated to an explicit `return err` / trailing `return nil`. - TransferESDTNFTExecuteWithTypedArgsWithFailure and TransferESDTNFTExecuteByUserWithTypedArgs each had 2 sites where the original called FailExecution twice in sequence (once for a RevertToSnapshot error, unconditionally again for the main execute error) — only the second call's ReturnCode/ReturnMessage survived in the original (SetReturnCode/SetReturnMessage get overwritten), but the error trace had two AddError entries. The migrated code returns a single error (the main one takes priority, matching the final observable ReturnCode/ReturnMessage exactly), which means the hypothetical case of both errors firing simultaneously would produce one trace entry instead of two. Confirmed BlockchainContextMock and every BlockchainHookStub wiring in the test suite always returns a nil error from RevertToSnapshot, so this is unreachable in every existing test, and no test asserts trace-entry count. - Several `...ByIndex` hooks (GetESDTTokenNonceByIndex and siblings) preserve distinct sentinel values per failure site in the same hook (e.g. -1 on a gas failure, 0 on a bad-index failure) rather than normalizing to one value, matching the original exactly. Fixed external call sites bypassing the generated wrapper across mock/contracts/{common,asyncParentSC,backTransferSC,directSimpleSC, esdtSimpleSC,storageSimpleSC,transferAndExecuteSC,deployerSimpleSC}.go, testcommon/testCallGraphUtils.go, and vmhost/hosttest/{execution_test, managedei_test}.go. One of these, Test_ManagedCallCheck, calls DeleteContract directly and needed HandleHookError rather than plain FailExecution since DeleteContract's callee (deleteContract) has an EarlyExit-producing path. Step 4 (file 8/8, final file) and step 6 of PLAN-go-hooks-error-migration.md. Verified: zero diff in mx-vm-executor-rs, clean go build/vet, zero new errcheck findings, make test-short and the full VMEXECUTOR=wasmer2 go test ./... both pass, and the §8 greps (SetRuntimeBreakpointValue/SignalUserError/ FailExecution(/resolveEarlyExit/applyEarlyExit) all come back exactly as the plan predicts now that all 8 hook files are migrated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the final enforcement from §5.2: a hook without a trailing error result is now a generator error. All 277 hooks already satisfy this (verified: regenerating still reports 277 methods with zero diff in either this repo's generated files or mx-vm-executor-rs), so this is purely a guard against a future hook reintroducing in-body failure handling. Sanity-checked the validation actually fires by temporarily reverting one hook's signature and confirming the generator rejects it. Step 8 of PLAN-go-hooks-error-migration.md — the migration invariant is now locked in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed no call sites remain anywhere in the repo (every former caller was migrated to return error and propagate through the generated wrapper's HandleHookError instead). It was never part of the executor.VMHooks interface, so removing it is safe. The package-level FailExecution(host, err) stays, unchanged — it's still used by mocks, tests, and non-hook callers per §6.4/§6.5. Step 9 (optional cleanup) of PLAN-go-hooks-error-migration.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rror handling in tests
andrei-marinica
marked this pull request as ready for review
November 5, 2026 13:17
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/new-executor #994 +/- ##
=====================================================
- Coverage 28.07% 27.06% -1.01%
=====================================================
Files 64 69 +5
Lines 17976 18708 +732
=====================================================
+ Hits 5046 5063 +17
- Misses 12601 13313 +712
- Partials 329 332 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VM hooks return errors instead of failing execution themselves
Refactor only. Contract-visible behaviour is meant to stay the same, and the Rust executor binaries are unchanged.
Why
Up to now, a VM hook that failed applied the failure itself, before returning. It called
FailExecution,SignalUserErroror set an out-of-gas breakpoint, and then returned a sentinel value. This is the only option over the C API: an imported function can return a value but cannot report an error.The new executor interfaces on the Rust side (
new_traits/vm_hooks_new.rs,VMHooksLegacyAdapter,VMHooksEarlyExit) let a hook report an error to its caller. This PR moves the Go hooks to that model, so they can later be used by a backend that propagates errors. The existing wasmer2 backend keeps working through an adapter.What changes
Every VM hook returns a trailing
error(vmhost/vmhooks/*.go)FailExecution(err); return Xbecomesreturn X, err. The sentinel value is kept next to the error, so a hook returns the same value it returned before....WithHost/...WithTypedArgshelpers return errors in the same way.SignalError/ManagedSignalErrorreturnvmhost.ExitUserError(msg). The legacy async/upgrade/delete paths that only set the out-of-gas breakpoint returnvmhost.ExitOutOfGas().Early exit model
executor.EarlyExit(executor/earlyExit.go): an error that says how to terminate (Code,Message,ConsumeAllGas,Cause). It is the Go counterpart of the RustVMHooksEarlyExit.executor.ExitExecutionFailedis the default for a plain error: burn the remaining gas, then fail execution.vmhost.ExitUserError/vmhost.ExitOutOfGas(vmhost/earlyExit.go) are the two exits that do not follow the default.VMHooksImpl.HandleHookError(vmhost/vmhooks/earlyExit.go) resolves an error into anEarlyExitand applies exactly one of the three termination routines that already existed.Interfaces (all generated)
executor.VMHooks(vmHooksNew.go): the error-returning interface.executor.VMHooksLegacy(vmHooks.go): the previous value-only interface, renamed. The file names match the Rust side.executor.VMHooksLegacyAdapter: turnsVMHooksintoVMHooksLegacy, passing errors to anexecutor.VMHooksErrorHandler.WrapperVMHookswraps the new interface and forwardsHandleHookError.Backends
wasmer2builds the legacy adapter in its factory and keeps working withVMHooksLegacyinternally. The cgo bindings are unchanged.ErrVMHooksCannotHandleErrorsif they cannot.Generator
errorresult and requires it on every hook.vmHooks.go,vmHooksNew.go,vmHooksLegacyAdapter.goand the wrapper.Tests and mocks
vmhooks.NewVMHooksLegacy(host). This gives them the same behaviour as a contract running under wasmer2.How it was verified
go test ./executor/... ./vmhost/... ./wasmer2/... ./mock/... ./integrationTests/...passes.vm_hooks.rs,new_traits/vm_hooks_new.rs,new_traits/vm_hooks_legacy_adapter.rs,capi_vm_hook_pointers.rs,wasmer_imports.rs) are identical tomx-vm-executor-rsrc/after-supernova.FailExecution(e); return vmaps toreturn v, e, with the same sentinel and the same termination routine.Notes for reviewers
vmhost/vmhooks/*.go, plus the generatedexecutor/vmHooksLegacyAdapter.goandexecutor/wrapper/wrapperVMHooks.go). The design is inexecutor/earlyExit.go,vmhost/earlyExit.go,vmhost/vmhooks/earlyExit.goand the two factories.