feat: implement P2P interception layer for peer misbehavior detection#107
feat: implement P2P interception layer for peer misbehavior detection#107g-k-s-03 wants to merge 4 commits into
Conversation
Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughValidationStatus now flows through state validation, block handling, and P2P message processing. P2P peers accumulate misbehavior counters with threshold-based bans and decay. CLI ban persistence uses the node datadir, and reorg validation checks receipt payload equality. 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 | ✅ 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}
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
Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
…lean bug and apply_transaction malformed guard Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
main.py (3)
173-188: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not trigger sync for malformed block status.
A syntactically parsed block with malformed transactions can make
chain.add_block()returnValidationStatus.MALFORMED; this branch still schedules catch-up/reorg traffic before P2P disconnects the peer.Proposed fix
status = chain.add_block(block) if status == ValidationStatus.VALID: logger.info("📥 Received Block #%d — added to chain", block.index) # Drop only confirmed transactions so higher nonces can remain queued. mempool.remove_transactions(block.transactions) else: + if status == ValidationStatus.MALFORMED: + logger.warning("📥 Received malformed Block #%s — dropping without sync.", block.index) + return status if block.index > chain.last_block.index + 1: logger.warning("📥 Received Block #%s — ahead of us (tip: %s). Requesting chunked sync...", block.index, chain.last_block.index)🤖 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 branch in the chain receive flow is treating every non-VALID result as a sync/reorg case; update the logic around chain.add_block and ValidationStatus so ValidationStatus.MALFORMED does not schedule any chain_request or asyncio.create_task(network._broadcast_raw(...)). Keep the existing catch-up/reorg requests only for legitimate ahead-of-tip or fork cases, and return the malformed status directly so the peer can be disconnected without triggering sync traffic.
452-453: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPass the active
peer_addrtodisconnect_peer()in the CLI ban pathdisconnect_peer()matches the live stream by address (peer:{peer_id}), so calling it with the rawpeer_idwill ban the peer in SQLite but leave the current connection open.🤖 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 CLI ban flow in main.py is disconnecting the peer with the raw peer_id, but disconnect_peer() expects the active peer_addr to match the live stream. Update the ban path so it uses the current peer address associated with the banned peer (the same address format used by disconnect_peer and the network stream lookup) before calling asyncio.create_task(network.disconnect_peer(...)), while keeping ban_peer() unchanged.
160-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDistinguish mempool rejection reasons before returning
FAILED
Mempool.add_transaction()returnsFalsefor more than bad signatures: stale replacements and a full mempool also hit this path. Mapping every rejection toValidationStatus.FAILEDcan penalize honest peers for valid gossip when the node is saturated. Return a reason/status and only count actual invalid transactions as failures.🤖 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 160 - 164, The tx validation path in the function that calls mempool.add_transaction currently collapses every False result into ValidationStatus.FAILED. Update this flow to capture and inspect the rejection reason from mempool.add_transaction, then map only truly invalid transactions to FAILED while treating stale replacements or a full mempool as non-failure outcomes. Use the existing validation/return path around logger.info and ValidationStatus to keep the behavior aligned with the specific rejection reason.minichain/state.py (1)
94-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject boolean
amountandfeevalues in all transaction entry points.
isinstance(..., int)acceptsTrue/False, so boolean values can slip through as1/0and be applied as valid amounts or fees. Use an explicittype(x) is intcheck (or excludebool) invalidate_and_apply(),validate_and_apply_with_status(), andapply_transaction().🤖 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 - 99, Reject boolean amount and fee values in all transaction validation paths. In minichain/state.py, update validate_and_apply(), validate_and_apply_with_status(), and apply_transaction() so they use an explicit integer-only check (for example, type(x) is int or an equivalent bool exclusion) instead of isinstance(..., int). Make sure tx.amount and the fee field both fail validation when they are True or False, while still allowing only non-negative real integers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@main.py`:
- Around line 173-188: The block-handling branch in the chain receive flow is
treating every non-VALID result as a sync/reorg case; update the logic around
chain.add_block and ValidationStatus so ValidationStatus.MALFORMED does not
schedule any chain_request or asyncio.create_task(network._broadcast_raw(...)).
Keep the existing catch-up/reorg requests only for legitimate ahead-of-tip or
fork cases, and return the malformed status directly so the peer can be
disconnected without triggering sync traffic.
- Around line 452-453: The CLI ban flow in main.py is disconnecting the peer
with the raw peer_id, but disconnect_peer() expects the active peer_addr to
match the live stream. Update the ban path so it uses the current peer address
associated with the banned peer (the same address format used by disconnect_peer
and the network stream lookup) before calling
asyncio.create_task(network.disconnect_peer(...)), while keeping ban_peer()
unchanged.
- Around line 160-164: The tx validation path in the function that calls
mempool.add_transaction currently collapses every False result into
ValidationStatus.FAILED. Update this flow to capture and inspect the rejection
reason from mempool.add_transaction, then map only truly invalid transactions to
FAILED while treating stale replacements or a full mempool as non-failure
outcomes. Use the existing validation/return path around logger.info and
ValidationStatus to keep the behavior aligned with the specific rejection
reason.
In `@minichain/state.py`:
- Around line 94-99: Reject boolean amount and fee values in all transaction
validation paths. In minichain/state.py, update validate_and_apply(),
validate_and_apply_with_status(), and apply_transaction() so they use an
explicit integer-only check (for example, type(x) is int or an equivalent bool
exclusion) instead of isinstance(..., int). Make sure tx.amount and the fee
field both fail validation when they are True or False, while still allowing
only non-negative real integers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 753a05af-06e0-4ce4-8e3c-245e6c5b1fc1
📒 Files selected for processing (2)
main.pyminichain/state.py
Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
| peer_id = parts[1] | ||
| 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=datadir or ".") |
There was a problem hiding this comment.
@g-k-s-03
One small bug here: network.disconnect_peer() expects the peer_addr format ("peer:<peer_id>"), not the bare peer_id. Because this passes the bare ID, the active stream won't actually be disconnected (though they will be successfully blacklisted for future attempts).
Could you change this to include the prefix?
asyncio.create_task(network.disconnect_peer(f"peer:{peer_id}"))
2 participants
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
VALIDstatus.Tests