Fix P2P message validation and host parameter#64
Fix P2P message validation and host parameter#64quickquickcode wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
WalkthroughThe pull request adds configurable host binding to the P2P network initialization, updating 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 Tip You can disable poems in the walkthrough.Disable the |
![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
| logger.info("🔄 Sent state sync to new peer") | ||
|
|
||
| network.set_on_peer_connected(on_peer_connected) | ||
| network._on_peer_connected = on_peer_connected |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer a public callback registration method over writing P2PNetwork internals.
Assigning network._on_peer_connected directly fixes the immediate error, but it hard-codes a private field name into main.py. A small public registration method in P2PNetwork would keep this contract centralized and easier to evolve.
Suggested API shape
# minichain/p2p.py
+ def register_on_peer_connected(self, callback):
+ if not callable(callback):
+ raise ValueError("callback must be callable")
+ self._on_peer_connected = callback# main.py
- network._on_peer_connected = on_peer_connected
+ network.register_on_peer_connected(on_peer_connected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` at 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.
| # Allow _peer_addr field added by _listen_to_peer | ||
| required_fields = {"type", "data"} | ||
| if not required_fields.issubset(set(message)): | ||
| return False | ||
| # Reject messages with unexpected fields (except _peer_addr) | ||
| allowed_fields = {"type", "data", "_peer_addr"} | ||
| if not set(message).issubset(allowed_fields): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Keep _peer_addr out of the wire-schema validator.
_peer_addr is local transport metadata, not part of the documented on-the-wire message shape. A tighter fix is to validate the raw {"type", "data"} envelope first, then attach _peer_addr only after it passes.
Suggested fix
def _validate_message(self, message):
if not isinstance(message, dict):
return False
- # Allow _peer_addr field added by _listen_to_peer
- required_fields = {"type", "data"}
- if not required_fields.issubset(set(message)):
- return False
- # Reject messages with unexpected fields (except _peer_addr)
- allowed_fields = {"type", "data", "_peer_addr"}
- if not set(message).issubset(allowed_fields):
+ if set(message) != {"type", "data"}:
return False- if isinstance(data, dict):
- data["_peer_addr"] = addr
-
if not self._validate_message(data):
logger.warning("Network: Invalid message schema from %s", addr)
continue
+
+ data["_peer_addr"] = addr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 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).
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