Feat/ transaction receipts#89
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a Receipt model and threads per-transaction receipts through state execution, mining, block construction, serialization, P2P validation, persistence, and tests; computes and verifies a receipt Merkle root and replaces the custom MPT with trie.HexaryTrie. ChangesTransaction Execution Receipts Integration
Sequence Diagram(s)sequenceDiagram
participant Miner as Miner (main.py)
participant State as State (minichain/state.py)
participant Receipt as Receipt (minichain/receipt.py)
participant Block as Block (minichain/block.py)
participant Chain as Chain (minichain/chain.py)
Miner->>State: validate_and_apply(tx)
State->>Receipt: construct Receipt(tx_hash,status,...)
State-->>Miner: Receipt or None
Miner->>Block: calculate_receipt_root(receipts)
Block-->>Miner: receipt_root
Miner->>Block: Block(..., receipt_root, receipts)
Chain->>State: temp_state.validate_and_apply(tx) (on add_block)
State-->>Chain: receipts
Chain->>Block: calculate_receipt_root(receipts)
Chain->>Chain: verify computed_root == block.receipt_root
Chain->>Chain: verify state_root
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/p2p.py (1)
178-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed receipt objects during block schema validation.
_validate_block_payload()now requires"receipts", but it never validates the items inside that list. A payload with non-dicts or receipts missingtx_hash/statuswill pass this check and only blow up later inBlock.from_dict(), which turns malformed peer input into handler exceptions instead of a clean schema reject.Suggested fix
+ def _validate_receipt_payload(self, payload): + if not isinstance(payload, dict): + return False + + required_fields = { + "tx_hash": str, + "status": int, + "gas_used": int, + "error_message": (str, type(None)), + "logs": list, + "contract_address": (str, type(None)), + } + + if set(payload) != set(required_fields): + return False + + for field, expected_type in required_fields.items(): + if not isinstance(payload.get(field), expected_type): + return False + + return True + def _validate_block_payload(self, payload): if not isinstance(payload, dict): return False @@ - return all( - self._validate_transaction_payload(tx_payload) - for tx_payload in payload["transactions"] - ) + return ( + all( + self._validate_transaction_payload(tx_payload) + for tx_payload in payload["transactions"] + ) + and all( + self._validate_receipt_payload(receipt_payload) + for receipt_payload in payload["receipts"] + ) + )🤖 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 178 - 214, _in _validate_block_payload ensure the "receipts" list is schema-validated: check payload["receipts"] is a list of dicts and for each receipt verify required fields "tx_hash" and "status" exist and have the expected types (e.g. "tx_hash" is str and "status" is int), rejecting the block if any receipt is malformed; update the validation in _validate_block_payload (and mirror any type allowances used elsewhere) so malformed receipts are rejected early instead of causing exceptions later in Block.from_dict.
🤖 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/chain.py`:
- Around line 116-134: The code validates only block.receipt_root against a
locally computed root but never compares the actual serialized block.receipts
payload, allowing a peer to supply forged receipts; update the block validation
logic (in the same function that uses temp_state.validate_and_apply,
compute_receipt_root and handles block.receipt_root) to also verify that the
locally generated receipts list exactly matches block.receipts (or its canonical
serialization) and reject the block (return False and log a warning) if they
differ, ensuring both the receipt root and the full receipts body are consistent
before accepting the block.
In `@setup.py`:
- Around line 8-10: Pin the trie dependency to an exact, tested version to avoid
consensus changes: update the install_requires entry in setup.py that currently
lists "trie>=3.1.0" to the exact version you vetted (e.g., "trie==3.1.0"), and
make the identical change in requirements.txt; ensure you modify the
install_requires array and the corresponding requirements.txt line so both
package specification locations match exactly.
In `@tests/test_persistence.py`:
- Around line 45-55: Add an explicit round-trip assertion that checks receipts
are persisted and restored: after constructing Block with receipts (using
temp_state.validate_and_apply(tx), calculate_receipt_root([receipt]), and
Block(... receipts=[receipt])), load/deserialize the stored block and assert
that loaded_block.receipt_root equals the original block.receipt_root and that
loaded_block.receipts contains a serialized receipt matching the original
receipt (e.g., compare serialized payload or fields), ensuring the persistence
layer actually keeps both receipt_root and receipts rather than only
header/state fields.
---
Outside diff comments:
In `@minichain/p2p.py`:
- Around line 178-214: _in _validate_block_payload ensure the "receipts" list is
schema-validated: check payload["receipts"] is a list of dicts and for each
receipt verify required fields "tx_hash" and "status" exist and have the
expected types (e.g. "tx_hash" is str and "status" is int), rejecting the block
if any receipt is malformed; update the validation in _validate_block_payload
(and mirror any type allowances used elsewhere) so malformed receipts are
rejected early instead of causing exceptions later in Block.from_dict.
🪄 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: 454f5cae-7614-4210-91e2-e7b25aa3737a
📒 Files selected for processing (15)
main.pyminichain/block.pyminichain/chain.pyminichain/contract.pyminichain/mpt.pyminichain/p2p.pyminichain/persistence.pyminichain/receipt.pyminichain/state.pyrequirements.txtsetup.pytests/test_contract.pytests/test_persistence.pytests/test_persistence_runtime.pytests/test_protocol_hardening.py
| receipt = temp_state.validate_and_apply(tx) | ||
|
|
||
| from minichain.block import calculate_receipt_root | ||
| block = Block( | ||
| index=1, | ||
| previous_hash=bc.last_block.hash, | ||
| transactions=[tx], | ||
| difficulty=1, | ||
| state_root=temp_state.state_root(), | ||
| receipt_root=calculate_receipt_root([receipt]), | ||
| receipts=[receipt], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an explicit receipt round-trip assertion.
This helper now persists receipts and receipt_root, but the suite still only verifies header/hash/state fields. Because Block.compute_hash() covers the header, a loader that silently drops block.receipts could still pass these tests. Please assert that the restored block preserves both receipt_root and the serialized receipt payload.
Proposed test addition
def test_block_hashes_preserved(self):
bc, _, _ = self._chain_with_tx()
save(bc, path=self.tmpdir)
restored = load(path=self.tmpdir)
for original, loaded in zip(bc.chain, restored.chain):
self.assertEqual(original.hash, loaded.hash)
self.assertEqual(original.index, loaded.index)
self.assertEqual(original.previous_hash, loaded.previous_hash)
+ self.assertEqual(restored.chain[1].receipt_root, bc.chain[1].receipt_root)
+ self.assertEqual(
+ [receipt.to_dict() for receipt in restored.chain[1].receipts],
+ [receipt.to_dict() for receipt in bc.chain[1].receipts],
+ )🤖 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_persistence.py` around lines 45 - 55, Add an explicit round-trip
assertion that checks receipts are persisted and restored: after constructing
Block with receipts (using temp_state.validate_and_apply(tx),
calculate_receipt_root([receipt]), and Block(... receipts=[receipt])),
load/deserialize the stored block and assert that loaded_block.receipt_root
equals the original block.receipt_root and that loaded_block.receipts contains a
serialized receipt matching the original receipt (e.g., compare serialized
payload or fields), ensuring the persistence layer actually keeps both
receipt_root and receipts rather than only header/state fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/p2p.py`:
- Around line 211-225: The loop validating each receipt in p2p.py currently
checks expected fields but allows extra keys; update the validation in the
receipts iteration (the for r_payload in payload.get("receipts", []) block) to
compute allowed_keys =
{"tx_hash","status","gas_used","error_message","logs","contract_address"} and
reject any r_payload whose set(r_payload.keys()) is not a subset of allowed_keys
so unexpected keys fail validation; align this check with Receipt.from_dict()
behavior to ensure P2P validation enforces the same canonical schema.
🪄 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: 25989440-e917-4c5c-98f6-5f0327869746
📒 Files selected for processing (4)
minichain/chain.pyminichain/p2p.pysetup.pytests/test_persistence.py
💤 Files with no reviewable changes (1)
- setup.py
| for r_payload in payload.get("receipts", []): | ||
| if not isinstance(r_payload, dict): | ||
| return False | ||
| if "tx_hash" not in r_payload or not isinstance(r_payload["tx_hash"], str): | ||
| return False | ||
| if "status" not in r_payload or not isinstance(r_payload["status"], int): | ||
| return False | ||
| if "gas_used" in r_payload and not isinstance(r_payload["gas_used"], int): | ||
| return False | ||
| if "error_message" in r_payload and not isinstance(r_payload["error_message"], (str, type(None))): | ||
| return False | ||
| if "logs" in r_payload and not isinstance(r_payload["logs"], list): | ||
| return False | ||
| if "contract_address" in r_payload and not isinstance(r_payload["contract_address"], (str, type(None))): | ||
| return False |
There was a problem hiding this comment.
Reject unexpected keys inside receipt payloads.
These checks validate known fields but still accept arbitrary extra keys in each receipt object. Receipt.from_dict() in minichain/receipt.py drops unknown fields, so a block message can pass P2P validation and then be silently rewritten during deserialization/persistence. Enforce the canonical receipt schema here too.
Proposed fix
+ allowed_receipt_fields = {
+ "tx_hash",
+ "status",
+ "gas_used",
+ "error_message",
+ "logs",
+ "contract_address",
+ }
for r_payload in payload.get("receipts", []):
if not isinstance(r_payload, dict):
return False
+ if not set(r_payload).issubset(allowed_receipt_fields):
+ return False
if "tx_hash" not in r_payload or not isinstance(r_payload["tx_hash"], str):
return False
if "status" not in r_payload or not isinstance(r_payload["status"], int):
return False📝 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.
| for r_payload in payload.get("receipts", []): | |
| if not isinstance(r_payload, dict): | |
| return False | |
| if "tx_hash" not in r_payload or not isinstance(r_payload["tx_hash"], str): | |
| return False | |
| if "status" not in r_payload or not isinstance(r_payload["status"], int): | |
| return False | |
| if "gas_used" in r_payload and not isinstance(r_payload["gas_used"], int): | |
| return False | |
| if "error_message" in r_payload and not isinstance(r_payload["error_message"], (str, type(None))): | |
| return False | |
| if "logs" in r_payload and not isinstance(r_payload["logs"], list): | |
| return False | |
| if "contract_address" in r_payload and not isinstance(r_payload["contract_address"], (str, type(None))): | |
| return False | |
| allowed_receipt_fields = { | |
| "tx_hash", | |
| "status", | |
| "gas_used", | |
| "error_message", | |
| "logs", | |
| "contract_address", | |
| } | |
| for r_payload in payload.get("receipts", []): | |
| if not isinstance(r_payload, dict): | |
| return False | |
| if not set(r_payload).issubset(allowed_receipt_fields): | |
| return False | |
| if "tx_hash" not in r_payload or not isinstance(r_payload["tx_hash"], str): | |
| return False | |
| if "status" not in r_payload or not isinstance(r_payload["status"], int): | |
| return False | |
| if "gas_used" in r_payload and not isinstance(r_payload["gas_used"], int): | |
| return False | |
| if "error_message" in r_payload and not isinstance(r_payload["error_message"], (str, type(None))): | |
| return False | |
| if "logs" in r_payload and not isinstance(r_payload["logs"], list): | |
| return False | |
| if "contract_address" in r_payload and not isinstance(r_payload["contract_address"], (str, type(None))): | |
| 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 211 - 225, The loop validating each receipt in
p2p.py currently checks expected fields but allows extra keys; update the
validation in the receipts iteration (the for r_payload in
payload.get("receipts", []) block) to compute allowed_keys =
{"tx_hash","status","gas_used","error_message","logs","contract_address"} and
reject any r_payload whose set(r_payload.keys()) is not a subset of allowed_keys
so unexpected keys fail validation; align this check with Receipt.from_dict()
behavior to ensure P2P validation enforces the same canonical schema.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/block.py (1)
81-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHeader serialization still includes
minerwhen absent.Line 90 unconditionally writes
"miner": self.miner, so the header always containsminer(includingnull). That breaks the optional-header contract and can produce hash/serialization mismatches across peers expecting omission.Suggested fix
def to_header_dict(self): header = { "index": self.index, "previous_hash": self.previous_hash, "merkle_root": self.merkle_root, "state_root": self.state_root, "receipt_root": self.receipt_root, "timestamp": self.timestamp, "difficulty": self.difficulty, "nonce": self.nonce, - "miner": self.miner, } - # Include miner in header only when present (optional field) <-- Reworded comment + # Include miner in header only when present (optional field) if self.miner is not None: - header["miner"] = self.miner + header["miner"] = self.miner return header🤖 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` around lines 81 - 95, The header dict currently includes "miner" unconditionally (key present with None), violating the optional-header contract; update the code in the block header serialization (where header is built) to stop setting "miner" in the initial dict and instead only insert header["miner"] = self.miner inside the existing if self.miner is not None block so the key is omitted when miner is absent.
🤖 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`:
- Around line 162-165: from_dict() currently checks merkle_root but doesn't
validate receipt_root against the receipts; compute the canonical receipt root
from payload["receipts"] (or block.recompute_receipt_root / equivalent logic)
and compare it to payload["receipt_root"], raising ValueError("receipt_root does
not match receipts") if they differ, analogous to the merkle_root check so
malformed payloads cannot carry inconsistent receipts.
---
Outside diff comments:
In `@minichain/block.py`:
- Around line 81-95: The header dict currently includes "miner" unconditionally
(key present with None), violating the optional-header contract; update the code
in the block header serialization (where header is built) to stop setting
"miner" in the initial dict and instead only insert header["miner"] = self.miner
inside the existing if self.miner is not None block so the key is omitted when
miner is absent.
🪄 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: ebae81eb-ae6a-40e2-b364-ddd05227a293
📒 Files selected for processing (5)
main.pyminichain/block.pyminichain/p2p.pytests/test_core.pytests/test_transaction_signing.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/block.py (1)
81-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHeader serialization still includes
minerwhen absent.Line 90 unconditionally writes
"miner": self.miner, so the header always containsminer(includingnull). That breaks the optional-header contract and can produce hash/serialization mismatches across peers expecting omission.Suggested fix
def to_header_dict(self): header = { "index": self.index, "previous_hash": self.previous_hash, "merkle_root": self.merkle_root, "state_root": self.state_root, "receipt_root": self.receipt_root, "timestamp": self.timestamp, "difficulty": self.difficulty, "nonce": self.nonce, - "miner": self.miner, } - # Include miner in header only when present (optional field) <-- Reworded comment + # Include miner in header only when present (optional field) if self.miner is not None: - header["miner"] = self.miner + header["miner"] = self.miner return header🤖 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` around lines 81 - 95, The header dict currently includes "miner" unconditionally (key present with None), violating the optional-header contract; update the code in the block header serialization (where header is built) to stop setting "miner" in the initial dict and instead only insert header["miner"] = self.miner inside the existing if self.miner is not None block so the key is omitted when miner is absent.
🤖 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`:
- Around line 162-165: from_dict() currently checks merkle_root but doesn't
validate receipt_root against the receipts; compute the canonical receipt root
from payload["receipts"] (or block.recompute_receipt_root / equivalent logic)
and compare it to payload["receipt_root"], raising ValueError("receipt_root does
not match receipts") if they differ, analogous to the merkle_root check so
malformed payloads cannot carry inconsistent receipts.
---
Outside diff comments:
In `@minichain/block.py`:
- Around line 81-95: The header dict currently includes "miner" unconditionally
(key present with None), violating the optional-header contract; update the code
in the block header serialization (where header is built) to stop setting
"miner" in the initial dict and instead only insert header["miner"] = self.miner
inside the existing if self.miner is not None block so the key is omitted when
miner is absent.
🪄 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: ebae81eb-ae6a-40e2-b364-ddd05227a293
📒 Files selected for processing (5)
main.pyminichain/block.pyminichain/p2p.pytests/test_core.pytests/test_transaction_signing.py
🛑 Comments failed to post (1)
minichain/block.py (1)
162-165:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
receipt_rootis not validated duringfrom_dict()parsing.
from_dict()validatesmerkle_rootbut skips validatingreceipt_rootagainstreceipts. A malformed payload can carry inconsistent receipts through this boundary.Suggested fix
# Recalculate and verify the Merkle root! if "merkle_root" in payload and payload["merkle_root"] != block.merkle_root: raise ValueError("merkle_root does not match transactions") + + if "receipt_root" in payload: + expected_receipt_root = calculate_receipt_root(block.receipts) + if payload["receipt_root"] != expected_receipt_root: + raise ValueError("receipt_root does not match receipts") return block🧰 Tools
🪛 Ruff (0.15.15)
[warning] 164-164: 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 `@minichain/block.py` around lines 162 - 165, from_dict() currently checks merkle_root but doesn't validate receipt_root against the receipts; compute the canonical receipt root from payload["receipts"] (or block.recompute_receipt_root / equivalent logic) and compare it to payload["receipt_root"], raising ValueError("receipt_root does not match receipts") if they differ, analogous to the merkle_root check so malformed payloads cannot carry inconsistent receipts.
1 participant
Addressed Issues:
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
Tests