Feat/fork choice#100
Conversation
WalkthroughAdds a full chain-sync and fork-resolution subsystem: ChangesChain Synchronization and Fork Resolution
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over NodeA,NodeB: Block received ahead of local tip
NodeB->>NodeA: block message (rejected – ahead of tip)
NodeA->>NodeB: broadcast_chain_request()
end
rect rgba(144, 238, 144, 0.5)
Note over NodeA,NodeB: Chain exchange
NodeB->>NodeA: chain_request
NodeA->>NodeB: send_chain_response(local blocks)
NodeB->>NodeA: chain_response(blocks)
end
rect rgba(255, 215, 0, 0.5)
Note over NodeA,Blockchain: Fork resolution
NodeA->>Blockchain: resolve_conflicts(new_chain_list)
Blockchain->>State: restore(_genesis_state_snapshot)
Blockchain->>Blockchain: replay & validate each block
Blockchain-->>NodeA: (True, orphans)
NodeA->>Mempool: add_transaction per orphan
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 8
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)
272-277:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
chain_requestmessages bypass duplicate detection, enabling spam attacks.
_message_idreturnsNoneforchain_requestandchain_response, so_is_duplicatealways returnsFalsefor these types. A malicious peer can repeatedly sendchain_requestmessages, causing the node to serialize and broadcast its entire chain each time, wasting CPU and bandwidth.Consider adding rate limiting or tracking seen
chain_requestmessages per peer address.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@minichain/p2p.py` around lines 272 - 277, The _message_id method in p2p.py returns None for chain_request and chain_response message types, bypassing duplicate detection and allowing spam attacks. Extend the _message_id method to handle chain_request and chain_response types by generating or extracting a message identifier from the payload (similar to how tx uses canonical_json_hash or block uses payload["hash"]), so that duplicate detection via _is_duplicate can properly identify and filter repeated messages from malicious 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.
Inline comments:
In `@main.py`:
- Around line 183-188: The code in the chain reorganization block only logs a
message when resolve_conflicts succeeds, but provides no feedback when it fails.
Add a log statement in an else block or separate conditional that triggers when
the success variable returned from chain.resolve_conflicts(new_chain) is False,
so that reorg failures are properly recorded for debugging sync issues.
- Line 164: The asyncio.create_task call for network.broadcast_chain_request()
does not store the returned task reference, which can cause exceptions to be
silently lost and allows the task to potentially be garbage collected. Store the
task reference in a variable or maintain it in a task collection (such as a set)
to ensure the task is not garbage collected and exceptions can be properly
handled. This will allow you to await the task or add callbacks to handle any
exceptions that occur during execution.
- Around line 168-172: The chain_request handler at the elif msg_type ==
"chain_request" block is using network._broadcast_raw() to send the chain
response to all peers instead of responding only to the requesting peer. To fix
this, modify the P2P layer to pass the requesting peer's writer in the message
data alongside _peer_addr, then update the handler to receive that writer and
use send_chain_response() with that specific peer's writer to send the response
directly to the requester instead of calling network._broadcast_raw(). This
avoids unnecessary network traffic by sending the chain only to the peer that
requested it.
In `@minichain/chain.py`:
- Around line 104-113: The get_total_work method has two security and
correctness issues in the sum calculation. First, the expression 2 **
(block.difficulty or 1) treats a valid difficulty value of 0 as falsy and
incorrectly substitutes 1, when 0 should be treated as a legitimate difficulty
value. Second, the computation allows unbounded peer-controlled exponents which
can cause expensive big-integer arithmetic during chain-sync operations. Fix
this by replacing the or fallback with explicit validation: ensure difficulty
has a sensible default only when missing or None (not when it is 0), and add a
maximum cap on the difficulty exponent to prevent peer-controlled DoS attacks.
The cap should be reasonable for your consensus algorithm while rejecting
malformed or malicious difficulty values.
- Around line 233-234: The assignment `self.chain = new_chain_list` on line 233
creates an alias to the external list instead of copying it, which means
external mutations to the caller's list can modify the node's chain without
replaying state. Replace this assignment with a copy of the list (using either
the copy() method or the list() constructor) to ensure the node maintains an
independent chain that cannot be mutated by the caller's modifications to their
original list.
In `@minichain/p2p.py`:
- Line 18: The class docstring does not reflect the new message types added to
SUPPORTED_MESSAGE_TYPES. Update the docstring that documents the JSON wire
format to include the two new message types chain_request and chain_response
alongside the existing sync, tx, and block types. Ensure the docstring
accurately represents all message types currently defined in
SUPPORTED_MESSAGE_TYPES.
- Around line 417-418: In the exception handler catching Exception for the send
chain response failure, replace the logger.error call with logger.exception. The
logger.exception method automatically preserves and includes the full stack
trace in the log output, whereas logger.error only logs the error message
itself. This change will provide the traceback needed for debugging network
issues.
- Around line 236-244: The _validate_chain_response method validates the
structure of the chain response but lacks a size limit check on the blocks list,
creating a DoS vulnerability. After verifying that payload["blocks"] is a list,
add a check to ensure the list length does not exceed a reasonable maximum
threshold (define an appropriate constant for max blocks allowed). Insert this
length validation before the loop that iterates through payload["blocks"] to
validate each block.
---
Outside diff comments:
In `@minichain/p2p.py`:
- Around line 272-277: The _message_id method in p2p.py returns None for
chain_request and chain_response message types, bypassing duplicate detection
and allowing spam attacks. Extend the _message_id method to handle chain_request
and chain_response types by generating or extracting a message identifier from
the payload (similar to how tx uses canonical_json_hash or block uses
payload["hash"]), so that duplicate detection via _is_duplicate can properly
identify and filter repeated messages from malicious peers.
🪄 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: a0c262fe-d745-4e0b-bfc7-3b1efae79478
📒 Files selected for processing (6)
main.pyminichain/chain.pyminichain/mempool.pyminichain/p2p.pyminichain/state.pytests/test_reorg.py
| logger.warning("📥 Received Block #%s — rejected", block.index) | ||
| if block.index > chain.last_block.index: | ||
| logger.warning("📥 Received Block #%s — ahead of us (tip: %s). Requesting chain sync...", block.index, chain.last_block.index) | ||
| asyncio.create_task(network.broadcast_chain_request()) |
There was a problem hiding this comment.
Fire-and-forget asyncio.create_task may silently lose exceptions.
The task reference is not stored, so if the task fails with an exception, it will be silently swallowed. Additionally, the task could theoretically be garbage collected before completion (though CPython currently keeps a reference).
Consider storing the task reference or using a background task set:
♻️ Proposed fix
+# At module level or in handler closure
+_background_tasks = set()
+
# In the handler:
- asyncio.create_task(network.broadcast_chain_request())
+ task = asyncio.create_task(network.broadcast_chain_request())
+ _background_tasks.add(task)
+ task.add_done_callback(_background_tasks.discard)🧰 Tools
🪛 Ruff (0.15.17)
[warning] 164-164: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 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` at line 164, The asyncio.create_task call for
network.broadcast_chain_request() does not store the returned task reference,
which can cause exceptions to be silently lost and allows the task to
potentially be garbage collected. Store the task reference in a variable or
maintain it in a task collection (such as a set) to ensure the task is not
garbage collected and exceptions can be properly handled. This will allow you to
await the task or add callbacks to handle any exceptions that occur during
execution.
Source: Linters/SAST tools
| elif msg_type == "chain_request": | ||
| logger.info("📡 Peer requested chain sync. Broadcasting our chain...") | ||
| blocks_dicts = [b.to_dict() for b in chain.chain] | ||
| payload = {"type": "chain_response", "data": {"blocks": blocks_dicts}} | ||
| asyncio.create_task(network._broadcast_raw(payload)) |
There was a problem hiding this comment.
chain_request handler broadcasts to all peers instead of responding to the requester.
When a peer sends a chain_request, the handler calls network._broadcast_raw(payload) which sends the chain to all connected peers, not just the requesting peer. This is inefficient and could cause unnecessary network traffic and potential amplification.
The handler should use send_chain_response with the specific peer's writer. However, the current handler signature doesn't provide access to the peer's writer. Consider passing the writer through the message data or adding a response mechanism.
🐛 Proposed approach
One solution is to include the writer in the callback data (requires P2P layer changes):
elif msg_type == "chain_request":
logger.info("📡 Peer requested chain sync. Broadcasting our chain...")
blocks_dicts = [b.to_dict() for b in chain.chain]
- payload = {"type": "chain_response", "data": {"blocks": blocks_dicts}}
- asyncio.create_task(network._broadcast_raw(payload))
+ writer = data.get("_writer")
+ if writer:
+ asyncio.create_task(network.send_chain_response(blocks_dicts, writer))
+ else:
+ logger.warning("Cannot respond to chain_request: no writer available")This requires the P2P layer to pass the writer in the message data alongside _peer_addr.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 172-172: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 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 168 - 172, The chain_request handler at the elif
msg_type == "chain_request" block is using network._broadcast_raw() to send the
chain response to all peers instead of responding only to the requesting peer.
To fix this, modify the P2P layer to pass the requesting peer's writer in the
message data alongside _peer_addr, then update the handler to receive that
writer and use send_chain_response() with that specific peer's writer to send
the response directly to the requester instead of calling
network._broadcast_raw(). This avoids unnecessary network traffic by sending the
chain only to the peer that requested it.
| if new_chain: | ||
| success, orphans = chain.resolve_conflicts(new_chain) | ||
| if success: | ||
| logger.info("🔄 Reorg complete! Restoring %d orphaned txs to mempool.", len(orphans)) | ||
| for tx in orphans: | ||
| mempool.add_transaction(tx) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Missing log when resolve_conflicts returns success=False.
When the reorg fails (e.g., the incoming chain has less total work), there's no log message indicating rejection. This makes debugging sync issues harder.
♻️ Proposed fix
if new_chain:
success, orphans = chain.resolve_conflicts(new_chain)
if success:
logger.info("🔄 Reorg complete! Restoring %d orphaned txs to mempool.", len(orphans))
for tx in orphans:
mempool.add_transaction(tx)
+ else:
+ logger.info("🔄 Received chain rejected (not heavier than local)")🤖 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 183 - 188, The code in the chain reorganization block
only logs a message when resolve_conflicts succeeds, but provides no feedback
when it fails. Add a log statement in an else block or separate conditional that
triggers when the success variable returned from
chain.resolve_conflicts(new_chain) is False, so that reorg failures are properly
recorded for debugging sync issues.
| def get_total_work(self, chain_list=None): | ||
| """ | ||
| Calculates the cumulative PoW of a chain. | ||
| Work is proportional to 2^difficulty. | ||
| """ | ||
| if chain_list is None: | ||
| with self._lock: | ||
| chain_list = self.chain | ||
| return sum(2 ** (block.difficulty or 1) for block in chain_list) | ||
|
|
There was a problem hiding this comment.
Harden cumulative-work computation against malformed peer difficulty values.
2 ** (block.difficulty or 1) has two problems: it incorrectly treats valid difficulty=0 as 1, and it allows unbounded peer-controlled exponents during conflict checks, which can cause expensive big-int work in the chain-sync path.
Proposed fix
def get_total_work(self, chain_list=None):
"""
Calculates the cumulative PoW of a chain.
Work is proportional to 2^difficulty.
"""
if chain_list is None:
with self._lock:
chain_list = self.chain
- return sum(2 ** (block.difficulty or 1) for block in chain_list)
+ total_work = 0
+ for block in chain_list:
+ difficulty = block.difficulty
+ if not isinstance(difficulty, int) or difficulty < 0 or difficulty > 64:
+ raise ValueError(f"Invalid block difficulty: {difficulty}")
+ total_work += 1 << difficulty
+ return total_work🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/chain.py` around lines 104 - 113, The get_total_work method has two
security and correctness issues in the sum calculation. First, the expression 2
** (block.difficulty or 1) treats a valid difficulty value of 0 as falsy and
incorrectly substitutes 1, when 0 should be treated as a legitimate difficulty
value. Second, the computation allows unbounded peer-controlled exponents which
can cause expensive big-integer arithmetic during chain-sync operations. Fix
this by replacing the or fallback with explicit validation: ensure difficulty
has a sensible default only when missing or None (not when it is 0), and add a
maximum cap on the difficulty exponent to prevent peer-controlled DoS attacks.
The cap should be reasonable for your consensus algorithm while rejecting
malformed or malicious difficulty values.
| self.chain = new_chain_list | ||
| self.state = temp_state |
There was a problem hiding this comment.
Avoid aliasing external chain lists when committing a reorg.
Assigning self.chain = new_chain_list keeps a shared list reference. If the caller passed a live chain object (for example tests/test_reorg.py Line 89), later appends on that source chain can mutate this node’s chain without replaying state, breaking chain/state consistency.
Proposed fix
- self.chain = new_chain_list
+ self.chain = list(new_chain_list)
self.state = temp_state📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.chain = new_chain_list | |
| self.state = temp_state | |
| self.chain = list(new_chain_list) | |
| self.state = temp_state |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/chain.py` around lines 233 - 234, The assignment `self.chain =
new_chain_list` on line 233 creates an alias to the external list instead of
copying it, which means external mutations to the caller's list can modify the
node's chain without replaying state. Replace this assignment with a copy of the
list (using either the copy() method or the list() constructor) to ensure the
node maintains an independent chain that cannot be mutated by the caller's
modifications to their original list.
|
|
||
| TOPIC = "minichain-global" | ||
| SUPPORTED_MESSAGE_TYPES = {"sync", "tx", "block"} | ||
| SUPPORTED_MESSAGE_TYPES = {"sync", "tx", "block", "chain_request", "chain_response"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Stale docstring does not reflect new message types.
The class docstring at line 26-27 still documents only "sync" | "tx" | "block" for the JSON wire format, but chain_request and chain_response are now supported. Update the docstring to include the new message types.
Also applies to: 26-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/p2p.py` at line 18, The class docstring does not reflect the new
message types added to SUPPORTED_MESSAGE_TYPES. Update the docstring that
documents the JSON wire format to include the two new message types
chain_request and chain_response alongside the existing sync, tx, and block
types. Ensure the docstring accurately represents all message types currently
defined in SUPPORTED_MESSAGE_TYPES.
| def _validate_chain_response(self, payload): | ||
| if not isinstance(payload, dict) or "blocks" not in payload: | ||
| return False | ||
| if not isinstance(payload["blocks"], list): | ||
| return False | ||
| for block_payload in payload["blocks"]: | ||
| if not self._validate_block_payload(block_payload): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Missing size limit on blocks list creates potential DoS vector.
_validate_chain_response validates block structure but doesn't limit the number of blocks. A malicious peer could send a chain_response with an extremely large blocks list, causing memory exhaustion or CPU starvation during validation.
Consider adding a reasonable upper bound:
🛡️ Proposed fix
+MAX_CHAIN_RESPONSE_BLOCKS = 500 # or appropriate limit
+
def _validate_chain_response(self, payload):
if not isinstance(payload, dict) or "blocks" not in payload:
return False
if not isinstance(payload["blocks"], list):
return False
+ if len(payload["blocks"]) > MAX_CHAIN_RESPONSE_BLOCKS:
+ return False
for block_payload in payload["blocks"]:
if not self._validate_block_payload(block_payload):
return False
return True🧰 Tools
🪛 Ruff (0.15.17)
[warning] 236-236: Missing return type annotation for private function _validate_chain_response
Add return type annotation: bool
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/p2p.py` around lines 236 - 244, The _validate_chain_response method
validates the structure of the chain response but lacks a size limit check on
the blocks list, creating a DoS vulnerability. After verifying that
payload["blocks"] is a list, add a check to ensure the list length does not
exceed a reasonable maximum threshold (define an appropriate constant for max
blocks allowed). Insert this length validation before the loop that iterates
through payload["blocks"] to validate each block.
| except Exception as e: | ||
| logger.error("Network: Failed to send chain response: %s", e) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use logger.exception to preserve stack trace on send failure.
The broad Exception catch loses the stack trace. Using logger.exception instead of logger.error will preserve the traceback for debugging network issues.
♻️ Proposed fix
except Exception as e:
- logger.error("Network: Failed to send chain response: %s", e)
+ logger.exception("Network: Failed to send chain response: %s", e)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error("Network: Failed to send chain response: %s", e) | |
| except Exception as e: | |
| logger.exception("Network: Failed to send chain response: %s", e) |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 417-417: Do not catch blind exception: Exception
(BLE001)
[warning] 418-418: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/p2p.py` around lines 417 - 418, In the exception handler catching
Exception for the send chain response failure, replace the logger.error call
with logger.exception. The logger.exception method automatically preserves and
includes the full stack trace in the log output, whereas logger.error only logs
the error message itself. This change will provide the traceback needed for
debugging network issues.
Source: Linters/SAST tools
1 participant
Addressed Issues:
Fixes #(TODO:issue number)
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests