SyncEndpointUrl accepts endpoints that later panic or fail Display round-tripping

[dumanoglu1]

Summary

SyncEndpointUrl accepts HTTP endpoints that can later panic in websocket() or lose information when formatted and reparsed.

Update after review: the original IPv6 case was incorrect. With url 2.5.8, Url::host_str() already returns bracketed IPv6 hosts, so that case should be ignored. The remaining issue is the runtime panic plus several Display / round-trip cases.

Cases

Maximum explicit HTTP port panics when the WebSocket URL is derived

let endpoint: SyncEndpointUrl = "http://localhost:65535".parse()?;
let _ = endpoint.websocket();

Parsing succeeds, but deriving the WebSocket URL executes:

http_port.checked_add(1).expect("port overflow")

That is an unconditional panic in every build profile. Invalid derived-port combinations should be rejected during parsing instead of failing later from a public accessor. An explicit override should still be accepted, e.g. http://localhost:65535,ws=9000.

HTTP path, query, and fragment are lost by Display

let endpoint: SyncEndpointUrl =
    "https://rpc.example.com/api/v1?key=value,wss=ws.example.com/websocket".parse()?;
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed);

Display reconstructs the HTTP side from only scheme, host, and port, so URL components such as /api/v1, ?key=value, and fragments are discarded.

HTTP userinfo is accepted by FromStr but dropped by Display

let endpoint: SyncEndpointUrl = "http://user:pass@localhost:8545,ws=8546".parse()?;
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed);

The parser accepts credentials because only the scheme is validated, but Display never emits them. Dropping credentials may be desirable for logs, but then Display should be treated as a log/debug format rather than a round-trippable serialization format.

Derived-vs-explicit WebSocket state breaks structural round-tripping

let endpoint: SyncEndpointUrl = "http://localhost:8545".parse()?; // ws: None
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;    // ws: Some(ws://localhost:8546/)
assert_eq!(endpoint, reparsed);

The two values resolve to the same websocket() URL, but they are structurally unequal because SyncEndpointUrl derives PartialEq over ws: Option<Url>. This affects the common/default form, including both default follow endpoints in crates/malachite-app/src/config.rs:

• https://rpc.testnet.arc.io/
• http://localhost:8545

Expected behavior

• Accepted endpoint values should not panic when websocket() is called.
• parse -> Display -> parse should preserve parser-produced endpoint values, or Display should be documented/treated as a lossy log format.
• Derived and explicitly equivalent WebSocket URLs should not become an accidental equality trap unless the distinction is intentionally meaningful.

Implementation notes

The cases split into two independent fixes.

1. Eagerly derive the WebSocket URL during parsing

Store ws: Url instead of ws: Option<Url>, deriving the default WebSocket URL inside FromStr when no override is supplied.

That would close:

• the :65535 panic, because checked_add can fail during parsing and return Err
• the derived-vs-explicit round-trip failure, because both forms store the same concrete Url

This does not require a Display change and should preserve the current canonical output for derived endpoints such as http://localhost:8545,ws=8546 and default-port forms such as https://example.com:443,wss=443.

The semantic caveat is that http://x:8545 and http://x:8545,ws=8546 would compare equal after parsing. That appears to have no production blast radius:

• SyncEndpointUrl derives Debug, Clone, PartialEq, and Eq, but not Hash, so it cannot be used directly as a HashMap / HashSet key.
• I did not find any dedup, retain, position, or contains use over rpc_sync_endpoints or PeerRegistry::endpoints.
• PeerRegistry builds peers by iterating the endpoint list positionally rather than comparing entries.

So the equality change is visible to tests or future code that chooses to assert on it, but there does not appear to be a current production consumer of the derived-vs-explicit distinction.

2. Decide whether Display is serialization or logging

Path/query/fragment loss and userinfo loss are Display-format problems. Emitting the HTTP side with URL-aware serialization, e.g. self.http.as_str(), would preserve those components, but it also changes pinned output such as explicit default ports and raises the delimiter question.

The current format is comma-delimited via split_once(','). It round-trips only for values produced by the current parser shape. If future code adds a public constructor, serde support, or a builder that can produce URLs containing commas or =, Display-as-serialization would need proper escaping or a different format.

