[Feature] MiniChain v0 - Educational Blockchain Prototype#16
Open
anuragShingare30 wants to merge 1 commit intoStabilityNexus:mainfrom
Open
[Feature] MiniChain v0 - Educational Blockchain Prototype#16anuragShingare30 wants to merge 1 commit intoStabilityNexus:mainfrom
anuragShingare30 wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughIntroduces MiniChain v0, a complete educational blockchain prototype featuring Ed25519 transactions, proof-of-work consensus, P2P networking, state management, and an interactive CLI for running blockchain nodes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Node as Node (main.py)
participant Mempool
participant Network
participant Blockchain
participant Consensus
User->>Node: send command
Node->>Mempool: add_tx(transaction)
alt Valid Transaction
Mempool-->>Node: True
Node->>Network: broadcast_tx()
Network-->>Network: send to peers
else Invalid
Mempool-->>Node: False
Node-->>User: error
end
sequenceDiagram
participant Miner as Miner Node
participant Mempool
participant Consensus as PoW Engine
participant Blockchain
participant Network
participant Peer as Other Nodes
Miner->>Mempool: get_pending(max_count)
Mempool-->>Miner: transactions
Miner->>Consensus: mine_block(prev_block, txs, miner_addr)
loop nonce increment
Consensus->>Consensus: compute_hash()
alt difficulty met
Consensus-->>Miner: Block
end
end
Miner->>Blockchain: add_block(block)
Blockchain-->>Blockchain: validate & apply
Blockchain-->>Miner: True
Miner->>Network: broadcast_block(block)
Network->>Peer: send block message
Peer->>Peer: validate & add
sequenceDiagram
participant NodeA as Node A
participant NetworkA as Network A
participant NodeB as Node B
participant NetworkB as Network B
participant BlockchainB as Blockchain B
NodeB->>NetworkB: connect(NodeA_host, NodeA_port)
NetworkB->>NetworkA: register message
NetworkA->>NodeA: record peer
NetworkB->>NetworkA: request_chain message
NetworkA->>NetworkA: get blockchain
NetworkA->>NetworkB: chain response
NetworkB->>BlockchainB: replace_chain(received_chain)
alt Chain valid & longer
BlockchainB-->>NetworkB: True
NetworkB->>NetworkB: clear mempool
else Invalid or shorter
BlockchainB-->>NetworkB: False
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 36
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Minichain_v0/block.py`:
- Around line 49-60: The from_dict method currently ignores the stored "hash"
field from serialized data; update Block.from_dict to explicitly handle it by
(a) reading data.get("hash") after constructing the Block (using Block(...)
which triggers compute_hash()) and then asserting or raising a ValueError if the
provided hash does not match block.hash (use compute_hash() or the block.hash
property for comparison), or if you prefer not to enforce it, add a clear
comment in Block.from_dict stating that the stored "hash" is intentionally
discarded because blocks recompute their hash in Block.__init__ (which calls
compute_hash()), referencing Block.from_dict, Block.__init__, compute_hash, and
to_dict so maintainers understand the behavior.
- Line 9: The import of typing.List is deprecated; remove "from typing import
List" and replace usages of the symbol List in type annotations (e.g., any
annotations referencing List in this module such as function signatures, class
attributes, or variable annotations) with the built-in generic form list[...] or
plain list as appropriate so the file uses the built-in list type instead of
typing.List (update annotations in Minichain_v0/block.py where List is
referenced).
- Around line 18-22: The timestamp assignment in Block.__init__ uses "timestamp
or time.time()" which treats 0.0 as falsy; change it to an explicit None check
so a provided 0.0 is preserved—use the timestamp parameter with "if timestamp is
not None else time.time()" (update the constructor in the Block class where
self.timestamp is set and keep variable name timestamp).
In `@Minichain_v0/blockchain.py`:
- Around line 30-36: get_state currently recomputes State from scratch by
iterating self.chain and apply_tx each call, which will degrade; change to cache
the latest state and update it incrementally: add cached attributes (e.g.,
self._cached_state and self._cached_length/index) and modify functions that
mutate the chain (e.g., the block/transaction intake methods that append or
replace blocks) to apply_tx only for the new blocks' transactions to advance
self._cached_state, or invalidate the cache on reorgs/replacements and recompute
only when needed; ensure get_state returns the cached state (recomputing once if
cache is empty or invalidated).
- Around line 38-39: The docstring on validate_block is misleading because it
claims transaction validation is included; update the validate_block method's
docstring to state it only checks block structure and proof-of-work (PoW) while
deferring transaction checks to validate_block_transactions, e.g., replace
"(structure, PoW, transactions)" with "(structure and PoW) — transaction
validation is handled by validate_block_transactions" so callers know where
transaction validation occurs.
- Around line 86-110: The is_valid_chain method currently applies transactions
with apply_tx to state before checking block structure, which wastes work and
can mask structural issues; modify is_valid_chain so that for each non-genesis
block it first calls self.validate_block(block, chain[i - 1]) and only if that
returns True proceeds to apply transactions via apply_tx (catching ValueError as
before), keeping the genesis check using create_genesis_block intact and
preserving the State() initialization and return semantics.
In `@Minichain_v0/config.py`:
- Around line 27-31: Replace the hardcoded TREASURY_PRIVATE_KEY and
TREASURY_ADDRESS constants with a runtime loader: check for an environment
variable (e.g., TREASURY_PRIVATE_KEY) and if absent generate a new Ed25519
keypair, derive TREASURY_ADDRESS from the private key, and persist the private
key to a file named treasury.key (which is already in .gitignore) for subsequent
runs; update any references to TREASURY_PRIVATE_KEY and TREASURY_ADDRESS in the
codebase to use the values returned by this loader so the secret is not embedded
in source control.
In `@Minichain_v0/consensus.py`:
- Around line 34-47: The mining loop in mine_block blocks the event loop and
must be cancellable and run off the networking thread: change mine_block to
accept an optional threading.Event (e.g., cancel_event) and check
cancel_event.is_set() each loop iteration (and return None if set); move the
infinite while True into while not (cancel_event and cancel_event.is_set()) and
keep the existing nonce/hash checks; start mine_block inside a threading.Thread
from the caller (instead of blocking the TCP handler) and signal cancellation by
setting the Event (e.g., when a new valid block/tx arrives). Ensure you
reference and update the Block creation/nonce logic inside mine_block and
respect DIFFICULTY/timeout behavior as before.
- Line 27: Replace the list concatenation used to assemble all_txs with sequence
unpacking to satisfy the Ruff RUF005 suggestion: instead of building all_txs by
adding [coinbase] + transactions, construct it using an unpacking form that
includes coinbase and expands transactions (update the assignment to all_txs and
keep the same coinbase and transactions symbols).
- Line 7: Replace the deprecated typing.List import and usages: remove "from
typing import List" and update any type hints that use List (e.g., functions or
variables referencing List such as parameters like chain: List[...], return
types, or class attributes) to use the built-in generic "list" (e.g., chain:
list[...]) instead; ensure all occurrences of "List" in consensus.py are changed
and the unused typing import is deleted so type annotations use the PEP 585
built-in container types.
In `@Minichain_v0/main.py`:
- Around line 156-158: Several print statements in main.py use an unnecessary
f-string prefix where there are no interpolations; remove the leading f from
those string literals (e.g., the prints around transaction handling and
broadcasting such as the lines that currently read f"✅ Transaction added to
mempool", f"📡 Broadcasted to peers", and the other occurrences at the reported
locations) so they become plain strings; locate these prints in the
transaction/mempool handling flow (the function/method that calls await
network.broadcast_tx(tx) and nearby print calls) and update each f"..." to "..."
to eliminate the extraneous f prefix.
- Around line 50-63: The banner border is misaligned because content lines are
not padded to a fixed inner width; in print_banner define a single BOX_WIDTH
(inner content width) and use it to format every content line (e.g., f"║
{text.ljust(BOX_WIDTH)} ║"), ensure address formatting (address[:20] + "..." +
address[-8:]) fits within BOX_WIDTH or is truncated accordingly, and replace the
ad-hoc spacing in print_banner with consistent ljust/truncation using BOX_WIDTH
so all lines align with the top/bottom borders.
- Line 128: Replace the deprecated asyncio.get_event_loop() call with
asyncio.get_running_loop() where the REPL line is read (the call using
run_in_executor to call input for "minichain> "); specifically change the
invocation used to create the executor task (the expression currently
referencing asyncio.get_event_loop().run_in_executor) to
asyncio.get_running_loop().run_in_executor so it uses the active async loop;
ensure you keep the same parameters (None, input, "minichain> ") and update any
related comments if present.
- Around line 25-42: The create_wallet function writes the raw Ed25519 seed to
disk without restrictive permissions; change the wallet file creation so the
private key file is owner-only (permission 0o600). After generating/writing the
key to wallet_file (or preferably write to a temporary file then rename), ensure
you set the file mode to 0o600 using os.chmod(wallet_file, 0o600) or create the
file with os.open and mode=0o600, keeping the rest of create_wallet
(SigningKey.generate(), get_wallet_file, address printing) unchanged.
- Around line 142-160: The send command handler currently parses amount with
int(parts[2]) which can raise ValueError and also allows zero/negative amounts;
update the send branch to parse the amount inside a try/except catching
ValueError and print a clear error (e.g., "Invalid amount") when parsing fails,
then validate that amount > 0 and print a rejection message if not before
creating Transaction(address, receiver, amount, ...), signing, adding to
mempool, and broadcasting; apply the identical parsing+positive-value validation
to the faucet command handler so faucet amounts are also validated and provide
clear user feedback instead of relying on the broad exception handler.
- Around line 183-189: Remove the unused SigningKey instance and gate the faucet
operation to bootstrap nodes: delete the unused treasury_key creation (the
SigningKey(...) call) near where Transaction is constructed, keep
tx.sign(TREASURY_PRIVATE_KEY) as the signing call, and wrap the faucet-handling
code so it only executes when is_bootstrap is true (return an error/ignore the
command for non-bootstrap nodes). Update the faucet command handler around the
Transaction creation (symbols: treasury_key, TREASURY_PRIVATE_KEY, Transaction,
tx.sign, is_bootstrap) to enforce the bootstrap check and eliminate the dead
variable.
In `@Minichain_v0/mempool.py`:
- Around line 54-58: The remove method is re-hashing transactions twice causing
unnecessary work; cache each transaction's hash when inserting and use the
cached value in remove. Change the mempool storage to keep (tx, tx_hash) pairs
or a dict keyed by tx_hash (update methods that append/iterate: e.g., where
self.transactions is mutated and where _seen_hashes is updated), then implement
remove to build hashes_to_remove from the provided txs by using their cached
hash (or computing once) and filter self.transactions by the stored tx_hash, and
update self._seen_hashes accordingly; also update any other code that accesses
self.transactions to account for the new tuple/dict structure and ensure
Transaction.hash() is called only at insertion.
- Around line 31-43: The add() validation currently compares tx.nonce to
state.get_nonce(tx.sender) so any second transaction from the same tx.sender
will be rejected until the first is mined; either document this behavior in
add() (add a comment/docstring near add(), the nonce check, and mention the
mempool only validates against committed state) or implement proper
pending-nonce handling by counting pending transactions from the same sender in
the mempool and requiring tx.nonce == state.get_nonce(tx.sender) +
pending_count; update the check around tx.nonce and any related logic to use
that computed expected nonce.
- Around line 19-43: The mempool add method and State.apply_tx must reject
non-positive transaction amounts to prevent minting via negative values; update
Minichain_v0/mempool.py in the add(self, tx: Transaction, state: State) path
(inside mempool.add) to check if tx.amount is > 0 and return False for tx.amount
<= 0, and also add the same guard in Minichain_v0/state.py inside apply_tx (or
State.apply_tx) to raise/return an error or abort when tx.amount <= 0 so both
the mempool and state layer enforce the invariant that Transaction.amount > 0.
In `@Minichain_v0/minichain`:
- Around line 18-26: The start case currently assigns PORT="${2:-8000}" without
validation; add a guard in the start branch to ensure PORT is an integer within
TCP port range (1–65535) and fallback to 8000 or print an error and exit on
invalid input before invoking python3 main.py; update the logic around the PORT
variable and the two python3 main.py invocations (the start case) so you
validate PORT with a numeric check (e.g., regex or arithmetic test) and range
check, then use the validated PORT when calling main.py or abort with a clear
message if invalid.
- Around line 1-16: Add fail-fast shell options by inserting "set -euo pipefail"
immediately after the shebang so the script exits on any command failure, treats
unset variables as errors, and fails on pipeline errors; place this before
computing SCRIPT_DIR and before sourcing the virtualenv (affecting the
SCRIPT_DIR assignment, cd "$SCRIPT_DIR", and the venv activation block) so a
failed cd or broken venv/bin/activate will cause an immediate exit.
In `@Minichain_v0/network.py`:
- Around line 111-118: The handler for msg_type "register" is comparing peer
tuples directly which can mismatch because peer_addr[0] is an IP while
previously stored peers may use "localhost"; normalize the host before
membership checks and storage: resolve peer_host to a canonical IP (e.g., via
socket.gethostbyname or similar) and use that normalized IP when checking and
appending to self.peers so duplicates like ("localhost", port) vs ("127.0.0.1",
port) are treated the same; update the logic around peer_host, peer_port and
where self.peers is appended (the register branch) and ensure connect() uses the
same normalization routine.
- Around line 43-44: The call to asyncio.create_task(self._run_server()) should
keep the returned Task on the instance to avoid GC/cancellation; update the
place where the server is started (the code that calls asyncio.create_task in
the Network class / its start method) to assign the task to an instance
attribute like self._server_task (or similar), so the Task returned from
create_task(self._run_server()) is retained for the lifetime of the server and
can be referenced later for cancellation or awaiting.
- Around line 148-169: The _send_to_peer coroutine currently calls
asyncio.open_connection and reader.read without timeouts, which can block the
broadcast; wrap the connect and the read in asyncio.wait_for with a small
configurable timeout (e.g., 5s) to prevent long stalls: use
asyncio.wait_for(asyncio.open_connection(host, port), timeout=CONNECT_TIMEOUT)
and when expecting a response wrap reader.read(1048576) in asyncio.wait_for(...,
timeout=READ_TIMEOUT), handle asyncio.TimeoutError by closing the writer (if
created) and returning None, and catch/log TimeoutError separately from other
exceptions so the function (and callers using msg["type"] == "request_chain")
won't hang on unreachable peers.
- Line 15: Remove the unused import PROTOCOL_ID from the top of network.py to
eliminate the unused symbol; specifically delete the line "from config import
PROTOCOL_ID" (or if PROTOCOL_ID was intended to be used, instead reference it
where needed in functions or classes that construct/identify the network
protocol, e.g., any send/receive or handshake code that should use PROTOCOL_ID
such as functions named handshake, start_listening, or connect_to_peer). Ensure
no other references to PROTOCOL_ID remain after removal.
- Around line 69-90: The connection handler currently uses reader.read(1048576)
and json.loads directly which can receive partial TCP data and corrupt JSON;
change _handle_connection to use a 4-byte big-endian length-prefixed framing:
read the 4-byte length with reader.readexactly(4), unpack it to get
payload_size, then reader.readexactly(payload_size) before json.loads, and when
sending responses prefix the JSON bytes with struct.pack(">I",
len(payload_bytes)) and writer.write(length_prefix + payload_bytes); apply this
framing consistently wherever reader.read / writer.write and json.loads /
json.dumps are used (e.g., in _handle_connection and any send/receive helpers),
and handle readexactly exceptions to log/close the connection gracefully.
- Around line 177-187: The current broadcast_tx and broadcast_block functions
call self._send_to_peer sequentially which blocks on slow/unreachable peers;
change both functions to fan-out concurrently by creating a list of send
coroutines/tasks for each (host, port) in self.peers and run them with
asyncio.gather(..., return_exceptions=True) so one slow peer won't stall others;
ensure you still log or handle exceptions from gather results (e.g., iterate
results and log non-None/Exception entries) and keep message building (msg =
{"type": ..., "data": ...}) and the call to self._send_to_peer unchanged.
In `@Minichain_v0/README.md`:
- Line 104: The file ends without the required final newline (MD047); update
README.md so the last line containing "`v0` (currently we are here) → `v1`
(optimizations) → `v2` (smart contracts)" is followed by a single trailing
newline character (ensure exactly one newline at EOF and no extra blank lines)
to satisfy the linter rule.
- Around line 23-42: The README's Test Scenarios section has markdownlint
issues: add a blank line after each level-3 heading (the "### 1. Mining for
rewards", "### 2. Distribute from treasury", and "### 3. Send coins" headings)
and update each fenced code block to include a language specifier (e.g., change
``` to ```bash) so the three blocks under those headings comply with MD022 and
MD040; adjust only the headings and the opening backtick fences in the blocks
shown.
- Line 71: Edit the README table row that lists `main.py` (the entry showing
"CLI interface") and replace the redundant phrase "CLI interface" with either
"CLI" or "Command-line interface" (preferably "CLI") so the description reads
simply `CLI`; update the table cell containing `CLI interface` accordingly.
- Around line 46-57: The table listing CLI commands lacks a header row so
Markdown renderers won't treat it as a table; add a header row (e.g., "Command |
Alias | Description") directly above the existing separator row and ensure the
separator remains as the next line (e.g., "|---|---|---|") so entries like
`balance`, `address`, `send <addr> <amt>`, `mine`, etc. render correctly as a
proper three-column table.
In `@Minichain_v0/requirements.txt`:
- Line 3: Update PyNaCl to at least 1.6.2 in both dependency declarations:
change the version specifier in Minichain_v0/requirements.txt (the existing
"pynacl>=1.5.0" entry) to "pynacl>=1.6.2" and update the PyNaCl entry in
setup.py (the install_requires or requirements list used by setup()/setup.cfg)
to match "pynacl>=1.6.2" so both sources align with the root requirements.txt.
In `@Minichain_v0/state.py`:
- Around line 33-37: The docstring for apply_tx is misleading because
apply_tx(State, Transaction) mutates the passed-in State object in place and
returns the same object; update the apply_tx function docstring to clearly state
it performs in-place mutation of the provided State and returns the same
(mutated) State object (or, if you prefer non-mutating semantics, implement a
copy-of-State at the start and apply mutations to the copy and return that) —
refer to the apply_tx function and the State and Transaction types when making
the change.
- Line 59: The raise statement uses an unnecessary f-string: replace the
spurious f-prefix in the raise ValueError(f"Insufficient balance") expression
with a normal string literal (raise ValueError("Insufficient balance")) so it no
longer triggers the Ruff F541 warning; locate the exact raise
ValueError(f"Insufficient balance") occurrence in state.py and update it
accordingly.
- Around line 39-42: The coinbase case (tx.sender == "0"*64) currently always
calls state.create_account(tx.receiver, tx.amount) which is a no-op if the
account exists, so mining rewards after the first block are discarded; change
that branch to credit the receiver instead of only creating: if the receiver
account does not exist call state.create_account(tx.receiver, tx.amount)
otherwise add tx.amount to the existing balance (e.g.
state.accounts[tx.receiver] += tx.amount or via an existing credit/add_balance
helper), ensuring coinbase rewards accumulate across blocks.
In `@Minichain_v0/transaction.py`:
- Around line 68-77: Transaction.from_dict currently accesses required fields
directly and will raise an unhelpful KeyError on malformed peer data; update
Transaction.from_dict to validate required keys (sender, receiver, amount,
nonce) by using data.get(...) or checking membership and wrap the construction
in a try/except that catches KeyError/TypeError and raises a clearer ValueError
(or custom exception) with context including the missing key and the offending
input; reference the Transaction.from_dict method and ensure Block.from_dict
callers can handle the new clear exception.
| import json | ||
| import hashlib | ||
| import time | ||
| from typing import List |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace deprecated typing.List with built-in list (Ruff UP035).
♻️ Proposed fix
-from typing import List
...
- def __init__(self, index: int, prev_hash: str, transactions: List[Transaction],
+ def __init__(self, index: int, prev_hash: str, transactions: list[Transaction],🧰 Tools
🪛 Ruff (0.15.1)
[warning] 9-9: typing.List is deprecated, use list instead
(UP035)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/block.py` at line 9, The import of typing.List is deprecated;
remove "from typing import List" and replace usages of the symbol List in type
annotations (e.g., any annotations referencing List in this module such as
function signatures, class attributes, or variable annotations) with the
built-in generic form list[...] or plain list as appropriate so the file uses
the built-in list type instead of typing.List (update annotations in
Minichain_v0/block.py where List is referenced).
Comment on lines
+18
to
+22
| timestamp: float = None, nonce: int = 0): | ||
| self.index = index | ||
| self.prev_hash = prev_hash | ||
| self.transactions = transactions | ||
| self.timestamp = timestamp or time.time() |
Contributor
There was a problem hiding this comment.
timestamp or time.time() is falsy-zero sensitive; use explicit None check.
or treats 0.0 as falsy, so Block(..., timestamp=0.0) would silently use time.time() instead. Prefer an explicit is None guard, which also fixes the implicit Optional Ruff warning (RUF013).
♻️ Proposed fix
- def __init__(self, index: int, prev_hash: str, transactions: List[Transaction],
- timestamp: float = None, nonce: int = 0):
+ def __init__(self, index: int, prev_hash: str, transactions: list[Transaction],
+ timestamp: float | None = None, nonce: int = 0):
...
- self.timestamp = timestamp or time.time()
+ self.timestamp = timestamp if timestamp is not None else time.time()🧰 Tools
🪛 Ruff (0.15.1)
[warning] 18-18: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/block.py` around lines 18 - 22, The timestamp assignment in
Block.__init__ uses "timestamp or time.time()" which treats 0.0 as falsy; change
it to an explicit None check so a provided 0.0 is preserved—use the timestamp
parameter with "if timestamp is not None else time.time()" (update the
constructor in the Block class where self.timestamp is set and keep variable
name timestamp).
Comment on lines
+49
to
+60
| @staticmethod | ||
| def from_dict(data: dict) -> "Block": | ||
| """Create block from dictionary.""" | ||
| txs = [Transaction.from_dict(tx) for tx in data["transactions"]] | ||
| block = Block( | ||
| index=data["index"], | ||
| prev_hash=data["prev_hash"], | ||
| transactions=txs, | ||
| timestamp=data["timestamp"], | ||
| nonce=data["nonce"] | ||
| ) | ||
| return block |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
from_dict silently ignores the stored "hash" field.
Block.__init__ always calls compute_hash(), so data["hash"] from to_dict is never read or validated during deserialization. A tampered hash field in the JSON is discarded; only the block's other fields matter. This is correct for correctness (chain validation does the real check), but it makes the "hash" entry in to_dict purely cosmetic. A brief comment or an assertion would make the intent explicit:
💡 Suggested clarification
`@staticmethod`
def from_dict(data: dict) -> "Block":
"""Create block from dictionary."""
txs = [Transaction.from_dict(tx) for tx in data["transactions"]]
block = Block(
index=data["index"],
prev_hash=data["prev_hash"],
transactions=txs,
timestamp=data["timestamp"],
nonce=data["nonce"]
)
+ # Note: data["hash"] is intentionally not used; hash is recomputed
+ # from block fields and validated by blockchain.is_valid_chain().
return block📝 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) -> "Block": | |
| """Create block from dictionary.""" | |
| txs = [Transaction.from_dict(tx) for tx in data["transactions"]] | |
| block = Block( | |
| index=data["index"], | |
| prev_hash=data["prev_hash"], | |
| transactions=txs, | |
| timestamp=data["timestamp"], | |
| nonce=data["nonce"] | |
| ) | |
| return block | |
| `@staticmethod` | |
| def from_dict(data: dict) -> "Block": | |
| """Create block from dictionary.""" | |
| txs = [Transaction.from_dict(tx) for tx in data["transactions"]] | |
| block = Block( | |
| index=data["index"], | |
| prev_hash=data["prev_hash"], | |
| transactions=txs, | |
| timestamp=data["timestamp"], | |
| nonce=data["nonce"] | |
| ) | |
| # Note: data["hash"] is intentionally not used; hash is recomputed | |
| # from block fields and validated by blockchain.is_valid_chain(). | |
| return block |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/block.py` around lines 49 - 60, The from_dict method currently
ignores the stored "hash" field from serialized data; update Block.from_dict to
explicitly handle it by (a) reading data.get("hash") after constructing the
Block (using Block(...) which triggers compute_hash()) and then asserting or
raising a ValueError if the provided hash does not match block.hash (use
compute_hash() or the block.hash property for comparison), or if you prefer not
to enforce it, add a clear comment in Block.from_dict stating that the stored
"hash" is intentionally discarded because blocks recompute their hash in
Block.__init__ (which calls compute_hash()), referencing Block.from_dict,
Block.__init__, compute_hash, and to_dict so maintainers understand the
behavior.
Comment on lines
+30
to
+36
| def get_state(self) -> State: | ||
| """Recompute current state by replaying all transactions from genesis.""" | ||
| state = State() | ||
| for block in self.chain: | ||
| for tx in block.transactions: | ||
| state = apply_tx(state, tx) | ||
| return state |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
get_state() replays the entire chain on every call — will degrade as chain grows.
This method is called on every CLI command (balance, send, mine, faucet, treasury) and on every incoming block/transaction via the network. As the chain grows, each call becomes linearly more expensive. For v0 this is acceptable, but consider caching the state and updating it incrementally when blocks are added/replaced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/blockchain.py` around lines 30 - 36, get_state currently
recomputes State from scratch by iterating self.chain and apply_tx each call,
which will degrade; change to cache the latest state and update it
incrementally: add cached attributes (e.g., self._cached_state and
self._cached_length/index) and modify functions that mutate the chain (e.g., the
block/transaction intake methods that append or replace blocks) to apply_tx only
for the new blocks' transactions to advance self._cached_state, or invalidate
the cache on reorgs/replacements and recompute only when needed; ensure
get_state returns the cached state (recomputing once if cache is empty or
invalidated).
Comment on lines
+38
to
+39
| def validate_block(self, block: Block, prev_block: Block) -> bool: | ||
| """Check if a block is valid (structure, PoW, transactions).""" |
Contributor
There was a problem hiding this comment.
Misleading docstring: validate_block does not check transactions.
The docstring says "Check if a block is valid (structure, PoW, transactions)" but the method only validates structure and PoW. Transaction validation is performed separately in validate_block_transactions. Update the docstring to avoid confusion.
Proposed fix
def validate_block(self, block: Block, prev_block: Block) -> bool:
- """Check if a block is valid (structure, PoW, transactions)."""
+ """Check if a block is valid (structure and PoW)."""📝 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 validate_block(self, block: Block, prev_block: Block) -> bool: | |
| """Check if a block is valid (structure, PoW, transactions).""" | |
| def validate_block(self, block: Block, prev_block: Block) -> bool: | |
| """Check if a block is valid (structure and PoW).""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/blockchain.py` around lines 38 - 39, The docstring on
validate_block is misleading because it claims transaction validation is
included; update the validate_block method's docstring to state it only checks
block structure and proof-of-work (PoW) while deferring transaction checks to
validate_block_transactions, e.g., replace "(structure, PoW, transactions)" with
"(structure and PoW) — transaction validation is handled by
validate_block_transactions" so callers know where transaction validation
occurs.
| @@ -0,0 +1,3 @@ | |||
| # MiniChain v0 Dependencies | |||
| # Cryptography: Ed25519 signatures | |||
| pynacl>=1.5.0 | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "requirements.txt" -type fRepository: StabilityNexus/MiniChain
Length of output: 118
🌐 Web query:
CVE-2025-69277 PyNaCl libsodium Ed25519
💡 Result:
CVE-2025-69277 is a libsodium Ed25519 point-validation flaw: in libsodium versions before commit ad3004e, the low-level function crypto_core_ed25519_is_valid_point() can incorrectly accept certain elliptic-curve points that are not in the main subgroup, but this matters primarily in “atypical use cases” where applications do custom cryptography and/or pass untrusted points to that function. Severity is typically rated Medium (CVSS 4.5). [1][2]
PyNaCl impact
- PyNaCl is considered affected before 1.6.2 (because it bundles/depends on libsodium builds that lacked the fix). [2]
- PyNaCl 1.6.2 (released 2026-01-01) updates bundled libsodium to 1.0.20-stable (2025-12-31 build) to resolve CVE-2025-69277. [3]
What to do
- If you use PyNaCl: upgrade to PyNaCl ≥ 1.6.2. [3]
- If you use system libsodium: upgrade to a distro-fixed package (e.g., Ubuntu lists the issue as fixed for multiple releases). [4]
Notes on exploitability
- Some ecosystems note you’re likely unaffected unless you call this low-level point-validation path directly (i.e., not just using high-level signature APIs). [5]
Sources: NVD entry for CVE-2025-69277 [1], GitLab advisory impact statement [2], PyNaCl changelog (1.6.2 fix) [3], Ubuntu CVE page [4], additional technical context from Haskell Cryptography Group [5].
🏁 Script executed:
rg -i "pynacl\|nacl" --type pyRepository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
cat -n ./Minichain_v0/requirements.txtRepository: StabilityNexus/MiniChain
Length of output: 165
🏁 Script executed:
cat -n ./requirements.txtRepository: StabilityNexus/MiniChain
Length of output: 109
🏁 Script executed:
rg "import" -t py | head -50Repository: StabilityNexus/MiniChain
Length of output: 2324
🏁 Script executed:
rg "nacl|crypto_core_ed25519|signature\|Sign" -iRepository: StabilityNexus/MiniChain
Length of output: 1265
🏁 Script executed:
find . -name "*.py" -type f | head -20Repository: StabilityNexus/MiniChain
Length of output: 499
Update PyNaCl to >=1.6.2 in Minichain_v0/requirements.txt and setup.py.
CVE-2025-69277 (Medium, CVSS 4.5) affects PyNaCl < 1.6.2 due to a libsodium Ed25519 point-validation flaw. MiniChain uses high-level signing APIs (SigningKey, VerifyKey) which are not directly impacted by this vulnerability—the CVE primarily affects custom cryptography calling the low-level crypto_core_ed25519_is_valid_point() function. However, upgrading to PyNaCl 1.6.2 for defense-in-depth is still recommended. Note: the root ./requirements.txt already specifies pynacl==1.6.2; Minichain_v0/requirements.txt and setup.py should be aligned.
📦 Proposed fixes
Minichain_v0/requirements.txt:
-pynacl>=1.5.0
+pynacl>=1.6.2setup.py:
-"PyNaCl>=1.5.0",
+"PyNaCl>=1.6.2",📝 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
| pynacl>=1.5.0 | |
| pynacl>=1.6.2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/requirements.txt` at line 3, Update PyNaCl to at least 1.6.2 in
both dependency declarations: change the version specifier in
Minichain_v0/requirements.txt (the existing "pynacl>=1.5.0" entry) to
"pynacl>=1.6.2" and update the PyNaCl entry in setup.py (the install_requires or
requirements list used by setup()/setup.cfg) to match "pynacl>=1.6.2" so both
sources align with the root requirements.txt.
Comment on lines
+33
to
+37
| def apply_tx(state: State, tx: Transaction) -> State: | ||
| """ | ||
| Apply transaction to state and return new state. | ||
| Raises ValueError if transaction is invalid. | ||
| This is the core state transition function of the blockchain. |
Contributor
There was a problem hiding this comment.
Misleading docstring: apply_tx mutates state in place, it does not return a copy.
The docstring says "Apply transaction to state and return new state", but state is mutated in place and the same object is returned. This can confuse callers who expect the original reference to remain unchanged.
📝 Suggested clarification
def apply_tx(state: State, tx: Transaction) -> State:
"""
- Apply transaction to state and return new state.
+ Apply transaction to state in place and return the mutated state.
Raises ValueError if transaction is invalid.
This is the core state transition function of the blockchain.
"""📝 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 apply_tx(state: State, tx: Transaction) -> State: | |
| """ | |
| Apply transaction to state and return new state. | |
| Raises ValueError if transaction is invalid. | |
| This is the core state transition function of the blockchain. | |
| def apply_tx(state: State, tx: Transaction) -> State: | |
| """ | |
| Apply transaction to state in place and return the mutated state. | |
| Raises ValueError if transaction is invalid. | |
| This is the core state transition function of the blockchain. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/state.py` around lines 33 - 37, The docstring for apply_tx is
misleading because apply_tx(State, Transaction) mutates the passed-in State
object in place and returns the same object; update the apply_tx function
docstring to clearly state it performs in-place mutation of the provided State
and returns the same (mutated) State object (or, if you prefer non-mutating
semantics, implement a copy-of-State at the start and apply mutations to the
copy and return that) — refer to the apply_tx function and the State and
Transaction types when making the change.
Comment on lines
+39
to
+42
| # Genesis transaction: just create receiver account | ||
| if tx.sender == "0" * 64: | ||
| state.create_account(tx.receiver, tx.amount) | ||
| return state |
Contributor
There was a problem hiding this comment.
Critical: mining rewards after the first block are silently discarded.
Both genesis and coinbase transactions share sender == "0" * 64, and both are dispatched to create_account(tx.receiver, tx.amount). create_account is a no-op when the account already exists (if address not in self.accounts), so every coinbase transaction for a miner who has already mined once will be silently dropped — the miner's balance never increases past the initial 50 coins.
Reproduction path:
- Mine block
#1→create_account(miner, 50)→ miner balance = 50. ✓ - Mine block
#2→create_account(miner, 50)→ account exists → no-op → miner balance stays at 50. ✗
🐛 Proposed fix
# Genesis transaction: just create receiver account
if tx.sender == "0" * 64:
- state.create_account(tx.receiver, tx.amount)
- return state
+ if not state.exists(tx.receiver):
+ state.create_account(tx.receiver, tx.amount)
+ else:
+ state.accounts[tx.receiver]["balance"] += tx.amount
+ return state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/state.py` around lines 39 - 42, The coinbase case (tx.sender ==
"0"*64) currently always calls state.create_account(tx.receiver, tx.amount)
which is a no-op if the account exists, so mining rewards after the first block
are discarded; change that branch to credit the receiver instead of only
creating: if the receiver account does not exist call
state.create_account(tx.receiver, tx.amount) otherwise add tx.amount to the
existing balance (e.g. state.accounts[tx.receiver] += tx.amount or via an
existing credit/add_balance helper), ensuring coinbase rewards accumulate across
blocks.
|
|
||
| # Validate balance | ||
| if state.get_balance(tx.sender) < tx.amount: | ||
| raise ValueError(f"Insufficient balance") |
Contributor
There was a problem hiding this comment.
Remove spurious f prefix — f-string has no placeholders (Ruff F541).
📝 Proposed fix
- raise ValueError(f"Insufficient balance")
+ raise ValueError("Insufficient balance")📝 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
| raise ValueError(f"Insufficient balance") | |
| raise ValueError("Insufficient balance") |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 59-59: Avoid specifying long messages outside the exception class
(TRY003)
[error] 59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/state.py` at line 59, The raise statement uses an unnecessary
f-string: replace the spurious f-prefix in the raise ValueError(f"Insufficient
balance") expression with a normal string literal (raise
ValueError("Insufficient balance")) so it no longer triggers the Ruff F541
warning; locate the exact raise ValueError(f"Insufficient balance") occurrence
in state.py and update it accordingly.
Comment on lines
+68
to
+77
| @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"], | ||
| signature=data.get("signature") | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
from_dict will raise an unclear KeyError on malformed data from peers.
When deserializing network messages, missing keys like "sender" or "amount" will produce a bare KeyError with no context. Since this is called on untrusted network input (via Block.from_dict → Transaction.from_dict), consider wrapping in a try/except or using .get() with validation to give a clearer error and prevent a peer from crashing the node.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Minichain_v0/transaction.py` around lines 68 - 77, Transaction.from_dict
currently accesses required fields directly and will raise an unhelpful KeyError
on malformed peer data; update Transaction.from_dict to validate required keys
(sender, receiver, amount, nonce) by using data.get(...) or checking membership
and wrap the construction in a try/except that catches KeyError/TypeError and
raises a clearer ValueError (or custom exception) with context including the
missing key and the offending input; reference the Transaction.from_dict method
and ensure Block.from_dict callers can handle the new clear exception.
Contributor
|
hi @anuragShingare30 this one is already covered in #16 and merged |
Author
Yeah.... This PR introduces the CLI to interact with nodes and test the core features like mining, p2p connections, transfer... Ig the PR you are pointing runs a script and I tried to keep no folder structure in this!!! |
Contributor
I am working on that , and suppose to produce as asked in the message keeping the folder structures same |
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.
Description
This PR introduces MiniChain v0 - a minimal educational blockchain prototype that embodies MiniChain's philosophy of minimality, education, and research focus.
Screenshots
Closes #15
What's New
Core Implementation
Educational Features
Architecture
Usage
Testing Checklist
Code Quality
File Structure
Next Steps
Future enhancements (separate PRs):
This PR establishes the foundational educational prototype - making MiniChain the "MiniSat of blockchains."
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores