Skip to content

Feat/fork choice#95

Open
SIDDHANTCOOKIE wants to merge 9 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:feat/fork-choice
Open

Feat/fork choice#95
SIDDHANTCOOKIE wants to merge 9 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:feat/fork-choice

Conversation

@SIDDHANTCOOKIE

@SIDDHANTCOOKIE SIDDHANTCOOKIE commented Jun 11, 2026

Copy link
Copy Markdown
Member

Addressed Issues:

  • Added get_total_work() to calculate cumulative chain PoW.
  • Implemented Blockchain.resolve_conflicts() to execute safe state reorganizations by snapshotting the genesis state and rebuilding against heavier chains.
  • Upgraded minichain/p2p.py and main.py to support new chain_request and chain_response messages for automated node sync when falling behind.
  • Handled mempool orphan recovery, ensuring txs dropped from an abandoned chain are restored for future mining.
  • Added comprehensive edge-case testing in test_reorg.py ensuring rollbacks handle conflicting nonces safely.

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:

  • This PR does not contain AI-generated code at all.
  • This PR contains AI-generated code. I have read the AI Usage Policy and this PR complies with this policy. I have tested the code locally and I am responsible for it.

I have used the following AI models and tools: TODO

Checklist

  • My PR addresses a single issue, fixes a single bug or makes a single improvement.
  • My code follows the project's code style and conventions
  • If applicable, I have made corresponding changes or additions to the documentation
  • If applicable, I have made corresponding changes or additions to tests
  • My changes generate no new warnings or errors
  • I have joined the Discord server and I will share a link to this PR with the project maintainers there
  • I have read the Contribution Guidelines
  • Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
  • I have filled this PR template completely and carefully, and I understand that my PR may be closed without review otherwise.

Summary by CodeRabbit

Release Notes

  • New Features
    • Added Python-based smart contract support with gas metering and sandboxed execution
    • Introduced transaction fee system with optional fee parameter
    • Added CLI commands deploy and call for contract interaction with optional amount and gas limits
    • Included example contracts: counter, constant-product DEX, and ERC-20-style stablecoin
    • Enhanced blockchain consensus with transaction receipts and fork resolution

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR adds smart contract execution with gas metering, transaction fees, blockchain fork resolution via proof-of-work, and P2P chain synchronization to MiniChain. Receipts track transaction execution results; Merkle Patricia Trie delegated to external library; three example contracts demonstrate capabilities.

Changes

Smart Contracts and Gas-Metered Execution

Layer / File(s) Summary
Receipt Data Model and Block Integration
minichain/receipt.py, minichain/block.py
Receipt class stores execution status, gas_used, error_message, and contract_address; Block extended with receipts list and receipt_root computed via Merkle tree; serialization/deserialization handles receipt payloads.
Transaction Fee Field
minichain/transaction.py
Transaction class extended with fee field (default 0); fee included in to_dict, to_signing_dict (part of signed payload), and from_dict deserialization.
State Validation and Application with Fees and Receipts
minichain/state.py
State.validate_and_apply now returns None for invalid semantic checks, otherwise applies transaction; apply_transaction returns Receipt objects (or None); validate balances against total_cost (amount + fee); snapshot/restore methods enable reorg rollback.
Gas Metering Infrastructure
minichain/contract.py (partial)
OutOfGasException and GasMeter class installed via sys.settrace to decrement gas per opcode; _safe_exec_worker computes and reports gas_used alongside execution status.
Contract Execution with Gas Measurement
minichain/contract.py (partial)
ContractMachine.execute accepts gas_limit and returns {success, gas_used, error}; handles timeout/crash/validation failures with explicit gas charging; commits storage only on success.

Chain Consensus and Fork Resolution

Layer / File(s) Summary
Chain Fork Resolution and Cumulative Work
minichain/chain.py
Blockchain.get_total_work() sums 2^(difficulty) across chain; Blockchain.resolve_conflicts() validates incoming chain genesis, rebuilds state from snapshot, validates receipt/receipt_root/state_root per block, commits reorg if heavier and valid, returns orphaned transactions.