So this part should be a deliberate decision:

• If Display is intended as a serialization format, use URL-aware serialization and handle delimiter/credential edge cases.
• If Display is intended only for logs/debug output, document that it is lossy and avoid treating parse -> Display -> parse as a contract.

[osr21]

Assessed against main and the url version pinned in Cargo.lock (2.5.8). Source review only — there's no Rust toolchain in my environment, so nothing below was executed; it's read from the repo source and the url crate's published source.

Two of the three cases are real. The third isn't, and PR #330 has already built a fix for it. There are also two further cases of exactly the same class that the issue misses — one of which breaks the round-trip property for the majority of real configs, including the endpoint this repo ships as a default.

Case 1 — port 65535: confirmed, and the reasoning is stronger than stated

Accurate, and you quoted the right expression. Worth adding why it's a clean crash rather than something worse: checked_add(1).expect("port overflow") is an unconditional panic, independent of build profile. This workspace's [profile.release] sets lto, opt-level, codegen-units, and strip but not overflow-checks — so had that line been a plain http_port + 1, release builds would have silently wrapped to port 0 and produced a follower quietly dialling the wrong port. Whoever wrote checked_add already saw this; the bug is that the check fires at call time instead of parse time.

Blast radius is narrow — it needs RPC-sync/follow mode plus an explicitly configured :65535 endpoint — but it's the only case here that can take a node down, which is worth separating from the rest.

Case 2 — path/query lost by Display: confirmed

Correct as written. Display rebuilt the HTTP side from scheme + host + port only, so /api/v1?key=value was genuinely discarded.

Case 3 — IPv6 brackets: I believe this one is incorrect

The claim is that host_str() returns ::1 and Display writes it unbracketed. url 2.5.8's own documentation for Url::host_str states the opposite:

Return the string representation of the host (domain or IP address) for this URL, if any. […] IPv6 addresses are given between [ and ] brackets.

The same holds one level down: Host's Display impl in host.rs explicitly writes "[", then write_ipv6(addr), then "]". So the existing code should already emit http://[::1]:8545 and the authority should already be valid.

This matters because #330 implemented a fix for it — a host_for_display helper that swaps url's WHATWG serializer for std's Ipv6Addr Display. Those differ on IPv4-mapped addresses (::ffff:127.0.0.1 vs ::ffff:7f00:1), so the remedy for a non-existent canonicalisation bug introduces a real one. I've left the detail on that PR.

Quickest way to settle it definitively: add http://[::1]:8545,ws=8546 as a Display test against unmodified main. If it already passes, the case can be struck.

Missing case A — userinfo is silently dropped

Same class as case 2, and more likely to be hit in practice than IPv6. validate_http_scheme only checks the scheme, so credentials parse fine, but Display never emits them:

let endpoint: SyncEndpointUrl = "http://user:pass@localhost:8545,ws=8546".parse()?;
// Display -> "http://localhost:8545,ws=8546"   (user:pass@ gone)
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed); // fails

Basic-auth RPC endpoints are a common shape for hosted providers, so this is a realistic follow endpoint. Note the flip side: for a value that gets logged, dropping credentials is arguably the desirable behaviour — which is a good argument for deciding deliberately whether Display is a serialisation format or a log format, rather than letting it be an accident of which components got hand-copied.

Missing case B — the derived-vs-explicit distinction is erased, so round-trip fails for most configs

This is the one I'd rank highest, because it defeats the issue's own stated expectation in the common case.

Display writes the ,<scheme>= segment unconditionally, even when ws is None. Since PartialEq is derived over { http: Url, ws: Option<Url> }, None != Some(_):

let endpoint: SyncEndpointUrl = "http://localhost:8545".parse()?;  // ws: None
// Display -> "http://localhost:8545,ws=8546"
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;     // ws: Some(ws://localhost:8546/)
assert_eq!(endpoint, reparsed); // fails

So parse -> Display -> parse is not identity for any endpoint without an explicit WS override. That includes both values in default_rpc_sync_endpoint (config.rs:198) — http://localhost:8545 for localdev and https://rpc.testnet.arc.io/ for testnet. The shipped defaults fail the invariant.

