Skip to content

fix(l1): drop peers on undecodable RLPx inbound frames - #7091

Open
NikhilSharmaWe wants to merge 2 commits into
lambdaclass:mainfrom
NikhilSharmaWe:fix/l1-rlpx-inbound-decode-drop
Open

fix(l1): drop peers on undecodable RLPx inbound frames#7091
NikhilSharmaWe wants to merge 2 commits into
lambdaclass:mainfrom
NikhilSharmaWe:fix/l1-rlpx-inbound-decode-drop

Conversation

@NikhilSharmaWe

Copy link
Copy Markdown

Closes #7035

Summary

A peer that sends an undecodable RLPx message kills the inbound Framed stream, but we only logged and skipped the error. The connection actor stayed alive, so peer selection kept handing that peer out until ping timeout (~30s), and every request burned a full reply timeout.

This change:

  • notifies the connection actor on inbound Framed/codec Err via InboundStreamFailed (removes the fake "Skipping invalid data" path)
  • stops the actor immediately with ProtocolError (decode/frame/crypto) or NetworkError (IO), so remove_peer runs and the peer is no longer selectable
  • sends a wire Disconnect on ProtocolError when outbound may still work

Test plan

  • cargo test -p ethrex-p2p --lib inbound_
  • cargo clippy -p ethrex-p2p --all-targets -- -D warnings

@NikhilSharmaWe
NikhilSharmaWe requested a review from a team as a code owner August 3, 2026 10:31
@github-actions github-actions Bot added the external-contributor PR opened by a contributor outside the team label Aug 3, 2026
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

The PR terminates established RLPx connections as soon as their inbound framed stream fails, preventing unusable peers from remaining selectable until ping timeout.

  • Adds an actor message carrying the inbound failure reason and diagnostic detail.
  • Classifies malformed frames, cryptographic failures, invalid lengths, and RLP decode failures as protocol errors; socket I/O failures become network errors.
  • Stops the connection actor, removes the peer through normal teardown, and attempts a wire Disconnect for protocol errors.
  • Adds classification and peer-removal regression tests.

Confidence Score: 5/5

The PR appears safe to merge, with inbound stream failures routed through prompt connection teardown and peer removal.

The changed path covers every error variant emitted by the established RLPx codec, sends the actor a terminal failure notification, and uses the existing stopped lifecycle to remove the unusable peer.

Important Files Changed

Filename Overview
crates/networking/p2p/rlpx/connection/server.rs Adds terminal handling for established inbound stream failures, maps codec errors to disconnect reasons, and verifies prompt peer removal with focused tests.

Reviews (1): Last reviewed commit: "fix(l1): drop peers on undecodable RLPx ..." | Re-trigger Greptile

async fn started(&mut self, ctx: &Context<Self>) {
// Unit tests may start the actor already in `Established` to exercise teardown
// without a full RLPx handshake. Production always starts as Initiator/Receiver.
#[cfg(test)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This puts test-only control flow into a production lifecycle hook: under cfg(test) an actor whose state is already Established skips started() entirely — no eth-version setup, none of the initialization below.

So the tests for this fix drive an actor that was never started the way a real one is. For a change about when the connection actor stops, that's the part you most want the tests to be faithful about: handle_inbound_stream_failed calls ctx.stop(), and whether stopping is correct depends on what started() spawned and registered.

The usual alternative is to construct the actor through a real (or in-memory) startup path in the test rather than branching in the hook — a test-only constructor, or a trait/param that lets the test supply a stub stream, so production code has no cfg(test) in it. If the seam has to stay, it's worth a comment saying which tests need it and why, because as written the next person can't tell whether it's load-bearing or leftover scaffolding.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed the cfg(test) early-return in started(). Reason bookkeeping is covered via apply_inbound_stream_failure, and peer removal is covered by a regression that runs a real RLPx handshake before sending InboundStreamFailed.

| PeerConnectionError::CryptographyError(_)
| PeerConnectionError::InvalidMessageLength => DisconnectReason::ProtocolError,
PeerConnectionError::IoError(_) => DisconnectReason::NetworkError,
_ => DisconnectReason::NetworkError,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IoError and the _ arm both produce NetworkError, so the explicit arm is redundant — but the catch-all is the part worth reconsidering.

The direction it defaults matters here. ProtocolError means "the peer misbehaved", and per handle_inbound_stream_failed it's the only reason that sends a wire Disconnect. NetworkError is the benign reading. So a PeerConnectionError variant added later — including a decode- or frame-shaped one, which is exactly the class this function exists to catch — silently classifies as benign and the peer is dropped without being told why.

Dropping the _ and matching exhaustively makes the compiler force that decision at the point a variant is added, which is the same fail-closed argument as the all_datadir_suffixes list on #7080. If some variants genuinely can't reach the inbound path, an explicit arm listing them with unreachable!() or a NetworkError default plus a comment documents the reasoning instead of hiding it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Dropped the redundant IoError arm and the _ catch-all. The match is now exhaustive: decode/frame/crypto failures map to ProtocolError, and everything else (including variants not expected on this path) maps to NetworkError explicitly.

Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
@NikhilSharmaWe
NikhilSharmaWe force-pushed the fix/l1-rlpx-inbound-decode-drop branch from e58cbd9 to cafccb5 Compare August 4, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor PR opened by a contributor outside the team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A peer sending an undecodable RLPx message is neither penalized nor discarded, and keeps getting selected

2 participants