Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion genesis.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"chain_id": "minichain-default",
"timestamp": 1716880000000,
"difficulty": 4,
"target": "0x0000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF",
"target_block_time": 10000,
"alpha": 0.1,
"initial_supply": 1500000000,
Expand Down
2 changes: 1 addition & 1 deletion main.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def mine_and_process_block(chain, mempool, miner_pk):
receipt_root=calculate_receipt_root(receipts),
receipts=receipts,
miner=miner_pk,
difficulty=chain.current_difficulty,
target=chain.current_target,
)

mined_block = mine_block(block)
Expand Down
20 changes: 9 additions & 11 deletions minichain/block.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def __init__(
previous_hash: str,
transactions: Optional[Sequence[Transaction]] = None,
timestamp: Optional[float] = None,
difficulty: Optional[int] = None,
target: Optional[int] = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

state_root: Optional[str] = None,
receipt_root: Optional[str] = None,
receipts: Optional[Sequence[Receipt]] = None,
Expand All @@ -59,7 +59,7 @@ def __init__(
if timestamp is None
else int(timestamp)
)
self.difficulty: Optional[int] = difficulty
self.target: Optional[int] = target
self.nonce: int = 0
self.hash: Optional[str] = None
self.state_root: Optional[str] = state_root
Expand All @@ -83,7 +83,7 @@ def to_header_dict(self):
"state_root": self.state_root,
"receipt_root": self.receipt_root,
"timestamp": self.timestamp,
"difficulty": self.difficulty,
"target": hex(self.target) if self.target is not None else None,
"nonce": self.nonce,
}
# Include miner in header only when present (optional field)
Expand Down Expand Up @@ -130,14 +130,12 @@ def from_dict(cls, payload: dict):
for r_payload in payload.get("receipts", [])
]

# Safely extract and cast difficulty and timestamp if they exist
raw_diff = payload.get("difficulty")
if raw_diff is not None:
parsed_diff = int(raw_diff)
if parsed_diff > 256:
raise ValueError(f"Difficulty too large: {parsed_diff}")
# Safely extract and cast target and timestamp if they exist
raw_target = payload.get("target")
if raw_target is not None:
parsed_target = int(raw_target, 16) if isinstance(raw_target, str) else int(raw_target)
else:
parsed_diff = None
parsed_target = None

raw_ts = payload.get("timestamp")
parsed_ts = int(raw_ts) if raw_ts is not None else None
Expand All @@ -146,7 +144,7 @@ def from_dict(cls, payload: dict):
previous_hash=payload["previous_hash"],
transactions=transactions,
timestamp=parsed_ts,
difficulty=parsed_diff,
target=parsed_target,
state_root=payload.get("state_root"),
receipt_root=payload.get("receipt_root"),
receipts=receipts,
Expand Down
66 changes: 35 additions & 31 deletions minichain/chain.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ def validate_block_link_and_hash(previous_block, block):
if block.hash != expected_hash:
raise ValueError(f"invalid hash {block.hash}")

target = "0" * (block.difficulty or 1)
if not block.hash.startswith(target):
raise ValueError(f"invalid Proof of Work: hash {block.hash} does not satisfy difficulty {block.difficulty}")
if block.target is None or int(block.hash, 16) >= block.target:
raise ValueError(f"invalid Proof of Work: hash {block.hash} does not satisfy target {block.target}")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

if block.timestamp <= previous_block.timestamp:
raise ValueError(f"invalid timestamp: {block.timestamp} is not strictly greater than previous block timestamp {previous_block.timestamp}")
Expand Down Expand Up @@ -88,19 +87,23 @@ def _create_genesis_block(self, genesis_path):
self.state.chain_id = self.chain_id

timestamp = config.get("timestamp")
difficulty = config.get("difficulty")
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)
else:
from .network_config import MAX_TARGET
self.current_target = MAX_TARGET

