documentation and implementation references for v0 verification#43
Closed
arunabha003 wants to merge 47 commits intoStabilityNexus:mainfrom
Closed
documentation and implementation references for v0 verification#43arunabha003 wants to merge 47 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 47 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughThis pull request introduces the complete MiniChain v0 blockchain implementation, adding cryptographic primitives, transaction/block models, Proof-of-Work consensus, chain management with fork handling, persistent storage, peer-to-peer networking, a transaction mempool, and a comprehensive CLI interface alongside configuration tooling and extensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Miner
participant Node
participant ChainManager
participant Mempool
participant Network
participant Peers
Miner->>Node: mine_one_block()
Node->>Mempool: get_transactions_for_mining()
Mempool-->>Node: [tx1, tx2, ...]
Node->>Node: build_candidate_block(mempool_txs)
Node->>Node: mine_block_header(candidate)
Node->>Node: Mine nonce until PoW valid
Node->>ChainManager: add_block(mined_block)
ChainManager->>ChainManager: Validate consensus, replay state
ChainManager-->>Node: "extended" / "reorg"
Node->>Mempool: remove_confirmed_transactions()
Node->>Network: submit_block(mined_block)
Network->>Peers: broadcast block_gossip
Peers->>Peers: Validate, apply to local chain
Peers-->>Network: ack
sequenceDiagram
participant CLI
participant Node
participant Mempool
participant ChainManager
participant Network
participant Peer
CLI->>CLI: Parse args (submit-tx --private-key ... --recipient ... --amount ...)
CLI->>Node: Derive sender from private_key
CLI->>Node: submit_transaction(Transaction{sender, recipient, amount, nonce, fee, ...})
Node->>Mempool: add_transaction(signed_tx)
Mempool->>Mempool: Validate signature, nonce, balance
Mempool-->>Node: OK
alt mine-now flag
CLI->>Node: mine_one_block()
Node->>Mempool: get_transactions_for_mining()
Node->>ChainManager: add_block(mined_block)
ChainManager-->>Node: "extended"
end
Node->>Network: submit_transaction(tx)
Network->>Peer: gossip transaction
Peer->>Peer: Mempool.add_transaction()
Peer-->>Network: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 36
♻️ Duplicate comments (1)
tests/test_block_gossip.py (1)
119-135: Duplicated_build_manager— same comment as intest_chain_sync.py.This is identical to the
_build_managerintests/test_chain_sync.py. See the earlier comment about extracting shared helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_block_gossip.py` around lines 119 - 135, The _build_manager helper is duplicated; extract the function into a single shared test helper module and import it from both tests instead of duplicating; move the logic that calls create_genesis_state with GenesisConfig and returns a ChainManager configured with ChainConfig into that shared helper (preserve the function name _build_manager or a clear alternative), update both tests to import the helper, and ensure type hints/returns reference ChainManager so existing uses still work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 20-23: Add a pip cache step before the "Install dependencies" step
in the CI workflow: use actions/cache to cache pip's download cache (e.g.,
~/.cache/pip) and create a cache key that includes the hash of pyproject.toml so
cache invalidates when dependencies change, and add sensible restore-keys; keep
the existing "Install dependencies" run block (python -m pip install --upgrade
pip / python -m pip install -e .[dev]) unchanged so it benefits from the cache
when available.
In `@Makefile`:
- Line 3: Add a new clean target to the Makefile and include it in the .PHONY
list on the existing .PHONY: line so checkmake's minphony warning is satisfied;
implement clean to remove common generated/artifact directories that are
gitignored (e.g., node_modules, dist, build, .venv, .pytest_cache, .mypy_cache,
coverage files) using recursive removal so local artifacts are purged without
touching tracked files, and ensure the target is idempotent and safe to run from
the project root.
In `@pyproject.toml`:
- Around line 17-19: Replace the incorrect PyPI package name in the extras list:
in the network extras array (the entry currently "py-libp2p>=0.2.0"), change the
dependency string to the actual PyPI package name "libp2p" (e.g. "libp2p>=0.2.0"
or the desired version range) so pip can find and install the package when using
the network extras.
In `@README.md`:
- Around line 145-147: The README references docs/issues.md and
docs/architectureProposal.md but those files are gitignored (the docs/ directory
is ignored), so a fresh clone won't have them; either remove these two lines
from README.md (delete the bullets referencing docs/issues.md and
docs/architectureProposal.md) or add committed copies of those documents (create
docs/issues.md and docs/architectureProposal.md and remove the docs/ entry from
.gitignore or force-add the files and update .gitignore accordingly) so the
referenced paths actually exist; update README.md to point to the chosen
location (or delete the references) and ensure the unique filenames
docs/issues.md and docs/architectureProposal.md reflect the change.
In `@src/minichain/__main__.py`:
- Around line 241-253: In _run_mine validate the optional max_transactions
parameter before using it: if max_transactions is not None ensure it's an int >
0 (reject zero or negative values) and raise ValueError with a clear message;
update the beginning of the _run_mine function (the parameters validated there)
so invalid max_transactions are rejected early rather than propagated into
node.mine_one_block.
In `@src/minichain/block.py`:
- Around line 29-34: The methods Block.hash() and BlockHeader.hash() shadow
Python's built-in hash() and return bytes; rename both to a non-colliding name
like compute_hash() or block_hash() (e.g., Block.compute_hash() and
BlockHeader.compute_hash()), update all call sites to use the new names, and
keep their return type as bytes; if instances must be usable as dict keys/sets,
implement __hash__ on the classes to return an int (e.g., an int derived from
the canonical bytes) rather than relying on the renamed methods.
In `@src/minichain/chain.py`:
- Around line 98-134: add_block currently calls _replay_state_for_tip for every
new block which replays from genesis and makes extensions O(n); change the logic
so that when parent_hash == self._tip_hash (the simple extension path) you apply
only the new block's state transitions to a copy of the current self.state (or
otherwise atomically update state) instead of calling _replay_state_for_tip, and
reserve calling _replay_state_for_tip(block_hash) only for fork/reorg cases
(when parent_is_tip is False or candidate_height > canonical_height) to validate
and rebuild the canonical path; ensure you still insert/remove from
self._blocks_by_hash and self._heights on error as before and keep the same
return strings ("extended", "reorg", "stored_fork", "duplicate").
In `@src/minichain/consensus.py`:
- Line 7: The import currently uses typing.Sequence; update the import to use
collections.abc.Sequence instead to follow modern Python conventions (Ruff
UP035). Replace "from typing import Sequence" with an import from
collections.abc (retain any existing annotations and compatibility with from
__future__ import annotations) so all uses of Sequence in this module (e.g.,
type hints in consensus.py) reference collections.abc.Sequence.
In `@src/minichain/genesis.py`:
- Around line 56-74: apply_genesis_block currently only validates height,
previous_hash, merkle_root and transactions but does not verify that the
block.header fields match the supplied GenesisConfig; update apply_genesis_block
to validate block.header.version, block.header.timestamp,
block.header.difficulty_target (and block.header.nonce if present in
GenesisConfig) against the corresponding values in the GenesisConfig (e.g.,
config.version, config.timestamp, config.difficulty_target, config.nonce) and
raise ValueError with a clear message when any mismatch occurs, placing these
checks after existing header validations and before applying
config.initial_balances so the genesis header must exactly match the config.
In `@src/minichain/mempool.py`:
- Around line 108-145: The loop in get_transactions_for_mining mutates
_sender_pools by calling _recompute_sender_pool (which may delete senders),
causing a "dictionary changed size" error; to fix, iterate over a snapshot
(e.g., list(self._sender_pools.keys()) or list(self._sender_pools.items())),
call _recompute_sender_pool(sender, state), then re-fetch the pool with
self._sender_pools.get(sender) and skip if None/empty before accessing
pool.entries or pool.ready_nonces; ensure sender_ready is only populated from
the re-fetched pool.
In `@src/minichain/mining.py`:
- Around line 34-35: The two separate reads parent = chain_manager.tip_block and
parent_hash = chain_manager.tip_hash can race with concurrent
ChainManager.add_block; change ChainManager to provide a single atomic accessor
(e.g., a method/property like tip() or tip_pair() that returns (block, hash)
together) and update mining.py to call that single accessor (replace uses of
tip_block and tip_hash with the tuple result) so both values are consistent, or
if you prefer, document and assert the single-threaded assumption in
ChainManager and gating code; ensure references to ChainManager.add_block,
tip_block, and tip_hash are updated accordingly.
In `@src/minichain/network.py`:
- Around line 429-442: The peer reader loop currently swallows all exceptions in
_peer_reader_loop which hides disconnect causes; update the except block to
capture the exception (except Exception as e) and surface it before closing the
connection by logging the full error/stack trace using the instance logger (e.g.
self._logger.exception or self._logger.error with stack info), include the
peer_id and context (reading via _read_message/_handle_peer_message) in the log,
and then proceed to call _close_connection(peer_id); if no logger exists, print
the error with peer_id and stack instead.
- Around line 943-973: The _read_message function currently always uses
connect_timeout_seconds which causes steady-state peer readers in
_start_peer_reader to drop idle peers; modify _read_message(reader, *, eof_ok:
bool = False) to accept an optional timeout: float | None = None and only wrap
reader.readline() with asyncio.wait_for when timeout is not None; update the
handshake call sites that expect a short timeout to call _read_message(...,
timeout=self.config.connect_timeout_seconds) and change the continuous reader
loop in _start_peer_reader to call _read_message(..., timeout=None) (or omit the
timeout argument) so normal idle reads do not use the connect timeout.
In `@src/minichain/node.py`:
- Around line 92-125: The start() method currently creates
SQLiteStorage(db_path) and then calls _initialize_chain_manager which may raise,
leaking the open storage handle; fix by surrounding the initialization of
chain_manager (and subsequent mempool setup) with a try/except/finally that
closes storage on any exception (call storage.close()) before re-raising the
error, and only assign self._storage, self._chain_manager, self._mempool and set
self._running = True after successful initialization; alternatively, open
SQLiteStorage in a context that guarantees storage.close() on failure —
reference the start() function, SQLiteStorage, _initialize_chain_manager,
storage, and self._storage when making the change.
In `@src/minichain/serialization.py`:
- Around line 8-25: The tuple constants TRANSACTION_FIELD_ORDER and
BLOCK_HEADER_FIELD_ORDER are misnamed — they function as required-field
whitelists for validation, not as output ordering guides (json.dumps(...,
sort_keys=True) forces alphabetical key order); rename them to
TRANSACTION_REQUIRED_FIELDS and BLOCK_HEADER_REQUIRED_FIELDS and update all
usages (e.g., any validations or references in _to_field_map or similar
functions) to use the new names, and add a short inline comment above each
constant stating that these are required-field whitelists and that
sort_keys=True dictates JSON key order.
- Line 6: Replace the deprecated typing.Mapping import with
collections.abc.Mapping: update the import line that currently reads "from
typing import Any, Mapping" so it imports Mapping from collections.abc (e.g.,
"from typing import Any" and "from collections.abc import Mapping" or combine
accordingly) to remove use of typing.Mapping; keep Any as-is and ensure any
references to Mapping in functions/classes (e.g., parameter/return type
annotations) continue to work.
- Around line 31-44: The object-vs-Mapping asymmetry: when building source in
the else branch you only pick fields in field_order so extra attributes on
objects never trigger the extras check; update the else branch that creates
source for non-Mapping inputs (where field_order and source are used) to also
collect the object's attribute keys (e.g., via vars(value) / dir(value) /
value.__dict__) and compare them against field_order, raising
ValueError("Unexpected fields: ...") on extra attributes, while preserving the
existing missing-field check and final ordered return.
In `@src/minichain/state.py`:
- Around line 45-72: The apply_transaction method currently calls get_account
which eagerly creates accounts and may mutate state on failed validation; change
apply_transaction to first lookup existing accounts using
self.accounts.get(transaction.sender) and
self.accounts.get(transaction.recipient) (or equivalent) into local variables,
perform all validations (is_coinbase, verify, nonce, sufficient balance) without
creating or mutating Account objects, and only after all checks pass
create/insert missing accounts into self.accounts and then apply the state
changes (debit sender, increment sender.nonce, credit recipient), making sure to
handle the sender==recipient case so you don't double-create or double-apply
balances; raise StateTransitionError as before on validation failures.
In `@src/minichain/storage.py`:
- Around line 148-171: The current save_state method deletes all rows and
re-inserts every account each time, which is O(n) and will be slow at scale;
modify save_state (or add a new method) to perform incremental updates by
tracking dirty accounts: on save, validate addresses and values as before, then
for each dirty address use INSERT OR REPLACE / UPDATE for changed accounts and
DELETE only for addresses explicitly removed (or batch upserts and deletes)
instead of truncating the entire accounts table; keep the existing connection
handling logic in save_state and add a dirty-tracking mechanism in State (or
accept a list/set of changed addresses) so only modified accounts are persisted.
- Line 228: Replace the explicit dict comprehension assigning kv with a direct
call to dict(rows); specifically, in the assignment where kv is built from rows
(kv = {key: value for key, value in rows}), use kv = dict(rows) to simplify and
satisfy Ruff C416 while preserving behavior.
- Around line 28-32: In Storage.__init__, after creating self._connection
(sqlite3.connect) and before _initialize_schema, enable WAL journal mode to
improve concurrent read performance by executing the PRAGMA "journal_mode=WAL"
on self._connection (and optionally set PRAGMA synchronous=NORMAL for a balance
of durability and speed); update the code around the
self._connection.execute("PRAGMA foreign_keys = ON") line to run the
journal_mode PRAGMA on the same connection so reads aren't blocked by writes
during mining/syncing.
- Around line 19-22: Create a shared utility module (e.g., minichain.validation
or minichain.utils) that exports the helper function _is_lower_hex(value: str,
expected_length: int) -> bool, then remove the duplicate implementations from
__main__.py, transaction.py, storage.py, node.py, mining.py, and genesis.py and
replace them with imports from the new module (e.g., from minichain.validation
import _is_lower_hex). Ensure the new module uses the same function signature
and behavior so callers like any references in Storage class or transaction
validation keep working unchanged.
- Around line 46-56: The DB keeps old canonical-height blocks after a reorg
because persist_block_state_and_metadata only writes the new tip; update
persistence so reorgs atomically replace conflicting-height rows: either (A)
change persist_block_state_and_metadata to accept and persist the full new
canonical block path (not just the tip) or (B) after detecting a reorg delete
any rows in blocks whose height is >= fork_height and whose hash is not on the
new canonical path before inserting the new tip; ensure get_block_by_height
continues to return only canonical-path blocks by making the
deletion/persistence atomic with the tip insert to avoid replay loading stale
blocks.
In `@src/minichain/transaction.py`:
- Around line 73-80: The transaction_id function currently computes a
deterministic hash from signing_bytes plus optional signature/public_key, which
for coinbase transactions (where signature and public_key are empty) yields
identical IDs for identical payloads; update the docstring of transaction_id to
explicitly state that coinbase transactions omit signature/public_key and thus
can produce identical transaction_id values, and reference the is_coinbase and
validate_coinbase checks that must be relied upon (or suggest adding a unique
coinbase nonce if uniqueness across identical payloads is later required).
- Around line 97-103: The sign() method currently allows signing with a key that
doesn't match self.sender; add an early check after obtaining verify_key (from
signing_key.verify_key) that assert/validate derive_address(verify_key) ==
self.sender and raise a ValueError with a clear message like "Signing key does
not match transaction sender" if it doesn't match; also tighten the type
annotation on signing_key to use nacl.signing.SigningKey (or an explicit
SigningKey protocol) and add the necessary import so static type checkers catch
misuse, leaving the rest of the existing logic (serialize_verify_key,
sign_message, signing_bytes) unchanged.
In `@tests/test_chain_sync.py`:
- Around line 91-103: In _mine_blocks, avoid allocating a new Mempool() each
loop iteration; create a single Mempool instance before the for loop and pass
that same instance into build_candidate_block on each iteration so
build_candidate_block(chain_manager=manager, mempool=<single_mempool>, ...)
reuses the mempool while leaving the rest of the logic (timestamp calc,
mine_candidate_block, manager.add_block and assert) unchanged.
- Around line 72-88: The tests duplicate helper functions like _build_manager
and _signed_transaction across multiple test files; extract these into a shared
test module (e.g. tests/conftest.py or tests/helpers.py) as fixtures or helper
functions, implement parameterized fixtures for variants (e.g., allow overriding
ChainConfig params for _build_manager and transaction parameters for
_signed_transaction), replace local definitions in tests/test_chain_sync.py,
tests/test_block_gossip.py, tests/test_mining.py, and tests/test_node.py with
imports or pytest fixtures, and update tests to call the new fixture names or
helper functions to remove duplication.
In `@tests/test_comprehensive_integration.py`:
- Around line 52-67: The two _wait_until calls inside the mining loop should be
made robust to late-binding and explicit about timeout semantics: call
_wait_until(..., strict=True) and capture the loop variable into each lambda
(e.g., use lambda expected_height=expected_height: ...) so the predicates
reference the correct expected_height; update both predicates that reference
expected_height and node state (the lambda checking node.manager.height and the
lambda checking tip_hash/height) and keep the calls around the existing
_mine_and_broadcast/miners/nodes loop.
In `@tests/test_consensus.py`:
- Around line 5-13: Replace the manual try/except exception assertions in the
consensus tests with pytest.raises: import pytest at top and wrap the code
expected to raise MiningInterrupted (and any other exception checks between the
blocks around the current imports and the later section ~76-96) in a with
pytest.raises(MiningInterrupted): context to assert the exception instead of
using Event/try/except boilerplate; update the test that currently triggers
mine_block_header/is_valid_pow to use this context and remove the manual failure
assertions so the tests use pytest idiomatic exception checking.
In `@tests/test_crypto.py`:
- Around line 40-47: The test function test_invalid_signature_is_rejected
unpacks signing_key from generate_key_pair() but never uses it, triggering
RUF059; rename the unused variable signing_key to _signing_key (or _) in the
test_invalid_signature_is_rejected function so the lint rule is satisfied while
leaving the rest of the test (other_signing_key, message, wrong_signature,
verify_signature, verify_key) unchanged.
In `@tests/test_genesis.py`:
- Around line 60-71: Replace the manual try/except/else exception checks in
test_genesis_requires_empty_state (and the similar block at lines 74-85) with
pytest's context manager: use with pytest.raises(ValueError, match="empty
state") and call apply_genesis_block inside that context; also ensure pytest is
imported at top of tests/test_genesis.py so the test uses pytest.raises rather
than the manual try/except/else pattern.
In `@tests/test_mempool.py`:
- Around line 122-124: The compound assertions using "and" make failures
ambiguous; replace each compound assert on selected[2] and selected[3] with two
separate assertions: one that checks the sender (selected[2].sender equals
sender_a, and selected[3].sender equals sender_a) and one that checks the nonce
(selected[2].nonce equals 0, and selected[3].nonce equals 1) so test failures
indicate which specific condition failed.
In `@tests/test_node.py`:
- Around line 40-41: In the three test functions
test_node_start_initializes_and_persists_genesis,
test_node_mine_block_then_reload_from_disk, and
test_accept_block_persists_chain_head, change the tmp_path parameter type
annotation from pytest.TempPathFactory to pathlib.Path (and add or ensure "from
pathlib import Path" is imported if missing); update the signature for each
function to use Path (tmp_path: Path) so the fixture type matches pytest's
tmp_path fixture semantics.
In `@tests/test_serialization.py`:
- Around line 12-34: Add a symmetric determinism test for block headers similar
to test_transaction_serialization_is_deterministic: create two dicts (e.g.,
hdr_a and hdr_b) with identical block-header fields in different key orders,
call serialize_block_header(hdr_a) and serialize_block_header(hdr_b), assert the
two results are equal and that the serialized bytes contain no spaces (b" " not
in ...); name the new test test_block_header_serialization_is_deterministic and
place it alongside the existing tests so it verifies key-order independence for
serialize_block_header.
- Around line 68-101: The pytest.mark.parametrize decorator currently passes the
parameter names as a single string; change it to a tuple of names to satisfy
PT006. Update the decorator on test_required_and_unexpected_fields_are_rejected
to use ("payload", "serializer", "expected") (or
('payload','serializer','expected')) instead of "payload,serializer,expected";
no other logic needs to change—this will properly bind the payloads for
serialize_transaction and serialize_block_header cases.
In `@tests/test_storage.py`:
- Line 59: The tmp_path fixture is incorrectly annotated as
pytest.TempPathFactory; update the type hints for the tmp_path parameter in
test_store_and_load_block_round_trip and the other two test functions in this
file to use pathlib.Path (or Path if you prefer from pathlib import Path), and
add the appropriate import if missing so linters/type-checkers recognize the
correct fixture type.
---
Duplicate comments:
In `@tests/test_block_gossip.py`:
- Around line 119-135: The _build_manager helper is duplicated; extract the
function into a single shared test helper module and import it from both tests
instead of duplicating; move the logic that calls create_genesis_state with
GenesisConfig and returns a ChainManager configured with ChainConfig into that
shared helper (preserve the function name _build_manager or a clear
alternative), update both tests to import the helper, and ensure type
hints/returns reference ChainManager so existing uses still work.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/ci.yml.gitignoreMakefileREADME.mdimplementation.mdpyproject.tomlsrc/minichain/__init__.pysrc/minichain/__main__.pysrc/minichain/block.pysrc/minichain/chain.pysrc/minichain/consensus.pysrc/minichain/crypto.pysrc/minichain/genesis.pysrc/minichain/mempool.pysrc/minichain/merkle.pysrc/minichain/mining.pysrc/minichain/network.pysrc/minichain/node.pysrc/minichain/serialization.pysrc/minichain/state.pysrc/minichain/storage.pysrc/minichain/transaction.pytests/test_block.pytests/test_block_gossip.pytests/test_chain.pytests/test_chain_sync.pytests/test_cli.pytests/test_comprehensive_integration.pytests/test_consensus.pytests/test_crypto.pytests/test_genesis.pytests/test_mempool.pytests/test_merkle.pytests/test_mining.pytests/test_network.pytests/test_node.pytests/test_scaffold.pytests/test_serialization.pytests/test_state.pytests/test_storage.pytests/test_transaction.pytests/test_transaction_gossip.py
Comment on lines
+20
to
+23
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install -e .[dev] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding pip dependency caching to speed up CI.
Each run currently re-downloads PyNaCl, pytest, and ruff from scratch. The cache key can use the hash of pyproject.toml.
⚡ Proposed addition for pip caching
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
+ cache: "pip"
- name: Install dependencies🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 20 - 23, Add a pip cache step before
the "Install dependencies" step in the CI workflow: use actions/cache to cache
pip's download cache (e.g., ~/.cache/pip) and create a cache key that includes
the hash of pyproject.toml so cache invalidates when dependencies change, and
add sensible restore-keys; keep the existing "Install dependencies" run block
(python -m pip install --upgrade pip / python -m pip install -e .[dev])
unchanged so it benefits from the cache when available.
| @@ -0,0 +1,21 @@ | |||
| PYTHON ?= python3 | |||
|
|
|||
| .PHONY: install dev-install test lint format start-node | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a clean target for hygiene.
A clean target removes generated artifacts that are already gitignored but may accumulate locally. This is a standard Makefile convention and also satisfies the checkmake minphony warning.
🧹 Proposed `clean` target
-.PHONY: install dev-install test lint format start-node
+.PHONY: install dev-install test lint format start-node clean
+
+clean:
+ find . -type d -name __pycache__ -exec rm -rf {} +
+ rm -rf .pytest_cache .ruff_cache🧰 Tools
🪛 checkmake (0.2.2)
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 3, Add a new clean target to the Makefile and include it in
the .PHONY list on the existing .PHONY: line so checkmake's minphony warning is
satisfied; implement clean to remove common generated/artifact directories that
are gitignored (e.g., node_modules, dist, build, .venv, .pytest_cache,
.mypy_cache, coverage files) using recursive removal so local artifacts are
purged without touching tracked files, and ensure the target is idempotent and
safe to run from the project root.
Comment on lines
+17
to
+19
| network = [ | ||
| "py-libp2p>=0.2.0", | ||
| ] |
Contributor
There was a problem hiding this comment.
Wrong PyPI package name: py-libp2p does not exist on PyPI — use libp2p.
The installable PyPI package is named libp2p (hosted at pypi.org/project/libp2p/). py-libp2p is the GitHub repository name; the published PyPI versions (including 0.2.0, 0.3.0, 0.4.0, 0.5.0) are all distributed as libp2p. Running pip install -e .[network] as documented will fail with a package-not-found error.
🐛 Proposed fix
[project.optional-dependencies]
network = [
- "py-libp2p>=0.2.0",
+ "libp2p>=0.2.0",
]📝 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
| network = [ | |
| "py-libp2p>=0.2.0", | |
| ] | |
| network = [ | |
| "libp2p>=0.2.0", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 17 - 19, Replace the incorrect PyPI package name
in the extras list: in the network extras array (the entry currently
"py-libp2p>=0.2.0"), change the dependency string to the actual PyPI package
name "libp2p" (e.g. "libp2p>=0.2.0" or the desired version range) so pip can
find and install the package when using the network extras.
Comment on lines
+145
to
+147
| - `implementation.md`: implementation details for all components | ||
| - `docs/issues.md`: issue-by-issue roadmap | ||
| - `docs/architectureProposal.md`: full architecture proposal |
Contributor
There was a problem hiding this comment.
docs/issues.md and docs/architectureProposal.md are gitignored and will not exist in a fresh clone.
.gitignore line 340 ignores the entire docs/ directory. Any reader who clones the repo and tries to follow these references will find neither file. Either:
- Remove these two lines from the README (they are local planning artifacts, not shipped docs), or
- Move the relevant content into committed files (e.g., the repo root or a committed
docs/path that is explicitlygit add -f'd).
implementation.md at the root level is unaffected and will be committed correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 145 - 147, The README references docs/issues.md and
docs/architectureProposal.md but those files are gitignored (the docs/ directory
is ignored), so a fresh clone won't have them; either remove these two lines
from README.md (delete the bullets referencing docs/issues.md and
docs/architectureProposal.md) or add committed copies of those documents (create
docs/issues.md and docs/architectureProposal.md and remove the docs/ entry from
.gitignore or force-add the files and update .gitignore accordingly) so the
referenced paths actually exist; update README.md to point to the chosen
location (or delete the references) and ensure the unique filenames
docs/issues.md and docs/architectureProposal.md reflect the change.
Comment on lines
+241
to
+253
| def _run_mine( | ||
| *, | ||
| node: MiniChainNode, | ||
| count: int, | ||
| max_transactions: int | None, | ||
| ) -> None: | ||
| if count <= 0: | ||
| raise ValueError("count must be positive") | ||
| for index in range(1, count + 1): | ||
| block = node.mine_one_block(max_transactions=max_transactions) | ||
| print( | ||
| f"mined_block_{index}=height:{block.header.block_height},hash:{block.hash().hex()}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Validate max_transactions when provided.
A negative or zero value can propagate into mining logic with undefined behavior; reject it early.
🛠️ Suggested fix
def _run_mine(
@@
if count <= 0:
raise ValueError("count must be positive")
+ if max_transactions is not None and max_transactions <= 0:
+ raise ValueError("max_transactions must be positive")
for index in range(1, count + 1):
block = node.mine_one_block(max_transactions=max_transactions)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 248-248: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/minichain/__main__.py` around lines 241 - 253, In _run_mine validate the
optional max_transactions parameter before using it: if max_transactions is not
None ensure it's an int > 0 (reject zero or negative values) and raise
ValueError with a clear message; update the beginning of the _run_mine function
(the parameters validated there) so invalid max_transactions are rejected early
rather than propagated into node.mine_one_block.
Comment on lines
+122
to
+124
| assert [tx.fee for tx in selected] == [8, 4, 1, 10] | ||
| assert selected[2].sender == sender_a and selected[2].nonce == 0 | ||
| assert selected[3].sender == sender_a and selected[3].nonce == 1 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Break compound assertions into separate statements for clearer failure messages.
Per the Ruff PT018 hint, compound assert ... and ... statements make it hard to tell which condition failed. Split them into individual assertions.
♻️ Proposed fix
- assert selected[2].sender == sender_a and selected[2].nonce == 0
- assert selected[3].sender == sender_a and selected[3].nonce == 1
+ assert selected[2].sender == sender_a
+ assert selected[2].nonce == 0
+ assert selected[3].sender == sender_a
+ assert selected[3].nonce == 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
| assert [tx.fee for tx in selected] == [8, 4, 1, 10] | |
| assert selected[2].sender == sender_a and selected[2].nonce == 0 | |
| assert selected[3].sender == sender_a and selected[3].nonce == 1 | |
| assert [tx.fee for tx in selected] == [8, 4, 1, 10] | |
| assert selected[2].sender == sender_a | |
| assert selected[2].nonce == 0 | |
| assert selected[3].sender == sender_a | |
| assert selected[3].nonce == 1 |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 123-123: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 124-124: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mempool.py` around lines 122 - 124, The compound assertions using
"and" make failures ambiguous; replace each compound assert on selected[2] and
selected[3] with two separate assertions: one that checks the sender
(selected[2].sender equals sender_a, and selected[3].sender equals sender_a) and
one that checks the nonce (selected[2].nonce equals 0, and selected[3].nonce
equals 1) so test failures indicate which specific condition failed.
Comment on lines
+40
to
+41
| def test_node_start_initializes_and_persists_genesis( | ||
| tmp_path: pytest.TempPathFactory, |
Contributor
There was a problem hiding this comment.
Incorrect type annotation for tmp_path fixture — same issue as test_storage.py.
All three test functions annotate tmp_path as pytest.TempPathFactory instead of pathlib.Path.
🔧 Proposed fix (apply to all three test functions)
+from pathlib import Path
+
-def test_node_start_initializes_and_persists_genesis(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_node_start_initializes_and_persists_genesis(
+ tmp_path: Path,
+) -> None:Same for test_node_mine_block_then_reload_from_disk and test_accept_block_persists_chain_head.
Also applies to: 72-73, 127-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_node.py` around lines 40 - 41, In the three test functions
test_node_start_initializes_and_persists_genesis,
test_node_mine_block_then_reload_from_disk, and
test_accept_block_persists_chain_head, change the tmp_path parameter type
annotation from pytest.TempPathFactory to pathlib.Path (and add or ensure "from
pathlib import Path" is imported if missing); update the signature for each
function to use Path (tmp_path: Path) so the fixture type matches pytest's
tmp_path fixture semantics.
Comment on lines
+12
to
+34
| def test_transaction_serialization_is_deterministic() -> None: | ||
| tx_a = { | ||
| "sender": "a1" * 20, | ||
| "recipient": "b2" * 20, | ||
| "amount": 25, | ||
| "nonce": 1, | ||
| "fee": 2, | ||
| "timestamp": 1_739_749_000, | ||
| } | ||
| tx_b = { | ||
| "timestamp": 1_739_749_000, | ||
| "fee": 2, | ||
| "nonce": 1, | ||
| "amount": 25, | ||
| "recipient": "b2" * 20, | ||
| "sender": "a1" * 20, | ||
| } | ||
|
|
||
| serialized_a = serialize_transaction(tx_a) | ||
| serialized_b = serialize_transaction(tx_b) | ||
|
|
||
| assert serialized_a == serialized_b | ||
| assert b" " not in serialized_a |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Block-header key-order independence is untested — add a symmetric determinism test.
test_transaction_serialization_is_deterministic verifies that two dicts with shuffled keys produce identical bytes. There is no equivalent for block headers; test_changing_block_header_field_changes_serialization only tests mutation, not key-order independence. Since block header hashes are consensus-critical, the same invariant should be exercised.
🧪 Suggested additional test
def test_block_header_serialization_is_deterministic() -> None:
hdr_a = {
"version": 0,
"previous_hash": "00" * 32,
"merkle_root": "11" * 32,
"timestamp": 123_456_789,
"difficulty_target": 1_000_000,
"nonce": 7,
"block_height": 3,
}
hdr_b = {
"block_height": 3,
"nonce": 7,
"difficulty_target": 1_000_000,
"timestamp": 123_456_789,
"merkle_root": "11" * 32,
"previous_hash": "00" * 32,
"version": 0,
}
assert serialize_block_header(hdr_a) == serialize_block_header(hdr_b)
assert b" " not in serialize_block_header(hdr_a)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_serialization.py` around lines 12 - 34, Add a symmetric
determinism test for block headers similar to
test_transaction_serialization_is_deterministic: create two dicts (e.g., hdr_a
and hdr_b) with identical block-header fields in different key orders, call
serialize_block_header(hdr_a) and serialize_block_header(hdr_b), assert the two
results are equal and that the serialized bytes contain no spaces (b" " not in
...); name the new test test_block_header_serialization_is_deterministic and
place it alongside the existing tests so it verifies key-order independence for
serialize_block_header.
Comment on lines
+68
to
+101
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", | ||
| [ | ||
| ( | ||
| { | ||
| "sender": "aa" * 20, | ||
| "recipient": "bb" * 20, | ||
| "amount": 1, | ||
| "nonce": 1, | ||
| "timestamp": 1, | ||
| }, | ||
| serialize_transaction, | ||
| "Missing required fields: fee", | ||
| ), | ||
| ( | ||
| { | ||
| "version": 0, | ||
| "previous_hash": "00" * 32, | ||
| "merkle_root": "11" * 32, | ||
| "timestamp": 1, | ||
| "difficulty_target": 1, | ||
| "nonce": 1, | ||
| "block_height": 1, | ||
| "extra": "x", | ||
| }, | ||
| serialize_block_header, | ||
| "Unexpected fields: extra", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_required_and_unexpected_fields_are_rejected( | ||
| payload: dict[str, object], serializer: Callable[[dict[str, object]], bytes], expected: str | ||
| ) -> None: | ||
| with pytest.raises(ValueError, match=expected): |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a tuple for the pytest.mark.parametrize parameter names argument (PT006).
♻️ Proposed fix
`@pytest.mark.parametrize`(
- "payload,serializer,expected",
+ ("payload", "serializer", "expected"),📝 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
| @pytest.mark.parametrize( | |
| "payload,serializer,expected", | |
| [ | |
| ( | |
| { | |
| "sender": "aa" * 20, | |
| "recipient": "bb" * 20, | |
| "amount": 1, | |
| "nonce": 1, | |
| "timestamp": 1, | |
| }, | |
| serialize_transaction, | |
| "Missing required fields: fee", | |
| ), | |
| ( | |
| { | |
| "version": 0, | |
| "previous_hash": "00" * 32, | |
| "merkle_root": "11" * 32, | |
| "timestamp": 1, | |
| "difficulty_target": 1, | |
| "nonce": 1, | |
| "block_height": 1, | |
| "extra": "x", | |
| }, | |
| serialize_block_header, | |
| "Unexpected fields: extra", | |
| ), | |
| ], | |
| ) | |
| def test_required_and_unexpected_fields_are_rejected( | |
| payload: dict[str, object], serializer: Callable[[dict[str, object]], bytes], expected: str | |
| ) -> None: | |
| with pytest.raises(ValueError, match=expected): | |
| `@pytest.mark.parametrize`( | |
| ("payload", "serializer", "expected"), | |
| [ | |
| ( | |
| { | |
| "sender": "aa" * 20, | |
| "recipient": "bb" * 20, | |
| "amount": 1, | |
| "nonce": 1, | |
| "timestamp": 1, | |
| }, | |
| serialize_transaction, | |
| "Missing required fields: fee", | |
| ), | |
| ( | |
| { | |
| "version": 0, | |
| "previous_hash": "00" * 32, | |
| "merkle_root": "11" * 32, | |
| "timestamp": 1, | |
| "difficulty_target": 1, | |
| "nonce": 1, | |
| "block_height": 1, | |
| "extra": "x", | |
| }, | |
| serialize_block_header, | |
| "Unexpected fields: extra", | |
| ), | |
| ], | |
| ) | |
| def test_required_and_unexpected_fields_are_rejected( | |
| payload: dict[str, object], serializer: Callable[[dict[str, object]], bytes], expected: str | |
| ) -> None: | |
| with pytest.raises(ValueError, match=expected): |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 69-69: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_serialization.py` around lines 68 - 101, The
pytest.mark.parametrize decorator currently passes the parameter names as a
single string; change it to a tuple of names to satisfy PT006. Update the
decorator on test_required_and_unexpected_fields_are_rejected to use ("payload",
"serializer", "expected") (or ('payload','serializer','expected')) instead of
"payload,serializer,expected"; no other logic needs to change—this will properly
bind the payloads for serialize_transaction and serialize_block_header cases.
| return block | ||
|
|
||
|
|
||
| def test_store_and_load_block_round_trip(tmp_path: pytest.TempPathFactory) -> None: |
Contributor
There was a problem hiding this comment.
Incorrect type annotation for tmp_path fixture.
The tmp_path pytest fixture provides a pathlib.Path, not pytest.TempPathFactory. pytest.TempPathFactory is the type for the tmp_path_factory fixture. While this doesn't cause a runtime error (Python doesn't enforce type hints at runtime), it's misleading and will produce false positives in type checkers like mypy/pyright.
🔧 Proposed fix (apply to all three test functions in this file)
+from pathlib import Path
+
-def test_store_and_load_block_round_trip(tmp_path: pytest.TempPathFactory) -> None:
+def test_store_and_load_block_round_trip(tmp_path: Path) -> None:-def test_state_and_metadata_persist_across_restart(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_state_and_metadata_persist_across_restart(
+ tmp_path: Path,
+) -> None:-def test_atomic_persist_rolls_back_on_metadata_failure(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_atomic_persist_rolls_back_on_metadata_failure(
+ tmp_path: Path,
+) -> None:Also applies to: 107-108, 132-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_storage.py` at line 59, The tmp_path fixture is incorrectly
annotated as pytest.TempPathFactory; update the type hints for the tmp_path
parameter in test_store_and_load_block_round_trip and the other two test
functions in this file to use pathlib.Path (or Path if you prefer from pathlib
import Path), and add the appropriate import if missing so linters/type-checkers
recognize the correct fixture type.
This was referenced Feb 24, 2026
Author
|
Superseded by #47 Closing this PR to keep review history in one place. |
1 task
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.
Addressed Issues:
PR body:
Part of #8
Depends on #42
Summary
Validation
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
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
Documentation
Tests
Chores