Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces two new P2P message types, EchoPing and EchoPong, to support future ping-pong functionality for measuring network latency between MPC nodes. The changes add these message variants to the Packet enum and update the packet handling logic to recognize (but currently ignore) them.
Changes:
- Added
EchoPing(u64)andEchoPong(u64)message variants to thePacketenum - Updated packet handling to recognize these new message types alongside the existing
Pingmessage - Added a comment explaining that these messages are recognized but not yet acted upon pending a future PR
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Packet::Ping => { | ||
| Packet::Ping | Packet::EchoPing(_) | Packet::EchoPong(_) => { | ||
| // Do nothing. Pings are just for TCP keepalive. | ||
| // EchoPing/EchoPong are recognized but ignored until https://github.com/near/mpc/pull/1687 is merged. |
There was a problem hiding this comment.
The comment references PR #1687, but the PR description says this fixes #1781. Please verify the correct issue/PR number is referenced in the comment.
| // EchoPing/EchoPong are recognized but ignored until https://github.com/near/mpc/pull/1687 is merged. | |
| // EchoPing/EchoPong are recognized but currently ignored until dedicated handling is implemented. |
| .context("Failed to deserialize packet")?; | ||
| match packet { | ||
| Packet::Ping => { | ||
| Packet::Ping | Packet::EchoPing(_) | Packet::EchoPong(_) => { |
There was a problem hiding this comment.
I think we should respond to ping messages in this one, otherwise new nodes won't get the expected pong messages and they will close the connection.
|
Closing this as discussed here. Instead, we’ll simply avoid sending pings to nodes that don’t support the new protocol version, which we can determine during the handshake. |
Fixes #1781