[Feature] Major refactor - Folder structure, node based interaction, CLI tool for demonstration#23
Open
anuragShingare30 wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughThis PR reorganizes the codebase into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Author
|
@Zahnentferner Added interactive CLI interface by refactoring certain portion of code keeping the core implementation same. Pls review and let me know |
![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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.py (2)
130-135:⚠️ Potential issue | 🔴 Critical
_run_nodecalled with 6 arguments but defined with only 5 —TypeErrorat runtime.Line 130 calls
_run_nodewith six positional arguments:await _run_node(network, state, chain, mempool, pending_nonce_map, claim_nonce)Line 135 defines it with five:
async def _run_node(network, chain, mempool, pending_nonce_map, get_next_nonce):Python will raise
TypeError: _run_node() takes 5 positional arguments but 6 were givenimmediately whennode_loop()is entered. Thestateargument inserted at position 2 (consistent with the refactor intent) was never added to the function signature.🐛 Proposed fix
-async def _run_node(network, chain, mempool, pending_nonce_map, get_next_nonce): +async def _run_node(network, state, chain, mempool, pending_nonce_map, get_next_nonce):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 130 - 135, The call site passes a missing state argument to _run_node, causing a TypeError; update the async def _run_node signature to accept the state parameter (e.g., async def _run_node(network, state, chain, mempool, pending_nonce_map, get_next_nonce):) and then update any references inside _run_node to use that state variable (or alternatively remove the extra argument at the call site if the refactor intended to drop state) so the parameter list and the call match.
81-81:⚠️ Potential issue | 🔴 Critical
Blockchain(state)raisesTypeError— the newBlockchain.__init__takes no arguments.
minichain.chain.Blockchain.__init__is defined asdef __init__(self):(see the provided chain.py snippet). Passingstateas a positional argument raises:TypeError: Blockchain.__init__() takes 1 positional argument but 2 were givenThe previous
core.Blockchainapparently accepted an externalstate, but the refactored class creates its own state internally via_create_genesis_block(). The call at line 81 was not updated when the import on line 7 was changed.🐛 Proposed fix
- state = State() - chain = Blockchain(state) + chain = Blockchain() + state = chain.state # use the state owned by the chain🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 81, The call "chain = Blockchain(state)" passes an obsolete external state into Blockchain.__init__ (which is now defined as def __init__(self) and creates its own state via _create_genesis_block), so remove the positional argument and instantiate with "chain = Blockchain()"; if the local variable state is needed elsewhere, replace usages with the chain's internal state (e.g., access chain.state or a provided getter) rather than passing state into the constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.py`:
- Around line 543-569: The prompt currently recomputes state by calling
self.node.blockchain.state.get_balance(...) on every loop iteration; initialize
a cached_balance variable before the loop (compute it once via
self.node.blockchain.state.get_balance(self.node.address)), use cached_balance
to render the prompt inside the while True loop, and only refresh cached_balance
after executing state-mutating commands (detect when cmd is one of "mine",
"send", or "faucet" or when the corresponding handler in cmds returns a flag
indicating state changed) by re-calling
self.node.blockchain.state.get_balance(self.node.address); ensure error handling
and non-mutating commands keep using the cached value.
- Around line 305-335: The predicted contract address uses nonce + 1 causing a
mismatch with how addresses are derived in apply_tx; in cmd_deploy replace the
call to derive_contract_address(sender, nonce + 1) with the actual transaction
nonce (use nonce or tx.nonce) so the Expected address computation matches
state.apply_tx's use of tx.nonce in derive_contract_address.
- Around line 68-84: The _load_or_create_wallet function currently writes the
private key file using default umask and may leave it world-readable; change the
creation path handling to create the file with restrictive permissions (owner
read/write only) — either open the file atomically with os.open and mode 0o600
or call os.chmod(path, 0o600) immediately after writing the SigningKey to ensure
only the owner can read/write the file; keep usages of WALLET_DIR, path, and
SigningKey intact and ensure reading (os.path.exists/path open) still works for
existing keys.
- Around line 226-266: cmd_mine currently builds Block with only mempool txs and
calls credit_mining_reward on a transient State, so the miner never receives an
on‑chain reward; fix by creating a coinbase Transaction for the miner and
prepending/inserting it into Block.transactions before calling mine_block (use
Block and Transaction constructors used in this file), remove or stop relying on
self.node.blockchain.state.credit_mining_reward (do not mutate get_state()
return), and let blockchain.add_block(mined) persist the reward when the node
processes the coinbase tx; keep broadcasting the mined block via
self.node.p2p.broadcast_block_sync(mined) and continue nonce syncs for tx.sender
as before.
In `@minichain.sh`:
- Line 3: The script currently runs cd "$(dirname "$0")" without checking its
result; update that command to guard against failure by appending a fallback
that exits on error (e.g., use cd "$(dirname "$0")" || exit 1) so the script
stops immediately if changing to the script directory fails—this addresses
ShellCheck SC2164 and prevents misleading downstream "No such file or directory"
errors when trying to run cli.py.
In `@minichain/.gitignore`:
- Around line 1-4: The subdirectory .gitignore contains duplicated patterns
(__pycache__/, *.py[cod], *$py.class, *.so) that are already covered by the
repository-wide ignore; remove this redundant .gitignore (or delete those
duplicate entries) so the repo relies on the root .gitignore rules instead,
ensuring only one source of truth for these patterns.
In `@minichain/block.py`:
- Around line 108-121: The from_dict method currently ignores the stored
merkle_root in data; modify Block.from_dict to read the serialized "merkle_root"
(e.g., stored_merkle = data.get("merkle_root")) after constructing the Block
(which recomputes merkle_root in Block.__init__), compare stored_merkle to
block.merkle_root, and raise an exception (ValueError or similar) or log and
reject when they differ so deserialization fails fast on tampered headers;
reference Block.from_dict, Block.__init__, and the merkle_root attribute when
making this change.
In `@minichain/chain.py`:
- Around line 46-57: get_state currently recomputes state by replaying
self.chain on every call (via State.apply_tx), causing heavy cost when accessed
frequently; change to cache the computed State in a private attribute (e.g.,
self._state) and make the state property return the cached state, computing it
once if None by calling get_state; update get_state to compute and store
self._state (or provide an incremental update path when adding a block) and
ensure any methods that mutate the chain (e.g., add_block, mine, replace_chain,
or other functions that append/replace self.chain) invalidate or update
self._state so the cache stays consistent.
- Around line 129-159: The _is_valid_chain method in class Chain fails to verify
that each non-genesis block's stored hash matches the computed hash of its
header, allowing forged blocks with correct difficulty prefixes to pass; inside
Chain._is_valid_chain, after checking previous_hash, index, and difficulty for
i>0, compute the canonical header hash (using
calculate_hash(block.to_header_dict()) or the project's equivalent) and compare
it to block.hash, returning False (and optionally logging) if they differ so
only blocks whose hash matches their actual content are accepted.
In `@minichain/config.py`:
- Around line 29-30: TREASURY_PRIVATE_KEY and TREASURY_ADDRESS are hardcoded
secrets; remove the literal values and change the module to load them from an
external source (e.g., environment variables or a local, gitignored config) so
they are not committed; add or reference a helper (e.g., a
generate_treasury_keypair or init_treasury script) that produces a new Ed25519
seed and address for first-run, and ensure code paths that use
TREASURY_PRIVATE_KEY and TREASURY_ADDRESS (the constants) validate presence and
fail fast with a clear message if not set.
In `@minichain/mempool.py`:
- Around line 64-78: remove_transaction currently calls _get_tx_id(t) repeatedly
inside the lock causing O(n·H) hashing under contention; convert the mempool to
a dict keyed by tx_id (replace _pending_txs: list -> dict[str, Transaction]) and
update add_transaction to compute tx_id once and store self._pending_txs[tx_id]
= tx, adjust get_transactions_for_block to return
list(self._pending_txs.values()), and change remove_transaction to compute tx_id
once (using _get_tx_id) then do self._pending_txs.pop(tx_id, None) and
self._seen_tx_ids.discard(tx_id) inside the lock so removals and duplicate
checks are O(1) and no per-entry hashing happens while holding _lock.
In `@minichain/p2p.py`:
- Around line 298-314: The f-string prefix on the literal at the start of
_handle_incoming_tx is unnecessary (Ruff F541); remove the leading "f" from the
string literal "\n[P2P] New transaction received!" in the _handle_incoming_tx
method so it becomes a normal string, keeping the surrounding logging/print
behavior (Transaction.from_dict, self.node.mempool.add_transaction,
self.on_tx_received remain unchanged).
- Around line 212-233: In _send_message rename the unused reader to _reader to
silence the linter (change "reader, writer = ..." to "_reader, writer = ..."),
and replace the immediate removal of peers in the
TimeoutError/ConnectionRefusedError handlers with a failure-count based
eviction: add or use a per-peer failure tracker (e.g. self._peer_failures dict
keyed by (host, port)), increment the counter on each
TimeoutError/ConnectionRefusedError, only discard the peer from self.peers when
the counter reaches a small threshold (e.g. 3), and reset/remove the counter on
a successful send so transient hiccups don’t cause permanent eviction; ensure
references to self.peers and _send_message are updated accordingly.
- Around line 49-56: Replace the brittle time.sleep and inline import with a
proper threading.Event-based ready signal: add a threading.Event instance (e.g.,
self._ready_event) in the class initializer, move the import time to the module
top, have _run_event_loop set self._ready_event when the server is fully
started, and in start_background call self._thread.start() then wait on
self._ready_event.wait(timeout=...) instead of time.sleep(0.1); ensure wait uses
a sensible timeout and handle the timeout case (raise or log) so callers know
the server failed to start.
- Line 9: Replace deprecated typing imports: remove Callable, Dict, List,
Optional, Set from typing import and instead import Callable from
collections.abc and Optional from typing if still needed (or use built-in | None
for Python 3.10+). Update all type annotations in this module that use Dict[…],
List[…], Set[…] to use built-in generics dict[…], list[…], set[…], and change
any Callable[...] annotations to collections.abc.Callable; update Optional[T]
usages to T | None where applicable. Ensure import line and annotations in
functions/classes in this file reflect these changes (replace the current "from
typing import Callable, Dict, List, Optional, Set" pattern).
- Around line 127-156: _connect_to_peer currently unconditionally adds the peer
because _send_message swallows exceptions; change _send_message to return a
boolean success (True on successful send/ack, False on any failure) and update
connect_to_peer to check that return value before mutating self.peers or calling
request_chain: call success = await self._send_message(...), only do
self.peers.add((host, port)), logger.info and await self.request_chain(host,
port) if success is True; otherwise log a warning and return False. Ensure
callers like connect_to_peer_sync handle the boolean return the same way.
- Around line 316-346: In _handle_incoming_block, change the unnecessary
f-string used for the static message " \n[P2P] Behind on blocks, syncing..." to
a plain string literal (remove the leading f) to satisfy the Ruff F541 lint
rule; update the await block that prints this message (the print call inside
_handle_incoming_block) so it uses a normal string instead of an f-string.
In `@minichain/state.py`:
- Around line 59-121: Consolidate the duplicate logic in apply_tx and
apply_transaction by making one canonical implementation (preferably
exception-based) — e.g., keep apply_tx as the single source of truth and
refactor apply_transaction to call apply_tx and translate exceptions to boolean
returns; ensure the canonical method includes missing checks such as contract
redeploy collision (the check at the apply_transaction path around
derive_contract_address/create_contract) and preserves contract
execution/rollback behavior (contract_machine.execute, balance and nonce
updates), and update callers to use the unified method.
- Around line 64-68: In apply_tx, don’t blindly credit COINBASE_SENDER
transactions: validate tx.amount equals the expected mining reward before
calling create_account/crediting; compute the expected reward from the chain
context (e.g., a get_block_reward or constant) and if it does not match,
raise/return an error so replace_chain will refuse the block. Update apply_tx
(and/or replace_chain where coinbase txs are applied) to pass the block height
or expected reward into the validation, and use the existing symbols apply_tx,
COINBASE_SENDER, create_account and replace_chain to locate and enforce this
check.
- Around line 91-96: The contract address is being derived with
derive_contract_address(tx.sender, tx.nonce) which mismatches CLI's expectation
of derive_contract_address(sender, nonce + 1); update the deployment branch in
the State handling (the block using tx.receiver, tx.data, create_contract) to
call derive_contract_address(tx.sender, tx.nonce + 1) so the predicted address
matches the CLI, keep passing tx.data and initial_balance=tx.amount to
create_contract and preserve the existing control flow and return value.
In `@minichain/transaction.py`:
- Around line 33-44: The from_dict factory method incorrectly treats
timestamp==0 as missing due to a truthiness check; update Transaction.from_dict
to detect presence of the timestamp key (and allow zero) rather than relying on
data.get(...) truthiness — e.g., if "timestamp" in data and data["timestamp"] is
not None then use data["timestamp"]/1000 else None — so the Transaction(...)
call receives the exact 0-derived timestamp instead of being replaced by the
constructor's current time.
- Around line 70-74: The sign_with_hex method currently signs with any provided
private key; update it to validate the key matches the transaction sender before
setting self.signature: decode the private_key_hex into a SigningKey (as done
now), derive the corresponding public key (or address) using the same conversion
used in sign(), compare it against self.sender, and raise an exception or return
an error if they don’t match; only after the sender check succeed should you
produce signed = signing_key.sign(self.hash_payload) and set self.signature =
signed.signature.hex(). Ensure you reference sign_with_hex, sign, SigningKey and
self.sender when making the change so the guard mirrors the existing sign()
behavior.
- Line 104: Add a short inline comment above the existing mid-file import "from
minichain.config import GENESIS_TIMESTAMP" explaining that the import is
intentionally located after the class definitions to avoid a circular import
with minichain.config (e.g., "placed here to prevent circular import with
minichain.config, required by GENESIS_TIMESTAMP"); keep the comment concise and
reference GENESIS_TIMESTAMP and minichain.config so future readers know why the
import is non-standard.
---
Outside diff comments:
In `@main.py`:
- Around line 130-135: The call site passes a missing state argument to
_run_node, causing a TypeError; update the async def _run_node signature to
accept the state parameter (e.g., async def _run_node(network, state, chain,
mempool, pending_nonce_map, get_next_nonce):) and then update any references
inside _run_node to use that state variable (or alternatively remove the extra
argument at the call site if the refactor intended to drop state) so the
parameter list and the call match.
- Line 81: The call "chain = Blockchain(state)" passes an obsolete external
state into Blockchain.__init__ (which is now defined as def __init__(self) and
creates its own state via _create_genesis_block), so remove the positional
argument and instantiate with "chain = Blockchain()"; if the local variable
state is needed elsewhere, replace usages with the chain's internal state (e.g.,
access chain.state or a provided getter) rather than passing state into the
constructor.
Comment on lines
+68
to
+84
| def _load_or_create_wallet(self): | ||
| """Load existing wallet or create new one for this port.""" | ||
| os.makedirs(WALLET_DIR, exist_ok=True) | ||
| path = os.path.join(WALLET_DIR, f"{self.wallet_name}.key") | ||
|
|
||
| if os.path.exists(path): | ||
| with open(path) as f: | ||
| sk = SigningKey(bytes.fromhex(f.read().strip())) | ||
| logger.info(f"Loaded wallet from {path}") | ||
| else: | ||
| sk = SigningKey.generate() | ||
| with open(path, "w") as f: | ||
| f.write(sk.encode().hex()) | ||
| logger.info(f"Created new wallet: {path}") | ||
|
|
||
| pk = sk.verify_key.encode(encoder=HexEncoder).decode() | ||
| return sk, pk |
Contributor
There was a problem hiding this comment.
Wallet private key stored with default file permissions.
The private key file is written with the process's default umask, typically resulting in world-readable permissions (e.g., 0o644). For an educational project this may be acceptable, but it's a bad security habit to form.
Proposed fix: restrict file permissions
else:
sk = SigningKey.generate()
- with open(path, "w") as f:
- f.write(sk.encode().hex())
+ fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w") as f:
+ f.write(sk.encode().hex())📝 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
| def _load_or_create_wallet(self): | |
| """Load existing wallet or create new one for this port.""" | |
| os.makedirs(WALLET_DIR, exist_ok=True) | |
| path = os.path.join(WALLET_DIR, f"{self.wallet_name}.key") | |
| if os.path.exists(path): | |
| with open(path) as f: | |
| sk = SigningKey(bytes.fromhex(f.read().strip())) | |
| logger.info(f"Loaded wallet from {path}") | |
| else: | |
| sk = SigningKey.generate() | |
| with open(path, "w") as f: | |
| f.write(sk.encode().hex()) | |
| logger.info(f"Created new wallet: {path}") | |
| pk = sk.verify_key.encode(encoder=HexEncoder).decode() | |
| return sk, pk | |
| def _load_or_create_wallet(self): | |
| """Load existing wallet or create new one for this port.""" | |
| os.makedirs(WALLET_DIR, exist_ok=True) | |
| path = os.path.join(WALLET_DIR, f"{self.wallet_name}.key") | |
| if os.path.exists(path): | |
| with open(path) as f: | |
| sk = SigningKey(bytes.fromhex(f.read().strip())) | |
| logger.info(f"Loaded wallet from {path}") | |
| else: | |
| sk = SigningKey.generate() | |
| fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| with os.fdopen(fd, "w") as f: | |
| f.write(sk.encode().hex()) | |
| logger.info(f"Created new wallet: {path}") | |
| pk = sk.verify_key.encode(encoder=HexEncoder).decode() | |
| return sk, pk |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 68-68: Missing return type annotation for private function _load_or_create_wallet
(ANN202)
[warning] 76-76: Logging statement uses f-string
(G004)
[warning] 81-81: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.py` around lines 68 - 84, The _load_or_create_wallet function currently
writes the private key file using default umask and may leave it world-readable;
change the creation path handling to create the file with restrictive
permissions (owner read/write only) — either open the file atomically with
os.open and mode 0o600 or call os.chmod(path, 0o600) immediately after writing
the SigningKey to ensure only the owner can read/write the file; keep usages of
WALLET_DIR, path, and SigningKey intact and ensure reading (os.path.exists/path
open) still works for existing keys.
Comment on lines
+226
to
+266
| def cmd_mine(self, args): | ||
| """mine - Mine pending transactions""" | ||
| miner = self.node.address | ||
| txs = self.node.mempool.get_transactions_for_block() | ||
|
|
||
| print(c("Mining...", Colors.DIM), end=" ", flush=True) | ||
|
|
||
| block = Block( | ||
| index=self.node.blockchain.last_block.index + 1, | ||
| previous_hash=self.node.blockchain.last_block.hash, | ||
| transactions=txs, | ||
| difficulty=DIFFICULTY, | ||
| ) | ||
|
|
||
| try: | ||
| start = time.time() | ||
| mined = mine_block(block, difficulty=DIFFICULTY) | ||
| elapsed = time.time() - start | ||
|
|
||
| if self.node.blockchain.add_block(mined): | ||
| self.node.blockchain.state.credit_mining_reward(miner) | ||
| for tx in txs: | ||
| self._sync_nonce(tx.sender) | ||
|
|
||
| # Broadcast to peers | ||
| self.node.p2p.broadcast_block_sync(mined) | ||
|
|
||
| new_bal = self.node.blockchain.state.get_balance(miner) | ||
|
|
||
| return ( | ||
| f"\r{c('✓ Block Mined!', Colors.GREEN)} \n" | ||
| f" Block: {c(f'#{mined.index}', Colors.CYAN)}\n" | ||
| f" Hash: {mined.hash[:24]}...\n" | ||
| f" Txs: {len(txs)} processed\n" | ||
| f" Time: {elapsed:.2f}s\n" | ||
| f" Reward: {c('+50', Colors.GREEN)} coins\n" | ||
| f" Balance: {c(f'{new_bal:,}', Colors.GREEN)} coins" | ||
| ) | ||
| return c("\r✗ Block rejected", Colors.RED) | ||
| except Exception as e: | ||
| return c(f"\r✗ Mining failed: {e}", Colors.RED) |
Contributor
There was a problem hiding this comment.
Mining reward is never persisted — credit_mining_reward mutates a transient state, and no coinbase tx is included in the block.
Two compounding issues:
- No coinbase transaction in the mined block (line 229–238): The block only contains mempool transactions. Without a coinbase tx, the miner receives no on-chain reward.
credit_mining_rewardon a throwaway state (line 246):self.node.blockchain.staterecomputes the state from the chain viaget_state(). The returnedStateobject is modified but immediately discarded — the balance change is never persisted.
After mining, the "reward" displayed at line 261 comes from a new state recomputation, which won't include the reward. The miner never actually gets paid.
Proposed fix: include a coinbase tx in the block
+from minichain.transaction import create_coinbase_tx
+from minichain.config import MINING_REWARD # or use State.DEFAULT_MINING_REWARD
...
def cmd_mine(self, args):
"""mine - Mine pending transactions"""
miner = self.node.address
txs = self.node.mempool.get_transactions_for_block()
+ # Add coinbase reward transaction
+ coinbase_tx = create_coinbase_tx(miner, 50, self.node.blockchain.last_block.index + 1)
+ txs = [coinbase_tx] + txs
+
print(c("Mining...", Colors.DIM), end=" ", flush=True)
block = Block(
index=self.node.blockchain.last_block.index + 1,
previous_hash=self.node.blockchain.last_block.hash,
transactions=txs,
difficulty=DIFFICULTY,
)
try:
start = time.time()
mined = mine_block(block, difficulty=DIFFICULTY)
elapsed = time.time() - start
if self.node.blockchain.add_block(mined):
- self.node.blockchain.state.credit_mining_reward(miner)
for tx in txs:
self._sync_nonce(tx.sender)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 226-226: Unused method argument: args
(ARG002)
[warning] 265-265: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.py` around lines 226 - 266, cmd_mine currently builds Block with only
mempool txs and calls credit_mining_reward on a transient State, so the miner
never receives an on‑chain reward; fix by creating a coinbase Transaction for
the miner and prepending/inserting it into Block.transactions before calling
mine_block (use Block and Transaction constructors used in this file), remove or
stop relying on self.node.blockchain.state.credit_mining_reward (do not mutate
get_state() return), and let blockchain.add_block(mined) persist the reward when
the node processes the coinbase tx; keep broadcasting the mined block via
self.node.p2p.broadcast_block_sync(mined) and continue nonce syncs for tx.sender
as before.
Comment on lines
+305
to
+335
| def cmd_deploy(self, args): | ||
| """deploy - Deploy a smart contract""" | ||
| print(f"{c('Enter contract code (empty line to finish):', Colors.CYAN)}") | ||
| lines = [] | ||
| while True: | ||
| try: | ||
| line = input(c(" | ", Colors.DIM)) | ||
| if line == "": | ||
| break | ||
| lines.append(line) | ||
| except EOFError: | ||
| break | ||
|
|
||
| if not lines: | ||
| return c("No code entered", Colors.RED) | ||
|
|
||
| code = "\n".join(lines) | ||
| sender = self.node.address | ||
| nonce = self._get_nonce(sender) | ||
|
|
||
| tx = Transaction(sender=sender, receiver=None, amount=0, nonce=nonce, data=code) | ||
| tx.sign(self.node.signing_key) | ||
|
|
||
| if self.node.mempool.add_transaction(tx): | ||
| addr = self.node.blockchain.state.derive_contract_address(sender, nonce + 1) | ||
| return ( | ||
| f"{c('✓', Colors.GREEN)} Contract deployment submitted\n" | ||
| f" Expected address: {addr[:32]}...\n" | ||
| f" {c('→ Run', Colors.DIM)} {c('mine', Colors.YELLOW)} {c('to deploy', Colors.DIM)}" | ||
| ) | ||
| return c("✗ Deployment rejected", Colors.RED) |
Contributor
There was a problem hiding this comment.
Predicted contract address uses wrong nonce (nonce + 1 vs. tx.nonce).
Line 329: self.node.blockchain.state.derive_contract_address(sender, nonce + 1) — but apply_tx in state.py (line 94) uses tx.nonce (the value before account nonce increment). The +1 here means the displayed "Expected address" will never match the actual deployed address. This should use nonce (the same value passed to the transaction).
Proposed fix
- addr = self.node.blockchain.state.derive_contract_address(sender, nonce + 1)
+ addr = self.node.blockchain.state.derive_contract_address(sender, nonce)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 305-305: Unused method argument: args
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.py` around lines 305 - 335, The predicted contract address uses nonce + 1
causing a mismatch with how addresses are derived in apply_tx; in cmd_deploy
replace the call to derive_contract_address(sender, nonce + 1) with the actual
transaction nonce (use nonce or tx.nonce) so the Expected address computation
matches state.apply_tx's use of tx.nonce in derive_contract_address.
Comment on lines
+543
to
+569
| while True: | ||
| try: | ||
| bal = self.node.blockchain.state.get_balance(self.node.address) | ||
| prompt = f"{c(f'[:{self.node.port}]', Colors.CYAN)} {c(f'({bal})', Colors.GREEN)} › " | ||
| line = input(prompt).strip() | ||
| if not line: | ||
| continue | ||
|
|
||
| parts = line.split() | ||
| cmd, args = parts[0].lower(), parts[1:] | ||
|
|
||
| if cmd in cmds: | ||
| result = cmds[cmd](args) | ||
| if result: | ||
| print(result) | ||
| print() | ||
| else: | ||
| print(c(f"Unknown command: {cmd}", Colors.RED)) | ||
| print(c("Type 'help' for available commands.\n", Colors.DIM)) | ||
|
|
||
| except KeyboardInterrupt: | ||
| print(c("\n(Use 'exit' or 'q' to quit)", Colors.DIM)) | ||
| except EOFError: | ||
| break | ||
| except Exception as e: | ||
| print(c(f"Error: {e}", Colors.RED)) | ||
| print() |
Contributor
There was a problem hiding this comment.
State recomputed from genesis on every prompt iteration.
Line 545 calls self.node.blockchain.state.get_balance(...) on every loop iteration just to display the prompt balance. As noted in the chain.py review, the state property replays the entire chain. This will cause noticeable lag as the chain grows.
As a quick mitigation independent of the caching suggestion in chain.py, consider caching the balance for the prompt and only refreshing it after state-changing commands (mine, send, faucet).
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 546-546: String contains ambiguous › (SINGLE RIGHT-POINTING ANGLE QUOTATION MARK). Did you mean > (GREATER-THAN SIGN)?
(RUF001)
[warning] 567-567: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.py` around lines 543 - 569, The prompt currently recomputes state by
calling self.node.blockchain.state.get_balance(...) on every loop iteration;
initialize a cached_balance variable before the loop (compute it once via
self.node.blockchain.state.get_balance(self.node.address)), use cached_balance
to render the prompt inside the while True loop, and only refresh cached_balance
after executing state-mutating commands (detect when cmd is one of "mine",
"send", or "faucet" or when the corresponding handler in cmds returns a flag
indicating state changed) by re-calling
self.node.blockchain.state.get_balance(self.node.address); ensure error handling
and non-mutating commands keep using the cached value.
| @@ -0,0 +1,4 @@ | |||
| #!/bin/bash | |||
| # MiniChain CLI launcher | |||
| cd "$(dirname "$0")" | |||
Contributor
There was a problem hiding this comment.
Add || exit 1 to guard against cd failure (ShellCheck SC2164).
If cd fails for any reason, the script silently continues in the wrong directory, causing a confusing No such file or directory: cli.py error.
🐛 Proposed fix
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit 1📝 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
| cd "$(dirname "$0")" | |
| cd "$(dirname "$0")" || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain.sh` at line 3, The script currently runs cd "$(dirname "$0")"
without checking its result; update that command to guard against failure by
appending a fallback that exits on error (e.g., use cd "$(dirname "$0")" || exit
1) so the script stops immediately if changing to the script directory
fails—this addresses ShellCheck SC2164 and prevents misleading downstream "No
such file or directory" errors when trying to run cli.py.
Comment on lines
+64
to
+68
| # Coinbase transaction: just create/credit receiver account | ||
| if tx.sender == COINBASE_SENDER: | ||
| self.create_account(tx.receiver, 0) | ||
| self.accounts[tx.receiver]["balance"] += tx.amount | ||
| return self |
Contributor
There was a problem hiding this comment.
Coinbase transactions bypass amount validation — a peer could broadcast a block with an inflated reward.
apply_tx unconditionally credits tx.amount for any coinbase transaction without verifying that the amount matches the expected mining reward. During chain sync via replace_chain, a malicious peer's block containing a coinbase tx with an arbitrary amount would be accepted and applied. Consider validating the coinbase amount against the expected reward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/state.py` around lines 64 - 68, In apply_tx, don’t blindly credit
COINBASE_SENDER transactions: validate tx.amount equals the expected mining
reward before calling create_account/crediting; compute the expected reward from
the chain context (e.g., a get_block_reward or constant) and if it does not
match, raise/return an error so replace_chain will refuse the block. Update
apply_tx (and/or replace_chain where coinbase txs are applied) to pass the block
height or expected reward into the validation, and use the existing symbols
apply_tx, COINBASE_SENDER, create_account and replace_chain to locate and
enforce this check.
Comment on lines
+91
to
+96
| # Handle contract deployment (receiver is None) | ||
| if tx.receiver is None or tx.receiver == "": | ||
| if tx.data: | ||
| contract_address = self.derive_contract_address(tx.sender, tx.nonce) | ||
| self.create_contract(contract_address, tx.data, initial_balance=tx.amount) | ||
| return self |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the nonce used for derive_contract_address in both locations
echo "=== state.py apply_tx ==="
rg -n 'derive_contract_address' minichain/state.py -A 2 -B 2
echo ""
echo "=== cli.py ==="
rg -n 'derive_contract_address' cli.py -A 2 -B 2Repository: StabilityNexus/MiniChain
Length of output: 1103
Contract address derivation uses tx.nonce, but CLI uses nonce + 1 — predicted address will be wrong.
Line 94 calls derive_contract_address(tx.sender, tx.nonce), but in cli.py line 329 the expected address is computed as derive_contract_address(sender, nonce + 1). The CLI will display an incorrect expected contract address to the user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/state.py` around lines 91 - 96, The contract address is being
derived with derive_contract_address(tx.sender, tx.nonce) which mismatches CLI's
expectation of derive_contract_address(sender, nonce + 1); update the deployment
branch in the State handling (the block using tx.receiver, tx.data,
create_contract) to call derive_contract_address(tx.sender, tx.nonce + 1) so the
predicted address matches the CLI, keep passing tx.data and
initial_balance=tx.amount to create_contract and preserve the existing control
flow and return value.
Comment on lines
+33
to
+44
| @staticmethod | ||
| def from_dict(data: dict) -> "Transaction": | ||
| """Create transaction from dictionary.""" | ||
| return Transaction( | ||
| sender=data["sender"], | ||
| receiver=data["receiver"], | ||
| amount=data["amount"], | ||
| nonce=data["nonce"], | ||
| data=data.get("data"), | ||
| signature=data.get("signature"), | ||
| timestamp=data.get("timestamp") / 1000 if data.get("timestamp") else None, | ||
| ) |
Contributor
There was a problem hiding this comment.
from_dict breaks round-trip when timestamp is 0 (falsy).
Line 43 uses a truthiness check: data.get("timestamp") / 1000 if data.get("timestamp") else None. If timestamp is 0, this evaluates to None, causing the constructor to stamp the transaction with time.time() — corrupting the reconstructed transaction. This would break genesis-block deserialization if the genesis timestamp ever resolves to 0, and is a latent bug for any block with timestamp=0 received via P2P replace_chain.
Proposed fix
`@staticmethod`
def from_dict(data: dict) -> "Transaction":
"""Create transaction from dictionary."""
+ ts = data.get("timestamp")
return Transaction(
sender=data["sender"],
receiver=data["receiver"],
amount=data["amount"],
nonce=data["nonce"],
data=data.get("data"),
signature=data.get("signature"),
- timestamp=data.get("timestamp") / 1000 if data.get("timestamp") else None,
+ timestamp=ts / 1000 if ts is not None else None,
)📝 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
| @staticmethod | |
| def from_dict(data: dict) -> "Transaction": | |
| """Create transaction from dictionary.""" | |
| return Transaction( | |
| sender=data["sender"], | |
| receiver=data["receiver"], | |
| amount=data["amount"], | |
| nonce=data["nonce"], | |
| data=data.get("data"), | |
| signature=data.get("signature"), | |
| timestamp=data.get("timestamp") / 1000 if data.get("timestamp") else None, | |
| ) | |
| `@staticmethod` | |
| def from_dict(data: dict) -> "Transaction": | |
| """Create transaction from dictionary.""" | |
| ts = data.get("timestamp") | |
| return Transaction( | |
| sender=data["sender"], | |
| receiver=data["receiver"], | |
| amount=data["amount"], | |
| nonce=data["nonce"], | |
| data=data.get("data"), | |
| signature=data.get("signature"), | |
| timestamp=ts / 1000 if ts is not None else None, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/transaction.py` around lines 33 - 44, The from_dict factory method
incorrectly treats timestamp==0 as missing due to a truthiness check; update
Transaction.from_dict to detect presence of the timestamp key (and allow zero)
rather than relying on data.get(...) truthiness — e.g., if "timestamp" in data
and data["timestamp"] is not None then use data["timestamp"]/1000 else None — so
the Transaction(...) call receives the exact 0-derived timestamp instead of
being replaced by the constructor's current time.
Comment on lines
+70
to
+74
| def sign_with_hex(self, private_key_hex: str): | ||
| """Sign transaction with hex-encoded private key.""" | ||
| signing_key = SigningKey(private_key_hex.encode(), encoder=HexEncoder) | ||
| signed = signing_key.sign(self.hash_payload) | ||
| self.signature = signed.signature.hex() |
Contributor
There was a problem hiding this comment.
sign_with_hex does not validate that the signing key matches self.sender.
Unlike sign() (line 65), sign_with_hex silently signs with any private key, even one that doesn't correspond to the transaction's sender. This can produce transactions that fail signature verification at validation time, making debugging difficult. Consider adding the same sender-match guard.
Proposed fix
def sign_with_hex(self, private_key_hex: str):
"""Sign transaction with hex-encoded private key."""
signing_key = SigningKey(private_key_hex.encode(), encoder=HexEncoder)
+ if signing_key.verify_key.encode(encoder=HexEncoder).decode() != self.sender:
+ raise ValueError("Signing key does not match sender")
signed = signing_key.sign(self.hash_payload)
self.signature = signed.signature.hex()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/transaction.py` around lines 70 - 74, The sign_with_hex method
currently signs with any provided private key; update it to validate the key
matches the transaction sender before setting self.signature: decode the
private_key_hex into a SigningKey (as done now), derive the corresponding public
key (or address) using the same conversion used in sign(), compare it against
self.sender, and raise an exception or return an error if they don’t match; only
after the sender check succeed should you produce signed =
signing_key.sign(self.hash_payload) and set self.signature =
signed.signature.hex(). Ensure you reference sign_with_hex, sign, SigningKey and
self.sender when making the change so the guard mirrors the existing sign()
behavior.
| return f"Tx({self.sender[:8]}→{(self.receiver or 'CONTRACT')[:8]}, {self.amount})" | ||
|
|
||
|
|
||
| from minichain.config import GENESIS_TIMESTAMP |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mid-file import to break circular dependency.
The from minichain.config import GENESIS_TIMESTAMP import at line 104 (after the class definition) is placed here to avoid a circular import. This is a known pattern but makes the dependency non-obvious. Consider adding a brief comment explaining why it's here.
Suggested comment
+# Late import to avoid circular dependency (config → transaction → config)
from minichain.config import GENESIS_TIMESTAMP📝 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
| from minichain.config import GENESIS_TIMESTAMP | |
| # Late import to avoid circular dependency (config → transaction → config) | |
| from minichain.config import GENESIS_TIMESTAMP |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/transaction.py` at line 104, Add a short inline comment above the
existing mid-file import "from minichain.config import GENESIS_TIMESTAMP"
explaining that the import is intentionally located after the class definitions
to avoid a circular import with minichain.config (e.g., "placed here to prevent
circular import with minichain.config, required by GENESIS_TIMESTAMP"); keep the
comment concise and reference GENESIS_TIMESTAMP and minichain.config so future
readers know why the import is non-standard.
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
Refactor MiniChain into a node-based, P2P-capable blockchain with deterministic genesis and a polished CLI for multi-node testing
Changes Added
minichainwith updated importsapply_tx, remove_transactionAdditions
config/constants (treasury, difficulty, genesis timestamp)Demo Video
minichain_demo_v1.mp4
Note: @Zahnentferner The folder structure is inspires by
PR #19, work done by @SIDDHANTCOOKIEcloses #22
Summary by CodeRabbit
Release Notes
New Features
Chores