fix: validate proof of work on block acceptance#115
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
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
| pass | ||
|
|
||
|
|
||
| def validate_difficulty(difficulty, max_difficulty): |
There was a problem hiding this comment.
Do we really need this validation function? Don't we know that we will always call this function with arguments that satisfy the conditions in line 18?
Also, some conditions in line 18 seem redundant anyway. For example, if difficulty is greater than len(block.hash), line 39 will fail anyway.
There was a problem hiding this comment.
Unless I'm overlooking something important, let's remove this validation function.
| raise ValueError(f"invalid difficulty {difficulty}") | ||
|
|
||
|
|
||
| def validate_block_link_and_hash(previous_block, block): |
There was a problem hiding this comment.
This function is now also checking difficulty, but its name is still suggesting that it checks only block link and hash.
Let's rename it to validate_block.
| raise ValueError(f"invalid hash {block.hash}") | ||
|
|
||
| validate_difficulty(block.difficulty, len(block.hash)) | ||
|
|
There was a problem hiding this comment.
In addition to the check in line 39, we must also check that block.difficulty is equal to the expected difficulty based on previous blocks.
There was a problem hiding this comment.
Otherwise, the miner can circumvent the difficulty check, by simply writing a small difficulty in block.difficulty.
There was a problem hiding this comment.
@Zahnentferner you're right. When I reviewed the attack was blocked by the separate difficulty checks in add_block and resolve_conflicts in the initial commit, but the architecture was flawed which I didn't notice since the checks already protected.
The difficulty check should have been moved inside the validator at the same time as the PoW check. That would have been the proper, complete fix. I focused too narrowly on, if the PoW check now exists or not.
The validation function and related issues came in later commits.
|
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
| for block in chain_list: | ||
| validate_difficulty(block.difficulty, len(block.hash)) | ||
|
|
||
| return sum(2 ** block.difficulty for block in chain_list) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject malformed hashes before measuring total work.
Block.from_dict() permits a missing hash, so an untrusted candidate can reach len(None). That raises TypeError, which resolve_conflicts() does not catch. Using the supplied hash length as the difficulty bound also accepts non-SHA256 lengths.
Proposed fix
for block in chain_list:
- validate_difficulty(block.difficulty, len(block.hash))
+ if not isinstance(block.hash, str) or len(block.hash) != 64:
+ raise ValueError("invalid block hash")
+ validate_difficulty(block.difficulty, 64)📝 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 block in chain_list: | |
| validate_difficulty(block.difficulty, len(block.hash)) | |
| return sum(2 ** block.difficulty for block in chain_list) | |
| for block in chain_list: | |
| if not isinstance(block.hash, str) or len(block.hash) != 64: | |
| raise ValueError("invalid block hash") | |
| validate_difficulty(block.difficulty, 64) | |
| return sum(2 ** block.difficulty for block in chain_list) |
🤖 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 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.
| mine_block(block, difficulty=bc.current_difficulty) | ||
| bc.add_block(block) | ||
| return bc |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the fixture block was accepted.
If add_block() regresses, this helper silently returns a genesis-only chain and the load test can still pass by comparing that chain with itself.
Proposed fix
mine_block(block, difficulty=bc.current_difficulty)
- bc.add_block(block)
+ result = bc.add_block(block)
+ self.assertEqual(result, ValidationStatus.VALID)
+ self.assertEqual(len(bc.chain), 2)
return bc📝 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.
| mine_block(block, difficulty=bc.current_difficulty) | |
| bc.add_block(block) | |
| return bc | |
| mine_block(block, difficulty=bc.current_difficulty) | |
| result = bc.add_block(block) | |
| self.assertEqual(result, ValidationStatus.VALID) | |
| self.assertEqual(len(bc.chain), 2) | |
| return bc |
🤖 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_persistence_runtime.py` around lines 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.
| block2 = Block( | ||
| index=len(restored.chain), | ||
| previous_hash=restored.last_block.hash, | ||
| transactions=[tx2], | ||
| difficulty=restored.current_difficulty, | ||
| state_root=temp_state.state_root(), | ||
| state_root=temp_state2.state_root(), | ||
| receipt_root=calculate_receipt_root([receipt2]), | ||
| receipts=[receipt2], | ||
| timestamp=restored.last_block.timestamp + 1000, | ||
| timestamp=int(time.time() * 1000) + 3000, | ||
| miner=new_pk, | ||
| ) | ||
| mine_block(block2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep replay fixtures independent of wall-clock time.
Both block builders derive timestamps from the test environment rather than the preceding block, making ordering and difficulty adjustment nondeterministic.
tests/test_persistence.py#L303-L314: userestored.last_block.timestamp + restored.target_block_time.tests/test_persistence_runtime.py#L70-L79: explicitly usebc.last_block.timestamp + bc.target_block_time.
📍 Affects 2 files
tests/test_persistence.py#L303-L314(this comment)tests/test_persistence_runtime.py#L70-L79
🤖 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_persistence.py` around lines 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.
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