The reason nobody noticed: every existing round-trip assertion uses an input that already carries an explicit override (,wss=rpc.testnet.example.com/websocket), and #330's two new ones do too (,wss=ws.example.com/websocket and ,ws=8546). display_http_only and display_https_only cover the derived case but only assert the output string — they never reparse. The invariant is untested in exactly the place it breaks.

In fairness this is semantically benign: ws: None and ws: Some(derived) yield identical websocket() results, and Display is idempotent from its first output onward. But it's structurally unequal, which is what the issue asks for.

On the proposed fix — your instinct was better than the implementation

The issue proposes "use URL-aware serialization," which is the right call and would have closed cases 2, 3, and A in one stroke: self.http.as_str() already emits userinfo, path, query, fragment, and bracketed canonical IPv6, because Url is its own serialiser. #330 instead hand-assembles the components, which is why it needed a bespoke IPv6 helper and still drops userinfo.

A complete fix is roughly two decisions:

1. HTTP side: emit self.http.as_str() rather than rebuilding it. Real cost to weigh: as_str() omits default ports, so https://rpc.example.com:443/... becomes https://rpc.example.com/.... Both reparse equal, but several existing assertions pin the :443 form and would need updating.
2. WS side: emit the ,<scheme>= segment only when self.ws.is_some(). That fixes case B and makes the round-trip genuinely identity.

Both change Display's canonical output, so they're a deliberate format decision rather than a patch — which is easier to justify given the point below.

Severity calibration

Worth stating plainly, since the issue frames all three cases together: SyncEndpointUrl::Display appears to have no production consumer. The connection path uses the typed accessors (peers.rs:54-58 calls url.http() and url.websocket()); the %endpoint tracing field in rpc_sync/client.rs:136 is a &Url, not this type (client.rs:86, :94, :204); and follow_endpoints is #[serde(skip)], so Display isn't used to persist or reload config. Configuration enters exclusively through FromStr, which was never broken.

That makes case 1 a genuine crash fix and cases 2/A/B a latent-trap cleanup — worth doing, since a Display/FromStr pair that loses data will eventually bite whoever first serialises one, but not a live production defect today.

[dumanoglu1]

Thanks for the careful review. I agree with the split here.

The IPv6 case should be struck from the issue: with url 2.5.8, host_str() already preserves brackets for IPv6 hosts, so that part was my mistake.

The two missing cases you pointed out are the same serialization/round-trip class and are worth covering instead:

• HTTP userinfo is accepted by FromStr but dropped by Display.
• An endpoint without an explicit websocket override reparses as ws: Some(derived), so parse -> Display -> parse is not identity for the default/common form even though websocket() resolves to the same URL.

I also checked the narrow behavior locally in an isolated repro pinned to url = 2.5.8: IPv6 display already passes, while the userinfo and derived-websocket round-trip cases reproduce. I could not run the full workspace test here because the Windows build hit sha3-asm requiring perl, but the repro is enough for the SyncEndpointUrl formatting behavior.

So the issue should be read as: port 65535 is the runtime panic case; path/query, userinfo, and derived-vs-explicit websocket are Display/round-trip cleanup cases. The IPv6 claim can be ignored.

[osr21]

Appreciate you re-testing rather than just taking my word for it, and the isolated repro pinned to url = 2.5.8 is the right instinct — that's the cheapest way to settle a claim about a dependency's serialisation behaviour.

Four things worth adding, in rough order of usefulness.

1. Your Windows blocker — the targeted build won't dodge it either

sha3-asm is pulled in transitively by asm-keccak, enabled on alloy-primitives at the workspace root (Cargo.toml:42). Its build script compiles CRYPTOGAMS-style assembly generated by perl scripts, hence the requirement.

The obvious workaround — narrowing to cargo test -p arc-consensus-types — does not help. That crate takes alloy-primitives = { workspace = true }, so it inherits the workspace feature set including asm-keccak; feature unification means you still build keccak-asm → sha3-asm. Worth saying because that's the first thing most people try.

