Modular Core Implementation & Smart Contracts and contract tests#6
Merged
Zahnentferner merged 18 commits intoStabilityNexus:mainfrom Feb 21, 2026
Merged
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:
WalkthroughAdds core account-based blockchain primitives (Block, Transaction, State, Blockchain), a ContractMachine, Proof-of-Work mining utilities, a thread-safe mempool, a minimal P2P abstraction, a demo node entrypoint and packaging, plus unit tests. Several packages re-export their public symbols. Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet as Wallet
participant Mempool as Mempool
participant Network as P2P_Network
participant Miner as Node/Miner
participant State as State
participant Contracts as ContractMachine
Wallet->>Mempool: sign(tx) & add_transaction(tx)
Mempool->>Mempool: verify signature & dedupe
Mempool->>Network: (optional) broadcast tx
Network-->>Miner: deliver tx
Miner->>Mempool: get_transactions_for_block()
Mempool-->>Miner: return txs (and clear)
Miner->>Miner: mine_block(txs) (PoW)
Miner->>State: apply_transaction(tx) for each tx
alt Deploy
State->>State: derive_contract_address()
State->>Contracts: execute(deploy code)
Contracts-->>State: commit storage / return status
else Call
State->>Contracts: execute(contract call)
Contracts-->>State: commit or rollback
else Transfer
State->>State: update balances & nonces
end
State-->>Miner: state updated
Miner->>Network: broadcast block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
aniket866
changed the title
![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: 30
🤖 Fix all issues with AI agents
In `@consensus/__init__.py`:
- Around line 1-3: The package public API currently re-exports mine_block and
calculate_hash but omits MiningExceededError, preventing callers from catching
it via the package; update the module that defines __all__ to import
MiningExceededError from consensus.pow (alongside mine_block and calculate_hash)
and add "MiningExceededError" to the __all__ list so consumers can do from
consensus import MiningExceededError and catch exceptions raised by mine_block.
In `@consensus/pow.py`:
- Around line 47-68: The mining loop in mine_block currently has no default
upper bound so when max_nonce is None it can run indefinitely; update mine_block
to enforce a sensible default limit or add an optional timeout/cancellation
parameter and honor it inside the loop. Specifically, set a default max_nonce
(or add a timeout_seconds or cancel_token parameter) and check it each iteration
alongside the existing max_nonce check, raise MiningExceededError when the
default limit or timeout is reached, and ensure progress_callback can signal
cancellation (e.g., return False) and that mine_block checks that signal to
break the loop and log via logger.info/warning where appropriate; keep the
checks near the existing block.nonce/max_nonce logic and use the same
MiningExceededError/logger symbols.
- Around line 44-45: Replace the eager f-string logging in the pow mining
messages with lazy %-style formatting: update the logger.info call that logs
"Mining block {block.index} (Difficulty: {difficulty})" and the later
logger.info on line 65 to use format strings with % and pass block.index and
difficulty (or the corresponding variables) as arguments to logger.info so the
message is constructed only if the log level is enabled; locate these calls via
the logger variable and the mining-related context in consensus/pow.py (e.g.,
where block.index and difficulty are referenced).
In `@core/__init__.py`:
- Line 2: The package __init__ imports a non-existent symbol Blockchain from
core.chain which causes ImportError; update core/__init__.py to import the
actual exported class State (or re-export it under the name Blockchain) by
replacing the incorrect import of Blockchain with the correct symbol State (or
add an alias like State as Blockchain) so that the module imports match the
class defined in core.chain.
- Around line 1-7: The package exports a conflicting State: both chain.py and
state.py define State while __init__.py imports State from state.py but expects
Blockchain from chain.py; consolidate by keeping account/contract logic in
state.py (class State) and moving/removing any duplicate State implementation
from chain.py, implement or restore a dedicated Blockchain class in chain.py
that handles chain management and block validation (class Blockchain), and
update core/__init__.py imports so it explicitly imports Blockchain from
chain.py and State from state.py (preserve ContractMachine/Block/Transaction
exports). Ensure any references in other modules use the single State and the
new Blockchain symbols.
In `@core/chain.py`:
- Around line 64-74: The deployment block derives the contract address using
tx.nonce while the account nonce has already been incremented, which is
confusing and error-prone; update the logic to consistently derive the address
from the post-incremented account nonce (use sender['nonce'] - 1 when calling
derive_contract_address) or else use tx.nonce everywhere, and add a short inline
comment in the same block referencing derive_contract_address, sender['nonce'],
and tx.nonce to document why the chosen nonce value is used.
- Around line 7-130: There are duplicate State class definitions across
core/chain.py and core/state.py causing divergence and maintenance issues;
consolidate by keeping a single canonical State class (retain methods like
get_account, verify_transaction_logic, apply_transaction,
derive_contract_address, create_contract, update_contract_storage,
credit_mining_reward) in core/state.py and remove the duplicate in
core/chain.py, while moving blockchain-specific logic into a new Blockchain
class in core/chain.py that uses State (implement copy() and
validate_and_apply() either on State or as methods on Blockchain as
appropriate), ensure the deployment rollback behavior present in chain.py (the
collision-nondeployment rollback in apply_transaction) is preserved in the
canonical implementation, and update imports/usages to reference the single
State and the new Blockchain class.
- Around line 76-100: The contract-call branch is entered for every transaction
because Transaction.__init__ normalizes None → "" (self.data = data or ""), so
change Transaction.__init__ to preserve None (set self.data = data) so the
existing checks like the gate in core/chain.py (the if tx.data is not None
block) and the similar check in core/state.py work correctly; after making this
change, scan uses of tx.data (e.g., contract_machine.execute call sites) and add
explicit None-safety where necessary, or alternatively if you prefer the other
approach change the gate in core/chain.py and core/state.py to a truthiness
check (if tx.data) to treat empty-string as no-data.
- Around line 7-8: core/chain.py currently only defines class State while
core/__init__.py expects a Blockchain export, causing ImportError; also there is
a duplicate State in core/state.py and a faulty transaction check because
Transaction.__init__ normalizes None to "" making the condition if tx.data is
not None always true. Fix by either renaming or re-exporting: if core/chain.py
is meant to provide the chain manager, rename class State to Blockchain (or add
a Blockchain wrapper that composes State) so core/__init__.py can import it;
remove or consolidate the duplicate State implementation in core/state.py to a
single source of truth; and change the transaction branch check in the function
that inspects tx.data (the conditional at line ~77) to test for non-empty data
(e.g., if tx.data != "" or if bool(tx.data)) so plain transfers don’t get
treated as contract calls.
In `@core/contract.py`:
- Around line 37-44: The exec call in execute_contract (using exec(code, {},
context)) leaves full builtins accessible so contract code can escape the
sandbox; fix by creating a restricted globals mapping and pass that to exec:
construct a safe_builtins dict containing only safe functions/constants (e.g.,
True, False, None, range, len, min, max, abs, etc.), set globals_for_exec =
{'__builtins__': safe_builtins}, merge or attach any needed names, then call
exec(code, globals_for_exec, context) and still read storage from context;
update the exec site (the exec invocation and the surrounding block that calls
self.state.update_contract_storage(contract_address, context['storage'])) to use
this restricted globals mapping to prevent access to
import/open/eval/system-level functions.
- Around line 27-35: The contract currently receives context['storage'] as the
same reference as the account's storage, so in-place mutations can corrupt the
real storage on exceptions; to fix, pass a shallow (or deep if nested structures
are used) copy of the account storage into the context before invoking the
contract (e.g., set context['storage'] = storage.copy() or a deep copy) and only
call update_contract_storage with the modified copy after the contract completes
successfully; update references to context['storage'], the contract invocation
point, and update_contract_storage to ensure the original account storage isn't
mutated on partial failure.
In `@core/state.py`:
- Around line 22-35: Replace the print() calls in verify_transaction_logic with
logging.error using %-style lazy formatting: import logging at the top if not
present, then change the three prints to logging.error("Error: Invalid signature
for tx from %s...", tx.sender[:8]), logging.error("Error: Insufficient balance
for %s...", tx.sender[:8]), and logging.error("Error: Invalid nonce. Expected
%s, got %s", sender_acc['nonce'], tx.nonce). Keep the surrounding logic in
verify_transaction_logic, reference tx.verify(), get_account(), and sender_acc
when making the replacements.
- Around line 56-65: The contract deployment branch in the function handling
transactions fails to roll back the sender's deducted balance and nonce when a
deployment collision is detected; when checking existing =
self.accounts.get(contract_address) and finding existing.get("code"), restore
the sender's state (increment sender's nonce and refund the deducted balance on
the sender account) before returning False — mirror the rollback logic used in
core/chain.py; update the branch that calls derive_contract_address and
create_contract so any early return on collision performs the balance and nonce
restoration for tx.sender and tx.nonce.
In `@core/transaction.py`:
- Around line 44-52: The verify() method currently only catches
BadSignatureError, so constructing VerifyKey(self.sender, encoder=HexEncoder)
can raise nacl.exceptions.CryptoError or ValueError for malformed/invalid hex
and bubble up; update verify() (the method that calls VerifyKey and
verify_key.verify) to also catch CryptoError and ValueError (alongside
BadSignatureError) and return False when those occur, ensuring any malformed
sender or invalid key material yields False instead of an exception.
In `@main.py`:
- Around line 155-160: The block-add branch for mined_block_2 calls
chain.add_block(mined_block_2) but never checks the boolean result of
state.apply_transaction for each transaction; update the loop over
mined_block_2.transactions to mirror the pattern used for block 1 by capturing
the return value of state.apply_transaction(tx), checking it (e.g., if not
success then print/log an error mentioning mined_block_2.index and the failing
tx), and take the same failure action (exit or mark block as invalid) as was
done for block 1 so contract-call failures are surfaced.
- Around line 74-79: The current use of raw Python in the contract_code string
(used/executed by ContractMachine) allows arbitrary execution and sandbox
escapes; update the codebase and PR to (1) remove or clearly label any example
that execs contract_code as unsafe, (2) add a prominent comment/doc entry near
the contract_code definition and in the README warning that exec is unsafe for
non-toy use, and (3) replace the ad-hoc exec approach before production by
wiring ContractMachine to a safer execution strategy (e.g., RestrictedPython, a
WASM-based VM, or a small domain-specific interpreter) and add a TODO
referencing ContractMachine so maintainers know where to implement the safer
runtime.
- Around line 37-38: Replace the print call in async def
handle_network_data(data) with a logger call using the logging module: get or
create a module-level logger (e.g., logger = logging.getLogger(__name__)) and
call logger.info or logger.debug inside handle_network_data using %-style lazy
formatting (e.g., logger.info("Received: %s", data)) instead of f-strings;
ensure the module configures logging (or relies on app-level config) so messages
are emitted.
- Around line 114-119: The loop handling mined_block_1.transactions (and the
similar loop for mined_block_2) only treats a string return from
state.apply_transaction(tx) as significant and silently ignores failures; update
both places to capture the return value into result, check explicitly for a
falsy failure (e.g., result is False or result is None) and log an error or
warning including the tx identifier (tx.hash or tx.id) and a short context
string, while preserving the existing contract-deployment path that treats a
string as contract_address; ensure you log any non-string, non-success results
too so rejected transactions are visible to operators.
- Around line 26-35: The pending_nonce_map can diverge from on-chain nonce after
block application or rejected txs; update get_next_nonce/increment_nonce logic
to re-sync pending_nonce_map from state.get_account(address)['nonce'] whenever a
block is applied or when state.apply_transaction returns False (or on
transaction rejection), and also reset or adjust pending_nonce_map entries after
successful state.apply_block/state.apply_transaction calls; specifically, ensure
code paths around state.apply_transaction/state.apply_block refresh
pending_nonce_map[address] (or delete it to force lazy re-read) so
get_next_nonce always reflects the on-chain nonce after block/tx outcomes.
- Line 140: tx_call is added to the mempool with
mempool.add_transaction(tx_call) but never broadcast to peers; after
mempool.add_transaction(tx_call) call the same network broadcast function used
earlier in the file (the one invoked for the previous two transactions) to
propagate tx_call to peers (e.g., broadcast_transaction(tx_call) or
peers.broadcast(tx_call) — use the exact broadcaster function already present in
this module) so the transaction is announced to other nodes.
- Around line 59-64: Transaction.__init__ currently normalizes data=None to an
empty string which causes State.apply_transaction (which checks "if tx.data is
not None") to misclassify plain transfers as contract calls; to fix, update
Transaction.__init__ in core/transaction.py to preserve None (i.e., set
self.data = data instead of self.data = data or "") so tx.data remains None for
regular transfers (or, if you prefer the alternate fix, change
State.apply_transaction to use a truthy check like "if tx.data:" to only treat
non-empty data as contract calls). Ensure you update the constructor for the
Transaction class (Transaction.__init__) and/or the contract-call condition in
State.apply_transaction accordingly so normal transfers are not routed into the
contract-call branch.
In `@network/p2p.py`:
- Around line 55-73: The current try/except around both parsing/validation and
the handler call masks application errors from handler_callback; refactor by
isolating message parsing/validation (the checks for msg.data, decoding,
json.loads, and structure validation that produce `data`) inside a try/except
that logs/parses network-related errors, then invoke await
self.handler_callback(data) outside that except block so any exceptions from
handler_callback propagate (or are handled separately) instead of being printed
as a generic "Network Error"; use the same symbols msg, decoded, data, and
handler_callback to locate and adjust the code.
- Around line 28-45: Replace print() calls in broadcast_transaction and
broadcast_block with the logging module using a module-level logger (e.g.,
logger = logging.getLogger(__name__)) and use %-style lazy formatting for
messages; for example call logger.info("Network: Broadcasting Tx from %s...",
tx.sender[:5]) and logger.info("Network: Broadcasting Block #%d", block.index),
and replace the mock-mode prints with logger.debug or logger.warning (e.g.,
logger.debug("Network: pubsub not initialized (mock mode)")) while keeping the
existing pubsub publish behavior in those methods.
In `@node/mempool.py`:
- Around line 38-49: The current get_transactions_for_block method clears
self.seen_tx_ids unconditionally, which allows previously-seen (and potentially
already-included) transactions to be re-added to the mempool; change the logic
so seen_tx_ids is not wiped each block—either persist seen_tx_ids across block
boundaries or remove only the IDs of transactions that were actually included:
after collecting txs = self.pending_txs[:], clear self.pending_txs but update
self.seen_tx_ids by discarding the IDs present in txs (or simply leave
seen_tx_ids intact), and if you intend state-level replay protection instead,
add a clear comment in get_transactions_for_block describing that design choice
and why seen_tx_ids is reset.
In `@requirements.txt`:
- Around line 1-2: setup.py currently declares the dependency string "py-libp2p"
which does not match the actual PyPI package name used in requirements.txt
("libp2p"); update the dependency declaration in setup.py to use "libp2p"
(replace "py-libp2p" with "libp2p") so that pip install . and pip install -r
requirements.txt resolve the same package/version (e.g., libp2p==0.5.0).
In `@tests/test_contract.py`:
- Around line 1-9: core/__init__.py is importing "Blockchain" from .chain but
chain.py doesn't define that name, causing ImportError and preventing tests
(which import State and Transaction) from running; edit core/__init__.py to
remove or correct the invalid import: either import the actual symbol(s) defined
in chain.py (replace "Blockchain" with the correct class name) or stop importing
chain at module import time and only export State and Transaction (or perform a
lazy import inside functions) so that "from core import State, Transaction"
succeeds without raising ImportError.
- Around line 59-66: The test_call_non_existent_contract relies on
Transaction.verify() only validating the sender and not the receiver, so using
the literal "nonexistent_address" is misleading; update the test to pass a
receiver value that has a valid public-key hex format (e.g., a hex string
matching the expected length/format) instead of the plain word, by changing the
Transaction instantiation in test_call_non_existent_contract to use a
syntactically valid receiver key while keeping the address value non-existent so
apply_transaction still fails; refer to Transaction.verify, Transaction, and
State.apply_transaction when making this change.
- Around line 92-112: The test_redeploy_same_address currently deploys with
sequential nonces so derive_contract_address(sender, nonce) produces different
addresses; change the test to force an actual address collision by computing the
expected contract address for the second deployment (call
derive_contract_address(self.pk, next_nonce) or derive_contract_address(self.pk,
0) as appropriate) and pre-populating the state's contract storage/registry at
that address (so that self.state.apply_transaction(tx2) tries to deploy to an
already-occupied address), then assert that apply_transaction(tx2) returns
False; reference test_redeploy_same_address, derive_contract_address,
apply_transaction, get_account and the state's contract storage insertion to
locate where to insert the existing contract entry before the second tx.
In `@tests/test_core.py`:
- Around line 6-12: Remove the fragile sys.path manipulation in
tests/test_core.py (delete the
sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
line) and instead rely on proper packaging/test runner configuration (e.g., add
a pyproject.toml/setup.py and run tests with an editable install or configure
pytest's pythonpath) so imports resolve reliably; also remove the unused imports
Blockchain and Block from the test (leave only Transaction and State) or, if the
tests actually need Blockchain, import the correct symbol from its defining
module (e.g., core.chain) and ensure the Blockchain class is implemented there.
- Around line 43-57: The test fails because Transaction.__init__ currently sets
self.data = data or "" so tx.data is an empty string and
State.apply_transaction's check (if tx.data is not None) treats it as a contract
call; fix by changing Transaction.__init__ to accept data=None by default and
assign self.data = data (so absent data stays None), or alternatively adjust
State.apply_transaction to treat empty strings as no-data by checking truthiness
(e.g., if tx.data:) — update the Transaction.__init__ signature and assignment
(Transaction.__init__) or the contract-call branch guard in
State.apply_transaction to use tx.data truthiness instead of is not None.
core/__init__.py
Outdated
Comment on lines
+1
to
+7
| from .block import Block | ||
| from .chain import Blockchain | ||
| from .transaction import Transaction | ||
| from .state import State | ||
| from .contract import ContractMachine | ||
|
|
||
| __all__ = ["Block", "Blockchain", "Transaction", "State", "ContractMachine"] No newline at end of file |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Both chain.py and state.py export State — clarify module responsibilities.
Currently chain.py and state.py both define a State class. This __init__.py imports State from state.py (line 4) while expecting a Blockchain from chain.py (line 2). The intended separation appears to be: chain.py → Blockchain (chain management, block validation), state.py → State (account/contract state). Please consolidate the duplicate State logic and add a proper Blockchain class to chain.py.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 7-7: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
In `@core/__init__.py` around lines 1 - 7, The package exports a conflicting
State: both chain.py and state.py define State while __init__.py imports State
from state.py but expects Blockchain from chain.py; consolidate by keeping
account/contract logic in state.py (class State) and moving/removing any
duplicate State implementation from chain.py, implement or restore a dedicated
Blockchain class in chain.py that handles chain management and block validation
(class Blockchain), and update core/__init__.py imports so it explicitly imports
Blockchain from chain.py and State from state.py (preserve
ContractMachine/Block/Transaction exports). Ensure any references in other
modules use the single State and the new Blockchain symbols.
Comment on lines
+1
to
+9
| import unittest | ||
| import sys | ||
| import os | ||
|
|
||
| sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | ||
|
|
||
| from core import State, Transaction | ||
| from nacl.signing import SigningKey | ||
| from nacl.encoding import HexEncoder |
Contributor
There was a problem hiding this comment.
All contract tests will fail at import time.
from core import State, Transaction (line 7) triggers core/__init__.py, which attempts from .chain import Blockchain — a name that doesn't exist in chain.py. This ImportError prevents any test in this file from running.
This is a downstream effect of the broken import in core/__init__.py (see prior comment).
🤖 Prompt for AI Agents
In `@tests/test_contract.py` around lines 1 - 9, core/__init__.py is importing
"Blockchain" from .chain but chain.py doesn't define that name, causing
ImportError and preventing tests (which import State and Transaction) from
running; edit core/__init__.py to remove or correct the invalid import: either
import the actual symbol(s) defined in chain.py (replace "Blockchain" with the
correct class name) or stop importing chain at module import time and only
export State and Transaction (or perform a lazy import inside functions) so that
"from core import State, Transaction" succeeds without raising ImportError.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated mining logic to include timeout and cancellation options.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@consensus/pow.py`:
- Around line 18-66: The mine_block function currently mutates the input Block
(block.nonce reset to 0 and incremented) even on failure and computes a hash
that may exceed the max_nonce boundary; to fix this, stop mutating the original
Block until success by using a local nonce variable (e.g., local_nonce) and only
assign block.nonce and block.hash when the target is found in mine_block, update
the max_nonce check to use >= against local_nonce to prevent an extra hash
attempt, and move the nonce increment semantics so the loop tests/increments
local_nonce before computing the next hash (adjust references to block.nonce in
progress_callback and logging to use local_nonce or include it in the callback
payload).
- Around line 7-9: The MiningExceededError class contains a docstring so the
trailing "pass" is redundant; remove the unnecessary "pass" statement from the
class definition (MiningExceededError) leaving just the docstring as the class
body to simplify the code.
Replaced print statements with logging for better monitoring and debugging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@main.py`:
- Around line 38-48: The nonce is incremented before mempool validation which
couples recovery to sync_nonce; change the flow so increment_nonce is only
called after mempool.add_transaction returns success: locate the call site that
signs and then currently calls increment_nonce (the caller using get_next_nonce
and increment_nonce) and move the pending_nonce_map update (logic inside
increment_nonce) into the success branch of mempool.add_transaction, leaving
get_next_nonce unchanged and removing the pre-validation increment; ensure
callers no longer pre-increment and that failed mempool.add_transaction paths do
not rely on sync_nonce to revert the pending_nonce_map.
- Around line 124-146: After successfully adding a mined block (inside the
chain.add_block(mined_block_1) true branch), call
state.credit_mining_reward(...) to credit the block miner before/after
processing transactions; locate mined_block_1 to obtain the miner/coinbase
address (e.g., mined_block_1.miner or mined_block_1.coinbase) and pass that to
state.credit_mining_reward(miner_address) so the miner receives reward coins,
then continue with the existing transaction processing and sync_nonce calls;
ensure you use the existing state.credit_mining_reward function name and handle
its return or errors consistently with the surrounding logging.
- Around line 31-36: The else branch in sync_nonce is dead because
state.get_account always returns a dict; remove the unreachable else that pops
pending_nonce_map and simplify the function to always set
pending_nonce_map[address] = account["nonce"] after calling state.get_account;
update the function sync_nonce and any callers if necessary to rely on that
behavior and remove the conditional check around account since account will
never be falsy.
Refactor blockchain management by introducing Blockchain class, handling block creation and state transitions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Refactor contract execution to improve safety and clarity.
Updated the Transaction class to preserve None for data and handle additional exceptions in the verify method.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@consensus/pow.py`:
- Around line 52-54: The loop mutates block.nonce each iteration but the
docstring promises no mutation until success; instead of assigning block.nonce =
local_nonce, build a header dict for hashing without changing the block: call
block.to_header_dict() once (or each iteration) into a local variable, set
header['nonce'] = local_nonce, then pass that header to calculate_hash(header);
keep the block object untouched and only set block.nonce when the mining is
successful (or raise MiningExceededError without leaving a stale nonce).
In `@core/chain.py`:
- Around line 34-57: The add_block method currently skips proof-of-work
verification; before validating transactions (i.e., before creating temp_state
or applying validate_and_apply), check that the block's PoW is valid and meets
the chain's difficulty target. Call the block hash verification routine (e.g., a
helper like Block.compute_hash() or an existing validator such as
self.hash_meets_difficulty(block.hash) / self.get_difficulty()) and if it fails
return False immediately. Insert this check right after the previous_hash
linkage check and before temp_state is created so only blocks with a valid PoW
proceed to transaction validation.
- Around line 44-52: State.copy() and State.validate_and_apply(tx) are missing
and will cause AttributeError when Chain.add_block() calls temp_state =
self.state.copy() and temp_state.validate_and_apply(tx); implement a copy(self)
method on the State class that returns a new State instance with the same
internal data (shallow or deep as appropriate for the state structures) and
implement validate_and_apply(self, tx) to perform transaction validation and
application (or simply call the existing apply_transaction(tx) after
validation), ensuring it returns True on success and False/None on failure so
Chain.add_block() behaves correctly.
In `@core/contract.py`:
- Around line 25-34: Remove the redundant string-key entries "True", "False",
and "None" from safe_builtins and tighten the sandbox to prevent literal
introspection escapes: ensure safe_builtins only exposes the explicitly allowed
callables (range, len, min, max, abs) and does not expose type/object, and add
an AST-level or bytecode-level check (e.g., a new validate_no_dunder_access /
AST NodeVisitor used before exec/eval) that rejects any attribute access whose
name starts with "__" (or any patterns like __class__, __bases__,
__subclasses__) to block constructs such as
().__class__.__bases__[0].__subclasses__(); keep the enforcement tied to the
code path that injects safe_builtins so callers of safe_builtins get the
stricter, minimal environment.
- Around line 18-19: The current defensive copy uses storage =
dict(account.get("storage", {})), which only performs a shallow copy and leaves
nested mutable objects shared; change this to a deep copy by importing deepcopy
(from copy import deepcopy or import copy) and replacing that line with storage
= deepcopy(account.get("storage", {})) so nested dicts/lists are fully copied
and cannot be mutated by failing contract executions; update imports in
core/contract.py accordingly.
In `@core/transaction.py`:
- Around line 9-16: Transaction.__init__ allows negative amounts which lets
State.apply_transaction invert balances; add validation to reject negative
amounts by checking tx.amount in Transaction.__init__ (or alternatively in
State.verify_transaction_logic) and raise an explicit exception (e.g.,
ValueError) when amount < 0; update the constructor (Transaction.__init__) or
verifier (State.verify_transaction_logic) to enforce amount >= 0 before any
state mutation in State.apply_transaction is performed.
In `@network/p2p.py`:
- Around line 31-47: Extract the duplicated hardcoded topic string into a
class-level constant (e.g., TOPIC = "minichain-global") and replace the
hardcoded occurrences in broadcast_transaction and broadcast_block with that
constant (use self.TOPIC or ClassName.TOPIC depending on style), ensuring the
calls to self.pubsub.publish("minichain-global", ...) become
self.pubsub.publish(self.TOPIC, ...) so both methods reference the same single
source of truth.
In `@tests/test_contract.py`:
- Around line 1-5: Remove the sys.path manipulation in tests/test_contract.py
(the
sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
line) and restore normal package imports (e.g., import core) and instead make
the package importable by adding proper project metadata (pyproject.toml or
setup.cfg) and/or instructing contributors to run pip install -e . . Update
CI/test instructions to install the package in editable or packaged form so
tests import core without runtime path hacks.
- Around line 117-129: The test test_balance_and_nonce_updates is cut off
mid-statement (the stray token contract_add_) causing a SyntaxError; remove the
incomplete token and finish the test by completing the deployment flow: after
creating and signing tx_deploy (Transaction, tx_deploy.sign), submit or apply
the deploy to the state (e.g., call the same method used elsewhere to add/apply
a contract and capture the resulting contract_address or receipt), create a
subsequent call transaction (using updated nonce) to exercise balance/nonce
changes, then assert the sender's balance and nonce changed as expected; also
either use initial_balance in the assertions or remove that unused variable.
Ensure references to Transaction, tx_deploy, tx_deploy.sign, and
test_balance_and_nonce_updates locate where to fix.
Comment on lines
+52
to
+54
| # Temporarily set nonce for hashing only | ||
| block.nonce = local_nonce | ||
| block_hash = calculate_hash(block.to_header_dict()) |
Contributor
There was a problem hiding this comment.
Docstring claims no mutation until success, but block.nonce is set every iteration.
Line 25 says "without mutating input block until success," but line 53 sets block.nonce = local_nonce on every loop iteration (needed for to_header_dict()). If MiningExceededError is raised, the block is left with a stale nonce and no hash. Either fix the docstring or build the header dict without mutating the block.
Option A: Fix the docstring
- """Mines a block using Proof-of-Work without mutating input block until success."""
+ """Mines a block using Proof-of-Work. Note: block.nonce is mutated during mining."""Option B: Avoid mutating the block during the loop
- # Temporarily set nonce for hashing only
- block.nonce = local_nonce
- block_hash = calculate_hash(block.to_header_dict())
+ header = block.to_header_dict()
+ header["nonce"] = local_nonce
+ block_hash = calculate_hash(header)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Temporarily set nonce for hashing only | |
| block.nonce = local_nonce | |
| block_hash = calculate_hash(block.to_header_dict()) | |
| # Get header dict and apply nonce without mutating block | |
| header = block.to_header_dict() | |
| header["nonce"] = local_nonce | |
| block_hash = calculate_hash(header) |
🤖 Prompt for AI Agents
In `@consensus/pow.py` around lines 52 - 54, The loop mutates block.nonce each
iteration but the docstring promises no mutation until success; instead of
assigning block.nonce = local_nonce, build a header dict for hashing without
changing the block: call block.to_header_dict() once (or each iteration) into a
local variable, set header['nonce'] = local_nonce, then pass that header to
calculate_hash(header); keep the block object untouched and only set block.nonce
when the mining is successful (or raise MiningExceededError without leaving a
stale nonce).
Comment on lines
+34
to
+57
| def add_block(self, block): | ||
| """ | ||
| Validates and adds a block to the chain if all transactions succeed. | ||
| Uses a copied State to ensure atomic validation. | ||
| """ | ||
|
|
||
| # Check previous hash linkage | ||
| if block.previous_hash != self.last_block.hash: | ||
| return False | ||
|
|
||
| # Validate transactions on a temporary state copy | ||
| temp_state = self.state.copy() | ||
|
|
||
| for tx in block.transactions: | ||
| result = temp_state.validate_and_apply(tx) | ||
|
|
||
| # Reject block if any transaction fails | ||
| if result is False or result is None: | ||
| return False | ||
|
|
||
| # All transactions valid → commit state and append block | ||
| self.state = temp_state | ||
| self.chain.append(block) | ||
| return True |
Contributor
There was a problem hiding this comment.
Atomic validation is a good pattern, but the block's PoW hash is not verified.
add_block checks previous_hash linkage and validates transactions atomically — good. However, it does not verify that block.hash actually satisfies the difficulty target (i.e., the PoW). Any peer could submit a block with an arbitrary hash and it would be accepted as long as transactions are valid and linkage matches.
🛡️ Proposed fix — add PoW verification
def add_block(self, block):
+ # Verify PoW
+ from consensus.pow import calculate_hash
+ computed_hash = calculate_hash(block.to_header_dict())
+ if computed_hash != block.hash:
+ return False
+ if block.difficulty and not block.hash.startswith("0" * block.difficulty):
+ return False
+
# Check previous hash linkage
if block.previous_hash != self.last_block.hash:
return False🤖 Prompt for AI Agents
In `@core/chain.py` around lines 34 - 57, The add_block method currently skips
proof-of-work verification; before validating transactions (i.e., before
creating temp_state or applying validate_and_apply), check that the block's PoW
is valid and meets the chain's difficulty target. Call the block hash
verification routine (e.g., a helper like Block.compute_hash() or an existing
validator such as self.hash_meets_difficulty(block.hash) /
self.get_difficulty()) and if it fails return False immediately. Insert this
check right after the previous_hash linkage check and before temp_state is
created so only blocks with a valid PoW proceed to transaction validation.
Comment on lines
+18
to
+19
| # Defensive copy of storage to prevent direct mutation | ||
| storage = dict(account.get("storage", {})) |
Contributor
There was a problem hiding this comment.
Shallow copy doesn't protect nested mutable values in storage.
dict(...) only copies the top-level keys. If a prior execution stored nested dicts/lists (e.g., storage['data'] = {'key': 'val'}), those inner objects are still shared references. A failing contract can mutate them in-place before raising, bypassing the rollback intent.
🐛 Proposed fix — use deep copy
+import copy
+
class ContractMachine:- storage = dict(account.get("storage", {}))
+ storage = copy.deepcopy(account.get("storage", {}))🤖 Prompt for AI Agents
In `@core/contract.py` around lines 18 - 19, The current defensive copy uses
storage = dict(account.get("storage", {})), which only performs a shallow copy
and leaves nested mutable objects shared; change this to a deep copy by
importing deepcopy (from copy import deepcopy or import copy) and replacing that
line with storage = deepcopy(account.get("storage", {})) so nested dicts/lists
are fully copied and cannot be mutated by failing contract executions; update
imports in core/contract.py accordingly.
Comment on lines
+25
to
+34
| safe_builtins = { | ||
| "True": True, | ||
| "False": False, | ||
| "None": None, | ||
| "range": range, | ||
| "len": len, | ||
| "min": min, | ||
| "max": max, | ||
| "abs": abs, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Safe builtins allowlist still permits certain bypasses.
True, False, None as string keys in the builtins dict are redundant — they're language keywords, not builtins lookups, in Python 3. More importantly, contracts can still access type, object, __class__ via literal introspection (e.g., ().__class__.__bases__[0].__subclasses__()) to escape. The warning on line 4 covers this, but consider also blocking attribute access on dunders if the project evolves beyond educational use.
🤖 Prompt for AI Agents
In `@core/contract.py` around lines 25 - 34, Remove the redundant string-key
entries "True", "False", and "None" from safe_builtins and tighten the sandbox
to prevent literal introspection escapes: ensure safe_builtins only exposes the
explicitly allowed callables (range, len, min, max, abs) and does not expose
type/object, and add an AST-level or bytecode-level check (e.g., a new
validate_no_dunder_access / AST NodeVisitor used before exec/eval) that rejects
any attribute access whose name starts with "__" (or any patterns like
__class__, __bases__, __subclasses__) to block constructs such as
().__class__.__bases__[0].__subclasses__(); keep the enforcement tied to the
code path that injects safe_builtins so callers of safe_builtins get the
stricter, minimal environment.
Comment on lines
+9
to
+16
| def __init__(self, sender, receiver, amount, nonce, data=None, signature=None): | ||
| self.sender = sender # Public key (Hex str) | ||
| self.receiver = receiver # Public key (Hex str) or None for Deploy | ||
| self.amount = amount | ||
| self.nonce = nonce | ||
| self.data = data # Preserve None (do NOT normalize to "") | ||
| self.timestamp = time.time() | ||
| self.signature = signature # Hex str |
Contributor
There was a problem hiding this comment.
No validation that amount is non-negative — allows balance theft.
A transaction with a negative amount would cause sender['balance'] -= tx.amount to increase the sender's balance and receiver['balance'] += tx.amount to decrease the receiver's balance in State.apply_transaction. This is exploitable by any signer.
🐛 Proposed fix — validate in constructor or in State
Option A — in Transaction constructor:
def __init__(self, sender, receiver, amount, nonce, data=None, signature=None):
+ if amount < 0:
+ raise ValueError("Transaction amount cannot be negative")
self.sender = senderOption B — in State.verify_transaction_logic (core/state.py):
def verify_transaction_logic(self, tx):
+ if tx.amount < 0:
+ print(f"Error: Negative amount in tx from {tx.sender[:8]}...")
+ return False
if not tx.verify():🧰 Tools
🪛 Ruff (0.15.0)
[warning] 9-9: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
🤖 Prompt for AI Agents
In `@core/transaction.py` around lines 9 - 16, Transaction.__init__ allows
negative amounts which lets State.apply_transaction invert balances; add
validation to reject negative amounts by checking tx.amount in
Transaction.__init__ (or alternatively in State.verify_transaction_logic) and
raise an explicit exception (e.g., ValueError) when amount < 0; update the
constructor (Transaction.__init__) or verifier (State.verify_transaction_logic)
to enforce amount >= 0 before any state mutation in State.apply_transaction is
performed.
Comment on lines
+31
to
+47
| async def broadcast_transaction(self, tx): | ||
| msg = json.dumps({"type": "tx", "data": tx.to_dict()}) | ||
| logger.info("Network: Broadcasting Tx from %s...", tx.sender[:5]) | ||
|
|
||
| if self.pubsub: | ||
| await self.pubsub.publish("minichain-global", msg.encode()) | ||
| else: | ||
| logger.debug("Network: pubsub not initialized (mock mode)") | ||
|
|
||
| async def broadcast_block(self, block): | ||
| msg = json.dumps({"type": "block", "data": block.to_dict()}) | ||
| logger.info("Network: Broadcasting Block #%d", block.index) | ||
|
|
||
| if self.pubsub: | ||
| await self.pubsub.publish("minichain-global", msg.encode()) | ||
| else: | ||
| logger.debug("Network: pubsub not initialized (mock mode)") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the hardcoded topic string into a class-level constant.
"minichain-global" is duplicated on lines 36 and 45. A single constant avoids drift if the topic name changes.
♻️ Proposed refactor
class P2PNetwork:
+ TOPIC = "minichain-global"
+
...
async def broadcast_transaction(self, tx):
...
if self.pubsub:
- await self.pubsub.publish("minichain-global", msg.encode())
+ await self.pubsub.publish(self.TOPIC, msg.encode())
...
async def broadcast_block(self, block):
...
if self.pubsub:
- await self.pubsub.publish("minichain-global", msg.encode())
+ await self.pubsub.publish(self.TOPIC, msg.encode())🤖 Prompt for AI Agents
In `@network/p2p.py` around lines 31 - 47, Extract the duplicated hardcoded topic
string into a class-level constant (e.g., TOPIC = "minichain-global") and
replace the hardcoded occurrences in broadcast_transaction and broadcast_block
with that constant (use self.TOPIC or ClassName.TOPIC depending on style),
ensuring the calls to self.pubsub.publish("minichain-global", ...) become
self.pubsub.publish(self.TOPIC, ...) so both methods reference the same single
source of truth.
Comment on lines
+117
to
+129
| def test_balance_and_nonce_updates(self): | ||
| """Verify sender balance and nonce after deploy and call.""" | ||
|
|
||
| sender_before = self.state.get_account(self.pk) | ||
| initial_balance = sender_before["balance"] | ||
| initial_nonce = sender_before["nonce"] | ||
|
|
||
| code = "storage['x'] = 1" | ||
|
|
||
| tx_deploy = Transaction(self.pk, None, 10, initial_nonce, data=code) | ||
| tx_deploy.sign(self.sk) | ||
|
|
||
| contract_add_ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual state of tests/test_contract.py
echo "=== File information ==="
if [ -f "tests/test_contract.py" ]; then
wc -l tests/test_contract.py
echo ""
echo "=== Lines 110-140 (around the alleged truncation) ==="
sed -n '110,140p' tests/test_contract.py
echo ""
echo "=== Last 15 lines of the file ==="
tail -15 tests/test_contract.py
else
echo "File not found at tests/test_contract.py"
fiRepository: StabilityNexus/MiniChain
Length of output: 1325
test_balance_and_nonce_updates method is incomplete — will cause SyntaxError.
The test file ends abruptly at line 129 with the incomplete expression contract_add_, preventing the Python module from parsing. This will cause all tests in this file to fail immediately on import.
Additionally, initial_balance is assigned on line 121 but never used in the method body.
🧰 Tools
🪛 Ruff (0.15.0)
[error] 121-121: Local variable initial_balance is assigned to but never used
Remove assignment to unused variable initial_balance
(F841)
[warning] 129-129: Found useless expression. Either assign it to a variable or remove it.
(B018)
[error] 129-129: Undefined name contract_add_
(F821)
🤖 Prompt for AI Agents
In `@tests/test_contract.py` around lines 117 - 129, The test
test_balance_and_nonce_updates is cut off mid-statement (the stray token
contract_add_) causing a SyntaxError; remove the incomplete token and finish the
test by completing the deployment flow: after creating and signing tx_deploy
(Transaction, tx_deploy.sign), submit or apply the deploy to the state (e.g.,
call the same method used elsewhere to add/apply a contract and capture the
resulting contract_address or receipt), create a subsequent call transaction
(using updated nonce) to exercise balance/nonce changes, then assert the
sender's balance and nonce changed as expected; also either use initial_balance
in the assertions or remove that unused variable. Ensure references to
Transaction, tx_deploy, tx_deploy.sign, and test_balance_and_nonce_updates
locate where to fix.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 1-4: Remove the fragile sys.path manipulation in conftest.py:
delete the sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) line
and the associated imports (sys, os), and instead make the project an
installable package so pytest can import modules normally (add a pyproject.toml
or setup.py and use pip install -e . in local/CI dev workflow); if immediate
packaging is not possible, document a temporary CI setup that installs the
package (or sets PYTHONPATH) rather than mutating sys.path at runtime.
In `@core/chain.py`:
- Around line 54-57: The hash verification in add_block() is wrong: replace the
use of block.to_dict() with block.to_header_dict() when calling calculate_hash
so the recomputed hash excludes the block.hash field (use
calculate_hash(block.to_header_dict()) and compare to block.hash). Also add a
difficulty check in add_block(): verify the block.hash meets the PoW target
(e.g., startswith("0" * difficulty) or otherwise compare against the computed
target) using the current difficulty value before accepting the block; update
any related logging in add_block()/logger.warning to reflect whether the block
failed hash or difficulty verification.
In `@core/contract.py`:
- Around line 116-118: In the exception handler inside core.contract.py (the
block using logger.error with exc_info=True), remove the unused exception
variable `e` and replace the logger call with logger.exception("Contract
Execution Failed") so the stack trace is logged correctly; i.e., change the
except clause from "except Exception as e:" to "except Exception:" and call
logger.exception(...) instead of logger.error(..., exc_info=True).
- Around line 41-43: The guard "if not account" is dead because
State.get_account() always returns a dict and causes a spurious empty account to
be created; update the contract existence check to avoid creating accounts: call
a non-mutating lookup (e.g., use state.peek_account(contract_address) or
state.get_account(contract_address, create=False) if supported) to fetch the
account without side effects, then check the account's 'code' (the existing "if
not code" check) and return False when appropriate; remove the "if not account"
branch and ensure contract_address/account lookup uses the non-creating API so
querying a missing contract does not mutate state.
- Around line 11-19: The current try block that imports resource and calls
resource.setrlimit (the "resource import" block that raises a RuntimeError on
ImportError) causes contract execution to fail on Windows; change it to degrade
gracefully by catching ImportError without raising RuntimeError — log or queue a
warning/message that resource limits are not enforced and continue execution,
leaving the resource.setrlimit call skipped; ensure the outer exception handler
(which currently captures exceptions from this block) no longer receives a
RuntimeError from missing resource so contracts run on non-Unix systems.
In `@core/state.py`:
- Around line 59-88: The deployment branch in apply_transaction increments the
account nonce (sender['nonce'] += 1) but then calls
derive_contract_address(tx.sender, tx.nonce) using the pre-increment tx.nonce;
add a concise inline comment next to that derive_contract_address call
explaining that the address intentionally uses the sender's pre-increment nonce
(tx.nonce) for deterministic CREATE-style address derivation and that
sender['nonce'] reflects the post-increment state; reference apply_transaction,
derive_contract_address, sender['nonce'], tx.nonce and create_contract in the
comment to make the rationale clear.
- Around line 49-57: The validate_and_apply method currently just forwards to
apply_transaction with a TODO for semantic validation; remove the indirection or
implement the intended checks. Either (A) implement semantic validation inside
validate_and_apply (perform necessary checks on the tx, return the same
success/failure shape, and only call apply_transaction(tx) when validations
pass) or (B) delete validate_and_apply and update callers to call
apply_transaction directly; update any documentation/comments and preserve the
same return shape and error handling when making the change. Refer to
validate_and_apply and apply_transaction to locate the code to modify.
In `@main.py`:
- Around line 36-61: The block's transactions are being applied twice: remove
the manual loop that iterates mined_block.transactions and calls
state.apply_transaction; instead rely on chain.add_block(mined_block) and use
the unified chain.state for any further state interactions (e.g., crediting
miner reward via chain.state.credit_mining_reward if needed and calling
sync_nonce with chain.state and pending_nonce_map). Specifically, delete the for
tx in mined_block.transactions: ... block that references
state.apply_transaction and use chain.state and sync_nonce(chain.state,
pending_nonce_map, tx.sender) or equivalent only if you need to reconcile nonces
from the already-applied transactions.
- Around line 229-231: Add a top-level synchronous main() function that
configures logging and runs the async node_loop() (e.g., call
asyncio.run(node_loop())), replace the current if __name__ == "__main__": block
to call main(), and ensure the module exports main() to satisfy the setup.py
console entry point "minichain=main:main"; reference the existing node_loop
function when implementing the new main().
- Around line 78-79: You created two separate State instances (state and
chain.state) causing divergence; fix by using the blockchain's state as the
single source of truth: remove the standalone State() usage or construct
Blockchain with the existing State instance so that all operations (genesis
credit, mining rewards, transaction application) use chain.state; update calls
that currently reference state (e.g., genesis credit application, mining reward
logic, transaction.apply/validate) to use chain.state and ensure
chain.add_block() and any validation operate on that same State instance.
- Around line 40-47: The mining reward always goes to BURN_ADDRESS because Block
instances lack a miner attribute; fix by ensuring the miner address is set and
used: either add a miner parameter/property to Block (update Block.__init__ in
core/block.py to accept and assign self.miner) and populate it where blocks are
created, or change the code path that constructs/returns mined_block to pass the
miner explicitly and update the reward logic to read that value (replace
getattr(mined_block, "miner", None) with the real field or use the explicit
miner argument) before calling state.credit_mining_reward(miner_address).
In `@node/mempool.py`:
- Around line 21-47: The issue is a potential race where tx.verify() is called
before acquiring self._lock in add_transaction, allowing tx.timestamp (used by
_get_tx_id via calculate_hash(tx.to_dict())) to be mutated between verify and id
calculation; fix by ensuring the critical sequence is atomic: acquire self._lock
before calling tx.verify() and before calling _get_tx_id (or alternatively
create an immutable snapshot of tx fields used for verify and id under the
lock), updating add_transaction so the lock surrounds tx.verify(), tx_id =
self._get_tx_id(tx), the duplicate check, and adding to pending_txs/seen_tx_ids
to prevent inconsistent tx_id computation.
In `@setup.py`:
- Around line 12-15: The console_scripts entry point references main:main but
main.py lacks a synchronous main(), causing AttributeError; add a top-level def
main() in main.py that sets up logging and calls asyncio.run(node_loop()),
ensuring it imports/uses node_loop() (the async function defined in main.py) so
setuptools can invoke the CLI entry point successfully.
In `@tests/test_contract.py`:
- Around line 2-3: Remove the unused top-level imports "sys" and "os" from
tests/test_contract.py; locate the import statements referencing sys and os at
the top of the file and delete them so there are no unused import warnings or
linter failures (ensure no remaining references to sys or os elsewhere in the
file before removing).
Comment on lines
+1
to
+4
| import sys | ||
| import os | ||
|
|
||
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using an installable package instead of sys.path manipulation.
This sys.path.insert hack is fragile and can mask import issues. If the project adopts a pyproject.toml (or setup.py) with pip install -e ., pytest will discover all modules without path manipulation, and this file becomes unnecessary.
For the current project scope this is acceptable, but it won't scale well if the project grows or CI environments differ.
🤖 Prompt for AI Agents
In `@conftest.py` around lines 1 - 4, Remove the fragile sys.path manipulation in
conftest.py: delete the sys.path.insert(0,
os.path.dirname(os.path.abspath(__file__))) line and the associated imports
(sys, os), and instead make the project an installable package so pytest can
import modules normally (add a pyproject.toml or setup.py and use pip install -e
. in local/CI dev workflow); if immediate packaging is not possible, document a
temporary CI setup that installs the package (or sets PYTHONPATH) rather than
mutating sys.path at runtime.
Comment on lines
+54
to
+57
| # Verify block hash | ||
| if block.hash != calculate_hash(block.to_dict()): | ||
| logger.warning("Block %s rejected: Invalid hash %s", block.index, block.hash) | ||
| return False |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the hash verification bug by checking to_dict and to_header_dict implementations
echo "=== to_dict in block.py ==="
rg -n 'def to_dict' -A 20 core/block.py
echo ""
echo "=== to_header_dict in block.py ==="
rg -n 'def to_header_dict' -A 20 core/block.py
echo ""
echo "=== mine_block hash computation ==="
rg -n 'def mine_block' -A 30 consensus/pow.py
echo ""
echo "=== add_block method in chain.py ==="
rg -n 'def add_block' -A 40 core/chain.pyRepository: StabilityNexus/MiniChain
Length of output: 3959
🏁 Script executed:
#!/bin/bash
# Get the full mine_block function to see hash computation
rg -n 'calculate_hash.*to_' consensus/pow.py
echo ""
echo "=== Full context around hash computation in mine_block ==="
rg -n 'block.hash' consensus/pow.py -B 3 -A 3
echo ""
echo "=== Check if difficulty is validated anywhere in add_block ==="
rg -n 'difficulty' core/chain.pyRepository: StabilityNexus/MiniChain
Length of output: 1149
Critical: Hash verification uses to_dict() instead of to_header_dict() — every block will be rejected.
Mining computes the block hash via calculate_hash(block.to_header_dict()) (consensus/pow.py, line 54), which excludes the hash field. However, validation here recomputes using calculate_hash(block.to_dict()) (line 55), which includes the hash field. Since to_dict() contains "hash": self.hash, the recomputed hash will never match the mined hash, causing add_block() to reject every valid mined block.
Additionally, the PoW difficulty target is not verified in add_block(). A block with a valid hash but insufficient difficulty (i.e., not matching "0" * difficulty) would incorrectly pass validation.
Fix: Use block.to_header_dict() for hash verification and add difficulty target validation:
Proposed fix
# Verify block hash
- if block.hash != calculate_hash(block.to_dict()):
+ computed_hash = calculate_hash(block.to_header_dict())
+ if block.hash != computed_hash:
logger.warning("Block %s rejected: Invalid hash %s", block.index, block.hash)
return False
+
+ # Verify PoW difficulty target
+ if block.difficulty and not block.hash.startswith("0" * block.difficulty):
+ logger.warning("Block %s rejected: Hash does not meet difficulty %s", block.index, block.difficulty)
+ return False🤖 Prompt for AI Agents
In `@core/chain.py` around lines 54 - 57, The hash verification in add_block() is
wrong: replace the use of block.to_dict() with block.to_header_dict() when
calling calculate_hash so the recomputed hash excludes the block.hash field (use
calculate_hash(block.to_header_dict()) and compare to block.hash). Also add a
difficulty check in add_block(): verify the block.hash meets the PoW target
(e.g., startswith("0" * difficulty) or otherwise compare against the computed
target) using the current difficulty value before accepting the block; update
any related logging in add_block()/logger.warning to reflect whether the block
failed hash or difficulty verification.
Comment on lines
+11
to
+19
| try: | ||
| # Attempt to set resource limits (Unix only) | ||
| try: | ||
| import resource | ||
| # Limit CPU time (seconds) and memory (bytes) - example values | ||
| resource.setrlimit(resource.RLIMIT_CPU, (1, 1)) | ||
| # resource.setrlimit(resource.RLIMIT_AS, (100 * 1024 * 1024, 100 * 1024 * 1024)) | ||
| except ImportError as e: | ||
| raise RuntimeError(f"Resource limits not enforced: {e}") |
Contributor
There was a problem hiding this comment.
Contract execution always fails on Windows due to resource import.
When import resource fails (Windows), a RuntimeError is raised, caught by the outer except on Line 24, and put into the queue as an error. This means all contract executions fail on Windows with "Resource limits not enforced." Since the README/PR doesn't restrict to Unix-only, this is a significant platform limitation.
🐛 Proposed fix — degrade gracefully on non-Unix
try:
import resource
resource.setrlimit(resource.RLIMIT_CPU, (1, 1))
- except ImportError as e:
- raise RuntimeError(f"Resource limits not enforced: {e}")
+ except ImportError:
+ pass # Resource limits not available on this platform (e.g., Windows)🧰 Tools
🪛 Ruff (0.15.0)
[warning] 19-19: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 19-19: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@core/contract.py` around lines 11 - 19, The current try block that imports
resource and calls resource.setrlimit (the "resource import" block that raises a
RuntimeError on ImportError) causes contract execution to fail on Windows;
change it to degrade gracefully by catching ImportError without raising
RuntimeError — log or queue a warning/message that resource limits are not
enforced and continue execution, leaving the resource.setrlimit call skipped;
ensure the outer exception handler (which currently captures exceptions from
this block) no longer receives a RuntimeError from missing resource so contracts
run on non-Unix systems.
Comment on lines
+41
to
+43
| account = self.state.get_account(contract_address) | ||
| if not account: | ||
| return False |
Contributor
There was a problem hiding this comment.
get_account always returns a truthy dict — this check is dead code.
State.get_account() lazily initializes and always returns a non-empty dict, so if not account is never True. The code falls through to if not code on Line 50, which is the actual guard. The side effect is that querying a non-existent contract address creates an empty account entry in state.
♻️ Proposed fix
account = self.state.get_account(contract_address)
- if not account:
- return False
-
code = account.get("code")🤖 Prompt for AI Agents
In `@core/contract.py` around lines 41 - 43, The guard "if not account" is dead
because State.get_account() always returns a dict and causes a spurious empty
account to be created; update the contract existence check to avoid creating
accounts: call a non-mutating lookup (e.g., use
state.peek_account(contract_address) or state.get_account(contract_address,
create=False) if supported) to fetch the account without side effects, then
check the account's 'code' (the existing "if not code" check) and return False
when appropriate; remove the "if not account" branch and ensure
contract_address/account lookup uses the non-creating API so querying a missing
contract does not mutate state.
Comment on lines
+116
to
+118
| except Exception as e: | ||
| logger.error("Contract Execution Failed", exc_info=True) | ||
| return False |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use logger.exception() and remove unused variable e.
Per Ruff F841 and G201, e is assigned but never used, and logger.error(..., exc_info=True) should be logger.exception(...).
♻️ Proposed fix
- except Exception as e:
- logger.error("Contract Execution Failed", exc_info=True)
+ except Exception:
+ logger.exception("Contract Execution Failed")
return False🧰 Tools
🪛 Ruff (0.15.0)
[error] 116-116: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
[warning] 117-117: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
🤖 Prompt for AI Agents
In `@core/contract.py` around lines 116 - 118, In the exception handler inside
core.contract.py (the block using logger.error with exc_info=True), remove the
unused exception variable `e` and replace the logger call with
logger.exception("Contract Execution Failed") so the stack trace is logged
correctly; i.e., change the except clause from "except Exception as e:" to
"except Exception:" and call logger.exception(...) instead of logger.error(...,
exc_info=True).
Comment on lines
+78
to
+79
| state = State() | ||
| chain = Blockchain() |
Contributor
There was a problem hiding this comment.
Critical: Two independent State objects — state and chain.state — cause state divergence.
State() on Line 78 and Blockchain() on Line 79 create two separate state machines. Genesis credit (Line 110), mining rewards (Line 47), and transaction applications (Line 50) all target the standalone state, while chain.add_block() validates against its internal chain.state (which has no balances). This means chain.add_block() will reject every block because chain.state sees all senders as having zero balance.
Use chain.state as the single source of truth, or pass the same State instance to Blockchain.
🐛 Proposed fix — use chain's state
- state = State()
- chain = Blockchain()
+ chain = Blockchain()
+ state = chain.state # Use the chain's internal state as the single source of truth🤖 Prompt for AI Agents
In `@main.py` around lines 78 - 79, You created two separate State instances
(state and chain.state) causing divergence; fix by using the blockchain's state
as the single source of truth: remove the standalone State() usage or construct
Blockchain with the existing State instance so that all operations (genesis
credit, mining rewards, transaction application) use chain.state; update calls
that currently reference state (e.g., genesis credit application, mining reward
logic, transaction.apply/validate) to use chain.state and ensure
chain.add_block() and any validation operate on that same State instance.
Comment on lines
+229
to
+231
| if __name__ == "__main__": | ||
| logging.basicConfig(level=logging.INFO) | ||
| asyncio.run(node_loop()) |
Contributor
There was a problem hiding this comment.
No main() function defined — the setup.py console script entry point will fail.
setup.py defines minichain=main:main, but this file has no main() function. Consider wrapping the entry point:
♻️ Proposed fix
+def main():
+ logging.basicConfig(level=logging.INFO)
+ asyncio.run(node_loop())
+
+
if __name__ == "__main__":
- logging.basicConfig(level=logging.INFO)
- asyncio.run(node_loop())
+ main()🤖 Prompt for AI Agents
In `@main.py` around lines 229 - 231, Add a top-level synchronous main() function
that configures logging and runs the async node_loop() (e.g., call
asyncio.run(node_loop())), replace the current if __name__ == "__main__": block
to call main(), and ensure the module exports main() to satisfy the setup.py
console entry point "minichain=main:main"; reference the existing node_loop
function when implementing the new main().
Comment on lines
+21
to
+47
| def add_transaction(self, tx): | ||
| """ | ||
| Adds a transaction to the pool if: | ||
| - Signature is valid | ||
| - Transaction is not a duplicate | ||
| """ | ||
|
|
||
| if not tx.verify(): | ||
| logger.warning("Mempool: Invalid signature rejected") | ||
| return False | ||
|
|
||
| with self._lock: | ||
| tx_id = self._get_tx_id(tx) | ||
|
|
||
| if tx_id in self.seen_tx_ids: | ||
| logger.warning(f"Mempool: Duplicate transaction rejected {tx_id}") | ||
| return False | ||
|
|
||
| if len(self.pending_txs) >= self.max_size: | ||
| # Simple eviction: drop oldest or reject. Here we reject. | ||
| logger.warning("Mempool: Full, rejecting transaction") | ||
| return False | ||
|
|
||
| self.pending_txs.append(tx) | ||
| self.seen_tx_ids.add(tx_id) | ||
|
|
||
| return True |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
tx.verify() is called outside the lock — consider the implications.
tx.verify() on Line 28 runs before acquiring self._lock. This is safe since verify() is stateless (pure signature check), but _get_tx_id on Line 33 performs calculate_hash(tx.to_dict()) which also reads tx.timestamp. If a transaction's timestamp were mutated between the verify() call and the _get_tx_id call by another thread, the tx_id would be inconsistent. In practice, Transaction objects are not expected to be mutated concurrently, so this is low risk.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 36-36: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In `@node/mempool.py` around lines 21 - 47, The issue is a potential race where
tx.verify() is called before acquiring self._lock in add_transaction, allowing
tx.timestamp (used by _get_tx_id via calculate_hash(tx.to_dict())) to be mutated
between verify and id calculation; fix by ensuring the critical sequence is
atomic: acquire self._lock before calling tx.verify() and before calling
_get_tx_id (or alternatively create an immutable snapshot of tx fields used for
verify and id under the lock), updating add_transaction so the lock surrounds
tx.verify(), tx_id = self._get_tx_id(tx), the duplicate check, and adding to
pending_txs/seen_tx_ids to prevent inconsistent tx_id computation.
Comment on lines
+12
to
+15
| entry_points={ | ||
| "console_scripts": [ | ||
| "minichain=main:main", # Requires main() function in main.py | ||
| ], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate main.py and verify its existence and content
echo "=== Locating main.py ==="
fd -t f 'main.py'
echo ""
echo "=== Checking for main() function definition ==="
rg -n 'def main' --type py
echo ""
echo "=== Checking for node_loop() function definition ==="
rg -n 'def node_loop' --type py
echo ""
echo "=== Checking if __main__ block exists ==="
rg -n 'if __name__' --type py
echo ""
echo "=== Displaying setup.py entry points section ==="
sed -n '10,20p' setup.pyRepository: StabilityNexus/MiniChain
Length of output: 647
🏁 Script executed:
# Let's check the __main__ block in main.py to understand what it does
echo "=== Content of __main__ block in main.py ==="
sed -n '225,235p' main.py
echo ""
echo "=== Check surrounding context to see imports ==="
head -20 main.pyRepository: StabilityNexus/MiniChain
Length of output: 771
Broken console script entry point — main:main function does not exist in main.py.
The entry point minichain=main:main will fail with AttributeError when run from the CLI. The file defines node_loop() as an async function (line 75) and runs it via asyncio.run(node_loop()) in the __main__ block (line 229), but there is no main() function.
Add a synchronous main() wrapper function in main.py that setuptools can call:
def main():
logging.basicConfig(level=logging.INFO)
asyncio.run(node_loop())🤖 Prompt for AI Agents
In `@setup.py` around lines 12 - 15, The console_scripts entry point references
main:main but main.py lacks a synchronous main(), causing AttributeError; add a
top-level def main() in main.py that sets up logging and calls
asyncio.run(node_loop()), ensuring it imports/uses node_loop() (the async
function defined in main.py) so setuptools can invoke the CLI entry point
successfully.
Comment on lines
+2
to
+3
| import sys | ||
| import os |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused imports: sys and os.
These were likely needed for a previous sys.path.append(...) hack that has been removed. They should be cleaned up.
♻️ Proposed fix
import unittest
-import sys
-import os
from core import State, Transaction🤖 Prompt for AI Agents
In `@tests/test_contract.py` around lines 2 - 3, Remove the unused top-level
imports "sys" and "os" from tests/test_contract.py; locate the import statements
referencing sys and os at the top of the file and delete them so there are no
unused import warnings or linter failures (ensure no remaining references to sys
or os elsewhere in the file before removing).
This was referenced Feb 21, 2026
Closed
This was referenced Feb 22, 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:
Closes #1
1. Overview
This Pull Request delivers the fully functional core implementation of MiniChain.
Key Objectives Achieved
2. What This PR Implements
2.1 Core Blockchain Features
2.2 Smart Contract System (Bonus Requirement)
2.3 Architecture Refactor
2.4 Type of Change
3. Project Folder Structure
3.1 Module Responsibilities
Core Layer
Consensus Layer
Network Layer
Node Layer
Testing
4. Testing Instructions
Step 1 — Setup Environment
Install dependencies:
Step 2 — Run Node Simulation
Executes full blockchain lifecycle:
Genesis → Transfer → Contract Deployment → Contract Execution
Step 3 — Run Unit Tests
Validate core and smart contracts:
Expected Test Output
5. Technical Highlights
6. Why This PR Matters
Screenshots of successful execution and testing
Checklist
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.
Summary by CodeRabbit
New Features
Tests
Chores