Feat/fee handling#91
Conversation
WalkthroughThis PR introduces a transaction fee system and comprehensive receipt tracking for the MiniChain blockchain. Transactions now carry a fee field that is deducted from the sender's balance. State application returns structured Receipt objects instead of booleans, capturing transaction outcomes including status codes and error details. Blocks include receipt roots and receipts lists for consensus validation. Mining rewards are augmented by collected transaction fees. ChangesTransaction Fees and Receipt Tracking System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_core.py (2)
59-60: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse explicit None check for failed transactions.
assertFalse(result)works becauseNoneis falsy, butassertIsNone(result)makes the expected behavior explicit and aligns with the receipt API contract.✅ Proposed improvement
result = self.state.apply_transaction(tx) - self.assertFalse(result) + self.assertIsNone(result)🤖 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_core.py` around lines 59 - 60, The test currently uses a falsy check for a failed transaction result; replace the generic self.assertFalse(result) with an explicit self.assertIsNone(result) to make the contract that apply_transaction(tx) returns None on failure explicit — locate the call to self.state.apply_transaction(tx) and the assertion on the local variable result and update the assertion accordingly.
45-46: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate assertion to validate receipt status explicitly.
With the receipt-based API,
assertTrue(result)is semantically unclear—it relies on receipt truthiness rather than validating success status.✅ Proposed improvement
result = self.state.apply_transaction(tx) - self.assertTrue(result) + self.assertIsNotNone(result) + self.assertEqual(result.status, 1)🤖 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_core.py` around lines 45 - 46, The test currently uses a truthiness check (self.assertTrue(result)) after calling self.state.apply_transaction(tx); replace this with an explicit assertion on the transaction receipt status returned by apply_transaction: capture the receipt (e.g., receipt = self.state.apply_transaction(tx)) and assert its success status explicitly (for example self.assertEqual(receipt.status, 'SUCCESS') or self.assertEqual(receipt['status'], 'SUCCESS') or self.assertEqual(receipt.status, ReceiptStatus.SUCCESS) depending on whether receipt is an object, dict, or uses an enum) so the test verifies the actual success indicator rather than truthiness.
🤖 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 `@minichain/block.py`:
- Line 56: In the Block class constructor remove the duplicate assignment of
self.miner so the field is only set once; locate the __init__ (or constructor)
where self.miner is assigned twice and delete one of the assignments (keep the
one that is in the correct logical place for initialization), ensuring no other
initialization logic depends on the removed line and that the remaining
assignment is the authoritative value for the miner attribute.
In `@minichain/mpt.py`:
- Around line 17-20: The get() method currently treats any non-None bytes
(including b'') as a present value, so missing keys that return b'' become empty
strings instead of None; update the logic in get (which calls self._trie.get) to
treat empty bytes as missing by returning None when val is None or len(val) ==
0, otherwise return val.decode(); change the condition from "if val is not None"
to an explicit emptiness check against val from self._trie.get.
In `@minichain/p2p.py`:
- Around line 143-144: The transaction validation currently checks
payload["amount"] but misses validating the fee; update the P2P-side validation
(the block that checks payload["amount"] in minichain/p2p.py, e.g., inside the
transaction validation/handler function) to also enforce payload["fee"] >= 0 and
return False when the fee is negative. Locate the same validation scope that
contains the line `if payload["amount"] < 0: return False` and add a similar
guard for `payload["fee"]`, ensuring the check uses the same semantics (reject
negative fees) and mirrors error handling/return behavior.
In `@requirements.txt`:
- Line 2: The requirements entry for the package "trie" currently uses an
open-ended range "trie>=3.1.0"; change it to a bounded or compatible specifier
to avoid accidental major upgrades — e.g., replace with "trie>=3.1.0,<4.0.0" or
"trie~=3.1.0" in requirements.txt so installations remain reproducible and safe
from breaking changes.
In `@tests/test_core.py`:
- Around line 65-114: Add a new unit test named
test_insufficient_balance_with_fee in tests/test_core.py that asserts a
transaction where amount alone is <= balance but amount + fee > balance is
rejected; ensure the test uses Transaction(..., fee=...) and signs it, then
calls the state's validation/apply method (validate_and_apply or
apply_transaction depending on existing API) and expects None for the failing
case and a non-None receipt for the borderline succeeding case. If tests fail,
fix the state validation in State.validate_and_apply / State.apply_transaction
to check total_cost = amount + fee (and nonce) against sender balance (use
get_account or accounts[...] to read balances) and return None / raise/abort
when balance < total_cost so fees are correctly considered in balance
validation.
- Around line 105-107: Don't mutate chain.state.accounts directly; instead use
the State/Chain public API to set balances and nonces so tests don't rely on
internal dict layout. Replace the three direct assignments to
chain.state.accounts[self.alice_pk]['balance'] / ['nonce'] and
chain.state.accounts[self.bob_pk]['balance'] with the appropriate API calls
(e.g. state.set_balance(self.alice_pk, 100), state.set_nonce(self.alice_pk, 0),
state.set_balance(self.bob_pk, 0) or
Chain.set_account_balance/Chain.set_account_nonce if those exist), or add small
test helpers on State/Chain to perform these updates atomically, referencing
chain.state and self.alice_pk/self.bob_pk from the diff.
- Around line 84-94: Remove the unused manual Block construction: the local
variable block created via Block(...) is never used because
mine_and_process_block later builds and processes its own block; delete the
entire Block(...) instantiation (the variable named block) from the test to
eliminate dead code and potential confusion, leaving the call to
mine_and_process_block intact.
---
Outside diff comments:
In `@tests/test_core.py`:
- Around line 59-60: The test currently uses a falsy check for a failed
transaction result; replace the generic self.assertFalse(result) with an
explicit self.assertIsNone(result) to make the contract that
apply_transaction(tx) returns None on failure explicit — locate the call to
self.state.apply_transaction(tx) and the assertion on the local variable result
and update the assertion accordingly.
- Around line 45-46: The test currently uses a truthiness check
(self.assertTrue(result)) after calling self.state.apply_transaction(tx);
replace this with an explicit assertion on the transaction receipt status
returned by apply_transaction: capture the receipt (e.g., receipt =
self.state.apply_transaction(tx)) and assert its success status explicitly (for
example self.assertEqual(receipt.status, 'SUCCESS') or
self.assertEqual(receipt['status'], 'SUCCESS') or
self.assertEqual(receipt.status, ReceiptStatus.SUCCESS) depending on whether
receipt is an object, dict, or uses an enum) so the test verifies the actual
success indicator rather than truthiness.
🪄 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: 828ad7d3-7b61-4f80-bb46-76d12b69e09c
📒 Files selected for processing (19)
main.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_transaction_signing.py
💤 Files with no reviewable changes (1)
- setup.py
| # Freeze transactions into an immutable tuple to prevent header/body mismatch | ||
| self.transactions = tuple(transactions) if transactions else () | ||
| self.receipts = tuple(receipts) if receipts else () | ||
| self.miner = miner |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Duplicate assignment of self.miner.
self.miner is assigned twice: once at line 56 and again at line 68. Remove one of them.
🧹 Proposed fix
self.transactions = tuple(transactions) if transactions else ()
self.receipts = tuple(receipts) if receipts else ()
- self.miner = miner
# Deterministic timestamp (ms)
self.timestamp: int = (
round(time.time() * 1000)Also applies to: 68-68
🤖 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/block.py` at line 56, In the Block class constructor remove the
duplicate assignment of self.miner so the field is only set once; locate the
__init__ (or constructor) where self.miner is assigned twice and delete one of
the assignments (keep the one that is in the correct logical place for
initialization), ensuring no other initialization logic depends on the removed
line and that the remaining assignment is the authoritative value for the miner
attribute.
| def get(self, key_hex: str) -> Optional[str]: | ||
| if not self.root: | ||
| return None | ||
| return self._get(self.root, to_nibbles(key_hex)) | ||
|
|
||
| def _get(self, node: Optional[Node], path: List[int]) -> Optional[str]: | ||
| if not node: | ||
| return None | ||
|
|
||
| if isinstance(node, LeafNode): | ||
| if node.path == path: | ||
| return node.value | ||
| return None | ||
|
|
||
| elif isinstance(node, ExtensionNode): | ||
| if path[:len(node.path)] == node.path: | ||
| return self._get(node.child, path[len(node.path):]) | ||
| return None | ||
|
|
||
| elif isinstance(node, BranchNode): | ||
| if not path: | ||
| return node.value | ||
| nibble = path[0] | ||
| return self._get(node.branches[nibble], path[1:]) | ||
|
|
||
| return None | ||
| key = bytes.fromhex(key_hex) | ||
| val = self._trie.get(key) | ||
| return val.decode() if val is not None else None |
There was a problem hiding this comment.
get() may return "" instead of None for missing keys.
HexaryTrie.get() typically returns b'' (empty bytes) for non-existent keys rather than None. The check if val is not None will be True for b'', causing the method to return "" instead of None for missing keys. This breaks the Optional[str] contract.
🐛 Proposed fix
def get(self, key_hex: str) -> Optional[str]:
key = bytes.fromhex(key_hex)
val = self._trie.get(key)
- return val.decode() if val is not None else None
+ return val.decode() if val else Nonepy-trie HexaryTrie get method return value for missing keys
🤖 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/mpt.py` around lines 17 - 20, The get() method currently treats any
non-None bytes (including b'') as a present value, so missing keys that return
b'' become empty strings instead of None; update the logic in get (which calls
self._trie.get) to treat empty bytes as missing by returning None when val is
None or len(val) == 0, otherwise return val.decode(); change the condition from
"if val is not None" to an explicit emptiness check against val from
self._trie.get.
| if payload["amount"] < 0: | ||
| return False |
There was a problem hiding this comment.
Enforce non-negative fee in transaction schema validation.
Line 143 validates amount but does not constrain fee. A negative fee is an economic-consensus exploit vector (can undercharge sender and distort miner rewards). Validate payload["fee"] >= 0 at the P2P boundary.
Proposed fix
- if payload["amount"] < 0:
+ if payload["amount"] < 0:
+ return False
+ if payload["fee"] < 0:
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/p2p.py` around lines 143 - 144, The transaction validation
currently checks payload["amount"] but misses validating the fee; update the
P2P-side validation (the block that checks payload["amount"] in
minichain/p2p.py, e.g., inside the transaction validation/handler function) to
also enforce payload["fee"] >= 0 and return False when the fee is negative.
Locate the same validation scope that contains the line `if payload["amount"] <
0: return False` and add a similar guard for `payload["fee"]`, ensuring the
check uses the same semantics (reject negative fees) and mirrors error
handling/return behavior.
| @@ -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 bounding the trie dependency version.
Using >=3.1.0 without an upper bound allows future major versions to be installed, which could introduce breaking changes. Consider using a bounded range like trie>=3.1.0,<4.0.0 or the compatible release specifier trie~=3.1.0 for reproducible builds.
♻️ Proposed fix
pynacl==1.6.2
-trie>=3.1.0
+trie>=3.1.0,<4.0.0📝 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.
| trie>=3.1.0 | |
| trie>=3.1.0,<4.0.0 |
🤖 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 requirements entry for the package "trie"
currently uses an open-ended range "trie>=3.1.0"; change it to a bounded or
compatible specifier to avoid accidental major upgrades — e.g., replace with
"trie>=3.1.0,<4.0.0" or "trie~=3.1.0" in requirements.txt so installations
remain reproducible and safe from breaking changes.
| def test_transaction_fee(self): | ||
| """Test that transaction fees are properly deducted and credited.""" | ||
| self.state.credit_mining_reward(self.alice_pk, 100) | ||
|
|
||
| tx = Transaction(self.alice_pk, self.bob_pk, 40, 0, fee=5) | ||
| tx.sign(self.alice_sk) | ||
|
|
||
| receipt = self.state.validate_and_apply(tx) | ||
| self.assertIsNotNone(receipt) | ||
|
|
||
| # Check sender balance (100 - 40 - 5 = 55) | ||
| self.assertEqual(self.state.get_account(self.alice_pk)['balance'], 55) | ||
| # Check receiver balance (40) | ||
| self.assertEqual(self.state.get_account(self.bob_pk)['balance'], 40) | ||
|
|
||
| # Test miner reward with fee | ||
| from minichain.block import Block, calculate_receipt_root | ||
| import main | ||
|
|
||
| # Manually create a block to simulate chain processing | ||
| block = Block( | ||
| index=1, | ||
| previous_hash="0", | ||
| transactions=[tx], | ||
| difficulty=1, | ||
| state_root=self.state.state_root(), | ||
| receipt_root=calculate_receipt_root([receipt]), | ||
| receipts=[receipt], | ||
| miner=self.bob_pk | ||
| ) | ||
|
|
||
| # We need to simulate the chain.add_block logic for the miner credit | ||
| # Actually, let's just use Blockchain.add_block logic directly by mining | ||
| self.chain.state = self.state.copy() | ||
|
|
||
| from minichain.mempool import Mempool | ||
| mempool = Mempool() | ||
| mempool.add_transaction(tx) | ||
|
|
||
| # Revert state to before tx since mine_and_process_block will re-apply it | ||
| self.chain.state.accounts[self.alice_pk]['balance'] = 100 | ||
| self.chain.state.accounts[self.alice_pk]['nonce'] = 0 | ||
| self.chain.state.accounts[self.bob_pk]['balance'] = 0 | ||
|
|
||
| mined_block = main.mine_and_process_block(self.chain, mempool, self.bob_pk) | ||
| self.assertIsNotNone(mined_block) | ||
|
|
||
| # Bob was the miner. Bob gets amount(40) + mining_reward(50) + fee(5) = 95 | ||
| self.assertEqual(self.chain.state.get_account(self.bob_pk)['balance'], 95) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test case for insufficient balance due to fees.
The test suite validates insufficient amount (test_insufficient_funds) and sufficient balance with fees (test_transaction_fee), but misses the edge case where amount is valid but amount + fee exceeds balance. This gap could mask bugs in the balance validation logic that checks amount + fee.
🧪 Suggested test case
def test_insufficient_balance_with_fee(self):
"""Test that transactions fail when amount+fee exceeds balance."""
self.state.credit_mining_reward(self.alice_pk, 50)
# Amount is 45 (valid), but amount + fee = 50, which equals balance
# Should succeed
tx1 = Transaction(self.alice_pk, self.bob_pk, 45, 0, fee=5)
tx1.sign(self.alice_sk)
result1 = self.state.apply_transaction(tx1)
self.assertIsNotNone(result1)
# Reset for next test
self.state.accounts[self.alice_pk]['balance'] = 50
self.state.accounts[self.alice_pk]['nonce'] = 0
# Amount is 46 (valid alone), but amount + fee = 52 > 50
# Should fail
tx2 = Transaction(self.alice_pk, self.bob_pk, 46, 0, fee=6)
tx2.sign(self.alice_sk)
result2 = self.state.apply_transaction(tx2)
self.assertIsNone(result2)🤖 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_core.py` around lines 65 - 114, Add a new unit test named
test_insufficient_balance_with_fee in tests/test_core.py that asserts a
transaction where amount alone is <= balance but amount + fee > balance is
rejected; ensure the test uses Transaction(..., fee=...) and signs it, then
calls the state's validation/apply method (validate_and_apply or
apply_transaction depending on existing API) and expects None for the failing
case and a non-None receipt for the borderline succeeding case. If tests fail,
fix the state validation in State.validate_and_apply / State.apply_transaction
to check total_cost = amount + fee (and nonce) against sender balance (use
get_account or accounts[...] to read balances) and return None / raise/abort
when balance < total_cost so fees are correctly considered in balance
validation.
| # Manually create a block to simulate chain processing | ||
| block = Block( | ||
| index=1, | ||
| previous_hash="0", | ||
| transactions=[tx], | ||
| difficulty=1, | ||
| state_root=self.state.state_root(), | ||
| receipt_root=calculate_receipt_root([receipt]), | ||
| receipts=[receipt], | ||
| miner=self.bob_pk | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove unused block construction.
This block is created but never used—mine_and_process_block on line 109 creates its own block. The dead code adds confusion.
🧹 Proposed cleanup
- # Test miner reward with fee
- from minichain.block import Block, calculate_receipt_root
- import main
-
- # Manually create a block to simulate chain processing
- block = Block(
- index=1,
- previous_hash="0",
- transactions=[tx],
- difficulty=1,
- state_root=self.state.state_root(),
- receipt_root=calculate_receipt_root([receipt]),
- receipts=[receipt],
- miner=self.bob_pk
- )
-
- # We need to simulate the chain.add_block logic for the miner credit
- # Actually, let's just use Blockchain.add_block logic directly by mining
+ # Test miner reward with fee by simulating full mining flow
+ import main
self.chain.state = self.state.copy()🤖 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_core.py` around lines 84 - 94, Remove the unused manual Block
construction: the local variable block created via Block(...) is never used
because mine_and_process_block later builds and processes its own block; delete
the entire Block(...) instantiation (the variable named block) from the test to
eliminate dead code and potential confusion, leaving the call to
mine_and_process_block intact.
| self.chain.state.accounts[self.alice_pk]['balance'] = 100 | ||
| self.chain.state.accounts[self.alice_pk]['nonce'] = 0 | ||
| self.chain.state.accounts[self.bob_pk]['balance'] = 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid direct state dictionary mutation.
Bypassing state management by directly mutating chain.state.accounts is brittle and couples the test to State's internal representation. If State changes its storage structure, this test will break.
♻️ Proposed refactor using state management API
- # Revert state to before tx since mine_and_process_block will re-apply it
- self.chain.state.accounts[self.alice_pk]['balance'] = 100
- self.chain.state.accounts[self.alice_pk]['nonce'] = 0
- self.chain.state.accounts[self.bob_pk]['balance'] = 0
+ # Create fresh state with initial balances for mining test
+ self.chain.state = State()
+ self.chain.state.credit_mining_reward(self.alice_pk, 100)🤖 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_core.py` around lines 105 - 107, Don't mutate chain.state.accounts
directly; instead use the State/Chain public API to set balances and nonces so
tests don't rely on internal dict layout. Replace the three direct assignments
to chain.state.accounts[self.alice_pk]['balance'] / ['nonce'] and
chain.state.accounts[self.bob_pk]['balance'] with the appropriate API calls
(e.g. state.set_balance(self.alice_pk, 100), state.set_nonce(self.alice_pk, 0),
state.set_balance(self.bob_pk, 0) or
Chain.set_account_balance/Chain.set_account_nonce if those exist), or add small
test helpers on State/Chain to perform these updates atomically, referencing
chain.state and self.alice_pk/self.bob_pk from the diff.
Addressed Issues:
This PR implements the transaction fee system. The system now fully supports senders attaching optional fees to their transactions, and miners collecting those fees as a reward for network participation!
Added a dedicated test case (test_transaction_fee) in test_core.py to ensure exact mathematical balances across sender, receiver, and miner.
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Improvements