refactor: change PoW difficulty to numeric target hash threshold - #133
refactor: change PoW difficulty to numeric target hash threshold#133SIDDHANTCOOKIE wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces difficulty-based proof of work with bounded numeric targets across block serialization, mining, validation, retargeting, genesis configuration, and chain reorganization. Tests and mining configuration now use the target model. ChangesTarget-Based Proof-of-Work Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant main.py
participant mine_block
participant Blockchain
main.py->>mine_block: pass current_target
mine_block->>mine_block: find hash below target
mine_block-->>main.py: return mined block
main.py->>Blockchain: add block
Blockchain->>Blockchain: validate and update target state
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@minichain/chain.py`:
- Around line 30-31: Enforce target validation at both consensus entry points in
minichain/chain.py: lines 30-31, validate that block.target is an integer within
the valid range before comparing the hash; lines 90-95, validate the genesis
target before assigning self.current_target. Reject non-integer, non-positive,
or MAX_TARGET-exceeding targets consistently at both sites.
In `@minichain/pow.py`:
- Around line 26-31: Update the mining flow around target selection and
block.to_header_dict() so an override target is committed to the mined
block/header before returning success. Ensure the hash is computed against the
same target that validation will inspect; alternatively reject overrides that
differ from block.target before mining.
In `@tests/test_persistence_runtime.py`:
- Around line 74-79: Update the block construction and mining call in the
persistence runtime test to use the chain’s configured target from the
Blockchain instance instead of hard-coded int("F"*64, 16) values. Reuse the
target exposed by bc so the test block matches the target enforced by
bc.add_block().
In `@tests/test_target.py`:
- Around line 13-14: Keep genesis block hashes consistent with their targets in
tests/test_target.py at lines 13-14, 37-38, and 44-45: initialize each chain
from target-configured genesis data or recompute the corresponding genesis block
hash after changing its target, applying the same correction to the chains in
each site.
- Line 54: Update the test call to chain1.resolve_conflicts so it does not bind
the unused orphan result: unpack it into success and _, unless the test contract
requires verifying that the orphan list is empty, in which case assert that
condition explicitly.
🪄 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 Plus
Run ID: 68b9b698-3641-49e3-a618-bca450af0881
📒 Files selected for processing (13)
genesis.jsonmain.pyminichain/block.pyminichain/chain.pyminichain/network_config.pyminichain/pow.pytests/test_core.pytests/test_persistence.pytests/test_persistence_runtime.pytests/test_protocol_hardening.pytests/test_reorg.pytests/test_serialization.pytests/test_target.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@minichain/chain.py`:
- Around line 96-102: In minichain/chain.py lines 96-102, update target parsing
to accept only non-boolean integers or valid hexadecimal strings, reject
booleans and floats without coercion, and catch malformed conversion values
before logging and exiting. In minichain/chain.py lines 30-34, update the
received block target validation to explicitly exclude booleans while preserving
non-boolean integer validation and existing bounds checks.
In `@minichain/pow.py`:
- Around line 26-32: Keep the target override local throughout the mining
operation and assign it to block.target only after mining succeeds; do not
mutate the input before timeout, cancellation, or max-nonce failures. Extend
target validation to reject values above MAX_TARGET while preserving the
positive-integer requirement. Ensure header construction and hashing use the
validated candidate target, and leave block.target unchanged on every failure
path.
In `@minichain/state.py`:
- Around line 189-204: Wrap the journaled transaction execution in the relevant
state-processing method, including the sender debit, nonce increment, and
downstream execution, with exception handling that calls the existing
rollback_and_refund() and restores original_accounts before propagating the
exception or creating a failure receipt. Ensure no unexpected exception leaves
self.accounts pointing to the StateJournal.
🪄 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 Plus
Run ID: 658aca11-589d-42c0-8ab5-d512605d902d
📒 Files selected for processing (5)
minichain/chain.pyminichain/pow.pyminichain/state.pytests/test_persistence_runtime.pytests/test_target.py
| raw_target = config.get("target") | ||
| if raw_target is not None: | ||
| self.current_target = int(raw_target, 16) if isinstance(raw_target, str) else int(raw_target) | ||
| from .network_config import MAX_TARGET | ||
| if not isinstance(self.current_target, int) or self.current_target <= 0 or self.current_target > MAX_TARGET: | ||
| logger.error("Genesis target out of bounds: %s", self.current_target) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parse protocol targets strictly before validating bounds. int(raw_target) silently converts values such as true and 1.9, while isinstance(block.target, int) also accepts True. Restrict targets to non-boolean integers or valid hex strings, and handle conversion failures consistently.
minichain/chain.py#L96-L102: reject booleans/floats instead of coercing them, and catch malformed hex or numeric values before exiting.minichain/chain.py#L30-L34: explicitly reject booleans when validating a received block target.
📍 Affects 1 file
minichain/chain.py#L96-L102(this comment)minichain/chain.py#L30-L34
🤖 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 96 - 102, In minichain/chain.py lines
96-102, update target parsing to accept only non-boolean integers or valid
hexadecimal strings, reject booleans and floats without coercion, and catch
malformed conversion values before logging and exiting. In minichain/chain.py
lines 30-34, update the received block target validation to explicitly exclude
booleans while preserving non-boolean integer validation and existing bounds
checks.
| target = target if target is not None else block.target | ||
| if not isinstance(target, int) or target <= 0: | ||
| raise ValueError("Target must be a positive integer.") | ||
| block.target = target | ||
|
|
||
| target = "0" * difficulty | ||
| local_nonce = 0 | ||
| header_dict = block.to_header_dict() # Construct header dict once outside loop |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep target overrides atomic and consensus-valid.
Line 29 mutates the input before timeout/cancellation/max-nonce failure, contradicting the documented contract; a later retry can mine with the stale override. Also reject targets above MAX_TARGET, otherwise mining can succeed for a block add_block must reject.
Proposed fix
def mine_block(...):
+ from .network_config import MAX_TARGET
+
target = target if target is not None else block.target
- if not isinstance(target, int) or target <= 0:
- raise ValueError("Target must be a positive integer.")
- block.target = target
+ if isinstance(target, bool) or not isinstance(target, int) or not 0 < target <= MAX_TARGET:
+ raise ValueError("Target must be an integer within consensus bounds.")
local_nonce = 0
- header_dict = block.to_header_dict()
+ header_dict = block.to_header_dict()
+ header_dict["target"] = hex(target)
...
if int(block_hash, 16) < target:
+ block.target = target
block.nonce = local_nonce📝 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.
| target = target if target is not None else block.target | |
| if not isinstance(target, int) or target <= 0: | |
| raise ValueError("Target must be a positive integer.") | |
| block.target = target | |
| target = "0" * difficulty | |
| local_nonce = 0 | |
| header_dict = block.to_header_dict() # Construct header dict once outside loop | |
| from .network_config import MAX_TARGET | |
| target = target if target is not None else block.target | |
| if isinstance(target, bool) or not isinstance(target, int) or not 0 < target <= MAX_TARGET: | |
| raise ValueError("Target must be an integer within consensus bounds.") | |
| local_nonce = 0 | |
| header_dict = block.to_header_dict() | |
| header_dict["target"] = hex(target) # Construct header dict once outside loop |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 28-28: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/pow.py` around lines 26 - 32, Keep the target override local
throughout the mining operation and assign it to block.target only after mining
succeeds; do not mutate the input before timeout, cancellation, or max-nonce
failures. Extend target validation to reject values above MAX_TARGET while
preserving the positive-integer requirement. Ensure header construction and
hashing use the validated candidate target, and leave block.target unchanged on
every failure path.
| original_accounts = self.accounts | ||
| journal = StateJournal(original_accounts) | ||
| self.accounts = journal | ||
|
|
||
| sender = self.accounts[tx.sender] | ||
| total_cost = tx.amount + (getattr(tx, 'gas_limit', 0) * getattr(tx, 'fee_per_gas', 0)) | ||
|
|
||
| sender['balance'] -= total_cost | ||
| sender['nonce'] += 1 | ||
|
|
||
| import copy | ||
| state_snapshot = copy.deepcopy(self.accounts) | ||
|
|
||
| def rollback_and_refund(error_message, gas_used): | ||
| self.accounts = copy.deepcopy(state_snapshot) | ||
| journal.rollback() | ||
| self.accounts = original_accounts | ||
| refund_acc = self.accounts[tx.sender] | ||
| refund_acc['balance'] += tx.amount | ||
| gas_refund = getattr(tx, 'gas_limit', 0) - gas_used | ||
| if gas_refund > 0: | ||
| refund_acc['balance'] += (gas_refund * getattr(tx, 'fee_per_gas', 0)) | ||
| refund_acc['balance'] -= (gas_used * getattr(tx, 'fee_per_gas', 0)) | ||
| refund_acc['nonce'] += 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore state when execution raises unexpectedly.
An exception after Lines 193-197 bypasses rollback_and_refund(), leaving self.accounts set to journal; its cached sender debit and nonce increment remain visible despite the failed transaction. Wrap the journaled body in exception handling that rolls back and restores original_accounts before propagating or converting the failure to a receipt.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 199-199: Missing return type annotation for private function rollback_and_refund
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/state.py` around lines 189 - 204, Wrap the journaled transaction
execution in the relevant state-processing method, including the sender debit,
nonce increment, and downstream execution, with exception handling that calls
the existing rollback_and_refund() and restores original_accounts before
propagating the exception or creating a failure receipt. Ensure no unexpected
exception leaves self.accounts pointing to the StateJournal.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/pow.py (1)
36-51: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winBound the full nonce search window to the configured range.
The config comment states that
MINING_INITIAL_NONCE_MINandMINING_INITIAL_NONCE_MAXcan create unique non-overlapping ranges, butlocal_noncecan reachseed + MINING_MAX_NONCE - 1. That can hash outside the configured initial range and reopen duplicate work. Enforce the upper bound on the search window instead of only onstart_nonce.🤖 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/pow.py` around lines 36 - 51, Update the nonce-bound logic in the mining loop to cap the effective search window at MINING_INITIAL_NONCE_MAX, rather than allowing local_nonce to advance beyond the configured initial range via max_nonce. Preserve the existing start_nonce-based limit while ensuring hashing never occurs outside the selected non-overlapping range.
🤖 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 `@minichain/pow.py`:
- Line 2: Update the random starting-point generation in the PoW logic around
the random import and lines 36–37 to use secrets.randbelow when unpredictability
is required, replacing random.randint and its import. If unpredictability is
intentionally not required, retain the existing source but add a narrowly scoped
Ruff S311 suppression with a rationale and document that it only reduces
overlap.
---
Outside diff comments:
In `@minichain/pow.py`:
- Around line 36-51: Update the nonce-bound logic in the mining loop to cap the
effective search window at MINING_INITIAL_NONCE_MAX, rather than allowing
local_nonce to advance beyond the configured initial range via max_nonce.
Preserve the existing start_nonce-based limit while ensuring hashing never
occurs outside the selected non-overlapping range.
🪄 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 Plus
Run ID: 6d39914d-a3b6-43a5-987c-217251f87b74
📒 Files selected for processing (3)
Roadmap.mdminichain/node_config.pyminichain/pow.py
💤 Files with no reviewable changes (1)
- Roadmap.md
| @@ -1,6 +1,11 @@ | |||
| import time | |||
| import random | |||
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Resolve the random-source contract and Ruff S311 finding.
random.randint is predictable. This does not invalidate a PoW hash, but it does not provide an unpredictable starting point for the overlap-reduction goal. If unpredictability is required, use secrets.randbelow. Otherwise, suppress S311 with a rationale and document that this only reduces overlap.
Proposed fix
-import random
+import secrets
...
- start_nonce = random.randint(MINING_INITIAL_NONCE_MIN, MINING_INITIAL_NONCE_MAX)
+ start_nonce = MINING_INITIAL_NONCE_MIN + secrets.randbelow(
+ MINING_INITIAL_NONCE_MAX - MINING_INITIAL_NONCE_MIN + 1
+ )Also applies to: 36-37
🤖 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/pow.py` at line 2, Update the random starting-point generation in
the PoW logic around the random import and lines 36–37 to use secrets.randbelow
when unpredictability is required, replacing random.randint and its import. If
unpredictability is intentionally not required, retain the existing source but
add a narrowly scoped Ruff S311 suppression with a rationale and document that
it only reduces overlap.
Source: Linters/SAST tools
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 (1)
minichain/chain.py (1)
146-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject invalid targets before calculating cumulative work.
get_total_workprocesses candidate blocks before_apply_blockvalidates them. Theor 1fallback maps target0orFalseto1, which grants the candidate maximum work. A non-numeric target can raiseTypeErrorand abortresolve_conflicts.Validate every target before comparing chain work. Remove the fallback and reject invalid candidates.
🤖 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 146 - 151, Update get_total_work to validate each block.target before performing cumulative-work arithmetic, rejecting zero, false, and other invalid targets instead of converting them to 1. Remove the “or 1” fallback, and ensure resolve_conflicts handles the resulting invalid candidate without aborting.
🤖 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 `@minichain/chain.py`:
- Around line 146-151: Update get_total_work to validate each block.target
before performing cumulative-work arithmetic, rejecting zero, false, and other
invalid targets instead of converting them to 1. Remove the “or 1” fallback, and
ensure resolve_conflicts handles the resulting invalid candidate without
aborting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd361269-a6fe-4808-b58f-7c0f29fe0ba8
📒 Files selected for processing (1)
minichain/chain.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/chain.py (1)
299-304: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCommit account storage only after the full reorganization validates.
_apply_blockmutatestemp_statebefore it can return an invalid status. Lines 302-304 commit those mutations before Line 305 checks the status. A rejected reorganization can therefore persist partial account changes. Keep the temporary account state isolated for the full loop, then commit only after every block is valid.🤖 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 299 - 304, The reorganization loop commits account mutations immediately after each _apply_block call, before validating the returned status. Remove the per-block commit and backing reassignment from the loop, keep temp_state.accounts isolated throughout all _apply_block iterations, and commit the temporary account storage only after every block has returned a valid status.
🤖 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 `@minichain/chain.py`:
- Around line 156-162: Validate genesis configuration’s target_block_time before
it reaches the proportional retargeting calculation in the chain
initialization/loading flow: reject booleans, non-integer values, and values
less than or equal to zero. Ensure only a positive integer is stored and used by
the new_target computation, preventing division by zero and fractional targets.
- Around line 272-278: Update the fast PoW validation loop in the reorg handling
code to first require each b.hash to be a string, then safely convert it from
hexadecimal while handling conversion errors. For missing or malformed hashes,
log the existing invalid-target/hash validation failure and return (False, [])
instead of allowing int(b.hash, 16) to raise.
In `@tests/test_target.py`:
- Around line 14-38: Update the retarget expectations in the affected test to
match the stated additive rule: expect start_target - 1 after the fast block and
expected_target_fast + 1 after the slow block, including the repeated assertions
noted in the comment. Modify Blockchain._next_target to apply a ±1 change based
on average block time instead of proportional scaling.
---
Outside diff comments:
In `@minichain/chain.py`:
- Around line 299-304: The reorganization loop commits account mutations
immediately after each _apply_block call, before validating the returned status.
Remove the per-block commit and backing reassignment from the loop, keep
temp_state.accounts isolated throughout all _apply_block iterations, and commit
the temporary account storage only after every block has returned a valid
status.
🪄 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 Plus
Run ID: b9349c2a-c355-459e-b601-88027380db5a
📒 Files selected for processing (3)
minichain/block.pyminichain/chain.pytests/test_target.py
|
|
||
| # Proportional difficulty adjustment: | ||
| # If blocks are too slow (avg_block_time > target_block_time), the target INCREASES (easier) | ||
| # If blocks are too fast (avg_block_time < target_block_time), the target DECREASES (harder) | ||
| new_target = (target * int(avg_block_time)) // self.target_block_time | ||
|
|
||
| return max(MIN_TARGET, min(MAX_TARGET, new_target)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate target_block_time before proportional retargeting.
If genesis config sets target_block_time to zero, Line 160 raises ZeroDivisionError. If it is a float, Line 160 can produce a non-integer target. Reject boolean, non-integer, and non-positive values when loading genesis config.
🤖 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 156 - 162, Validate genesis configuration’s
target_block_time before it reaches the proportional retargeting calculation in
the chain initialization/loading flow: reject booleans, non-integer values, and
values less than or equal to zero. Ensure only a positive integer is stored and
used by the new_target computation, preventing division by zero and fractional
targets.
| for b in new_chain_list: | ||
| if not isinstance(b.target, int) or b.target <= 0 or b.target > MAX_TARGET: | ||
| logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid target)", b.index) | ||
| return False, [] | ||
| if int(b.hash, 16) >= b.target: | ||
| logger.warning("Reorg failed: Fast PoW check failed for block %s (hash >= target)", b.index) | ||
| return False, [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject malformed hashes without raising.
int(b.hash, 16) raises for None or malformed values. Block.from_dict in minichain/block.py Lines 157-162 permits a missing hash, so a peer can make conflict resolution fail before _apply_block returns an invalid status. Validate that b.hash is a string and catch conversion errors before this comparison.
Proposed fix
for b in new_chain_list:
if not isinstance(b.target, int) or b.target <= 0 or b.target > MAX_TARGET:
logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid target)", b.index)
return False, []
- if int(b.hash, 16) >= b.target:
+ try:
+ hash_value = int(b.hash, 16)
+ except (TypeError, ValueError):
+ logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid hash)", b.index)
+ return False, []
+ if hash_value >= b.target:
logger.warning("Reorg failed: Fast PoW check failed for block %s (hash >= target)", b.index)
return False, []📝 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.
| for b in new_chain_list: | |
| if not isinstance(b.target, int) or b.target <= 0 or b.target > MAX_TARGET: | |
| logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid target)", b.index) | |
| return False, [] | |
| if int(b.hash, 16) >= b.target: | |
| logger.warning("Reorg failed: Fast PoW check failed for block %s (hash >= target)", b.index) | |
| return False, [] | |
| for b in new_chain_list: | |
| if not isinstance(b.target, int) or b.target <= 0 or b.target > MAX_TARGET: | |
| logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid target)", b.index) | |
| return False, [] | |
| try: | |
| hash_value = int(b.hash, 16) | |
| except (TypeError, ValueError): | |
| logger.warning("Reorg failed: Fast PoW check failed for block %s (invalid hash)", b.index) | |
| return False, [] | |
| if hash_value >= b.target: | |
| logger.warning("Reorg failed: Fast PoW check failed for block %s (hash >= target)", b.index) | |
| return False, [] |
🤖 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 272 - 278, Update the fast PoW validation
loop in the reorg handling code to first require each b.hash to be a string,
then safely convert it from hexadecimal while handling conversion errors. For
missing or malformed hashes, log the existing invalid-target/hash validation
failure and return (False, []) instead of allowing int(b.hash, 16) to raise.
| # Start with a target comfortably in the middle | ||
| start_target = MAX_TARGET // 2 | ||
| chain.current_target = start_target | ||
| chain.chain[0].target = start_target | ||
| chain.chain[0].hash = chain.chain[0].compute_hash() | ||
|
|
||
| # Fast mining: timestamps only 1ms apart | ||
| # avg = 0.5 * 1 + 0.5 * 1000 = 500.5 (which truncates to 500 in integer ops if needed, but in Python it's a float) | ||
| # new_target = (start_target * int(500.5)) // 1000 = (start_target * 500) // 1000 = start_target // 2 | ||
| ts = chain.last_block.timestamp + 1 | ||
| block1 = Block(index=1, previous_hash=chain.last_block.hash, transactions=[], timestamp=ts, target=chain.current_target, state_root=chain.state.state_root()) | ||
| mined_block1 = mine_block(block1) | ||
| self.assertEqual(chain.add_block(mined_block1), ValidationStatus.VALID) | ||
| expected_target_fast = (start_target * 500) // 1000 | ||
| self.assertEqual(chain.current_target, expected_target_fast) | ||
|
|
||
| # Slow mining: timestamp 5000ms apart | ||
| # avg = 0.5 * 5000 + 0.5 * 500.5 = 2750.25 | ||
| # new_target = (expected_target_fast * int(2750.25)) // 1000 = (expected_target_fast * 2750) // 1000 | ||
| ts = chain.last_block.timestamp + 5000 | ||
| block2 = Block(index=2, previous_hash=chain.last_block.hash, transactions=[], timestamp=ts, target=chain.current_target, state_root=chain.state.state_root()) | ||
| mined_block2 = mine_block(block2) | ||
| self.assertEqual(chain.add_block(mined_block2), ValidationStatus.VALID) | ||
| expected_target_slow = (expected_target_fast * 2750) // 1000 | ||
| self.assertEqual(chain.current_target, expected_target_slow) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align target expectations with the stated retarget rule.
Lines 27 and 37 expect proportional target changes. Lines 63-64 repeat the same expectation. The PR objective specifies a target change of ±1 from the average block time.
This test currently accepts multiplicative consensus retargeting. Update these expectations to start_target - 1 after the fast block and expected_target_fast + 1 after the slow block. Update Blockchain._next_target in the same change.
Proposed test changes
- expected_target_fast = (start_target * 500) // 1000
+ expected_target_fast = start_target - 1
...
- expected_target_slow = (expected_target_fast * 2750) // 1000
+ expected_target_slow = expected_target_fast + 1Also applies to: 63-64
🤖 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_target.py` around lines 14 - 38, Update the retarget expectations
in the affected test to match the stated additive rule: expect start_target - 1
after the fast block and expected_target_fast + 1 after the slow block,
including the repeated assertions noted in the comment. Modify
Blockchain._next_target to apply a ±1 change based on average block time instead
of proportional scaling.
| with self._lock: | ||
| chain_list = self.chain | ||
| return sum(2 ** (block.difficulty or 1) for block in chain_list) | ||
| return sum((1 << 256) // (block.target or 1) for block in chain_list) |
There was a problem hiding this comment.
Write a comment explaining what this does.
| Validates `block` against `prev_block` and applies its transactions to `state` | ||
| (mutated in place). On any non-VALID status the caller must discard `state`. | ||
| Returns: (ValidationStatus, new_difficulty, new_avg_block_time) | ||
| Returns: (ValidationStatus, new_target, new_avg_block_time) |
There was a problem hiding this comment.
Is a new_avg_block_time being computed? I don't see it.
Potential critical bug.
| transactions: Optional[Sequence[Transaction]] = None, | ||
| timestamp: Optional[float] = None, | ||
| difficulty: Optional[int] = None, | ||
| target: Optional[int] = None, |
There was a problem hiding this comment.
A target is not Optional. Every block must have have a target.
Every dev goes through a phase of abusing the use of Optional. This leads to anti-patterns in the code, where one needs to be explicitly unwrapping Optional, checking for None and handling None in ad-hoc ways. I have the impression that this is happening to you currently.
Inspect every use of Optional (not only for target and not only for Block) and ask yourself whether you really need it.
For example, for transactions, one could use an empty sequence of transactions, instead of None.
Often special cases can be handled by using a natural default value instead of None.
2 participants
Addressed Issues:
Replaces the leading-zero
difficultymechanic with a granulartargethash threshold for Proof of Work validation.int(hash, 16) < targetMAX_TARGETinnetwork_config.pyfor modularityScreenshots/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