Penalize peers that send an invalid rpc request by ackintosh · Pull Request #6986 · sigp/lighthouse

ackintosh · 2025-02-11T22:23:56Z

Issue Addressed

Since #6847, invalid BlocksByRange/BlobsByRange requests, which do not comply with the spec, are handled in the Handler. Any peer that sends an invalid request is penalized and disconnected.

However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains.

Proposed Changes

I have added handling for the ListenUpgradeError event to notify the application of an RPCError:InvalidData error and disconnect to the peer that sent the invalid rpc request.

I also added tests for handling invalid rpc requests.

ackintosh · 2025-02-12T22:31:32Z

beacon_node/lighthouse_network/src/rpc/handler.rs

+            ConnectionEvent::ListenUpgradeError(e) => {
+                if matches!(e.error, RPCError::InvalidData(_)) {
+                    // Peer is not complying with the protocol. Notify the application and disconnect.
+                    let inbound_substream_id = self.current_inbound_substream_id;
+                    self.current_inbound_substream_id.0 += 1;
+
+                    self.events_out.push(HandlerEvent::Err(HandlerErr::Inbound {
+                        id: inbound_substream_id,
+                        proto: Protocol::DataColumnsByRange, // FIXME: replace this hardcoded protocol
+                        error: e.error,
+                    }));
+                    self.shutdown(None);
+                }
+            }


// FIXME: replace this hardcoded protocol

@jxs

The RPCError::InvalidData error is emitted by the inbound codec here. The problem I'm facing is that the handler cannot determine which protocol the invalid data belongs to. I think changing the type of InboundUpgrade::Error from RPCError to (Protocol, RPCError) might solves this. What do you think?

Updated in 2ff8cf2

because that such invalid requests are handled in the Handler since the following PRs: sigp#6847 sigp#6986

mergify · 2025-05-19T05:07:52Z

This pull request has merge conflicts. Could you please resolve them @ackintosh? 🙏

# Conflicts: # beacon_node/lighthouse_network/tests/rpc_tests.rs

mergify · 2025-05-19T23:22:03Z

Some required checks have failed. Could you please take a look @ackintosh? 🙏

jxs

Hi Akihito, overall this looks good to me, sorry for the delay in the review, left some comments

jxs · 2025-05-29T11:04:37Z

beacon_node/lighthouse_network/src/rpc/handler.rs

+            ConnectionEvent::ListenUpgradeError(e) => {
+                if matches!(e.error.1, RPCError::InvalidData(_)) {


you don't need the nested matches Akihito, you can do:

Suggested change

ConnectionEvent::ListenUpgradeError(e) => {

if matches!(e.error.1, RPCError::InvalidData(_)) {

ConnectionEvent::ListenUpgradeError(ListenUpgradeError {

error: (_, RPCError::IoError(_)),

..

}) => {

Refactored in 7e09c47

jxs · 2025-05-29T11:05:37Z

beacon_node/lighthouse_network/src/rpc/protocol.rs

 {
    type Output = InboundOutput<TSocket, E>;
-    type Error = RPCError;
+    type Error = (Protocol, RPCError);


why use a tuple instead of putting the Protocol inside RPCError?

I saw a struct that has Protocol and RPCError as separate fields like this, and I thought it was a reasonable way to represent what the error is and where it happened separately:

lighthouse/beacon_node/lighthouse_network/src/rpc/handler.rs

Lines 938 to 946 in c56c63e

self.events_out.push(HandlerEvent::Err(HandlerErr::Inbound {

id: self.current_inbound_substream_id,

proto: Protocol::BlocksByRange,

error: RPCError::InvalidData(format!(

"requested exceeded limit. allowed: {}, requested: {}",

max_allowed,

request.count()

)),

}));

The receiver might not receive a response due to a race condition: sigp#6986 (comment)

ackintosh · 2025-09-28T22:17:24Z

@jimmygchen Thanks for your feedback! I've updated the tests to remove response validation.

…o-large # Conflicts: # beacon_node/lighthouse_network/tests/rpc_tests.rs

beacon_node/lighthouse_network/src/rpc/handler.rs

…a/SSZDecodeError

beacon_node/lighthouse_network/src/rpc/handler.rs

pawanjay176

Nice!

mergify · 2026-02-10T22:20:30Z

Merge Queue Status

Rule: default

✅ Entered queue — 2026-02-10 22:19 UTC
✅ Checks passed · on draft merge queue: embarking unstable (56eb81a) and #6986 together #8798
✅ Merged — 2026-02-10 22:49 UTC · at f666c7a8609e927dd69a6223b3f402bd1aa95f87

This pull request spent 29 minutes 35 seconds in the queue, including 27 minutes 33 seconds running CI.

Required conditions to merge

check-success=local-testnet-success
check-success=test-suite-success

Since #6847, invalid `BlocksByRange`/`BlobsByRange` requests, which do not comply with the spec, are [handled in the Handler](https://github.com/sigp/lighthouse/blob/3d16d1080f5b93193404967dcb5525fa68840ea0/beacon_node/lighthouse_network/src/rpc/handler.rs#L880-L911). Any peer that sends an invalid request is penalized and disconnected. However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains. I have added handling for the `ListenUpgradeError` event to notify the application of an `RPCError:InvalidData` error and disconnect to the peer that sent the invalid rpc request. I also added tests for handling invalid rpc requests. Co-Authored-By: ackintosh <[email protected]>

ackintosh added 2 commits February 11, 2025 08:37

Add tests for handling invalid requests

d27c6a3

Penalize peers that send an invalid DataColumnsByRange request

3d16d10

ackintosh added the work-in-progress PR is a work-in-progress label Feb 11, 2025

Fix clippy error

c6c306e

ackintosh commented Feb 12, 2025

View reviewed changes

ackintosh changed the title ~~Penalize peers that send an invalid DataColumnsByRange request~~ Penalize peers that send an invalid request Feb 12, 2025

ackintosh changed the title ~~Penalize peers that send an invalid request~~ Penalize peers that send an invalid rpc request Feb 12, 2025

ackintosh added the Networking label Feb 12, 2025

ackintosh added 2 commits February 14, 2025 06:46

Add Protocol to InboundUpgrade::Error

2ff8cf2

Update how the test ends to address race condition issues

525bc13

ackintosh marked this pull request as ready for review February 13, 2025 23:02

ackintosh requested a review from jxs as a code owner February 13, 2025 23:02

ackintosh added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Feb 13, 2025

ackintosh added a commit to ackintosh/lighthouse that referenced this pull request Feb 14, 2025

Remove RPC::is_request_size_too_large

cfea9d2

because that such invalid requests are handled in the Handler since the following PRs: sigp#6847 sigp#6986

ackintosh mentioned this pull request Feb 14, 2025

Delayed RPC Send Using Tokens #5923

Merged

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 19, 2025

ackintosh added 3 commits May 20, 2025 07:36

Merge branch 'unstable' into test-request-too-large

ca6fe1b

# Conflicts: # beacon_node/lighthouse_network/tests/rpc_tests.rs

Fix old field name

e41943e

cargo fmt

ea184d2

Merge branch 'unstable' into test-request-too-large

884b501

jxs reviewed May 29, 2025

View reviewed changes

ackintosh added 3 commits June 17, 2025 22:17

Refactor ListenUpgradeError handling

7e09c47

Merge branch 'unstable' into test-request-too-large

c56c63e

Fix rpc_tests to fit latest updates

86f50b1

ackintosh added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 4, 2025

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 17, 2025

Fix missing tracing logs

cf647a2

jimmygchen removed the ready-for-merge This PR is ready to merge. label Sep 23, 2025

ackintosh added 4 commits September 29, 2025 06:53

Add comments

2aaa90b

Add span to distinguish between sender and receiver

a46fe90

Remove response validation

c93f46d

The receiver might not receive a response due to a race condition: sigp#6986 (comment)

cargo fmt

bd64a25

ackintosh added the ready-for-review The code is ready for review label Oct 9, 2025

Merge remote-tracking branch 'upstream/unstable' into test-request-to…

4cc9aea

…o-large # Conflicts: # beacon_node/lighthouse_network/tests/rpc_tests.rs

dapplion reviewed Feb 10, 2026

View reviewed changes

beacon_node/lighthouse_network/src/rpc/handler.rs Outdated Show resolved Hide resolved

ackintosh added 2 commits February 10, 2026 11:57

Fix compile error to follow the latest updates

2e4e128

Notify application of all RPCError variants; disconnect on InvalidDat…

9f9e9dc

…a/SSZDecodeError

ackintosh force-pushed the test-request-too-large branch from 03b9cb6 to 9f9e9dc Compare February 10, 2026 03:32

pawanjay176 reviewed Feb 10, 2026

View reviewed changes

beacon_node/lighthouse_network/src/rpc/handler.rs Outdated Show resolved Hide resolved

Handler should just report to the application

f666c7a

pawanjay176 approved these changes Feb 10, 2026

View reviewed changes

pawanjay176 added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 10, 2026

mergify bot added the queued label Feb 10, 2026

mergify bot added a commit that referenced this pull request Feb 10, 2026

Merge of #6986

aa99fa0

mergify bot mentioned this pull request Feb 10, 2026

merge queue: embarking unstable (56eb81a) and #6986 together #8798

Closed

6 tasks

mergify bot added a commit that referenced this pull request Feb 10, 2026

Merge of #6986

325131b

mergify bot merged commit e1d3dcc into sigp:unstable Feb 10, 2026
36 checks passed

mergify bot removed the queued label Feb 10, 2026

ackintosh deleted the test-request-too-large branch February 10, 2026 23:07

		ConnectionEvent::ListenUpgradeError(e) => {
		if matches!(e.error.1, RPCError::InvalidData(_)) {

	self.events_out.push(HandlerEvent::Err(HandlerErr::Inbound {
	id: self.current_inbound_substream_id,
	proto: Protocol::BlocksByRange,
	error: RPCError::InvalidData(format!(
	"requested exceeded limit. allowed: {}, requested: {}",
	max_allowed,
	request.count()
	)),
	}));

Conversation

ackintosh commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

ackintosh Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ackintosh Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 19, 2025

Uh oh!

mergify bot commented May 19, 2025

Uh oh!

jxs left a comment

Choose a reason for hiding this comment

Uh oh!

jxs May 29, 2025

Choose a reason for hiding this comment

Uh oh!

ackintosh Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

jxs May 29, 2025

Choose a reason for hiding this comment

Uh oh!

ackintosh Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

ackintosh commented Sep 28, 2025

Uh oh!

Uh oh!

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ackintosh commented Feb 11, 2025 •

edited

Loading

ackintosh Feb 12, 2025 •

edited

Loading

mergify bot commented Feb 10, 2026 •

edited

Loading