fix(l1): explicitly filter discv4 and discv5 packets in multiplexer

[azteca1998]

Summary

• Updates route_to_discv5() in the discovery multiplexer to silently drop packets that fail decoding with identification errors (InvalidProtocol, InvalidHeader, InvalidSize, CipherError)
• Unrecognized packets are now logged at trace! level instead of producing noisy debug! errors
• Other discv5 decoding errors continue to be logged at debug! level

Motivation

The previous logic routed every non-discv4 packet to discv5 unconditionally, and logged all decoding failures as debug errors. Any stray UDP traffic, malformed packets, or discv5 packets encrypted for a different node ID would cause a continuous stream of Failed to decode discv5 packet error=InvalidHeader debug logs (as observed when running alongside Lighthouse).

Changes

The fix explicitly handles common identification errors by:

1. Catching InvalidProtocol, InvalidHeader, InvalidSize, and CipherError separately
2. Logging these as trace-level "Dropping unrecognized UDP packet" messages
3. Letting other decoding errors continue to be logged at debug level (these indicate actual issues with valid discv5 packets)

This reduces log noise from stray UDP traffic while preserving visibility into genuine discv5 protocol issues.

Test plan

• Run node alongside Lighthouse and verify InvalidHeader spam is gone
• Verify discv5 peer discovery still works normally

[github-actions]

🤖 Kimi Code Review

The changes look correct and improve the multiplexer's robustness by properly discriminating discovery protocols. A few observations:

Code Correctness

• The is_discv5_packet function in messages.rs correctly implements the discv5 static header decryption using the destination node ID (first 16 bytes as AES-128 key) and validates the protocol ID/version magic bytes.
• The slice ranges data[..IV_MASKING_SIZE] and data[IV_MASKING_SIZE..STATIC_HEADER_END] assume MIN_PACKET_SIZE >= STATIC_HEADER_END, which should hold given discv5 spec constants (16 byte IV + static header).

Security Considerations

• Timing side channels: The function has early returns on different failure paths (size check, cipher init, decryption failure). While this could theoretically leak packet structure via timing, this is acceptable for a packet classification function that only determines routing (not authentication).
• DoS resilience: Every non-discv4 UDP packet now triggers AES-128-CTR decryption. This is unavoidable for correct multiplexing, but consider that attackers can force decryption operations on arbitrary packets. The size bounds check mitigates allocation-based DoS.

Suggestions

