feat: node orchestration with startup persistence mining and clean shutdown#36
Closed
arunabha003 wants to merge 33 commits intoStabilityNexus:mainfrom
Closed
feat: node orchestration with startup persistence mining and clean shutdown#36arunabha003 wants to merge 33 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 33 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughThis PR introduces the complete MiniChain blockchain implementation in Python, establishing core blockchain primitives, cryptographic utilities, consensus and mining logic, chain management with fork resolution, mempool transaction handling, persistent SQLite storage, state machine, CLI entrypoints, and comprehensive test coverage across all components. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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: 28
🤖 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 15-23: In the CI workflow's "Setup Python" step using
actions/setup-python@v5, add the pip cache option by adding cache: 'pip' under
the with block so dependencies are cached between runs; update the Setup Python
step (named "Setup Python") to include with: python-version: "3.11" and cache:
'pip' to avoid re-downloading packages during the "Install dependencies" run
step.
In `@README.md`:
- Around line 51-71: Update the "## Repository Layout" section in README.md to
either list all newly added modules/tests or explicitly mark the list as
non‑exhaustive; edit the "Repository Layout" block (the section header "##
Repository Layout" and the following code block) to include the new test files
and any new src modules you added (e.g., new test files under tests/ and new
Python modules under src/minichain/), or add a trailing comment like "…
(non‑exhaustive)" so the README no longer omits recent additions.
- Around line 36-43: Update the "make start-node" description in the Common
Commands section so it reflects the current behavior instead of calling it a
"scaffold node entrypoint"; change the comment for the make target (the line
containing "make start-node # ...") to a concise, accurate description of what
make start-node actually does (for example "start local node", "run development
node", or the current runtime behavior), ensuring the update is applied to the
README's Common Commands block where "make start-node" appears.
- Around line 1-15: Update the "Current Status" section text under the README's
"Current Status" header to remove the scaffolding/placeholder language and
accurately list implemented functionality: mention that full node orchestration
and chain implementation are included, enumerate the concrete modules
implemented (crypto, transaction, block, state, mempool, consensus, network,
storage, node), note the Python package layout under src/minichain, existing
tests under tests/, CLI/tools or example scripts if present, and the CI/workflow
and Makefile; ensure the language replaces "placeholder" with clear status
(implemented vs. planned) and briefly summarises what works (e.g., chain
syncing, block/tx processing, basic consensus) so readers know the current
capabilities.
In `@src/minichain/__main__.py`:
- Around line 11-25: build_parser currently defines --host and --port but those
values are never propagated into the node startup, so callers are misled; update
the CLI wiring to either (A) add host and port fields to NodeConfig and pass
args.host/args.port into the Node creation/start path (e.g., where NodeConfig is
constructed and where the node binds sockets), or (B) remove the --host/--port
arguments (or add a clear TODO comment) if networking is not yet implemented.
Locate build_parser and the code that constructs NodeConfig / starts the node
(the NodeConfig constructor and the node start/create call) and ensure the
parsed args.host and args.port are used there (or remove the args) so the CLI
reflects actual behavior.
- Around line 36-41: node.start() is being called outside the try/finally so if
start() raises (for example during _initialize_chain_manager after opening
SQLiteStorage) the finally never runs and the SQLite connection leaks; move the
node.start() call into the try block (or wrap start() in its own try/finally
that always calls node.stop()) so that node.stop() is guaranteed to run on
startup failure; reference node.start(), node.stop(), self._storage and
_initialize_chain_manager when applying the change.
In `@src/minichain/chain.py`:
- Around line 190-205: The consensus check in _validate_consensus currently
omits timestamp validation allowing miners to manipulate difficulty via
timestamps; update _validate_consensus to verify block.header.timestamp is not
earlier than the last parent header's timestamp (use
parent_headers[-1].timestamp) and also enforce an upper bound (e.g.,
block.header.timestamp <= now + allowed_drift) using either an existing config
value (add/ use self.config.max_future_block_time_seconds) or a defined
MAX_FUTURE_DRIFT constant; raise ChainValidationError with a clear message if
the timestamp is out of range so compute_next_difficulty_target and is_valid_pow
cannot be gamed.
- Around line 98-134: add_block currently calls _replay_state_for_tip for every
block causing O(n) per addition; optimize the common "extend tip" path by
applying the new block directly to the current state instead of replaying the
whole chain: after validating parent_hash and before calling
_replay_state_for_tip, detect parent_is_tip and
candidate_height==canonical_height+1 and attempt to produce candidate_state by
applying the new block to self.state (e.g., via an existing apply/apply_to_state
method or a small helper that validates and mutates a copy of self.state), fall
back to calling _replay_state_for_tip only when parent_is_tip is False or the
direct-apply fails (or for reorgs); preserve the current rollback logic (remove
entries from _blocks_by_hash/_heights) on failure and ensure
_canonical_hashes/_tip_hash/state are updated only after successful apply or
full replay.
In `@src/minichain/consensus.py`:
- Line 7: The import currently uses typing.Sequence which is deprecated; update
the import to use Sequence from collections.abc instead (replace the typing
import of Sequence in the module where "from typing import Sequence" appears),
and update any references to typing.Sequence in this file (e.g., function
signatures or type hints) to use the imported collections.abc.Sequence to
address Ruff UP035 compatibility for Python 3.11+.
In `@src/minichain/genesis.py`:
- Around line 29-38: GenesisConfig.validate() currently only ensures
difficulty_target > 0 but not the upper bound; update it to also check
difficulty_target <= MAX_TARGET (the same bound used by
validate_difficulty_target() in consensus.py) and raise ValueError if exceeded
(e.g., "Genesis difficulty_target must be <= MAX_TARGET" or similar). Reference
the same MAX_TARGET symbol from the consensus module (import if needed) and add
the check alongside the existing positive check in the validate method so
invalid overly-large genesis difficulty values are rejected early.
In `@src/minichain/mempool.py`:
- Around line 108-145: The iteration over self._sender_pools.items() in
get_transactions_for_mining mutates the dict because _recompute_sender_pool can
remove entries; change the loop to iterate over a snapshot (e.g.
list(self._sender_pools.keys()) or list(self._sender_pools.items())) and after
calling _recompute_sender_pool(sender, state) re-fetch the pool from
self._sender_pools before accessing pool.entries or pool.ready_nonces so you
don't iterate the original dict while it is modified.
In `@src/minichain/merkle.py`:
- Around line 18-25: The current Merkle tree loop duplicates the last leaf when
a level is odd (in the code path around the while loop and use of _hash_pair),
which enables second-preimage collisions; change the construction to pad odd
levels with a fixed zero-hash value (compute a canonical ZERO_HASH once and
append that instead of level[-1]) and switch hashing to domain-separated inputs
by distinguishing leaf and internal node hashing (introduce/modify the functions
that produce leaf hashes vs internal hashes so _hash_pair uses the internal-node
domain separator and leaf insertion uses the leaf-domain separator). Update any
call sites that build the initial leaves to use the leaf-domain hasher and
ensure ZERO_HASH is the same byte-length as other hashes so padding is
unambiguous.
In `@src/minichain/network.py`:
- Line 1: The module src/minichain/network.py is a placeholder with only a
docstring and needs a visible TODO and tracking reference so it isn't shipped
silently; add a clear "# TODO: implement P2P networking (py-libp2p) — track via
ISSUE-XXXX" comment at the top of network.py and create or reference a follow-up
issue in your issue tracker (e.g., ISSUE-XXXX) describing required pieces (peer
discovery, gossip, block/tx propagation) so reviewers and CI know the dependency
and planned work; ensure the module-level comment mentions that py-libp2p is
intentionally not yet declared in pyproject/requirements and links to the issue
ID.
In `@src/minichain/node.py`:
- Around line 243-253: The start_node function currently calls node.start() then
immediately shuts it down in the finally block, and although it accepts host and
port it does not open any network server; update the start_node docstring (and
add a clear TODO comment) to state this is a placeholder that prints status and
stops immediately, note that host/port are unused and a real event loop/server
for MiniChainNode is pending, and consider adding guidance for future changes
(e.g., remove the finally shutdown, return the MiniChainNode, or implement an
async/server loop) to make the intended behavior explicit; reference symbols:
start_node, MiniChainNode, node.start, node.stop, host, port.
- Around line 134-146: accept_block currently prunes only the incoming block's
transactions on a "reorg", leaving transactions confirmed in earlier
newly-canonical blocks and dropping transactions from orphaned blocks; fix by
having chain_manager.add_block return detailed reorg information (e.g., result
plus lists added_blocks and removed_blocks) or call a new chain_manager method
to get the branch diff after add_block, then in accept_block when result ==
"reorg" iterate over all added_blocks and call
self.mempool.remove_confirmed_transactions on each block.transactions (using
self.chain_manager.state for final state), and iterate over removed_blocks to
re-add their transactions back into the mempool (e.g.,
self.mempool.add_transactions or equivalent) before calling self._persist_head
so the mempool reflects the new canonical chain.
- Around line 1-16: The Node class accesses mutable attributes (_running,
_storage, _chain_manager, _mempool) without synchronization; add a thread-safe
lock (e.g., self._lock = threading.RLock()) to Node and guard all public
lifecycle and mutating methods (start, stop, submit, accept, mine_one_block, any
submit/accept wrappers) with that lock to prevent races and TOCTOU checks. For
short checks/assignments (e.g., check/set _running, assign
_storage/_chain_manager/_mempool) hold the lock; for long-running ops (mining,
I/O) acquire the lock only to snapshot the needed references into local
variables then release it so work proceeds without blocking other callers.
Ensure all reads/writes of _running, _storage, _chain_manager, and _mempool
happen under the lock to be consistent.
- Around line 92-111: The start method can leak the SQLiteStorage connection if
SQLiteStorage(db_path) succeeds but _initialize_chain_manager(...) or
Mempool(...) raises before self._storage is assigned; modify start to create
storage and then wrap the rest of initialization in a try/except/finally: on
exception ensure storage.close() (or storage.shutdown()) is called before
re-raising, and only assign self._storage, self._chain_manager, self._mempool
and set self._running = True after all initializations succeed; reference the
SQLiteStorage constructor, _initialize_chain_manager, Mempool, start and stop so
the cleanup mirrors stop's shutdown behavior.
- Around line 262-271: The _states_equal helper currently compares only
account.balance and account.nonce which will miss any future fields; update
_states_equal to delegate to State.__eq__ (or compare full Account objects) so
the equality check stays in sync with the model—locate the _states_equal
function and replace the manual dict-of-tuples comparison with a call to left ==
right (or compare left.accounts == right.accounts using Account.__eq__) so new
Account fields are automatically considered.
In `@src/minichain/serialization.py`:
- Around line 47-55: The json.dumps call inside serialize_canonical is forcing
lexicographic key ordering (sort_keys=True) which overrides the sequence
produced by _to_field_map(value, field_order); remove the sort_keys=True
argument so json.dumps preserves the declared field order from _to_field_map
(which is important for consensus-critical orders like TRANSACTION_FIELD_ORDER),
leaving ensure_ascii, separators, and ensure_utf8 behavior unchanged.
In `@src/minichain/transaction.py`:
- Around line 105-124: The verify() method currently catches a broad Exception
around deserialize_verify_key(self.public_key); narrow this to the
PyNaCl-specific exception(s) by importing and catching
nacl.exceptions.CryptoError (or the more specific nacl.exceptions.ValueError and
nacl.exceptions.TypeError) instead of Exception in the try/except in verify(),
so deserialize_verify_key and subsequent logic only silence the known crypto/key
parsing errors while letting unexpected exceptions bubble up.
In `@tests/test_consensus.py`:
- Around line 50-52: Replace the hardcoded max target in the test with the
shared constant: import MAX_TARGET from minichain.consensus and use it when
calling _header_template in test_valid_pow_is_accepted so the header is created
with difficulty_target=MAX_TARGET rather than (1 << 256) - 1; update the test
import list and the call in test_valid_pow_is_accepted accordingly.
- Around line 76-86: Replace the try/except/else pattern in
test_mining_honors_stop_event with pytest.raises: wrap the call to
mine_block_header(...) in a with pytest.raises(MiningInterrupted) as excinfo:
block, then assert "interrupted" in str(excinfo.value). Update the same pattern
in the other test at lines 89-96; reference the test function name
test_mining_honors_stop_event and the mine_block_header and MiningInterrupted
symbols to locate and change the tests accordingly.
In `@tests/test_crypto.py`:
- Around line 40-47: The test creates an unused local variable signing_key which
triggers Ruff RUF059; in test_invalid_signature_is_rejected replace the unused
binding by discarding it (e.g., assign the first value to _ or delete
signing_key) when calling generate_key_pair() so only verify_key is kept,
keeping the calls to generate_key_pair(), sign_message, and verify_signature
intact.
In `@tests/test_genesis.py`:
- Around line 60-71: Replace the manual try/except in
test_genesis_requires_empty_state with pytest.raises: use
pytest.raises(ValueError) as excinfo around the call to
apply_genesis_block(state, block, config) and assert the error message contains
"empty state" via str(excinfo.value); ensure pytest is imported at the top of
the test file. Locate this change around the test_genesis_requires_empty_state
function where GenesisConfig, create_genesis_block, State, and
apply_genesis_block are used.
In `@tests/test_mempool.py`:
- Around line 123-124: The two compound assertions combining sender and nonce
checks on selected[2] and selected[3] should be split into separate assertions
for clearer failure messages: replace each "assert selected[i].sender ==
sender_a and selected[i].nonce == N" with two assertions, e.g. assert
selected[i].sender == sender_a and then assert selected[i].nonce == N for both
selected[2] (nonce 0) and selected[3] (nonce 1); update the assertions in
tests/test_mempool.py referencing selected, sender_a, and nonce accordingly.
In `@tests/test_scaffold.py`:
- Around line 27-28: The assertion "assert imported is not None" after calling
importlib.import_module is redundant because
importlib.import_module(f"minichain.{module}") either returns a module or
raises; remove the dead assertion line to clean up tests/test_scaffold.py and
rely on the import call itself (referencing importlib.import_module and the
variable imported) to surface failures.
In `@tests/test_serialization.py`:
- Around line 68-96: The parametrize call is using a string for the first
argument which triggers Ruff PT006; change the first argument of
pytest.mark.parametrize from the single string "payload,serializer,expected" to
an explicit tuple of names ("payload", "serializer", "expected") so the test
decorator uses a tuple of parameter names for the serialize_transaction /
serialize_block_header parameterization.
In `@tests/test_storage.py`:
- Line 59: The tmp_path parameter is incorrectly annotated as
pytest.TempPathFactory; change its type annotation to pathlib.Path in
test_store_and_load_block_round_trip and the other two tests that accept
tmp_path (the tests around lines ~107 and ~132), and add/ensure an import for
pathlib.Path if missing; specifically replace "tmp_path: pytest.TempPathFactory"
with "tmp_path: pathlib.Path" in those test function signatures.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
.github/workflows/ci.yml.gitignoreMakefileREADME.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_chain.pytests/test_consensus.pytests/test_crypto.pytests/test_genesis.pytests/test_mempool.pytests/test_merkle.pytests/test_mining.pytests/test_node.pytests/test_scaffold.pytests/test_serialization.pytests/test_state.pytests/test_storage.pytests/test_transaction.py
Comment on lines
+15
to
+23
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" | ||
|
|
||
| - 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 caching to speed up CI.
Adding a cache: 'pip' option to setup-python avoids re-downloading packages on every run.
♻️ Proposed change
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
+ cache: "pip"📝 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
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip | |
| python -m pip install -e .[dev] | |
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| cache: "pip" | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip | |
| python -m pip install -e .[dev] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 15 - 23, In the CI workflow's "Setup
Python" step using actions/setup-python@v5, add the pip cache option by adding
cache: 'pip' under the with block so dependencies are cached between runs;
update the Setup Python step (named "Setup Python") to include with:
python-version: "3.11" and cache: 'pip' to avoid re-downloading packages during
the "Install dependencies" run step.
Comment on lines
+1
to
+15
| # MiniChain | ||
|
|
||
| <!-- Organization Logo --> | ||
| <div align="center" style="display: flex; align-items: center; justify-content: center; gap: 16px;"> | ||
| <img alt="Stability Nexus" src="public/stability.svg" width="175"> | ||
| <img src="public/todo-project-logo.svg" width="175" /> | ||
| </div> | ||
| MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository currently contains the project scaffolding and development environment for the v0 core chain roadmap. | ||
|
|
||
| | ||
| ## Current Status | ||
|
|
||
| <!-- Organization Name --> | ||
| <div align="center"> | ||
| Issue #1 (project scaffolding) is implemented with: | ||
|
|
||
| [](https://TODO.stability.nexus/) | ||
| - Python package layout under `src/minichain` | ||
| - Placeholder component modules for: | ||
| - `crypto`, `transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`, `node` | ||
| - `pyproject.toml` project configuration | ||
| - `Makefile` for common developer tasks | ||
| - Basic CI workflow (`.github/workflows/ci.yml`) | ||
| - Baseline tests under `tests/` |
Contributor
There was a problem hiding this comment.
Update “Current Status” to reflect the current implementation.
This section still describes scaffolding and placeholder modules, which appears outdated given the full node orchestration and chain implementation in this PR.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ...on developer tasks - Basic CI workflow (.github/workflows/ci.yml) - Baseline tests und...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 1 - 15, Update the "Current Status" section text
under the README's "Current Status" header to remove the scaffolding/placeholder
language and accurately list implemented functionality: mention that full node
orchestration and chain implementation are included, enumerate the concrete
modules implemented (crypto, transaction, block, state, mempool, consensus,
network, storage, node), note the Python package layout under src/minichain,
existing tests under tests/, CLI/tools or example scripts if present, and the
CI/workflow and Makefile; ensure the language replaces "placeholder" with clear
status (implemented vs. planned) and briefly summarises what works (e.g., chain
syncing, block/tx processing, basic consensus) so readers know the current
capabilities.
Comment on lines
+36
to
43
| ## Common Commands | ||
|
|
||
| ```bash | ||
| npm run dev | ||
| # or | ||
| yarn dev | ||
| # or | ||
| pnpm dev | ||
| make test # run unit tests | ||
| make lint # run ruff checks | ||
| make format # format with ruff | ||
| make start-node # run scaffold node entrypoint | ||
| ``` |
Contributor
There was a problem hiding this comment.
Adjust the “start-node” description to match current behavior.
The comment still calls it a scaffold entrypoint.
✏️ Suggested edit
-make start-node # run scaffold node entrypoint
+make start-node # run node entrypoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 36 - 43, Update the "make start-node" description in
the Common Commands section so it reflects the current behavior instead of
calling it a "scaffold node entrypoint"; change the comment for the make target
(the line containing "make start-node # ...") to a concise, accurate
description of what make start-node actually does (for example "start local
node", "run development node", or the current runtime behavior), ensuring the
update is applied to the README's Common Commands block where "make start-node"
appears.
Comment on lines
+51
to
+71
| ## Repository Layout | ||
|
|
||
| ```text | ||
| .github/workflows/ci.yml | ||
| src/minichain/ | ||
| __init__.py | ||
| __main__.py | ||
| crypto.py | ||
| transaction.py | ||
| block.py | ||
| state.py | ||
| mempool.py | ||
| consensus.py | ||
| network.py | ||
| storage.py | ||
| node.py | ||
| tests/ | ||
| test_scaffold.py | ||
| issues.md | ||
| architectureProposal.md | ||
| ``` |
Contributor
There was a problem hiding this comment.
Repository layout list looks incomplete.
It omits newly added tests and modules; consider expanding or marking it as non‑exhaustive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 51 - 71, Update the "## Repository Layout" section in
README.md to either list all newly added modules/tests or explicitly mark the
list as non‑exhaustive; edit the "Repository Layout" block (the section header
"## Repository Layout" and the following code block) to include the new test
files and any new src modules you added (e.g., new test files under tests/ and
new Python modules under src/minichain/), or add a trailing comment like "…
(non‑exhaustive)" so the README no longer omits recent additions.
Comment on lines
+11
to
+25
| def build_parser() -> argparse.ArgumentParser: | ||
| parser = argparse.ArgumentParser(description="Run a MiniChain node.") | ||
| parser.add_argument("--host", default="127.0.0.1", help="Host interface for the node") | ||
| parser.add_argument("--port", default=7000, type=int, help="Port for the node") | ||
| parser.add_argument( | ||
| "--data-dir", | ||
| default=".minichain", | ||
| help="Directory for node data (sqlite db, chain state)", | ||
| ) | ||
| parser.add_argument( | ||
| "--miner-address", | ||
| default=None, | ||
| help="Optional 20-byte lowercase hex address used for mining rewards", | ||
| ) | ||
| return parser |
Contributor
There was a problem hiding this comment.
--host and --port are accepted but never used for actual networking.
Both arguments are parsed and printed (Line 38) but are never passed to NodeConfig and no socket is ever bound. The node performs no network I/O. Either wire these into a future host/port field on NodeConfig, or remove the arguments (or add a TODO comment) so callers aren't misled into believing the node is listening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/minichain/__main__.py` around lines 11 - 25, build_parser currently
defines --host and --port but those values are never propagated into the node
startup, so callers are misled; update the CLI wiring to either (A) add host and
port fields to NodeConfig and pass args.host/args.port into the Node
creation/start path (e.g., where NodeConfig is constructed and where the node
binds sockets), or (B) remove the --host/--port arguments (or add a clear TODO
comment) if networking is not yet implemented. Locate build_parser and the code
that constructs NodeConfig / starts the node (the NodeConfig constructor and the
node start/create call) and ensure the parsed args.host and args.port are used
there (or remove the args) so the CLI reflects actual behavior.
Comment on lines
+60
to
+71
| def test_genesis_requires_empty_state() -> None: | ||
| config = GenesisConfig(initial_balances={"dd" * 20: 1}) | ||
| block = create_genesis_block(config) | ||
| state = State() | ||
| state.set_account("ff" * 20, Account(balance=1, nonce=0)) | ||
|
|
||
| try: | ||
| apply_genesis_block(state, block, config) | ||
| except ValueError as exc: | ||
| assert "empty state" in str(exc) | ||
| else: | ||
| raise AssertionError("Expected ValueError for non-empty state") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files tests/ | head -20Repository: StabilityNexus/MiniChain
Length of output: 376
🏁 Script executed:
cat -n tests/test_genesis.py | head -100Repository: StabilityNexus/MiniChain
Length of output: 3371
🏁 Script executed:
cat -n tests/test_genesis.py | head -20Repository: StabilityNexus/MiniChain
Length of output: 727
🏁 Script executed:
# Check if pytest is imported in test files
rg "import pytest" tests/Repository: StabilityNexus/MiniChain
Length of output: 461
🏁 Script executed:
# Check project dependencies
find . -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" | head -5Repository: StabilityNexus/MiniChain
Length of output: 84
🏁 Script executed:
cat pyproject.tomlRepository: StabilityNexus/MiniChain
Length of output: 771
🏁 Script executed:
# Check if other tests in test_genesis.py use pytest pattern
rg "pytest.raises" tests/test_genesis.pyRepository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
# Check if pytest is used in other test files with raises
rg "pytest.raises" tests/ | head -10Repository: StabilityNexus/MiniChain
Length of output: 1001
Use pytest.raises for exception assertions.
This is the idiomatic pytest pattern and aligns with the exception handling style used throughout the rest of the test suite.
♻️ Suggested refactor
+import pytest
@@
- try:
- apply_genesis_block(state, block, config)
- except ValueError as exc:
- assert "empty state" in str(exc)
- else:
- raise AssertionError("Expected ValueError for non-empty state")
+ with pytest.raises(ValueError, match="empty state"):
+ apply_genesis_block(state, block, config)
@@
- try:
- apply_genesis_block(state, block, config)
- except ValueError as exc:
- assert "previous_hash" in str(exc)
- else:
- raise AssertionError("Expected ValueError for invalid previous_hash")
+ with pytest.raises(ValueError, match="previous_hash"):
+ apply_genesis_block(state, block, config)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 69-69: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 71-71: 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 `@tests/test_genesis.py` around lines 60 - 71, Replace the manual try/except in
test_genesis_requires_empty_state with pytest.raises: use
pytest.raises(ValueError) as excinfo around the call to
apply_genesis_block(state, block, config) and assert the error message contains
"empty state" via str(excinfo.value); ensure pytest is imported at the top of
the test file. Locate this change around the test_genesis_requires_empty_state
function where GenesisConfig, create_genesis_block, State, and
apply_genesis_block are used.
Comment on lines
+123
to
+124
| 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
Split compound assertions for clearer failure messages (Ruff PT018).
♻️ 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 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 |
🧰 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 123 - 124, The two compound assertions
combining sender and nonce checks on selected[2] and selected[3] should be split
into separate assertions for clearer failure messages: replace each "assert
selected[i].sender == sender_a and selected[i].nonce == N" with two assertions,
e.g. assert selected[i].sender == sender_a and then assert selected[i].nonce ==
N for both selected[2] (nonce 0) and selected[3] (nonce 1); update the
assertions in tests/test_mempool.py referencing selected, sender_a, and nonce
accordingly.
Comment on lines
+27
to
+28
| imported = importlib.import_module(f"minichain.{module}") | ||
| assert imported is not None |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assert imported is not None is a dead assertion.
importlib.import_module either returns a module object or raises ModuleNotFoundError; it never returns None. The assertion adds no protective value and should be removed.
♻️ Proposed cleanup
def test_component_modules_are_importable() -> None:
for module in COMPONENT_MODULES:
- imported = importlib.import_module(f"minichain.{module}")
- assert imported is not None
+ importlib.import_module(f"minichain.{module}")📝 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
| imported = importlib.import_module(f"minichain.{module}") | |
| assert imported is not None | |
| def test_component_modules_are_importable() -> None: | |
| for module in COMPONENT_MODULES: | |
| importlib.import_module(f"minichain.{module}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_scaffold.py` around lines 27 - 28, The assertion "assert imported
is not None" after calling importlib.import_module is redundant because
importlib.import_module(f"minichain.{module}") either returns a module or
raises; remove the dead assertion line to clean up tests/test_scaffold.py and
rely on the import call itself (referencing importlib.import_module and the
variable imported) to surface failures.
Comment on lines
+68
to
+96
| @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", | ||
| ), | ||
| ], |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a tuple for the first pytest.mark.parametrize argument (Ruff 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", | |
| ), | |
| ], | |
| `@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", | |
| ), | |
| ], |
🧰 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 - 96, The parametrize call is
using a string for the first argument which triggers Ruff PT006; change the
first argument of pytest.mark.parametrize from the single string
"payload,serializer,expected" to an explicit tuple of names ("payload",
"serializer", "expected") so the test decorator uses a tuple of parameter names
for the serialize_transaction / serialize_block_header parameterization.
| return block | ||
|
|
||
|
|
||
| def test_store_and_load_block_round_trip(tmp_path: pytest.TempPathFactory) -> None: |
Contributor
There was a problem hiding this comment.
tmp_path fixture is typed pathlib.Path, not pytest.TempPathFactory.
tmp_path yields a pathlib.Path for a per-test temporary directory. pytest.TempPathFactory is the type of the separate tmp_path_factory fixture. All three test functions carry the wrong annotation.
♻️ Proposed fix (apply to all three functions)
+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,
+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,
+def test_atomic_persist_rolls_back_on_metadata_failure(
+ tmp_path: Path,
) -> None:Also applies to: 107-109, 132-134
🤖 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 parameter is incorrectly
annotated as pytest.TempPathFactory; change its type annotation to pathlib.Path
in test_store_and_load_block_round_trip and the other two tests that accept
tmp_path (the tests around lines ~107 and ~132), and add/ensure an import for
pathlib.Path if missing; specifically replace "tmp_path: pytest.TempPathFactory"
with "tmp_path: pathlib.Path" in those test function signatures.
This was referenced Feb 24, 2026
Author
|
Superseded by #37. Closing this PR to keep review history in one place. |
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:
Part of #8
Depends on #35
Summary
limits, and chain/genesis settings
genesis if fresh
canonical apply
optional --miner-address
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