Feat/peer blacklisting#106
Open
SIDDHANTCOOKIE wants to merge 1 commit into
Open
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughGenesis, mining, transaction validation, block acceptance, and reorg handling now use EMA difficulty tracking and validation statuses. SQLite-backed banned-peer storage and CLI commands were added, and related tests and fixtures were updated for the new block difficulty behavior. ChangesConsensus difficulty and validation
Banned peer persistence and CLI
Sequence Diagram(s)sequenceDiagram
participant MainPy as "main.py"
participant MineBlock as "mine_block"
participant BlockchainAddBlock as "Blockchain.add_block"
participant StateValidateAndApplyWithStatus as "State.validate_and_apply_with_status"
MainPy->>MineBlock: build Block with chain.current_difficulty
MineBlock-->>MainPy: mined Block
MainPy->>BlockchainAddBlock: submit Block to Blockchain.add_block
BlockchainAddBlock->>StateValidateAndApplyWithStatus: validate transaction
StateValidateAndApplyWithStatus-->>BlockchainAddBlock: ValidationStatus + Receipt
BlockchainAddBlock-->>MainPy: VALID or INVALID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.py (1)
95-100: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCompare
add_block()againstValidationStatus.VALID.ValidationStatusmembers are truthy, soINVALID/FAILED/MALFORMEDstill enter the success path here and can drop txs from the mempool and broadcast a rejected block.🤖 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 95 - 100, The success check around add_block() is treating every ValidationStatus as truthy, so rejected outcomes can still follow the success path. Update the block-processing flow in main.py to explicitly compare the result of chain.add_block(mined_block) against ValidationStatus.VALID, and only perform mempool cleanup and block broadcast when that exact status is returned. Use the add_block() call and ValidationStatus enum as the key symbols when locating the fix.minichain/chain.py (1)
251-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReorg replay skips the receipts payload check.
add_blockrejects mismatchedblock.receipts, butresolve_conflictsonly validatesreceipt_root. A heavier chain can therefore be accepted with forged or stale receipt payloads, andself.chain = new_chain_listpreserves that bad data.Suggested 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, [] if block.state_root != temp_state.state_root():🤖 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 251 - 258, resolve_conflicts currently only checks receipt_root, so it can accept a replayed block with tampered or stale receipts payloads; add an explicit receipts payload validation in the reorg replay path. Update the validation logic around calculate_receipt_root, add_block, and resolve_conflicts so the reconstructed receipts list is compared against block.receipts before accepting the new chain, and reject the reorg if they do not match.
🤖 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 `@genesis.json`:
- Around line 5-6: The chain identity currently only commits the genesis Block
fields, so peers can share a genesis hash while using different
target_block_time and alpha values and later diverge on difficulty validation.
Update the genesis identity/hash path to include these EMA parameters as part of
the committed genesis configuration, using the existing genesis Block/chain
identity generation flow so any change to target_block_time or alpha produces a
different network identity.
In `@minichain/chain.py`:
- Around line 137-139: The PoW validation in the block validation path only
compares the advertised difficulty and does not verify that block.hash actually
meets the target. Update the validation logic in the block-checking code
(including the current difficulty comparison in Chain’s block validation and the
other referenced validation path) to compute/compare the hash against the
difficulty target, and reject blocks whose hash does not satisfy
block.difficulty even if the header fields are self-consistent.
- Around line 174-181: Reject non-monotonic block timestamps before updating the
EMA in the difficulty adjustment logic. In the block-processing path that uses
self.last_block.timestamp, validate that the new block timestamp is strictly
greater than the previous block’s timestamp and discard/reject the block if not,
so time_diff never goes negative. Apply the same timestamp monotonicity check in
both affected difficulty-update sections (the one around the EMA update and the
corresponding duplicate path) before modifying self.avg_block_time or
self.current_difficulty.
- Around line 131-135: The status handling in chain.py is incorrectly inferred
from the ValueError message text in the validate_block_link_and_hash try/except
path, which misclassifies peer input like invalid index. Update the rejection
logic in the block validation flow around validate_block_link_and_hash to map
exceptions to ValidationStatus explicitly based on the actual failure type or
validation outcome, not by checking substrings in exc. Keep the logger.warning
in place, and ensure all invalid peer-supplied blocks return the appropriate
invalid status consistently.
- Around line 128-152: `Chain.add_block()` is no longer boolean-compatible, but
existing callers like `main.py` still rely on `if chain.add_block(...)`
semantics. Update `add_block()` so it preserves the old truthy/falsey contract
for success versus rejection, or ensure all callers are changed to compare
against `ValidationStatus.VALID`; use the `Chain.add_block` return path and the
`ValidationStatus` enum as the key points to fix.
In `@minichain/persistence.py`:
- Around line 285-288: `unban_peer()` and `get_banned_peers()` currently treat a
missing `data.db` as a successful no-op, which makes callers like `main.py`
report misleading success. Update the missing-storage path in these persistence
helpers to return an explicit status or raise a dedicated error instead of
silently returning an empty result, and then adjust the `main.py` caller
handling so it can distinguish “storage missing” from “peer unbanned” or “no
banned peers.” Focus on the `unban_peer` and `get_banned_peers` functions and
their call sites.
In `@minichain/state.py`:
- Around line 108-113: validate_and_apply_with_status() is double-running
transaction validation because it calls verify_transaction_logic(tx) and then
apply_transaction(tx), which verifies again. Refactor State by extracting the
mutation path into a prevalidated helper (for example, a private apply_* method
used by both apply_transaction and validate_and_apply_with_status) so
signature/logic checks happen only once. Update the acceptance/reorg replay flow
to use the prevalidated helper, and keep verify_transaction_logic as the single
validation entrypoint.
- Around line 57-60: The transaction validation in state.py is using tx.fee in
consensus accounting without first checking that it is present and non-negative.
Update the validation path around the balance check in the
transaction-processing logic (including the code that computes total_cost and
later gas_used) to reject any tx with a missing, non-numeric, or negative fee
before it can affect balances or rewards. Use the existing transaction
validation flow and symbols like tx.amount, getattr(tx, 'fee', 0), and the
validation methods around the referenced sections to apply the same fee guard
consistently wherever fee is consumed.
---
Outside diff comments:
In `@main.py`:
- Around line 95-100: The success check around add_block() is treating every
ValidationStatus as truthy, so rejected outcomes can still follow the success
path. Update the block-processing flow in main.py to explicitly compare the
result of chain.add_block(mined_block) against ValidationStatus.VALID, and only
perform mempool cleanup and block broadcast when that exact status is returned.
Use the add_block() call and ValidationStatus enum as the key symbols when
locating the fix.
In `@minichain/chain.py`:
- Around line 251-258: resolve_conflicts currently only checks receipt_root, so
it can accept a replayed block with tampered or stale receipts payloads; add an
explicit receipts payload validation in the reorg replay path. Update the
validation logic around calculate_receipt_root, add_block, and resolve_conflicts
so the reconstructed receipts list is compared against block.receipts before
accepting the new chain, and reject the reorg if they do not match.
🪄 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: 3d0772ce-56ab-4d82-b8b3-327422ef6d55
📒 Files selected for processing (10)
genesis.jsonmain.pyminichain/chain.pyminichain/persistence.pyminichain/pow.pyminichain/state.pyminichain/validators.pytests/test_difficulty.pytests/test_persistence.pytests/test_reorg.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.py (1)
95-100: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCompare
add_block()againstValidationStatus.VALID.ValidationStatusmembers are truthy, soINVALID/FAILED/MALFORMEDstill enter the success path here and can drop txs from the mempool and broadcast a rejected block.🤖 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 95 - 100, The success check around add_block() is treating every ValidationStatus as truthy, so rejected outcomes can still follow the success path. Update the block-processing flow in main.py to explicitly compare the result of chain.add_block(mined_block) against ValidationStatus.VALID, and only perform mempool cleanup and block broadcast when that exact status is returned. Use the add_block() call and ValidationStatus enum as the key symbols when locating the fix.minichain/chain.py (1)
251-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReorg replay skips the receipts payload check.
add_blockrejects mismatchedblock.receipts, butresolve_conflictsonly validatesreceipt_root. A heavier chain can therefore be accepted with forged or stale receipt payloads, andself.chain = new_chain_listpreserves that bad data.Suggested 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, [] if block.state_root != temp_state.state_root():🤖 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 251 - 258, resolve_conflicts currently only checks receipt_root, so it can accept a replayed block with tampered or stale receipts payloads; add an explicit receipts payload validation in the reorg replay path. Update the validation logic around calculate_receipt_root, add_block, and resolve_conflicts so the reconstructed receipts list is compared against block.receipts before accepting the new chain, and reject the reorg if they do not match.
🤖 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 `@genesis.json`:
- Around line 5-6: The chain identity currently only commits the genesis Block
fields, so peers can share a genesis hash while using different
target_block_time and alpha values and later diverge on difficulty validation.
Update the genesis identity/hash path to include these EMA parameters as part of
the committed genesis configuration, using the existing genesis Block/chain
identity generation flow so any change to target_block_time or alpha produces a
different network identity.
In `@minichain/chain.py`:
- Around line 137-139: The PoW validation in the block validation path only
compares the advertised difficulty and does not verify that block.hash actually
meets the target. Update the validation logic in the block-checking code
(including the current difficulty comparison in Chain’s block validation and the
other referenced validation path) to compute/compare the hash against the
difficulty target, and reject blocks whose hash does not satisfy
block.difficulty even if the header fields are self-consistent.
- Around line 174-181: Reject non-monotonic block timestamps before updating the
EMA in the difficulty adjustment logic. In the block-processing path that uses
self.last_block.timestamp, validate that the new block timestamp is strictly
greater than the previous block’s timestamp and discard/reject the block if not,
so time_diff never goes negative. Apply the same timestamp monotonicity check in
both affected difficulty-update sections (the one around the EMA update and the
corresponding duplicate path) before modifying self.avg_block_time or
self.current_difficulty.
- Around line 131-135: The status handling in chain.py is incorrectly inferred
from the ValueError message text in the validate_block_link_and_hash try/except
path, which misclassifies peer input like invalid index. Update the rejection
logic in the block validation flow around validate_block_link_and_hash to map
exceptions to ValidationStatus explicitly based on the actual failure type or
validation outcome, not by checking substrings in exc. Keep the logger.warning
in place, and ensure all invalid peer-supplied blocks return the appropriate
invalid status consistently.
- Around line 128-152: `Chain.add_block()` is no longer boolean-compatible, but
existing callers like `main.py` still rely on `if chain.add_block(...)`
semantics. Update `add_block()` so it preserves the old truthy/falsey contract
for success versus rejection, or ensure all callers are changed to compare
against `ValidationStatus.VALID`; use the `Chain.add_block` return path and the
`ValidationStatus` enum as the key points to fix.
In `@minichain/persistence.py`:
- Around line 285-288: `unban_peer()` and `get_banned_peers()` currently treat a
missing `data.db` as a successful no-op, which makes callers like `main.py`
report misleading success. Update the missing-storage path in these persistence
helpers to return an explicit status or raise a dedicated error instead of
silently returning an empty result, and then adjust the `main.py` caller
handling so it can distinguish “storage missing” from “peer unbanned” or “no
banned peers.” Focus on the `unban_peer` and `get_banned_peers` functions and
their call sites.
In `@minichain/state.py`:
- Around line 108-113: validate_and_apply_with_status() is double-running
transaction validation because it calls verify_transaction_logic(tx) and then
apply_transaction(tx), which verifies again. Refactor State by extracting the
mutation path into a prevalidated helper (for example, a private apply_* method
used by both apply_transaction and validate_and_apply_with_status) so
signature/logic checks happen only once. Update the acceptance/reorg replay flow
to use the prevalidated helper, and keep verify_transaction_logic as the single
validation entrypoint.
- Around line 57-60: The transaction validation in state.py is using tx.fee in
consensus accounting without first checking that it is present and non-negative.
Update the validation path around the balance check in the
transaction-processing logic (including the code that computes total_cost and
later gas_used) to reject any tx with a missing, non-numeric, or negative fee
before it can affect balances or rewards. Use the existing transaction
validation flow and symbols like tx.amount, getattr(tx, 'fee', 0), and the
validation methods around the referenced sections to apply the same fee guard
consistently wherever fee is consumed.
---
Outside diff comments:
In `@main.py`:
- Around line 95-100: The success check around add_block() is treating every
ValidationStatus as truthy, so rejected outcomes can still follow the success
path. Update the block-processing flow in main.py to explicitly compare the
result of chain.add_block(mined_block) against ValidationStatus.VALID, and only
perform mempool cleanup and block broadcast when that exact status is returned.
Use the add_block() call and ValidationStatus enum as the key symbols when
locating the fix.
In `@minichain/chain.py`:
- Around line 251-258: resolve_conflicts currently only checks receipt_root, so
it can accept a replayed block with tampered or stale receipts payloads; add an
explicit receipts payload validation in the reorg replay path. Update the
validation logic around calculate_receipt_root, add_block, and resolve_conflicts
so the reconstructed receipts list is compared against block.receipts before
accepting the new chain, and reject the reorg if they do not match.
🪄 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: 3d0772ce-56ab-4d82-b8b3-327422ef6d55
📒 Files selected for processing (10)
genesis.jsonmain.pyminichain/chain.pyminichain/persistence.pyminichain/pow.pyminichain/state.pyminichain/validators.pytests/test_difficulty.pytests/test_persistence.pytests/test_reorg.py
🛑 Comments failed to post (8)
genesis.json (1)
5-6: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Commit EMA parameters to the chain identity.
target_block_timeandalphanow affect difficulty validation, but the provided genesis hash path only hashes the genesisBlockfields. Peers can therefore share a genesis hash while using different EMA parameters and later disagree on block validity.🤖 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 `@genesis.json` around lines 5 - 6, The chain identity currently only commits the genesis Block fields, so peers can share a genesis hash while using different target_block_time and alpha values and later diverge on difficulty validation. Update the genesis identity/hash path to include these EMA parameters as part of the committed genesis configuration, using the existing genesis Block/chain identity generation flow so any change to target_block_time or alpha produces a different network identity.minichain/chain.py (4)
128-152: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C2 '\badd_block\s*\(' main.py tests minichainRepository: StabilityNexus/MiniChain
Length of output: 3583
Keep
add_block()boolean-compatiblemain.pystill usesif chain.add_block(...), so this return-type change breaks the existing contract unless every caller switches to an explicit== ValidationStatus.VALIDcheck. Otherwise rejected blocks can follow the success path and their transactions can be dropped from the mempool.🤖 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 128 - 152, `Chain.add_block()` is no longer boolean-compatible, but existing callers like `main.py` still rely on `if chain.add_block(...)` semantics. Update `add_block()` so it preserves the old truthy/falsey contract for success versus rejection, or ensure all callers are changed to compare against `ValidationStatus.VALID`; use the `Chain.add_block` return path and the `ValidationStatus` enum as the key points to fix.
131-135: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not derive block status from exception text.
"invalid index"currently returnsFAILEDonly because the message does not contain"hash". That is still invalid peer input, and any retry/blacklist logic built on these statuses will misclassify it.Suggested fix
try: validate_block_link_and_hash(self.last_block, block) except ValueError as exc: logger.warning("Block %s rejected: %s", block.index, exc) - return ValidationStatus.INVALID if "hash" in str(exc) else ValidationStatus.FAILED + return ValidationStatus.INVALID📝 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.try: validate_block_link_and_hash(self.last_block, block) except ValueError as exc: logger.warning("Block %s rejected: %s", block.index, exc) return ValidationStatus.INVALID🤖 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 131 - 135, The status handling in chain.py is incorrectly inferred from the ValueError message text in the validate_block_link_and_hash try/except path, which misclassifies peer input like invalid index. Update the rejection logic in the block validation flow around validate_block_link_and_hash to map exceptions to ValidationStatus explicitly based on the actual failure type or validation outcome, not by checking substrings in exc. Keep the logger.warning in place, and ensure all invalid peer-supplied blocks return the appropriate invalid status consistently.
137-139: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Validate the PoW target, not just the
difficultyfield.Both paths only compare the advertised difficulty value. Neither verifies that
block.hashactually satisfiesblock.difficulty, so a peer can submit any self-consistent header and bypass mining entirely.Suggested fix
+ if not block.hash.startswith("0" * block.difficulty): + logger.warning( + "Block %s rejected: hash does not satisfy difficulty %s", + block.index, + block.difficulty, + ) + return ValidationStatus.INVALID + if block.difficulty != self.current_difficulty: logger.warning("Block %s rejected: Invalid difficulty. Expected %s, got %s", block.index, self.current_difficulty, block.difficulty) return ValidationStatus.INVALIDAlso applies to: 228-230
🤖 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 137 - 139, The PoW validation in the block validation path only compares the advertised difficulty and does not verify that block.hash actually meets the target. Update the validation logic in the block-checking code (including the current difficulty comparison in Chain’s block validation and the other referenced validation path) to compute/compare the hash against the difficulty target, and reject blocks whose hash does not satisfy block.difficulty even if the header fields are self-consistent.
174-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-monotonic timestamps before feeding the EMA.
time_diffcan go negative here. With the current millisecond genesis timestamps, a child block built fromint(time.time())seconds is still accepted and produces a huge negative delta, letting peers skew difficulty arbitrarily.Suggested fix
+ if block.timestamp <= self.last_block.timestamp: + logger.warning( + "Block %s rejected: non-monotonic timestamp %s <= %s", + block.index, + block.timestamp, + self.last_block.timestamp, + ) + return ValidationStatus.INVALID + # Update EMA difficulty state time_diff = block.timestamp - self.last_block.timestamp self.avg_block_time = self.alpha * time_diff + (1 - self.alpha) * self.avg_block_timeAlso applies to: 260-266
🤖 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 174 - 181, Reject non-monotonic block timestamps before updating the EMA in the difficulty adjustment logic. In the block-processing path that uses self.last_block.timestamp, validate that the new block timestamp is strictly greater than the previous block’s timestamp and discard/reject the block if not, so time_diff never goes negative. Apply the same timestamp monotonicity check in both affected difficulty-update sections (the one around the EMA update and the corresponding duplicate path) before modifying self.avg_block_time or self.current_difficulty.minichain/persistence.py (1)
285-288: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Differentiate “missing storage” from a successful no-op.
unban_peer()returns silently whendata.dbis missing, andget_banned_peers()maps the same condition to[]. Inmain.py, that becomes “peer unbanned” / “none”, so operators get a success-looking result even when no blacklist state was read or changed. Please return an explicit status (or raise) for the missing-DB case so callers can report it correctly.Also applies to: 309-312
🤖 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/persistence.py` around lines 285 - 288, `unban_peer()` and `get_banned_peers()` currently treat a missing `data.db` as a successful no-op, which makes callers like `main.py` report misleading success. Update the missing-storage path in these persistence helpers to return an explicit status or raise a dedicated error instead of silently returning an empty result, and then adjust the `main.py` caller handling so it can distinguish “storage missing” from “peer unbanned” or “no banned peers.” Focus on the `unban_peer` and `get_banned_peers` functions and their call sites.minichain/state.py (2)
57-60: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Validate
feebefore using it in consensus accounting.
feeis added tototal_costand later used asgas_used, but onlyamountis checked. A negative fee can maketotal_costsmaller thanamountand corrupt balances/rewards.Proposed fix
from .validators import ValidationStatus + fee = getattr(tx, 'fee', 0) + if not isinstance(tx.amount, int) or tx.amount < 0 or not isinstance(fee, int) or fee < 0: + return ValidationStatus.MALFORMED if not tx.verify(): logger.error("Error: Invalid signature for tx from %s...", tx.sender[:8]) return ValidationStatus.INVALID @@ - total_cost = tx.amount + getattr(tx, 'fee', 0) + total_cost = tx.amount + fee @@ - 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 None @@ - 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 ValidationStatus.MALFORMED, NoneAlso applies to: 94-106, 120-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 57 - 60, The transaction validation in state.py is using tx.fee in consensus accounting without first checking that it is present and non-negative. Update the validation path around the balance check in the transaction-processing logic (including the code that computes total_cost and later gas_used) to reject any tx with a missing, non-numeric, or negative fee before it can affect balances or rewards. Use the existing transaction validation flow and symbols like tx.amount, getattr(tx, 'fee', 0), and the validation methods around the referenced sections to apply the same fee guard consistently wherever fee is consumed.
108-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid validating the same transaction twice.
validate_and_apply_with_status()verifies the transaction, thenapply_transaction()verifies it again. Split the mutation logic into a prevalidated helper so block acceptance/reorg replay do not double-run signature validation.Also applies to: 120-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() is double-running transaction validation because it calls verify_transaction_logic(tx) and then apply_transaction(tx), which verifies again. Refactor State by extracting the mutation path into a prevalidated helper (for example, a private apply_* method used by both apply_transaction and validate_and_apply_with_status) so signature/logic checks happen only once. Update the acceptance/reorg replay flow to use the prevalidated helper, and keep verify_transaction_logic as the single validation entrypoint.
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:
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Bug Fixes