Feat/fork choice#95
Conversation
WalkthroughThis 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. ChangesSmart Contracts and Gas-Metered Execution
Chain Consensus and Fork Resolution
P2P Networking for Chain Synchronization
Transaction Selection and CLI Interface
Internal Refactoring and Persistence
Contract Examples and Documentation
Test Coverage
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (24)
README.mdexamples/counter.pyexamples/dex.pyexamples/stablecoin.pymain.pyminichain/block.pyminichain/chain.pyminichain/contract.pyminichain/mempool.pyminichain/mpt.pyminichain/p2p.pyminichain/persistence.pyminichain/receipt.pyminichain/state.pyminichain/transaction.pyrequirements.txtsetup.pytests/test_contract.pytests/test_core.pytests/test_persistence.pytests/test_persistence_runtime.pytests/test_protocol_hardening.pytests/test_reorg.pytests/test_transaction_signing.py
💤 Files with no reviewable changes (1)
- setup.py
| # 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 |
There was a problem hiding this comment.
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).
| parts = msg['data'].split(':') | ||
| tokens_to_sell = int(parts[1]) |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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.
| 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()) |
There was a problem hiding this comment.
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.
|
|
||
| ### Writing a Contract |
There was a problem hiding this comment.
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
|
|
||
| ### Interacting via CLI |
There was a problem hiding this comment.
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.
| ### 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
| @@ -1,2 +1,2 @@ | |||
| pynacl==1.6.2 | |||
| libp2p==0.5.0 | |||
| trie>=3.1.0 | |||
There was a problem hiding this comment.
🧹 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.
| @@ -0,0 +1,117 @@ | |||
| import pytest | |||
| import os | |||
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
🧹 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:
- Moving
mine_and_process_blockto a shared test utility module within thetests/directory - Refactoring
main.pyto 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.
Addressed Issues:
get_total_work()to calculate cumulative chain PoW.Blockchain.resolve_conflicts()to execute safe state reorganizations by snapshotting the genesis state and rebuilding against heavier chains.minichain/p2p.pyandmain.pyto support newchain_requestandchain_responsemessages for automated node sync when falling behind.test_reorg.pyensuring 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:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Release Notes
deployandcallfor contract interaction with optional amount and gas limits