Evm automation feature implementation

[aregng]

No description provided.

[isaacdoidge]

Production-readiness review of the whole PR (reviewed at 47cefd8, the commit smr-moonshot actually pins — tree-identical to this head). Split into three parts: the core revm engine delta, the supra-extension Rust crate, and the genesis Solidity contracts. Inline comments below; process/summary points here.

Most important finding — a fund-loss bug in the genesis contracts (Critical): LibAccounting narrows uint128 cycle-fee amounts to uint64 on the refund/unlock paths. Since the token is 18-decimal, any cycle fee above ~18.4 SUPRA is silently truncated on refund and the locked-fee accounting is corrupted. This fires on ordinary governance actions (disabling automation) and stopTasks. Details inline on LibAccounting.sol. This alone blocks baking these contracts into production genesis.

Consensus-safety verdict (good news): I traced the two cross-layer risks the core-engine review raised, into smr-moonshot:

• The tx_hash field is not user-forgeable on the consensus path: EvmExecutor::convert_to_tx_env always sets it to txn.digest_as_b256() (the canonical computed digest) for every tx type, so the TxHash precompile returns a trustworthy hash. Only the RPC simulation path can see a zero/placeholder hash, which is harmless.
• For the default User execution mode, the engine delta is behaviorally equivalent to upstream revm v29 (gas, nonce, balance, refunds, EIP-3607 all gated behind ExecutionMode, default User). The one un-gated new rule is validate_caller (comment inline) — benign in practice but a genuine new consensus rule with a stringly-typed error.

