Fix 7 critical bugs preventing node from running correctly#32
Closed
VincenzoImp wants to merge 6 commits intoStabilityNexus:mainfrom
Closed
Fix 7 critical bugs preventing node from running correctly#32VincenzoImp wants to merge 6 commits intoStabilityNexus:mainfrom
VincenzoImp wants to merge 6 commits intoStabilityNexus:mainfrom
Conversation
1. main.py: Add missing `state` parameter to `_run_node()` signature. The function was called with 6 arguments but only accepted 5, causing TypeError at startup. 2. network/p2p.py: Make `handler_callback` optional in P2PNetwork.__init__(). node_loop() creates P2PNetwork before defining the callback, so passing None caused ValueError. 3. network/p2p.py: Add missing `stop()` method. node_loop() calls `await network.stop()` in its finally block, but the method didn't exist, causing AttributeError on shutdown. 4. core/chain.py: Accept optional `state` parameter in Blockchain.__init__(). node_loop() passes a State instance to Blockchain(), but the constructor didn't accept arguments, causing TypeError. This also fixes the duplicate-State bug where node_loop and Blockchain each had separate State instances, leading to completely desynchronized balances and nonces. 5. core/transaction.py: Fix timestamp double-multiplication on network reconstruction. When a transaction was deserialized from JSON (where timestamp is already in milliseconds), the constructor multiplied it by 1000 again, corrupting the hash and invalidating signatures for all network-received transactions.
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:
WalkthroughThe PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
main.py (1)
138-138: Ruff ARG001 (stateunused) is a valid warning — addressed by the fix proposed in thecore/chain.pycomment above.
stateis accepted but never referenced inside_run_node; every state access goes throughchain.state. The proposed fix there removes this parameter and its callsite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 138, The _run_node function currently accepts an unused parameter state which triggers Ruff ARG001; remove the state parameter from the async def _run_node(...) signature and update every callsite that passes state to instead rely on chain.state (as other code does), ensuring the function definition and all invocations (including any tests or task schedulers that call _run_node) are updated to match the new signature and that no remaining references to the removed parameter remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/transaction.py`:
- Line 15: The constructor currently assigns self.timestamp potentially as a
float, breaking deterministic serialization; update the Transaction constructor
(where self.timestamp is set) to coerce any provided timestamp to an integer
(e.g., use int(...) on the provided timestamp) and when generating a default
timestamp use int(round(time.time() * 1000)) or int(time.time() * 1000) so
hash_payload and signature logic see a consistent integer-millisecond value.
In `@network/p2p.py`:
- Around line 62-64: The logger call in the message handling path uses
logger.warning when no handler is set, which is noisy; change the log level to
logger.debug in the method that checks self.handler_callback (referencing
handler_callback and the message handling block in the P2PNetwork class /
function where the current check if not callable(self.handler_callback) exists)
so that receiving messages with a None callback is logged at DEBUG instead of
WARNING; update the log text if needed but keep the same intent.
---
Duplicate comments:
In `@main.py`:
- Line 138: The _run_node function currently accepts an unused parameter state
which triggers Ruff ARG001; remove the state parameter from the async def
_run_node(...) signature and update every callsite that passes state to instead
rely on chain.state (as other code does), ensuring the function definition and
all invocations (including any tests or task schedulers that call _run_node) are
updated to match the new signature and that no remaining references to the
removed parameter remain.
- Remove unused `state` parameter from `_run_node` (accessed via `chain.state`) - Coerce transaction timestamp to int for deterministic serialization - Downgrade no-handler log from warning to debug (normal during startup)
claim_nonce captured the local `state` variable, but add_block() replaces chain.state with a deep copy on every block commit. After the first block, claim_nonce was reading from the original (now stale) state object instead of the authoritative chain.state. This caused desynchronized nonce tracking in multi-block scenarios.
VincenzoImp
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
83-84: 🧹 Nitpick | 🔵 TrivialRemove the orphaned
statevariable —Blockchain()now creates its own.After
chain = Blockchain(state), the localstatevariable is never read again; all subsequent state access correctly goes throughchain.state. Onceadd_block()replaceschain.statewith a deep copy,statesilently diverges — any future developer who reaches forstateby mistake will introduce exactly the bug this PR just fixed.Since
Blockchain.__init__acceptsstate=Noneand createsState()internally, simplify to:Proposed change
- state = State() - chain = Blockchain(state) + chain = Blockchain()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 83 - 84, Remove the unused local State instance and stop passing it into the Blockchain constructor: delete the orphaned variable "state" and construct the chain with "chain = Blockchain()" since Blockchain.__init__(state=None) already creates its own State; also ensure no other code references the removed local "state" (especially around add_block which replaces chain.state), so update any accidental uses to reference chain.state instead.
♻️ Duplicate comments (1)
network/p2p.py (1)
62-64: Previously flagged log-level issue is now resolved.Guard correctly short-circuits on non-callable callbacks;
logger.debug(rather thanlogger.warning) avoids noise during the transient window betweenP2PNetwork(None)construction and callback assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/p2p.py` around lines 62 - 64, The guard that short-circuits when self.handler_callback is not callable should remain using logger.debug rather than logger.warning to avoid noisy logs during the transient P2PNetwork(None) construction window; in the method that checks handler_callback (look for the callable(self.handler_callback) check in network/p2p.py) keep the early return and the logger.debug("Network: No handler callback set, ignoring message") line as-is so non-callable callbacks are silently ignored at debug level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@network/p2p.py`:
- Around line 22-23: Add an explicit return annotation to the constructor:
change the method signature of __init__(self, handler_callback=None) to include
"-> None" (i.e., __init__(self, handler_callback=None) -> None) so the __init__
method has an explicit None return type; keep the existing handler_callback
parameter as-is.
---
Outside diff comments:
In `@main.py`:
- Around line 83-84: Remove the unused local State instance and stop passing it
into the Blockchain constructor: delete the orphaned variable "state" and
construct the chain with "chain = Blockchain()" since
Blockchain.__init__(state=None) already creates its own State; also ensure no
other code references the removed local "state" (especially around add_block
which replaces chain.state), so update any accidental uses to reference
chain.state instead.
---
Duplicate comments:
In `@network/p2p.py`:
- Around line 62-64: The guard that short-circuits when self.handler_callback is
not callable should remain using logger.debug rather than logger.warning to
avoid noisy logs during the transient P2PNetwork(None) construction window; in
the method that checks handler_callback (look for the
callable(self.handler_callback) check in network/p2p.py) keep the early return
and the logger.debug("Network: No handler callback set, ignoring message") line
as-is so non-callable callbacks are silently ignored at debug level.
Addresses Ruff ANN204 warning flagged in code review.
Author
|
@coderabbitai resolve |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
network/p2p.py (1)
62-64: LGTM —logger.debugcorrectly addresses the past review.The early-return guard gracefully handles the transient state where no callback is wired yet, and the log level no longer produces noise in production or mock mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/p2p.py` around lines 62 - 64, The guard correctly checks if self.handler_callback is callable and logs at debug level, so no code change is required; keep the early-return in the method that contains the callable check (reference: self.handler_callback and logger.debug in network/p2p.py) as-is to handle transient uninitialized state without noisy logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@network/p2p.py`:
- Around line 54-56: Add a return type annotation -> None to the async stop
method to match the project's ANN204 convention applied to __init__ and other
methods; update the method signature async def stop(self): to async def
stop(self) -> None: (also consider adding the same annotation to start() if you
plan to update older methods for consistency) so tools like Ruff recognize the
explicit None return type for stop().
---
Duplicate comments:
In `@network/p2p.py`:
- Around line 62-64: The guard correctly checks if self.handler_callback is
callable and logs at debug level, so no code change is required; keep the
early-return in the method that contains the callable check (reference:
self.handler_callback and logger.debug in network/p2p.py) as-is to handle
transient uninitialized state without noisy logs.
Contributor
✅ Actions performedComments resolved and changes approved. |
Blockchain.__init__ already creates its own State internally when none is provided. The local `state` variable was no longer referenced after the claim_nonce fix (which correctly uses chain.state), so it was just a dangling reference waiting to cause confusion. Also removes the now-unused State import.
Resolve conflicts from upstream PR #19 (flattened directory structure core/, network/, node/, consensus/ → minichain/) while preserving all bug fixes from this branch. Kept State removed from imports (unused after previous fix) and accepted upstream type hints.
VincenzoImp
force-pushed
the
fix/critical-runtime-bugs
branch
from
e00c24a to
7b00276
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 86-88: The guard that returns when self._handler_callback is not
callable makes the later "else" branch unreachable; remove the redundant
conditional and else branch and directly use the callable path: ensure the
initial check either is removed or changed to a warning without return, and
replace the later "if/else" around invoking self._handler_callback with a single
direct invocation (or a simple callable check + invoke) so only one canonical
call-site for self._handler_callback exists; target references:
self._handler_callback and the message-handling block that currently contains
the unreachable else (the two redundant checks noted).
Comment on lines
+86
to
+88
| if not callable(self._handler_callback): | ||
| logger.debug("Network: No handler callback set, ignoring message") | ||
| return |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant check creates unreachable code.
The early guard at lines 86-88 returns if self._handler_callback is not callable. This makes the else branch at lines 118-119 unreachable—if execution reaches line 116, the callback is guaranteed to be callable (and thus truthy).
♻️ Suggested simplification
try:
- if self._handler_callback:
- await self._handler_callback(data)
- else:
- logger.warning("Network Error: No handler_callback registered")
+ await self._handler_callback(data)
except Exception:
logger.exception("Error in network handler callback for data: %s", data)Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 86 - 88, The guard that returns when
self._handler_callback is not callable makes the later "else" branch
unreachable; remove the redundant conditional and else branch and directly use
the callable path: ensure the initial check either is removed or changed to a
warning without return, and replace the later "if/else" around invoking
self._handler_callback with a single direct invocation (or a simple callable
check + invoke) so only one canonical call-site for self._handler_callback
exists; target references: self._handler_callback and the message-handling block
that currently contains the unreachable else (the two redundant checks noted).
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
1 participant
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.
Summary
While reviewing the codebase, I found several critical bugs that prevent the node from starting or cause silent state corruption. The application crashes immediately with
TypeError,ValueError, orAttributeErrorbefore any blockchain logic executes. This PR fixes all of them with minimal, targeted changes.Bugs Fixed
1.
_run_node()parameter mismatch —TypeErrorFile:
main.py_run_nodeis called with 6 arguments (line 133) but defined with only 5 parameters (line 138). Thestateargument is passed but missing from the signature.Fix: Removed the unnecessary
stateargument from the call site since_run_nodecorrectly accesses state throughchain.state.2.
P2PNetwork(None)—ValueErrorFile:
network/p2p.pynode_loop()createsP2PNetwork(None)before defining_handle_network_data, but the constructor validatescallable(handler_callback)and raisesValueError.Fix: Made
handler_callbackoptional (defaultNone), moved validation tohandle_message()with adebug-level log (since this is a normal transient state during startup).3. Missing
stop()method —AttributeErrorFile:
network/p2p.pynode_loop()callsawait network.stop()in itsfinallyblock, butP2PNetworkhas nostop()method.Fix: Added
stop()method stub.4.
Blockchain(state)constructor doesn't accept arguments —TypeErrorFile:
core/chain.pynode_loop()passes aStateinstance toBlockchain(state), but__init__takes no parameters.Fix: Made
statean optional parameter.5. Duplicate State instances — silent state corruption
Files:
main.py+core/chain.pyTwo separate
Stateobjects existed: one innode_loop()and one created insideBlockchain.__init__(). Changes to one didn't affect the other, causing completely desynchronized balances and nonces.Fix: Resolved by bug 4's fix —
Blockchainnow accepts and uses the sharedStateinstance.6. Transaction timestamp corruption — breaks P2P communication
File:
core/transaction.pyWhen reconstructing a transaction from network JSON, the timestamp (already in milliseconds) was multiplied by 1000 again:
round(timestamp * 1000). This changedhash_payload, invalidating the signature for every network-received transaction.Fix: Coerce timestamp to
int(for deterministic serialization) when provided, only generate millisecond timestamp whenNone:int(timestamp) if timestamp is not None else int(time.time() * 1000).7. Stale state reference in
claim_nonce— nonce desynchronizationFile:
main.pyclaim_nonceclosure captured the localstatevariable, butadd_block()replaceschain.statewith a deep copy on every block commit. After the first block,claim_noncewas reading from the original (now stale) state object instead of the authoritativechain.state.Fix: Changed
state.get_account(address)→chain.state.get_account(address)inclaim_nonce.Test Results
After applying all fixes, 10/11 existing tests pass. The single failure (
test_deploy_and_execute) is a pre-existing issue caused by macOSrlimitrestrictions in the contract sandbox — unrelated to these changes.Files Changed
main.pystateparam from_run_node, fixedclaim_nonceto usechain.statenetwork/p2p.pystop()method, validation guard inhandle_message()core/chain.pystateparam inBlockchain.__init__()core/transaction.pyint, no double-multiplicationSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor