Skip to content

feat: surface inner-frame revert data and implement check_mesh#71

Open
crazywriter1 wants to merge 1 commit into
circlefin:mainfrom
crazywriter1:feat/relay-bubble-revert-mesh-check
Open

feat: surface inner-frame revert data and implement check_mesh#71
crazywriter1 wants to merge 1 commit into
circlefin:mainfrom
crazywriter1:feat/relay-bubble-revert-mesh-check

Conversation

@crazywriter1
Copy link
Copy Markdown
Contributor

Summary

This PR addresses two unrelated but small in-scope items that surfaced during a repository sweep for FIXME / todo!() markers:

  1. tests/localdev/NativeFiatToken.test.ts// FIXME no error message for address blocked?
    The denylist scenarios in NativeFiatToken rely on NativeTransferHelper.relay to forward a call into a denylisted address. With the previous helper implementation the inner revert reason was swallowed and replaced with a generic "Relay reverted", so the assertions could only check for that generic string instead of the real on-chain reason ("Blocked address" / ERR_BLOCKED_ADDRESS).

    NativeTransferHelper.relay now propagates the inner call's revert data verbatim when requireSuccess == true, and the affected tests now assert ERR_BLOCKED_ADDRESS (which already exists in tests/helpers/NativeCoinControl.ts).

  2. crates/test/checks/src/mesh.rspub async fn check_mesh(...) { todo!() }
    The function was exported and reachable as a public API of arc-checks, so any caller wiring it up would have panicked at runtime. Execution JSON-RPC does not expose gossipsub mesh state, so this PR implements check_mesh as a documented best-effort proxy using net_peerCount (devp2p peer count) plus an explicit skip / not verifiable path when the method is missing or disabled, instead of leaving a panic in the codebase.


Changes

1. contracts/src/mocks/NativeTransferHelper.sol

relay(address target, uint256 amount, bool requireSuccess, bytes calldata data) no longer collapses an inner revert into "Relay reverted". When requireSuccess is true and the inner call reverts, the function now re-emits the original revert payload via inline assembly:

(bool success, bytes memory returndata) = target.call{value: amount}(data);
if (!success && requireSuccess) {
    assembly ("memory-safe") {
        let len := mload(returndata)
        revert(add(returndata, 0x20), len)
    }
}

Behavior summary:

Case Old behavior New behavior
success == true no revert no revert (unchanged)
success == false, requireSuccess == false no revert no revert (unchanged)
success == false, requireSuccess == true revert("Relay reverted") (generic) revert(<inner returndata>) (propagated)

This means callers and tests can now distinguish why the inner frame failed (blocked address, balance, etc.) instead of seeing a single shape for every failure. No production code reads "Relay reverted"; this helper is only used by tests under tests/localdev and tests/simulation.

2. tests/localdev/NativeFiatToken.test.ts

Replaces the placeholder assertion at the // FIXME no error message for address blocked? site with the existing ERR_BLOCKED_ADDRESS regex from tests/helpers/NativeCoinControl.ts:

// before
).to.be.rejectedWith(ContractFunctionExecutionError, /Relay reverted/)

// after
).to.be.rejectedWith(ContractFunctionExecutionError, ERR_BLOCKED_ADDRESS)

ERR_BLOCKED_ADDRESS (/Blocked address/) is already used throughout the same file for direct (top-level) blocklist hits; this PR makes the inner-frame requireSuccess=true case assert the same shape, so denylist behavior is verified end-to-end whether the rejection happens at the top frame or inside a relayed sub-call. The FIXME comment is removed.

3. crates/test/checks/src/mesh.rs

Replaces the todo!() placeholder with a working check_mesh that:

  • Iterates over each (node_name, rpc_url) in the supplied list.
  • For each node, looks up the expected peer set in expected_peers.
    • Empty expected set → records a passed = true skip-style result with "mesh: no expected peers configured for this node (skipped)". This keeps callers passing a partial config from being treated as a failure.
  • Issues a single JSON-RPC call: net_peerCount (5 s request timeout, async reqwest::Client).
  • Maps the outcome to a CheckResult:
    • RPC OK, peer count parses, count >= expected.len() → pass ("net_peerCount=N >= expected M (devp2p peer count as connectivity proxy)").
    • RPC OK, count below expected → fail with the actual peer names so the operator can see which links are missing.
    • RPC OK, unparsable response value → fail (defensive).
    • JSON-RPC error response (method not enabled / not implemented) → pass with an explanatory message: gossipsub topology is not verified, but the check is not flipped to a hard fail in that case (mirrors existing convention in arc-checks where missing introspection surfaces degrade gracefully, same way admin_* based checks behave).
    • Transport error (no response, connection refused, JSON parse error) → fail (real connectivity problem worth surfacing).

