diff --git a/NEWS.md b/NEWS.md index 8068ecbc9fa..0ae16c38e6a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,21 @@ # NEWS +## Unreleased + +### What's New + +- **`ethereum.decode()` failures are now logged and counted.** `ethereum.decode` and `ethereum.decodeParams` return `null` on failure, which a mapping is free to swallow silently. Both now log the type string and the underlying error, and increment `deployment_ethereum_decode_failures{deployment, host_fn, kind}`. See the note below. ([#6702](https://github.com/graphprotocol/graph-node/pull/6702)) +- **Gas exhaustion inside `ethereum.decode`, `ethereum.decodeParams` and `ethereum.encode` now aborts the handler** instead of surfacing to the mapping as `null`. A handler that ran out of gas at one of these calls and made no further gas-consuming call previously completed, so this can change POI for that case against earlier versions. ([#6702](https://github.com/graphprotocol/graph-node/pull/6702)) + +### Note on `ethereum.decode()` type string handling + +The `ethabi` → `alloy` migration in v0.42.0 ([#6063](https://github.com/graphprotocol/graph-node/pull/6063)) made ABI type string parsing strict: type strings `ethabi` accepted but that were never valid ABI are now rejected. `kind` distinguishes: + +- **`invalid_type`** — the type string cannot be parsed (e.g. `bytes128`). It is a literal in the mapping, so every call returns `null` on every block; a mapping that logs and returns then writes no entities while the deployment stays healthy and synced at chain head, diverging in POI from indexers still on pre-v0.42.0 graph-node. Logged at **error**; the subgraph must be republished. Alert on this. ([#6683](https://github.com/graphprotocol/graph-node/issues/6683)) +- **`invalid_data`** — data does not match an otherwise valid type. Can legitimately vary per event; logged at **warning**. + +Separately, `ethabi` decoded a leading space (the `" address"` in `"(uint256, address)"`) as `Uint(8)`; `alloy` parses it correctly, so mappings calling `.toBigInt()` on an `Address` abort on v0.42.0+. Recompile with the correct accessor. ([#6461](https://github.com/graphprotocol/graph-node/issues/6461)) + ## v0.45.0 ``` @@ -119,18 +135,6 @@ Thanks to all contributors for this release: @erayack, @fordN, @incrypto32, @lut - Fixed `graphman config pools` not working due to hardcoded pool size override. ([#6444](https://github.com/graphprotocol/graph-node/pull/6444)) - Fixed unfail retry mechanism stopping after the first attempt when the deployment head was still behind the error block. ([#6529](https://github.com/graphprotocol/graph-node/pull/6529)) -### Note on `ethereum.decode()` whitespace handling - -The migration from `ethabi` to `alloy` in v0.42.0 ([#6063](https://github.com/graphprotocol/graph-node/pull/6063)) incidentally fixed a long-standing parsing bug in `ethabi` where type strings containing whitespace before a type name (e.g. `" address"` with a leading space) were silently decoded as `Uint(8)` instead of the intended type. `alloy` parses these correctly. - -Subgraphs that relied on the incorrect `Uint(8)` decoding to subsequently call `.toBigInt()` on what is actually an `Address` value will abort on v0.42.0+ with: - -``` -Mapping aborted ... Ethereum value is not an int or uint. -``` - -This is not a graph-node regression. Recompile the subgraph with the correct accessor (`.toAddress()` for addresses) to fix. See [#6461](https://github.com/graphprotocol/graph-node/issues/6461) for details. - ### gnd (Graph Node Dev) - `gnd indexer` command that delegates to `graph-indexer`, allowing indexer management (allocations, rules, cost models, status) directly through gnd. ([#6492](https://github.com/graphprotocol/graph-node/pull/6492)) diff --git a/graph/src/components/subgraph/host.rs b/graph/src/components/subgraph/host.rs index 40bdc10f8eb..ace9f8542c2 100644 --- a/graph/src/components/subgraph/host.rs +++ b/graph/src/components/subgraph/host.rs @@ -100,6 +100,7 @@ pub struct HostMetrics { handler_execution_time: Box, host_fn_execution_time: Box, eth_call_execution_time: Box, + ethereum_decode_failures: Box, pub gas_metrics: GasMetrics, pub stopwatch: StopwatchMetrics, } @@ -139,15 +140,33 @@ impl HostMetrics { vec![0.025, 0.05, 0.2, 2.0, 8.0, 20.0], ) .expect("failed to create `deployment_host_fn_execution_time` histogram"); + + let ethereum_decode_failures = registry + .new_deployment_counter_vec( + "deployment_ethereum_decode_failures", + "Counts ethereum.decode and ethereum.decodeParams calls that returned null", + subgraph, + vec![String::from("host_fn"), String::from("kind")], + ) + .expect("failed to create `deployment_ethereum_decode_failures` counter"); + Self { handler_execution_time, host_fn_execution_time, stopwatch, gas_metrics, eth_call_execution_time, + ethereum_decode_failures, } } + /// `kind` is `invalid_type` or `invalid_data`. + pub fn inc_ethereum_decode_failure(&self, host_fn: &str, kind: &str) { + self.ethereum_decode_failures + .with_label_values(&[host_fn, kind][..]) + .inc(); + } + pub fn observe_handler_execution_time(&self, duration: f64, handler: &str) { self.handler_execution_time .with_label_values(&[handler][..]) diff --git a/runtime/wasm/src/host_exports.rs b/runtime/wasm/src/host_exports.rs index b6ab94a066f..35ae9cd4835 100644 --- a/runtime/wasm/src/host_exports.rs +++ b/runtime/wasm/src/host_exports.rs @@ -42,6 +42,52 @@ impl IntoTrap for HostExportError { } } +/// Why an `ethereum.decode` / `ethereum.decodeParams` call could not produce a +/// value. Both variants make the host function return `null` to the mapping. +#[derive(Debug)] +pub(crate) enum DecodeError { + /// The type string is not a valid ABI type. It is typically a literal in + /// the mapping, so this fails identically on every block: the subgraph can + /// never decode this value. + InvalidType { + types: String, + source: anyhow::Error, + }, + + /// The type is valid but `data` does not match it. This can legitimately + /// vary from one event to the next. + InvalidData { + types: String, + source: anyhow::Error, + }, +} + +impl DecodeError { + /// Metric label. Kept short and stable; operators alert on this. + pub(crate) fn kind(&self) -> &'static str { + match self { + DecodeError::InvalidType { .. } => "invalid_type", + DecodeError::InvalidData { .. } => "invalid_data", + } + } + + pub(crate) fn types(&self) -> &str { + match self { + DecodeError::InvalidType { types, .. } | DecodeError::InvalidData { types, .. } => { + types + } + } + } + + pub(crate) fn source(&self) -> &anyhow::Error { + match self { + DecodeError::InvalidType { source, .. } | DecodeError::InvalidData { source, .. } => { + source + } + } + } +} + pub struct HostExports { pub(crate) subgraph_id: DeploymentHash, subgraph_network: String, @@ -1217,13 +1263,16 @@ impl HostExports { Ok(encoded) } + /// The outer `Result` is whether the host function could run at all; gas + /// errors must abort the mapping rather than surface as `null`. The inner + /// one is the decode outcome, which the caller turns into `null`. pub(crate) fn ethereum_decode( &self, types: String, data: Vec, gas: &GasCounter, state: &mut BlockState, - ) -> Result { + ) -> Result, DeterministicHostError> { Self::track_gas_and_ops( gas, state, @@ -1231,9 +1280,7 @@ impl HostExports { "ethereum_decode", )?; - let ty: abi::DynSolType = types.parse().context("Failed to read types")?; - - ty.abi_decode(&data).context("Failed to decode") + Ok(decode_abi(&types, &data)) } /// Like [`Self::ethereum_decode`], but decodes `data` as ABI function @@ -1247,7 +1294,7 @@ impl HostExports { data: Vec, gas: &GasCounter, state: &mut BlockState, - ) -> Result { + ) -> Result, DeterministicHostError> { Self::track_gas_and_ops( gas, state, @@ -1255,9 +1302,7 @@ impl HostExports { "ethereum_decode_params", )?; - let ty: abi::DynSolType = types.parse().context("Failed to read types")?; - - ty.abi_decode_params(&data).context("Failed to decode") + Ok(decode_abi_params(&types, &data)) } pub(crate) fn yaml_from_bytes( @@ -1313,6 +1358,38 @@ fn bytes_to_string(logger: &Logger, bytes: Vec) -> String { s.trim_end_matches('\u{0000}').to_string() } +fn parse_type(types: &str) -> Result { + types + .parse::() + .map_err(|e| DecodeError::InvalidType { + types: types.to_string(), + source: anyhow::Error::new(e), + }) +} + +/// Decode `data` as a single ABI value of type `types`. +fn decode_abi(types: &str, data: &[u8]) -> Result { + let ty = parse_type(types)?; + + ty.abi_decode(data).map_err(|e| DecodeError::InvalidData { + types: types.to_string(), + source: anyhow::Error::new(e), + }) +} + +/// Like [`decode_abi`], but decodes `data` as ABI function parameters (the +/// layout used by transaction calldata and event data) rather than as a single +/// ABI value. +fn decode_abi_params(types: &str, data: &[u8]) -> Result { + let ty = parse_type(types)?; + + ty.abi_decode_params(data) + .map_err(|e| DecodeError::InvalidData { + types: types.to_string(), + source: anyhow::Error::new(e), + }) +} + /// Expose some host functions for testing only #[cfg(debug_assertions)] pub mod test_support { @@ -1412,3 +1489,91 @@ fn bytes_to_string_is_lossy() { ) ) } + +#[cfg(test)] +mod decode_tests { + use super::*; + + /// `(uint32, bytes32)` holding `7` and 32 bytes of `0xaa`. Both fields are + /// static, so `abi_decode` and `abi_decode_params` accept the same layout. + fn encoded_uint32_bytes32() -> Vec { + let mut data = vec![0u8; 32]; + data[31] = 7; + data.extend_from_slice(&[0xaa; 32]); + data + } + + /// `bytes128` is not an ABI type at all — fixed size bytes stop at + /// `bytes32` — but ethabi read it as `FixedBytes(128)`, so subgraphs using + /// it kept working until v0.42.0. See #6683. + #[test] + fn unparseable_type_strings_are_invalid_type() { + let types = [ + "bytes128", + "(uint32,uint32,uint32,uint64,bytes32,bytes32,bytes32,bytes128)", + "(uint32,", + "uint7", + // Leading whitespace is only tolerated inside a tuple, so + // `"(uint256, address)"` parses but a bare `" address"` does not. + " address", + ]; + + for ty in types { + for err in [ + decode_abi(ty, &encoded_uint32_bytes32()).unwrap_err(), + decode_abi_params(ty, &encoded_uint32_bytes32()).unwrap_err(), + ] { + assert!( + matches!(err, DecodeError::InvalidType { .. }), + "expected `{ty}` to be rejected as an invalid type, got {err:?}" + ); + assert_eq!(err.kind(), "invalid_type"); + assert_eq!(err.types(), ty); + } + } + } + + /// Data that cannot be read against an otherwise valid type. Unlike an + /// unparseable type string this can legitimately differ per event, which is + /// why the two are kept apart. + #[test] + fn data_not_matching_a_valid_type_is_invalid_data() { + for data in [vec![], vec![0u8; 8], vec![0u8; 63]] { + for err in [ + decode_abi("(uint32,bytes32)", &data).unwrap_err(), + decode_abi_params("(uint32,bytes32)", &data).unwrap_err(), + ] { + assert!( + matches!(err, DecodeError::InvalidData { .. }), + "expected {} bytes to fail as invalid data, got {err:?}", + data.len() + ); + assert_eq!(err.kind(), "invalid_data"); + assert_eq!(err.types(), "(uint32,bytes32)"); + } + } + } + + #[test] + fn valid_type_and_data_decodes() { + for decoded in [ + decode_abi("(uint32,bytes32)", &encoded_uint32_bytes32()).unwrap(), + decode_abi_params("(uint32,bytes32)", &encoded_uint32_bytes32()).unwrap(), + ] { + let abi::DynSolValue::Tuple(fields) = decoded else { + panic!("expected a tuple, got {decoded:?}"); + }; + + assert!( + matches!(fields[0], abi::DynSolValue::Uint(v, 32) if v == abi::AlloyU256::from(7)), + "unexpected first field: {:?}", + fields[0] + ); + assert!( + matches!(&fields[1], abi::DynSolValue::FixedBytes(b, 32) if b[..32] == [0xaa; 32]), + "unexpected second field: {:?}", + fields[1] + ); + } + } +} diff --git a/runtime/wasm/src/module/context.rs b/runtime/wasm/src/module/context.rs index 70d65489304..647e9a8e723 100644 --- a/runtime/wasm/src/module/context.rs +++ b/runtime/wasm/src/module/context.rs @@ -15,6 +15,7 @@ use never::Never; use crate::HostExports; use crate::asc_abi::class::*; +use crate::host_exports::DecodeError; use graph::data::store; use crate::ExperimentalFeatures; @@ -1152,12 +1153,10 @@ impl WasmInstanceContext<'_> { let token = asc_get(self, token_ptr, gas)?; let host_exports = self.as_ref().ctx.host_exports.cheap_clone(); let ctx = &mut self.as_mut().ctx; - let data = host_exports.ethereum_encode(token, gas, &mut ctx.state); - // return `null` if it fails - match data { - Ok(bytes) => asc_new(self, &*bytes, gas).await, - Err(_) => Ok(AscPtr::null()), - } + // `abi_encode` is infallible, so the only error here is gas, which must + // abort the mapping rather than surface as `null`. + let bytes = host_exports.ethereum_encode(token, gas, &mut ctx.state)?; + asc_new(self, &*bytes, gas).await } /// function decode(types: String, data: Bytes): ethereum.Value | null @@ -1171,12 +1170,14 @@ impl WasmInstanceContext<'_> { let data = asc_get(self, data_ptr, gas)?; let host_exports = self.as_ref().ctx.host_exports.cheap_clone(); let ctx = &mut self.as_mut().ctx; - let result = host_exports.ethereum_decode(types, data, gas, &mut ctx.state); + let result = host_exports.ethereum_decode(types, data, gas, &mut ctx.state)?; - // return `null` if it fails match result { Ok(token) => asc_new(self, &token, gas).await, - Err(_) => Ok(AscPtr::null()), + Err(e) => { + self.report_decode_failure("ethereum.decode", &e); + Ok(AscPtr::null()) + } } } @@ -1191,15 +1192,53 @@ impl WasmInstanceContext<'_> { let data = asc_get(self, data_ptr, gas)?; let host_exports = self.as_ref().ctx.host_exports.cheap_clone(); let ctx = &mut self.as_mut().ctx; - let result = host_exports.ethereum_decode_params(types, data, gas, &mut ctx.state); + let result = host_exports.ethereum_decode_params(types, data, gas, &mut ctx.state)?; - // return `null` if it fails match result { Ok(token) => asc_new(self, &token, gas).await, - Err(_) => Ok(AscPtr::null()), + Err(e) => { + self.report_decode_failure("ethereum.decodeParams", &e); + Ok(AscPtr::null()) + } } } + /// Log a decode failure and count it in `deployment_ethereum_decode_failures`. + fn report_decode_failure(&self, host_fn: &'static str, err: &DecodeError) { + let data = self.as_ref(); + + // The type string comes from the mapping and has no length limit, and + // the parse error quotes it back, so cap both for logging. The value + // passed to the decoder is never truncated. + let types = truncate_for_logging(err.types()); + let source = truncate_for_logging(&format!("{:#}", err.source())); + + match err { + // Recurs on every matching trigger and never clears without + // republishing the subgraph. + DecodeError::InvalidType { .. } => error!( + data.ctx.logger, + "{} returned null: invalid ABI type string, so every such call fails \ + and the mapping may be dropping data", + host_fn; + "types" => &types, + "kind" => err.kind(), + "error" => &source, + ), + DecodeError::InvalidData { .. } => warn!( + data.ctx.logger, + "{} returned null: data does not match the type", + host_fn; + "types" => &types, + "kind" => err.kind(), + "error" => &source, + ), + } + + data.host_metrics + .inc_ethereum_decode_failure(host_fn, err.kind()); + } + /// function arweave.transactionData(txId: string): Bytes | null pub async fn arweave_transaction_data( &self, @@ -1272,6 +1311,15 @@ impl WasmInstanceContext<'_> { } } +/// Mapping-supplied strings have no length limit, so only log the first 1024 +/// characters. Splits on a character boundary since the string is arbitrary. +fn truncate_for_logging(s: &str) -> String { + match s.char_indices().nth(1024) { + Some((end, _)) => format!("(truncated) {}", &s[..end]), + None => s.to_string(), + } +} + /// For debugging, it might be useful to know exactly which bytes could not be parsed as YAML, but /// since we can parse large YAML documents, even one bad mapping could produce terabytes of logs. /// To avoid this, we only log the first 1024 bytes of the failed YAML source.