-
-
Notifications
You must be signed in to change notification settings - Fork 18
fix: validate proof of work on block acceptance #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,15 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class InvalidProofOfWorkError(ValueError): | ||
| pass | ||
|
|
||
|
|
||
| def validate_difficulty(difficulty, max_difficulty): | ||
| if type(difficulty) is not int or difficulty < 1 or difficulty > max_difficulty: | ||
| raise ValueError(f"invalid difficulty {difficulty}") | ||
|
|
||
|
|
||
| def validate_block_link_and_hash(previous_block, block): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if block.previous_hash != previous_block.hash: | ||
| raise ValueError( | ||
|
|
@@ -25,6 +34,12 @@ def validate_block_link_and_hash(previous_block, block): | |
| if block.hash != expected_hash: | ||
| raise ValueError(f"invalid hash {block.hash}") | ||
|
|
||
| validate_difficulty(block.difficulty, len(block.hash)) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition to the check in line 39, we must also check that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise, the miner can circumvent the difficulty check, by simply writing a small difficulty in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
| if not block.hash.startswith("0" * block.difficulty): | ||
| raise InvalidProofOfWorkError( | ||
| f"invalid PoW: hash {block.hash} does not satisfy difficulty {block.difficulty}" | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| class Blockchain: | ||
| """ | ||
|
|
@@ -118,7 +133,11 @@ def get_total_work(self, chain_list=None): | |
| if chain_list is None: | ||
| with self._lock: | ||
| chain_list = self.chain | ||
| return sum(2 ** (block.difficulty or 1) for block in chain_list) | ||
|
|
||
| for block in chain_list: | ||
| validate_difficulty(block.difficulty, len(block.hash)) | ||
|
|
||
| return sum(2 ** block.difficulty for block in chain_list) | ||
|
|
||
| def _next_difficulty(self, difficulty, avg_block_time): | ||
| """Advance the EMA difficulty control after a block, returning the new value.""" | ||
|
|
@@ -139,11 +158,13 @@ def _apply_block(self, prev_block, block, state, difficulty, avg_block_time): | |
|
|
||
| try: | ||
| validate_block_link_and_hash(prev_block, block) | ||
| except InvalidProofOfWorkError as exc: | ||
| logger.warning("Block %s rejected: %s", block.index, exc) | ||
| return ValidationStatus.INVALID, difficulty, avg_block_time | ||
| except ValueError as exc: | ||
| logger.warning("Block %s rejected: %s", block.index, exc) | ||
| status = ValidationStatus.INVALID if "hash" in str(exc) else ValidationStatus.FAILED | ||
| return status, difficulty, avg_block_time | ||
|
|
||
| if block.difficulty != difficulty: | ||
| logger.warning("Block %s rejected: Invalid difficulty. Expected %s, got %s", block.index, difficulty, block.difficulty) | ||
| return ValidationStatus.INVALID, difficulty, avg_block_time | ||
|
|
@@ -212,9 +233,12 @@ def resolve_conflicts(self, new_chain_list) -> tuple[bool, list]: | |
| return False, [] | ||
|
|
||
| with self._lock: | ||
| current_work = self.get_total_work() | ||
| new_work = self.get_total_work(new_chain_list) | ||
|
|
||
| try: | ||
| current_work = self.get_total_work() | ||
| new_work = self.get_total_work(new_chain_list) | ||
| except ValueError as exc: | ||
| logger.warning("Reorg rejected: %s", exc) | ||
| return False, [] | ||
| if new_work <= current_work: | ||
| logger.debug("Incoming chain (work: %s) is not heavier than local chain (work: %s). Rejecting.", new_work, current_work) | ||
| return False, [] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| from minichain import Blockchain, Block | ||
| from minichain.validators import ValidationStatus | ||
|
|
||
|
|
||
| def _make_block_with_invalid_pow(chain): | ||
| block = Block( | ||
| index=chain.last_block.index + 1, | ||
| previous_hash=chain.last_block.hash, | ||
| transactions=[], | ||
| difficulty=chain.current_difficulty, | ||
| state_root=chain.state.state_root(), | ||
| ) | ||
|
|
||
| while True: | ||
| block.hash = block.compute_hash() | ||
| if not block.hash.startswith("0" * block.difficulty): | ||
| return block | ||
| block.nonce += 1 | ||
|
|
||
|
|
||
| def test_add_block_rejects_invalid_pow(): | ||
| chain = Blockchain() | ||
| block = _make_block_with_invalid_pow(chain) | ||
|
|
||
| assert chain.add_block(block) == ValidationStatus.INVALID | ||
| assert len(chain.chain) == 1 | ||
|
|
||
|
|
||
| def test_resolve_conflicts_rejects_invalid_pow(): | ||
| chain = Blockchain() | ||
| block = _make_block_with_invalid_pow(chain) | ||
|
|
||
| success, _ = chain.resolve_conflicts([chain.chain[0], block]) | ||
|
|
||
| assert success is False | ||
| assert len(chain.chain) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm overlooking something important, let's remove this validation function.