remove code smells, duplicate codex and refactor#116
Conversation
WalkthroughThis PR refactors main.py CLI/network flows with new helper functions (request_chain, parse_amount_fee, submit_and_broadcast), centralizes block validation in chain.py via a shared _apply_block pipeline, standardizes error handling in contract.py and rpc.py, refactors p2p.py misbehavior tracking around ValidationStatus, and updates persistence.py snapshot/banned-peer logic and state.py transaction rollback. ChangesCore refactor across CLI, chain, contract, network, and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant NetworkHandler
participant request_chain
participant P2PNetwork
participant Peer
CLI->>NetworkHandler: block/chain_response event
NetworkHandler->>request_chain: request_chain(network, start_index, limit)
request_chain->>P2PNetwork: broadcast chain_request message
P2PNetwork->>Peer: send chain_request
Peer-->>NetworkHandler: chain_response
sequenceDiagram
participant Caller
participant add_block
participant resolve_conflicts
participant _apply_block
participant State
Caller->>add_block: add_block(block)
add_block->>_apply_block: _apply_block(block, temp_state)
_apply_block->>State: validate/apply transactions
_apply_block-->>add_block: (status, difficulty, avg_block_time)
Caller->>resolve_conflicts: resolve_conflicts(chain)
resolve_conflicts->>_apply_block: _apply_block(block, temp_state)
_apply_block-->>resolve_conflicts: (status, difficulty, avg_block_time)
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| try: | ||
| conn = _connect(db_path) | ||
| except sqlite3.DatabaseError as exc: | ||
| raise ValueError(f"Invalid SQLite persistence data in '{db_path}'") from exc |
There was a problem hiding this comment.
@Zahnentferner Will it be better to keep these how they were before or just "invalid" is alright
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 51-55: `request_chain` drops the `asyncio.create_task` handle, so
the broadcast task can be garbage-collected before finishing. Update
`request_chain` to keep a strong reference to the task (for example via a
module-level task set as suggested), and remove it when the task completes; use
the existing `request_chain` and `network._broadcast_raw` symbols to place the
fix without changing the call sites.
In `@minichain/chain.py`:
- Around line 140-145: The validation status logic in the block link/hash check
is too broad because the string match in the exception handling can classify
fork or ordering mismatches as INVALID. Update the try/except path around
validate_block_link_and_hash so only true block-hash failures return
ValidationStatus.INVALID, while previous-hash or index/link mismatches stay
ValidationStatus.FAILED; use the existing validate_block_link_and_hash and
ValidationStatus symbols to narrow the condition or switch to typed exceptions.
In `@minichain/persistence.py`:
- Around line 275-320: The banned-peer persistence helpers are still
synchronous, and `ban_peer` is being called directly from the async
`_handle_validation_status` flow, which can block the event loop. Update the
async misbehavior handling path to offload the SQLite work for `ban_peer` (and
any similar calls like `is_peer_banned` if needed) through `asyncio.to_thread`
or an executor, keeping `_banned_peers_conn` and the existing persistence logic
intact.
🪄 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: ac2ff11f-af4a-41ab-8f4d-0b04a3a5a878
📒 Files selected for processing (8)
main.pyminichain/block.pyminichain/chain.pyminichain/contract.pyminichain/p2p.pyminichain/persistence.pyminichain/rpc.pyminichain/state.py
| def request_chain(network, start_index, limit): | ||
| """Broadcast a chain_request for blocks starting at start_index.""" | ||
| req = {"type": "chain_request", "data": {"start_index": start_index, "limit": limit}} | ||
| asyncio.create_task(network._broadcast_raw(req)) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
asyncio.create_task result not retained — tasks may be silently dropped.
request_chain creates a fire-and-forget task without keeping a strong reference. Per the asyncio docs, "Save a reference to the result of this function, to avoid a task disappearing mid-execution... The event loop only keeps weak references to tasks... A task that isn't referenced elsewhere may get garbage collected at any time, even before it's done." This helper is now called from 5+ call sites (hello, block, chain_response paths), amplifying the risk that chain sync requests are silently dropped, especially under CPython 3.12+ GC behavior.
🐛 Proposed fix using a module-level task set
+_background_tasks = set()
+
def request_chain(network, start_index, limit):
"""Broadcast a chain_request for blocks starting at start_index."""
req = {"type": "chain_request", "data": {"start_index": start_index, "limit": limit}}
- asyncio.create_task(network._broadcast_raw(req))
+ task = asyncio.create_task(network._broadcast_raw(req))
+ _background_tasks.add(task)
+ task.add_done_callback(_background_tasks.discard)📝 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.
| def request_chain(network, start_index, limit): | |
| """Broadcast a chain_request for blocks starting at start_index.""" | |
| req = {"type": "chain_request", "data": {"start_index": start_index, "limit": limit}} | |
| asyncio.create_task(network._broadcast_raw(req)) | |
| _background_tasks = set() | |
| def request_chain(network, start_index, limit): | |
| """Broadcast a chain_request for blocks starting at start_index.""" | |
| req = {"type": "chain_request", "data": {"start_index": start_index, "limit": limit}} | |
| task = asyncio.create_task(network._broadcast_raw(req)) | |
| _background_tasks.add(task) | |
| task.add_done_callback(_background_tasks.discard) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 54-54: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 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 51 - 55, `request_chain` drops the
`asyncio.create_task` handle, so the broadcast task can be garbage-collected
before finishing. Update `request_chain` to keep a strong reference to the task
(for example via a module-level task set as suggested), and remove it when the
task completes; use the existing `request_chain` and `network._broadcast_raw`
symbols to place the fix without changing the call sites.
Source: Linters/SAST tools
| try: | ||
| validate_block_link_and_hash(prev_block, block) | ||
| except ValueError as exc: | ||
| logger.warning("Block %s rejected: %s", block.index, exc) | ||
| status = ValidationStatus.INVALID if "hash" in str(exc) else ValidationStatus.FAILED | ||
| return status, difficulty, avg_block_time |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the exact messages raised and every caller that maps them to a status.
rg -nP -C2 'invalid (previous )?hash|invalid index' minichain/chain.py
rg -nP -C3 '_apply_block|validate_block_link_and_hash' minichain/ main.pyRepository: StabilityNexus/MiniChain
Length of output: 3921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the status enum and peer handling path.
sed -n '1,220p' minichain/validators.py
printf '\n--- main.py ---\n'
sed -n '1,260p' main.py
printf '\n--- p2p.py ---\n'
sed -n '1,320p' minichain/p2p.py
# Find every place ValidationStatus.INVALID is interpreted.
printf '\n--- INVALID usages ---\n'
rg -n -C3 'ValidationStatus\.INVALID|INVALID_THRESHOLD|_handle_validation_status|disconnect|ban' minichain main.pyRepository: StabilityNexus/MiniChain
Length of output: 39253
Avoid banning peers for fork/ordering mismatches
"hash" in str(exc) also matches "invalid previous hash", so a link/index mismatch is classified as INVALID instead of FAILED. P2PNetwork._handle_validation_status() immediately disconnects and bans INVALID peers with the default threshold of 1, so a routine fork or a peer that is simply ahead can be banned on the first block. Match only the block-hash failure here, or raise typed exceptions to keep link/order errors as FAILED.
🤖 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 140 - 145, The validation status logic in
the block link/hash check is too broad because the string match in the exception
handling can classify fork or ordering mismatches as INVALID. Update the
try/except path around validate_block_link_and_hash so only true block-hash
failures return ValidationStatus.INVALID, while previous-hash or index/link
mismatches stay ValidationStatus.FAILED; use the existing
validate_block_link_and_hash and ValidationStatus symbols to narrow the
condition or switch to typed exceptions.
| @contextmanager | ||
| def _banned_peers_conn(path: str, create: bool): | ||
| """ | ||
| Yield a connection with the banned_peers table ensured, always closing it. | ||
| When *create* is False and no DB exists yet, yields None so read-only callers | ||
| can short-circuit without touching the filesystem. | ||
| """ | ||
| db_path = os.path.join(path, _DB_FILE) | ||
| os.makedirs(path, exist_ok=True) | ||
| if not create and not os.path.exists(db_path): | ||
| yield None | ||
| return | ||
| if create: | ||
| 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()) | ||
| ) | ||
| yield conn | ||
| finally: | ||
| conn.close() | ||
|
|
||
| def ban_peer(peer_id: str, reason: str, path: str = ".") -> None: | ||
| with _banned_peers_conn(path, create=True) as conn, conn: | ||
| conn.execute( | ||
| "INSERT OR REPLACE INTO banned_peers (peer_id, reason, timestamp) VALUES (?, ?, ?)", | ||
| (peer_id, reason, time.time()) | ||
| ) | ||
|
|
||
| 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 _banned_peers_conn(path, create=False) as conn: | ||
| if conn is None: | ||
| return | ||
| 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() | ||
| with _banned_peers_conn(path, create=False) as conn: | ||
| if conn is None: | ||
| return False | ||
| return conn.execute("SELECT peer_id FROM banned_peers WHERE peer_id = ?", (peer_id,)).fetchone() is not None | ||
|
|
||
| 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) | ||
| with _banned_peers_conn(path, create=False) as conn: | ||
| if conn is None: | ||
| return [] | ||
| 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] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Blocking SQLite calls inside async misbehavior handling.
_banned_peers_conn and ban_peer/is_peer_banned remain synchronous, but per the provided p2p.py snippet, _handle_validation_status is an async def that calls ban_peer(...) directly on the event loop. Each ban opens a new SQLite connection and executes a blocking write, which can stall the asyncio loop under load (e.g., during simultaneous block validation). Consider running these calls via asyncio.to_thread/executor if this proves to be a bottleneck in practice.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 276-276: Missing return type annotation for private function _banned_peers_conn
(ANN202)
[warning] 276-276: Boolean-typed positional argument in function definition
(FBT001)
🤖 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 275 - 320, The banned-peer persistence
helpers are still synchronous, and `ban_peer` is being called directly from the
async `_handle_validation_status` flow, which can block the event loop. Update
the async misbehavior handling path to offload the SQLite work for `ban_peer`
(and any similar calls like `is_peer_banned` if needed) through
`asyncio.to_thread` or an executor, keeping `_banned_peers_conn` and the
existing persistence logic intact.
|
|
||
| SUPPORTED_MESSAGE_TYPES = {"hello", "tx", "block", "chain_request", "chain_response"} | ||
| PROTOCOL_ID = TProtocol("/minichain/1.0.0") | ||
| MAX_FRAME_BYTES = 1 * 1024 * 1024 # 1 MB |
There was a problem hiding this comment.
Should move magic numbers and constants to a separate config file.
|
|
||
| # Misbehavior tracking | ||
| # Misbehavior tracking, keyed directly by ValidationStatus so there is a | ||
| # single vocabulary for statuses (no parallel string keys to convert). |
There was a problem hiding this comment.
Can remove this comment about strings.
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