Skip to content

Conversation

@phrwlk
Copy link
Contributor

@phrwlk phrwlk commented Nov 6, 2025

Previously, ECIESStream::connect_without_timeout() remapped any error from ECIESCodec::new_client() to io::Error::other("invalid handshake"), which collapsed structured ECIESError variants (e.g., Secp256k1) into a generic IO error.

This broke downstream error classification in the networking layer, which matches on ECIESErrorImpl variants to decide on backoff and whether a failure is fatal.

Changes:

  • Propagate the original ECIESError from ECIESCodec::new_client() to preserve semantics.
  • Add a regression test that passes an invalid remote_id and asserts the error’s inner
    variant is ECIESErrorImpl::Secp256k1(_), not IO.

This aligns client error handling with server-side incoming() and ensures consistent, actionable error semantics for callers.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is better

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Nov 6, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 6, 2025

CodSpeed Performance Report

Merging #19558 will improve performances by 10.87%

Comparing phrwlk:fix/ecies-propagate-handshake-error (fe6d1ea) with main (7997cd4)

Summary

⚡ 1 improvement
✅ 76 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
hash builder[init size 10000 | update size 100 | num updates 5] 45.3 ms 40.9 ms +10.87%

@mattsse mattsse added this pull request to the merge queue Nov 6, 2025
@mattsse mattsse added C-debt A clean up/refactor of existing code A-networking Related to networking in general labels Nov 6, 2025
Merged via the queue into paradigmxyz:main with commit cb78b9d Nov 6, 2025
42 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker Nov 6, 2025
yongkangc pushed a commit that referenced this pull request Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-networking Related to networking in general C-debt A clean up/refactor of existing code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants