Skip to content

Fix P2P message validation and host parameter #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Zahnentferner merged 2 commits into from
Mar 26, 2026
Merged

Fix P2P message validation and host parameter #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
195f09d
Fix P2P message validation and host parameter
quickquickcode Mar 21, 2026
7b238b2
Fix peer callback registration and message envelope validation
quickquickcode Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ async def cli_loop(sk, pk, chain, mempool, network):
# Main entry point
# ──────────────────────────────────────────────

async def run_node(port: int, connect_to: str | None, fund: int, datadir: str | None):
async def run_node(port: int, host: str, connect_to: str | None, fund: int, datadir: str | None):
"""Boot the node, optionally connect to a peer, then enter the CLI."""
sk, pk = create_wallet()

Expand Down Expand Up @@ -326,7 +326,7 @@ async def on_peer_connected(writer):
await writer.drain()
logger.info("🔄 Sent state sync to new peer")

network.set_on_peer_connected(on_peer_connected)
network.register_on_peer_connected(on_peer_connected)

await network.start(port=port, host=host)

Expand Down Expand Up @@ -373,7 +373,7 @@ def main():
)

try:
asyncio.run(run_node(args.port, args.connect, args.fund, args.datadir))
asyncio.run(run_node(args.port, args.host, args.connect, args.fund, args.datadir))
except KeyboardInterrupt:
print("\nNode shut down.")

Expand Down
35 changes: 20 additions & 15 deletions minichain/p2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,19 @@ def register_handler(self, handler_callback):
raise ValueError("handler_callback must be callable")
self._handler_callback = handler_callback

async def start(self, port: int = 9000):
def register_on_peer_connected(self, handler_callback):
if not callable(handler_callback):
raise ValueError("handler_callback must be callable")
self._on_peer_connected = handler_callback

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)
Comment on lines +51 to +56
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


async def start(self, port: int = 9000, host: str = "127.0.0.1"):
"""Start listening for incoming peer connections on the given port."""
self._port = port
self._server = await asyncio.start_server(
Expand Down Expand Up @@ -80,11 +92,7 @@ async def connect_to_peer(self, host: str, port: int) -> bool:
self._listen_to_peer(reader, writer, f"{host}:{port}")
)
self._listen_tasks.append(task)
if self._on_peer_connected:
try:
await self._on_peer_connected(writer)
except Exception:
logger.exception("Network: Error during outbound peer sync")
await self._notify_peer_connected(writer, "Network: Error during outbound peer sync")
logger.info("Network: Connected to peer %s:%d", host, port)
return True
except Exception as exc:
Expand All @@ -103,11 +111,7 @@ async def _handle_incoming(
self._peers.append((reader, writer))
task = asyncio.create_task(self._listen_to_peer(reader, writer, addr))
self._listen_tasks.append(task)
if self._on_peer_connected:
try:
await self._on_peer_connected(writer)
except Exception:
logger.exception("Network: Error during peer sync")
await self._notify_peer_connected(writer, "Network: Error during peer sync")

def _validate_transaction_payload(self, payload):
if not isinstance(payload, dict):
Expand Down Expand Up @@ -206,7 +210,10 @@ def _validate_block_payload(self, payload):
def _validate_message(self, message):
if not isinstance(message, dict):
return False
if set(message) != {"type", "data"}:
required_fields = {"type", "data"}
if not required_fields.issubset(set(message)):
return False
if not set(message).issubset(required_fields):
return False
Comment on lines +213 to 217
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


msg_type = message.get("type")
Expand Down Expand Up @@ -265,9 +272,6 @@ async def _listen_to_peer(
except (json.JSONDecodeError, UnicodeDecodeError):
logger.warning("Network: Malformed message from %s", addr)
continue
if isinstance(data, dict):
data["_peer_addr"] = addr

if not self._validate_message(data):
logger.warning("Network: Invalid message schema from %s", addr)
continue
Expand All @@ -278,6 +282,7 @@ async def _listen_to_peer(
logger.info("Network: Duplicate %s ignored from %s", msg_type, addr)
continue
self._mark_seen(msg_type, payload)
data["_peer_addr"] = addr

if self._handler_callback:
try:
Expand Down
Loading