[p2p] Use existing Error::InvalidChannel + Assert on Invalid Message Sends
#1158
Conversation
|
|
||
| /// Unpack `msg` and verify the underlying `channel` is registered. | ||
| fn validate_msg<V>(msg: Option<Data>, rate_limits: &HashMap<u32, V>) -> Result<Data, Error> { | ||
| fn validate_outbound_msg<V>( |
There was a problem hiding this comment.
I did a double take and thought this was checking incoming messages were on a valid channel, so figured it would be clearer to rename.
InvalidChannel errorError::InvalidChannel
| None => { // Treat unknown channels as invalid | ||
| debug!(?peer, channel = data.channel, "invalid channel"); | ||
| self.received_messages | ||
| .get_or_create(&metrics::Message::new_invalid(&peer)) |
There was a problem hiding this comment.
I think error should be aligned with the metric name.
| if !rate_limits.contains_key(&data.channel) { | ||
| return Err(Error::InvalidChannel); | ||
| } | ||
| assert!( |
There was a problem hiding this comment.
This indicates there exists some serious logic error somewhere in p2p and we should exit to avoid getting banned/blacklisted by peers.
There was a problem hiding this comment.
Sure that's fine
we should exit to avoid getting banned/blacklisted by peers
It did already short-circuit and not send the message, but I guess assert is fine if you prefer crashing
There was a problem hiding this comment.
We should never actually be able to hit this with the public interface, so I think it is worth asserting.
Error::InvalidChannelError::InvalidChannel + Assert on Invalid Message Sends
There was a problem hiding this comment.
Pull Request Overview
This PR adapts error handling for channel validation by removing the unused UnknownChannel variant and updating outbound messaging functions to assert invalid channel usage.
- Remove the UnknownChannel error variant from both peer modules
- Replace conditional error returns with assertions in outbound message validation
- Update log messages to reflect the unified error type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| p2p/src/authenticated/lookup/actors/peer/mod.rs | Removed the UnknownChannel variant from the Error enum to align with existing errors |
| p2p/src/authenticated/lookup/actors/peer/actor.rs | Updated inbound validation to outbound assertions and modified error logging |
| p2p/src/authenticated/discovery/actors/peer/mod.rs | Removed the UnknownChannel variant from the Error enum to align with existing errors |
| p2p/src/authenticated/discovery/actors/peer/actor.rs | Updated inbound validation to outbound assertions and modified error logging |
Co-authored-by: Copilot <[email protected]>
| if !rate_limits.contains_key(&data.channel) { | ||
| return Err(Error::InvalidChannel); | ||
| } | ||
| assert!( |
There was a problem hiding this comment.
Sure that's fine
we should exit to avoid getting banned/blacklisted by peers
It did already short-circuit and not send the message, but I guess assert is fine if you prefer crashing
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1158 +/- ##
=======================================
Coverage 91.02% 91.02%
=======================================
Files 215 215
Lines 57433 57439 +6
=======================================
+ Hits 52276 52282 +6
Misses 5157 5157
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Related: #1133
We should align the error (which already exists) with the metric name.