What should work, in increasing order of effort:

• Drop the feature locally. Remove "asm-keccak" from Cargo.toml:42. It's a pure performance feature — same keccak algorithm, same outputs, no API difference — so it cannot affect the correctness of anything you'd be testing. A one-line local edit, not something to commit.
• Install Strawberry Perl and put it on PATH, if you'd rather keep the workspace pristine.
• WSL2, which also matches what CI actually runs.

On which: CI here is Ubuntu-only (ubuntu-24.04 / ubuntu-latest). Windows isn't a supported build target for this repo, so you weren't skipping a check that would otherwise have run — the isolated repro was the pragmatic call and nothing was lost by it. (I can't execute Rust in my environment either, so treat the above as reasoned from the manifests rather than verified end to end.)

2. Cases 1 and B are the same bug, and one change closes both

This is the part I'd most encourage you to fold into the issue. websocket() is:

self.ws.clone().unwrap_or_else(|| { /* derive: set_scheme, checked_add(1).expect("port overflow") */ })

self.ws is read in exactly one place in the entire workspace — that line. Nothing anywhere distinguishes "user supplied a WS override" from "we derived one." The Option carries no information that anyone consumes; it only defers work from parse time to call time.

So: derive eagerly in FromStr and store ws: Url instead of ws: Option<Url>. Consequences:

• The checked_add now runs during parsing, where it can return Err instead of panicking. Case 1 disappears — and without needing a separate validate_derived_ws_port or its has_ws_override flag, because when an override is supplied no derivation happens at all. http://localhost:65535,ws=9000 still parses fine, for free.
• ws is always populated, so Display's unconditional ,<scheme>= segment becomes correct rather than lossy. Case B disappears.
• websocket() becomes an infallible field read, deleting all three expect()s ("port overflow", "valid WebSocket scheme", "valid port") from a public accessor.
• No test churn. I checked the derived-form outputs: http://localhost:8545 still displays as ,ws=8546 and https://example.com still as ,wss=443, because websocket() already returns exactly those values today. display_http_only and display_https_only keep passing unchanged.

That last point is why I'd now prefer this over what I suggested earlier (omitting the segment when ws is None) — same fix for case B, but it preserves the canonical output format instead of rewriting every expectation.

One honest caveat: normalising makes http://x:8545 and http://x:8545,ws=8546 compare equal, where today they're unequal via None != Some(_). I'd argue that's more correct, since they're behaviourally identical — but it is a semantic change to PartialEq. No current test asserts they differ (there's no assert_ne! in the module), so nothing breaks today; it's just a decision to make deliberately rather than discover later.

3. The real ceiling on "Display as a round-trippable format"

Worth naming before anyone invests further, because it bounds how far this can go. The format is comma-delimited and FromStr uses split_once(','). That's only safe because the fields are private, there's no public constructor, and there's no Deserialize impl — so the only values Display ever sees came from a parser that structurally cannot produce a comma in the HTTP part.

Add any one of those three — a constructor, serde support, a with_ws() builder — and the delimiter becomes ambiguous. Userinfo makes this sharper, not weaker: once Display starts emitting credentials, a password containing a comma or = would produce a string that reparses into something different, silently.

So the honest framing is that this is a log/debug format that happens to round-trip for parser-produced values, not a serialisation format. Making it genuinely round-trip-safe means percent-encoding or a different delimiter. Given that Display currently has no production consumer at all, I'd bank cases 2/A/B as cheap correctness hygiene and explicitly decide not to promote it to a real format — rather than half-promoting it and inheriting the escaping obligations.

4. Coordination with #330

Flagging since it's a different author: #330 carries Fixes #323, so merging it as-is would auto-close this issue while cases A and userinfo/derived-WS remain unaddressed — and it would land the host_for_display helper built for the IPv6 case you've now struck.

Might be worth either narrowing that to a partial reference, or noting in the PR that the issue's scope changed after it was opened, so the remaining round-trip cases don't get silently closed out.

[dumanoglu1]

Thanks again. I folded this into the issue body so the current description no longer leaves the wrong IPv6 claim in the main report.

The issue now treats :65535 as the runtime panic case, keeps path/query/fragment as the original serialization loss, and adds the userinfo plus derived-vs-explicit websocket round-trip cases. I also changed the proposed fix toward eager websocket derivation in FromStr, with the PartialEq caveat called out explicitly.

[osr21]

The rewrite reads accurately, and I checked the two claims it introduces that weren't in the earlier discussion — both hold up:

• crates/malachite-app/src/config.rs is the right location: default_rpc_sync_endpoint at :198, returning https://rpc.testnet.arc.io/ (testnet) and http://localhost:8545 (localdev). Neither carries a WS override, so both do sit in the derived-vs-explicit case.
• Neither default is affected by moving the failure into FromStr — both parse cleanly, and default_rpc_sync_endpoint already returns eyre::Result, so there's no new panic surface at the call site.

Three things worth adding before anyone implements this.

The PartialEq caveat is safer than the issue currently states

You wrote that the distinction is unused as a belief ("no workspace consumer appears to use"). I went looking for a counterexample and there isn't one — worth upgrading to a verified claim, since it's the only real risk in the proposal:

• No dedup, HashSet, BTreeSet, retain, position, or contains over rpc_sync_endpoints or PeerRegistry::endpoints anywhere in the workspace. The contains( hits in malachite-app are all &str assertions on error messages in tests.
• SyncEndpointUrl derives Debug, Clone, PartialEq, Eq but not Hash, so it structurally cannot be a HashMap/HashSet key. That closes the whole class of "two endpoints silently collapse into one" concerns.
• peers.rs iterates the list positionally to build its registry rather than comparing entries.

So the equality change has no production blast radius — it's visible only to tests that choose to assert on it. That's a much stronger position than "probably fine."

Round-trip identity does hold for the default-port forms

This is the case I'd expect to break under eager derivation, because url elides default ports — so it's worth confirming ahead of implementation rather than discovering in review. Tracing https://example.com (no explicit port, no override):

• Derived WS is wss://example.com/, no port set.
• Display takes the same-host/no-path branch, emits port_or_known_default() → ,wss=443. HTTP side emits :443 likewise.
• Reparsing https://example.com:443,wss=443: url elides :443 for https, giving https://example.com/; the 443 override is applied via set_port and elided again for wss, giving wss://example.com/.

Both fields land back on exactly the original values, so identity holds. Same for http://localhost → ,ws=80. Eager derivation doesn't disturb the default-port normalisation.

One clarity risk in the current "Proposed fix" section

As written, the section leads with eager derivation and lists five benefits, then mentions URL-aware serialisation only in a conditional closing paragraph. An implementer skimming it could reasonably conclude the first change addresses everything. It doesn't — the four cases split cleanly into two independent fixes:

• Eager derivation closes the :65535 panic and the derived-vs-explicit round trip. Requires no Display change and no test churn.
• Emitting the HTTP side via self.http.as_str() closes path/query/fragment and userinfo. This is the one with real cost: it drops the pinned :443/:8546 forms that several existing assertions depend on, and it's the change that forces the log-format-vs-serialisation-format decision, because emitting credentials is what makes the comma/= delimiter a genuine escaping problem.

Making that split explicit would also let the two be landed independently — the first is close to free, the second needs a deliberate call on what Display is for.

Separately, and only as a coordination note: #330 is still at 2d1885b with Fixes #323, so it currently proposes validate_derived_ws_port — a mechanism the issue's updated fix would make unnecessary — alongside the IPv6 helper for the struck case.

[dumanoglu1]

I updated the issue body again with this split.

The proposed fix section now separates the two concerns:

1. eager websocket derivation in FromStr, which closes the :65535 panic and the derived-vs-explicit structural round-trip case without requiring Display churn; and
2. a separate Display decision for path/query/fragment and userinfo, since treating Display as serialization brings the delimiter/credential escaping question with it.

I also checked the PartialEq risk in the workspace and added the concrete result: SyncEndpointUrl does not derive Hash, I did not find set/dedup/position/contains usage over the endpoint list, and PeerRegistry builds from endpoints positionally. So the equality normalization looks visible to tests/future code, not to a current production consumer.