fix: address CodeRabbit comments for pr #46#54
Merged
Zahnentferner merged 6 commits intoStabilityNexus:mainfrom Mar 20, 2026
Merged
fix: address CodeRabbit comments for pr #46#54Zahnentferner merged 6 commits intoStabilityNexus:mainfrom
Zahnentferner merged 6 commits intoStabilityNexus:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds host-configurable network binding, peer trust and localhost validation during sync, restores pending transactions to the mempool on block rejection, enriches incoming messages with peer address metadata, introduces peer-connected callback invocation, and improves graceful network shutdown behavior. Changes
Sequence DiagramsequenceDiagram
participant Peer as External Peer
participant Net as P2P Network
participant Val as Validator/Policy
participant CB as on_peer_connected Callback
participant Store as Local Node State
Peer->>Net: Establish connection / send sync request (includes addr)
Net->>Net: Extract "_peer_addr" and normalize host
Net->>Val: Check host against TRUSTED_PEERS / LOCALHOST_PEERS
Val->>Store: Are there local accounts present?
alt Local accounts present and peer untrusted/non-local
Val->>Peer: Reject sync request
Peer-->>Net: Sync rejected
else Peer trusted or no local accounts
Val->>Net: Accept sync
Net->>CB: Invoke on_peer_connected callback
CB-->>Net: Callback done (errors handled)
Net->>Store: Accept remote accounts payload (validated)
Net-->>Peer: Sync accepted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 34-35: TRUSTED_PEERS is defined as an empty set so the trust check
using peer_addr in TRUSTED_PEERS will always fail; either populate it from
configuration or document intent. Fix by adding configuration parsing to
populate TRUSTED_PEERS (e.g., read a comma-separated env var like TRUSTED_PEERS
or add an argparse option --trusted-peer/--trusted-peers and split into a set)
and ensure LOCALHOST_PEERS remains separate; update the trust check logic that
references peer_addr to consult the populated TRUSTED_PEERS set. If emptiness is
intentional for a demo, instead add a clear comment above the TRUSTED_PEERS
declaration stating that behavior.
- Around line 94-99: The code's peer_host extracted from peer_addr (used in
is_trusted/is_localhost checks) can include IPv6 brackets (e.g., "[::1]") so
comparisons against TRUSTED_PEERS/LOCALHOST_PEERS fail; update the logic around
peer_addr/peer_host (used where peer_host is derived and in the
is_trusted/is_localhost checks inside the block guarded by chain.state.accounts)
to strip surrounding "[" and "]" if present (e.g., if peer_host.startswith("[")
and peer_host.endswith("]") then peer_host = peer_host[1:-1]) before membership
tests and before logging the rejected peer via logger.warning so bracketed IPv6
peers are recognized as localhost/trusted.
In `@minichain/p2p.py`:
- Around line 162-170: The except block that swallows exceptions during writer
cleanup should log the exception at DEBUG to aid troubleshooting; update the
except Exception: handler around writer.close() / await writer.wait_closed() to
call the module or instance logger (e.g., self.logger.debug(...) or
logging.getLogger(__name__).debug(...)) and include the exception information
and context (reader, writer, or peer identifier) without changing the existing
removal behavior of pair from self._peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b02a162c-3021-492c-8796-749e3df5518a
📒 Files selected for processing (2)
main.pyminichain/p2p.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 98-100: The check rejecting syncs when chain.state.accounts is
truthy blocks bootstrap for nodes pre-funded by run_node(); either defer the
initial funding in run_node() until after peer bootstrap completes, or relax the
sync guard in the peer handling code to allow bootstrapping when the only
existing account is the node's self-funded local wallet; locate the funding call
in run_node() and move it to run after the bootstrap routine, or update the
conditional around logger.warning("🔒 Rejected sync from untrusted peer %s",
peer_addr) to treat the case where chain.state.accounts contains only the local
wallet (or when a --fund flag is present) as bootstrappable so remote seeds
aren’t rejected.
- Line 157: Help text uses the incorrect placeholder "connect <host:port>" —
update the help string to match the CLI format used elsewhere by changing the
placeholder to "connect <host>:<port>" (locate the help/menu string in main.py
where the line contains "connect <host:port>" and replace it so the documented
syntax matches the actual command format).
- Around line 103-108: The payload["accounts"] handling is unsafe: validate that
payload is a dict and that payload.get("accounts") returns a mapping before
iterating; ensure each account value (acc) is a dict before mutating
chain.state.accounts or calling acc.get. In the block using remote_accounts,
first coerce or reject non-dict accounts (skip entries where not isinstance(acc,
dict) or addr not a str), accumulate only valid entries, then update
chain.state.accounts with those valid entries and use logger.info to report
synced accounts and optionally warn about skipped/malformed entries; reference
the variables and methods remote_accounts, payload, chain.state.accounts, addr,
acc, and logger.info when making the changes.
Contributor
There was a problem hiding this comment.
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)
306-311: 🧹 Nitpick | 🔵 TrivialConsider renaming to avoid shadowing the
hostparameter.Line 308 shadows the
hostparameter with a local variable. While not a bug (the original value was used at line 303), this reduces readability.♻️ Suggested refactor for clarity
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 306 - 311, The local variable created from connect_to.rsplit(":", 1) shadows the outer parameter named host; rename that local variable (e.g., to peer_host or remote_host) and update its use in the network.connect_to_peer call so you don't reuse the parameter name — adjust the try block where connect_to is parsed and the await network.connect_to_peer(host, int(peer_port)) invocation accordingly (leave the outer host parameter unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main.py`:
- Around line 306-311: The local variable created from connect_to.rsplit(":", 1)
shadows the outer parameter named host; rename that local variable (e.g., to
peer_host or remote_host) and update its use in the network.connect_to_peer call
so you don't reuse the parameter name — adjust the try block where connect_to is
parsed and the await network.connect_to_peer(host, int(peer_port)) invocation
accordingly (leave the outer host parameter unchanged).
SIDDHANTCOOKIE
changed the title
SIDDHANTCOOKIE
changed the title
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
Contributor
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 (5)
minichain/p2p.py (3)
46-52:⚠️ Potential issue | 🔴 CriticalCritical:
hostparameter missing from method signature.The
start()method referenceshoston lines 50 and 52, but the parameter is not defined in the method signature. This will cause aNameErrorat runtime.The AI summary indicates the signature should be
async def start(self, port: int = 9000, host: str = "0.0.0.0"), but the actual code only hasport.🐛 Proposed fix
- async def start(self, port: int = 9000): + async def start(self, port: int = 9000, host: str = "0.0.0.0"): """Start listening for incoming peer connections on the given port.""" self._port = port self._server = await asyncio.start_server( self._handle_incoming, host, port ) logger.info("Network: Listening on %s:%d", host, port)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/p2p.py` around lines 46 - 52, The start method references an undefined host variable; update the async def start(self, port: int = 9000) signature to include host (e.g., host: str = "0.0.0.0") and adjust the docstring accordingly, then use that host variable when calling asyncio.start_server(self._handle_incoming, host, port) and in logger.info("Network: Listening on %s:%d", host, port) so host is defined and passed consistently in the PeerNetwork.start method.
37-39:⚠️ Potential issue | 🔴 CriticalCritical: Missing
set_on_peer_connected()method.The class initializes
self._on_peer_connected = Nonebut provides no public setter. However,main.pyat line 341 callsnetwork.set_on_peer_connected(on_peer_connected), which will raiseAttributeError.🐛 Proposed fix: Add the setter method
self._on_peer_connected = None self._seen_tx_ids = set() self._seen_block_hashes = set() + + def set_on_peer_connected(self, callback): + """Set a callback to be invoked when a new peer connects.""" + if callback is not None and not callable(callback): + raise ValueError("callback must be callable") + self._on_peer_connected = callback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/p2p.py` around lines 37 - 39, Add a public setter method named set_on_peer_connected in the class that initializes self._on_peer_connected so external code can register the callback; implement set_on_peer_connected(self, callback) to assign the provided callback to self._on_peer_connected and validate (optional) callable-ness, and ensure any internal callsites reference self._on_peer_connected when invoking the callback (e.g., where peers are accepted/connected).
268-273:⚠️ Potential issue | 🔴 CriticalCritical: Peer address injection breaks message validation.
The
_peer_addrkey is injected intodataat lines 268-269, but_validate_message()at line 209 checksset(message) != {"type", "data"}which requires exactly those two keys. After injection, the message has three keys (type,data,_peer_addr), causing all messages to fail validation.🐛 Proposed fix: Validate before injecting, or update the validator
Option 1 (preferred): Validate before injecting the peer address
if isinstance(data, dict): - data["_peer_addr"] = addr - - if not self._validate_message(data): + if not self._validate_message(data): + logger.warning("Network: Invalid message schema from %s", addr) + continue + data["_peer_addr"] = addr + else: logger.warning("Network: Invalid message schema from %s", addr) continueOption 2: Update the validator to allow
_peer_addrdef _validate_message(self, message): if not isinstance(message, dict): return False - if set(message) != {"type", "data"}: + allowed_keys = {"type", "data", "_peer_addr"} + if not set(message).issubset(allowed_keys) or not {"type", "data"}.issubset(set(message)): 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 268 - 273, The peer address is injected into the incoming message (data["_peer_addr"] = addr) before calling _validate_message, but _validate_message expects exactly {"type","data"} so injection causes validation to fail; move the peer-address injection to after the call to self._validate_message(data) in the same message-handling flow (or alternatively update _validate_message to accept an optional "_peer_addr" key by checking set(message) minus {"_peer_addr"} == {"type","data"}), referencing the data dict and the _validate_message function to locate where to change the order or adjust the validator.main.py (2)
346-351: 🧹 Nitpick | 🔵 TrivialVariable shadowing:
hostis reused with different meaning.At line 348,
hostis reassigned to the peer's hostname, buthostis also (intended to be) the local bind address parameter. This creates confusion and could cause subtle bugs if code is reordered. Consider using a distinct variable name:♻️ 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 346 - 351, The code reuses the variable name host when parsing connect_to which shadows the local bind address; update the parsing to use a distinct name (e.g., peer_host) instead of host so the local bind variable isn't overwritten: in the block handling connect_to (where you call network.connect_to_peer) split connect_to into peer_host and peer_port, convert peer_port to int and pass peer_host to network.connect_to_peer, and keep the existing logger.error for the ValueError path unchanged.
303-303:⚠️ Potential issue | 🔴 CriticalCritical:
hostparameter not wired through the call chain.The
--hostCLI argument is parsed at line 377 but never passed torun_node():
- Line 303:
run_nodesignature is missing thehostparameter- Line 343: Uses undefined
hostvariable (await network.start(port=port, host=host))- Line 391:
main()doesn't passargs.hosttorun_node()This will cause a
NameErrorat runtime.🐛 Proposed fix
Line 303 - Add host parameter to signature:
-async def run_node(port: int, connect_to: str | None, fund: int, datadir: str | None): +async def run_node(port: int, connect_to: str | None, fund: int, datadir: str | None, host: str = "127.0.0.1"):Line 391 - Pass host argument:
- asyncio.run(run_node(args.port, args.connect, args.fund, args.datadir)) + asyncio.run(run_node(args.port, args.connect, args.fund, args.datadir, args.host))Also applies to: 341-343, 391-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 303, The run_node function signature currently lacks a host parameter and therefore the host parsed from CLI is never propagated—update run_node to accept a host: str | None parameter, update its internal call to network.start (the await network.start(port=port, host=host) call in run_node) to use that parameter, and update main() where run_node(...) is invoked to pass args.host into run_node; ensure all references to host inside run_node point to the new parameter so no undefined variable is used.
🤖 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`:
- Around line 95-99: The current `restored` counter can be misleading because
`mempool.add_transaction(tx)` may return False for transactions already present
(duplicate), so rename the variable and update the log to reflect intent (e.g.,
`reinserted_or_already_present` or `processed`) and change the logger message
accordingly; update references to `restored` in this block (the loop over
`pending_txs`, calls to `mempool.add_transaction(tx)`, and the `logger.info`
call) so the variable name and the log text clearly indicate you counted
transactions that were successfully reinserted versus those already present, not
strictly "restored".
- Around line 358-359: The variable nonce_counter (the mutable list declared as
nonce_counter = [0] with its comment) is unused and should be removed: delete
the declaration and its explanatory comment from the code to eliminate dead
code; also scan for any intended references in CLI closures or related functions
(e.g., any closure that might have been meant to mutate nonce_counter) and
either implement that usage correctly or update those closures to not rely on
nonce_counter before committing the removal.
---
Outside diff comments:
In `@main.py`:
- Around line 346-351: The code reuses the variable name host when parsing
connect_to which shadows the local bind address; update the parsing to use a
distinct name (e.g., peer_host) instead of host so the local bind variable isn't
overwritten: in the block handling connect_to (where you call
network.connect_to_peer) split connect_to into peer_host and peer_port, convert
peer_port to int and pass peer_host to network.connect_to_peer, and keep the
existing logger.error for the ValueError path unchanged.
- Line 303: The run_node function signature currently lacks a host parameter and
therefore the host parsed from CLI is never propagated—update run_node to accept
a host: str | None parameter, update its internal call to network.start (the
await network.start(port=port, host=host) call in run_node) to use that
parameter, and update main() where run_node(...) is invoked to pass args.host
into run_node; ensure all references to host inside run_node point to the new
parameter so no undefined variable is used.
In `@minichain/p2p.py`:
- Around line 46-52: The start method references an undefined host variable;
update the async def start(self, port: int = 9000) signature to include host
(e.g., host: str = "0.0.0.0") and adjust the docstring accordingly, then use
that host variable when calling asyncio.start_server(self._handle_incoming,
host, port) and in logger.info("Network: Listening on %s:%d", host, port) so
host is defined and passed consistently in the PeerNetwork.start method.
- Around line 37-39: Add a public setter method named set_on_peer_connected in
the class that initializes self._on_peer_connected so external code can register
the callback; implement set_on_peer_connected(self, callback) to assign the
provided callback to self._on_peer_connected and validate (optional)
callable-ness, and ensure any internal callsites reference
self._on_peer_connected when invoking the callback (e.g., where peers are
accepted/connected).
- Around line 268-273: The peer address is injected into the incoming message
(data["_peer_addr"] = addr) before calling _validate_message, but
_validate_message expects exactly {"type","data"} so injection causes validation
to fail; move the peer-address injection to after the call to
self._validate_message(data) in the same message-handling flow (or alternatively
update _validate_message to accept an optional "_peer_addr" key by checking
set(message) minus {"_peer_addr"} == {"type","data"}), referencing the data dict
and the _validate_message function to locate where to change the order or adjust
the validator.
🪄 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: 5f13a175-df58-41dc-ad96-bff7192d8d27
📒 Files selected for processing (2)
main.pyminichain/p2p.py
Zahnentferner
requested changes
Mar 18, 2026
main.py
Outdated
| ║ send <to> <amount> - send coins ║ | ||
| ║ mine - mine a block ║ | ||
| ║ peers - show connected peers ║ | ||
| ║ connect <host>:<port> - connect to a peer ║ |
Contributor
There was a problem hiding this comment.
There is a misalignment here.
…NTCOOKIE/minichain into fix-code-rabbits-comments
Contributor
There was a problem hiding this comment.
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)
303-304:⚠️ Potential issue | 🔴 CriticalFix undefined
hostin startup path (runtime blocker).Line 343 uses
host, butrun_node(...)does not define it, which will raiseNameErrorat runtime. Also, Line 391 does not passargs.host, so--hostis currently ignored.🐛 Proposed fix
-async def run_node(port: int, connect_to: str | None, fund: int, datadir: str | None): +async def run_node(port: int, connect_to: str | None, fund: int, datadir: str | None, host: str = "127.0.0.1"): @@ - await network.start(port=port, host=host) + await network.start(port=port, host=host) @@ - asyncio.run(run_node(args.port, args.connect, args.fund, args.datadir)) + asyncio.run(run_node(args.port, args.connect, args.fund, args.datadir, args.host))Also applies to: 343-343, 391-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 303 - 304, The startup path references an undefined variable host inside run_node, and the CLI caller doesn't pass args.host, so add a host parameter to the run_node signature (e.g., async def run_node(port: int, host: str | None, connect_to: str | None, fund: int, datadir: str | None)) and replace the undefined host usage inside run_node with this parameter; then update the invocation that currently calls run_node(...) (the CLI entry) to pass args.host into the new host parameter so the --host flag is honored (ensure default/None handling matches existing behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main.py`:
- Around line 303-304: The startup path references an undefined variable host
inside run_node, and the CLI caller doesn't pass args.host, so add a host
parameter to the run_node signature (e.g., async def run_node(port: int, host:
str | None, connect_to: str | None, fund: int, datadir: str | None)) and replace
the undefined host usage inside run_node with this parameter; then update the
invocation that currently calls run_node(...) (the CLI entry) to pass args.host
into the new host parameter so the --host flag is honored (ensure default/None
handling matches existing behavior).
Zahnentferner
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
This PR applies the CodeRabbit-requested fixes in
main.pyandminichain/p2p.pywith minimal, targeted changes.Screenshots/Recordings:
Additional Notes:
connectcommand to display success/failure message based onconnect_to_peer(...)return value.P2PNetwork.set_on_peer_connected(callback)network._on_peer_connected = ...to setterP2PNetwork.start(..., host="127.0.0.1")--hostCLI arg throughmain.pyto allow explicit external binding_listen_tasksbefore clearingStreamWriters, awaitwait_closed(), then remove peer tuplesChecklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: N/A
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Summary by CodeRabbit
New Features
--hostCLI option to bind the node to a chosen network interface.Bug Fixes
Improvements