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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds EMA-based proof-of-work difficulty adjustment (reading ChangesEMA Difficulty, ValidationStatus, and Peer Banning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
+436
to
+437
| from minichain.persistence import get_banned_peers | ||
| banned = get_banned_peers() |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the configured node datadir for ban persistence.
These CLI commands use the default "." path, while the node may be running with --datadir; bans then land in a different SQLite file than the node’s persisted state/P2P ban checks.
Proposed fix
- banned = get_banned_peers()
+ banned = get_banned_peers(path=network.data_path)
...
- ban_peer(peer_id, reason="Manual ban via CLI")
+ ban_peer(peer_id, reason="Manual ban via CLI", path=network.data_path)
...
- unban_peer(peer_id)
+ unban_peer(peer_id, path=network.data_path)Also wire P2PNetwork(data_path=datadir or ".") when constructing the network.
Also applies to: 451-452, 461-462
🤖 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 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.
Comment on lines
+451
to
+453
| from minichain.persistence import ban_peer | ||
| ban_peer(peer_id, reason="Manual ban via CLI") | ||
| print(f" ✅ Peer {peer_id} banned.") |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disconnect peers immediately after a manual ban.
Persisting the ban only rejects future stream entries; an already-connected peer can continue sending until separately disconnected.
Proposed fix
from minichain.persistence import ban_peer
- ban_peer(peer_id, reason="Manual ban via CLI")
+ ban_peer(peer_id, reason="Manual ban via CLI", path=network.data_path)
+ await network.disconnect_peer(f"peer:{peer_id}")
print(f" ✅ Peer {peer_id} banned.")📝 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
| from minichain.persistence import ban_peer | |
| ban_peer(peer_id, reason="Manual ban via CLI") | |
| print(f" ✅ Peer {peer_id} banned.") | |
| from minichain.persistence import ban_peer | |
| ban_peer(peer_id, reason="Manual ban via CLI", path=network.data_path) | |
| 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 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.
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
165
to
+167
| if [r.to_dict() for r in block.receipts] != [r.to_dict() for r in receipts]: | ||
| logger.warning("Block %s rejected: Receipts payload mismatch", block.index) | ||
| return False | ||
| return ValidationStatus.INVALID |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mirror receipt payload validation during reorg.
add_block() now rejects blocks whose block.receipts payload differs from locally computed receipts, but resolve_conflicts() can still accept and persist the same malformed block body during reorg.
🧩 Proposed fix
computed_receipt_root = calculate_receipt_root(receipts)
if block.receipt_root != computed_receipt_root:
logger.warning("Reorg failed: Invalid receipt root at block %s. Expected %s, got %s", block.index, computed_receipt_root, block.receipt_root)
return False, []
+
+ if [r.to_dict() for r in block.receipts] != [r.to_dict() for r in receipts]:
+ logger.warning("Reorg failed: Receipts payload mismatch at block %s", block.index)
+ return False, []Also applies to: 240-276
🤖 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 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.
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
+94
to
+106
| # Semantic validation: amount must be an integer and non-negative | ||
| if not isinstance(tx.amount, int) or tx.amount < 0: | ||
| return None | ||
| # Further checks can be added here | ||
| return self.apply_transaction(tx) | ||
|
|
||
| def validate_and_apply_with_status(self, tx): | ||
| """ | ||
| Validate and apply a transaction, bubbling up the precise ValidationStatus. | ||
| Returns: (ValidationStatus, Receipt|None) | ||
| """ | ||
| from .validators import ValidationStatus | ||
| if not isinstance(tx.amount, int) or tx.amount < 0: | ||
| return ValidationStatus.MALFORMED, None |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Validate fee alongside amount before applying balance math.
Only tx.amount is checked here, but fee is later included in total_cost; a negative fee can increase the sender balance, and a non-integer fee can crash validation instead of returning MALFORMED.
🛡️ Proposed fix
- if not isinstance(tx.amount, int) or tx.amount < 0:
+ fee = getattr(tx, 'fee', 0)
+ if (
+ not isinstance(tx.amount, int)
+ or tx.amount < 0
+ or not isinstance(fee, int)
+ or fee < 0
+ ):
return NoneApply the same check in validate_and_apply_with_status, returning ValidationStatus.MALFORMED.
🤖 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 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.
Comment on lines
+108
to
+113
| status = self.verify_transaction_logic(tx) | ||
| if status != ValidationStatus.VALID: | ||
| return status, None | ||
|
|
||
| # We know it's valid, so apply_transaction will succeed and return a Receipt | ||
| return ValidationStatus.VALID, self.apply_transaction(tx) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid re-running transaction verification on the status-aware path.
validate_and_apply_with_status() verifies once, then apply_transaction() verifies again. This doubles signature and account validation work for every block transaction; split the mutation body into an internal “apply already-validated tx” helper and have both public APIs share it.
Also applies to: 115-122
🤖 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 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.
Comment on lines
+5
to
+9
| class ValidationStatus(Enum): | ||
| VALID = auto() | ||
| INVALID = auto() | ||
| FAILED = auto() | ||
| MALFORMED = auto() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This enum breaks existing boolean callers.
ValidationStatus members are all truthy, so existing checks like if chain.add_block(...) in main.py and assertTrue(chain.add_block(...)) in the tests will treat INVALID and FAILED as success. That will incorrectly remove transactions from the mempool and mark rejected blocks as accepted. Audit those call sites to compare explicitly against ValidationStatus.VALID (or make boolean use impossible here).
🤖 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/validators.py` around lines 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.
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>
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
1 participant
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
Improvements