feat: coinbase transaction support with block-level reward validation#30
Closed
arunabha003 wants to merge 23 commits intoStabilityNexus:mainfrom
Closed
feat: coinbase transaction support with block-level reward validation#30arunabha003 wants to merge 23 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 23 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughThis PR establishes the foundational infrastructure and core blockchain components for a Python-based mini blockchain implementation. It introduces the build system (Makefile, pyproject.toml), CI/CD automation (GitHub Actions), refactored documentation, and all core modules including transaction and block models, cryptographic operations, Merkle trees, Proof-of-Work consensus, state management, genesis initialization, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BlockValidator as Block<br/>Validator
participant StateEngine as State<br/>Engine
participant CoinbaseChk as Coinbase<br/>Validator
participant TxnApplier as Transaction<br/>Applier
Client->>BlockValidator: apply_block(block)
BlockValidator->>BlockValidator: verify merkle root
BlockValidator->>CoinbaseChk: validate_coinbase(reward)
CoinbaseChk->>CoinbaseChk: check position 0<br/>check amount = reward + fees
CoinbaseChk-->>BlockValidator: ✓ valid
BlockValidator->>StateEngine: snapshot state
StateEngine-->>BlockValidator: snapshot
BlockValidator->>TxnApplier: apply coinbase txn
TxnApplier->>StateEngine: credit miner
StateEngine-->>TxnApplier: ✓
loop for each transaction
BlockValidator->>TxnApplier: apply transaction
TxnApplier->>TxnApplier: verify signature
TxnApplier->>TxnApplier: check nonce
TxnApplier->>TxnApplier: check balance
TxnApplier->>StateEngine: update accounts
StateEngine-->>TxnApplier: ✓
end
alt all valid
BlockValidator-->>Client: ✓ block applied
else validation error
BlockValidator->>StateEngine: restore snapshot
BlockValidator-->>Client: ✗ StateTransitionError
end
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: 31
🤖 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-18: CI step using actions/setup-python@v5 doesn't enable pip
caching, which causes dependencies to be downloaded every run; update the Setup
Python step (actions/setup-python@v5) to include the cache option (e.g., set
cache: "pip") so the action caches pip packages between runs, and ensure the
python-version: "3.11" setting remains unchanged and test a workflow run to
confirm the cache is being used.
In @.gitignore:
- Line 340: The .gitignore currently contains a blanket "docs/" rule which can
silently omit handwritten documentation; update the .gitignore by narrowing or
removing that rule: either remove "docs/" to keep the directory versioned,
replace it with a scoped path for generated output (e.g., "docs/_build/" or
"docs/build/") if docs are generated, or add explicit allow-list exceptions
(e.g., negate patterns like "!docs/**/*.md" or specific files) so ADRs and
manual docs are not ignored; locate the "docs/" entry in .gitignore and apply
one of these scoped alternatives accordingly.
- Around line 261-270: Add `.env` to the Python/dev section of the .gitignore to
prevent accidental commits of environment secret files; locate the block
containing entries like "__pycache__/", "*.py[cod]", ".pytest_cache/",
".ruff_cache/", and ".venv/" and insert a new line with ".env" alongside them so
local environment files are ignored.
- Around line 265-270: The Python ignore lines (__pycache__/, *.py[cod],
.pytest_cache/, .ruff_cache/, .venv/) are currently inserted in the middle of
the LaTeX auxiliary-files block (near the "# xcolor" entry); move these
Python-specific entries out of that block and into a dedicated Python section
(or append them to the end of the file near the docs/ entries) so the LaTeX
section remains contiguous and Python ignores are grouped together.
In `@Makefile`:
- Line 9: The Makefile target invoking pip uses an unquoted bracket expression
(the command invoking $(PYTHON) -m pip install -e .[dev]), which can be expanded
by the shell; fix it by quoting or escaping the extras spec so pip receives the
literal ".[dev]" (e.g., change the pip install invocation to use '.[dev]' or
".[dev]" or escape the brackets) while leaving the $(PYTHON) variable and the
rest of the command intact.
- Line 3: Add a new clean phony target to the Makefile and include it in the
existing .PHONY list: declare a target named clean that removes build and cache
artifacts (e.g., __pycache__, .ruff_cache, dist, and any *.egg-info) so CI and
local dev can reset state; update the .PHONY line (currently listing install
dev-install test lint format start-node) to also include clean and ensure the
clean target performs safe removals (rm -rf or equivalent) for those
directories/patterns.
In `@pyproject.toml`:
- Around line 17-19: The optional dependency group "network" in pyproject.toml
uses the incorrect PyPI name "py-libp2p"; change that entry to the actual PyPI
package name "libp2p>=0.2.0" so pip can install it; update the string in the
network list (replace "py-libp2p>=0.2.0" with "libp2p>=0.2.0") while keeping the
same version constraint.
In `@README.md`:
- Around line 53-71: The README's "Repository layout" is missing the new
modules; update that section to include genesis.py, merkle.py, and
serialization.py under src/minichain (e.g., add lines for genesis.py, merkle.py,
serialization.py alongside existing files such as crypto.py and block.py) so the
list reflects the current codebase; ensure the filenames match exactly
(genesis.py, merkle.py, serialization.py) and maintain the same
indentation/format as the surrounding entries.
In `@src/minichain/block.py`:
- Around line 59-81: The validate_coinbase method currently sums fees from
self.transactions[1:] without checking they are non-negative; add a defensive
check at the start of validate_coinbase (before computing total_fees) that
iterates the non-coinbase transactions and raises BlockValidationError if any
transaction.fee < 0 (e.g., use any(tx.fee < 0 for tx in self.transactions[1:])
or validate per-item during the sum), then proceed to compute total_fees and
expected_amount as before; reference validate_coinbase, self.transactions, and
Transaction.verify to keep validation self-contained.
In `@src/minichain/consensus.py`:
- Around line 92-99: The hot loop currently calls
dataclasses.replace(header_template, nonce=nonce) per iteration which allocates
a new BlockHeader and its serialized form each time; to reduce allocation
overhead, change the loop to reuse a single mutable header object (modify its
nonce field in-place) or maintain a pre-serialized header prefix and only
serialize/append the nonce before computing candidate.hash(); update the loop
that uses header_template, replace, candidate, nonce, candidate.hash(),
hash_to_int and candidate.difficulty_target to mutate the header nonce (or reuse
the serialized prefix) and re-hash without creating a new BlockHeader each
iteration.
- Line 7: The file imports Sequence from typing; update the import to use
collections.abc instead: replace the "from typing import Sequence" import in
src/minichain/consensus.py with "from collections.abc import Sequence" so the
module uses the modern ABC location (ensure any references to Sequence in this
file remain unchanged and the import is the only edit).
In `@src/minichain/crypto.py`:
- Around line 21-24: Replace the fragile globals() sentinel by introducing an
explicit boolean flag (e.g., _NACL_AVAILABLE) that is set in the import
try/except where PyNaCl is imported, and modify _require_nacl to check that flag
(not the presence of "blake2b"); if the flag is False raise RuntimeError using
the existing _NACL_IMPORT_ERROR as the cause. Update any places that relied on
checking "blake2b" in globals() to use _NACL_AVAILABLE instead and ensure the
import block assigns _NACL_AVAILABLE and captures _NACL_IMPORT_ERROR.
In `@src/minichain/genesis.py`:
- Line 47: The genesis code directly calls blake2b_digest(b"") instead of using
the compute_merkle_root abstraction; update both create_genesis_block and
apply_genesis_block to derive the empty Merkle root via compute_merkle_root([])
(or compute_merkle_root([]).hex() if the original code expects a hex string)
instead of blake2b_digest(b"").hex() so the genesis logic stays consistent with
compute_merkle_root's behaviour.
- Line 24: GenesisConfig is declared frozen but contains a mutable dict field
initial_balances; fix by making the field immutable in __post_init__: convert
initial_balances into a read-only MappingProxyType (or similar immutable
mapping) and assign it back using object.__setattr__ (because the dataclass is
frozen). Update the type annotation to use Mapping[str,int] if you prefer, and
perform the conversion inside GenesisConfig.__post_init__ to ensure callers
cannot mutate the underlying dict.
- Around line 14-17: Duplicate helper _is_lower_hex should be extracted to a
shared internal utility: create src/minichain/_utils.py exporting
is_lower_hex(value: str, expected_length: int) with the same logic, then replace
the private _is_lower_hex definitions in both genesis.py and transaction.py with
from minichain._utils import is_lower_hex and update local calls to use
is_lower_hex (remove the duplicated function definitions); ensure any imports
reference the new symbol and delete the old duplicated code blocks.
In `@src/minichain/serialization.py`:
- Around line 47-56: The function serialize_canonical currently uses
sort_keys=True to produce the canonical JSON ordering, while the field_order
parameter (and any FIELD_ORDER tuple elsewhere) only controls validation via
_to_field_map; update the serialize_canonical docstring (or add a one-line
inline comment above the json.dumps call) to state that canonical ordering is
provided by json.dumps(..., sort_keys=True) and that field_order only
enforces/validates which keys must be present (reference serialize_canonical and
_to_field_map to locate the logic), so readers aren’t misled into thinking
field_order determines output order.
- Around line 28-44: The helper _to_field_map currently rejects unexpected keys
for Mapping inputs but silently ignores extra attributes on object inputs;
update the function's docstring (for _to_field_map) to state this asymmetry
clearly: explain that when value is a Mapping it enforces exact keys against
field_order and raises on extras, whereas when value is an object it only
collects attributes named in field_order and ignores other attributes;
optionally note how to make behavior strict (e.g., by using vars(value) or
inspecting __dict__ to detect extras) so future maintainers know the
alternative.
- Line 6: The import statement currently uses Mapping from typing; replace it
with Mapping from collections.abc to satisfy Ruff UP035 and modern Python
conventions—update the import line in src/minichain/serialization.py so it
imports Any from typing but imports Mapping from collections.abc (keep other
symbols unchanged) and run the linter/tests to confirm no other references need
updating.
In `@src/minichain/state.py`:
- Around line 95-102: The module-level functions apply_transaction and
apply_block are thin pass-through wrappers over State.apply_transaction and
State.apply_block with identical signatures, adding indirection with no extra
logic; either remove these top-level wrappers and call the State methods
directly, or keep them but add a brief docstring/comment above apply_transaction
and apply_block stating they exist for backward-compatibility or as a public
facade (mentioning State.apply_transaction and State.apply_block by name) so
future maintainers understand the intent.
- Around line 40-43: get_account currently mutates state on every read by
inserting a default Account into self.accounts; change it to be a pure read that
returns a copy/default without persisting (e.g., return
self.accounts.get(address, Account()) or a shallow copy) and add a new helper
_get_or_create_account that performs the current create-if-missing behavior;
update callers that intend to mutate state (apply_transaction and
apply_coinbase_transaction) to use _get_or_create_account, leaving read-only
code and copy() to use the non-mutating get_account to avoid spurious inserts
and state bloat.
In `@src/minichain/storage.py`:
- Line 1: The file currently contains only a placeholder string; add a minimal
storage contract by implementing an abstract base class StorageBackend declaring
methods save_block, load_block, save_state, load_state (and optionally
delete_block/list_blocks), then provide a simple InMemoryStorage class
implementing that interface for testing; update exports so other modules can
import StorageBackend and InMemoryStorage and use the in-memory implementation
as a drop-in until a persistent backend is available.
In `@src/minichain/transaction.py`:
- Around line 23-26: The _is_lower_hex function is duplicated; extract it into a
shared internal utility (e.g., create a helper function in a new or existing
module such as minichain.utils or minichain._utils) and replace the local
implementations in both transaction.py and genesis.py with an import from that
shared helper; ensure the signature remains def _is_lower_hex(value: str,
expected_length: int) -> bool and update both modules to import and call that
single implementation.
- Around line 116-119: The code currently uses a broad "except Exception" around
deserialize_verify_key(self.public_key); narrow this to only the expected
failure types (e.g., catch ValueError and PyNaCl-specific exceptions such as
nacl.exceptions.CryptoError or the specific PyNaCl exception your codebase uses)
so unexpected errors aren't swallowed. Update the try/except in transaction.py
(around deserialize_verify_key and self.public_key) to catch those specific
exceptions, or if you intentionally need a broader fallback for the no-nacl
environment, keep a documented fallback branch using your module-level
_require_nacl pattern and limit the broad catch to that guard only.
- Around line 97-103: The sign() method currently overwrites coinbase
transactions; add an explicit guard at the start of sign() to reject coinbase
transactions by calling self.is_coinbase() (after or before
_validate_common_fields()) and raising a ValueError (e.g., "Cannot sign coinbase
transaction") if true, so public_key/signature aren't populated for coinbase
transactions and verify()/derive_address(verify_key) remain correct.
In `@tests/test_consensus.py`:
- Around line 76-96: Replace the bare try/except/assert pattern in tests with
pytest.raises: in test_mining_honors_stop_event, use
pytest.raises(MiningInterrupted) as excinfo around the mine_block_header(...)
call (passing stop_event=stop) and then assert the exception message contains
"interrupted" using str(excinfo.value).lower(); in
test_mining_raises_when_nonce_range_exhausted, use pytest.raises(RuntimeError)
as excinfo around mine_block_header(header, start_nonce=0, max_nonce=10) and
assert "No valid nonce found" in str(excinfo.value). Also ensure pytest is
imported in the test file if not already.
In `@tests/test_crypto.py`:
- Around line 40-47: The test unpacks signing_key but never uses it, triggering
Ruff RUF059; update the unpacking in test_invalid_signature_is_rejected so the
unused signer is prefixed with an underscore (e.g., _signing_key, verify_key =
generate_key_pair()) to silence the linter while keeping verify_key available,
leaving other_signing_key, _ = generate_key_pair() unchanged; ensure you only
modify the variable name in the unpacking of generate_key_pair() for this test
function.
In `@tests/test_genesis.py`:
- Around line 66-85: Replace the bare try/except assertion pattern in the tests
test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises: import pytest
at the top of the file, then use with pytest.raises(ValueError, match="empty
state") when calling apply_genesis_block(state, block, config) in the first
test, and with pytest.raises(ValueError, match="previous_hash") when calling
apply_genesis_block(state, block, config) in the second test (references:
apply_genesis_block, create_genesis_block, GenesisConfig, State).
In `@tests/test_scaffold.py`:
- Around line 23-26: Refactor test_component_modules_are_importable to be
parametrized with pytest.mark.parametrize so each entry in COMPONENT_MODULES is
a separate test case; replace the in-function loop over COMPONENT_MODULES with a
test that accepts a single module name parameter and calls
importlib.import_module(f"minichain.{module}") and asserts the result is not
None, optionally providing readable ids via pytest.param for better diagnostics
(refer to symbols test_component_modules_are_importable, COMPONENT_MODULES, and
importlib.import_module).
In `@tests/test_serialization.py`:
- Around line 68-69: The pytest parametrize call uses a comma-separated string
for multiple parameter names which triggers Ruff PT006; update the
pytest.mark.parametrize invocation so the first argument is a tuple of parameter
names (e.g., ("payload", "serializer", "expected")) instead of a comma-separated
string to satisfy the linter and keep the parameters payload, serializer, and
expected in the same order.
In `@tests/test_transaction.py`:
- Around line 49-55: In test_mismatched_public_key_and_sender_is_rejected, avoid
the post-hoc suppression `_ = other_signing_key` by directly unpacking only the
needed verify key from generate_key_pair(); change the current unpacking to
discard the unused signing key (e.g., "_, other_verify_key =
generate_key_pair()") and remove the `_ = other_signing_key` line so the rest of
the test (creating tampered = replace(...,
public_key=serialize_verify_key(other_verify_key)) and asserting not
tampered.verify()) remains unchanged.
- Around line 77-85: The test currently only tampers with signature but the
coinbase gating in is_coinbase() checks both signature and public_key; update
test_coinbase_with_auth_fields_is_rejected to cover both auth fields by adding a
second case (or parametrize) that creates a coinbase via
create_coinbase_transaction, uses replace to set public_key to a non-empty value
(e.g., "00"*... matching expected length), and asserts that tampered.verify() is
False; reference the existing use of create_coinbase_transaction, replace,
tampered.verify(), and the is_coinbase()/signature/public_key logic when adding
the new case.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/ci.yml.gitignoreMakefileREADME.mdpyproject.tomlsrc/minichain/__init__.pysrc/minichain/__main__.pysrc/minichain/block.pysrc/minichain/consensus.pysrc/minichain/crypto.pysrc/minichain/genesis.pysrc/minichain/mempool.pysrc/minichain/merkle.pysrc/minichain/network.pysrc/minichain/node.pysrc/minichain/serialization.pysrc/minichain/state.pysrc/minichain/storage.pysrc/minichain/transaction.pytests/test_block.pytests/test_consensus.pytests/test_crypto.pytests/test_genesis.pytests/test_merkle.pytests/test_scaffold.pytests/test_serialization.pytests/test_state.pytests/test_transaction.py
Comment on lines
+15
to
+18
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enabling pip caching to speed up CI.
actions/setup-python@v5 supports a cache option that avoids re-downloading packages on every run.
Proposed fix
- 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: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| cache: "pip" |
🤖 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 - 18, CI step using
actions/setup-python@v5 doesn't enable pip caching, which causes dependencies to
be downloaded every run; update the Setup Python step (actions/setup-python@v5)
to include the cache option (e.g., set cache: "pip") so the action caches pip
packages between runs, and ensure the python-version: "3.11" setting remains
unchanged and test a workflow run to confirm the cache is being used.
Comment on lines
+261
to
+270
| # MiniChain local planning docs (do not commit) | ||
| issues.md | ||
| architectureProposal.md | ||
|
|
||
| # Python caches and virtualenvs | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| .ruff_cache/ | ||
| .venv/ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add .env to prevent accidental secret commits.
The Python section adds cache and virtualenv entries, but omits .env. For a blockchain project that handles private keys and may source secrets from environment files, committing a .env file by accident is a meaningful security risk.
🔒 Proposed addition
# Python caches and virtualenvs
__pycache__/
*.py[cod]
.pytest_cache/
.ruff_cache/
.venv/
+.env
+.env.*📝 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
| # MiniChain local planning docs (do not commit) | |
| issues.md | |
| architectureProposal.md | |
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| # MiniChain local planning docs (do not commit) | |
| issues.md | |
| architectureProposal.md | |
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| .env | |
| .env.* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 261 - 270, Add `.env` to the Python/dev section of
the .gitignore to prevent accidental commits of environment secret files; locate
the block containing entries like "__pycache__/", "*.py[cod]", ".pytest_cache/",
".ruff_cache/", and ".venv/" and insert a new line with ".env" alongside them so
local environment files are ignored.
Comment on lines
+265
to
+270
| # Python caches and virtualenvs | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| .ruff_cache/ | ||
| .venv/ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Python entries are inserted mid-LaTeX-section — consider moving them to the end.
Lines 265–270 split the continuous LaTeX auxiliary-files block (which continues at line 272 with # xcolor). Python-specific entries belong either in their own section at the end of the file, grouped with the docs/ entry at lines 338–340, or in a dedicated Python section before the LaTeX block.
♻️ Suggested reorganization
-# Python caches and virtualenvs
-__pycache__/
-*.py[cod]
-.pytest_cache/
-.ruff_cache/
-.venv/
-
# xcolor
*.xcpAnd append at the end of the file (after the existing content):
+
+# Python caches and virtualenvs
+__pycache__/
+*.py[cod]
+.pytest_cache/
+.ruff_cache/
+.venv/
+.env🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 265 - 270, The Python ignore lines (__pycache__/,
*.py[cod], .pytest_cache/, .ruff_cache/, .venv/) are currently inserted in the
middle of the LaTeX auxiliary-files block (near the "# xcolor" entry); move
these Python-specific entries out of that block and into a dedicated Python
section (or append them to the end of the file near the docs/ entries) so the
LaTeX section remains contiguous and Python ignores are grouped together.
| #*Notes.bib | ||
|
|
||
|
|
||
| docs/ |
Contributor
There was a problem hiding this comment.
docs/ blanket ignore may silently drop versioned documentation.
Ignoring the entire docs/ directory is appropriate only when its contents are fully auto-generated (e.g., Sphinx HTML output). If the directory also contains hand-written files — ADRs, architecture proposals, migration notes, or API references — they will be invisible to git and permanently unversioned without any warning.
If docs are auto-generated, scope the rule to the build output sub-path instead:
🛡️ Proposed scoped alternative
-docs/
+docs/_build/📝 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
| docs/ | |
| docs/_build/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 340, The .gitignore currently contains a blanket "docs/"
rule which can silently omit handwritten documentation; update the .gitignore by
narrowing or removing that rule: either remove "docs/" to keep the directory
versioned, replace it with a scoped path for generated output (e.g.,
"docs/_build/" or "docs/build/") if docs are generated, or add explicit
allow-list exceptions (e.g., negate patterns like "!docs/**/*.md" or specific
files) so ADRs and manual docs are not ignored; locate the "docs/" entry in
.gitignore and apply one of these scoped alternatives accordingly.
| @@ -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
Consider adding a clean target.
checkmake flags the missing clean phony target. Cleaning __pycache__, .ruff_cache, dist, and *.egg-info is useful in CI and local development.
♻️ Proposed addition
-.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 {} +
+ find . -type d -name '*.egg-info' -exec rm -rf {} +
+ rm -rf .ruff_cache dist📝 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
| .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 {} + | |
| find . -type d -name '*.egg-info' -exec rm -rf {} + | |
| rm -rf .ruff_cache dist |
🧰 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 phony target to the Makefile and include
it in the existing .PHONY list: declare a target named clean that removes build
and cache artifacts (e.g., __pycache__, .ruff_cache, dist, and any *.egg-info)
so CI and local dev can reset state; update the .PHONY line (currently listing
install dev-install test lint format start-node) to also include clean and
ensure the clean target performs safe removals (rm -rf or equivalent) for those
directories/patterns.
Comment on lines
+66
to
+85
| 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") | ||
|
|
||
|
|
||
| def test_genesis_block_rejects_wrong_previous_hash() -> None: | ||
| config = GenesisConfig(initial_balances={"ee" * 20: 10}) | ||
| block = create_genesis_block(config) | ||
| block.header = replace(block.header, previous_hash="11" * 32) | ||
| state = State() | ||
|
|
||
| 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") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace bare try/except + assertion with pytest.raises.
The pattern used in test_genesis_requires_empty_state and test_genesis_block_rejects_wrong_previous_hash is flagged by Ruff (PT017). The other test files (test_block.py, test_state.py) consistently use pytest.raises, so these two tests are out of step with the rest of the suite.
♻️ Proposed refactor
- 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)Also add import pytest at the top of the file.
📝 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
| 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") | |
| def test_genesis_block_rejects_wrong_previous_hash() -> None: | |
| config = GenesisConfig(initial_balances={"ee" * 20: 10}) | |
| block = create_genesis_block(config) | |
| block.header = replace(block.header, previous_hash="11" * 32) | |
| state = State() | |
| 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="empty state"): | |
| apply_genesis_block(state, block, config) | |
| def test_genesis_block_rejects_wrong_previous_hash() -> None: | |
| config = GenesisConfig(initial_balances={"ee" * 20: 10}) | |
| block = create_genesis_block(config) | |
| block.header = replace(block.header, previous_hash="11" * 32) | |
| state = State() | |
| with pytest.raises(ValueError, match="previous_hash"): | |
| apply_genesis_block(state, block, config) |
🧰 Tools
🪛 Ruff (0.15.1)
[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)
[warning] 83-83: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 85-85: 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 66 - 85, Replace the bare try/except
assertion pattern in the tests test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises: import pytest
at the top of the file, then use with pytest.raises(ValueError, match="empty
state") when calling apply_genesis_block(state, block, config) in the first
test, and with pytest.raises(ValueError, match="previous_hash") when calling
apply_genesis_block(state, block, config) in the second test (references:
apply_genesis_block, create_genesis_block, GenesisConfig, State).
Comment on lines
+23
to
+26
| def test_component_modules_are_importable() -> None: | ||
| for module in COMPONENT_MODULES: | ||
| imported = importlib.import_module(f"minichain.{module}") | ||
| assert imported is not None |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parametrizing the module import test for better failure diagnostics.
Currently, if mempool fails to import, the test stops and you don't know whether consensus, network, etc. also fail. Using @pytest.mark.parametrize reports each module independently.
Proposed refactor
+import pytest
+
-def test_component_modules_are_importable() -> None:
- for module in COMPONENT_MODULES:
- imported = importlib.import_module(f"minichain.{module}")
- assert imported is not None
+@pytest.mark.parametrize("module", COMPONENT_MODULES)
+def test_component_modules_are_importable(module: str) -> None:
+ imported = importlib.import_module(f"minichain.{module}")
+ assert imported is not None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_component_modules_are_importable() -> None: | |
| for module in COMPONENT_MODULES: | |
| imported = importlib.import_module(f"minichain.{module}") | |
| assert imported is not None | |
| `@pytest.mark.parametrize`("module", COMPONENT_MODULES) | |
| def test_component_modules_are_importable(module: str) -> None: | |
| imported = importlib.import_module(f"minichain.{module}") | |
| assert imported is not None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_scaffold.py` around lines 23 - 26, Refactor
test_component_modules_are_importable to be parametrized with
pytest.mark.parametrize so each entry in COMPONENT_MODULES is a separate test
case; replace the in-function loop over COMPONENT_MODULES with a test that
accepts a single module name parameter and calls
importlib.import_module(f"minichain.{module}") and asserts the result is not
None, optionally providing readable ids via pytest.param for better diagnostics
(refer to symbols test_component_modules_are_importable, COMPONENT_MODULES, and
importlib.import_module).
Comment on lines
+68
to
+69
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a tuple for the first argument of pytest.mark.parametrize (Ruff PT006).
The comma-separated string form is functionally valid but Ruff PT006 requires a tuple when multiple parameter names are provided.
♻️ 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", | |
| `@pytest.mark.parametrize`( | |
| ("payload", "serializer", "expected"), |
🧰 Tools
🪛 Ruff (0.15.1)
[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 - 69, The pytest parametrize
call uses a comma-separated string for multiple parameter names which triggers
Ruff PT006; update the pytest.mark.parametrize invocation so the first argument
is a tuple of parameter names (e.g., ("payload", "serializer", "expected"))
instead of a comma-separated string to satisfy the linter and keep the
parameters payload, serializer, and expected in the same order.
Comment on lines
+49
to
+55
| def test_mismatched_public_key_and_sender_is_rejected() -> None: | ||
| tx, _ = _build_signed_transaction() | ||
| other_signing_key, other_verify_key = generate_key_pair() | ||
| _ = other_signing_key | ||
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | ||
|
|
||
| assert not tampered.verify() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer direct _ unpacking to suppress the unused-variable.
_ = other_signing_key is a post-hoc suppression that leaves an extra named binding. The idiomatic form avoids the intermediate name entirely.
♻️ Proposed fix
- other_signing_key, other_verify_key = generate_key_pair()
- _ = other_signing_key
+ _, other_verify_key = generate_key_pair()📝 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 test_mismatched_public_key_and_sender_is_rejected() -> None: | |
| tx, _ = _build_signed_transaction() | |
| other_signing_key, other_verify_key = generate_key_pair() | |
| _ = other_signing_key | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | |
| assert not tampered.verify() | |
| def test_mismatched_public_key_and_sender_is_rejected() -> None: | |
| tx, _ = _build_signed_transaction() | |
| _, other_verify_key = generate_key_pair() | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | |
| assert not tampered.verify() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_transaction.py` around lines 49 - 55, In
test_mismatched_public_key_and_sender_is_rejected, avoid the post-hoc
suppression `_ = other_signing_key` by directly unpacking only the needed verify
key from generate_key_pair(); change the current unpacking to discard the unused
signing key (e.g., "_, other_verify_key = generate_key_pair()") and remove the
`_ = other_signing_key` line so the rest of the test (creating tampered =
replace(..., public_key=serialize_verify_key(other_verify_key)) and asserting
not tampered.verify()) remains unchanged.
Comment on lines
+77
to
+85
| def test_coinbase_with_auth_fields_is_rejected() -> None: | ||
| tx = create_coinbase_transaction( | ||
| miner_address="ef" * 20, | ||
| amount=55, | ||
| timestamp=1_739_760_111, | ||
| ) | ||
| tampered = replace(tx, signature="00" * 64) | ||
|
|
||
| assert not tampered.verify() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name implies both auth fields but only signature is exercised — add a public_key tamper case.
is_coinbase() gates on both self.signature == "" and self.public_key == "". A coinbase where only public_key is non-empty will also fail verification via the same is_coinbase() → normal-verify path, but that branch has no coverage. The name test_coinbase_with_auth_fields_is_rejected suggests both are tested.
♻️ Proposed fix — parametrize over both auth fields
-def test_coinbase_with_auth_fields_is_rejected() -> None:
- tx = create_coinbase_transaction(
- miner_address="ef" * 20,
- amount=55,
- timestamp=1_739_760_111,
- )
- tampered = replace(tx, signature="00" * 64)
-
- assert not tampered.verify()
+@pytest.mark.parametrize(
+ ("field", "value"),
+ [
+ ("signature", "00" * 64),
+ ("public_key", "00" * 32),
+ ],
+)
+def test_coinbase_with_auth_fields_is_rejected(field: str, value: str) -> None:
+ tx = create_coinbase_transaction(
+ miner_address="ef" * 20,
+ amount=55,
+ timestamp=1_739_760_111,
+ )
+ tampered = replace(tx, **{field: value})
+
+ assert not tampered.verify()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_transaction.py` around lines 77 - 85, The test currently only
tampers with signature but the coinbase gating in is_coinbase() checks both
signature and public_key; update test_coinbase_with_auth_fields_is_rejected to
cover both auth fields by adding a second case (or parametrize) that creates a
coinbase via create_coinbase_transaction, uses replace to set public_key to a
non-empty value (e.g., "00"*... matching expected length), and asserts that
tampered.verify() is False; reference the existing use of
create_coinbase_transaction, replace, tampered.verify(), and the
is_coinbase()/signature/public_key logic when adding the new case.
This was referenced Feb 23, 2026
Author
|
Superseded by #31. 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 #29
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
Release Notes
New Features
Documentation
Tests
Chores