P2P Networking for Chain Synchronization

Layer / File(s) Summary
P2P Chain Sync Protocol
minichain/p2p.py
SUPPORTED_MESSAGE_TYPES adds chain_request/chain_response; block validation requires receipt_root and receipts with per-receipt field checks; P2PNetwork broadcasts requests and sends responses; message dispatch routes to new chain validators.
Mining, Block Construction, and Network Wiring
main.py (partial), minichain/chain.py (partial)
Mining collects receipts, computes total_fees from gas_used, credits miner with base reward + fees; blocks include state_root, receipt_root, receipts; incoming block handler triggers chain sync on "ahead" blocks; resolved orphaned transactions restored to mempool.

Transaction Selection and CLI Interface

Layer / File(s) Summary
Mempool Fee-Based Ordering
minichain/mempool.py
Transaction selection prioritizes by (-fee, timestamp, sender, nonce) instead of (timestamp, sender, nonce); validation requires fee field and allows amount >= 0.
CLI Contract Commands
main.py (partial)
Balance output adds [Contract] marker; send accepts optional fee; new deploy command reads code file and sends deployment transaction; new call command sends data payload; both support optional amount/fee with non-negative validation.

Internal Refactoring and Persistence

Layer / File(s) Summary
MPT Implementation Delegation
minichain/mpt.py
Merkle Patricia Trie replaced with thin HexaryTrie wrapper; removed custom node types, nibble conversion, and hashing; get/put delegate to external library with hex↔bytes conversion.
Persistence and Dependencies
minichain/persistence.py, requirements.txt
load() calls Block.from_dict directly instead of helper; _deserialize_block removed; setup.py distribution config removed; libp2p==0.5.0 replaced with trie>=3.1.0.

Contract Examples and Documentation

Layer / File(s) Summary
Examples and Documentation
README.md, examples/counter.py, examples/stablecoin.py, examples/dex.py
README documents contract execution model and CLI usage; counter example demonstrates state machine (increment/decrement/reset); stablecoin example implements ERC-20 style mint/transfer; DEX example implements constant-product AMM (init/buy/sell).

Test Coverage

Layer / File(s) Summary
Transaction and Signature Tests
tests/test_core.py, tests/test_transaction_signing.py
test_transaction_fee validates fee deduction/crediting through apply_transaction and mining; signature tests use object.setattr to tamper with transaction fields and verify signing/verification.
Contract and Receipt Tests
tests/test_contract.py
Tests deploy/execute with fee and receipt assertions; validates insufficient balance (None), non-existent contract (status=0, error), runtime exceptions (error_message); tests out-of-gas (status=0, error="Out of gas!", gas_used=fee), AST validation, and restricted builtins.
Persistence and Reorg Tests
tests/test_persistence.py, tests/test_persistence_runtime.py, tests/test_reorg.py, tests/test_protocol_hardening.py
test_receipt_data_preserved validates receipt field preservation across save/load; test_reorg validates fork resolution with heavier chain selection, transaction orphaning, and lighter chain rejection; block schema updated to require receipt_root and receipts.

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

  • StabilityNexus/MiniChain#4: Implements get_total_work() and resolve_conflicts() for fork-choice and chain reorg behavior as described in the issue.

Possibly related PRs

  • StabilityNexus/MiniChain#87: Both modify genesis bootstrapping/initialization in minichain/chain.py; main PR adds receipt-related fields to genesis block initialization.
  • StabilityNexus/MiniChain#60: Both modify minichain/mempool.py transaction selection; main PR adds fee-based prioritization to existing sorting logic.
  • StabilityNexus/MiniChain#58: Overlaps canonical JSON/tx signing in minichain/transaction.py, P2P validation in minichain/p2p.py, and mempool selection in minichain/mempool.py.

Suggested labels

Python Lang, Documentation

Suggested reviewers

  • Zahnentferner

