PID difficulty adjuster with pure integer arithmetic#63
PID difficulty adjuster with pure integer arithmetic#63anshulchikhale30-p wants to merge 10 commits intoStabilityNexus:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR introduces PID-based difficulty adjustment to the blockchain. A new Changes
Sequence DiagramsequenceDiagram
participant Miner
participant PoW
participant Block
participant Blockchain
participant PIDAdj as PID Adjuster
Miner->>PoW: mine_block(block, difficulty, timeout)
PoW->>PoW: Iterate nonce until hash matches difficulty
PoW->>Block: Set mining_time = elapsed duration
PoW-->>Miner: Return mined block with mining_time
Miner->>Blockchain: add_block(block)
Blockchain->>Blockchain: Compute/assign block.difficulty
Blockchain->>Blockchain: Validate PoW: hash prefix matches difficulty
alt PoW Validation Fails
Blockchain-->>Miner: Reject block
else PoW Validation Passes
Blockchain->>PIDAdj: adjust(current_difficulty, block.mining_time)
PIDAdj->>PIDAdj: Compute PID terms (P, I, D)
PIDAdj->>PIDAdj: Clamp adjustment to ±10%
PIDAdj-->>Blockchain: Return new_difficulty
Blockchain->>Blockchain: Set current_difficulty = new_difficulty
Blockchain-->>Miner: Accept block
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
minichain/block.py (1)
108-115:⚠️ Potential issue | 🔴 CriticalTypeError:
Block.__init__does not acceptmining_timeparameter.The
Block.__init__method (lines 37-44) has signature(index, previous_hash, transactions, timestamp, difficulty)but line 114 passesmining_time=payload.get("mining_time"). This will raise:TypeError: __init__() got an unexpected keyword argument 'mining_time'Additionally,
to_dict()(lines 89-94) does not serializemining_time, so round-tripping a block throughto_dict()/from_dict()loses this field entirely.🐛 Add mining_time to Block class properly
Update
__init__to accept and storemining_time:def __init__( self, index: int, previous_hash: str, transactions: Optional[List[Transaction]] = None, timestamp: Optional[float] = None, difficulty: Optional[int] = None, + mining_time: Optional[float] = None, ): self.index = index self.previous_hash = previous_hash self.transactions: List[Transaction] = transactions or [] # ... existing code ... self.difficulty: Optional[int] = difficulty self.nonce: int = 0 self.hash: Optional[str] = None + self.mining_time: Optional[float] = mining_timeUpdate
to_dict()to includemining_time:def to_dict(self): return { **self.to_header_dict(), **self.to_body_dict(), "hash": self.hash, + "mining_time": getattr(self, 'mining_time', None), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/block.py` around lines 108 - 115, The Block construction is passing mining_time but Block.__init__ doesn't accept or store it and to_dict() doesn't emit it, causing a TypeError and data loss; update Block.__init__ (the constructor) to accept a mining_time: Optional[float] (with a sensible default like None) and assign it to self.mining_time, then update Block.to_dict() to include "mining_time": self.mining_time so that from_dict()/cls(...) round-trips correctly (no other call sites need changing since from_dict already passes mining_time).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/chain.py`:
- Around line 38-39: The constructor currently invokes _create_genesis_block()
twice, creating two genesis blocks; remove the duplicate call so
_create_genesis_block() is only called once during initialization (locate the
duplicate invocations of _create_genesis_block in the Chain class or __init__
and delete the extra call), and optionally make _create_genesis_block()
idempotent or guard it with a check that the chain is empty before appending to
prevent future regressions.
- Around line 107-118: The difficulty_adjuster.adjust call is currently inside
the per-transaction loop (and duplicated elsewhere), causing multiple PID
updates per block; move the call to
difficulty_adjuster.adjust(self.current_difficulty, block.mining_time if
hasattr(block, 'mining_time') else None) out of the for tx in block.transactions
loop so it runs exactly once per accepted block, and remove the redundant
adjustment (the earlier/other adjust call) so only the single post-validation
update in the block-accept path updates self.current_difficulty and the PID
state.
- Around line 75-95: The PID and PoW checks need fixing: don't let
difficulty_adjuster.adjust(...) mutate PID state before the block is fully
validated — call adjust in a non-mutating way (e.g., use a pure compute method,
pass a copy of the PID state, or make adjust return (new_difficulty,
new_pid_state) and only commit the pid state when the block passes validation)
so that failed PoW validations do not corrupt integral/previous_error; and fix
the PoW target computation to match pow.py by using target_prefix = '0' *
difficulty (not '0' * (difficulty // 256 + 1)) when validating block.hash.
Ensure you update locations referencing difficulty_adjuster.adjust,
block.difficulty assignment, and the PoW check logic so state is only updated
after the hash check passes.
In `@minichain/pid.py`:
- Around line 120-127: Remove the redundant int() cast and unreachable branch:
assign delta directly from clamped_adjustment (delta = clamped_adjustment) and
drop the conditional that attempts to force ±1 when clamped_adjustment != 0
(since clamped_adjustment is already an int), then compute new_difficulty =
current_difficulty + delta and return max(1, new_difficulty); update the block
using the variables clamped_adjustment, delta and current_difficulty in the
PID/difficulty adjustment function in pid.py.
In `@minichain/pow.py`:
- Around line 60-65: Remove the redundant success log: delete the earlier
logger.info("Success! Hash: %s", block_hash) so only the later
logger.info("Success! Hash: %s, Mining time: %.2fs", block_hash,
block.mining_time) remains; the change should be made in the mining routine
where block_hash, start_time and block.mining_time are used (retain computation
of block.mining_time = time.monotonic() - start_time and the final logger call).
In `@test_pid_integration.py`:
- Around line 146-155: Replace uses of the interactive-only exit() calls in the
__main__ block that runs test_pid_integration() with sys.exit() for script-safe
termination; add an import sys at the top of the file if not present, and update
both exit(0 if success else 1) and exit(1) in the exception handler to
sys.exit(...) so the script exits reliably from test_pid_integration() failures
and successes.
- Around line 75-80: The printed expectation messages are inverted: when
mined_block1.mining_time < 10 (block mined faster than target) the code
currently prints "Expected: Difficulty should DECREASE ↓" but the PID
(actual_block_time < target → positive error → increases difficulty) and
protocol require difficulty to INCREASE; change the message for the fast case to
"Expected: Difficulty should INCREASE ↑" and switch the slow-case message
(mined_block1.mining_time >= 10) to "Expected: Difficulty should DECREASE ↓".
Update the two print statements that reference mined_block1.mining_time to
reflect these corrected directions.
In `@tests/test_difficulty.py`:
- Around line 366-369: The loop variable named "time" shadows the imported time
module; rename the loop variable (e.g., to "t" or "elapsed") wherever it's used
in the loop so calls like adjuster.adjust(difficulty, t) (and the changes.append
line) use the new name, and ensure any other references to the module `time`
outside/inside the loop still refer to the module, not the loop variable.
- Around line 410-412: The script's top-level main block calls exit() which is
less reliable; replace the call to exit(0 if success else 1) with sys.exit(0 if
success else 1) in the __main__ block where run_tests() is invoked, and add an
import sys at the module top if it's not already present so the symbol sys is
available.
---
Outside diff comments:
In `@minichain/block.py`:
- Around line 108-115: The Block construction is passing mining_time but
Block.__init__ doesn't accept or store it and to_dict() doesn't emit it, causing
a TypeError and data loss; update Block.__init__ (the constructor) to accept a
mining_time: Optional[float] (with a sensible default like None) and assign it
to self.mining_time, then update Block.to_dict() to include "mining_time":
self.mining_time so that from_dict()/cls(...) round-trips correctly (no other
call sites need changing since from_dict already passes mining_time).
🪄 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: 11254c34-1d7e-4ed1-9c7d-29e2f275373f
📒 Files selected for processing (6)
minichain/block.pyminichain/chain.pyminichain/pid.pyminichain/pow.pytest_pid_integration.pytests/test_difficulty.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Implements dynamic difficulty adjustment using PID controller
to maintain target block time with 100% deterministic behavior.
Changes
Add PIDDifficultyAdjuster with integer-only math
Integrate into blockchain difficulty adjustment
Track mining_time in blocks
Replace float * 0.1 with integer // 10
Add 60+ unit tests
Key Fix
Addresses PR #52 feedback:
Files Changed
Testing
All tests pass
Integration verified
Only 5 files changed
Summary by CodeRabbit
Release Notes
New Features
Tests