Skip to content

remove code smells, duplicate codex and refactor #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SIDDHANTCOOKIE wants to merge 2 commits into
base: main
Choose a base branch
from
+271 −297
Open

remove code smells, duplicate codex and refactor #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6694b7
remove code smells, duplicate codex and refactor
SIDDHANTCOOKIE Jul 5, 2026
c97295b
refactor codebase
SIDDHANTCOOKIE Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 59 additions & 47 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import argparse
import asyncio
import json
import logging
import re
import sys
Expand Down Expand Up @@ -47,6 +48,36 @@ def create_wallet():
return sk, pk


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))

Comment on lines +51 to +55

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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


def parse_amount_fee(parts, start):
"""Parse optional non-negative amount/fee at parts[start] and parts[start+1].
Returns (amount, fee), or None if invalid (a message is printed for the user)."""
try:
amount = int(parts[start]) if len(parts) > start else 0
fee = int(parts[start + 1]) if len(parts) > start + 1 else 0
except ValueError:
print(" Amount and fee must be integers.")
return None
if amount < 0 or fee < 0:
print(" Amount and fee cannot be negative.")
return None
return amount, fee


async def submit_and_broadcast(mempool, network, tx, ok_msg, reject_msg):
"""Add a signed tx to the mempool and broadcast it, printing the outcome."""
if mempool.add_transaction(tx):
await network.broadcast_transaction(tx)
print(ok_msg)
else:
print(reject_msg)


# ──────────────────────────────────────────────
# Block mining
# ──────────────────────────────────────────────
Expand Down Expand Up @@ -117,7 +148,6 @@ def mine_and_process_block(chain, mempool, miner_pk):

def make_network_handler(chain, mempool, network):
"""Return an async callback that processes incoming P2P messages."""
from minichain.validators import ValidationStatus

async def handler(data):
msg_type = data.get("type")
Expand All @@ -143,8 +173,7 @@ async def handler(data):
peer_tip = payload.get("latest_block_index", 0)
if peer_tip > chain.last_block.index:
logger.info("📡 Peer %s is ahead (%d > %d). Initiating chunked sync...", peer_addr, peer_tip, chain.last_block.index)
req = {"type": "chain_request", "data": {"start_index": chain.last_block.index + 1, "limit": 500}}
asyncio.create_task(network._broadcast_raw(req))
request_chain(network, chain.last_block.index + 1, 500)

elif msg_type == "tx":
try:
Expand Down Expand Up @@ -178,13 +207,11 @@ async def handler(data):
else:
if block.index > chain.last_block.index + 1:
logger.warning("📥 Received Block #%s — ahead of us (tip: %s). Requesting chunked sync...", block.index, chain.last_block.index)
req = {"type": "chain_request", "data": {"start_index": chain.last_block.index + 1, "limit": 500}}
asyncio.create_task(network._broadcast_raw(req))
request_chain(network, chain.last_block.index + 1, 500)
else:
logger.warning("📥 Received Block #%s — rejected. Fork detected, trigger reorg sync.", block.index)
# For a fork, request the full chain to use resolve_conflicts
req = {"type": "chain_request", "data": {"start_index": 0, "limit": 1000000}} # Request full chain for reorg
asyncio.create_task(network._broadcast_raw(req))
request_chain(network, 0, 1000000)
return status

elif msg_type == "chain_request":
Expand Down Expand Up @@ -234,17 +261,15 @@ async def handler(data):
mempool.remove_transactions(block.transactions)
else:
logger.warning("❌ Sync failed at Block #%d. Fork detected. Requesting full chain.", block.index)
req = {"type": "chain_request", "data": {"start_index": 0, "limit": 1000000}}
asyncio.create_task(network._broadcast_raw(req))
request_chain(network, 0, 1000000)
all_added = False
break

# If we added all blocks and we hit the limit, request next batch
if all_added and len(new_chain) == requested_limit:
next_index = chain.last_block.index + 1
logger.info("📡 Requesting next batch from index %d", next_index)
req = {"type": "chain_request", "data": {"start_index": next_index, "limit": requested_limit}}
asyncio.create_task(network._broadcast_raw(req))
request_chain(network, next_index, requested_limit)

return handler

Expand Down Expand Up @@ -360,11 +385,11 @@ async def cli_loop(sk, pk, chain, mempool, network, datadir: str | None = None):
tx = Transaction(sender=pk, receiver=receiver, amount=amount, nonce=nonce, fee=fee, chain_id=chain.chain_id)
tx.sign(sk)

if mempool.add_transaction(tx):
await network.broadcast_transaction(tx)
print(f" {C_GREEN}✅ Tx sent:{C_RESET} {amount} coins → {receiver[:12]}...")
else:
print(f" {C_RED}❌ Transaction rejected{C_RESET} (invalid sig, duplicate, or mempool full).")
await submit_and_broadcast(
mempool, network, tx,
f" {C_GREEN}✅ Tx sent:{C_RESET} {amount} coins → {receiver[:12]}...",
f" {C_RED}❌ Transaction rejected{C_RESET} (invalid sig, duplicate, or mempool full).",
)

# ── deploy ──
elif cmd == "deploy":
Expand All @@ -379,26 +404,20 @@ async def cli_loop(sk, pk, chain, mempool, network, datadir: str | None = None):
print(f" File not found: {filepath}")
continue

try:
amount = int(parts[2]) if len(parts) > 2 else 0
fee = int(parts[3]) if len(parts) > 3 else 0
except ValueError:
print(" Amount and fee must be integers.")
continue

if amount < 0 or fee < 0:
print(" Amount and fee cannot be negative.")
parsed = parse_amount_fee(parts, 2)
if parsed is None:
continue
amount, fee = parsed

nonce = chain.state.get_account(pk).get("nonce", 0)
tx = Transaction(sender=pk, receiver=None, amount=amount, nonce=nonce, fee=fee, data=code, chain_id=chain.chain_id)
tx.sign(sk)

if mempool.add_transaction(tx):
await network.broadcast_transaction(tx)
print(f" ✅ Deploy Tx sent (nonce={nonce}). Mine a block to confirm.")
else:
print(" ❌ Deploy Transaction rejected.")
await submit_and_broadcast(
mempool, network, tx,
f" ✅ Deploy Tx sent (nonce={nonce}). Mine a block to confirm.",
" ❌ Deploy Transaction rejected.",
)

# ── call ──
elif cmd == "call":
Expand All @@ -410,27 +429,21 @@ async def cli_loop(sk, pk, chain, mempool, network, datadir: str | None = None):
print(" Invalid receiver format. Expected 40 or 64 hex characters.")
continue
payload = parts[2]

try:
amount = int(parts[3]) if len(parts) > 3 else 0
fee = int(parts[4]) if len(parts) > 4 else 0
except ValueError:
print(" Amount and fee must be integers.")
continue

if amount < 0 or fee < 0:
print(" Amount and fee cannot be negative.")
parsed = parse_amount_fee(parts, 3)
if parsed is None:
continue
amount, fee = parsed

nonce = chain.state.get_account(pk).get("nonce", 0)
tx = Transaction(sender=pk, receiver=receiver, amount=amount, nonce=nonce, fee=fee, data=payload, chain_id=chain.chain_id)
tx.sign(sk)

if mempool.add_transaction(tx):
await network.broadcast_transaction(tx)
print(f" ✅ Call Tx sent to {receiver[:12]}... (payload='{payload}'). Mine a block to confirm.")
else:
print(" ❌ Call Transaction rejected.")
await submit_and_broadcast(
mempool, network, tx,
f" ✅ Call Tx sent to {receiver[:12]}... (payload='{payload}'). Mine a block to confirm.",
" ❌ Call Transaction rejected.",
)

# ── mine ──
elif cmd == "mine":
Expand Down Expand Up @@ -545,8 +558,7 @@ async def run_node(port: int, host: str, connect_to: str | None, fund: int, data

# When a new peer connects, send our hello so they can handshake
async def on_peer_connected(writer):
import json as _json
sync_msg = _json.dumps({
sync_msg = json.dumps({
"type": "hello",
"data": {
"chain_id": chain.chain_id,
Expand Down
23 changes: 10 additions & 13 deletions minichain/block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import time
import hashlib
from typing import Optional # <-- Removed 'List' as requested
from typing import Optional
from collections.abc import Sequence

from .transaction import Transaction
Expand All @@ -24,7 +24,6 @@ def _calculate_merkle_tree(hashes: Sequence[str]) -> Optional[str]:
hashes_list = new_level
return hashes_list[0]

# <-- Updated to Sequence to accept the frozen tuple
def _calculate_merkle_root(transactions: Sequence[Transaction]) -> Optional[str]:
if not transactions:
return None
Expand Down Expand Up @@ -65,9 +64,8 @@ def __init__(
self.hash: Optional[str] = None
self.state_root: Optional[str] = state_root
self.receipt_root: Optional[str] = receipt_root
self.miner: Optional[str] = miner

# NEW: compute merkle roots once
# Compute merkle roots once
self.merkle_root: Optional[str] = _calculate_merkle_root(self.transactions)

# If receipt_root is missing but we have receipts, calculate it.
Expand All @@ -88,7 +86,7 @@ def to_header_dict(self):
"difficulty": self.difficulty,
"nonce": self.nonce,
}
# Include miner in header only when present (optional field) <-- Reworded comment
# Include miner in header only when present (optional field)
if self.miner is not None:
header["miner"] = self.miner
return header
Expand Down Expand Up @@ -132,27 +130,26 @@ def from_dict(cls, payload: dict):
for r_payload in payload.get("receipts", [])
]

# Safely extract and cast difficulty if it exists
# Safely extract and cast difficulty and timestamp if they exist
raw_diff = payload.get("difficulty")
parsed_diff = int(raw_diff) if raw_diff is not None else None

# Safely extract and cast timestamp if it exists <-- Added explicit timestamp casting

raw_ts = payload.get("timestamp")
parsed_ts = int(raw_ts) if raw_ts is not None else None
block = cls(
index=int(payload["index"]),
index=int(payload["index"]),
previous_hash=payload["previous_hash"],
transactions=transactions,
timestamp=parsed_ts, # <-- Passed the casted timestamp
difficulty=parsed_diff,
timestamp=parsed_ts,
difficulty=parsed_diff,
state_root=payload.get("state_root"),
receipt_root=payload.get("receipt_root"),
receipts=receipts,
miner=payload.get("miner"),
)
block.nonce = int(payload.get("nonce", 0))
block.nonce = int(payload.get("nonce", 0))
block.hash = payload.get("hash")

# Verify the block hash
expected_hash = block.compute_hash()
if block.hash is not None and block.hash != expected_hash:
Expand Down
Loading