The module documentation explicitly calls out that this is a proxy for the missing libp2p gossipsub mesh introspection. If/when a dedicated RPC (or a different surface like Prometheus metrics) is added, this function can be tightened without breaking existing callers, because the public signature is unchanged:

pub async fn check_mesh(
    rpc_urls: &[(String, Url)],
    expected_peers: &HashMap<String, Vec<String>>,
) -> color_eyre::eyre::Result<Report>

Note: check_mesh is currently exported but has no callers in this repo. This PR does not wire it into a binary or smoke job; it only ensures the function is implementable and would not panic if invoked by an external tool or a future check pipeline.


Why one PR?

Both changes share a single small theme — replace placeholder/error-eating code paths with something that actually carries the underlying signal (revert reason in one case, peer-count signal in the other) — and the diffs are narrow and isolated. Touching all of them in one PR avoids two near-empty branches. If you'd prefer split PRs (Solidity helper + tests vs. Rust check), I'm happy to break it up — let me know.


Files touched

File Lines changed Theme
contracts/src/mocks/NativeTransferHelper.sol +7 / −2 helper bubbles inner revert
tests/localdev/NativeFiatToken.test.ts ~+1 / −1 lines + FIXME removed assert ERR_BLOCKED_ADDRESS instead of /Relay reverted/
crates/test/checks/src/mesh.rs +~135 / −1 replace todo!() with net_peerCount-based check

Testing

  • forge build --sizes — clean. Warnings (selfdestruct, payable fallback, unused params) are pre-existing in upstream files unrelated to this PR.
  • forge test -vvv279 passed, 0 failed, 1 skipped (pre-existing testEthCodeHash skip).
  • cargo fmt --all -- --check — clean.
  • cargo clippy -p arc-checks --all-targets -- -D warnings — clean. The new check_mesh only depends on reqwest, serde, serde_json, url, color_eyre — all already in arc-checks.
  • make smoke-reth — ran locally on WSL2 (Windows 11). The Reth localdev suite has a number of pre-existing flakes / environment-dependent failures on this local setup that are unrelated to this PR (e.g. genesis: Memo (genesis-placed), genesis: Multicall3From (genesis-placed), ProtocolConfig implementation (salt=0) address mismatch, several mock-CL timing timeouts). The same failure set appears against origin/main with no local changes applied, so they are not introduced here. The CI smoke job should be the source of truth for those.

Non-goals / out of scope

  • Real gossipsub mesh introspection. That requires either a new JSON-RPC method or a different transport (e.g. a metrics exporter) and is intentionally left as future work; the module docstring spells this out.
  • Behavior change for callers that already gracefully ignored "Relay reverted". The new revert payload is the inner call's exact revert data, which is what most tests/inspectors actually want anyway.
  • No changes to genesis, consensus, networking, or any production node path.

Compatibility

  • NativeTransferHelper is a test mock under contracts/src/mocks/; no production contract depends on it.
  • check_mesh is currently exported but unused inside this repository; behavior is additive.
  • No ABI / wire-format / storage layout changes.

…rs check

NativeTransferHelper.relay now propagates inner call revert data via inline
assembly when requireSuccess is true, so blocklist errors like
ERR_BLOCKED_ADDRESS surface in tests instead of a generic "Relay reverted".
NativeFiatToken.test.ts updated to assert ERR_BLOCKED_ADDRESS for the
blocked-address inner frame case (resolves the FIXME).
crates/test/checks/src/mesh.rs replaces the todo!() placeholder with a
net_peerCount-based peer count check (gossipsub mesh isn't exposed; this
is the closest available proxy for healthcheck purposes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant