Fix P2P message validation and host parameter#64
Fix P2P message validation and host parameter#64quickquickcode wants to merge 2 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughThe PR adds configurable host binding for the P2P server, changes how peer-connected callbacks are registered, centralizes peer-connected notification handling, and relaxes message validation/peer-address attachment timing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Line 329: Replace the direct assignment to the private attribute
network._on_peer_connected by adding a public registration method on the
P2PNetwork class (e.g., register_on_peer_connected or
add_peer_connected_handler) that accepts the callback and sets/updates the
internal handler, then call that method from main.py instead of writing to
network._on_peer_connected; modify P2PNetwork to store the handler and invoke it
where peers connect, and update main.py to call the new public method with
on_peer_connected.
In `@minichain/p2p.py`:
- Around line 209-215: The current validator is allowing `_peer_addr` in the
on‑wire schema; instead validate the raw envelope first by checking
required_fields = {"type","data"} against the incoming message (ensure issubset
of set(message)) and reject any extra fields immediately (i.e. ensure
set(message).issubset({"type","data"})) before attaching transport metadata;
remove `_peer_addr` from allowed_fields validation and only add `_peer_addr`
after the message passes validation (the place where `_listen_to_peer` attaches
`_peer_addr` should perform that attachment post‑validation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95a741f2-3b3e-41b0-8eba-b6f1edbbfcf3
📒 Files selected for processing (2)
main.pyminichain/p2p.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
334-339:⚠️ Potential issue | 🟡 MinorVariable shadowing:
hostis reassigned from bind address to peer address.Line 336 reuses the
hostvariable (originally the bind address) to store the peer's host fromconnect_to. While this doesn't cause a bug in the current code flow, it's confusing and error-prone for future maintenance. Consider using a distinct name likepeer_host.🔧 Proposed fix
if connect_to: try: - host, peer_port = connect_to.rsplit(":", 1) - await network.connect_to_peer(host, int(peer_port)) + peer_host, peer_port = connect_to.rsplit(":", 1) + await network.connect_to_peer(peer_host, int(peer_port)) except ValueError: logger.error("Invalid --connect format. Use host:port")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 334 - 339, The code reuses the variable name host (originally the bind address) when parsing connect_to, which can be confusing; change the parsing to use a distinct name (e.g., peer_host) so you do: peer_host, peer_port = connect_to.rsplit(":", 1) and then call await network.connect_to_peer(peer_host, int(peer_port)) (keep the existing try/except ValueError unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/p2p.py`:
- Around line 51-56: Add an explicit return type annotation to the helper method
_notify_peer_connected: change its signature to declare it returns None (i.e.,
async def _notify_peer_connected(self, writer, error_message) -> None). This
satisfies type checkers and clarifies intent while leaving the existing await
self._on_peer_connected(writer) and exception logging logic unchanged.
- Around line 213-217: Replace the two bidirectional subset checks with a single
equality check: ensure the validator uses required_fields = {"type", "data"} and
then return False if set(message) != required_fields; update the code around the
current checks (the variables required_fields and message in this scope) so the
logic is a single equality comparison and continues to exclude _peer_addr from
the wire-schema validation.
---
Outside diff comments:
In `@main.py`:
- Around line 334-339: The code reuses the variable name host (originally the
bind address) when parsing connect_to, which can be confusing; change the
parsing to use a distinct name (e.g., peer_host) so you do: peer_host, peer_port
= connect_to.rsplit(":", 1) and then call await
network.connect_to_peer(peer_host, int(peer_port)) (keep the existing try/except
ValueError unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 616067d3-7364-47f6-88b9-1337ce311b32
📒 Files selected for processing (2)
main.pyminichain/p2p.py
| async def _notify_peer_connected(self, writer, error_message): | ||
| if self._on_peer_connected: | ||
| try: | ||
| await self._on_peer_connected(writer) | ||
| except Exception: | ||
| logger.exception(error_message) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding return type annotation.
The helper centralizes callback invocation nicely. Adding -> None would satisfy type checkers and improve clarity.
🔧 Proposed fix
- async def _notify_peer_connected(self, writer, error_message):
+ async def _notify_peer_connected(self, writer, error_message) -> None:🧰 Tools
🪛 Ruff (0.15.6)
[warning] 51-51: Missing return type annotation for private function _notify_peer_connected
Add return type annotation: None
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 51 - 56, Add an explicit return type
annotation to the helper method _notify_peer_connected: change its signature to
declare it returns None (i.e., async def _notify_peer_connected(self, writer,
error_message) -> None). This satisfies type checkers and clarifies intent while
leaving the existing await self._on_peer_connected(writer) and exception logging
logic unchanged.
| required_fields = {"type", "data"} | ||
| if not required_fields.issubset(set(message)): | ||
| return False | ||
| if not set(message).issubset(required_fields): | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying to a single equality check.
The bidirectional subset checks (required_fields.issubset(set(message)) AND set(message).issubset(required_fields)) are logically equivalent to set(message) == required_fields. A single equality check would be more direct.
This properly addresses the past review feedback to keep _peer_addr out of the wire-schema validator.
🔧 Proposed simplification
def _validate_message(self, message):
if not isinstance(message, dict):
return False
- required_fields = {"type", "data"}
- if not required_fields.issubset(set(message)):
- return False
- if not set(message).issubset(required_fields):
+ if set(message) != {"type", "data"}:
return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 213 - 217, Replace the two bidirectional
subset checks with a single equality check: ensure the validator uses
required_fields = {"type", "data"} and then return False if set(message) !=
required_fields; update the code around the current checks (the variables
required_fields and message in this scope) so the logic is a single equality
comparison and continues to exclude _peer_addr from the wire-schema validation.
1 participant
Problem
When starting a second MiniChain node and connecting to the first node, the following issues occurred:
'P2PNetwork' object has no attribute 'set_on_peer_connected'run_nodefunction_validate_messagemethod strictly checked for{"type", "data"}fields, but_listen_to_peeradds_peer_addrfield, causing validation to failSolution
hostparameter torun_nodefunction inmain.pynetwork.set_on_peer_connected()tonetwork._on_peer_connected = on_peer_connectedP2PNetwork.start()method to accepthostparameter_validate_messageto allow_peer_addrfield in addition totypeanddataTesting
Changes
main.py: Fixed function signature and method callminichain/p2p.py: Added host parameter and fixed message validation logicSummary by CodeRabbit
New Features
Improvements