Consensus CLI should reject execution WebSocket endpoints when IPC transport is selected #338

Activity

1. osr21 commented on Sep 4, 2026

   

   osr21

   on Sep 4, 2026

   More actions

   Confirmed against 97f8da0. The premise holds exactly as written, and #337 is a faithful implementation of it. Source review only — no Rust toolchain available here, so I traced call paths rather than running your test. Three things worth adding: one that strengthens the report, one that calibrates its severity down, and one upgrade-compatibility caveat I'd want in the PR before it merges.

   The silent-drop is real

   StartConfig::engine_config() (crates/malachite-app/src/config.rs:159) checks the IPC pair first and builds:

   Some(EngineConfig::Ipc(EthIpcConfig { eth_socket, execution_socket }))

   EthIpcConfig has exactly those two fields — there is no ws slot to carry the value into, so execution_ws_endpoint isn't merely deprioritized, it is structurally unrepresentable in the IPC branch. Only the RPC branch reads it (:172, with the derive_ws_url fallback). Your root-cause diagnosis is correct.

   Stronger than you argued: the mixed config violates an invariant the test suite already asserts

   Because execution_ws_endpoint is in uses_rpc_transport (:712), the mixed config sets that flag true and the node emits the deprecation warning — which tells the operator:

   Migrate to IPC: on the consensus layer use --eth-socket and --execution-socket …

   …to an operator who has just passed exactly --eth-socket and --execution-socket. So today's behaviour isn't only "silently ignores a flag", it's "actively instructs you to do the thing you already did."

   That matters because validate_no_deprecation_warning_in_ipc_mode (start.rs:918) exists specifically to assert:

   assert!(!logs_contain("deprecated"), "IPC mode must not emit the RPC deprecation warning");

   The invariant is already written down — the test just doesn't exercise the ws case, so the gap slipped through. This reframes the issue from "missing conflict check" to "an existing, explicitly tested invariant is violated on an uncovered input," which is a considerably stronger case for merging. #337 fixes it as a side effect, since validation now returns Err before reaching the warning.

   Severity calibration: no events are actually lost

   The "Why this matters" section says an operator "may believe the configured WebSocket endpoint is being used for execution-layer events." Worth being precise about what actually happens, because it's better than implied. SubscriptionEndpoint (crates/eth-engine/src/engine.rs:53) has two variants:

   pub enum SubscriptionEndpoint {
       Ipc { socket_path: String },
       Ws { url: Url },
   }

   Engine::new_ipc unconditionally builds SubscriptionEndpoint::Ipc { socket_path: eth_socket }, while Engine::new_rpc builds ws_endpoint.map(|url| SubscriptionEndpoint::Ws { url }). So in IPC mode the subscription the ws flag exists to enable is still established — over the eth socket. The operator loses no functionality; they lose an accurate mental model of which transport carries the events, which has real consequences for firewalling, monitoring and debugging, but is not event loss. I'd call this a correctness-of-configuration bug, not a functional one, and I think saying so plainly makes the issue more likely to be accepted rather than less.

   The mirror-image case is arguably worse and is not covered here

   In the RPC branch sub_endpoint is an Option and can legitimately end up None: derive_ws_url returns None when the URL has no explicit port (the repo's own derive_ws_url_returns_none_without_port test pins this). So:

   --eth-rpc-endpoint http://host  --execution-endpoint …  --execution-jwt …
   

   with no explicit --execution-ws-endpoint yields no subscription endpoint at all — silently, and with no validation error. That is the same failure mode you're arguing against ("explicitly supplied options must not be silently ignored") in its harsher form: a required capability silently absent rather than a redundant flag silently dropped. It's out of scope for #337 and I'd keep it out, but it's worth a separate issue.

   On #337

   I checked the diff against every existing test in the module and it is non-breaking:

   • validate_warns_on_execution_ws_endpoint_alone (:905) — ws alone now sets has_rpc_options, but has_ipc_options is false, so it still returns Ok and still warns. Passes.
   • validate_no_deprecation_warning_in_ipc_mode (:918) — ws is None, unaffected.
   • validate_ok_with_only_rpc_options, validate_err_when_mixing_ipc_and_rpc, validate_err_with_another_mix_of_options — unaffected.

   Two suggestions:

   1. Collapse the two lists instead of extending one. The root cause is two hand-maintained enumerations of "what counts as an RPC option" that drifted apart. After #337 they are character-for-character identical — both are eth_rpc_endpoint || execution_endpoint || execution_ws_endpoint || execution_jwt. Adding a fourth item to one list fixes this instance while preserving the exact structure that produced it. Since they now agree, they can simply be one binding:

   let has_rpc_options = self.eth_rpc_endpoint.is_some()
       || self.execution_endpoint.is_some()
       || self.execution_ws_endpoint.is_some()
       || self.execution_jwt.is_some();
   
   // … conflict check …
   
   if has_rpc_options {   // was: uses_rpc_transport

   That makes a future divergence impossible rather than merely fixed, and it's strictly smaller than the current diff.

   2. Add the full-RPC-plus-ws case. Your own regression list includes "RPC configuration with --execution-ws-endpoint remains valid," but the only existing coverage is the alone form. The realistic production shape — eth_rpc_endpoint + execution_endpoint + execution_jwt + execution_ws_endpoint — isn't asserted anywhere, and it's precisely what a careless future refactor of has_rpc_options would break.

   Upgrade-compatibility caveat worth calling out in the PR

   This turns a previously accepted configuration into a hard startup failure, and there is a plausible population of configs in exactly that state. start.rs:659 sits in the deployment flag-reconstruction block (--- Deployment-specific fields (set by quake …) ---) and emits --execution-ws-endpoint unconditionally when set, right beside --eth-socket and --execution-socket. Combined with the active RPC→IPC deprecation push, an operator who migrated by adding the two socket flags and leaving a now-inert --execution-ws-endpoint in their manifest has a node that starts fine today and will refuse to start after this change.

   That's the correct fail-fast outcome and I'm not arguing against it — but it's a breaking config change rather than a pure bug fix, so it warrants a release-note line, and the error message could earn its keep by saying the ws endpoint is unused in IPC mode and can simply be removed. The current generic "choose either IPC or RPC" wording doesn't tell that operator that deleting one line is the whole fix.

   Nicely scoped report — the has_rpc_options vs uses_rpc_transport diff is exactly the right place to point, and pinning it to a named commit made it quick to confirm.
2. huklaa commented on Sep 10, 2026

   

   huklaa

   on Sep 10, 2026

   More actions

   Hi, I would like to work on this issue. Could you assign it to me?