From 3b6aea1726b20252e272f4d249ae7fc4a1b34bbf Mon Sep 17 00:00:00 2001 From: kp2pml30 Date: Thu, 20 Aug 2026 17:00:04 +0900 Subject: [PATCH] =?UTF-8?q?fix(manager):=20guard=20fatal=20VM=20result=20p?= =?UTF-8?q?ublication=20=F0=9F=94=92=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/modules-interfaces/src/domain.rs | 2 + ...pre-validating untrusted decoded inputs.md | 11 ++- docs/adr/014. manager host socket protocol.md | 10 +- .../src/impl-spec/02-vm/03-consensus.rst | 5 +- .../src/impl-spec/appendix/manager-socket.rst | 18 ++++ docs/website/src/spec/03-vm/05-result.rst | 14 ++- executors/v0.3.x | 2 +- implementation/src/manager/run.rs | 99 ++++++++++++++++--- implementation/src/manager/run_test.rs | 68 +++++++++++++ tests/runner/origin/base_host.py | 6 +- tests/system/manager-socket/test.py | 17 ++++ 11 files changed, 221 insertions(+), 31 deletions(-) diff --git a/crates/modules-interfaces/src/domain.rs b/crates/modules-interfaces/src/domain.rs index a083b58c..ca16e8fd 100644 --- a/crates/modules-interfaces/src/domain.rs +++ b/crates/modules-interfaces/src/domain.rs @@ -397,6 +397,8 @@ pub struct ReportedResult { /// what a caller in another executor folds. pub small_hash: bytes::Bytes, + /// `FatalVmError` is legal only while transporting a nested result; a + /// top-level report must publish the same payload as `VmError` pub kind: ResultCode, pub data: genlayer_calldata::unparsed::Maybe, pub backtrace: Option, diff --git a/docs/adr/013. pre-validating untrusted decoded inputs.md b/docs/adr/013. pre-validating untrusted decoded inputs.md index 1317889b..af14d049 100644 --- a/docs/adr/013. pre-validating untrusted decoded inputs.md +++ b/docs/adr/013. pre-validating untrusted decoded inputs.md @@ -99,7 +99,7 @@ hiding a leader whose execution diverged into extra blocks. Now: if the leader supplied more results than blocks run, the run's result is replaced by `leader_fault nondet_output extra `, `` = first 6 chars of `gvm32(sha3_256(b))` where `b` is the **full `[result_code][data]` buffer** the -run would otherwise have returned (`RunOk::as_bytes()`) — fingerprinting the +run would otherwise have returned — fingerprinting the discarded result rather than losing it. This is **not** a non-deterministic disagreement (it's a property of the counts, not a re-executed block). No fix-point escape is needed: a `leader_fault nondet_output` code is never @@ -114,13 +114,16 @@ Before the computed result is pushed to the host (and hashed): - A `VMError`'s fused ` # ` suffix is **stripped** — the nondet channel is detail-free (diagnostic, no compat promise, and a free-form byte channel into the hash otherwise). The local `cause` is kept for logs. -- **Nothing else.** The leader must *not* run Rule 1 on its own output: +- A fatal VM error is not encoded into `nondet_results`; it propagates through + the caller and is downgraded to `VMError` only at the topmost publication + boundary +- **Nothing else is rewritten.** The leader must *not* run Rule 1 on its own output: self-filtering can only rewrite an honest result into a derived-namespace code that validators replace again — a guaranteed honest-vs-honest hash split. Rule 1 is for hostile input, Rule 1b for trusted output. -Stripping suffices because the rest is valid by construction: codes come from -the generated constructors and canonical `exit_code ` (a generated test +The remaining encodable results are valid by construction: codes come from the +generated constructors and canonical `exit_code ` (a generated test asserts every constructor passes the validity check). The one arbitrary-string site, the `vmError "…"` fee builtin, is unreachable from a nondet child (every bucket that raises it needs a permission the child lacks); the child also can't diff --git a/docs/adr/014. manager host socket protocol.md b/docs/adr/014. manager host socket protocol.md index c7036c2c..804b61af 100644 --- a/docs/adr/014. manager host socket protocol.md +++ b/docs/adr/014. manager host socket protocol.md @@ -174,9 +174,13 @@ flushes its buffered host writers before exiting, which the ACK round-trip used to force implicitly). The method is deleted from the shared `host-fns` enum and from both executor lines. -`consume_result` is untouched: it routes to host 1 and is how the manager -obtains a trusted copy of the result without round-tripping it through the -node. Orthogonal to this change. +`consume_result` still routes to host 1 and is how the manager obtains the +result without round-tripping it through the node. For a top-level run, the +manager validates the outer framing and decoded `ReportedResult` before +retaining it. Malformed reports are refused; a fatal result triggers a debug +assertion and is logged and downgraded to `VMError` in release builds. Embedded +`nondet_results` remain opaque because their encoding belongs to the executor +line ### HTTP surface diff --git a/docs/website/src/impl-spec/02-vm/03-consensus.rst b/docs/website/src/impl-spec/02-vm/03-consensus.rst index b6cf04b6..a07226ce 100644 --- a/docs/website/src/impl-spec/02-vm/03-consensus.rst +++ b/docs/website/src/impl-spec/02-vm/03-consensus.rst @@ -45,8 +45,9 @@ followed by an encoded payload: - ``VmError`` — UTF-8 error code from :doc:`/spec/appendix/constants` ``vm_error``. -The validator reconstructs an ``rt::vm::RunOk`` from these bytes -(``genlayer_sdk.rs:1502``) and the contract observes the same value as the leader did. +The validator reconstructs a catchable contract outcome from these bytes and +the contract observes the same value as the leader did. Any other result code, +including ``FatalVmError``, is treated as malformed leader input Validator Comparison -------------------- diff --git a/docs/website/src/impl-spec/appendix/manager-socket.rst b/docs/website/src/impl-spec/appendix/manager-socket.rst index 12d49409..5f22661a 100644 --- a/docs/website/src/impl-spec/appendix/manager-socket.rst +++ b/docs/website/src/impl-spec/appendix/manager-socket.rst @@ -255,6 +255,24 @@ started with one, ``host_genvm_id``. Variants: "artifact_sizes": { "stdout": u64, "stderr": u64, "genvm_log": u64 } } } +For a top-level run, ``consumed_result`` is one outer ``ResultCode`` byte +followed by a calldata-encoded ``ReportedResult`` map. Before retaining it, the +manager checks that: + +#. The outer byte is a known result code and agrees with the map's ``kind`` +#. The map decodes completely +#. ``execution_hash`` and ``small_hash`` are each 32 bytes unless the result is + ``InternalError`` + +An invalid report is refused without an acknowledgement and is not published +as ``consumed_result``. ``FatalVmError`` is also illegal at this boundary: a +debug manager asserts, while a release manager logs the executor violation and +rewrites both result-code locations to ``VmError`` before publication. Clients +therefore never receive a top-level ``FatalVmError`` + +The manager does not decode entries of the reported ``nondet_results`` vector. +Those bytes remain opaque, executor-line-specific consensus proposals + Lifecycle guarantees: - Exactly one terminal event (``failed_to_start`` or ``finished``) per run, diff --git a/docs/website/src/spec/03-vm/05-result.rst b/docs/website/src/spec/03-vm/05-result.rst index 572d30ac..50cda1d1 100644 --- a/docs/website/src/spec/03-vm/05-result.rst +++ b/docs/website/src/spec/03-vm/05-result.rst @@ -31,6 +31,13 @@ set. A non-fatal error of a :term:`sub-VM` is returned to its caller as terminates with the same VM error, and propagation continues until the topmost VM boundary +Nested transport encodes a fatal VM error with result code ``4``. At the +topmost publication boundary, the executor MUST downgrade it to an ordinary +:ref:`gvm-def-vm-error` with the same payload before producing the reported +result. Result code ``4`` is therefore forbidden in a top-level reported +result. Both its :ref:`gvm-def-execution-hash` and its +:ref:`gvm-def-subvm-hash` use ``VMError`` as the result kind + .. _gvm-def-vm-error-code: VM Error Code Format @@ -105,7 +112,9 @@ Non-Deterministic Block Result Encoding - :ref:`gvm-def-vm-error`\: utf-8 string These three are the only codes a leader-proposed non-deterministic block result -may carry; validators treat every other byte as a malformed leader result +may carry; validators treat every other byte as a malformed leader result. A +fatal VM error computed by a leader's non-deterministic child propagates to its +caller and MUST NOT be encoded into ``nondet_results`` Contract Result Encoding ------------------------ @@ -176,6 +185,9 @@ with the following keys (in this order): Two runs that agree on the deterministic result produce the same execution hash, so consensus can compare a single 32-byte value instead of the full result. +A fatal VM error is committed with ``VMError`` as ``kind``. Fatality controls +propagation and is not a distinct consensus-visible outcome + ``emissions`` covers the whole content of every emitted message and event, not just its metered cost: two emissions can carry different calldata or different event topics for the same fee, so a fee-only commitment would let nodes agree on diff --git a/executors/v0.3.x b/executors/v0.3.x index a54b15b7..55b13368 160000 --- a/executors/v0.3.x +++ b/executors/v0.3.x @@ -1 +1 @@ -Subproject commit a54b15b7f3d4d81e362b80e6d2a678f89ae52b37 +Subproject commit 55b133688edd8950b8873aad9ce5c2f1040a8c98 diff --git a/implementation/src/manager/run.rs b/implementation/src/manager/run.rs index beea74b0..90cd7399 100644 --- a/implementation/src/manager/run.rs +++ b/implementation/src/manager/run.rs @@ -1387,29 +1387,84 @@ fn nested_internal_error() -> genvm_modules_interfaces::NestedRunReply { } } -fn nested_reply_from_consumed_result( +fn result_code_from_byte( + value: u8, + source: &str, +) -> anyhow::Result { + match value { + 0 => Ok(genvm_modules_interfaces::ResultCode::Return), + 1 => Ok(genvm_modules_interfaces::ResultCode::UserError), + 2 => Ok(genvm_modules_interfaces::ResultCode::VmError), + 3 => Ok(genvm_modules_interfaces::ResultCode::InternalError), + 4 => Ok(genvm_modules_interfaces::ResultCode::FatalVmError), + value => anyhow::bail!("{source} returned unknown result code {value}"), + } +} + +fn decode_reported_result( data: &[u8], -) -> anyhow::Result { + source: &str, +) -> anyhow::Result<( + genvm_modules_interfaces::ResultCode, + genvm_modules_interfaces::ReportedResult, +)> { let (&kind, encoded) = data .split_first() - .ok_or_else(|| anyhow::anyhow!("nested executor returned an empty result"))?; - let kind = match kind { - 0 => genvm_modules_interfaces::ResultCode::Return, - 1 => genvm_modules_interfaces::ResultCode::UserError, - 2 => genvm_modules_interfaces::ResultCode::VmError, - 3 => genvm_modules_interfaces::ResultCode::InternalError, - 4 => genvm_modules_interfaces::ResultCode::FatalVmError, - value => anyhow::bail!("nested executor returned unknown result code {value}"), - }; + .ok_or_else(|| anyhow::anyhow!("{source} returned an empty result"))?; + let kind = result_code_from_byte(kind, source)?; let reported: genvm_modules_interfaces::ReportedResult = calldata::decode_obj(encoded)?; - // The code is stated twice: once as the framing byte, once inside the - // reported map that the execution hash commits to. A disagreement means the - // callee is not the implementation we think it is. + // The framing byte and reported map must name the same committed result anyhow::ensure!( kind == reported.kind, - "nested executor result code {kind:?} disagrees with the reported {:?}", + "{source} result code {kind:?} disagrees with the reported {:?}", reported.kind ); + + Ok((kind, reported)) +} + +fn downgrade_fatal_reported_result( + mut reported: genvm_modules_interfaces::ReportedResult, +) -> Vec { + debug_assert_eq!( + reported.kind, + genvm_modules_interfaces::ResultCode::FatalVmError + ); + reported.kind = genvm_modules_interfaces::ResultCode::VmError; + let mut normalized = vec![genvm_modules_interfaces::ResultCode::VmError as u8]; + normalized.extend(calldata::encode_obj(&reported)); + normalized +} + +fn guard_top_level_consumed_result(data: Vec, genvm_id: GenVMId) -> anyhow::Result> { + let (kind, reported) = decode_reported_result(&data, "top-level executor")?; + if kind != genvm_modules_interfaces::ResultCode::InternalError { + anyhow::ensure!( + reported.execution_hash.len() == 32, + "top-level executor returned an invalid execution hash length" + ); + anyhow::ensure!( + reported.small_hash.len() == 32, + "top-level executor returned an invalid small hash length" + ); + } + if kind != genvm_modules_interfaces::ResultCode::FatalVmError { + return Ok(data); + } + + debug_assert_ne!( + kind, + genvm_modules_interfaces::ResultCode::FatalVmError, + "top-level executor returned a fatal VM error after its publication boundary" + ); + log_error_into!(&LoggerWithId, genvm_id:id = genvm_id.0; "top-level executor returned a fatal VM error; downgrading it to vm_error"); + Ok(downgrade_fatal_reported_result(reported)) +} + +fn nested_reply_from_consumed_result( + data: &[u8], +) -> anyhow::Result { + let (kind, reported) = decode_reported_result(data, "nested executor")?; if kind != genvm_modules_interfaces::ResultCode::InternalError { anyhow::ensure!( reported.small_hash.len() == 32, @@ -1507,6 +1562,7 @@ fn read_manager_host_stream( parent_req: Arc, consumed_result: sync::DArc>>, genvm_id: GenVMId, + is_top_level: bool, stream_state: Arc, ) -> std::pin::Pin + Send>> { Box::pin(async move { @@ -1548,6 +1604,17 @@ fn read_manager_host_stream( return; } }; + let data = if is_top_level { + match guard_top_level_consumed_result(data, genvm_id) { + Ok(data) => data, + Err(e) => { + log_error_into!(&LoggerWithId, genvm_id:id = genvm_id.0, error:ah = &e; "refusing invalid top-level consume_result"); + return; + } + } + } else { + data + }; log_debug_into!(&LoggerWithId, genvm_id:id = genvm_id.0, len = data.len(); "manager received consume_result"); let _ = consumed_result.set(data); @@ -2330,7 +2397,6 @@ async fn run_genvm_process( _ => req.selector.clone(), }; let version = resolve_selector(&full_ctx.ver_ctx, &selector, req.timestamp, genvm_id).await?; - let ctx = full_ctx.clone().into_gep(|x| &x.run_ctx); // Capture controls how logs and stdout/stderr are kept: disabled (forwarded @@ -2443,6 +2509,7 @@ async fn run_genvm_process( Arc::new(req.clone()), consumed_result, genvm_id, + is_top_level, manager_stream_state.clone(), )); diff --git a/implementation/src/manager/run_test.rs b/implementation/src/manager/run_test.rs index 1c8a2be8..098d36b9 100644 --- a/implementation/src/manager/run_test.rs +++ b/implementation/src/manager/run_test.rs @@ -567,6 +567,74 @@ fn encoded_nested_result(reported: &genvm_modules_interfaces::ReportedResult) -> encoded } +#[test] +fn top_level_guard_accepts_a_valid_report() { + let encoded = encoded_nested_result(&clean_reported()); + + assert_eq!( + guard_top_level_consumed_result(encoded.clone(), GenVMId(1)).unwrap(), + encoded + ); +} + +#[test] +fn top_level_guard_rejects_disagreeing_result_codes() { + let mut encoded = encoded_nested_result(&clean_reported()); + encoded[0] = genvm_modules_interfaces::ResultCode::VmError as u8; + + assert!(guard_top_level_consumed_result(encoded, GenVMId(1)).is_err()); +} + +#[test] +fn top_level_guard_rejects_invalid_framing() { + for encoded in [Vec::new(), vec![5], vec![0]] { + assert!(guard_top_level_consumed_result(encoded, GenVMId(1)).is_err()); + } +} + +#[test] +fn top_level_guard_rejects_invalid_hash_lengths() { + let mut reported = clean_reported(); + reported.execution_hash = bytes::Bytes::new(); + + assert!(guard_top_level_consumed_result(encoded_nested_result(&reported), GenVMId(1)).is_err()); +} + +#[cfg(debug_assertions)] +#[test] +#[should_panic(expected = "fatal VM error after its publication boundary")] +fn top_level_guard_asserts_on_fatal_in_debug_builds() { + let mut reported = clean_reported(); + reported.kind = genvm_modules_interfaces::ResultCode::FatalVmError; + + let _ = guard_top_level_consumed_result(encoded_nested_result(&reported), GenVMId(1)); +} + +#[cfg(not(debug_assertions))] +#[test] +fn top_level_guard_downgrades_fatal_in_release_builds() { + let mut reported = clean_reported(); + reported.kind = genvm_modules_interfaces::ResultCode::FatalVmError; + + let encoded = + guard_top_level_consumed_result(encoded_nested_result(&reported), GenVMId(1)).unwrap(); + let (kind, reported) = decode_reported_result(&encoded, "test").unwrap(); + + assert_eq!(kind, genvm_modules_interfaces::ResultCode::VmError); + assert_eq!(reported.kind, genvm_modules_interfaces::ResultCode::VmError); +} + +#[test] +fn fatal_downgrade_updates_both_result_codes() { + let mut reported = clean_reported(); + reported.kind = genvm_modules_interfaces::ResultCode::FatalVmError; + let encoded = downgrade_fatal_reported_result(reported); + let (kind, reported) = decode_reported_result(&encoded, "test").unwrap(); + + assert_eq!(kind, genvm_modules_interfaces::ResultCode::VmError); + assert_eq!(reported.kind, genvm_modules_interfaces::ResultCode::VmError); +} + fn some_storage_delta() -> genvm_modules_interfaces::StorageDelta { genvm_modules_interfaces::StorageDelta::new([0; 36], vec![1]) } diff --git a/tests/runner/origin/base_host.py b/tests/runner/origin/base_host.py index f35cf414..23debcd5 100644 --- a/tests/runner/origin/base_host.py +++ b/tests/runner/origin/base_host.py @@ -525,6 +525,8 @@ def decode(cls, raw: typing.Any) -> 'ConsumedResult': empty = not as_bytes if not empty: result_kind = host_fns.ResultCode(as_bytes[0]) + if result_kind == host_fns.ResultCode.FATAL_VM_ERROR: + raise ValueError('fatal_vm_error crossed the top-level result boundary') decoded = gvm_calldata.decode(as_bytes[1:]) except Exception as exc: # Unreadable bytes are a protocol violation rather than a result, so @@ -540,10 +542,6 @@ def decode(cls, raw: typing.Any) -> 'ConsumedResult': return cls.internal_error('empty_result') if not isinstance(decoded, dict): return cls.internal_error('result is not a mapping') - # The executor reports fatality; degrading it to an ordinary VM error - # is the host's job, so that a caller in another major can still see it - if result_kind == host_fns.ResultCode.FATAL_VM_ERROR: - result_kind = host_fns.ResultCode.VM_ERROR return cls( execution_hash=decoded.get('execution_hash', b''), result_kind=result_kind, diff --git a/tests/system/manager-socket/test.py b/tests/system/manager-socket/test.py index f375043c..96300778 100644 --- a/tests/system/manager-socket/test.py +++ b/tests/system/manager-socket/test.py @@ -14,6 +14,7 @@ import genvm_tool_plugins.genvm as genvm import origin.base_host as base_host import origin.calldata as gvm_calldata +import origin.host_fns as host_fns from gvm_extra.mock_host import MockHost, MockStorage from origin.calldata import Address from origin.manager_api import CURRENT_MAJOR, Errors, Methods @@ -376,6 +377,15 @@ async def _malformed_unknown_and_survival(self): await client.send(Methods.CANCEL, 3, {'cancel': {'genvm_id': 999}}) await _read_error(client, 3, Errors.UNKNOWN_ID) + async def _fatal_consumed_result_is_rejected(self): + raw = bytes([host_fns.ResultCode.FATAL_VM_ERROR]) + gvm_calldata.encode({}) + try: + base_host.ConsumedResult.decode(raw) + except base_host.ConsumedResultDecodeError as exc: + assert 'fatal_vm_error crossed the top-level result boundary' in str(exc) + else: + raise AssertionError('fatal consumed_result was accepted') + async def _oversized_closes_connection(self): async with await self._manager('oversized', max_message_bytes=32) as manager: async with ManagerWsClient(manager.uri) as client: @@ -920,6 +930,13 @@ def service( ) cases = [ + ( + 'fatal-result-rejected', + '_fatal_consumed_result_is_rejected', + {}, + None, + genvm.ManagerService, + ), ( 'protocol-errors', '_malformed_unknown_and_survival',