🐰 Hop and build contracts bright,
Gas meters dance and state's in flight,
When chains diverge, consensus mends,
Fee-sorted blocks, the work ascends!

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feat/fork choice' is directly related to the PR's main objective of implementing fork-choice consensus and state reorganization logic.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 13

🤖 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 `@examples/dex.py`:
- Around line 38-49: The buy path uses integer division when computing
new_token_reserve (storage['k'] // new_native_reserve), which truncates and
breaks the constant-product invariant; change the calculation to preserve k by
using exact/fixed-point division or a rounding strategy that always favors the
pool (e.g., compute new_token_reserve = floor(k / new_native_reserve) only where
floor reduces user payout, or use decimal/fraction arithmetic), then compute
tokens_out = storage['token_reserve'] - new_token_reserve accordingly; make the
same change in the sell path (the symmetric calculation around storage['k'] and
new_native_reserve/new_token_reserve) and add a brief comment explaining the
chosen rounding strategy to prevent draining via repeated trades (refer to
storage['k'], new_token_reserve, tokens_out and the buy/sell logic in
examples/dex.py).
- Around line 60-61: The payload parsing assumes msg['data'] splits into at
least two parts and that parts[1] converts to int; add defensive validation
around msg['data'] -> parts (check length >=2) and wrap the int(parts[1])
conversion in a try/except to catch ValueError, then produce a clear error
response/log (e.g., return or raise a descriptive error mentioning the invalid
payload) instead of letting a raw ValueError bubble up; update the code around
parts, tokens_to_sell and any caller handling to use the validated
tokens_to_sell.

In `@examples/stablecoin.py`:
- Around line 20-29: The transfer amount parsing currently does int(parts[2])
without validation which can raise ValueError; update the transfer handling (the
branch that checks msg['data'].startswith('transfer:') and uses parts,
to_address and amount) to wrap the int conversion in a try/except, catching
ValueError and raising a clear Exception/ValueError like "Invalid transfer
amount: must be an integer, got '<value>'", then retain the existing amount <= 0
check to reject non-positive amounts; ensure the error message includes the
original parts[2] for easier debugging.
- Around line 9-14: The mint payload parsing uses int(msg['data'].split(':')[1])
without protection; wrap the parsing in a try/except around the 'mint:' branch
(the code that sets amount from msg['data']), catch ValueError and IndexError,
and raise a clear Exception like "Invalid mint payload, expected
'mint:<positive-integer>'" so callers get a helpful error; then proceed to the
existing amount <= 0 check as before.

In `@main.py`:
- Line 164: The fire-and-forget Task created at the call to
asyncio.create_task(network.broadcast_chain_request()) can be garbage-collected;
create a module- or handler-scoped container (e.g. _background_tasks = set())
and add the returned Task to it, e.g. task =
asyncio.create_task(network.broadcast_chain_request());
_background_tasks.add(task); task.add_done_callback(_background_tasks.discard);
do the same for the other create_task usage around the similar call (the one
noted at line 172) or store the Task reference directly if only a single
outstanding task is needed.
- Around line 168-172: The chain_request branch currently builds blocks_dicts
and calls network._broadcast_raw which sends the chain_response to all peers;
change this to reply only to the requester by accepting the requester's writer
(pass the peer's writer into the handler that checks msg_type ==
"chain_request") and invoke the targeted send function (e.g., call
send_chain_response or a network method that sends to a single writer) with the
payload instead of using network._broadcast_raw; update the handler signature
and call sites to pass the writer and replace the broadcast call with the
single-peer send using the same blocks_dicts payload.

In `@minichain/chain.py`:
- Around line 188-190: Remove the unused snapshot variable created in the reorg
flow: delete the assignment to state_snapshot in the method where the code takes
a snapshot (the lines with state_snapshot = self.state.snapshot() and
original_chain = list(self.chain)); leave original_chain only if it is used
later, otherwise remove it too. Update references in the surrounding method
(e.g., the reorg/validation routine in class Chain or method handling reorgs) so
there are no unused local variables remaining.

In `@minichain/transaction.py`:
- Around line 48-52: The from_dict method in class Transaction will raise
KeyError for legacy payloads missing the fee key; update Transaction.from_dict
to read fee via payload.get("fee", 0) instead of payload["fee"] so that missing
fee defaults to 0 (preserve other fields as-is) to maintain backward
compatibility with persisted/in-flight legacy transactions.

In `@README.md`:
- Around line 110-111: Add a blank line before the "### Writing a Contract"
heading to follow Markdown best practices and improve readability; update the
README.md by inserting an empty line immediately above the "### Writing a
Contract" heading so the heading is separated from the preceding paragraph or
content.
- Around line 118-119: Add a single blank line before the "### Interacting via
CLI" heading in README.md to follow Markdown best practices and improve
readability; locate the heading string "### Interacting via CLI" and insert one
empty line immediately above it.

In `@requirements.txt`:
- Line 2: The requirement entry "trie>=3.1.0" should be made deterministic to
avoid breakages; update the requirements.txt entry for the package "trie" (the
line containing trie>=3.1.0) to either pin to a specific version (e.g.,
trie==3.1.0) or add an upper bound to cap major upgrades (e.g., trie>=3.1.0,<4)
depending on your compatibility policy, then run your dependency install/test
matrix to verify no regressions.

In `@tests/test_reorg.py`:
- Around line 10-13: The test is mutating sys.path to import
mine_and_process_block from main.py which is fragile; instead move the shared
test helper into a proper test utilities module (e.g., create tests/utils.py
exporting mine_and_process_block or a wrapper) or refactor the implementation
out of main.py into a dedicated module (e.g., core.mining or lib/mining) and
update tests/test_reorg.py to import mine_and_process_block from that module so
you can remove the sys.path.insert manipulation; update any imports in test
files to point to the new utilities module and ensure mine_and_process_block
remains exported for tests.
- Line 2: Remove the duplicate import of the os module in tests/test_reorg.py by
keeping a single top-level "import os" and deleting the redundant second "import
os" statement (the duplicate import occurrences in the file); ensure any other
imports or code relying on os remain unchanged and run tests to confirm no
breakage.
🪄 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: 6bcefca3-98d0-4848-8061-643f793ce66b

📥 Commits

Reviewing files that changed from the base of the PR and between 329e7cb and ce2cd52.

📒 Files selected for processing (24)
  • README.md
  • examples/counter.py
  • examples/dex.py
  • examples/stablecoin.py
  • main.py
  • minichain/block.py
  • minichain/chain.py
  • minichain/contract.py
  • minichain/mempool.py
  • minichain/mpt.py
  • minichain/p2p.py
  • minichain/persistence.py
  • minichain/receipt.py
  • minichain/state.py
  • minichain/transaction.py
  • requirements.txt
  • setup.py
  • tests/test_contract.py
  • tests/test_core.py
  • tests/test_persistence.py
  • tests/test_persistence_runtime.py
  • tests/test_protocol_hardening.py
  • tests/test_reorg.py
  • tests/test_transaction_signing.py
💤 Files with no reviewable changes (1)
  • setup.py

Comment thread examples/dex.py
Comment on lines +38 to +49
# Calculate how many tokens to give using x * y = k
# (native_reserve + msg['value']) * (token_reserve - tokens_out) = k
new_native_reserve = storage['native_reserve'] + msg['value']
new_token_reserve = storage['k'] // new_native_reserve

tokens_out = storage['token_reserve'] - new_token_reserve
if tokens_out <= 0:
raise Exception("Not enough tokens to dispense")

# Update reserves
storage['native_reserve'] = new_native_reserve
storage['token_reserve'] = new_token_reserve

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.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Integer division breaks constant product invariant.

The buy logic uses integer division at line 41 (storage['k'] // new_native_reserve), which truncates and violates the x * y = k invariant. Repeated trades can drain value through rounding manipulation. The same issue exists in the sell path at line 74.

For an educational example, consider documenting this limitation in comments, or use a rounding strategy that preserves k (e.g., always round reserves in favor of the pool). Production AMMs typically use fixed-point arithmetic or maintain k strictly.

🧰 Tools
🪛 Ruff (0.15.15)

[error] 40-40: Undefined name storage

(F821)


[error] 40-40: Undefined name msg

(F821)


[error] 41-41: Undefined name storage

(F821)


[error] 43-43: Undefined name storage

(F821)


[warning] 45-45: Create your own exception

(TRY002)


[warning] 45-45: Avoid specifying long messages outside the exception class

(TRY003)


[error] 48-48: Undefined name storage

(F821)


[error] 49-49: Undefined name storage

(F821)

🤖 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 `@examples/dex.py` around lines 38 - 49, The buy path uses integer division
when computing new_token_reserve (storage['k'] // new_native_reserve), which
truncates and breaks the constant-product invariant; change the calculation to
preserve k by using exact/fixed-point division or a rounding strategy that
always favors the pool (e.g., compute new_token_reserve = floor(k /
new_native_reserve) only where floor reduces user payout, or use
decimal/fraction arithmetic), then compute tokens_out = storage['token_reserve']
- new_token_reserve accordingly; make the same change in the sell path (the
symmetric calculation around storage['k'] and
new_native_reserve/new_token_reserve) and add a brief comment explaining the
chosen rounding strategy to prevent draining via repeated trades (refer to
storage['k'], new_token_reserve, tokens_out and the buy/sell logic in
examples/dex.py).

Comment thread examples/dex.py
Comment on lines +60 to +61
parts = msg['data'].split(':')
tokens_to_sell = int(parts[1])

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add error handling for payload parsing.

Line 61 calls int(parts[1]) without a try/except, which raises ValueError if the payload format is invalid (e.g., sell:abc). While the contract will fail safely (charging gas), a clearer error message improves user experience.

🛡️ Proposed fix
     parts = msg['data'].split(':')
-    tokens_to_sell = int(parts[1])
+    try:
+        tokens_to_sell = int(parts[1])
+    except (ValueError, IndexError):
+        raise Exception("Invalid sell format. Use: sell:<amount>")
🧰 Tools
🪛 Ruff (0.15.15)

[error] 60-60: Undefined name msg

(F821)

🤖 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 `@examples/dex.py` around lines 60 - 61, The payload parsing assumes
msg['data'] splits into at least two parts and that parts[1] converts to int;
add defensive validation around msg['data'] -> parts (check length >=2) and wrap
the int(parts[1]) conversion in a try/except to catch ValueError, then produce a
clear error response/log (e.g., return or raise a descriptive error mentioning
the invalid payload) instead of letting a raw ValueError bubble up; update the
code around parts, tokens_to_sell and any caller handling to use the validated
tokens_to_sell.

Comment thread examples/stablecoin.py
Comment on lines +9 to +14
if msg['data'].startswith('mint:'):
# In a real contract, you would restrict this to an owner address!
# For this example, anyone can mint tokens to themselves.
amount = int(msg['data'].split(':')[1])
if amount <= 0:
raise Exception("Amount must be positive")

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add error handling for payload parsing.

Line 12 calls int(msg['data'].split(':')[1]) without a try/except, which can raise ValueError (invalid integer) or IndexError (missing colon). While the contract will fail safely (charging gas), a clearer error message improves user experience.

🛡️ Proposed fix
 if msg['data'].startswith('mint:'):
     # In a real contract, you would restrict this to an owner address!
     # For this example, anyone can mint tokens to themselves.
-    amount = int(msg['data'].split(':')[1])
+    try:
+        amount = int(msg['data'].split(':')[1])
+    except (ValueError, IndexError):
+        raise Exception("Invalid mint format. Use: mint:<amount>")
     if amount <= 0:
🧰 Tools
🪛 Ruff (0.15.15)

[error] 9-9: Undefined name msg

(F821)


[error] 12-12: Undefined name msg

(F821)


[warning] 14-14: Create your own exception

(TRY002)


[warning] 14-14: Avoid specifying long messages outside the exception class

(TRY003)

🤖 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 `@examples/stablecoin.py` around lines 9 - 14, The mint payload parsing uses
int(msg['data'].split(':')[1]) without protection; wrap the parsing in a
try/except around the 'mint:' branch (the code that sets amount from
msg['data']), catch ValueError and IndexError, and raise a clear Exception like
"Invalid mint payload, expected 'mint:<positive-integer>'" so callers get a
helpful error; then proceed to the existing amount <= 0 check as before.

Comment thread examples/stablecoin.py
Comment on lines +20 to +29
elif msg['data'].startswith('transfer:'):
parts = msg['data'].split(':')
if len(parts) != 3:
raise Exception("Invalid transfer format")

to_address = parts[1]
amount = int(parts[2])

if amount <= 0:
raise Exception("Amount must be positive")

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add error handling for amount parsing.

Line 26 calls int(parts[2]) without a try/except, which can raise ValueError if the amount is not a valid integer (e.g., transfer:addr:abc). While the contract will fail safely (charging gas), a clearer error message improves user experience.

🛡️ Proposed fix
     to_address = parts[1]
-    amount = int(parts[2])
+    try:
+        amount = int(parts[2])
+    except ValueError:
+        raise Exception("Invalid amount format. Must be an integer.")
     
     if amount <= 0:
🧰 Tools
🪛 Ruff (0.15.15)

[error] 20-20: Undefined name msg

(F821)


[error] 21-21: Undefined name msg

(F821)


[warning] 23-23: Create your own exception

(TRY002)


[warning] 23-23: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 29-29: Create your own exception

(TRY002)


[warning] 29-29: Avoid specifying long messages outside the exception class

(TRY003)

🤖 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 `@examples/stablecoin.py` around lines 20 - 29, The transfer amount parsing
currently does int(parts[2]) without validation which can raise ValueError;
update the transfer handling (the branch that checks
msg['data'].startswith('transfer:') and uses parts, to_address and amount) to
wrap the int conversion in a try/except, catching ValueError and raising a clear
Exception/ValueError like "Invalid transfer amount: must be an integer, got
'<value>'", then retain the existing amount <= 0 check to reject non-positive
amounts; ensure the error message includes the original parts[2] for easier
debugging.

Comment thread main.py
logger.warning("📥 Received Block #%s — rejected", block.index)
if block.index > chain.last_block.index:
logger.warning("📥 Received Block #%s — ahead of us (tip: %s). Requesting chain sync...", block.index, chain.last_block.index)
asyncio.create_task(network.broadcast_chain_request())

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Store references to fire-and-forget tasks to prevent garbage collection.

asyncio.create_task() returns a Task that may be garbage-collected before completion if no reference is kept. This could cause the chain sync request or response broadcast to silently fail.

🛠️ Proposed fix

Add a set to track background tasks at the module or handler scope:

_background_tasks = set()

# Then in handler:
task = asyncio.create_task(network.broadcast_chain_request())
_background_tasks.add(task)
task.add_done_callback(_background_tasks.discard)

Or store the task reference directly if a single task is sufficient.

Also applies to: 172-172

🧰 Tools
🪛 Ruff (0.15.15)

[warning] 164-164: 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` at line 164, The fire-and-forget Task created at the call to
asyncio.create_task(network.broadcast_chain_request()) can be garbage-collected;
create a module- or handler-scoped container (e.g. _background_tasks = set())
and add the returned Task to it, e.g. task =
asyncio.create_task(network.broadcast_chain_request());
_background_tasks.add(task); task.add_done_callback(_background_tasks.discard);
do the same for the other create_task usage around the similar call (the one
noted at line 172) or store the Task reference directly if only a single
outstanding task is needed.

Comment thread README.md
Comment on lines +110 to +111

### Writing a Contract

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add blank line before heading for better readability.

Markdown best practice requires blank lines surrounding headings.

📝 Proposed fix
 The execution engine uses `sys.settrace` for precise **Gas Metering** (charging 1 gas per executed opcode) and `multiprocessing` for **Sandboxed Execution** to ensure network security.
+
 ### Writing a Contract
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 111-111: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 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 `@README.md` around lines 110 - 111, Add a blank line before the "### Writing a
Contract" heading to follow Markdown best practices and improve readability;
update the README.md by inserting an empty line immediately above the "###
Writing a Contract" heading so the heading is separated from the preceding
paragraph or content.

Source: Linters/SAST tools

Comment thread README.md
Comment on lines +118 to +119

### Interacting via CLI

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add blank line before heading for better readability.

Markdown best practice requires blank lines surrounding headings.

📝 Proposed fix
 - `examples/dex.py` - An Automated Market Maker (AMM) using the constant product formula (x * y = k).
+
 ### Interacting via CLI
📝 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
### Interacting via CLI
- `examples/dex.py` - An Automated Market Maker (AMM) using the constant product formula (x * y = k).
### Interacting via CLI
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 119-119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 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 `@README.md` around lines 118 - 119, Add a single blank line before the "###
Interacting via CLI" heading in README.md to follow Markdown best practices and
improve readability; locate the heading string "### Interacting via CLI" and
insert one empty line immediately above it.

Source: Linters/SAST tools

Comment thread requirements.txt
@@ -1,2 +1,2 @@
pynacl==1.6.2
libp2p==0.5.0
trie>=3.1.0

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.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider pinning or capping the trie version.

Using >=3.1.0 without an upper bound risks silent breakage from future major releases. Consider pinning (e.g., trie==3.1.0) for reproducible builds, or capping (e.g., trie>=3.1.0,<4).

🤖 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 `@requirements.txt` at line 2, The requirement entry "trie>=3.1.0" should be
made deterministic to avoid breakages; update the requirements.txt entry for the
package "trie" (the line containing trie>=3.1.0) to either pin to a specific
version (e.g., trie==3.1.0) or add an upper bound to cap major upgrades (e.g.,
trie>=3.1.0,<4) depending on your compatibility policy, then run your dependency
install/test matrix to verify no regressions.

Comment thread tests/test_reorg.py
@@ -0,0 +1,117 @@
import pytest
import os

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove duplicate import os.

Line 2 and line 11 both import os. The second import on line 11 is redundant.

🔧 Proposed fix
 
 import sys
-import os
 sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))

Also applies to: 10-12

🤖 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_reorg.py` at line 2, Remove the duplicate import of the os module
in tests/test_reorg.py by keeping a single top-level "import os" and deleting
the redundant second "import os" statement (the duplicate import occurrences in
the file); ensure any other imports or code relying on os remain unchanged and
run tests to confirm no breakage.

Comment thread tests/test_reorg.py
Comment on lines +10 to +13
import sys
import os
sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
from main import mine_and_process_block

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.

🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff

Consider moving test helper import to avoid sys.path manipulation.

Using sys.path.insert is generally discouraged as it can cause import conflicts and makes tests fragile. Consider either:

  1. Moving mine_and_process_block to a shared test utility module within the tests/ directory
  2. Refactoring main.py to export testable functions from a dedicated module
🤖 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_reorg.py` around lines 10 - 13, The test is mutating sys.path to
import mine_and_process_block from main.py which is fragile; instead move the
shared test helper into a proper test utilities module (e.g., create
tests/utils.py exporting mine_and_process_block or a wrapper) or refactor the
implementation out of main.py into a dedicated module (e.g., core.mining or
lib/mining) and update tests/test_reorg.py to import mine_and_process_block from
that module so you can remove the sys.path.insert manipulation; update any
imports in test files to point to the new utilities module and ensure
mine_and_process_block remains exported for tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant