[FEATURE]: Implement Canonical Serialization for Blocks and Transactions#67
[FEATURE]: Implement Canonical Serialization for Blocks and Transactions#67sanaica wants to merge 33 commits into
Conversation
WalkthroughAdds canonical JSON payloads and a ChangesBlock serialization & validation
P2P broadcast & message validation
CLI mining integration
Determinism tests
Sequence Diagram(s)sequenceDiagram
participant Creator as Creator
participant Block as Block
participant Canonical as canonical_json_bytes / canonical_json_hash
participant P2P as P2PNetwork
participant Peer as Remote Peer
Creator->>Block: create Block(index, previous_hash, transactions, miner)
Block->>Canonical: compute header hash (canonical_json_hash(header))
Canonical-->>Block: header hash
Block->>Canonical: canonical_payload = canonical_json_bytes(Block.to_dict())
Canonical-->>Block: canonical payload bytes
P2P->>Block: call block.to_dict()
P2P->>Canonical: canonical_json_dumps({"type":"block","data":block.to_dict()})
Canonical-->>P2P: wire bytes + "\n"
P2P->>Peer: send wire bytes (concurrent write/drain)
Peer->>P2P: receive -> _validate_message (ensure dict + schema)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🤖 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
| # ... (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.
| 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.
|
|
@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] |
SIDDHANTCOOKIE
left a comment
SIDDHANTCOOKIE
left a comment
There was a problem hiding this comment.
@Zahnentferner @sanaica few changes are needed then this is good actually fixes a lot of security issues!
| await self._broadcast_raw(payload) | ||
|
|
||
| async def broadcast_block(self, block, miner=None): | ||
| async def broadcast_block(self, block): |
There was a problem hiding this comment.
You are doing json.loads(block.canonical_payload.decode("utf-8")).
Here is what happens under the hood:
- canonical_payload serializes the block into JSON bytes.
- .decode() turns it into a string.
- json.loads() parses that string back into a Python dictionary.
- You pass that dictionary to _broadcast_raw(), which re-serializes the whole thing back into bytes using canonical_json_dumps.
- For a block with thousands of transactions, this double-serialization is very expensive. Since _broadcast_raw already enforces strict, deterministic JSON serialization on the entire payload, you can skip the intermediate step!
I'll say Just pass the dictionary directly.
|
|
||
|
|
||
| class Block: | ||
| def __init__( |
There was a problem hiding this comment.
I like that you are freezing the self.transactions into an immutable tuple, that is an excellent security measure to prevent accidental modifications!
However, the constructor signature still strictly type-hints transactions: Optional[list[Transaction]]. If a developer passes a tuple of transactions into the constructor, a static type checker (like mypy) will flag it as an error. Update the type hint to Sequence to perfectly match your use of tuples
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 326-336: Unpack the peer tuple using an underscore-prefixed name
for the unused reader to signal it's intentionally unused; in the loop over
results (where you currently do "reader, writer = pair"), change that unpacking
to "_reader, writer = pair" so the unused variable follows convention while the
rest of the logic (closing writer, awaiting wait_closed, and removing pair from
self._peers) remains 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: 02bdf606-9166-4f51-986f-e473c3d0254a
📒 Files selected for processing (2)
minichain/block.pyminichain/p2p.py
|
@SIDDHANTCOOKIE Done, Let me know if there's anything more you would like to change. Thank you!! |
| index=chain.last_block.index + 1, | ||
| previous_hash=chain.last_block.hash, | ||
| transactions=mineable_txs, | ||
| index=chain.last_block.index + 1, |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main.py (1)
80-85:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFinish the receive-side miner migration.
This change embeds
minerinto the block, but the block handler still creditspayload.get("miner", BURN_ADDRESS)on Line 152. Relayed blocks will therefore burn the mining reward on remote nodes instead of payingblock.miner, so peer state still diverges.Suggested fix
- miner = payload.get("miner", BURN_ADDRESS) + miner = getattr(block, "miner", None) or BURN_ADDRESS chain.state.credit_mining_reward(miner)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 80 - 85, The receive-side handler still credits mining rewards from the raw payload's "miner" field instead of the embedded Block.miner, so relayed blocks burn rewards; update the code that processes incoming blocks (the block handler that currently uses payload.get("miner", BURN_ADDRESS)) to read and credit the miner from the deserialized Block object (Block.miner), falling back to BURN_ADDRESS only if Block.miner is missing or None, so relayed blocks pay the block.miner value rather than burning the reward.
🤖 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.
Duplicate comments:
In `@main.py`:
- Around line 80-85: The receive-side handler still credits mining rewards from
the raw payload's "miner" field instead of the embedded Block.miner, so relayed
blocks burn rewards; update the code that processes incoming blocks (the block
handler that currently uses payload.get("miner", BURN_ADDRESS)) to read and
credit the miner from the deserialized Block object (Block.miner), falling back
to BURN_ADDRESS only if Block.miner is missing or None, so relayed blocks pay
the block.miner value rather than burning the reward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0703e209-b77e-4107-868a-fcf12aa4c66b
📒 Files selected for processing (2)
main.pyminichain/p2p.py
|
@SIDDHANTCOOKIE Done with the indentation! Anything else needed? |
LGTM |
|
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
minichain/block.py (2)
71-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not serialize
minerwhen it is absent.Line 80 makes
minerpart of the canonical header even when it isNone, socompute_hash()hashes{"miner": null}instead of omitting the field. That breaks the optional-field contract inminichain/p2p.pyand can makefrom_dict()reject payloads whose hash was computed withmineromitted.Proposed 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, "timestamp": self.timestamp, "difficulty": self.difficulty, "nonce": self.nonce, - "miner": self.miner, } # Include miner in header only when present (optional field) if self.miner is not None: 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 71 - 85, to_header_dict currently always sets "miner" in the header dict then conditionally sets it again, causing {"miner": None} to be serialized; change to only include the miner key when self.miner is not None by removing the initial "miner": self.miner entry and moving the miner assignment into the existing conditional in Block.to_header_dict so compute_hash and Block.from_dict see the field omitted when absent.
39-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWire
state_rootthrough the constructor and deserializer.Line 62 currently raises a
NameErroron everyBlock(...)call becausestate_rootis not in the constructor signature. Even after adding the parameter,from_dict()still dropspayload["state_root"], so deserialized blocks would recompute their header hash against the wrong data. This also conflicts withminichain/p2p.py's block schema, which requiresstate_rooton the wire.Proposed fix
def __init__( self, index: int, previous_hash: str, transactions: Optional[Sequence[Transaction]] = None, timestamp: Optional[float] = None, difficulty: Optional[int] = None, + state_root: Optional[str] = None, miner: Optional[str] = None ): self.index = index self.previous_hash = previous_hash # Freeze transactions into an immutable tuple to prevent header/body mismatch self.transactions = tuple(transactions) if transactions else () - self.miner = miner # Deterministic timestamp (ms) self.timestamp: int = ( round(time.time() * 1000) if timestamp is None else int(timestamp) ) self.difficulty: Optional[int] = difficulty self.nonce: int = 0 self.hash: Optional[str] = None self.state_root: Optional[str] = state_root self.miner: Optional[str] = miner @@ block = cls( index=int(payload["index"]), previous_hash=payload["previous_hash"], transactions=transactions, timestamp=parsed_ts, difficulty=parsed_diff, + state_root=payload["state_root"], miner=payload.get("miner"), )Also applies to: 62-63, 127-134
main.py (1)
79-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the computed
state_rootinto the mined block.Lines 79-80 update
temp_state, but theBlock(...)created at Lines 81-86 still never receives a state root derived from that state.minichain/p2p.pynow rejects block payloads unlesspayload["state_root"]is astr, so locally mined blocks will still broadcast withstate_root=Noneand get dropped by peers. Populate the block’sstate_rootfrom the post-transaction/post-reward temporary state beforemine_block(block)so the hashed header and network payload describe the same state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 79 - 86, After applying credit_mining_reward on temp_state, compute the post-reward state root from temp_state and assign it to the new Block instance before calling mine_block(block) so the block.header/payload and payload["state_root"] match; update the Block(...) created after temp_state.credit_mining_reward(miner_pk) to include the state_root derived from temp_state (e.g. temp_state.compute_state_root()/temp_state.root()/appropriate accessor) and ensure that value is what is serialized into the network payload used by mine_block and peers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@main.py`:
- Around line 79-86: After applying credit_mining_reward on temp_state, compute
the post-reward state root from temp_state and assign it to the new Block
instance before calling mine_block(block) so the block.header/payload and
payload["state_root"] match; update the Block(...) created after
temp_state.credit_mining_reward(miner_pk) to include the state_root derived from
temp_state (e.g. temp_state.compute_state_root()/temp_state.root()/appropriate
accessor) and ensure that value is what is serialized into the network payload
used by mine_block and peers.
In `@minichain/block.py`:
- Around line 71-85: to_header_dict currently always sets "miner" in the header
dict then conditionally sets it again, causing {"miner": None} to be serialized;
change to only include the miner key when self.miner is not None by removing the
initial "miner": self.miner entry and moving the miner assignment into the
existing conditional in Block.to_header_dict so compute_hash and Block.from_dict
see the field omitted when absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c0ca345-116f-40c3-baed-8dca492d1469
📒 Files selected for processing (3)
main.pyminichain/block.pyminichain/p2p.py
|
@sanaica can you check outside diff range comments once as posted by coderabbit, no need to fix it directly just verify those first |
2 participants
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