Reproducible-genesis blocker (same root cause as my smr-moonshot bluealloy#2791 comment): the Solidity toolchain is unpinned — floating pragmas, no solc_version, default bytecode_hash=ipfs metadata — so independently-built validators can produce byte-different genesis bytecode and different CREATE2 addresses. Comment on foundry.toml.

Dependency hygiene: Cargo.toml [patch.crates-io] points gmp-mpfr-sys at a moving branch=master ssh ref; and smr-moonshot consumes this repo via branch = task/issue-2908 rather than a tag (flagged on the smr-moonshot PRs). Cut an immutable tag for the release.

Open review threads: most of your existing threads are answered; the still-relevant ones are the tx_hash end-user-settability question (resolved above — enforced node-side), the ByPass→Bypass rename, and the doc typos. The .gitmodules private-submodule access question also applies to the smr-moonshot build.

Verified sound: automation transaction-construction (automated_transaction/automation_record/block_metadata) is deterministic (BTreeMaps, no clock/RNG on the hot path) and panic-free on the execution path; Diamond deployment is atomic (no uninit-proxy window); AppStorage slot-0 pattern avoids facet storage collisions; the privileged Solidity surface (diamondCut, config setters, mint/burn, BlockMeta admin) is consistently owner/VM-signer gated.

[isaacdoidge]

[Critical — fund loss + accounting corruption] uint64(_refundFee) truncates a uint128 cycle fee. safeFeeRefund then uses that truncated value for both the payout (safeRefund(..., _refundableFee, ...)) and the cycleLockedFees decrement. With an 18-decimal token, uint64 max ≈ 18.4 SUPRA, so any task whose cycle fee exceeds that (a long-lived / high-gas task) is refunded only fee mod 2^64 — the owner loses the rest — and cycleLockedFees is left over-locked forever.

Scenario: governance disables automation → refundTaskFees runs for every task → a user owed 100 SUPRA receives 100e18 mod 2^64 (a tiny fraction), irrecoverably. Fix: thread cycle-fee amounts as uint128/uint256 end-to-end and drop the uint64 narrowing (same at line 403 in unlockDepositAndCycleFee, where the mismatch is even more direct — payout uses the untruncated cycleFeeRefund while the unlock uses uint64(cycleLockedFeeForTask)).

[isaacdoidge]

[Critical — paired with the line-226 finding] uint64(cycleLockedFeeForTask) truncates the uint128 locked cycle fee when unlocking on stopTasks, while the actual refund paid out (cycleFeeRefund, derived from taskFeeForResidualTime / REFUND_FACTOR) is not truncated. For a locked fee above ~18.4 SUPRA these diverge: cycleLockedFees is decremented by the wrong amount (permanent accounting corruption that withdrawFees relies on), or the operation reverts and the user cannot stop their task. Same fix — carry the fee as uint128/uint256.

[isaacdoidge]

[High — threshold bypass] removeOwners removes owners from the owners set but never decrements transaction.numConfirmations nor purges confirmations[_txIndex] for pending transactions, and executeTransaction only checks numConfirmations >= numConfirmationsRequired without re-verifying each confirmer is still an owner. So confirmations from removed owners still count toward quorum. Since this multisig is the governance root for the Diamond, ERC20Supra, and BlockMeta, a rotated-out / compromised key's stale confirmation can still help execute privileged actions. Purge pending confirmations from removed owners (and re-derive numConfirmations), or re-validate owner status at execution time.

[isaacdoidge]

[High — non-reproducible genesis] No solc_version is pinned, pragmas float (^0.8.0, ^0.8.27), and bytecode_hash/cbor_metadata are left at defaults (ipfs metadata trailer). build.rs embeds the compiled bytecode via include_bytes! and it becomes genesis state with CREATE2-derived addresses. Two validators building from source at different times can get different solc patch releases / metadata and thus different genesis bytecode and deploy addresses → divergent genesis state root. Pin an exact solc_version, set bytecode_hash = "none" and cbor_metadata = false, and freeze the pragmas. (Lib deps are already pinned via foundry.lock — good.)

[isaacdoidge]

[Medium] validate() calls validate_caller unconditionally, so the SUPRA-reserved-address rejection runs for every transaction including plain User ones (confirmed: node's transact_one routes User txns through handler.run → this validate). It's benign in practice (no real user controls a key in the 0x…535550xx band) but it is a permanent new consensus rule diverging from upstream/Ethereum, and it's ordered before validate_env, changing error precedence. Also the error is from_string(...) (there's a // TODO create InvalidTransaction variant), so eth tooling can't classify it. Please add a typed InvalidTransaction variant and document the reserved band as an intentional consensus rule.

[isaacdoidge]

Resolving your earlier question here: I traced the enforcement into smr-moonshot. EvmExecutor::convert_to_tx_env (consensus/execution/src/evm/executor.rs) sets tx_hash to txn.digest_as_b256() — the canonical computed digest — for User and every automation variant, so a user cannot inject a chosen tx_hash into consensus execution and the TxHash precompile returns a trustworthy value. Recommend a comment here stating the field is populated only by the app→execution conversion (never from inbound user bytes), and ideally making the TxEnvBuilder::tx_hash setter conversion-only, so this invariant can't regress.

[isaacdoidge]

[Medium] const CONTRACT_BYTECODES: Lazy<...> = Lazy::new(...) — a const is inlined at every use site, so each load_contract_bytecode call constructs a fresh Lazy and re-runs the bincode deserialization of the whole bytecode blob; the caching Lazy provides never actually caches. It also trips clippy::declare_interior_mutable_const. Make it a static.

[isaacdoidge]

[Medium] let _ = output.succeeded(); discards the Solidity compile status — a solc failure won't fail the build here, it only surfaces later as a missing-artifact error for whichever contract names are looked up, so a partially-failed compile can slip through and produce incomplete genesis. Propagate a hard error when !output.succeeded().

[aregng]

Internally the call checks and asserts the success, so the errors will cause early termination on this call.

[isaacdoidge]

[Low] Two things on the dep graph: (1) the [patch.crates-io] gmp-mpfr-sys = { git = ..., branch = "master" } uses a moving branch ref in a consensus-critical build — pin to a rev/tag. (2) The build/codegen deps kept "for regenerating bindings" (your reply on this thread) — please confirm they're [build-dependencies]/dev-only and not pulled into the node's runtime dependency graph. And cut a release tag for this repo so smr-moonshot can pin to it instead of branch = task/issue-2908.

[aregng]

This comment will be solved when branch is ready to be tagged for the final release.

[isaacdoidge]

[Medium — block-production robustness] blockPrologue does target.call(abi.encodePacked(selector)), forwarding all remaining gas with no per-call cap. Reverts are caught (so one bad target can't halt the block — good), but a registered target with an expensive/unbounded loop can drain the prologue's gas budget every block on every validator. Registration is onlyOwner so it's a governance-config hazard, not an open attack, but a per-call gas ceiling would harden this system hook against a mis-registered or upgraded-malicious target.

[aregng]

Will be fixed in a separate PR. I think introducing user specified gas-cap for each registered entry will be more appropriate rather to have it fixed for each call. and the contract may have total cap which should not exceed by the sum caps of registered entries.

@udityadav-supraoracles , can you please follow up on this.

[isaacdoidge]

One more item from the deeper pass on the cross-boundary types (follow-up to the main review).

[isaacdoidge]

[Medium — forward-compat wire break] AutomatedTransactionType (and AutomationTaskPredicate above, plus the automation_record.rs / block_metadata.rs types) are bcs-serialized and persisted / re-exported to the node, but they carry no version discriminant and rely on declaration order for their bcs variant indices with no explicit index pinning. Round-trip is correct today, but a future PR that inserts or reorders a variant (or reorders a struct field) silently changes the wire layout and mis-decodes already-persisted/on-chain data — e.g. adding a variant before GST makes every stored GST transaction decode as the new variant. Since smr-moonshot persists these, please add a version field (or pin variant indices) on the cross-boundary types before they carry production data. Note the parallel v1 API concern I raised on smr-moonshot bluealloy#2791 (EvmAutomationPayloadVersioned using untagged serde).

[isaacdoidge]

One more from a second core-engine pass (verified against the diff).

[isaacdoidge]

[Medium — confirm invariant] Nonce-based CREATE address collision in non-nonce-updating modes. make_create_frame derives created_address from old_nonce (inputs.caller.create(old_nonce)) but only increments the caller nonce when should_update_nonce, which is false for Automated/AutomatedGasless/System/ReadOnly. So if a transaction in one of those modes performs two nonce-based CREATE deployments from the same caller, both resolve to the same address and the second hits a CreateCollision, diverging from standard EVM semantics (where each CREATE advances the nonce). It's deterministic — the mode is in the committed CfgEnv, so it can't fork the chain — but it is a latent correctness hazard. Please confirm automated/system transactions never perform nonce-based CREATE (CREATE2 is unaffected), or derive the address from a locally-incremented counter even when the persisted nonce is frozen.