fix: validate proof of work on block acceptance#115
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughStrengthens Proof-of-Work and difficulty validation, applies those checks during block acceptance and conflict resolution, and changes persistence loading to replay blocks through ChangesConsensus and Persistence Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Snapshot
participant persistence.load
participant Blockchain
participant State
Snapshot->>persistence.load: provide persisted accounts and blocks
persistence.load->>Blockchain: validate genesis and initialize replay state
persistence.load->>Blockchain: apply each non-genesis block
Blockchain->>State: validate and apply block state changes
Blockchain-->>persistence.load: return status and updated difficulty
persistence.load-->>Snapshot: return reconstructed blockchain
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 1
🤖 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 28-31: The PoW validation in block handling is currently tied to
fragile string matching in add_block, where ValueError classification depends on
whether the message contains “hash.” Update the proof-of-work check in the block
validation path to raise a distinct error type or carry an explicit reason/code
from the PoW check, and update add_block to classify based on that symbol
instead of substring inspection. Use the existing block validation logic and
add_block flow to keep INVALID vs FAILED handling stable even if the error
wording changes.
🪄 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: 3f2bc44e-a4f5-4c11-a81e-8a5cdfd08fec
📒 Files selected for processing (2)
minichain/chain.pytests/test_pow_validation.py
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
7a7f3c1 to
9423173
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 32-34: The PoW validation in validate_block_link_and_hash() uses
block.difficulty directly, which can cause type coercion issues or expensive
string allocation before the later checks in _apply_block(). Add a difficulty
sanity check in validate_block_link_and_hash() using the existing
block.difficulty and InvalidProofOfWorkError path so add_block() rejects
non-integer, boolean, negative, or otherwise invalid difficulty values early and
consistently.
🪄 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: ada65b4d-f692-4efc-9c72-48ee7c94821e
📒 Files selected for processing (2)
minichain/chain.pytests/test_pow_validation.py
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
…all-clock Fixes non-deterministic difficulty drift: using time.time() created a multi-year gap from genesis's fixed timestamp, causing EMA difficulty adjustment to swing unpredictably run-to-run. Anchoring to last_block.timestamp + target_block_time matches the pattern already used in test_difficulty.py and keeps difficulty stable and deterministic.
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 151-154: Update the chain validation loop before the total-work
calculation to validate each block’s hash as a required SHA256-length value,
rejecting missing or malformed hashes before calling validate_difficulty. Avoid
using len(block.hash) as the difficulty bound; use the established SHA256
hash-length constraint, while preserving the existing work summation for valid
chains.
In `@tests/test_persistence_runtime.py`:
- Around line 80-82: Update the blockchain fixture helper around mine_block and
bc.add_block to assert that add_block accepts the mined fixture block before
returning bc. Keep returning the populated chain only after the acceptance
assertion succeeds, so failures cannot produce a silently genesis-only fixture.
In `@tests/test_persistence.py`:
- Around line 303-314: Make replay fixture timestamps deterministic by deriving
each new block timestamp from the preceding block and target interval. In
tests/test_persistence.py at lines 303-314, update block2’s timestamp to use
restored.last_block.timestamp plus restored.target_block_time; in
tests/test_persistence_runtime.py at lines 70-79, update the corresponding block
builder to use bc.last_block.timestamp plus bc.target_block_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: d0a92967-fff0-4f50-8244-ca8210158181
📒 Files selected for processing (5)
minichain/chain.pyminichain/persistence.pytests/test_persistence.pytests/test_persistence_runtime.pytests/test_pow_validation.py
✅ Action performedReview finished.
|
3 participants
Addressed Issues:
Received blocks were checked for linkage and hash consistency, but their hashes were not verified against the required Proof-of-Work difficulty.
This PR adds PoW validation to
validate_block_link_and_hash(). Since bothadd_block()andresolve_conflicts()use this shared validator, invalid-PoW blocks are now rejected during both normal block acceptance and chain reorganization.Screenshots/Recordings:
Not applicable.
Additional Notes:
Added regression tests covering both affected paths:
add_block()rejects a block whose hash does not satisfy its claimed difficulty.resolve_conflicts()rejects a candidate chain containing a block with invalid PoW.The regression tests fail without the PoW validation check and pass with the fix.
Testing:
python -m pytest tests/test_pow_validation.py -v: 2 passedpython -m pytest: 75 passed, 1 pre-existing failure inTestSmartContract.test_out_of_gasOut of gas!Execution timed outorigin/mainAI Usage Disclosure:
I have used the following AI models and tools: ChatGPT and Claude for reviewing the changes and refining the tests.
Checklist
Summary by CodeRabbit