1. Add inline hint (optimization):
   In messages.rs, consider marking is_discv5_packet as #[inline] since it's called for every incoming UDP packet that isn't discv4:

   #[inline]
   pub fn is_discv5_packet(dest_id: &H256, data: &[u8]) -> bool {

2. Verify constant relationship (if not already ensured):
   Ensure STATIC_HEADER_END == IV_MASKING_SIZE + STATIC_HEADER_SIZE to prevent the try_into array conversion from failing at compile time or causing runtime panics.

3. Trace level appropriateness (nit):
   In multiplexer.rs:176, the trace! for dropped packets is appropriate, though consider if this might be too noisy during a DoS attack against the discovery port. trace! is fine since it's filtered by default.

Overall: The PR correctly fixes the previous issue where non-discovery packets were incorrectly routed to the discv5 server. The fail-closed approach (drop unrecognized packets) is the correct security posture.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

1. multiplexer.rs:173: is_discv5_packet() now runs before checking whether discv5 is actually enabled or even has a live handler. That means every non-discv4 UDP datagram still does AES-CTR setup and header unmasking from untrusted input, even when discv5 is disabled or unavailable. Before this PR, those cases returned early in route_to_discv5() without crypto work. I’d gate the discriminator with self.config.discv5_enabled && self.discv5_handle.is_some() to avoid unnecessary attack-surface/CPU cost on the shared UDP port.

2. messages.rs:111: this changes routing semantics from “all non-discv4 packets go through discv5 decode” to “drop unless the masked static header decrypts cleanly with local_node_id”. That looks correct, but it is a subtle protocol boundary and there are no local tests covering it. Add targeted tests for valid Ordinary, WhoAreYou, and Handshake packets encoded for the local node ID, plus random / wrong-destination packets, so future header-format changes do not silently blackhole discovery traffic.

Aside from those points, the discriminator itself is consistent with Packet::decode() and I did not spot a direct correctness or consensus/security bug in the diff.

Verification was source inspection only; cargo check -p ethrex-p2p could not run in this sandbox because rustup was unable to write under ~/.rustup.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

Lines of code report

Total lines added: 8
Total lines removed: 0
Total lines changed: 8

Detailed view

+-------------------------------------------------------+-------+------+
| File                                                  | Lines | Diff |
+-------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/multiplexer.rs | 184   | +8   |
+-------------------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

Now I have all the context needed for a thorough review. Here is my assessment:

---

Review: fix(p2p): explicitly filter discv4 and discv5 packets in multiplexer

Overall: The fix is correct and addresses a real operational issue. The implementation is clean, the logic is sound. A few minor observations below.

---

crates/networking/p2p/discv5/messages.rs — is_discv5_packet

Correctness — looks good. The decryption logic mirrors what PacketHeader::decode does for the static header: same cipher type (Aes128Ctr64BE), same key (dest_id[..16]), same IV (data[..IV_MASKING_SIZE]), same 23-byte slice. The protocol-ID and version checks at lines 129–130 match the layout documented in PacketHeader::encode (PROTOCOL_ID || PROTOCOL_VERSION || ...). No correctness issue.

Cipher initialization style inconsistency (minor).
Packet::decode (line 83) uses new() (infallible given the known-length slices), while is_discv5_packet (line 116–119) uses new_from_slices(), which returns Result. Both &dest_id[..16] (16 bytes from H256) and masking_iv (16 bytes, guarded by the size check) are always the correct lengths for Aes128Ctr64BE, so the Err arm at line 118–119 is unreachable in practice. Not a bug, but slightly misleads the reader. Matching the existing style (new(...).into()) or adding a comment clarifying why the error arm exists would improve readability.

Second unreachable error arm (trivial).

// lines 121–124
let Ok(mut static_header): Result<[u8; STATIC_HEADER_SIZE], _> =
    data[IV_MASKING_SIZE..STATIC_HEADER_END].try_into()
else {
    return false;
};

STATIC_HEADER_END = 39, MIN_PACKET_SIZE = 63, so the bounds check on line 112 already guarantees this slice is exactly 23 bytes. The else branch is dead code. Again, not wrong, but consider replacing with an expect/direct cast to signal intent.

try_apply_keystream cannot fail here (minor).
StreamCipher::try_apply_keystream fails only if the CTR counter would overflow — which requires processing more than 2⁶⁴ blocks. Decrypting 23 bytes will never trigger this. The error branch (line 127) is unreachable. For the same reason Packet::decode uses cipher.try_apply_keystream(...) and maps the error — this is just the API's shape. The pattern is defensively correct if verbose.

Double-decryption per legitimate discv5 packet.
When is_discv5_packet returns true, the multiplexer calls route_to_discv5, which eventually calls Packet::decode — redoing the AES-CTR decryption from scratch. This is a minor redundancy (23 bytes of AES-CTR twice), not a problem in practice given the low packet rate. Worth noting if this function is ever called in a tight loop.

---

crates/networking/p2p/discovery/multiplexer.rs — route_packet

The change is clean. The if/else-if/else structure correctly expresses exclusive routing with a drop-and-log fallback:

// line 171–177
if is_discv4_packet(data) {
    self.route_to_discv4(data, from).await;
} else if is_discv5_packet(&self.local_node_id, data) {
    self.route_to_discv5(data, from).await;
} else {
    trace!(from=?from, "Dropping unrecognized UDP packet");
}

Discv4 checked before discv5. Since discv4 uses keccak hash verification (a pretty strong discriminator) and discv5 uses decrypted protocol ID, the ordering is reasonable. A packet can't legitimately be both, but if an adversarially crafted packet somehow passed both checks, discv4 wins — this is fine.

trace!(from=?from, ...) — trivial nit.
SocketAddr implements Display, so %from (Display) would emit a cleaner "1.2.3.4:30303" instead of the Debug form. The Debug output is also readable, so this is purely stylistic.

---

Summary

The PR correctly solves the InvalidHeader log spam by adding a lightweight positive-identification check before routing. No correctness, safety, or security issues. The main suggestions are:

1. The new_from_slices error branch and the try_into error branch in is_discv5_packet are both unreachable — consider aligning with the Packet::decode style or adding comments to explain the defensive pattern.
2. Consider %from over ?from in the trace log (cosmetic).

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Kimi Code Review

File: crates/networking/p2p/discv5/messages.rs

Issue 1: Potential panic due to insufficient length validation (Line 113-122)
The function assumes MIN_PACKET_SIZE >= STATIC_HEADER_END, but this isn't verified locally. If constants are misconfigured, slice indexing will panic.

Suggestion: Add an explicit guard or static assertion:

pub fn is_discv5_packet(dest_id: &H256, data: &[u8]) -> bool {
    // Ensure MIN_PACKET_SIZE is sufficient for header decryption
    const_assert!(MIN_PACKET_SIZE >= STATIC_HEADER_END);
    
    if data.len() < STATIC_HEADER_END || data.len() > MAX_PACKET_SIZE {
        return false;
    }
    // ... rest of function
}

Issue 2: Performance/DoS vector (Line 112)
This function performs AES-CTR decryption on every UDP packet that isn't valid discv4. In a high-traffic scenario with random garbage packets, this creates a CPU exhaustion vector.

Suggestion: Document the cryptographic cost in the function docstring:

/// Returns true if the data looks like a valid discv5 packet destined for `dest_id`.
/// 
/// Note: This performs AES-128-CTR decryption and should be considered expensive.
/// Callers should ensure this is only invoked when necessary.

Issue 3: Magic numbers (Line 124-125)
Offsets 6 and 7 correspond to protocol version location in the static header, but use hardcoded indices.

Suggestion: Use named constants:

const PROTOCOL_ID_SIZE: usize = 6;
const VERSION_OFFSET: usize = PROTOCOL_ID_SIZE; // 6
static_header[..PROTOCOL_ID_SIZE] == PROTOCOL_ID
    && u16::from_be_bytes([static_header[VERSION_OFFSET], static_header[VERSION_OFFSET + 1]]) == PROTOCOL_VERSION

Issue 4: Non-constant-time comparison (Line 124)
While likely acceptable for a multiplexer (not a cryptographic oracle), the protocol ID comparison uses standard equality which may be vulnerable to timing attacks if this were used for authentication. For defense-in-depth, consider using subtle::ConstantTimeEq if this check becomes security-critical elsewhere.

---

File: crates/networking/p2p/discovery/multiplexer.rs

Acknowledgment: The logic change from if/else to if/else if/else (Lines 173-178) correctly prevents garbage packets from being routed to the discv5 handler. The addition of the trace! log (Line 179) improves observability for debugging dropped packets.

Minor: Ensure self.local_node_id is the correct H256 type expected by is_discv5_packet (first 16 bytes used as AES key). The usage appears correct assuming standard discv5 node ID formatting.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[greptile-apps]

Greptile Summary

This PR fixes noisy InvalidHeader debug-log spam that occurred when running alongside Lighthouse by adding explicit positive identification for discv5 packets before routing them. Previously, any non-discv4 UDP datagram was forwarded unconditionally to the discv5 handler, which produced a continuous stream of decode errors for stray traffic or discv5 packets encrypted for a different node.

• Adds is_discv5_packet(dest_id, data) in discv5/messages.rs: creates a fresh AES-128-CTR cipher from dest_id[..16] + the packet's masking-IV, decrypts a copy of the 23-byte static header, and checks the protocol ID (b"discv5") and version (0x0001). The original data slice is untouched, so the subsequent Packet::decode call in route_to_discv5 can re-derive its own cipher and decode normally.
• Updates route_packet in multiplexer.rs to require a positive match for both protocols; packets matching neither are silently dropped with a trace!-level log instead of being forwarded to discv5 where they'd trigger debug-level error messages.
• The approach is robust: since the static header is AES-CTR-encrypted with the destination's node ID, only packets genuinely addressed to this node will pass the check, making false positives cryptographically negligible.

Confidence Score: 5/5

• Safe to merge — the fix is targeted, correct, and eliminates production log noise without any risk of dropping legitimate packets.
• The implementation is correct: is_discv5_packet decrypts a copy of the static header (not in-place on the original slice), so Packet::decode in the hot path still receives the original untouched bytes and creates its own fresh cipher from scratch. Bounds safety is guaranteed by the MIN_PACKET_SIZE (63) > STATIC_HEADER_END (39) invariant. The new function is a strict subset of what Packet::decode already does, so it cannot introduce new failure modes. The only minor observation is that valid discv5 packets now decrypt the 23-byte static header twice (once in the filter, once in full decode), but this overhead is negligible for UDP discovery traffic.
• No files require special attention.

Important Files Changed

Filename	Overview
crates/networking/p2p/discv5/messages.rs	Adds is_discv5_packet() that positively identifies a discv5 packet by creating a fresh AES-128-CTR cipher from dest_id[..16] + masking-IV, decrypting a copy of the static header, and verifying protocol ID (b"discv5") and version (0x0001). Logic is correct: the function operates on a copy of the bytes so the original data slice is unmodified, allowing Packet::decode to re-decrypt from scratch using its own fresh cipher. Bounds are safe (MIN_PACKET_SIZE=63 > STATIC_HEADER_END=39).
crates/networking/p2p/discovery/multiplexer.rs	Updates route_packet to require positive identification for both protocols before routing, falling back to a trace!-level drop for unrecognized packets. The fix eliminates the noisy InvalidHeader debug spam caused by forwarding arbitrary UDP traffic to discv5. Import updated to include the new is_discv5_packet function.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[UDP Packet Received] --> B{is_discv4_packet?\nkeccak256 hash check}
    B -- Yes --> C[route_to_discv4\nDiscv4Packet::decode]
    B -- No --> D{is_discv5_packet?\nAES-CTR decrypt static header\ncheck protocol-id + version}
    D -- Yes\nmatches our node ID --> E[route_to_discv5\nDiscv5Packet::decode]
    D -- No\nwrong node / garbage / unknown --> F[trace! drop\nDropping unrecognized UDP packet]
    C --> G{discv4_enabled?}
    G -- No --> H[silent drop]
    G -- Yes --> I[send to Discv4Server handler]
    E --> J{discv5_enabled?}
    J -- No --> K[silent drop]
    J -- Yes --> L[send to Discv5Server handler]

Loading

_{Reviews (1): Last reviewed commit: "style(l1): fix cargo fmt in discv5 messa..." | Re-trigger Greptile}

[github-actions]

🤖 Codex Code Review

One minor performance concern:

1. crates/networking/p2p/discovery/multiplexer.rs:173 together with crates/networking/p2p/discv5/messages.rs:111 now decrypt the discv5 static header twice for every valid discv5 packet: once in is_discv5_packet(), then again in Discv5Packet::decode() at messages.rs:73. This is on the hot UDP receive path, so it adds avoidable per-packet work. A cleaner shape would be to classify by attempting decode once and only forward on Ok(_), or have the helper return reusable parsed header/packet data instead of a boolean.

Aside from that, I did not find a correctness, consensus, or security bug in this patch. The discriminator is using the local node ID as the discv5 masking key, which matches the expected receive-side header unmasking flow.

I could not run cargo test/cargo check here because the sandboxed rustup setup fails before tool invocation due a read-only filesystem error.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Claude Code Review

Here is my review:

---

PR Review: fix(l1): explicitly filter discv4 and discv5 packets in multiplexer

Overall: The fix is correct and well-motivated. The implementation closely mirrors the existing Packet::decode logic for initialization and masking, so the approach is sound.

---

crates/networking/p2p/discv5/messages.rs

Double-decryption safety (correct)

is_discv5_packet correctly copies the static header bytes into a new [u8; STATIC_HEADER_SIZE] via try_into() before decrypting in-place. The original data slice is untouched, so the subsequent call to Discv5Packet::decode in route_to_discv5 will re-decrypt properly from the raw bytes. No double-decryption bug here.

Unreachable error path for new_from_slices (minor)

// line 116
let Ok(mut cipher) = <Aes128Ctr64BE as KeyIvInit>::new_from_slices(&dest_id[..16], masking_iv)
else {
    return false;
};

new_from_slices fails only if the key or IV slice has the wrong length. The key is always &dest_id[..16] (16 bytes) and masking_iv is always &data[..IV_MASKING_SIZE] (16 bytes, guaranteed by the size guard above). This branch is unreachable in practice. Defensive handling is acceptable, but a comment noting this would be clearer. Alternatively, new_from_slices(...).unwrap() with a comment would be more honest than silently treating an impossible condition as "not a discv5 packet."

Magic number 6 (minor, pre-existing style)

// line 128
&static_header[..6] == PROTOCOL_ID

The 6 is a magic number that happens to equal PROTOCOL_ID.len(). The same pattern exists in PacketHeader::decode at line 155, so this is consistent with existing style — not a regression introduced by this PR. Still worth noting: using PROTOCOL_ID.len() would be more robust and self-documenting.

Code duplication (minor)

The cipher setup and static-header extraction in is_discv5_packet (lines 115–127) are near-identical to the first 15 lines of Packet::decode. A private helper fn decrypt_static_header(dest_id: &H256, data: &[u8]) -> Option<[u8; STATIC_HEADER_SIZE]> could serve both call sites. Not critical for a targeted fix, but worth a follow-up to avoid drift.

---

crates/networking/p2p/discovery/multiplexer.rs

Crypto work when discv5_enabled = false (minor)

// line 173
} else if is_discv5_packet(&self.local_node_id, data) {
    self.route_to_discv5(data, from).await;

route_to_discv5 already guards with if !self.config.discv5_enabled { return; }, but is_discv5_packet is still called unconditionally, performing AES decryption on every non-discv4 packet even when the discv5 handler is disabled. Under load this is extra crypto work for no benefit. Consider short-circuiting:

} else if self.config.discv5_enabled && is_discv5_packet(&self.local_node_id, data) {
    self.route_to_discv5(data, from).await;

Trace log field syntax (nit)

trace!(from=?from, "Dropping unrecognized UDP packet");

from=?from repeats the variable name. tracing supports the shorthand %from (Display) or ?from (Debug). Since SocketAddr implements Display, %from is idiomatic here:

trace!(%from, "Dropping unrecognized UDP packet");

---

Security

No security regressions. An adversary can craft a payload that passes is_discv5_packet (by getting the AES-CTR decryption of the first 8 bytes of their payload under the target node's key to equal b"discv5\x00\x01"), but all this achieves is having Discv5Packet::decode called, which will then fail gracefully — the same downstream behavior as before.

---

Summary

The core logic is correct and the fix effectively eliminates the InvalidHeader spam without impacting legitimate packet routing. The items above are all minor; the most actionable ones are the discv5_enabled short-circuit guard and the PROTOCOL_ID.len() style improvement.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[iovoid]

Why not catch InvalidProtocol and the 'header too small' error instead of decrypting twice?