self.target_block_time = config.get("target_block_time", 10000)
self.alpha = config.get("alpha", 0.1)
self.current_difficulty = difficulty
self.avg_block_time = self.target_block_time

genesis_block = Block(
index=0,
previous_hash="0",
transactions=[],
timestamp=timestamp,
difficulty=difficulty,
target=self.current_target,
state_root=self.state.state_root(),
receipt_root=None,
receipts=[]
Expand Down Expand Up @@ -133,27 +136,28 @@ def last_block(self):
def get_total_work(self, chain_list=None):
"""
Calculates the cumulative PoW of a chain.
Work is proportional to 2^difficulty.
Work is inversely proportional to target.
"""
if chain_list is None:
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a comment explaining what this does.


def _next_difficulty(self, difficulty, avg_block_time):
"""Advance the EMA difficulty control after a block, returning the new value."""
def _next_target(self, target, avg_block_time):
"""Advance the EMA target control after a block, returning the new value."""
from .network_config import MAX_TARGET, MIN_TARGET
if avg_block_time > self.target_block_time:
return max(1, difficulty - 1)
return min(MAX_TARGET, target + 1)
if avg_block_time < self.target_block_time:
return difficulty + 1
return difficulty
return max(MIN_TARGET, target - 1)
return target

def _apply_block(self, prev_block, block, state, difficulty, avg_block_time):
def _apply_block(self, prev_block, block, state, target, avg_block_time):
"""
Canonical block-application pipeline shared by add_block and resolve_conflicts.
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a new_avg_block_time being computed? I don't see it.

Potential critical bug.

"""
from .validators import ValidationStatus

Expand All @@ -162,18 +166,18 @@ def _apply_block(self, prev_block, block, state, 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
return status, target, 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
if block.target != target:
logger.warning("Block %s rejected: Invalid target. Expected %s, got %s", block.index, target, block.target)
return ValidationStatus.INVALID, target, avg_block_time

receipts = []
for tx in block.transactions:
status, receipt = state.validate_and_apply_with_status(tx)
if status != ValidationStatus.VALID:
logger.warning("Block %s rejected: Transaction failed validation", block.index)
return status, difficulty, avg_block_time
return status, target, avg_block_time
receipts.append(receipt)

total_fees = sum(getattr(r, 'gas_used', 0) * getattr(tx, 'fee_per_gas', 0) for r, tx in zip(receipts, block.transactions))
Expand All @@ -183,19 +187,19 @@ def _apply_block(self, prev_block, block, state, difficulty, avg_block_time):
computed_receipt_root = calculate_receipt_root(receipts)
if block.receipt_root != computed_receipt_root:
logger.warning("Block %s rejected: Invalid receipt root. Expected %s, got %s", block.index, computed_receipt_root, block.receipt_root)
return ValidationStatus.INVALID, difficulty, avg_block_time
return ValidationStatus.INVALID, target, avg_block_time

if [r.to_dict() for r in block.receipts] != [r.to_dict() for r in receipts]:
logger.warning("Block %s rejected: Receipts payload mismatch", block.index)
return ValidationStatus.INVALID, difficulty, avg_block_time
return ValidationStatus.INVALID, target, avg_block_time

computed_state_root = state.state_root()
if block.state_root != computed_state_root:
logger.warning("Block %s rejected: Invalid state root. Expected %s, got %s", block.index, computed_state_root, block.state_root)
return ValidationStatus.INVALID, difficulty, avg_block_time
return ValidationStatus.INVALID, target, avg_block_time

new_avg = self.alpha * (block.timestamp - prev_block.timestamp) + (1 - self.alpha) * avg_block_time
return ValidationStatus.VALID, self._next_difficulty(difficulty, new_avg), new_avg
return ValidationStatus.VALID, self._next_target(target, new_avg), new_avg

def add_block(self, block):
"""
Expand All @@ -207,15 +211,15 @@ def add_block(self, block):
with self._lock:
temp_state = self.state.copy()
temp_state.chain_id = self.chain_id
status, new_difficulty, new_avg = self._apply_block(
self.last_block, block, temp_state, self.current_difficulty, self.avg_block_time
status, new_target, new_avg = self._apply_block(
self.last_block, block, temp_state, self.current_target, self.avg_block_time
)
if status != ValidationStatus.VALID:
return status

# All transactions valid → commit state and append block
self.state = temp_state
self.current_difficulty = new_difficulty
self.current_target = new_target
self.avg_block_time = new_avg
self.chain.append(block)
return ValidationStatus.VALID
Expand Down Expand Up @@ -264,12 +268,12 @@ def resolve_conflicts(self, new_chain_list) -> tuple[bool, list]:
temp_state.chain_id = self.chain_id
temp_state.restore(self._genesis_state_snapshot)

temp_difficulty = proposed_chain[0].difficulty
temp_target = proposed_chain[0].target
temp_avg_block_time = self.target_block_time

for i in range(1, len(proposed_chain)):
status, temp_difficulty, temp_avg_block_time = self._apply_block(
proposed_chain[i - 1], proposed_chain[i], temp_state, temp_difficulty, temp_avg_block_time
status, temp_target, temp_avg_block_time = self._apply_block(
proposed_chain[i - 1], proposed_chain[i], temp_state, temp_target, temp_avg_block_time
)
if status != ValidationStatus.VALID:
logger.warning("Reorg failed at block %s", proposed_chain[i].index)
Expand All @@ -281,7 +285,7 @@ def resolve_conflicts(self, new_chain_list) -> tuple[bool, list]:

self.chain = proposed_chain
self.state = temp_state
self.current_difficulty = temp_difficulty
self.current_target = temp_target
self.avg_block_time = temp_avg_block_time
logger.info("Reorg successful! Switched to new chain tip: Block %s", self.last_block.index)
return True, orphans
3 changes: 3 additions & 0 deletions minichain/network_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@
MAX_FUTURE_BLOCK_TIME_MS = 15000 # Max allowed ms in the future for a block timestamp
GAS_PER_BYTE = 10 # Cost per byte of state storage written
MAX_CALL_DEPTH = 10 # Maximum depth for cross-contract calls
MAX_TARGET = int("F" * 64, 16)
MIN_TARGET = 1

17 changes: 8 additions & 9 deletions minichain/pow.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def calculate_hash(block_dict):

def mine_block(
block,
difficulty=None,
target=None,
max_nonce=None,
timeout_seconds=None,
logger=None,
Expand All @@ -23,20 +23,19 @@ def mine_block(
"""Mines a block using Proof-of-Work without mutating input block until success."""
max_nonce = max_nonce if max_nonce is not None else MINING_MAX_NONCE

difficulty = difficulty if difficulty is not None else block.difficulty
if not isinstance(difficulty, int) or difficulty <= 0:
raise ValueError("Difficulty must be a positive integer.")
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.")

target = "0" * difficulty
local_nonce = 0
header_dict = block.to_header_dict() # Construct header dict once outside loop
Comment on lines +31 to 38

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

start_time = time.monotonic()

if logger:
logger.info(
"Mining block %s (Difficulty: %s)",
"Mining block %s (Target: %s)",
block.index,
difficulty,
target,
)

while True:
Expand All @@ -56,8 +55,8 @@ def mine_block(
header_dict["nonce"] = local_nonce
block_hash = calculate_hash(header_dict)

# Check difficulty target
if block_hash.startswith(target):
# Check target
if int(block_hash, 16) < target:
block.nonce = local_nonce # Assign only on success
block.hash = block_hash
if logger:
Expand Down
2 changes: 1 addition & 1 deletion tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def test_transaction_fee(self):
index=1,
previous_hash="0",
transactions=[tx],
difficulty=1,
target=int("F"*64, 16),
state_root=self.state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
Expand Down
4 changes: 2 additions & 2 deletions tests/test_persistence.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def _chain_with_tx(self):
index=1,
previous_hash=bc.last_block.hash,
transactions=[tx],
difficulty=bc.current_difficulty,
target=bc.current_target,
state_root=temp_state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
Expand Down Expand Up @@ -245,7 +245,7 @@ def test_loaded_chain_can_add_new_block(self):
index=len(restored.chain),
previous_hash=restored.last_block.hash,
transactions=[tx2],
difficulty=restored.current_difficulty,
target=restored.current_target,
state_root=temp_state.state_root(),
receipt_root=calculate_receipt_root([receipt2]),
receipts=[receipt2],
Expand Down
4 changes: 2 additions & 2 deletions tests/test_persistence_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,12 @@ def _chain_with_tx(self):
index=1,
previous_hash=bc.last_block.hash,
transactions=[tx],
difficulty=1,
target=int("F"*64, 16),
state_root=temp_state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
)
mine_block(block, difficulty=1)
mine_block(block, target=int("F"*64, 16))
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
bc.add_block(block)
return bc

Expand Down
6 changes: 3 additions & 3 deletions tests/test_protocol_hardening.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def test_canonical_json_is_order_independent(self):

def test_block_hash_matches_compute_hash(self):
block = Block(index=1, previous_hash="abc", transactions=[], timestamp=1234567890)
block.difficulty = 2
block.target = 2
block.nonce = 7

self.assertEqual(block.compute_hash(), calculate_hash(block.to_header_dict()))
Expand Down Expand Up @@ -117,7 +117,7 @@ async def test_block_schema_accepts_current_block_wire_format(self):
previous_hash="0" * 64,
transactions=[tx],
timestamp=1600000000000,
difficulty=2,
target=int("F"*64, 16),
state_root="0"*64,
receipts=[receipt],
receipt_root=calculate_receipt_root([receipt])
Expand Down Expand Up @@ -150,7 +150,7 @@ async def test_duplicate_tx_and_block_detection(self):
"previous_hash": "0" * 64,
"transactions": [tx_message["data"]],
"timestamp": 123,
"difficulty": 2,
"target": int("F"*64, 16),
"nonce": 1,
"hash": "f" * 64,
},
Expand Down
2 changes: 1 addition & 1 deletion tests/test_reorg.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def genesis_file(tmp_path):
pk = sk.verify_key.encode(encoder=HexEncoder).decode()
data = {
"timestamp": int(time.time()),
"difficulty": 1,
"target": int("F"*64, 16),
"alloc": {
pk: {"balance": 1000}
}
Expand Down
6 changes: 3 additions & 3 deletions tests/test_serialization.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ def test_block_serialization_determinism():
tx2 = Transaction(**tx_params)

# Add the miner field
block1 = Block(index=1, previous_hash="0"*64, transactions=[tx1], difficulty=2, timestamp=999999, miner="a" * 40)
block2 = Block(index=1, previous_hash="0"*64, transactions=[tx2], difficulty=2, timestamp=999999, miner="a" * 40)
block1 = Block(index=1, previous_hash="0"*64, transactions=[tx1], target=int("F"*64, 16), timestamp=999999, miner="a" * 40)
block2 = Block(index=1, previous_hash="0"*64, transactions=[tx2], target=int("F"*64, 16), timestamp=999999, miner="a" * 40)

# Pre-compute the hashes before asserting
block1.hash = block1.compute_hash()
Expand All @@ -55,7 +55,7 @@ def test_block_from_dict_rejects_tampered_payload():
tx = Transaction(sender="A", receiver="B", amount=10, nonce=5, timestamp=1000)
block = Block(
index=1, previous_hash="0"*64, transactions=[tx],
difficulty=2, timestamp=999999, miner="a"*40
target=int("F"*64, 16), timestamp=999999, miner="a"*40
)
block.hash = block.compute_hash()

Expand Down
Loading
Loading