feat: mempool implementation with nonce queueing and fee-priority selection#31
Open
arunabha003 wants to merge 25 commits intoStabilityNexus:mainfrom
Open
feat: mempool implementation with nonce queueing and fee-priority selection#31arunabha003 wants to merge 25 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 25 commits intoStabilityNexus:mainfrom
Conversation
1 task
Contributor
WalkthroughThis pull request establishes a complete Python-based minichain blockchain scaffold, including build configuration, cryptographic primitives, consensus mechanisms, transaction and block models, state management, transaction mempool, and a comprehensive test suite covering all core components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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: The "Setup Python" step using actions/setup-python@v5
should enable pip dependency caching to avoid re-downloading packages each run;
update the step (named "Setup Python") to include with: cache: "pip" and set an
appropriate cache-dependency-path (e.g., requirements.txt or the repo paths that
list pip dependencies) so the action caches installed wheels based on those
files.
- Around line 13-16: The workflow uses mutable tags actions/checkout@v4 and
actions/setup-python@v5; update those action references to pinned commit SHAs
(while keeping the tag in a comment for readability) so the CI runs an immutable
release. Locate the uses: "actions/checkout" and "actions/setup-python" in the
workflow and replace the `@vX` tags with the corresponding full commit SHA for the
desired release, ensuring the comment retains the original tag (e.g., uses:
actions/checkout@<SHA> # v4) and do the same for actions/setup-python to
prevent silent updates.
In @.gitignore:
- Around line 265-270: The .gitignore Python block is missing a rule to ignore
local environment files; add a pattern to ignore ".env" while allowing a
checked-in template like ".env.example" (use a negation) so secrets (API keys,
private keys) are not accidentally committed; update the same block containing
"__pycache__/", "*.py[cod]", ".pytest_cache/", ".ruff_cache/", ".venv/" to
include ".env" and a negated "!.env.example".
- Around line 261-270: The Python ignore entries (__pycache__/, *.py[cod],
.pytest_cache/, .ruff_cache/, .venv/) are inserted mid-LaTeX section; remove
this block from its current location and relocate it to a new or existing
"Project / Python" section at the end of .gitignore together with other
project-specific entries like docs/, ensuring LaTeX auxiliary-file rules
(easy-todo, xcolor, etc.) remain contiguous and the file is cleanly sectioned
for readability.
In `@Makefile`:
- Line 3: Add a new clean Makefile target and include it in the .PHONY list:
update the .PHONY line (which currently lists install dev-install test lint
format start-node) to also include clean, then implement a clean target named
"clean" that removes common Python build/artifacts such as __pycache__
directories, .pytest_cache, *.pyc files and optional dist/build/egg-info; use rm
-rf patterns or find -name commands (e.g., find . -name '__pycache__' -exec rm
-rf {} + and similar for .pytest_cache and *.pyc) so running make clean reliably
tidies the repo.
In `@pyproject.toml`:
- Around line 17-19: The extras entry named network currently lists the wrong
PyPI package string "py-libp2p>=0.2.0"; update that entry to the correct PyPI
distribution name by replacing "py-libp2p>=0.2.0" with "libp2p>=0.2.0" in the
pyproject.toml extras -> network array so pip install -e .[network] can find the
package.
In `@src/minichain/genesis.py`:
- Around line 14-17: The _is_lower_hex helper in genesis.py is duplicated in
transaction.py; extract it into a single shared utility module (e.g., create a
minichain.utils or minichain.serialization module) and replace both copies with
an import. Move the function definition (named _is_lower_hex) into the new
module, update genesis.py and transaction.py to import _is_lower_hex from that
module, and remove the duplicate definitions so only the shared implementation
remains.
In `@src/minichain/mempool.py`:
- Around line 184-190: The eviction loop in the mempool (while
len(self._entries_by_id) > self.max_size) repeatedly calls min(...) over
self._entries_by_id which makes eviction O(n²) in worst cases; replace this with
a heap or sorted structure keyed by (fee, received_at) to achieve O(n log n)
eviction: build a heap of entries (or maintain one incrementally on
insert/remove), then pop the smallest entries and call self._remove_entry(entry)
until len(self._entries_by_id) <= self.max_size; ensure the heap and the
existing _entries_by_id stay synchronized when adding/removing entries so that
_remove_entry and any insertion methods update both structures.
- Around line 63-106: The add_transaction path accepts future-nonce transactions
without any balance or per-sender limit, enabling mempool DoS; update
add_transaction to reject or limit waiting transactions by adding a check when
transaction.nonce > sender_account.nonce: either require the sender has at least
some balance (e.g., >= transaction.fee or a configurable min_hold) or enforce a
per-sender waiting cap (check _sender_pools.setdefault(sender,
_SenderPool()).entries size against a config like max_waiting_per_sender) and
raise MempoolValidationError if the check fails; adjust related logic in
_recompute_sender_pool or evict if you add a new per-sender cap/config to ensure
consistent enforcement.
- Around line 108-144: The selection uses each entry's individual fee as the
heap key in get_transactions_for_mining which can mis-order senders when later
nonces have much lower fees; change the heap priority to reflect the sender's
cumulative fee for the contiguous ready window starting at the sender's first
ready nonce: for each sender in sender_ready compute sender_total_fee =
sum(entry.fee for entry in entries[0:K]) where K is the count of contiguous
ready entries (or cap by limit), push (-sender_total_fee,
entries[0].transaction.nonce, sender, 0, K) onto heap instead of
(-first.fee...), and when popping update the heap by recomputing the remaining
sender_total_fee for the sender (subtracting fees of selected nonces) before
pushing the next state; use the existing symbols sender_ready, _PoolEntry,
entry.fee, heap, heappush/heappop, and selected to locate and implement the
change.
- Around line 118-126: The loop over self._sender_pools.items() mutates the dict
because _recompute_sender_pool may call _remove_entry which pops from
_sender_pools; fix by iterating over a snapshot (e.g.,
list(self._sender_pools.items()) or list(self._sender_pools.keys())) so the
dictionary can be safely mutated during the loop; update the iteration in the
block that builds sender_ready (referencing _sender_pools,
_recompute_sender_pool, _remove_entry, and sender_ready) to use the snapshot
approach.
- Around line 146-165: In remove_confirmed_transactions, avoid recomputing
sender pools twice: either remove the first loop that calls
_recompute_sender_pool for each sender in touched_senders since the subsequent
loop over self._sender_pools already recomputes every sender, or change the
final loop to only recompute non-touched senders by iterating
self._sender_pools.keys() minus touched_senders; update the code around the
touched_senders set and the two _recompute_sender_pool calls accordingly so each
sender is recomputed exactly once.
In `@src/minichain/merkle.py`:
- Around line 8-9: The _hash_pair function currently concatenates raw bytes
without domain separation; change hashing to use explicit domain prefixes so
leaf and internal-node hashes cannot collide: update _hash_pair to prefix inputs
with an internal-node tag (e.g., b'\x01') before calling blake2b_digest, and
ensure wherever leaves are hashed (the leaf-hashing helper or the place that
constructs leaf hashes—look for functions or code that compute leaf digests)
uses a distinct leaf tag (e.g., b'\x00') before calling blake2b_digest; keep the
same blake2b_digest call but feed it the tagged bytes so that _hash_pair and the
leaf-hash implementation are unambiguous.
In `@src/minichain/node.py`:
- Line 7: The docstring "Start a MiniChain node (placeholder for Issue `#20`
integration)." contains an incorrect issue reference; update the docstring in
the node module to reference the correct issue (`#8`) or replace the issue mention
with a generic placeholder (e.g., "placeholder for mempool/node orchestration
integration") so it reflects this PR's intent; change the string in the
top-level docstring that mentions "Issue `#20`" to either "Issue `#8`" or a
non-issue placeholder, ensuring the text around "Start a MiniChain node" and any
similar comment is updated consistently.
In `@src/minichain/state.py`:
- Around line 45-71: apply_transaction currently calls get_account (which lazily
inserts empty Account entries) before validation, causing phantom accounts to
remain when validation fails; change the flow to perform non-mutating lookups
for sender/recipient during validation (e.g., use
self.accounts.get(transaction.sender) / self.accounts.get(transaction.recipient)
or construct temporary Account objects like the mempool does) and only call the
mutating get_account(...) once all checks pass and you are about to apply state
changes; update references in apply_transaction and ensure nonce/balance checks
use the temporary/non-mutating accounts so get_account side-effects are avoided
on validation failure.
In `@src/minichain/storage.py`:
- Line 1: The storage module is just a placeholder docstring and needs a real
storage abstraction and a concrete backend plus integration hooks; add an
abstract class StorageBackend (with open/close/get/put/delete/iterator methods),
implement at least one concrete backend (e.g., LevelDBStorage or SQLiteStorage)
that implements StorageBackend, and add a StorageManager or storage_factory
function to initialize the backend and expose it to state and mempool modules
(inject or import the StorageManager into state and mempool and replace any
direct file/db access with calls to StorageBackend.get/put/delete); ensure
lifecycle methods (open/close) are called during node startup/shutdown and
include basic error handling and unit tests for the new classes.
In `@src/minichain/transaction.py`:
- Around line 97-103: The sign method currently types signing_key as object
which loses type safety; update the parameter annotation in def sign(self,
signing_key: object) -> None to use the concrete SigningKey type (or at minimum
typing.Any) exported by minichain.crypto so static checkers can validate access
to .verify_key; adjust the import to bring SigningKey into scope (or alias it)
and keep usage of serialize_verify_key, sign_message, and signing_bytes
unchanged so public_key and signature assignment remains the same.
In `@tests/test_block.py`:
- Around line 49-51: The test helper _make_block_with_coinbase currently
discards miner_key with an explicit assignment `_ = miner_key`; instead unpack
the pair returned by generate_key_pair() idiomatically by binding the unused
value to `_`, e.g. change `miner_key, miner_verify = generate_key_pair()`
followed by `_ = miner_key` to `_, miner_verify = generate_key_pair()` so only
miner_verify is kept.
In `@tests/test_consensus.py`:
- Around line 60-73: Replace the manual reconstruction of mined_header with a
dataclasses.replace call to copy header and set the new nonce: use
dataclasses.replace(header, nonce=nonce) instead of manually reassigning every
BlockHeader field; locate this change in the test function
test_mining_finds_valid_nonce_for_reasonable_target where BlockHeader is
currently built from header values and swap it for dataclasses.replace(header,
nonce=nonce) so the test remains equivalent and concise.
- Around line 115-126: The test
test_difficulty_target_decreases_when_blocks_are_fast currently uses timestamps
that make the unbounded computed target equal to the minimum cap (500_000), so
it only verifies the cap behavior rather than an actual uncapped decrease;
update the test's timestamps (or heights) so elapsed time yields an unbounded
target above the floor (for example use timestamps like [0, 8, 16, 24, 32] to
produce unbounded=800_000) and assert new_target == 800_000 when calling
compute_next_difficulty_target with adjustment_interval=4 and
target_block_time_seconds=10 to ensure the decrease is genuine and not just the
cap.
- Around line 76-96: The tests use try/except + assert + manual AssertionError;
replace them with pytest.raises context managers: in
test_mining_honors_stop_event wrap the mine_block_header call in with
pytest.raises(MiningInterrupted, match="interrupted") and call
mine_block_header(header, max_nonce=1_000_000, stop_event=stop); in
test_mining_raises_when_nonce_range_exhausted use with
pytest.raises(RuntimeError, match="No valid nonce found") and call
mine_block_header(header, start_nonce=0, max_nonce=10). Ensure pytest is
imported at top if not already.
In `@tests/test_crypto.py`:
- Around line 40-47: The test function test_invalid_signature_is_rejected
declares an unused variable signing_key from generate_key_pair(), which triggers
a lint warning; fix it by either discarding that value (rename to _signing_key)
or removing the binding and only unpacking the required verify_key from
generate_key_pair() so the function still calls generate_key_pair(), uses
other_signing_key with sign_message to produce wrong_signature, and then calls
verify_signature to assert the rejection.
In `@tests/test_genesis.py`:
- Around line 60-85: The two tests test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash use try/except manual assertions;
change them to use pytest.raises(ValueError) context managers instead: wrap the
apply_genesis_block(...) call in with pytest.raises(ValueError) as excinfo: and
then assert the expected substring is in str(excinfo.value) (e.g., assert "empty
state" in str(excinfo.value) and assert "previous_hash" in str(excinfo.value)).
Keep the existing setup (GenesisConfig, create_genesis_block, State, replace)
and only replace the try/except blocks with the pytest.raises pattern.
In `@tests/test_mempool.py`:
- Around line 180-218: Add a new test named
test_nonce_promotion_stops_when_balance_exhausted that targets Mempool's
nonce-promotion balance gating: create a sender with limited balance and two
transactions with consecutive nonces where the first ready transaction's total
cost (amount+fee) exhausts the sender's available_balance so the second cannot
be promoted by _recompute_sender_pool; use the same helpers as existing tests
(generate_key_pair, derive_address, _signed_transaction, State, Mempool), add
the higher-nonce tx first to go into waiting, then add the lower-nonce tx and
assert that after addition ready_count() == 1 and waiting_count() == 1 and that
get_transactions_for_mining(state, limit=2) returns only the promotable tx
(nonce 0) while nonce 1 remains waiting.
- Around line 123-124: The test uses compound assertions which hide which
condition failed; update the two lines referencing selected[2] and selected[3]
to use separate assertions per condition: for each index (selected[2],
selected[3]) assert .sender == sender_a in one statement and assert .nonce ==
<expected> in a separate statement so failures show the exact failing comparison
(refer to selected, sender_a, and the .nonce checks).
- Around line 39-312: Many tests redundantly assign recipient_key then
immediately shadow it with `_ = recipient_key`; update each call site (e.g., in
test_deduplicates_transactions_by_id,
test_fee_priority_respects_sender_nonce_ordering,
test_evicts_low_fee_when_pool_exceeds_max_size,
test_nonce_gap_is_held_then_promoted_when_gap_filled,
test_confirmed_transaction_removal_revalidates_pending_set,
test_rejects_duplicate_sender_nonce_even_if_tx_id_differs,
test_evicts_stale_transactions_by_age) to unpack the unused value into `_`
directly (use "_, recipient_verify = generate_key_pair()") so the unused key is
not bound to a named variable; keep the rest of the test logic and variable
names (recipient_verify, recipient, etc.) unchanged.
In `@tests/test_scaffold.py`:
- Around line 23-26: The test_component_modules_are_importable test currently
assigns importlib.import_module(...) to a variable and asserts it's not None,
which is vacuous because import_module either returns a module or raises; remove
the unnecessary assignment and assertion inside the loop: iterate over
COMPONENT_MODULES and call importlib.import_module(f"minichain.{module}")
directly (no `imported` variable, no `assert imported is not None`) so any
ImportError will fail the test as intended.
In `@tests/test_serialization.py`:
- Around line 68-69: The parametrize decorator currently passes a single string
to pytest.mark.parametrize; change the first argument to be a tuple of parameter
names (e.g., ("payload", "serializer", "expected")) so pytest recognizes
separate parameters; update the pytest.mark.parametrize call in the
tests/test_serialization.py test (the decorator line using
pytest.mark.parametrize) to use a tuple instead of a string.
In `@tests/test_state.py`:
- Around line 15-33: The _signed_transaction helper is duplicated across tests
with inconsistent signatures; extract a single canonical helper into
tests/conftest.py (or tests/helpers.py) and update both tests to use it: choose
one consistent signature (e.g., sender_key, sender_address, recipient, amount,
nonce, fee=1, timestamp=1_739_900_000) and implementation (create Transaction,
set timestamp += nonce, call tx.sign(sender_key), return tx), remove the
duplicate definitions from tests/test_state.py and tests/test_mempool.py, and
import/use the shared helper in both test files so they rely on the same
function (refer to _signed_transaction in both test files and adjust call sites
if needed to match the unified parameter conventions).
- Around line 172-176: The two assertions that call
state.get_account(recipient_address) (balance and nonce) mutate state.accounts
by inserting a new Account; replace those with non-mutating checks (e.g., assert
recipient_address not in state.accounts or assert
state.accounts.get(recipient_address) is None) so you do not create an entry
during the rollback test; keep the existing sender assertions using
state.get_account(sender_address) as-is.
In `@tests/test_transaction.py`:
- Around line 51-53: Remove the redundant temporary assignment to
other_signing_key by unpacking the tuple directly into an unused placeholder:
replace the two-line pattern using generate_key_pair() and "_ =
other_signing_key" with a single unpack like "_, other_verify_key =
generate_key_pair()". Update the subsequent use of other_verify_key (e.g.,
serialize_verify_key(other_verify_key)) to remain unchanged; no other logic
needs modification.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
.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_mempool.pytests/test_merkle.pytests/test_scaffold.pytests/test_serialization.pytests/test_state.pytests/test_transaction.py
Comment on lines
+13
to
+16
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Python | ||
| uses: actions/setup-python@v5 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin GitHub Actions to commit SHAs instead of mutable version tags.
checkout@v4 and setup-python@v5 resolve to whatever commit the tag currently points to; a tag re-point silently executes different code in your pipeline. Best practice is to pin to an immutable SHA with the tag kept as a comment.
🔒 Proposed fix for action pinning
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4- uses: actions/setup-python@v5
+ uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5📝 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
| uses: actions/checkout@v4 | |
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 | |
| - name: Setup Python | |
| uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 13 - 16, The workflow uses mutable
tags actions/checkout@v4 and actions/setup-python@v5; update those action
references to pinned commit SHAs (while keeping the tag in a comment for
readability) so the CI runs an immutable release. Locate the uses:
"actions/checkout" and "actions/setup-python" in the workflow and replace the
`@vX` tags with the corresponding full commit SHA for the desired release,
ensuring the comment retains the original tag (e.g., uses:
actions/checkout@<SHA> # v4) and do the same for actions/setup-python to
prevent silent updates.
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
Add pip dependency caching to the setup-python step.
Without caching, every run re-downloads all packages. actions/setup-python supports this natively.
⚡ Proposed fix to enable pip cache
- 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, The "Setup Python" step using
actions/setup-python@v5 should enable pip dependency caching to avoid
re-downloading packages each run; update the step (named "Setup Python") to
include with: cache: "pip" and set an appropriate cache-dependency-path (e.g.,
requirements.txt or the repo paths that list pip dependencies) so the action
caches installed wheels based on those files.
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
Python entries are placed mid-LaTeX-section; consider grouping them at the bottom.
The new Python block (lines 261–270) is embedded between easy-todo and xcolor LaTeX auxiliary-file rules, and docs/ (line 340) is a separate stray entry at the end. Collecting all project-specific additions in one clearly labelled section at the bottom would improve readability.
♻️ Proposed reorganisation (move Python/project entries to end of file)
Remove the Python block from lines 261-270 and relocate everything after the last LaTeX entry:
+## Project-specific ignores
+# 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.*
+!.env.example
+
+# Generated documentation
+docs/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 261 - 270, The Python ignore entries (__pycache__/,
*.py[cod], .pytest_cache/, .ruff_cache/, .venv/) are inserted mid-LaTeX section;
remove this block from its current location and relocate it to a new or existing
"Project / Python" section at the end of .gitignore together with other
project-specific entries like docs/, ensuring LaTeX auxiliary-file rules
(easy-todo, xcolor, etc.) remain contiguous and the file is cleanly sectioned
for readability.
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.
Add .env to prevent accidental secret commits.
The Python block is missing a .env pattern. In a blockchain/crypto project, .env files frequently hold private keys, RPC endpoints, or API secrets. Without this entry a developer could inadvertently push credentials.
🔒 Proposed addition
# Python caches and virtualenvs
__pycache__/
*.py[cod]
.pytest_cache/
.ruff_cache/
.venv/
+.env
+.env.*
+!.env.exampleThe negation pattern preserves any checked-in .env.example template.
📝 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
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| .env | |
| .env.* | |
| !.env.example |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 265 - 270, The .gitignore Python block is missing a
rule to ignore local environment files; add a pattern to ignore ".env" while
allowing a checked-in template like ".env.example" (use a negation) so secrets
(API keys, private keys) are not accidentally committed; update the same block
containing "__pycache__/", "*.py[cod]", ".pytest_cache/", ".ruff_cache/",
".venv/" to include ".env" and a negated "!.env.example".
| @@ -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 for developer ergonomics.
Static analysis flags missing all and clean. all is irrelevant for a pure-Python project, but a clean target removing __pycache__, .pytest_cache, *.pyc, etc. is useful and keeps the repo tidy.
🧹 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 {} +
+ find . -type d -name ".pytest_cache" -exec rm -rf {} +
+ find . -name "*.pyc" -delete🧰 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 Makefile target and include it in the
.PHONY list: update the .PHONY line (which currently lists install dev-install
test lint format start-node) to also include clean, then implement a clean
target named "clean" that removes common Python build/artifacts such as
__pycache__ directories, .pytest_cache, *.pyc files and optional
dist/build/egg-info; use rm -rf patterns or find -name commands (e.g., find .
-name '__pycache__' -exec rm -rf {} + and similar for .pytest_cache and *.pyc)
so running make clean reliably tidies the repo.
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
Vacuous assertion — importlib.import_module never returns None.
importlib.import_module raises ImportError on failure; it always returns a module object on success. The assert imported is not None line will always be True if the preceding call didn't raise, so it tests nothing. Drop the variable assignment and assertion entirely — the bare import call is the real test.
♻️ Proposed fix
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}")🤖 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, The
test_component_modules_are_importable test currently assigns
importlib.import_module(...) to a variable and asserts it's not None, which is
vacuous because import_module either returns a module or raises; remove the
unnecessary assignment and assertion inside the loop: iterate over
COMPONENT_MODULES and call importlib.import_module(f"minichain.{module}")
directly (no `imported` variable, no `assert imported is not None`) so any
ImportError will fail the test as intended.
Comment on lines
+68
to
+69
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", |
Contributor
There was a problem hiding this comment.
Ruff PT006: first pytest.mark.parametrize argument should be a tuple, not a str.
🔧 Proposed fix
`@pytest.mark.parametrize`(
- "payload,serializer,expected",
+ ("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 parametrize decorator
currently passes a single string to pytest.mark.parametrize; change the first
argument to be a tuple of parameter names (e.g., ("payload", "serializer",
"expected")) so pytest recognizes separate parameters; update the
pytest.mark.parametrize call in the tests/test_serialization.py test (the
decorator line using pytest.mark.parametrize) to use a tuple instead of a
string.
Comment on lines
+15
to
+33
| def _signed_transaction( | ||
| sender_key: object, | ||
| sender_address: str, | ||
| recipient: str, | ||
| amount: int, | ||
| nonce: int, | ||
| fee: int = 1, | ||
| timestamp: int = 1_739_900_000, | ||
| ) -> Transaction: | ||
| tx = Transaction( | ||
| sender=sender_address, | ||
| recipient=recipient, | ||
| amount=amount, | ||
| nonce=nonce, | ||
| fee=fee, | ||
| timestamp=timestamp + nonce, | ||
| ) | ||
| tx.sign(sender_key) | ||
| return tx |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
_signed_transaction helper is duplicated from tests/test_mempool.py — consider a shared conftest.py.
tests/test_state.py and tests/test_mempool.py each define a _signed_transaction helper with identical logic but different parameter conventions (positional vs. keyword-only, different fee defaults). Extracting a single canonical version into tests/conftest.py (or a tests/helpers.py module) would eliminate the duplication and keep future helper changes in one place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_state.py` around lines 15 - 33, The _signed_transaction helper is
duplicated across tests with inconsistent signatures; extract a single canonical
helper into tests/conftest.py (or tests/helpers.py) and update both tests to use
it: choose one consistent signature (e.g., sender_key, sender_address,
recipient, amount, nonce, fee=1, timestamp=1_739_900_000) and implementation
(create Transaction, set timestamp += nonce, call tx.sign(sender_key), return
tx), remove the duplicate definitions from tests/test_state.py and
tests/test_mempool.py, and import/use the shared helper in both test files so
they rely on the same function (refer to _signed_transaction in both test files
and adjust call sites if needed to match the unified parameter conventions).
Comment on lines
+172
to
+176
| assert state.get_account(sender_address).balance == 100 | ||
| assert state.get_account(sender_address).nonce == 0 | ||
| assert state.get_account(recipient_address).balance == 0 | ||
| assert state.get_account(recipient_address).nonce == 0 | ||
| assert miner_address not in state.accounts |
Contributor
There was a problem hiding this comment.
get_account at lines 174–175 mutates state.accounts as a side effect.
After the rollback, recipient_address is absent from state.accounts. Calling state.get_account(recipient_address) creates and inserts a new Account() entry into the dict. The assertion still passes, but it leaves recipient_address in state.accounts. This is inconsistent with line 176, which uses not in state.accounts directly (no side-effect creation). Prefer the non-mutating form for rollback assertions:
🔧 Proposed fix
- assert state.get_account(recipient_address).balance == 0
- assert state.get_account(recipient_address).nonce == 0
+ assert state.accounts.get(recipient_address, Account()).balance == 0
+ assert state.accounts.get(recipient_address, Account()).nonce == 0
assert miner_address not in state.accounts📝 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 state.get_account(sender_address).balance == 100 | |
| assert state.get_account(sender_address).nonce == 0 | |
| assert state.get_account(recipient_address).balance == 0 | |
| assert state.get_account(recipient_address).nonce == 0 | |
| assert miner_address not in state.accounts | |
| assert state.get_account(sender_address).balance == 100 | |
| assert state.get_account(sender_address).nonce == 0 | |
| assert state.accounts.get(recipient_address, Account()).balance == 0 | |
| assert state.accounts.get(recipient_address, Account()).nonce == 0 | |
| assert miner_address not in state.accounts |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_state.py` around lines 172 - 176, The two assertions that call
state.get_account(recipient_address) (balance and nonce) mutate state.accounts
by inserting a new Account; replace those with non-mutating checks (e.g., assert
recipient_address not in state.accounts or assert
state.accounts.get(recipient_address) is None) so you do not create an entry
during the rollback test; keep the existing sender assertions using
state.get_account(sender_address) as-is.
Comment on lines
+51
to
+53
| other_signing_key, other_verify_key = generate_key_pair() | ||
| _ = other_signing_key | ||
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same redundant _ = other_signing_key pattern — unpack with _ directly.
- 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
| other_signing_key, other_verify_key = generate_key_pair() | |
| _ = other_signing_key | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | |
| _, other_verify_key = generate_key_pair() | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_transaction.py` around lines 51 - 53, Remove the redundant
temporary assignment to other_signing_key by unpacking the tuple directly into
an unused placeholder: replace the two-line pattern using generate_key_pair()
and "_ = other_signing_key" with a single unpack like "_, other_verify_key =
generate_key_pair()". Update the subsequent use of other_verify_key (e.g.,
serialize_verify_key(other_verify_key)) to remain unchanged; no other logic
needs modification.
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 #30
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
Chores