feat: implement P2P interception layer for peer misbehavior detection#107
Open
g-k-s-03 wants to merge 2 commits into
Open
feat: implement P2P interception layer for peer misbehavior detection#107g-k-s-03 wants to merge 2 commits into
g-k-s-03 wants to merge 2 commits into
Conversation
Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
Contributor
Walkthrough
ChangesValidationStatus Propagation, P2P Misbehavior Banning, and CLI Persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
minichain/chain.py (1)
123-186: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winUpdate boolean
add_blockcallers for the new enum contract.
ValidationStatus.INVALID,FAILED, andMALFORMEDare truthy, so existingif chain.add_block(...)call sites can treat rejected blocks as accepted and remove mempool transactions or skip sync recovery. Compare explicitly withValidationStatus.VALID.🔧 Example caller-side fix
- if chain.add_block(mined_block): + status = chain.add_block(mined_block) + if status == ValidationStatus.VALID: logger.info("✅ Block #%d mined and added (%d txs)", mined_block.index, len(mineable_txs)) mempool.remove_transactions(mineable_txs)🤖 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 123 - 186, The add_block return value in chain.py now uses ValidationStatus instead of a boolean, so caller sites may mis-handle rejected blocks because INVALID/FAILED/MALFORMED are truthy. Update every call site that uses chain.add_block to compare explicitly against ValidationStatus.VALID, especially any logic that removes mempool transactions, marks sync complete, or skips recovery based on success. Use the add_block method and ValidationStatus enum as the reference points when locating and fixing these callers.main.py (2)
232-235: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCompare
add_block()againstValidationStatus.VALIDhere too.
ValidationStatus.INVALIDandValidationStatus.FAILEDare truthy enum values, so this linear sync branch treats rejected blocks as successfully added and removes their transactions.Proposed fix
- if chain.add_block(block): + if chain.add_block(block) == ValidationStatus.VALID: logger.info("📥 Synced Block #%d", block.index) mempool.remove_transactions(block.transactions)🤖 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 232 - 235, The linear sync branch in `main.py` is treating any truthy result from `chain.add_block()` as success, so rejected blocks can still be logged as synced and removed from the mempool. Update the `chain.add_block()` check to explicitly compare against `ValidationStatus.VALID`, matching the existing validation handling elsewhere in this flow. Keep the success path (`logger.info(...)` and `mempool.remove_transactions(...)`) only for the valid status, and leave the rejection path unchanged.
173-188: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAvoid returning peer-misbehavior status for sync/fork cases.
This branch requests sync/reorg for ahead or forked blocks, but still returns
chain.add_block()’s status to P2P. Since link/hash mismatches can map toINVALID, an honest peer on a fork can be banned before reorg resolution.🤖 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 173 - 188, The block-handling path in add_block/receive logic is returning chain.add_block()’s ValidationStatus even when the block is only being treated as a sync or fork signal; this can incorrectly surface INVALID to P2P and penalize honest peers. Update the branch that logs “ahead of us” and “fork detected” so it still triggers the chain_request broadcast/reorg sync, but returns a neutral/non-misbehavior result instead of propagating ValidationStatus from chain.add_block(). Keep the actual validation status only for true invalid blocks, and use the existing chain.last_block, network._broadcast_raw, and ValidationStatus checks to route sync cases separately.
🤖 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 436-437: The ban-related CLI paths are still using the default
persistence location instead of the node’s configured datadir, so update the ban
lookup/write flow to use the same data path consistently. In the command
handlers around the existing get_banned_peers usage, thread the configured
datadir through to persistence, and when constructing the network ensure
P2PNetwork is initialized with data_path=datadir or "." so ban checks and SQLite
state stay in sync. Apply the same fix to the other ban command call sites
referenced by the current get_banned_peers pattern.
- Around line 451-453: Manual bans currently only persist the ban state and do
not cut off an already connected peer; update the CLI ban flow around ban_peer
in main.py to immediately disconnect the target peer after banning it. Use the
existing peer_id in the same manual-ban branch to locate the active connection
and invoke the appropriate disconnect/close action right after ban_peer, keeping
the current ban persistence and success message intact.
In `@minichain/chain.py`:
- Around line 73-76: Validate the consensus-critical initialization in
Chain.__init__ before assigning current_difficulty and avg_block_time: reject
non-positive difficulty and target_block_time values, and ensure alpha from
config is numeric and within a sane range such as 0 < alpha <= 1. Add the checks
near the existing self.target_block_time, self.alpha, self.current_difficulty,
and self.avg_block_time setup so invalid genesis inputs are rejected early.
- Around line 165-167: Mirror the receipt payload validation in
resolve_conflicts() so reorgs reject the same malformed blocks that add_block()
already rejects. Reuse the existing receipts comparison logic from add_block() /
ValidationStatus.INVALID flow when processing candidate blocks in
resolve_conflicts(), and make sure the block.body/block.receipts payload is
checked before persisting or adopting the replacement chain.
In `@minichain/p2p.py`:
- Around line 37-45: Validate the policy inputs in p2p.P2P.__init__ before any
background decay or ban logic starts by rejecting non-positive
decay_interval_minutes and non-positive malformed_threshold, failed_threshold,
and invalid_threshold values. Add the checks near the constructor initialization
so invalid policy settings fail fast, and make sure the defaults/constants used
by _decay_counters and the ban thresholds are only accepted when they are
greater than zero. Also review the related uses around the decay setup and
threshold comparisons to ensure they rely on these validated values.
- Around line 217-219: The P2P reader in the message-processing loop should not
let malformed supported messages escape through `_is_duplicate()` and stop the
asyncio reader. Add validation before calling `_is_duplicate()` so `"block"`
payloads with missing or non-dict `data` are treated as `MALFORMED` and skipped,
rather than reaching `_message_id()` and raising on `payload["hash"]`. Update
the flow around `SUPPORTED_MESSAGE_TYPES`, `_is_duplicate()`, and `_mark_seen()`
to safely reject bad payloads while keeping the loop alive.
- Around line 280-293: The read loop in p2p.py is parsing each chunk from
stream.read(4096) as if it were a full JSON record, so split newline-delimited
messages can be misclassified as MALFORMED. Update the stream-handling logic
around the read loop to keep a per-connection buffer, append each chunk, split
only on complete newline-terminated frames, and leave any trailing partial frame
in the buffer for the next read before calling json.loads and posting to
self._to_asyncio.
In `@minichain/state.py`:
- Around line 108-113: `validate_and_apply_with_status()` in `State` is doing
transaction verification twice because it calls `verify_transaction_logic()` and
then `apply_transaction()` rechecks the same tx. Split the actual state
mutation/receipt creation into an internal helper for already-validated
transactions, then have both `validate_and_apply_with_status()` and the existing
`apply_transaction()` path reuse it so verification happens only once while
preserving the current `ValidationStatus` and `Receipt` behavior.
- Around line 94-106: The validation in validate_and_apply_with_status only
checks tx.amount, but total_cost later includes tx.fee, so add the same
integer/non-negative validation for tx.fee before any balance math. Update the
guard in State.validate_and_apply_with_status (and keep behavior consistent with
the existing validate_and_apply path) so a bad fee returns
ValidationStatus.MALFORMED, None instead of allowing negative balances or
raising on non-integer fees.
In `@minichain/validators.py`:
- Around line 5-9: ValidationStatus is being used in boolean contexts, but all
enum members are truthy, so invalid or failed results are treated as success.
Update the callers of chain.add_block and any related test assertions to compare
explicitly against ValidationStatus.VALID instead of relying on truthiness, and
audit any other boolean checks on ValidationStatus to prevent rejected blocks
from being accepted or mempool transactions from being removed incorrectly.
In `@tests/test_difficulty.py`:
- Around line 18-20: `Blockchain.add_block()` now returns `ValidationStatus`, so
update the affected assertions in `test_difficulty.py` to check the explicit
success value instead of relying on truthiness; in the `test_difficulty` cases
around `mine_block`/`chain.add_block`, compare the return value against
`ValidationStatus.VALID` and keep using the same `chain.add_block` call sites
for the other listed assertions as well.
In `@tests/test_persistence.py`:
- Line 8: The persistence tests are violating the millisecond timestamp contract
and masking restored difficulty behavior in
`test_loaded_chain_can_add_new_block`. Update the fixture/setup code to keep
`Block` header timestamps in milliseconds instead of `int(time.time())`, and
make the loaded-chain assertion verify that `load()` restores the
difficulty-related chain state, not just `chain` and `state.accounts`. In the
`restored.current_difficulty` path, ensure the test uses the restored difficulty
values from the loaded blockchain object and confirms the persisted state is
actually being reused.
---
Outside diff comments:
In `@main.py`:
- Around line 232-235: The linear sync branch in `main.py` is treating any
truthy result from `chain.add_block()` as success, so rejected blocks can still
be logged as synced and removed from the mempool. Update the `chain.add_block()`
check to explicitly compare against `ValidationStatus.VALID`, matching the
existing validation handling elsewhere in this flow. Keep the success path
(`logger.info(...)` and `mempool.remove_transactions(...)`) only for the valid
status, and leave the rejection path unchanged.
- Around line 173-188: The block-handling path in add_block/receive logic is
returning chain.add_block()’s ValidationStatus even when the block is only being
treated as a sync or fork signal; this can incorrectly surface INVALID to P2P
and penalize honest peers. Update the branch that logs “ahead of us” and “fork
detected” so it still triggers the chain_request broadcast/reorg sync, but
returns a neutral/non-misbehavior result instead of propagating ValidationStatus
from chain.add_block(). Keep the actual validation status only for true invalid
blocks, and use the existing chain.last_block, network._broadcast_raw, and
ValidationStatus checks to route sync cases separately.
In `@minichain/chain.py`:
- Around line 123-186: The add_block return value in chain.py now uses
ValidationStatus instead of a boolean, so caller sites may mis-handle rejected
blocks because INVALID/FAILED/MALFORMED are truthy. Update every call site that
uses chain.add_block to compare explicitly against ValidationStatus.VALID,
especially any logic that removes mempool transactions, marks sync complete, or
skips recovery based on success. Use the add_block method and ValidationStatus
enum as the reference points when locating and fixing these callers.
🪄 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: 946bb094-70a8-4dbc-939c-74ceab26863d
📒 Files selected for processing (11)
genesis.jsonmain.pyminichain/chain.pyminichain/p2p.pyminichain/persistence.pyminichain/pow.pyminichain/state.pyminichain/validators.pytests/test_difficulty.pytests/test_persistence.pytests/test_reorg.py
Comment on lines
+73
to
+76
| self.target_block_time = config.get("target_block_time", 10000) | ||
| self.alpha = config.get("alpha", 0.1) | ||
| self.current_difficulty = difficulty | ||
| self.avg_block_time = self.target_block_time |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate genesis difficulty and EMA parameters before using them.
difficulty, target_block_time, and alpha become consensus-critical state. Reject non-positive difficulty/target values and require alpha to be numeric within a sane range, e.g. 0 < alpha <= 1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/chain.py` around lines 73 - 76, Validate the consensus-critical
initialization in Chain.__init__ before assigning current_difficulty and
avg_block_time: reject non-positive difficulty and target_block_time values, and
ensure alpha from config is numeric and within a sane range such as 0 < alpha <=
1. Add the checks near the existing self.target_block_time, self.alpha,
self.current_difficulty, and self.avg_block_time setup so invalid genesis inputs
are rejected early.
Comment on lines
+37
to
+45
| def __init__( | ||
| self, | ||
| handler_callback=None, | ||
| data_path: str = ".", | ||
| malformed_threshold: int = MALFORMED_THRESHOLD, | ||
| failed_threshold: int = FAILED_THRESHOLD, | ||
| invalid_threshold: int = INVALID_THRESHOLD, | ||
| decay_interval_minutes: float = DECAY_INTERVAL_MINUTES, | ||
| ): |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate policy values before starting background decay.
decay_interval_minutes <= 0 makes _decay_counters spin, and non-positive thresholds make bans trigger immediately.
Proposed fix
def __init__(
self,
handler_callback=None,
data_path: str = ".",
malformed_threshold: int = MALFORMED_THRESHOLD,
failed_threshold: int = FAILED_THRESHOLD,
invalid_threshold: int = INVALID_THRESHOLD,
decay_interval_minutes: float = DECAY_INTERVAL_MINUTES,
):
+ if min(malformed_threshold, failed_threshold, invalid_threshold) < 1:
+ raise ValueError("misbehavior thresholds must be >= 1")
+ if decay_interval_minutes <= 0:
+ raise ValueError("decay_interval_minutes must be > 0")
self._handler_callback = handler_callbackAlso applies to: 55-60, 191-193
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 37-37: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
🤖 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 37 - 45, Validate the policy inputs in
p2p.P2P.__init__ before any background decay or ban logic starts by rejecting
non-positive decay_interval_minutes and non-positive malformed_threshold,
failed_threshold, and invalid_threshold values. Add the checks near the
constructor initialization so invalid policy settings fail fast, and make sure
the defaults/constants used by _decay_counters and the ban thresholds are only
accepted when they are greater than zero. Also review the related uses around
the decay setup and threshold comparisons to ensure they rely on these validated
values.
Comment on lines
+18
to
+20
| mined_block1 = mine_block(block1) | ||
| self.assertTrue(chain.add_block(mined_block1)) | ||
| self.assertEqual(chain.current_difficulty, 4) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the explicit ValidationStatus here.
Blockchain.add_block() now returns ValidationStatus, so these checks are no longer validating the success contract directly. Compare against ValidationStatus.VALID instead of relying on enum truthiness or ignoring the return value.
Proposed fix
import unittest
from minichain import Blockchain, Block
from minichain.pow import mine_block
+from minichain.validators import ValidationStatus
@@
- self.assertTrue(chain.add_block(mined_block1))
+ self.assertEqual(chain.add_block(mined_block1), ValidationStatus.VALID)
@@
- self.assertTrue(chain.add_block(mined_block2))
+ self.assertEqual(chain.add_block(mined_block2), ValidationStatus.VALID)
@@
- chain2.add_block(block1)
+ self.assertEqual(chain2.add_block(block1), ValidationStatus.VALID)Also applies to: 26-28, 46-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_difficulty.py` around lines 18 - 20, `Blockchain.add_block()` now
returns `ValidationStatus`, so update the affected assertions in
`test_difficulty.py` to check the explicit success value instead of relying on
truthiness; in the `test_difficulty` cases around
`mine_block`/`chain.add_block`, compare the return value against
`ValidationStatus.VALID` and keep using the same `chain.add_block` call sites
for the other listed assertions as well.
| import shutil | ||
| import sqlite3 | ||
| import tempfile | ||
| import time |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep these persistence fixtures on the millisecond difficulty contract.
Block headers use millisecond timestamps, but these tests now inject int(time.time()) seconds. That can distort EMA difficulty in the saved chain, and test_loaded_chain_can_add_new_block still mines with restored.current_difficulty even though load() only swaps in chain and state.accounts. As written, this can pass without proving the persisted difficulty state was restored correctly.
Proposed fix
-import time
@@
block = Block(
index=1,
previous_hash=bc.last_block.hash,
transactions=[tx],
difficulty=bc.current_difficulty,
state_root=temp_state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
- timestamp=int(time.time()),
+ timestamp=bc.last_block.timestamp + 1,
)
mine_block(block)
@@
restored = load(path=self.tmpdir)
+ self.assertEqual(restored.current_difficulty, bc.current_difficulty)
@@
block2 = Block(
index=len(restored.chain),
previous_hash=restored.last_block.hash,
transactions=[tx2],
difficulty=restored.current_difficulty,
state_root=temp_state.state_root(),
receipt_root=calculate_receipt_root([receipt2]),
receipts=[receipt2],
- timestamp=int(time.time()),
+ timestamp=restored.last_block.timestamp + 1,
)Also applies to: 53-59, 248-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_persistence.py` at line 8, The persistence tests are violating the
millisecond timestamp contract and masking restored difficulty behavior in
`test_loaded_chain_can_add_new_block`. Update the fixture/setup code to keep
`Block` header timestamps in milliseconds instead of `int(time.time())`, and
make the loaded-chain assertion verify that `load()` restores the
difficulty-related chain state, not just `chain` and `state.accounts`. In the
`restored.current_difficulty` path, ensure the test uses the restored difficulty
values from the loaded blockchain object and confirms the persisted state is
actually being reused.
Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
Member
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
149-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t collapse every mempool rejection into
FAILED.Line 160 treats all
mempool.add_transaction()failures the same, but the existing rejection path here already conflates invalid signatures, duplicates, and mempool pressure (see Line 333). That means invalid txs won’t be classified asINVALID, while honest peers can accrue failure strikes for duplicates or local backpressure. The handler needs a reasoned validation result before feeding the P2P ban policy.🤖 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 149 - 164, The tx handling branch in the message validator currently collapses every mempool rejection from mempool.add_transaction into ValidationStatus.FAILED, which hides whether the tx was invalid, a duplicate, or rejected due to local pressure. Update the tx path in the main message handler to inspect the add_transaction result or expose a reason from mempool.add_transaction, then return ValidationStatus.INVALID for bad txs and only use FAILED for transient/mempool-pressure cases so the P2P ban policy in the validation flow can distinguish peers correctly.
♻️ Duplicate comments (1)
main.py (1)
452-453: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winManual ban disconnect is still a no-op.
Line 453 passes a bare
peer_id, butminichain/p2p.pymatches live connections aspeer:{peer_id}in the DISCONNECT path. The ban persists, yet the current connection stays open until it reconnects. Use the samepeer:address format here, andawaitthe coroutine directly since it only enqueues the disconnect.Proposed fix
from minichain.persistence import ban_peer ban_peer(peer_id, reason="Manual ban via CLI", path=datadir or ".") - asyncio.create_task(network.disconnect_peer(peer_id)) + await network.disconnect_peer(f"peer:{peer_id}") print(f" ✅ Peer {peer_id} banned.")🤖 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 452 - 453, The manual ban flow in main.py is not disconnecting the active peer because network.disconnect_peer expects the same live-connection address format used by minichain/p2p.py. Update the CLI ban path around ban_peer and network.disconnect_peer so it passes peer:{peer_id} instead of a bare peer_id, and call the coroutine with await rather than asyncio.create_task since it only enqueues the disconnect. Keep the fix localized to the manual ban disconnect logic so the active connection is dropped immediately after the ban is applied.
🤖 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`:
- Line 232: The remaining add_block() check in mine_and_process_block still
relies on truthiness, so every ValidationStatus value is treated as success.
Update that call to explicitly compare against ValidationStatus.VALID, matching
the sync-path fix, and ensure the mined block is only accepted, its transactions
removed, and the block returned for broadcast when the validation result is
valid.
In `@minichain/state.py`:
- Around line 121-129: `apply_transaction()` no longer preserves the
malformed-input behavior because it calls `verify_transaction_logic()` before
checking the new `amount`/`fee` requirements. Add the same malformed-input guard
used elsewhere in `State.apply_transaction` before verification so a direct call
returns None for bad input instead of raising, while keeping the existing
`verify_transaction_logic()` and `_apply_validated_tx()` flow unchanged.
---
Outside diff comments:
In `@main.py`:
- Around line 149-164: The tx handling branch in the message validator currently
collapses every mempool rejection from mempool.add_transaction into
ValidationStatus.FAILED, which hides whether the tx was invalid, a duplicate, or
rejected due to local pressure. Update the tx path in the main message handler
to inspect the add_transaction result or expose a reason from
mempool.add_transaction, then return ValidationStatus.INVALID for bad txs and
only use FAILED for transient/mempool-pressure cases so the P2P ban policy in
the validation flow can distinguish peers correctly.
---
Duplicate comments:
In `@main.py`:
- Around line 452-453: The manual ban flow in main.py is not disconnecting the
active peer because network.disconnect_peer expects the same live-connection
address format used by minichain/p2p.py. Update the CLI ban path around ban_peer
and network.disconnect_peer so it passes peer:{peer_id} instead of a bare
peer_id, and call the coroutine with await rather than asyncio.create_task since
it only enqueues the disconnect. Keep the fix localized to the manual ban
disconnect logic so the active connection is dropped immediately after the ban
is applied.
🪄 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: 534b6271-59f7-4671-a69c-d2a8104171a0
📒 Files selected for processing (5)
main.pyminichain/chain.pyminichain/state.pytests/test_difficulty.pytests/test_persistence_runtime.py
| if block.index <= chain.last_block.index: | ||
| continue # Ignore already known blocks | ||
| if chain.add_block(block): | ||
| if chain.add_block(block) == ValidationStatus.VALID: |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the remaining add_block() truthiness check.
You fixed the sync path here, but mine_and_process_block() at Line 100 still uses if chain.add_block(mined_block):. Every ValidationStatus member is truthy, so a rejected locally mined block is treated as accepted, its transactions are dropped from the mempool, and the bad block is returned for broadcast.
🤖 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 232, The remaining add_block() check in
mine_and_process_block still relies on truthiness, so every ValidationStatus
value is treated as success. Update that call to explicitly compare against
ValidationStatus.VALID, matching the sync-path fix, and ensure the mined block
is only accepted, its transactions removed, and the block returned for broadcast
when the validation result is valid.
Comment on lines
121
to
+129
| def apply_transaction(self, tx): | ||
| """ | ||
| Applies transaction and mutates state. | ||
| Returns: Receipt object if mathematically valid, None if invalid. | ||
| Validates and applies a transaction. | ||
| Returns: Receipt object if valid, None if invalid. | ||
| """ | ||
| from .validators import ValidationStatus | ||
| status = self.verify_transaction_logic(tx) | ||
| if status != ValidationStatus.VALID: | ||
| if self.verify_transaction_logic(tx) != ValidationStatus.VALID: | ||
| return None | ||
| return self._apply_validated_tx(tx) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep apply_transaction()'s malformed-input contract intact.
Line 127 now enters verify_transaction_logic() without the new amount/fee guards, so a direct apply_transaction() call can still raise on malformed input instead of returning None as documented. Please reuse the same malformed check here before verification.
Proposed fix
def apply_transaction(self, tx):
"""
Validates and applies a transaction.
Returns: Receipt object if valid, None if invalid.
"""
+ if not isinstance(tx.amount, int) or tx.amount < 0:
+ return None
+ fee = getattr(tx, "fee", 0)
+ if not isinstance(fee, int) or fee < 0:
+ return None
from .validators import ValidationStatus
if self.verify_transaction_logic(tx) != ValidationStatus.VALID:
return None
return self._apply_validated_tx(tx)📝 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.
Suggested change
| def apply_transaction(self, tx): | |
| """ | |
| Applies transaction and mutates state. | |
| Returns: Receipt object if mathematically valid, None if invalid. | |
| Validates and applies a transaction. | |
| Returns: Receipt object if valid, None if invalid. | |
| """ | |
| from .validators import ValidationStatus | |
| status = self.verify_transaction_logic(tx) | |
| if status != ValidationStatus.VALID: | |
| if self.verify_transaction_logic(tx) != ValidationStatus.VALID: | |
| return None | |
| return self._apply_validated_tx(tx) | |
| def apply_transaction(self, tx): | |
| """ | |
| Validates and applies a transaction. | |
| Returns: Receipt object if valid, None if invalid. | |
| """ | |
| if not isinstance(tx.amount, int) or tx.amount < 0: | |
| return None | |
| fee = getattr(tx, "fee", 0) | |
| if not isinstance(fee, int) or fee < 0: | |
| return None | |
| from .validators import ValidationStatus | |
| if self.verify_transaction_logic(tx) != ValidationStatus.VALID: | |
| return None | |
| return self._apply_validated_tx(tx) |
🤖 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/state.py` around lines 121 - 129, `apply_transaction()` no longer
preserves the malformed-input behavior because it calls
`verify_transaction_logic()` before checking the new `amount`/`fee`
requirements. Add the same malformed-input guard used elsewhere in
`State.apply_transaction` before verification so a direct call returns None for
bad input instead of raising, while keeping the existing
`verify_transaction_logic()` and `_apply_validated_tx()` flow unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues: Fixes #97
Summary:
It builds directly on top of the Ledger taxonomy and CLI logic implemented in #106 .
What was implemented:
Before accepting any incoming connection, stream_handler now calls is_peer_banned(). If the peer is already blacklisted, the stream is reset immediately and the connection is dropped before any data is processed.
2. MALFORMED signalling from JSON parse errors
The bare except Exception: pass that previously swallowed all JSON decode errors silently has been replaced. Parse failures now signal a ("MALFORMED", addr) event to the asyncio side, which applies the full misbehavior policy rather than ignoring the failure.
3. Callback return value capture
_asyncio_reader now captures the ValidationStatus returned by the handler callback. Previously the return value was discarded entirely. For tx and block message types, the returned status is passed to _handle_validation_status for policy enforcement.
4. _handle_validation_status policy enforcement
Single point that enforces the agreed taxonomy:
MALFORMED → always disconnect, ban only if counter exceeds threshold N
FAILED → drop silently, ban and disconnect only if counter exceeds threshold M
INVALID → ban and disconnect immediately (default L=1 means first occurrence always bans)
Each peer has three independent counters (malformed, failed, invalid). A background asyncio task (_decay_counters) runs every T minutes and divides all counters by 2. This avoids the fixed-window boundary exploit where a peer could spam up to the limit, wait for reset, and repeat indefinitely.
6. Configurable thresholds
All four values are configurable as module-level constants and overridable per P2PNetwork instance:
MALFORMED_THRESHOLD = 15 (N)
FAILED_THRESHOLD = 15 (M)
INVALID_THRESHOLD = 1 (L)
DECAY_INTERVAL_MINUTES = 10 (T)
The network message handler in main.py now returns explicit ValidationStatus values for tx and block messages so the P2P layer can act on them. Non-content messages (hello, chain_request, chain_response) return None implicitly, which the interception logic correctly ignores.
Testing:
All 74 existing tests pass with no modifications.
py -3.11 -m pytest tests/ -v
74 passed, 1 warning in 24.55s
Summary by CodeRabbit
New Features
Bug Fixes