From 4cd442ddb96b82b6d68bfb3c1fec35ca814ffa61 Mon Sep 17 00:00:00 2001 From: mfw78 Date: Tue, 8 Sep 2026 23:07:47 +0000 Subject: [PATCH] fix(cow): attribute an unreadable poll refusal before acting on it `classify_revert` fell through to `TryNextBlock` whenever it could not find a selector, so a refusal with no readable payload re-polled on every block indefinitely, silently, with nothing to distinguish it from a healthy scheduling gap. The poll interface is closed: a generator answers through `GeneratorResult` codes and the registry's own refusals carry selectors. An empty payload is outside that, so it names a defect. It does not say whose, and the candidates are not alike, so the classifier no longer guesses. `classify_revert` returns a `Refusal` and the keeper attributes it with one `eth_getCode`. A codeless owner cannot answer the registry's ERC-1271 probe, which makes that call revert in the caller's own frame with nothing attached. That is the registration answering for itself and it drops, loudly. Every other cause points outward: a gas cap too low for the handler, or a registry address that is not the fork. Those are the same for every commitment, so a drop would delete a whole watch set to report an operator's typo. They back off for an hour and name the likely cause. A failed `eth_getCode` reads as unattributable and takes that path too. Reachable in production by an ordinary mistake, and the codeless case is recoverable rather than permanent: an EOA can gain code through an EIP-7702 delegation, which is exactly the flow #658 describes. Closes #692. AI Assistance: Claude Code used for the fix and the tests. --- crates/composable-cow/src/fork.rs | 103 ++++++++++------- crates/composable-cow/src/lib.rs | 2 +- modules/ccow-monitor/src/keeper.rs | 175 +++++++++++++++++++++++++---- 3 files changed, 214 insertions(+), 66 deletions(-) diff --git a/crates/composable-cow/src/fork.rs b/crates/composable-cow/src/fork.rs index 026c89a0..ba3ab230 100644 --- a/crates/composable-cow/src/fork.rs +++ b/crates/composable-cow/src/fork.rs @@ -212,34 +212,47 @@ sol! { } } -/// Classify a failed poll `eth_call`. Every reachable revert is -/// deterministic on-chain state, so all are terminal; a re-`create` -/// re-indexes through its own event. A payload-free failure is the -/// transport, not the contract, so it stays retryable. +/// What a poll revert says about the commitment. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Refusal { + /// The contract refused with a selector this build can read. + Named(Selector), + /// The node executed the call and refused with nothing readable. + /// + /// The poll interface is closed: a generator answers through + /// `GeneratorResult` codes and the registry's own refusals carry + /// selectors. An empty payload is outside that, so it names a + /// defect. It does not say whose, and the candidates are not alike: + /// a codeless owner is the registration's, a gas cap too low for the + /// handler or a wrong registry address is the operator's. + Unattributed, + /// The call never reached the node. + Transport, +} + +/// Classify a poll revert. Attribution of [`Refusal::Unattributed`] +/// needs a second chain read, so it happens at the call site. #[must_use] -pub fn classify_revert(err: &ChainError) -> Verdict { +pub fn classify_revert(err: &ChainError) -> Refusal { let ChainError::Rpc(rpc) = err else { - // `ChainError` is `#[non_exhaustive]`: transport faults and any - // future case are payload-free, so they stay retryable. - return Verdict::TryNextBlock { - reason: Selector::ZERO, - }; - }; - let Some(data) = rpc.data.as_deref() else { - return Verdict::TryNextBlock { - reason: Selector::ZERO, - }; + return Refusal::Transport; }; - let Some(reason) = data.get(..4).map(Selector::from_slice) else { - return Verdict::TryNextBlock { - reason: Selector::ZERO, - }; - }; - // Unrecognised still means the contract refused; a handler `Panic` - // lands here. - Verdict::Invalid { reason } + rpc.data + .as_deref() + .and_then(|data| data.get(..4)) + .map_or(Refusal::Unattributed, |s| { + Refusal::Named(Selector::from_slice(s)) + }) } +/// How long a payload-free revert takes the commitment out of the +/// rotation. +/// +/// Long enough that the poll budget is not spent on a deterministic +/// failure, short enough that a commitment recovers on its own if the +/// cause was the node rather than the registration. +pub const PAYLOAD_FREE_BACKOFF_S: u64 = 3_600; + /// Selectors the classifier recognises, for logging and tests. #[must_use] pub fn is_residual_selector(selector: Selector) -> bool { @@ -591,7 +604,7 @@ mod tests { mod residual_tests { use alloy_primitives::fixed_bytes; use alloy_sol_types::SolError; - use nexum_sdk::host::RpcError; + use nexum_sdk::host::{Fault, RpcError}; use super::*; @@ -614,11 +627,8 @@ mod residual_tests { ] .map(Selector::from) { - let verdict = classify_revert(&reverted(Some(selector.to_vec()))); - assert!( - matches!(verdict, Verdict::Invalid { reason } if reason == selector), - "{selector:?} produced {verdict:?}", - ); + let refusal = classify_revert(&reverted(Some(selector.to_vec()))); + assert_eq!(refusal, Refusal::Named(selector), "{selector:?}"); assert!(is_residual_selector(selector)); } } @@ -628,22 +638,33 @@ mod residual_tests { fn an_unrecognised_selector_is_terminal() { let panic_selector = Selector::new([0x4e, 0x48, 0x7b, 0x71]); assert!(!is_residual_selector(panic_selector)); - assert!(matches!( + assert_eq!( classify_revert(&reverted(Some(panic_selector.to_vec()))), - Verdict::Invalid { .. } - )); + Refusal::Named(panic_selector), + ); } + /// The closed poll interface specifies neither of these, so both + /// name a defect the caller has to attribute before acting. #[test] - fn a_payload_free_failure_stays_retryable() { - assert!(matches!( - classify_revert(&reverted(None)), - Verdict::TryNextBlock { .. } - )); - assert!(matches!( - classify_revert(&reverted(Some(vec![1, 2]))), - Verdict::TryNextBlock { .. } - )); + fn an_unreadable_payload_is_not_attributed_here() { + for data in [None, Some(vec![1, 2])] { + assert_eq!( + classify_revert(&reverted(data.clone())), + Refusal::Unattributed, + "{data:?}", + ); + } + } + + /// A transport fault never reached the node, so it says nothing + /// about the contract. + #[test] + fn a_transport_fault_is_its_own_class() { + assert_eq!( + classify_revert(&ChainError::Fault(Fault::Unavailable("node down".into()))), + Refusal::Transport, + ); } /// A rename upstream must fail here, not silently reclassify a diff --git a/crates/composable-cow/src/lib.rs b/crates/composable-cow/src/lib.rs index b56da0a9..5add6f41 100644 --- a/crates/composable-cow/src/lib.rs +++ b/crates/composable-cow/src/lib.rs @@ -16,7 +16,7 @@ pub mod poll; #[cfg(feature = "run")] pub mod run; -pub use fork::{Mapped, PollResult, Suppressed, classify_revert, map_verdict, to_verdict}; +pub use fork::{Mapped, PollResult, Refusal, Suppressed, classify_revert, map_verdict, to_verdict}; pub use poll::{NextPoll, Verdict}; #[cfg(feature = "run")] pub use run::run; diff --git a/modules/ccow-monitor/src/keeper.rs b/modules/ccow-monitor/src/keeper.rs index 9f1fcea4..ce0daf7e 100644 --- a/modules/ccow-monitor/src/keeper.rs +++ b/modules/ccow-monitor/src/keeper.rs @@ -11,7 +11,9 @@ use alloy_primitives::{Address, B256, Bytes, Selector, keccak256}; use alloy_sol_types::{SolCall, SolEvent, SolValue}; use composable::ConditionalOrderParams; -use composable_cow::fork::{classify_revert, decode_poll_return, map_verdict, to_verdict}; +use composable_cow::fork::{ + PAYLOAD_FREE_BACKOFF_S, Refusal, classify_revert, decode_poll_return, map_verdict, to_verdict, +}; use composable_cow::{Verdict, run}; use cow_venue::CowClient; // The poll path receives the order inside `PollResult`, so the bare type @@ -628,31 +630,72 @@ fn poll_one( to_verdict(map_verdict(&result, &signature), valid_to) }, ), - Err(err) => { - let outcome = classify_revert(&err); - match &err { - ChainError::Fault(fault) => { - tracing::warn!("eth_call failed ({fault}); retrying next block"); + Err(err) => match classify_revert(&err) { + Refusal::Transport => { + tracing::warn!("eth_call failed ({err}); retrying next block"); + Verdict::TryNextBlock { + reason: Selector::ZERO, } - // A permanent drop deserves its cause on the record: the - // selector and the node's message are unrecoverable once - // the commitment is gone. - ChainError::Rpc(rpc) if matches!(outcome, Verdict::Invalid { .. }) => { - let selector = rpc - .data - .as_deref() - .and_then(|data| data.get(..4)) - .map(|s| format!("{:#x}", Selector::from_slice(s))) - .unwrap_or_else(|| "none".to_string()); - tracing::warn!( - "eth_call reverted permanently (selector {selector}, {}); \ - dropping commitment", - rpc.message, - ); - } - _ => {} } - outcome + // A permanent drop deserves its cause on the record: the + // selector is unrecoverable once the commitment is gone. + Refusal::Named(reason) => { + tracing::warn!( + "eth_call reverted permanently (selector {reason:#x}, {err}); \ + dropping commitment" + ); + Verdict::Invalid { reason } + } + Refusal::Unattributed => attribute_unreadable(host, tick, owner, &err), + }, + } +} + +/// Decide who an unreadable refusal belongs to. +/// +/// The registry probes the owner for ERC-1271 support before it builds a +/// signature, and a codeless owner makes that probe revert in the +/// caller's own frame with nothing attached. So an empty payload plus an +/// owner with no code is the registration answering for itself, and no +/// later poll of this commitment changes it. +/// +/// Every other cause points outward, at a gas cap too low for the +/// handler or a registry address that is not the fork. Those are the +/// operator's, they are the same for every commitment, and removing a +/// watch set is not how to report them. +fn attribute_unreadable( + host: &H, + tick: &Tick, + owner: &Address, + err: &ChainError, +) -> Verdict { + let params = format!(r#"["{owner:#x}","latest"]"#); + let code = host + .request(tick.chain_id, "eth_getCode", ¶ms) + .ok() + .and_then(|json| parse_eth_call_result(&json)); + match code.as_deref() { + Some([]) => { + tracing::error!( + "dropping commitment: owner {owner:#x} has no code, so the registry cannot \ + build an ERC-1271 signature for it ({err})" + ); + Verdict::Invalid { + reason: Selector::ZERO, + } + } + // Includes the read failing: without an answer this is not + // attributable, and the safe reading of that is the loud one. + _ => { + tracing::error!( + "poll refused with no readable payload and owner {owner:#x} has code; \ + check the poll gas cap and the registry address ({err}); \ + backing off {PAYLOAD_FREE_BACKOFF_S}s" + ); + Verdict::WaitTimestamp { + wait_until: tick.epoch_s.saturating_add(PAYLOAD_FREE_BACKOFF_S), + reason: Selector::ZERO, + } } } } @@ -1873,6 +1916,90 @@ mod tests { assert!(!store.keys().any(|k| k.starts_with("submitted:"))); } + /// A codeless owner cannot answer the registry's ERC-1271 probe, so + /// the poll reverts in the caller's own frame with nothing attached. + /// That is the registration answering for itself. + #[test] + fn an_unreadable_refusal_with_a_codeless_owner_drops() { + use nexum_sdk::host::RpcError; + + let host = MockHost::new(); + let venue = MockVenue::default(); + let owner = address!("0011223344556677889900AABBCCDDEEFF001122"); + let params = sample_params(); + let key = seed_commitment(&host, owner, ¶ms); + + host.chain.respond_to( + "eth_call", + programmed_eth_call_params(owner, ¶ms), + Err(ChainError::Rpc(RpcError { + code: 3, + message: "execution reverted".into(), + data: None, + })), + ); + host.chain.respond_to( + "eth_getCode", + format!(r#"["{owner:#x}","latest"]"#), + Ok("\"0x\"".to_owned()), + ); + + let (result, logs) = capture_tracing(|| dispatch(&host, &venue, sample_block(1_000))); + result.unwrap(); + + assert!( + !host.store.snapshot().contains_key(&key), + "the commitment is removed", + ); + assert!( + logs.any(|e| e.message.contains("has no code")), + "and the drop says why", + ); + } + + /// Every other cause of an unreadable refusal points outward, at the + /// gas cap or the registry address. Those are the same for every + /// commitment, so removing one reports nothing and loses a watch. + #[test] + fn an_unreadable_refusal_with_a_contract_owner_backs_off() { + use nexum_sdk::host::RpcError; + + let host = MockHost::new(); + let venue = MockVenue::default(); + let owner = address!("0011223344556677889900AABBCCDDEEFF001122"); + let params = sample_params(); + let key = seed_commitment(&host, owner, ¶ms); + + host.chain.respond_to( + "eth_call", + programmed_eth_call_params(owner, ¶ms), + Err(ChainError::Rpc(RpcError { + code: 3, + message: "out of gas".into(), + data: None, + })), + ); + host.chain.respond_to( + "eth_getCode", + format!(r#"["{owner:#x}","latest"]"#), + Ok("\"0x60806040\"".to_owned()), + ); + + let (result, logs) = capture_tracing(|| dispatch(&host, &venue, sample_block(1_000))); + result.unwrap(); + + let store = host.store.snapshot(); + assert!(store.contains_key(&key), "the commitment survives"); + assert!( + store.keys().any(|k| k.starts_with("next_epoch:")), + "and is gated forward: {store:?}", + ); + assert!( + logs.any(|e| e.message.contains("gas cap")), + "and the operator is pointed at the likely cause", + ); + } + #[test] fn poll_invalid_drops_commitment_and_gates() { // A residual revert must delete the commitment and any stale