[FEATURE]: Implement Canonical Serialization for Blocks and Transactions#67
[FEATURE]: Implement Canonical Serialization for Blocks and Transactions#67sanaica wants to merge 29 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughAdds canonical JSON payloads and a Changes
Sequence Diagram(s)sequenceDiagram
participant Creator as Creator
participant Block as Block
participant Canonical as Canonical JSON util
participant P2P as P2P Network
participant Peer as Remote Peer
Creator->>Block: create Block(index, previous_hash, transactions, miner)
Block->>Canonical: compute header canonical hash (canonical_json_hash)
Canonical-->>Block: header hash
Block->>Canonical: canonical_payload = canonical_json_bytes(to_dict())
Canonical-->>Block: canonical payload bytes
P2P->>Block: request canonical_payload
Block-->>P2P: canonical_payload bytes
P2P->>P2P: data = json.loads(canonical_payload.decode("utf-8"))
P2P->>P2P: wire = canonical_json_dumps({"type":"block","data":data}) + "\n"
P2P->>Peer: send wire over TCP
Peer->>Peer: receive -> _validate_message (ensure dict + schema)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 121-124: broadcast_block currently sends block.to_dict() instead
of using Block.canonical_payload, which breaks deterministic wire encoding;
change broadcast_block to transmit the canonical bytes from canonical_payload
and remove the miner field from the serialized canonical block, instead
injecting the dynamic miner info only in the outer envelope (e.g., send
{payload: canonical_payload, miner: miner_id} or equivalent) so the canonical
serialization (canonical_json_bytes / canonical_payload) remains unchanged;
update any receiver/deserializer to read the envelope, verify/parse the
canonical payload with canonical_json_bytes semantics, and ensure
canonical_payload, to_dict, and the miner injection logic (previously inline in
broadcast_block) are the only touched symbols.
In `@tests/test_serialization.py`:
- Around line 32-44: Add a direct unit test that asserts Block.canonical_payload
is deterministic: call block.canonical_payload (or Block.canonical_payload() if
it's a method) twice on the same Block instance (constructed as in
test_block_hash_consistency with Transaction(sender="A", receiver="B",
amount=10, nonce=5) and difficulty=2) and assert the returned payload values are
identical; also create a second Block with identical fields and assert its
canonical_payload equals the first block's canonical_payload to verify
cross-instance determinism. Ensure you reference Block.canonical_payload and use
the same construction pattern as in test_block_hash_consistency so the new
assertions integrate with existing tests.
- Around line 45-52: The test entrypoint currently catches AssertionError and
only prints a message, which hides failures; update the main guard so assertion
failures propagate (either remove the try/except entirely or re-raise the caught
exception after printing). Locate the block that calls
test_raw_data_determinism(), test_transaction_id_stability(), and
test_block_hash_consistency() under if __name__ == "__main__": and modify it to
let AssertionError bubble up (or call raise after the print) so test runners see
a non-zero exit on failures.
🪄 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: f3d3c1f0-fc6b-457e-8bc4-e8ba4987f6f3
📒 Files selected for processing (2)
minichain/block.pytests/test_serialization.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/p2p.py`:
- Around line 337-342: broadcast_block() now places miner at the top-level of
payload causing receivers that expect miner inside payload["data"] to drop it
and fall back to BURN_ADDRESS; fix by keeping backward compatibility: inject
miner into the existing block_data object (e.g., set block_data["miner"] =
miner) and then set payload = {"type":"block","data": block_data} so receivers
(main.py reading data["data"]["miner"]) keep working, or alternatively update
the receiver to read the top-level "miner" field (adjust the code that processes
incoming payloads in main.py to use payload["miner"] if present). Ensure the
change references the payload construction in broadcast_block, the block_data
variable, and the receiver logic that reads data["data"] so both sides remain
consistent.
- Around line 205-206: _validate_message currently assumes message is a dict and
calls set(message), which raises TypeError for non-dict JSON values; modify
_validate_message to first check isinstance(message, dict) and immediately
return False (and log the invalid message) if not a dict, then proceed to
validate the presence of "type" and "data" keys; update any log in the
listen/handler path that uses json.loads(...) to ensure invalid messages are
logged and ignored rather than crashing the task.
In `@tests/test_serialization.py`:
- Around line 19-29: The tests rely on auto-generated timestamps/signatures so
they only check same-instance stability; update test_transaction_id_stability
and test_block_serialization_determinism to pass explicit deterministic values
(e.g., timestamp and signature where applicable) into the Transaction and Block
constructors (reference: Transaction.__init__/tx_id and
Block.__init__/block_hash) and create two separate instances with identical
inputs, then assert tx1.tx_id == tx2.tx_id (and similarly block1.block_hash ==
block2.block_hash) to verify cross-instance determinism.
🪄 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: bcdb9803-3e58-4df9-ad9c-3f3c939f7187
📒 Files selected for processing (2)
minichain/p2p.pytests/test_serialization.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/p2p.py`:
- Around line 339-349: Do not inject the external "miner" into the deserialized
block dict in p2p.py (the block_data = json.loads(block.canonical_payload...);
block_data["miner"] = miner) because miner must be part of the canonical payload
or derived from the block's coinbase; remove that injection and instead either
(A) ensure Block.to_dict()/Block.canonical_payload includes the miner when the
block is created so miner is inside block.canonical_payload, or (B) validate
that the claimed miner matches a coinbase/first transaction inside block_data
before using it for state changes (and reject or ignore mismatches). Update code
paths that rely on payload.get("miner") (e.g., the reward crediting in main.py)
to only use a miner value that is present inside block.canonical_payload or
derived from the block's transactions.
- Around line 46-50: The start() method currently binds the listener to
"0.0.0.0" which exposes the TCP endpoint on all interfaces; change start(self,
port: int = 9000) to accept a host parameter (e.g. start(self, port: int = 9000,
host: str = "127.0.0.1")) and use that host when calling
asyncio.start_server(self._handle_incoming, host, port), update the logger
message to include the host, and ensure _port/_server behavior remains the same
so operators can explicitly opt into binding to "0.0.0.0" by passing
host="0.0.0.0".
- Around line 304-307: In _broadcast_raw remove the unnecessary json.dumps
serialization: the first assignment to line using json.dumps(payload) is
immediately overwritten by the canonical_json_dumps call, doubling work on the
hot path; delete the json.dumps-based line assignment and keep only the
canonical_json_dumps(payload) + "\n").encode() form so the function serializes
payload once (refer to function _broadcast_raw and the variable line).
🪄 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: 98b8d654-98b9-4374-af5b-73a576e8f442
📒 Files selected for processing (2)
minichain/p2p.pytests/test_serialization.py
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)
68-77:⚠️ Potential issue | 🔴 CriticalIncluding
minerinto_header_dict()breaks backward compatibility with previously persisted blocks.Adding
minerto the hash input changes the hash computation for all blocks. This causes a critical backward compatibility issue:When blocks are loaded from storage (via
Block.from_dict()), theminerfield will beNoneif it's missing from the persisted data. Sinceto_header_dict()now includes"miner": self.miner, the hash recomputation atchain.py:21will produce a different value than what was originally calculated when that block was created without theminerfield in the hash input. This breaks chain validation for any previously saved blocks.If this codebase needs to preserve old blocks, either exclude
minerfrom the hash input or implement a migration strategy for old blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 68 - 77, The change adding "miner" to Block.to_header_dict() alters the header used for hash computation and breaks validation of previously persisted blocks; revert the header shape used for hashing by removing "miner" from to_header_dict() (keep miner only in the full serialization like Block.to_dict() or add a separate to_full_dict()) so Block.from_dict()-loaded blocks still produce the same hash as originally computed in chain.py (where headers are re-hashed), or implement an explicit migration path that re-computes and updates stored block hashes—update the to_header_dict() function to exclude the miner field and ensure any serialization/deserialization code uses the separate full dict that includes miner.
♻️ Duplicate comments (1)
minichain/p2p.py (1)
304-307: 🧹 Nitpick | 🔵 TrivialRemove the redundant
json.dumps()call.Line 306 serializes
payloadwithjson.dumps(), but line 307 immediately overwriteslinewithcanonical_json_dumps(). This doubles CPU work on every broadcast for no effect.♻️ Proposed fix
async def _broadcast_raw(self, payload: dict): """Send a JSON message to every connected peer.""" - line = (json.dumps(payload) + "\n").encode() line = (canonical_json_dumps(payload) + "\n").encode()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/p2p.py` around lines 304 - 307, In _broadcast_raw remove the redundant serialization: delete the initial json.dumps(...) assignment to line so you only call canonical_json_dumps(payload) + "\n" and then .encode(); update the code around async def _broadcast_raw and the local variable line to stop double-serializing the payload and only perform the canonical_json_dumps step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/p2p.py`:
- Around line 336-344: The broadcast_block signature was changed to remove the
miner parameter but main.py still calls await network.broadcast_block(mined,
miner=pk); fix by setting the miner on the block before broadcasting (e.g.,
assign mined.miner = pk just prior to the broadcast call in main.py) or
alternatively restore/accept the miner argument in broadcast_block and use it to
populate the payload (referencing broadcast_block and the payload["miner"]
assignment) so the call site and implementation agree.
---
Outside diff comments:
In `@minichain/block.py`:
- Around line 68-77: The change adding "miner" to Block.to_header_dict() alters
the header used for hash computation and breaks validation of previously
persisted blocks; revert the header shape used for hashing by removing "miner"
from to_header_dict() (keep miner only in the full serialization like
Block.to_dict() or add a separate to_full_dict()) so Block.from_dict()-loaded
blocks still produce the same hash as originally computed in chain.py (where
headers are re-hashed), or implement an explicit migration path that re-computes
and updates stored block hashes—update the to_header_dict() function to exclude
the miner field and ensure any serialization/deserialization code uses the
separate full dict that includes miner.
---
Duplicate comments:
In `@minichain/p2p.py`:
- Around line 304-307: In _broadcast_raw remove the redundant serialization:
delete the initial json.dumps(...) assignment to line so you only call
canonical_json_dumps(payload) + "\n" and then .encode(); update the code around
async def _broadcast_raw and the local variable line to stop double-serializing
the payload and only perform the canonical_json_dumps step.
🪄 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: e90a03ce-66f2-401f-8118-cfabba16f4f3
📒 Files selected for processing (2)
minichain/block.pyminichain/p2p.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 91-96: The to_dict() method in Block currently always sets
"miner": self.miner which causes P2P validation to fail when miner is None;
update Block.to_dict() so it only adds the "miner" key when self.miner is not
None (and ideally only if it's a str) so peers' _validate_block_payload in
p2p.py will accept genesis/pre-mined blocks; locate the to_dict function (method
name: to_dict) and change the logic to conditionally include the miner field
when present.
- Around line 44-49: The constructor's miner parameter is missing a type
annotation; update the Block.__init__ signature to annotate miner (e.g., miner:
Optional[str] = None or the appropriate Miner type) and add the corresponding
attribute type on self.miner (ensure Optional is imported from typing if used)
so miner has an explicit type like self.miner: Optional[str] = miner.
🪄 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: d4347d56-4e52-4289-9529-4e7a0ae5d8b2
📒 Files selected for processing (2)
minichain/block.pyminichain/p2p.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
minichain/p2p.py (1)
205-224:⚠️ Potential issue | 🟠 MajorValidate the duplicated
minerfield, or drop it.
broadcast_block()now emitsminerinsidedataand again at the top level, but_validate_message()only checks that"type"and"data"exist. A peer can therefore send conflictingminervalues and still pass schema validation, which leaves the block envelope ambiguous. Either keep a single source of truth, or reject block messages unless the top-levelmineris astrequal todata.get("miner")and omit it entirely whenblock.minerisNone.🔧 Minimal guard
def _validate_message(self, message): if not isinstance(message, dict): logger.warning("Network: Received non-dict message") return False if not {"type", "data"}.issubset(message): return False msg_type = message.get("type") payload = message.get("data") if msg_type not in SUPPORTED_MESSAGE_TYPES: return False + if msg_type == "block" and "miner" in message: + if not isinstance(message["miner"], str): + return False + if not isinstance(payload, dict) or message["miner"] != payload.get("miner"): + return False validators = { "sync": self._validate_sync_payload, "tx": self._validate_transaction_payload, "block": self._validate_block_payload, } return validators[msg_type](payload)- payload["miner"] = block.miner + if block.miner is not None: + payload["miner"] = block.minerAlso applies to: 335-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/p2p.py` around lines 205 - 224, The block messages currently allow a duplicated conflicting "miner" (top-level and inside data) so update validation and emission: in _validate_message() (and/or _validate_block_payload()) enforce that for msg_type "block" the top-level "miner" must either be absent when data.get("miner") is None, or must be a str equal to data.get("miner") when present; reject the message otherwise, and ensure SUPPORTED_MESSAGE_TYPES logic still routes to validators; also change broadcast_block() to avoid emitting a top-level "miner" when block.miner is None (or ensure it sets both places consistently) so there is a single source of truth.
♻️ Duplicate comments (1)
minichain/block.py (1)
91-100:⚠️ Potential issue | 🟠 Major
minercan still change without changing the block ID.
to_dict()/canonical_payloadnow putmineron the wire andfrom_dict()accepts it back, butcompute_hash()still hashes onlyto_header_dict(), which excludesminer. That means two different canonical block payloads can share the samehash;minichain/p2p.pythen treatspayload["hash"]as the block identity, so a relay can rewrite the miner without invalidating the block ID. Please either bindminerinto the hashed contents or derive it from a transaction instead of a mutable side field.Also applies to: 120-120, 128-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 91 - 100, The block identity can be altered by changing the mutable miner field because compute_hash() hashes only to_header_dict() (which excludes miner) while to_dict()/canonical_payload include miner; fix by binding miner into the hashed contents: update compute_hash() to include miner (e.g., have compute_hash() hash a header that includes self.miner or simply hash the same payload returned by to_dict()/to_header_dict() after adding miner) and make corresponding adjustments to to_header_dict() so it includes miner when present, ensuring canonical_payload, to_dict(), from_dict(), and compute_hash() all agree on the exact set of fields used for the block hash (alternatively, if miner must be derived, move miner into a transaction and remove the separate miner field so compute_hash()/to_header_dict() derive it from transactions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@minichain/p2p.py`:
- Around line 205-224: The block messages currently allow a duplicated
conflicting "miner" (top-level and inside data) so update validation and
emission: in _validate_message() (and/or _validate_block_payload()) enforce that
for msg_type "block" the top-level "miner" must either be absent when
data.get("miner") is None, or must be a str equal to data.get("miner") when
present; reject the message otherwise, and ensure SUPPORTED_MESSAGE_TYPES logic
still routes to validators; also change broadcast_block() to avoid emitting a
top-level "miner" when block.miner is None (or ensure it sets both places
consistently) so there is a single source of truth.
---
Duplicate comments:
In `@minichain/block.py`:
- Around line 91-100: The block identity can be altered by changing the mutable
miner field because compute_hash() hashes only to_header_dict() (which excludes
miner) while to_dict()/canonical_payload include miner; fix by binding miner
into the hashed contents: update compute_hash() to include miner (e.g., have
compute_hash() hash a header that includes self.miner or simply hash the same
payload returned by to_dict()/to_header_dict() after adding miner) and make
corresponding adjustments to to_header_dict() so it includes miner when present,
ensuring canonical_payload, to_dict(), from_dict(), and compute_hash() all agree
on the exact set of fields used for the block hash (alternatively, if miner must
be derived, move miner into a transaction and remove the separate miner field so
compute_hash()/to_header_dict() derive it from transactions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74ae8c05-d8e5-43ee-afa1-5514d9c06be0
📒 Files selected for processing (2)
minichain/block.pyminichain/p2p.py
|
Hi! All automated tests have passed and CodeRabbit review is fully approved ✅ |
|
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract.py`:
- Line 9: The assignment uses self.timeout at module scope which raises
NameError; either make it a module-level constant (e.g., TIMEOUT = 15) or move
the assignment into the appropriate class constructor/method (set self.timeout =
15 inside the class's __init__ or the relevant initializer). Locate the stray
self.timeout assignment in contract.py and replace it with a module constant or
relocate it into the class (e.g., inside __init__ of the class that should own
timeout) so self is only referenced on an instance.
In `@minichain/contract.py`:
- Around line 1-13: This module has been turned into a resource-limit stub but
must still define and export the ContractMachine class used elsewhere; restore a
ContractMachine class (with the same constructor and public methods expected by
minichain/__init__.py and minichain/state.py — the call sites at lines
referencing ContractMachine construction and calls must continue to work) and
ensure it is exposed (either via a top-level class named ContractMachine and/or
added to __all__), while you may keep the resource.setrlimit and multiprocessing
changes but do not remove or rename ContractMachine; make sure the restored
class implements the same API signatures used by state.py (constructors and
methods invoked at those call sites) so imports and contract execution succeed.
- Around line 9-13: The example under the if __name__ == "__main__" block is
broken: import the multiprocessing module, provide or reference a real callable
instead of the undefined target_function (or replace it with a documented
example like worker_task), and replace the incorrect p.join(...) call with the
AsyncResult API (use p.wait(timeout=15) and/or p.get(timeout=15)) so the
Pool.apply_async result is handled correctly; update the block to call
multiprocessing.Pool(...) and use the AsyncResult.wait/get methods on the
returned object.
- Line 5: resource.setrlimit(resource.RLIMIT_CPU, (15, 15)) is being executed at
module import in minichain.contract and must be removed from the top-level;
instead, create a child-process initializer function (e.g., def
init_child_process(): resource.setrlimit(resource.RLIMIT_CPU, (15, 15))) and
call that initializer when spawning worker processes (for example via
multiprocessing.Pool(initializer=init_child_process) or when forking a
subprocess), and remove the top-level setrlimit call so the parent process is
not globally limited.
In `@minichain/p2p.py`:
- Around line 339-349: The broadcast_block function currently mutates
block.miner just before serializing block.canonical_payload; remove that
transport-layer mutation and make broadcast_block read-only: stop assigning
block.miner inside broadcast_block (remove the "if miner: block.miner = miner"
behavior) and instead validate that the Block already has a populated miner
before serializing (in broadcast_block) — if miner is missing, either raise an
error or log and return/ignore the broadcast; update callers that previously
passed miner to ensure they construct/populate Block.miner prior to calling
broadcast_block. Ensure references to broadcast_block, block.miner, and
block.canonical_payload are used in the validation/error path so it's clear
where the check happens.
🪄 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: 47c23049-0e53-4210-9d4d-bef83ee05bec
📒 Files selected for processing (3)
contract.pyminichain/contract.pyminichain/p2p.py
contract.py
Outdated
| # ... (other code) | ||
|
|
||
| # line 103 -- Update timeout | ||
| self.timeout = 15 # Updated from 2 to 15 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "contract.py" -type fRepository: StabilityNexus/MiniChain
Length of output: 105
🏁 Script executed:
cat -n ./contract.pyRepository: StabilityNexus/MiniChain
Length of output: 356
🏁 Script executed:
cat -n ./minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 524
🏁 Script executed:
wc -l ./contract.pyRepository: StabilityNexus/MiniChain
Length of output: 84
🏁 Script executed:
head -20 ./contract.pyRepository: StabilityNexus/MiniChain
Length of output: 279
This assignment will raise NameError on import.
Line 9 assigns self.timeout at module scope, but self is undefined at module scope. This will raise NameError: name 'self' is not defined when importing contract.py. Move this assignment into a class method or convert it to a module-level constant without self.
🧰 Tools
🪛 Ruff (0.15.7)
[error] 9-9: Undefined name self
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract.py` at line 9, The assignment uses self.timeout at module scope
which raises NameError; either make it a module-level constant (e.g., TIMEOUT =
15) or move the assignment into the appropriate class constructor/method (set
self.timeout = 15 inside the class's __init__ or the relevant initializer).
Locate the stray self.timeout assignment in contract.py and replace it with a
module constant or relocate it into the class (e.g., inside __init__ of the
class that should own timeout) so self is only referenced on an instance.
minichain/contract.py
Outdated
| import resource | ||
|
|
||
| import json # Moved to module-level import | ||
| logger = logging.getLogger(__name__) | ||
| # ... other code ... | ||
|
|
||
| def _safe_exec_worker(code, globals_dict, context_dict, result_queue): | ||
| """ | ||
| Worker function to execute contract code in a separate process. | ||
| """ | ||
| try: | ||
| # Attempt to set resource limits (Unix only) | ||
| try: | ||
| import resource | ||
| # Limit CPU time (seconds) and memory (bytes) - example values | ||
| resource.setrlimit(resource.RLIMIT_CPU, (2, 2)) # Align with p.join timeout (2 seconds) | ||
| resource.setrlimit(resource.RLIMIT_AS, (100 * 1024 * 1024, 100 * 1024 * 1024)) | ||
| except ImportError: | ||
| logger.warning("Resource module not available. Contract will run without OS-level resource limits.") | ||
| except (OSError, ValueError) as e: | ||
| logger.warning("Failed to set resource limits: %s", e) | ||
| resource.setrlimit(resource.RLIMIT_CPU, (15, 15)) # Changed from (2, 2) to (15, 15) | ||
|
|
||
| exec(code, globals_dict, context_dict) | ||
| # Return the updated storage | ||
| result_queue.put({"status": "success", "storage": context_dict.get("storage")}) | ||
| except Exception as e: | ||
| result_queue.put({"status": "error", "error": str(e)}) | ||
| # ... other code ... | ||
|
|
||
| class ContractMachine: | ||
| """ | ||
| A minimal execution environment for Python-based smart contracts. | ||
| WARNING: Still not production-safe. For educational use only. | ||
| """ | ||
|
|
||
| def __init__(self, state): | ||
| self.state = state | ||
|
|
||
| def execute(self, contract_address, sender_address, payload, amount): | ||
| """ | ||
| Executes the contract code associated with the contract_address. | ||
| """ | ||
|
|
||
| account = self.state.get_account(contract_address) | ||
| if not account: | ||
| return False | ||
|
|
||
| code = account.get("code") | ||
|
|
||
| # Defensive copy of storage to prevent direct mutation | ||
| storage = dict(account.get("storage", {})) | ||
|
|
||
| if not code: | ||
| return False | ||
|
|
||
| # AST Validation to prevent introspection | ||
| if not self._validate_code_ast(code): | ||
| return False | ||
|
|
||
| # Restricted builtins (explicit allowlist) | ||
| safe_builtins = { | ||
| "True": True, | ||
| "False": False, | ||
| "None": None, | ||
| "range": range, | ||
| "len": len, | ||
| "min": min, | ||
| "max": max, | ||
| "abs": abs, | ||
| "str": str, # Keeping str for basic functionality, relying on AST checks for safety | ||
| "bool": bool, | ||
| "float": float, | ||
| "list": list, | ||
| "dict": dict, | ||
| "tuple": tuple, | ||
| "sum": sum, | ||
| "Exception": Exception, # Added to allow contracts to raise exceptions | ||
| } | ||
|
|
||
| globals_for_exec = { | ||
| "__builtins__": safe_builtins | ||
| } | ||
|
|
||
| # Execution context (locals) | ||
| context = { | ||
| "storage": storage, | ||
| "msg": { | ||
| "sender": sender_address, | ||
| "value": amount, | ||
| "data": payload, | ||
| }, | ||
| # "print": print, # Removed for security | ||
| } | ||
|
|
||
| try: | ||
| # Execute in a subprocess with timeout | ||
| queue = multiprocessing.Queue() | ||
| p = multiprocessing.Process( | ||
| target=_safe_exec_worker, | ||
| args=(code, globals_for_exec, context, queue) | ||
| ) | ||
| p.start() | ||
| p.join(timeout=2) # 2 second timeout | ||
|
|
||
| if p.is_alive(): | ||
| p.kill() | ||
| p.join() | ||
| logger.error("Contract execution timed out") | ||
| return False | ||
|
|
||
| try: | ||
| result = queue.get(timeout=1) | ||
| except Exception: | ||
| logger.error("Contract execution crashed without result") | ||
| return False | ||
| if result["status"] != "success": | ||
| logger.error(f"Contract Execution Failed: {result.get('error')}") | ||
| return False | ||
|
|
||
| # Validate storage is JSON serializable | ||
| try: | ||
| json.dumps(result["storage"]) | ||
| except (TypeError, ValueError): | ||
| logger.error("Contract storage not JSON serializable") | ||
| return False | ||
|
|
||
| # Commit updated storage only after successful execution | ||
| self.state.update_contract_storage( | ||
| contract_address, | ||
| result["storage"] | ||
| ) | ||
|
|
||
| return True | ||
|
|
||
| except Exception as e: | ||
| logger.error("Contract Execution Failed", exc_info=True) | ||
| return False | ||
|
|
||
| def _validate_code_ast(self, code): | ||
| """Reject code that uses double underscores or introspection.""" | ||
| try: | ||
| tree = ast.parse(code) | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.Attribute) and node.attr.startswith("__"): | ||
| logger.warning("Rejected contract code with double-underscore attribute access.") | ||
| return False | ||
| if isinstance(node, ast.Name) and node.id.startswith("__"): | ||
| logger.warning("Rejected contract code with double-underscore name.") | ||
| return False | ||
| if isinstance(node, (ast.Import, ast.ImportFrom)): | ||
| logger.warning("Rejected contract code with import statement.") | ||
| return False | ||
| if isinstance(node, ast.Call): | ||
| if isinstance(node.func, ast.Name) and node.func.id == 'type': | ||
| logger.warning("Rejected type() call.") | ||
| return False | ||
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id in {"getattr", "setattr", "delattr"}: | ||
| logger.warning(f"Rejected direct call to {node.func.id}.") | ||
| return False | ||
| if isinstance(node, ast.Constant) and isinstance(node.value, str): | ||
| if "__" in node.value: | ||
| logger.warning("Rejected string literal with double-underscore.") | ||
| return False | ||
| if isinstance(node, ast.JoinedStr): # f-strings | ||
| logger.warning("Rejected f-string usage.") | ||
| return False | ||
| return True | ||
| except SyntaxError: | ||
| return False | ||
| if __name__ == "__main__": | ||
| # Process Pool example | ||
| with multiprocessing.Pool(processes=4) as pool: | ||
| p = pool.apply_async(target_function) | ||
| p.join(timeout=15) # Changed from timeout=2 to timeout=15 |
There was a problem hiding this comment.
This module still needs to export ContractMachine.
minichain/__init__.py Line 6 still imports ContractMachine, and minichain/state.py Lines 14, 50, and 112 still construct/call it. Replacing this module with a resource-limit stub makes package import and contract execution fail before any serialization code runs.
🧰 Tools
🪛 Ruff (0.15.7)
[error] 11-11: Undefined name multiprocessing
(F821)
[error] 12-12: Undefined name target_function
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 1 - 13, This module has been turned into
a resource-limit stub but must still define and export the ContractMachine class
used elsewhere; restore a ContractMachine class (with the same constructor and
public methods expected by minichain/__init__.py and minichain/state.py — the
call sites at lines referencing ContractMachine construction and calls must
continue to work) and ensure it is exposed (either via a top-level class named
ContractMachine and/or added to __all__), while you may keep the
resource.setrlimit and multiprocessing changes but do not remove or rename
ContractMachine; make sure the restored class implements the same API signatures
used by state.py (constructors and methods invoked at those call sites) so
imports and contract execution succeed.
minichain/contract.py
Outdated
| logger.warning("Resource module not available. Contract will run without OS-level resource limits.") | ||
| except (OSError, ValueError) as e: | ||
| logger.warning("Failed to set resource limits: %s", e) | ||
| resource.setrlimit(resource.RLIMIT_CPU, (15, 15)) # Changed from (2, 2) to (15, 15) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "contract.py" -type f | head -20Repository: StabilityNexus/MiniChain
Length of output: 105
🏁 Script executed:
head -30 minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 433
🏁 Script executed:
cat minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 433
🏁 Script executed:
wc -l minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 92
🏁 Script executed:
cat -n minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 524
🏁 Script executed:
rg "from minichain" | head -20Repository: StabilityNexus/MiniChain
Length of output: 1249
🏁 Script executed:
rg "import.*contract|from.*contract" . --type pyRepository: StabilityNexus/MiniChain
Length of output: 188
Move RLIMIT_CPU limit to child process initialization, not module top-level.
resource.setrlimit(resource.RLIMIT_CPU, (15, 15)) is process-global and executes at import time (line 5). This caps the entire parent process to 15 CPU seconds when minichain.contract is imported—which occurs in minichain/__init__.py and minichain/state.py, affecting any code importing the library. The CPU limit belongs in isolated child process initialization (e.g., multiprocessing pool initializer), not module-level code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` at line 5, resource.setrlimit(resource.RLIMIT_CPU,
(15, 15)) is being executed at module import in minichain.contract and must be
removed from the top-level; instead, create a child-process initializer function
(e.g., def init_child_process(): resource.setrlimit(resource.RLIMIT_CPU, (15,
15))) and call that initializer when spawning worker processes (for example via
multiprocessing.Pool(initializer=init_child_process) or when forking a
subprocess), and remove the top-level setrlimit call so the parent process is
not globally limited.
minichain/contract.py
Outdated
| if __name__ == "__main__": | ||
| # Process Pool example | ||
| with multiprocessing.Pool(processes=4) as pool: | ||
| p = pool.apply_async(target_function) | ||
| p.join(timeout=15) # Changed from timeout=2 to timeout=15 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "contract.py" -type fRepository: StabilityNexus/MiniChain
Length of output: 105
🏁 Script executed:
head -30 ./minichain/contract.py | cat -nRepository: StabilityNexus/MiniChain
Length of output: 524
🏁 Script executed:
cat -n ./minichain/contract.pyRepository: StabilityNexus/MiniChain
Length of output: 524
🌐 Web query:
multiprocessing.pool.AsyncResult join method python documentation
💡 Result:
The multiprocessing.pool.AsyncResult.join method does not exist. The AsyncResult class, returned by asynchronous Pool methods like apply_async, map_async, and starmap_async, provides methods such as get([timeout]) to block and retrieve the result, wait([timeout]) to block until completion, ready to check if ready without blocking, and successful to check if completed without exception (after ready). There is no join method on AsyncResult objects. To wait for completion, use result.wait or result.get. The Pool object itself has join to wait for worker processes after close. Official Python documentation confirms AsyncResult methods are get([timeout]) [1,2,6,8]. Source code in Lib/multiprocessing/pool.py shows subclasses like ApplyResult and MapResult inherit from a base with get and wait, but no join [19,20]. Secondary sources describe ready, successful, wait, get but no join [13,14,15]. join appears in Process and Pool contexts, not AsyncResult [7].
Citations:
- 1: https://docs.python.org/3.13/library/multiprocessing.html
- 2: https://docs.python.org/3/library/multiprocessing.html
- 3: https://docs.python.org/3.15/library/multiprocessing.html
- 4: https://docs.python.org/library/multiprocessing
- 5: https://github.com/python/cpython/blob/main/Lib/multiprocessing/pool.py
- 6: https://github.com/python/cpython/blob/3.12/Lib/multiprocessing/pool.py
- 7: https://runebook.dev/en/docs/python/library/multiprocessing/multiprocessing.pool.AsyncResult.ready
- 8: https://runebook.dev/en/articles/python/library/multiprocessing/multiprocessing.pool.AsyncResult
- 9: https://superfastpython.com/multiprocessing-pool-asyncresult/
- 10: https://stackoverflow.com/questions/25391025/what-exactly-is-python-multiprocessing-modules-join-method-doing
The __main__ example will fail immediately.
Line 11 references multiprocessing but it is not imported. Line 12 references target_function but it is not defined. Additionally, pool.apply_async() returns an AsyncResult object which supports wait() and get() methods, not join().
🧰 Tools
🪛 Ruff (0.15.7)
[error] 11-11: Undefined name multiprocessing
(F821)
[error] 12-12: Undefined name target_function
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 9 - 13, The example under the if __name__
== "__main__" block is broken: import the multiprocessing module, provide or
reference a real callable instead of the undefined target_function (or replace
it with a documented example like worker_task), and replace the incorrect
p.join(...) call with the AsyncResult API (use p.wait(timeout=15) and/or
p.get(timeout=15)) so the Pool.apply_async result is handled correctly; update
the block to call multiprocessing.Pool(...) and use the AsyncResult.wait/get
methods on the returned object.
|
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: |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_serialization.py (1)
62-78:⚠️ Potential issue | 🟡 MinorRemove the duplicate Merkle tamper case and leftover
assert False.Line 67 still uses
assert False, which can be skipped withpython -O, and the same Merkle tamper path is tested twice in this section.🧪 Proposed fix
- # Test tampered Merkle Root - bad_merkle = block.to_dict() - bad_merkle["merkle_root"] = "f" * 64 - try: - Block.from_dict(bad_merkle) - assert False, "Expected ValueError for tampered merkle_root" - except ValueError: - pass - # Test tampered Merkle Root bad_merkle = block.to_dict() bad_merkle["merkle_root"] = "f" * 64 try: Block.from_dict(bad_merkle) raise AssertionError("Expected ValueError for tampered merkle_root") # <-- CHANGED except ValueError: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_serialization.py` around lines 62 - 78, Remove the duplicated "tampered Merkle Root" test block so the tamper path is only tested once; locate the two identical blocks that call block.to_dict() and then Block.from_dict(...) and delete one of them. In the remaining test replace the fragile `assert False` with an explicit failure (e.g., raise AssertionError("Expected ValueError for tampered merkle_root")) or, better, convert the case to use pytest.raises(ValueError) around Block.from_dict(...) to make the assertion robust. Ensure references to block.to_dict() and Block.from_dict(...) remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 92-96: Block.to_dict currently rebuilds the body by serializing
transactions inline which risks header/body drift; modify to_dict to reuse the
existing to_body_dict() for the body (e.g., call self.to_body_dict() and merge
with self.to_header_dict()), ensure "transactions" (or whatever keys
to_body_dict returns) are taken from to_body_dict, preserve the "hash" field as
before, and remove the inline list-comprehension to avoid duplicated
serialization logic.
---
Duplicate comments:
In `@tests/test_serialization.py`:
- Around line 62-78: Remove the duplicated "tampered Merkle Root" test block so
the tamper path is only tested once; locate the two identical blocks that call
block.to_dict() and then Block.from_dict(...) and delete one of them. In the
remaining test replace the fragile `assert False` with an explicit failure
(e.g., raise AssertionError("Expected ValueError for tampered merkle_root")) or,
better, convert the case to use pytest.raises(ValueError) around
Block.from_dict(...) to make the assertion robust. Ensure references to
block.to_dict() and Block.from_dict(...) remain intact.
🪄 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: f71196bd-6da1-4e76-8714-d9e06f9e4466
📒 Files selected for processing (2)
minichain/block.pytests/test_serialization.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
68-77:⚠️ Potential issue | 🔴 CriticalPreserve hash compatibility for blocks that predate
miner.
compute_hash()now canonicalizes a header that always containsminer, so blocks serialized before this field existed will fail the new hash check inBlock.from_dict().minichain/persistence.pyroutes every persisted block through that path, which means an upgrade can no longer reopen an existing chain. Either omitminerfrom the hashed header when it is unset, or add an explicit versioned/legacy-hash migration before enforcing the comparison.Possible compatibility-preserving change
def to_header_dict(self): - return { + header = { "index": self.index, "previous_hash": self.previous_hash, "merkle_root": self.merkle_root, "timestamp": self.timestamp, "difficulty": self.difficulty, "nonce": self.nonce, - "miner": self.miner, } + if self.miner is not None: + header["miner"] = self.miner + return header🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 68 - 77, The current to_header_dict() always includes "miner", which breaks compute_hash() canonicalization for blocks created before miner existed; update to_header_dict() to omit the "miner" key when the Block instance has no miner (e.g., is None or empty) so compute_hash() and Block.from_dict() compute/compare legacy hashes correctly, and ensure persistence.py continues to load old blocks by routing them through Block.from_dict() without forcing a new miner key; alternatively implement a small migration in Block.from_dict() that recognizes legacy blocks (no miner) and compares using the legacy header format before enforcing the new header format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 131-134: The canonical_payload property currently serializes the
live self.transactions list which can be mutated after construction, causing
header/body mismatches; fix this by freezing transactions in the Block
constructor (e.g., copy and convert the incoming transactions list to an
immutable sequence like a tuple and store that instead of keeping the caller's
list reference) and ensure any hash/commitment calculations (the code that
computes block header/hash) run against this frozen transactions value so the
block is treated as immutable (you can also set an internal flag such as _frozen
after hashing); update to_dict()/canonical_payload/canonical_json_bytes usage to
serialize the frozen transactions so Block.from_dict() will accept the canonical
bytes produced.
---
Outside diff comments:
In `@minichain/block.py`:
- Around line 68-77: The current to_header_dict() always includes "miner", which
breaks compute_hash() canonicalization for blocks created before miner existed;
update to_header_dict() to omit the "miner" key when the Block instance has no
miner (e.g., is None or empty) so compute_hash() and Block.from_dict()
compute/compare legacy hashes correctly, and ensure persistence.py continues to
load old blocks by routing them through Block.from_dict() without forcing a new
miner key; alternatively implement a small migration in Block.from_dict() that
recognizes legacy blocks (no miner) and compares using the legacy header format
before enforcing the new header format.
🪄 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: 7b40a5b4-2283-4c55-a1e3-7c7a73959655
📒 Files selected for processing (2)
minichain/block.pytests/test_serialization.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
minichain/block.py (1)
137-140:⚠️ Potential issue | 🟠 MajorReject invalid blocks before network serialization.
canonical_payloadstill emits bytes even whenself.hashis missing/stale or the cachedmerkle_rootno longer matches the body. That lets this node originate payloads thatBlock.from_dict()andminichain/p2p.py:176-200will reject immediately.🛠️ Proposed fix
`@property` def canonical_payload(self) -> bytes: """Returns the full block (header + body) as canonical bytes for networking.""" + if self.hash is None: + raise ValueError("block hash is missing") + if self.hash != self.compute_hash(): + raise ValueError("block hash does not match header") + if _calculate_merkle_root(self.transactions) != self.merkle_root: + raise ValueError("merkle_root does not match transactions") return canonical_json_bytes(self.to_dict())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 137 - 140, Before serializing in canonical_payload, validate the block invariants: ensure self.hash exists and matches the canonical header hash (recompute via a compute_hash()/recalculate_hash() helper and compare) and ensure self.merkle_root matches a recomputed merkle root of the body (via compute_merkle_root()/recalculate_merkle()). If either check fails, do not emit bytes — raise a ValueError (or similar) so invalid/stale blocks are rejected locally instead of being serialized; only call return canonical_json_bytes(self.to_dict()) after those validations pass. Include references to canonical_payload, self.hash, merkle_root, to_dict(), and Block.from_dict in your change so reviewers can see the invariant checks that prevent p2p rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@minichain/block.py`:
- Around line 137-140: Before serializing in canonical_payload, validate the
block invariants: ensure self.hash exists and matches the canonical header hash
(recompute via a compute_hash()/recalculate_hash() helper and compare) and
ensure self.merkle_root matches a recomputed merkle root of the body (via
compute_merkle_root()/recalculate_merkle()). If either check fails, do not emit
bytes — raise a ValueError (or similar) so invalid/stale blocks are rejected
locally instead of being serialized; only call return
canonical_json_bytes(self.to_dict()) after those validations pass. Include
references to canonical_payload, self.hash, merkle_root, to_dict(), and
Block.from_dict in your change so reviewers can see the invariant checks that
prevent p2p rejections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02e6a2a1-09eb-4c64-8850-668e061cb1cc
📒 Files selected for processing (1)
minichain/block.py
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 (1)
minichain/block.py (1)
116-130:⚠️ Potential issue | 🟠 MajorCanonicalize integral header fields in deserialization to ensure consistent hashing across peers.
In the
from_dictmethod (lines 116-130),timestampis normalized tointvia the constructor (line 54), butindex,difficulty, andnonceare not. When deserializing from JSON, if any of these fields arrive as floats (e.g.,7.0instead of7), they remain as floats and propagate toto_header_dict(). Sincecompute_hash()relies on Python'sjson.dumps()number rendering,{"nonce": 7}and{"nonce": 7.0}produce different canonical bytes, causing hash mismatches between peers receiving semantically equivalent blocks.Normalize
index,difficulty, andnonceto their integral types immediately infrom_dictbefore any subsequent operations:block.nonce = int(payload.get("nonce", 0))Also ensure
difficultyis normalized in the constructor call andindexis coerced tointif needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 116 - 130, from_dict currently leaves index, difficulty and nonce as floats when payload contains floats which changes JSON canonicalization used by compute_hash/to_header_dict; update from_dict (and the Block constructor call there) to coerce index and difficulty to int (e.g., int(payload["index"]), int(payload.get("difficulty", 0))) and set block.nonce = int(payload.get("nonce", 0)) before computing expected_hash so all integral header fields are canonicalized for consistent hashing across peers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@minichain/block.py`:
- Around line 116-130: from_dict currently leaves index, difficulty and nonce as
floats when payload contains floats which changes JSON canonicalization used by
compute_hash/to_header_dict; update from_dict (and the Block constructor call
there) to coerce index and difficulty to int (e.g., int(payload["index"]),
int(payload.get("difficulty", 0))) and set block.nonce =
int(payload.get("nonce", 0)) before computing expected_hash so all integral
header fields are canonicalized for consistent hashing across peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ab4ac34-5b6e-4a24-aff6-7414ced5847d
📒 Files selected for processing (1)
minichain/block.py
… canonical hashing
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 80-82: Update the misleading comment above the conditional that
adds the miner to the header: instead of "Backward compatibility: Only include
miner in hash if it exists", clarify that this handles optional fields by
writing something like "Include miner in header only if present (handles
optional miner field)"; locate the conditional referencing self.miner and header
(the block where if self.miner is not None: header['miner'] = self.miner) and
replace the comment accordingly.
- Line 5: Replace legacy typing aliases with built-in generics: change imports
and annotations that use typing.List and typing.Optional to use the built-in
list and Optional or remove Optional in favor of union syntax; specifically
update the import line removing List and replace occurrences like
List[Transaction] and Optional[List[Transaction]] with list[Transaction] and
Optional[list[Transaction]] (and update the declaration transactions:
Optional[list[Transaction]] = None), ensuring any other references to
List[Transaction] in functions or class attributes (e.g., in class Block or
methods handling transactions) are updated accordingly.
- Around line 140-150: The defensive merkle root recalculation in the
canonical_payload property is redundant and costly because transactions are
frozen at construction and merkle_root is computed there; update
canonical_payload (method canonical_payload) to avoid recomputing
_calculate_merkle_root(self.transactions) on every access — either remove that
check entirely (trust the constructor-computed merkle_root) or add a one-time
validation flag set during __init__ (or where merkle_root is computed) and have
canonical_payload consult that cached flag instead of recomputing; ensure you
keep the existing hash checks (self.hash and self.compute_hash()) unchanged.
- Around line 115-138: The timestamp from payload is passed to cls(...) without
explicit casting which can raise an unhelpful ValueError; before constructing
the block (in the from_payload flow that sets raw_diff/parsed_diff and
block.nonce), parse and validate payload.get("timestamp") into an int (e.g.,
parsed_ts = int(raw_ts) if raw_ts is not None else None) and pass parsed_ts into
cls(...) so Block.timestamp is consistently an int and failures produce a
clearer conversion error; reference the class constructor call (cls(...)), the
local variable block, and ensure compute_hash()/merkle_root validations remain
unchanged.
🪄 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: 69cc9480-c087-4d07-a9a4-55339a01eee7
📒 Files selected for processing (1)
minichain/block.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/block.py`:
- Around line 12-13: The type annotation for _calculate_merkle_root is wrong for
the actual usage: it receives self.transactions (a tuple) but is typed as
list[Transaction]; update the function signature for _calculate_merkle_root to
accept the correct collection type (either use
collections.abc.Sequence[Transaction] or tuple[Transaction, ...]) and update the
import/type hints accordingly so static type checkers accept passing
self.transactions; keep the parameter name transactions and ensure any internal
code that relied on list-specific methods is adjusted to work with a generic
Sequence or tuple.
🪄 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: d425c335-1f48-4cd2-8c72-fad408f586b6
📒 Files selected for processing (1)
minichain/block.py
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review All previous feedback addressed. Latest CodeRabbit review has no actionable comments and approves the changes. All checks green, no conflicts, ready for merge ✅ |
|
The PR description image shows a PowerShell terminal running
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve Latest review has zero actionable comments and approves everything. All checks green, no conflicts, cleanly mergeable. Ready for human review & merge! |
|
Tip For best results, initiate chat on the files or code changes. The PR description image shows the PowerShell terminal running
[resolve] |
1 participant
Addressed Issues:
Closes #2
Screenshots/Recordings:
Additional Notes:
This PR implements a strict, deterministic serialization format to ensure data integrity across the network.
Added a canonical_payload property to the Block class to support full block transmission for networking.
Ensured that all signing and hashing operations in Transaction and Block use sorted keys and eliminated whitespace.
This work is a prerequisite for my GSoC 2026 proposal, which relies on immutable data structures for automated portfolio management.
AI Usage Disclosure:
[ ] This PR does not contain AI-generated code at all.
[x] This PR contains AI-generated code. I have read the AI Usage Policy and this PR complies with this policy. I have tested the code locally and I am responsible for it.
I have used Gemini Thinking model.
Checklist
[x] My PR addresses a single issue, fixes a single bug or makes a single improvement.
[x] My code follows the project's code style and conventions.
[x] If applicable, I have made corresponding changes or additions to the documentation.
[x] If applicable, I have made corresponding changes or additions to tests.
[x] My changes generate no new warnings or errors.
[x] I have joined the Discord server and I will share a link to this PR with the project maintainers there.
[x] I have read the Contribution Guidelines.
[x] Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
[x] I have filled this PR template completely and carefully, and I understand that my PR may be closed without review otherwise.
Summary by CodeRabbit
Tests
Refactor
Networking
UX