Feat/libp2p#111
Conversation
feat: implemented (EMA) difficulty adjustment
Feat/peer blacklisting
Feat/difficulty
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughThis PR adds EMA-based dynamic difficulty adjustment to the blockchain consensus (genesis config, block validation, reorg handling, mining) using a new ChangesDynamic Difficulty (EMA) Consensus
Peer Banning Persistence and CLI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Miner
participant PoW as mine_block
participant Blockchain
participant State
participant ChainStore as chain/state storage
Miner->>Blockchain: request current_difficulty
Blockchain-->>Miner: current_difficulty
Miner->>PoW: mine_block(block)
PoW-->>Miner: mined block (nonce, hash)
Miner->>Blockchain: add_block(block)
Blockchain->>Blockchain: check block.difficulty == current_difficulty
Blockchain->>State: validate_and_apply_with_status(tx)
State-->>Blockchain: ValidationStatus, Receipt
Blockchain->>Blockchain: verify receipts and state_root
Blockchain->>Blockchain: update avg_block_time and current_difficulty (EMA)
Blockchain->>ChainStore: commit state and chain
Blockchain-->>Miner: ValidationStatus.VALID
sequenceDiagram
participant CLI as main.py CLI
participant Persistence as persistence.py
participant DB as SQLite banned_peers
CLI->>Persistence: ban_peer(peer_id, reason)
Persistence->>DB: INSERT OR REPLACE with timestamp
CLI->>Persistence: list-banned -> get_banned_peers()
Persistence->>DB: SELECT ordered by timestamp
DB-->>Persistence: rows
Persistence-->>CLI: banned peers list
CLI->>Persistence: unban_peer(peer_id)
Persistence->>DB: DELETE peer_id row
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/chain.py (1)
133-186: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftKeep
add_blockboolean-compatible, or update every caller to compare againstValidationStatus.VALID.main.pystill usesif chain.add_block(...)at multiple sites, so rejected blocks will follow the success path and be broadcast / removed from the mempool. The tests also still useassertTrue(chain.add_block(...)), so they no longer distinguish success from failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@minichain/chain.py` around lines 133 - 186, The add_block flow in chain.py now returns ValidationStatus instead of a boolean, but existing callers still treat it as truthy. Either make add_block boolean-compatible again or update every caller of add_block, including the main.py broadcast/mempool paths and the tests using assertTrue, to compare explicitly against ValidationStatus.VALID so rejected blocks don’t follow the success path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.py`:
- Around line 426-455: Banned peers are only being recorded in persistence, but
the P2P connection flow still ignores that state. Update the relevant logic in
minichain/p2p.py to consult is_peer_banned() before accepting or maintaining a
connection, and ensure any banned peer is rejected/disconnected there. Use the
existing ban_peer/unban_peer helpers and the P2P connection-handling code paths
so the ban status is enforced at runtime, not just stored in SQLite.
In `@minichain/persistence.py`:
- Around line 271-283: The ban_peer helper currently writes whatever peer_id and
reason it receives, so add input validation before
_connect/_ensure_banned_peers_table is used. In ban_peer, reject non-string or
empty/whitespace-only peer_id values, validate reason is a string, and enforce a
reasonable maximum length for reason before inserting into banned_peers. Keep
the checks close to ban_peer so the CLI path in main.py cannot create rows with
an empty or malformed peer_id.
- Around line 271-320: The banned-peer persistence helpers are doing blocking
SQLite work directly from the async CLI path, which can stall the event loop.
Update the call sites in cli_loop to run ban_peer, unban_peer, and
get_banned_peers off the loop (for example via an executor or async wrapper),
and keep the synchronous DB helpers in persistence.py as the isolated
implementation behind those async-safe wrappers. Use the ban_peer, unban_peer,
and get_banned_peers symbols to locate the affected flow.
- Around line 266-320: The new banned-peers helpers repeat the same database
setup and teardown logic in ban_peer, unban_peer, is_peer_banned, and
get_banned_peers. Factor the shared db_path creation, _connect call,
_ensure_banned_peers_table invocation, and conn.close handling into a small
shared helper such as a context manager or private function, then update each of
those functions to use it. Keep the existing behavior for missing databases in
the read/delete paths, but centralize the connection lifecycle around the
banned_peers table.
- Around line 260-264: The module-level import for time is misplaced in the
middle of the file near the “Banned Peers (Track 1)” section; move it into the
top import block alongside the existing imports such as os and sqlite3 in
minichain/persistence.py. Keep the import list organized with all standard
library imports together and remove the mid-file import from that section.
In `@tests/test_difficulty.py`:
- Line 52: The result of chain1.resolve_conflicts(chain2.chain) assigns orphans
but the variable is never used, triggering Ruff RUF059. Update the test to avoid
binding the unused value in the test_difficulty.py case, either by ignoring it
directly or only capturing the value that is asserted, keeping the call to
resolve_conflicts and the relevant chain1/chain2 test logic intact.
- Line 19: `Blockchain.add_block()` returns a ValidationStatus enum, so the
current truthiness checks can incorrectly pass for non-VALID outcomes. Update
the assertions in `tests/test_difficulty.py` to compare the returned value from
`chain.add_block(...)` directly against `ValidationStatus.VALID`, using the
existing `chain.add_block` call sites in the difficulty tests.
In `@tests/test_persistence.py`:
- Line 256: The assertion on restored.add_block(block2) is too loose because
assertTrue will pass for any truthy ValidationStatus value, including rejected
states. Update the persistence test to compare the result from
restored.add_block and the ValidationStatus enum explicitly, using
ValidationStatus.VALID as the expected status so the test only passes when the
block is actually accepted.
---
Outside diff comments:
In `@minichain/chain.py`:
- Around line 133-186: The add_block flow in chain.py now returns
ValidationStatus instead of a boolean, but existing callers still treat it as
truthy. Either make add_block boolean-compatible again or update every caller of
add_block, including the main.py broadcast/mempool paths and the tests using
assertTrue, to compare explicitly against ValidationStatus.VALID so rejected
blocks don’t follow the success path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f1cc0c3-c7c8-4060-bccc-a4692824a6d8
📒 Files selected for processing (10)
genesis.jsonmain.pyminichain/chain.pyminichain/persistence.pyminichain/pow.pyminichain/state.pyminichain/validators.pytests/test_difficulty.pytests/test_persistence.pytests/test_reorg.py
| # ── list-banned ── | ||
| elif cmd == "list-banned": | ||
| from minichain.persistence import get_banned_peers | ||
| banned = get_banned_peers() | ||
| if not banned: | ||
| print(" No peers are currently banned.") | ||
| else: | ||
| print(f" {len(banned)} banned peer(s):") | ||
| for p in banned: | ||
| print(f" - {p['peer_id']} (Reason: {p['reason']}, Time: {p['timestamp']})") | ||
|
|
||
| # ── ban ── | ||
| elif cmd == "ban": | ||
| if len(parts) < 2: | ||
| print(" Usage: ban <peer_id>") | ||
| continue | ||
| peer_id = parts[1] | ||
| from minichain.persistence import ban_peer | ||
| ban_peer(peer_id, reason="Manual ban via CLI") | ||
| print(f" ✅ Peer {peer_id} banned.") | ||
|
|
||
| # ── unban ── | ||
| elif cmd == "unban": | ||
| if len(parts) < 2: | ||
| print(" Usage: unban <peer_id>") | ||
| continue | ||
| peer_id = parts[1] | ||
| from minichain.persistence import unban_peer | ||
| unban_peer(peer_id) | ||
| print(f" ✅ Peer {peer_id} unbanned.") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm banned-peer checks are wired into the networking layer
rg -nP -C3 '\bis_peer_banned\s*\(' --type=pyRepository: StabilityNexus/MiniChain
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg '(^|/)(main\.py|network\.py|.*persistence.*\.py|.*peer.*\.py)$'
echo
echo "== search for ban-related symbols =="
rg -n -C 3 '\b(ban_peer|unban_peer|get_banned_peers|is_peer_banned|banned peer|banned_peers|ban)\b' --type=py .
echo
echo "== inspect likely networking files =="
for f in $(git ls-files | rg '(^|/)network\.py$|(^|/)persistence\.py$|(^|/)main\.py$'); do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,260p' "$f" | cat -n
doneRepository: StabilityNexus/MiniChain
Length of output: 28629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all ban/network references =="
rg -n -C 3 '\b(is_peer_banned|banned_peers|ban_peer|unban_peer|get_banned_peers|disconnect_peer|connect_peer|_broadcast_raw|_unicast_raw|hello|chain_request|chain_response|tx|block)\b' . --type=py
echo
echo "== P2PNetwork definition(s) =="
rg -n -C 4 '\bclass\s+P2PNetwork\b|\bdef\s+(connect_peer|disconnect_peer|add_peer|remove_peer|handle_message|handle_connection|start|stop|listen)\b' . --type=py
echo
echo "== exported symbols around minichain package =="
git ls-files minichain | sortRepository: StabilityNexus/MiniChain
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== minichain/p2p.py (focused ranges) =="
cat -n minichain/p2p.py | sed -n '1,220p'
echo
echo "== exact ban symbol usage =="
rg -n -C 2 '\bis_peer_banned\b|\bban_peer\b|\bunban_peer\b|\bget_banned_peers\b' minichain main.py tests --type=pyRepository: StabilityNexus/MiniChain
Length of output: 12428
Wire banned-peer checks into the P2P path. ban_peer() only persists to SQLite; minichain/p2p.py never calls is_peer_banned(), so a banned peer can remain connected and reconnect normally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 426 - 455, Banned peers are only being recorded in
persistence, but the P2P connection flow still ignores that state. Update the
relevant logic in minichain/p2p.py to consult is_peer_banned() before accepting
or maintaining a connection, and ensure any banned peer is rejected/disconnected
there. Use the existing ban_peer/unban_peer helpers and the P2P
connection-handling code paths so the ban status is enforced at runtime, not
just stored in SQLite.
| # --------------------------------------------------------------------------- | ||
| # Banned Peers (Track 1) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| import time |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move import time to the top-level imports.
Placing import time mid-file, inside a new section, is inconsistent with standard module structure and existing import conventions (likely already established at the top of this file for os, sqlite3, etc.).
♻️ Suggested fix
# ---------------------------------------------------------------------------
# Banned Peers (Track 1)
# ---------------------------------------------------------------------------
-import time
-
def _ensure_banned_peers_table(conn: sqlite3.Connection) -> None:And add import time alongside the other imports at the top of the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/persistence.py` around lines 260 - 264, The module-level import for
time is misplaced in the middle of the file near the “Banned Peers (Track 1)”
section; move it into the top import block alongside the existing imports such
as os and sqlite3 in minichain/persistence.py. Keep the import list organized
with all standard library imports together and remove the mid-file import from
that section.
| def _ensure_banned_peers_table(conn: sqlite3.Connection) -> None: | ||
| conn.execute( | ||
| "CREATE TABLE IF NOT EXISTS banned_peers (peer_id TEXT PRIMARY KEY, reason TEXT, timestamp REAL)" | ||
| ) | ||
|
|
||
| def ban_peer(peer_id: str, reason: str, path: str = ".") -> None: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| os.makedirs(path, exist_ok=True) | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| with conn: | ||
| conn.execute( | ||
| "INSERT OR REPLACE INTO banned_peers (peer_id, reason, timestamp) VALUES (?, ?, ?)", | ||
| (peer_id, reason, time.time()) | ||
| ) | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def unban_peer(peer_id: str, path: str = ".") -> None: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| with conn: | ||
| conn.execute("DELETE FROM banned_peers WHERE peer_id = ?", (peer_id,)) | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def is_peer_banned(peer_id: str, path: str = ".") -> bool: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return False | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| row = conn.execute("SELECT peer_id FROM banned_peers WHERE peer_id = ?", (peer_id,)).fetchone() | ||
| return row is not None | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def get_banned_peers(path: str = ".") -> list[dict[str, Any]]: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return [] | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| rows = conn.execute("SELECT peer_id, reason, timestamp FROM banned_peers ORDER BY timestamp DESC").fetchall() | ||
| return [{"peer_id": r["peer_id"], "reason": r["reason"], "timestamp": r["timestamp"]} for r in rows] | ||
| finally: | ||
| conn.close() | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Repeated connect/ensure-table/close boilerplate across all four functions.
Each of ban_peer, unban_peer, is_peer_banned, and get_banned_peers repeats the same connect → _ensure_banned_peers_table → try/finally close pattern, along with duplicated os.path.join(path, _DB_FILE) construction. Consider factoring this into a small context manager (e.g., _banned_peers_connection(path)) that yields a ready connection with the table ensured, or a decorator, to reduce duplication and centralize connection handling for this new table.
♻️ Example consolidation
+from contextlib import contextmanager
+
+@contextmanager
+def _banned_peers_conn(path: str, create_if_missing: bool = True):
+ db_path = os.path.join(path, _DB_FILE)
+ if not create_if_missing and not os.path.exists(db_path):
+ yield None
+ return
+ os.makedirs(path, exist_ok=True)
+ conn = _connect(db_path)
+ try:
+ _ensure_banned_peers_table(conn)
+ yield conn
+ finally:
+ conn.close()Each function can then use this helper, handling the None case for read/delete operations against a nonexistent DB.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/persistence.py` around lines 266 - 320, The new banned-peers
helpers repeat the same database setup and teardown logic in ban_peer,
unban_peer, is_peer_banned, and get_banned_peers. Factor the shared db_path
creation, _connect call, _ensure_banned_peers_table invocation, and conn.close
handling into a small shared helper such as a context manager or private
function, then update each of those functions to use it. Keep the existing
behavior for missing databases in the read/delete paths, but centralize the
connection lifecycle around the banned_peers table.
| def ban_peer(peer_id: str, reason: str, path: str = ".") -> None: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| os.makedirs(path, exist_ok=True) | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| with conn: | ||
| conn.execute( | ||
| "INSERT OR REPLACE INTO banned_peers (peer_id, reason, timestamp) VALUES (?, ?, ?)", | ||
| (peer_id, reason, time.time()) | ||
| ) | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
No validation on peer_id/reason inputs.
ban_peer accepts arbitrary strings without checking for emptiness or type, and there's no upper bound on reason length. Given this is reachable from the CLI (ban <peer_id> in main.py) with user-supplied input, an empty or malformed peer_id would silently create a banned-peer row with an empty primary key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/persistence.py` around lines 271 - 283, The ban_peer helper
currently writes whatever peer_id and reason it receives, so add input
validation before _connect/_ensure_banned_peers_table is used. In ban_peer,
reject non-string or empty/whitespace-only peer_id values, validate reason is a
string, and enforce a reasonable maximum length for reason before inserting into
banned_peers. Keep the checks close to ban_peer so the CLI path in main.py
cannot create rows with an empty or malformed peer_id.
| def ban_peer(peer_id: str, reason: str, path: str = ".") -> None: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| os.makedirs(path, exist_ok=True) | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| with conn: | ||
| conn.execute( | ||
| "INSERT OR REPLACE INTO banned_peers (peer_id, reason, timestamp) VALUES (?, ?, ?)", | ||
| (peer_id, reason, time.time()) | ||
| ) | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def unban_peer(peer_id: str, path: str = ".") -> None: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| with conn: | ||
| conn.execute("DELETE FROM banned_peers WHERE peer_id = ?", (peer_id,)) | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def is_peer_banned(peer_id: str, path: str = ".") -> bool: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return False | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| row = conn.execute("SELECT peer_id FROM banned_peers WHERE peer_id = ?", (peer_id,)).fetchone() | ||
| return row is not None | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def get_banned_peers(path: str = ".") -> list[dict[str, Any]]: | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| if not os.path.exists(db_path): | ||
| return [] | ||
| conn = _connect(db_path) | ||
| try: | ||
| _ensure_banned_peers_table(conn) | ||
| rows = conn.execute("SELECT peer_id, reason, timestamp FROM banned_peers ORDER BY timestamp DESC").fetchall() | ||
| return [{"peer_id": r["peer_id"], "reason": r["reason"], "timestamp": r["timestamp"]} for r in rows] | ||
| finally: | ||
| conn.close() | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Synchronous, blocking sqlite calls invoked from async CLI context.
main.py's cli_loop is an async def that awaits input() via an executor, and directly calls ban_peer, unban_peer, and get_banned_peers synchronously inline. Each call opens/closes a fresh SQLite connection and executes a blocking CREATE TABLE IF NOT EXISTS. For a single-user interactive CLI this is low risk, but if this network layer / cli_loop shares an event loop with other async network I/O (peer connections, block broadcasting per the AI summary), these blocking calls will stall the loop until the DB operation completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/persistence.py` around lines 271 - 320, The banned-peer persistence
helpers are doing blocking SQLite work directly from the async CLI path, which
can stall the event loop. Update the call sites in cli_loop to run ban_peer,
unban_peer, and get_banned_peers off the loop (for example via an executor or
async wrapper), and keep the synchronous DB helpers in persistence.py as the
isolated implementation behind those async-safe wrappers. Use the ban_peer,
unban_peer, and get_banned_peers symbols to locate the affected flow.
| ts = chain.last_block.timestamp + 1 | ||
| block1 = Block(index=1, previous_hash=chain.last_block.hash, transactions=[], timestamp=ts, difficulty=chain.current_difficulty, state_root=chain.state.state_root()) | ||
| mined_block1 = mine_block(block1) | ||
| self.assertTrue(chain.add_block(mined_block1)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ValidationStatus definition/base type and VALID value
rg -n "class ValidationStatus" -A 10 minichain/validators.pyRepository: StabilityNexus/MiniChain
Length of output: 412
Assert ValidationStatus.VALID instead of truthiness. Blockchain.add_block() returns a ValidationStatus Enum with auto() members, so assertTrue(chain.add_block(...)) will pass for INVALID/FAILED too. Compare the result directly against ValidationStatus.VALID in tests/test_difficulty.py:19, 27, 48.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_difficulty.py` at line 19, `Blockchain.add_block()` returns a
ValidationStatus enum, so the current truthiness checks can incorrectly pass for
non-VALID outcomes. Update the assertions in `tests/test_difficulty.py` to
compare the returned value from `chain.add_block(...)` directly against
`ValidationStatus.VALID`, using the existing `chain.add_block` call sites in the
difficulty tests.
| self.assertEqual(chain2.current_difficulty, 2) | ||
|
|
||
| # Reorg chain1 to chain2 | ||
| success, orphans = chain1.resolve_conflicts(chain2.chain) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Unused orphans variable (Ruff RUF059).
🔧 Proposed fix
- success, orphans = chain1.resolve_conflicts(chain2.chain)
+ success, _orphans = chain1.resolve_conflicts(chain2.chain)📝 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.
| success, orphans = chain1.resolve_conflicts(chain2.chain) | |
| success, _orphans = chain1.resolve_conflicts(chain2.chain) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 52-52: Unpacked variable orphans is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_difficulty.py` at line 52, The result of
chain1.resolve_conflicts(chain2.chain) assigns orphans but the variable is never
used, triggering Ruff RUF059. Update the test to avoid binding the unused value
in the test_difficulty.py case, either by ignoring it directly or only capturing
the value that is asserted, keeping the call to resolve_conflicts and the
relevant chain1/chain2 test logic intact.
Source: Linters/SAST tools
| mine_block(block2, difficulty=1) | ||
| mine_block(block2) | ||
|
|
||
| self.assertTrue(restored.add_block(block2)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
printf 'Repo root: '; pwd
git ls-files | rg '(^tests/test_persistence\.py$|^tests/test_difficulty\.py$|ValidationStatus|add_block)'
echo '--- tests/test_persistence.py around target ---'
sed -n '220,280p' tests/test_persistence.py
echo '--- tests/test_difficulty.py around target ---'
sed -n '1,220p' tests/test_difficulty.py
echo '--- search ValidationStatus ---'
rg -n "class ValidationStatus|ValidationStatus|def add_block|return ValidationStatus|VALID" .Repository: StabilityNexus/MiniChain
Length of output: 7528
🏁 Script executed:
set -e
echo '--- minichain/validators.py ---'
sed -n '1,120p' minichain/validators.py
echo '--- minichain/chain.py add_block ---'
sed -n '120,195p' minichain/chain.pyRepository: StabilityNexus/MiniChain
Length of output: 4176
Use an explicit status assertion here restored.add_block(block2) returns a ValidationStatus, and assertTrue will accept any enum member, including rejected states. Compare against ValidationStatus.VALID instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_persistence.py` at line 256, The assertion on
restored.add_block(block2) is too loose because assertTrue will pass for any
truthy ValidationStatus value, including rejected states. Update the persistence
test to compare the result from restored.add_block and the ValidationStatus enum
explicitly, using ValidationStatus.VALID as the expected status so the test only
passes when the block is actually accepted.
2 participants
Addressed Issues:
Fixes #(TODO:issue number)
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
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. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Bug Fixes