Feat/libp2p#102
Open
SIDDHANTCOOKIE wants to merge 8 commits into
Open
Conversation
Contributor
WalkthroughThis PR replaces the asyncio TCP P2P transport with libp2p/trio, adds a JSON-RPC HTTP server, rewrites chain conflict resolution to use total-work comparison with full state replay, adds contract-to-address outbound transfer support with atomic rollback, and refactors the mempool from a nested dict to a single sorted list. ChangesMiniChain Protocol Upgrade
Sequence Diagram(s)sequenceDiagram
participant Client
participant JSONRPCServer
participant Blockchain
participant Mempool
participant P2PNetwork
rect rgba(70, 130, 180, 0.5)
Note over Client,JSONRPCServer: JSON-RPC HTTP POST
Client->>JSONRPCServer: POST {method: mc_sendTransaction, params: [tx_dict], id}
JSONRPCServer->>JSONRPCServer: Transaction.from_dict / verify()
JSONRPCServer->>Mempool: add_transaction(tx)
Mempool-->>JSONRPCServer: accepted
JSONRPCServer->>P2PNetwork: broadcast_transaction(tx) [async]
JSONRPCServer-->>Client: {result: tx_id, id}
end
rect rgba(60, 179, 113, 0.5)
Note over P2PNetwork,Blockchain: Chain sync via chain_request/response
P2PNetwork->>P2PNetwork: receives chain_request
P2PNetwork->>P2PNetwork: send_chain_response(blocks)
P2PNetwork->>Blockchain: resolve_conflicts(new_chain_list)
Blockchain->>Blockchain: get_total_work comparison
Blockchain->>Blockchain: replay blocks on temp_state
Blockchain-->>P2PNetwork: (True, orphans)
P2PNetwork->>Mempool: add_transaction(orphan) per orphan
end
rect rgba(176, 196, 222, 0.5)
Note over Mempool,Blockchain: Contract transfer with rollback
Mempool->>Blockchain: include tx in block
Blockchain->>Blockchain: apply_transaction(tx)
Blockchain->>Blockchain: contract execution → transfers
Blockchain->>Blockchain: validate transfer total vs balance
alt insufficient balance
Blockchain->>Blockchain: restore(snapshot) / rollback
else valid
Blockchain->>Blockchain: debit receiver, credit recipients
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)main.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.25][ERROR]: unable to find a config; path minichain/contract.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.27][ERROR]: unable to find a config; path minichain/p2p.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.42][ERROR]: unable to find a config; path
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/mempool.py (1)
13-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject invalid amount/fee values before admitting transactions.
add_transaction()is now a shared ingress for P2P/RPC, but it only checks the signature. A signed transaction with a negative or non-integeramount/feecan enter the sorted list and poison block assembly; validate these cheap invariants before insertion.🛡️ Proposed validation
def add_transaction(self, tx): + amount = getattr(tx, "amount", None) + fee = getattr(tx, "fee", 0) + if not isinstance(amount, int) or amount < 0: + logger.warning("Mempool: Invalid amount rejected") + return False + if not isinstance(fee, int) or fee < 0: + logger.warning("Mempool: Invalid fee rejected") + return False + if not tx.verify(): logger.warning("Mempool: Invalid signature rejected") return False🤖 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/mempool.py` around lines 13 - 16, The add_transaction() method currently only validates the transaction signature but does not check if the amount and fee values are valid. Add validation before the transaction is admitted to check that both amount and fee are non-negative integers. If either value fails validation, log an appropriate warning and return False to reject the transaction, similar to the existing signature verification check.
🤖 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 441-443: The RPC port calculation using `rpc_port = 8545 + (port -
9000)` can produce invalid port numbers outside the valid range of 1-65535. Add
validation logic after the rpc_port assignment and before the
asyncio.create_task call to verify that rpc_port is within the valid range (1 to
65535). If the calculated rpc_port is invalid, raise an appropriate exception or
log an error and exit to prevent the rpc_server.start call from failing at
runtime.
In `@minichain/chain.py`:
- Around line 233-234: The assignment `self.chain = new_chain_list` at the chain
assignment line creates an alias to the caller's list instead of making an
independent copy. This causes mutations to the caller's list to affect the
object's chain without proper state replay. Replace the direct assignment with a
copy operation to ensure the chain list is independent from the caller's list.
- Around line 219-226: The reorg replay validation in resolve_conflicts() only
verifies that the computed receipt root matches the block's receipt root, but
fails to validate the actual receipt payload contents like add_block() does.
This allows blocks with mismatched receipts to be accepted through reorg when
normal block import would reject them. Add a validation check after computing
receipts to ensure block.receipts matches the computed receipts list directly,
not just their root hash. This additional payload validation should occur
alongside the existing receipt root and state root checks to maintain parity
with add_block() validation logic.
- Around line 104-112: The current implementation uses `block.difficulty or 1`
in the return statement of the get_total_work method, which treats both None and
0 as falsy values and converts them to 1. This causes difficulty-0 blocks to be
counted as 2^1 instead of 2^0, making them indistinguishable from difficulty-1
blocks. Replace the `or 1` fallback with an explicit None check using a
conditional expression (like `block.difficulty if block.difficulty is not None
else 1`) so that an explicit difficulty value of 0 is preserved and calculates
correctly as 2^0 = 1, while only defaulting to 1 when difficulty is actually
None.
- Around line 228-231: The orphans list being returned contains transactions
with stale nonces that are unmineable and should be filtered out. Before
returning the orphans list after computing it from old_txs and new_tx_ids,
filter out any orphaned transactions where the sender has already advanced to a
higher nonce in the new chain. Build a mapping of senders to their maximum nonce
in new_chain_list by iterating through all transactions in the new chain blocks,
then exclude any orphaned transactions where the sender's current nonce in the
new chain is already at or beyond the orphaned transaction's nonce value.
In `@minichain/contract.py`:
- Around line 50-55: In the transfer_out function, the address validation
currently only checks if the address is a string type, which allows malformed
addresses to be persisted. Add format validation to ensure the address follows a
canonical hex format using regex pattern matching. Import the re module at the
top of the file, then add a validation check after the isinstance type check
that uses a regex pattern to validate the address conforms to the expected
format before allowing it to be appended to the transfers list.
In `@minichain/p2p.py`:
- Around line 58-61: In the _message_id method, when msg_type is "block", the
code directly accesses payload["hash"] which raises KeyError if the key is
missing in malformed payloads. Replace the direct dictionary access
payload["hash"] with payload.get("hash") to safely extract the hash value,
returning None if the key does not exist. This prevents crashes during duplicate
message checks and allows the code to handle hostile or malformed block payloads
gracefully.
- Around line 46-47: The asyncio reader task created by
asyncio.create_task(self._asyncio_reader()) on line 47 is not stored, making it
impossible to stop during shutdown. Store the task reference in an instance
variable when creating it, then explicitly cancel or await stopping that task in
the shutdown logic around lines 52-53 along with the trio side shutdown. Apply
the same pattern to the other instances mentioned at lines 50-53 and 95-98 to
ensure all asyncio tasks are properly tracked and cancelled during graceful
shutdown.
- Around line 106-117: The PEER_CONNECTED handler is incorrectly broadcasting
callback-generated data to all peers via _broadcast_raw when it should only be
sent to the newly connected peer. Remove the _broadcast_raw call that forwards
the JSON data from the MockWriter to all peers, and instead send the response
data directly to the peer that initiated the connection. The callback's output
is intended for that specific peer only, not for network-wide distribution.
- Around line 133-142: The stream reading loop splits each 4096-byte read()
chunk by newline without handling fragmented JSON lines that span multiple
reads. This causes incomplete lines to fail parsing and be silently dropped. Fix
this by maintaining a persistent buffer across loop iterations: append newly
read data to the buffer, split by newline, parse only complete lines (those that
ended with a newline), and keep any incomplete line (without trailing newline)
in the buffer for the next read() call to complete it.
In `@minichain/rpc.py`:
- Line 84: The response handler at line 84 always returns a JSON-RPC response
regardless of whether the request is a notification or not. Check if the request
contains an `id` field before returning the response. If `req_id` is None or
missing (indicating a notification), the function should not return a response
payload. Only return the dictionary with jsonrpc, result, and id when the
request is not a notification.
- Around line 17-22: The start() method creates AppRunner and TCPSite instances
but stores them only as local variables, preventing cleanup during shutdown.
Store the runner and site instances as instance variables (self.runner and
self.site) instead of local variables, and implement an explicit stop() method
that properly shuts down these resources by calling their cleanup methods to
ensure reliable resource cleanup.
- Around line 30-34: When processing batch requests in the section where
isinstance(req_data, list) is checked, add a validation to detect empty batches
before the loop that processes individual requests. If the list is empty, return
a JSON-RPC 2.0 Invalid Request error response with error code -32600 instead of
returning an empty array, as per the JSON-RPC 2.0 specification for empty batch
requests.
- Around line 40-41: The validation check on line 40 allows req to be non-dict,
but line 41 calls req.get("id") which will raise AttributeError if req is not a
dict. Extract the id value safely by only calling req.get("id") when req is
known to be a dict, otherwise use a default value like None for the id field in
the error response.
In `@requirements.txt`:
- Around line 3-4: The aiohttp package is imported in minichain/rpc.py and
tests/test_rpc.py but is missing from requirements.txt, which will cause
ImportError on clean installations. Add aiohttp as a new dependency line in
requirements.txt alongside the existing entries like libp2p and multiaddr to
ensure the package is installed when the project dependencies are resolved.
In `@tests/test_contract_transfers.py`:
- Around line 22-102: Add a new test method after
test_transfer_with_incoming_funds to cover the malformed address case for
transfer_out. Create a test that deploys a contract with code calling
transfer_out using an invalid hex address (like "not_hex"), then assert that the
transaction fails with status 0, the error message indicates an invalid address,
and verify that the contract balance, target balance, and contract storage all
remain unchanged from their initial states to confirm proper rollback behavior.
In `@tests/test_protocol_hardening.py`:
- Around line 99-100: The test for Transaction.from_dict(invalid_payload) is
catching a generic Exception which is too broad and could mask unrelated
regressions. Replace the Exception with the specific exception type that
Transaction.from_dict actually raises when given malformed input (likely
KeyError or another specific exception), so the test only passes when the
expected failure mode occurs and fails if any other unexpected exceptions occur.
---
Outside diff comments:
In `@minichain/mempool.py`:
- Around line 13-16: The add_transaction() method currently only validates the
transaction signature but does not check if the amount and fee values are valid.
Add validation before the transaction is admitted to check that both amount and
fee are non-negative integers. If either value fails validation, log an
appropriate warning and return False to reject the transaction, similar to the
existing signature verification check.
🪄 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: da4e76ee-cda9-4e42-9fce-eac46594aa76
📒 Files selected for processing (12)
main.pyminichain/chain.pyminichain/contract.pyminichain/mempool.pyminichain/p2p.pyminichain/rpc.pyminichain/state.pyrequirements.txttests/test_contract_transfers.pytests/test_protocol_hardening.pytests/test_reorg.pytests/test_rpc.py
Comment on lines
+441
to
+443
| # Start RPC server on a port correlated to the node port (e.g. 8545 if P2P is 9000) | ||
| rpc_port = 8545 + (port - 9000) | ||
| rpc_task = asyncio.create_task(rpc_server.start(host="127.0.0.1", port=rpc_port)) |
Contributor
There was a problem hiding this comment.
Validate the derived RPC port before startup.
rpc_port = 8545 + (port - 9000) can fall outside 1..65535 for valid CLI --port inputs, causing runtime startup failures.
🤖 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 441 - 443, The RPC port calculation using `rpc_port =
8545 + (port - 9000)` can produce invalid port numbers outside the valid range
of 1-65535. Add validation logic after the rpc_port assignment and before the
asyncio.create_task call to verify that rpc_port is within the valid range (1 to
65535). If the calculated rpc_port is invalid, raise an appropriate exception or
log an error and exit to prevent the rpc_server.start call from failing at
runtime.
Comment on lines
+104
to
+112
| def get_total_work(self, chain_list=None): | ||
| """ | ||
| Calculates the cumulative PoW of a chain. | ||
| Work is proportional to 2^difficulty. | ||
| """ | ||
| if chain_list is None: | ||
| with self._lock: | ||
| chain_list = self.chain | ||
| return sum(2 ** (block.difficulty or 1) for block in chain_list) |
Contributor
There was a problem hiding this comment.
Preserve explicit difficulty 0 in total-work calculation.
Line 112 treats 0 as 1, so difficulty-0 blocks are counted as 2 ** 1 instead of 2 ** 0. That makes difficulty 0 and 1 indistinguishable for fork choice.
🧮 Proposed fix
- return sum(2 ** (block.difficulty or 1) for block in chain_list)
+ return sum(
+ 2 ** (block.difficulty if block.difficulty is not None else 1)
+ for block in chain_list
+ )🤖 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 104 - 112, The current implementation uses
`block.difficulty or 1` in the return statement of the get_total_work method,
which treats both None and 0 as falsy values and converts them to 1. This causes
difficulty-0 blocks to be counted as 2^1 instead of 2^0, making them
indistinguishable from difficulty-1 blocks. Replace the `or 1` fallback with an
explicit None check using a conditional expression (like `block.difficulty if
block.difficulty is not None else 1`) so that an explicit difficulty value of 0
is preserved and calculates correctly as 2^0 = 1, while only defaulting to 1
when difficulty is actually None.
Comment on lines
+219
to
+226
| computed_receipt_root = calculate_receipt_root(receipts) | ||
| if block.receipt_root != computed_receipt_root: | ||
| logger.warning("Reorg failed: Invalid receipt root at block %s. Expected %s, got %s", block.index, computed_receipt_root, block.receipt_root) | ||
| return False, [] | ||
|
|
||
| if block.state_root != temp_state.state_root(): | ||
| logger.warning("Reorg failed: Invalid state root at block %s", block.index) | ||
| return False, [] |
Contributor
There was a problem hiding this comment.
Validate replayed receipt payloads, not only the receipt root.
add_block() rejects blocks whose block.receipts payload differs from computed receipts, but reorg replay omits that check. A chain accepted through resolve_conflicts() can therefore store receipts that normal block import would reject.
✅ Proposed parity check
computed_receipt_root = calculate_receipt_root(receipts)
if block.receipt_root != computed_receipt_root:
logger.warning("Reorg failed: Invalid receipt root at block %s. Expected %s, got %s", block.index, computed_receipt_root, block.receipt_root)
return False, []
+ if [r.to_dict() for r in block.receipts] != [r.to_dict() for r in receipts]:
+ logger.warning("Reorg failed: Receipts payload mismatch at block %s", block.index)
+ return False, []
+
if block.state_root != temp_state.state_root():
logger.warning("Reorg failed: Invalid state root at block %s", block.index)
return False, []🤖 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 219 - 226, The reorg replay validation in
resolve_conflicts() only verifies that the computed receipt root matches the
block's receipt root, but fails to validate the actual receipt payload contents
like add_block() does. This allows blocks with mismatched receipts to be
accepted through reorg when normal block import would reject them. Add a
validation check after computing receipts to ensure block.receipts matches the
computed receipts list directly, not just their root hash. This additional
payload validation should occur alongside the existing receipt root and state
root checks to maintain parity with add_block() validation logic.
Comment on lines
+228
to
+231
| # 4. Success! Compute orphaned transactions. | ||
| old_txs = {tx.tx_id: tx for b in original_chain[1:] for tx in b.transactions} | ||
| new_tx_ids = {tx.tx_id for b in new_chain_list[1:] for tx in b.transactions} | ||
| orphans = [tx for tx_id, tx in old_txs.items() if tx_id not in new_tx_ids] |
Contributor
There was a problem hiding this comment.
Filter stale nonce-conflicting orphans before returning them.
This returns every old-chain transaction missing from the new chain. In the reorg test shape, tx1 has nonce 0 while the accepted chain has already advanced that sender to nonce 2; main.py then re-adds it to the mempool, where only the signature is checked. That can leave an unmineable stale transaction poisoning block assembly.
🧹 Suggested direction
old_txs = {tx.tx_id: tx for b in original_chain[1:] for tx in b.transactions}
new_tx_ids = {tx.tx_id for b in new_chain_list[1:] for tx in b.transactions}
- orphans = [tx for tx_id, tx in old_txs.items() if tx_id not in new_tx_ids]
+ candidate_orphans = [tx for tx_id, tx in old_txs.items() if tx_id not in new_tx_ids]
+ orphans = []
+ for tx in candidate_orphans:
+ sender_nonce = temp_state.accounts.get(tx.sender, {}).get("nonce", 0)
+ if tx.nonce < sender_nonce:
+ logger.debug("Skipping stale orphan tx %s with nonce %s < %s", tx.tx_id, tx.nonce, sender_nonce)
+ continue
+ orphans.append(tx)🤖 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 228 - 231, The orphans list being returned
contains transactions with stale nonces that are unmineable and should be
filtered out. Before returning the orphans list after computing it from old_txs
and new_tx_ids, filter out any orphaned transactions where the sender has
already advanced to a higher nonce in the new chain. Build a mapping of senders
to their maximum nonce in new_chain_list by iterating through all transactions
in the new chain blocks, then exclude any orphaned transactions where the
sender's current nonce in the new chain is already at or beyond the orphaned
transaction's nonce value.
Comment on lines
+233
to
+234
| self.chain = new_chain_list | ||
| self.state = temp_state |
Contributor
There was a problem hiding this comment.
Do not retain the caller-owned chain list.
self.chain = new_chain_list aliases the caller’s list. In tests this means node_a.resolve_conflicts(node_b.chain) makes both nodes share the same chain object; later appends to node_b.chain can mutate node_a.chain without replaying state.
🔒 Proposed fix
- self.chain = new_chain_list
+ self.chain = list(new_chain_list)
self.state = temp_state📝 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
| self.chain = new_chain_list | |
| self.state = temp_state | |
| self.chain = list(new_chain_list) | |
| self.state = temp_state |
🤖 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 233 - 234, The assignment `self.chain =
new_chain_list` at the chain assignment line creates an alias to the caller's
list instead of making an independent copy. This causes mutations to the
caller's list to affect the object's chain without proper state replay. Replace
the direct assignment with a copy operation to ensure the chain list is
independent from the caller's list.
| else: | ||
| return {"jsonrpc": "2.0", "error": {"code": -32601, "message": f"Method not found: {method}"}, "id": req_id} | ||
|
|
||
| return {"jsonrpc": "2.0", "result": result, "id": req_id} |
Contributor
There was a problem hiding this comment.
Don’t return responses for JSON-RPC notifications.
Requests without id are notifications and should not emit a result/error response payload.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 84-84: Consider moving this statement to an else block
(TRY300)
🤖 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/rpc.py` at line 84, The response handler at line 84 always returns
a JSON-RPC response regardless of whether the request is a notification or not.
Check if the request contains an `id` field before returning the response. If
`req_id` is None or missing (indicating a notification), the function should not
return a response payload. Only return the dictionary with jsonrpc, result, and
id when the request is not a notification.
Comment on lines
+22
to
+102
| def test_successful_transfer_out(self): | ||
| # 1. Deploy Contract | ||
| code = """ | ||
| target = msg['data']['target'] | ||
| transfer_out(target, 50) | ||
| transfer_out(target, 25) | ||
| """ | ||
| deploy_tx = self._sign(Transaction(self.sender_pk, None, amount=100, nonce=0, data=code, fee=1000)) | ||
| receipt = self.state.apply_transaction(deploy_tx) | ||
| self.assertEqual(receipt.status, 1) | ||
| contract_addr = receipt.contract_address | ||
|
|
||
| # Sender sent 100 to contract, plus 10 fee | ||
| self.assertEqual(self.state.get_account(contract_addr)['balance'], 100) | ||
| self.assertEqual(self.state.get_account(self.target_pk)['balance'], 0) | ||
|
|
||
| # 2. Call Contract to transfer out 75 coins | ||
| call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=0, nonce=1, data={"target": self.target_pk}, fee=1000)) | ||
| receipt2 = self.state.apply_transaction(call_tx) | ||
|
|
||
| self.assertEqual(receipt2.status, 1) | ||
|
|
||
| # Contract balance should be 100 - 75 = 25 | ||
| self.assertEqual(self.state.get_account(contract_addr)['balance'], 25) | ||
|
|
||
| # Target should have 75 | ||
| self.assertEqual(self.state.get_account(self.target_pk)['balance'], 75) | ||
|
|
||
| def test_failed_transfer_out_insufficient_balance(self): | ||
| # 1. Deploy Contract | ||
| code = """ | ||
| target = msg['data']['target'] | ||
| # Try to transfer 500, but contract only has 100 | ||
| transfer_out(target, 500) | ||
| storage['malicious_state'] = 'corrupted' | ||
| """ | ||
| deploy_tx = self._sign(Transaction(self.sender_pk, None, amount=100, nonce=0, data=code, fee=1000)) | ||
| receipt = self.state.apply_transaction(deploy_tx) | ||
| self.assertEqual(receipt.status, 1) | ||
| contract_addr = receipt.contract_address | ||
|
|
||
| # 2. Call Contract | ||
| call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=0, nonce=1, data={"target": self.target_pk}, fee=1000)) | ||
| receipt2 = self.state.apply_transaction(call_tx) | ||
|
|
||
| # Should fail with status 0 | ||
| self.assertEqual(receipt2.status, 0) | ||
| self.assertEqual(receipt2.error_message, "Insufficient contract balance for transfers") | ||
|
|
||
| # State should be completely rolled back (target balance 0, contract balance remains 100) | ||
| self.assertEqual(self.state.get_account(contract_addr)['balance'], 100) | ||
| self.assertEqual(self.state.get_account(self.target_pk)['balance'], 0) | ||
|
|
||
| # Storage should NOT be updated | ||
| self.assertEqual(self.state.get_account(contract_addr)['storage'], {}) | ||
|
|
||
| def test_transfer_with_incoming_funds(self): | ||
| # 1. Deploy Contract (0 initial balance) | ||
| code = """ | ||
| target = msg['data']['target'] | ||
| # We use the incoming funds to instantly transfer out! | ||
| transfer_out(target, msg['value']) | ||
| """ | ||
| deploy_tx = self._sign(Transaction(self.sender_pk, None, amount=0, nonce=0, data=code, fee=1000)) | ||
| receipt = self.state.apply_transaction(deploy_tx) | ||
| self.assertEqual(receipt.status, 1) | ||
| contract_addr = receipt.contract_address | ||
|
|
||
| self.assertEqual(self.state.get_account(contract_addr)['balance'], 0) | ||
|
|
||
| # 2. Call Contract sending 50 coins | ||
| call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=50, nonce=1, data={"target": self.target_pk}, fee=1000)) | ||
| receipt2 = self.state.apply_transaction(call_tx) | ||
|
|
||
| self.assertEqual(receipt2.status, 1) | ||
|
|
||
| # Contract balance should be 0 (received 50, sent 50) | ||
| self.assertEqual(self.state.get_account(contract_addr)['balance'], 0) | ||
|
|
||
| # Target should have exactly 50 | ||
| self.assertEqual(self.state.get_account(self.target_pk)['balance'], 50) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a regression test for invalid transfer_out target addresses.
Current tests miss the malformed-address path. Add a case where contract code calls transfer_out("not_hex", 1) and assert failed receipt plus unchanged balances/storage.
Suggested test shape
+ def test_transfer_out_rejects_invalid_target_address(self):
+ code = """
+transfer_out('not_hex', 1)
+"""
+ deploy_tx = self._sign(Transaction(self.sender_pk, None, amount=10, nonce=0, data=code, fee=1000))
+ receipt = self.state.apply_transaction(deploy_tx)
+ self.assertEqual(receipt.status, 1)
+ contract_addr = receipt.contract_address
+
+ call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=0, nonce=1, data={}, fee=1000))
+ receipt2 = self.state.apply_transaction(call_tx)
+
+ self.assertEqual(receipt2.status, 0)
+ self.assertEqual(self.state.get_account(contract_addr)["balance"], 10)🤖 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_contract_transfers.py` around lines 22 - 102, Add a new test
method after test_transfer_with_incoming_funds to cover the malformed address
case for transfer_out. Create a test that deploys a contract with code calling
transfer_out using an invalid hex address (like "not_hex"), then assert that the
transaction fails with status 0, the error message indicates an invalid address,
and verify that the contract balance, target balance, and contract storage all
remain unchanged from their initial states to confirm proper rollback behavior.
Comment on lines
+99
to
+100
| with self.assertRaises(Exception): | ||
| Transaction.from_dict(invalid_payload) |
Contributor
There was a problem hiding this comment.
Assert a specific exception type for malformed tx payloads.
Using Exception is too broad; this test should assert the expected failure mode (e.g., KeyError) so unrelated regressions don’t pass silently.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 99-99: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 99-99: Do not assert blind exception: Exception
(B017)
🤖 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_protocol_hardening.py` around lines 99 - 100, The test for
Transaction.from_dict(invalid_payload) is catching a generic Exception which is
too broad and could mask unrelated regressions. Replace the Exception with the
specific exception type that Transaction.from_dict actually raises when given
malformed input (likely KeyError or another specific exception), so the test
only passes when the expected failure mode occurs and fails if any other
unexpected exceptions occur.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_contract_transfers.py (1)
64-76: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExercise rollback with non-zero incoming value in failure case.
At Line 64 the failing call uses
amount=0, so this test does not validate the sender-refund branch for failed contract execution with incoming funds. Use a non-zeroamountand assert sender balance only losesfee + gas_used.Suggested test hardening
- call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=0, nonce=1, data={"target": self.target_pk}, fee=1000)) + sender_before = self.state.get_account(self.sender_pk)['balance'] + call_tx = self._sign(Transaction(self.sender_pk, contract_addr, amount=50, nonce=1, data={"target": self.target_pk}, fee=1000)) receipt2 = self.state.apply_transaction(call_tx) # Should fail with status 0 self.assertEqual(receipt2.status, 0) self.assertEqual(receipt2.error_message, "Insufficient contract balance for transfers") @@ # Storage should NOT be updated self.assertEqual(self.state.get_account(contract_addr)['storage'], {}) + + sender_after = self.state.get_account(self.sender_pk)['balance'] + self.assertEqual(sender_before - sender_after, call_tx.fee + receipt2.gas_used)🤖 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_contract_transfers.py` around lines 64 - 76, The test case for the failing contract call uses amount=0 in the call_tx Transaction, which does not exercise the refund logic for failed executions with incoming funds. Modify the amount parameter in the Transaction constructor from 0 to a non-zero value (such as 50), then add an assertion after the receipt is processed to verify that the sender's balance (self.sender_pk) has decreased by only the fee amount (or fee plus gas_used if applicable), demonstrating that the incoming amount was properly refunded when the contract execution failed.
🤖 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 125-142: The hello message handler (and similarly the
chain_request and chain_response handlers) does not validate that payload is not
None before calling payload.get() methods on it. Since payload is extracted with
data.get("data") which returns None if the "data" key is missing, this causes an
AttributeError crash on malformed P2P messages. Add a null-check immediately
after payload extraction to validate it is not None before processing any
subsequent payload.get() calls in the message handler branches, similar to how
the tx and block handlers already implement error handling.
In `@minichain/contract.py`:
- Line 167: The `p.join(timeout=10)` call sets a 10-second timeout but the
`RLIMIT_CPU` constraint on line 41 is set to 2 seconds, causing the CPU limit to
kill the process before the timeout can take effect on Unix systems. Update the
`RLIMIT_CPU` value to match the 10-second timeout value to ensure consistent
behavior and proper timeout enforcement across all platforms.
In `@minichain/p2p.py`:
- Around line 97-99: The peer_count property in the P2P class currently returns
a hardcoded 0 because the streams list is inaccessible from the asyncio side.
Create a thread-safe counter variable _peer_count to track the actual number of
connected peers. Replace the hardcoded return value in the peer_count property
to return this _peer_count variable. Then increment _peer_count when streams are
added in the stream_handler method and decrement _peer_count when streams are
removed in the DISCONNECT handler, ensuring proper synchronization between the
trio and asyncio contexts.
In `@requirements.txt`:
- Line 4: The aiohttp dependency in requirements.txt is currently unpinned,
which creates security and reproducibility risks as multiple versions with known
vulnerabilities may be installed. Replace the unpinned aiohttp entry with a
specific version constraint such as aiohttp>=3.14.1 (for the latest stable
version) or aiohttp>=3.10.11 (to address request smuggling vulnerabilities),
aligning with the project's practice of pinning dependencies like pynacl to
ensure consistent and secure builds across environments.
---
Outside diff comments:
In `@tests/test_contract_transfers.py`:
- Around line 64-76: The test case for the failing contract call uses amount=0
in the call_tx Transaction, which does not exercise the refund logic for failed
executions with incoming funds. Modify the amount parameter in the Transaction
constructor from 0 to a non-zero value (such as 50), then add an assertion after
the receipt is processed to verify that the sender's balance (self.sender_pk)
has decreased by only the fee amount (or fee plus gas_used if applicable),
demonstrating that the incoming amount was properly refunded when the contract
execution failed.
🪄 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: 84ee31df-77a5-4704-9f0f-605e95aaf58e
📒 Files selected for processing (14)
genesis.jsonmain.pyminichain/chain.pyminichain/contract.pyminichain/mempool.pyminichain/p2p.pyminichain/rpc.pyminichain/state.pyminichain/transaction.pyrequirements.txttests/test_contract_transfers.pytests/test_reorg.pytests/test_rpc.pytests/test_transaction_signing.py
💤 Files with no reviewable changes (1)
- tests/test_reorg.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.py (1)
141-145: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUnicast the catch-up
chain_requestto the ahead peer instead of broadcasting.The
hellohandler already knows the specific peer that is ahead (peer_addr), yet Line 145 sends thechain_requestvia_broadcast_rawto every connected peer. Each peer then replies with a full block batch, causing duplicate sync traffic and amplification. Target the originating peer with_unicast_raw.Proposed fix
req = {"type": "chain_request", "data": {"start_index": chain.last_block.index + 1, "limit": 500}} - asyncio.create_task(network._broadcast_raw(req)) + asyncio.create_task(network._unicast_raw(peer_addr, req))🤖 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 141 - 145, The chain_request is being broadcast to all connected peers instead of being sent only to the peer that is ahead, causing duplicate sync traffic. In the hello handler where peer_addr is known, replace the network._broadcast_raw call with network._unicast_raw and pass peer_addr as the target peer parameter to send the catch-up request only to the specific peer that is ahead rather than amplifying the request across the entire network.minichain/p2p.py (1)
93-94: 🚀 Performance & Scalability | 🟠 Major
send_chain_responsebroadcasts to all peers instead of unicasting to the requesting peer.Using
_broadcast_rawsends the full block batch to every connected peer, causing unnecessary bandwidth amplification and wasted network resources. Thepeer_streamparameter accepts a target but is never used. Use_unicast_raw(peer_addr, payload)to send the response only to the requesting peer, matching the pattern already implemented inmain.py'schain_requesthandler (line ~193).Note: This method has no call sites in the codebase; it may be unused and could be removed entirely unless it's part of a public API contract.
🤖 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/p2p.py` around lines 93 - 94, The send_chain_response method currently uses _broadcast_raw which sends the chain response to all connected peers instead of unicasting to only the requesting peer. Replace the _broadcast_raw call with _unicast_raw, extracting the peer address from the peer_stream parameter and passing it as the first argument along with the payload dictionary. This ensures the chain response is sent only to the requesting peer, matching the pattern already implemented in the chain_request handler. The peer_stream parameter is currently unused but should be utilized to identify the target peer for unicasting.
🤖 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.
Outside diff comments:
In `@main.py`:
- Around line 141-145: The chain_request is being broadcast to all connected
peers instead of being sent only to the peer that is ahead, causing duplicate
sync traffic. In the hello handler where peer_addr is known, replace the
network._broadcast_raw call with network._unicast_raw and pass peer_addr as the
target peer parameter to send the catch-up request only to the specific peer
that is ahead rather than amplifying the request across the entire network.
In `@minichain/p2p.py`:
- Around line 93-94: The send_chain_response method currently uses
_broadcast_raw which sends the chain response to all connected peers instead of
unicasting to only the requesting peer. Replace the _broadcast_raw call with
_unicast_raw, extracting the peer address from the peer_stream parameter and
passing it as the first argument along with the payload dictionary. This ensures
the chain response is sent only to the requesting peer, matching the pattern
already implemented in the chain_request handler. The peer_stream parameter is
currently unused but should be utilized to identify the target peer for
unicasting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d34f6e9-95e8-40b8-9313-ea72bf526bae
📒 Files selected for processing (5)
main.pyminichain/contract.pyminichain/p2p.pyrequirements.txttests/test_contract_transfers.py
| temp_state.chain_id = self.chain_id | ||
| temp_state.restore(self._genesis_state_snapshot) | ||
|
|
||
| # Verify and apply blocks 1 to N |
Contributor
There was a problem hiding this comment.
Typically, the two chains will have a common prefix. In other words, there will be a a block N*, such that both chains will be equal to each other up to block N*.
Instead of recomputing everything from genesis, it would be more efficient to roll_back the state to N* and then recompute everything only from N*.
I would like to accept and merge this PR as it is, though. But could you create an issue to improve this aspect in a future PR?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Implement libp2p
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
Release Notes
New Features
Bug Fixes
Tests