lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Published 20 hours ago • verified publisher icon sigp

Penalize peers that send an invalid rpc request

Open ackintosh opened this issue 9 months ago • 2 comments

Issue Addressed

Since https://github.com/sigp/lighthouse/pull/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 avatar Feb 11 '25 22:02 ackintosh

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

mergify[bot] avatar May 19 '25 05:05 mergify[bot]

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

mergify[bot] avatar May 19 '25 23:05 mergify[bot]

@ackintosh @jxs is this PR still relevant?

jimmygchen avatar Sep 10 '25 12:09 jimmygchen

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

mergify[bot] avatar Sep 10 '25 21:09 mergify[bot]

@jxs @jimmygchen Thanks for your review. ✨

ackintosh avatar Sep 12 '25 13:09 ackintosh

Thanks for the extra review @pawanjay176 @ackintosh I'm not sure why the CLA check wasn't triggered, so it didn't satisfiy mergify condition, might have to find a way to trigger it, or re-open this PR :/

jimmygchen avatar Sep 16 '25 06:09 jimmygchen

This might hack the CLA thing: https://cla-assistant.io/check/sigp/lighthouse?pullRequest=6986

michaelsproul avatar Sep 16 '25 07:09 michaelsproul

Nice, looks like it's there now!

jimmygchen avatar Sep 16 '25 07:09 jimmygchen

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #8061.

You may have to fix your CI before adding the pull request to the queue again. If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again. If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

mergify[bot] avatar Sep 17 '25 01:09 mergify[bot]

@mergify requeue

jimmygchen avatar Sep 17 '25 02:09 jimmygchen

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify[bot] avatar Sep 17 '25 02:09 mergify[bot]

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #8063.

You may have to fix your CI before adding the pull request to the queue again. If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again. If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

mergify[bot] avatar Sep 17 '25 03:09 mergify[bot]

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

ackintosh avatar Sep 28 '25 22:09 ackintosh