feat: Introduce Merkle Tree for Transaction Verification (SPV Support)#28
Open
Muneerali199 wants to merge 8 commits intoStabilityNexus:mainfrom
Open
feat: Introduce Merkle Tree for Transaction Verification (SPV Support)#28Muneerali199 wants to merge 8 commits intoStabilityNexus:mainfrom
Muneerali199 wants to merge 8 commits intoStabilityNexus:mainfrom
Conversation
- Implement MerkleTree utility class in core/merkle.py - Add proof generation and verification methods - Update Block class to use MerkleTree for merkle_root - Add FastAPI server with /verify_transaction endpoint - Add /block/<index> endpoint with Merkle proofs for each transaction - Add tests for Merkle tree functionality
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Merkle tree implementation and integrates SPV support into blocks, persists chain and state to disk, moves mining logic into a core mining module, and exposes a FastAPI app with endpoints for chain info, per-transaction Merkle proofs, transaction verification, and mining. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as FastAPI (api/main.py)
participant Chain as Blockchain (core/chain.py)
participant Block as Block (core/block.py)
participant Merkle as MerkleTree (core/merkle.py)
Client->>API: GET /verify_transaction?tx_hash=<h>&block_index=<i>
API->>Chain: fetch block by index <i>
Chain-->>API: Block (includes merkle_root, transactions)
API->>Block: locate tx_index for tx_hash
API->>Block: get_merkle_proof(tx_index)
Block->>Merkle: get_proof(tx_index)
Merkle-->>Block: proof list
API->>Merkle: verify_proof(tx_hash, proof, merkle_root)
Merkle-->>API: verification result
API-->>Client: VerifyTransactionResponse (valid, proof, merkle_root)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/main.py`:
- Line 12: The module-level blockchain singleton (variable blockchain created
from class Blockchain) stores state only in memory so the chain is lost on
restart; add persistence by implementing serialization/deserialization on the
Blockchain class (e.g., to_dict()/from_dict() or
save_to_file()/load_from_file()) to write the chain and pending transactions to
disk (JSON or SQLite) and restore them at startup, then modify the module to
call Blockchain.load_from_file(...) when constructing the blockchain instance
and ensure you save state at shutdown or after mutating operations (use atomic
file writes and error handling in methods like Blockchain.save_to_file and
Blockchain.load_from_file to avoid corruption).
- Around line 26-35: Update the BlockResponse Pydantic model so its hash field
allows None to match Block.hash: change BlockResponse.hash from str to
Optional[str] (and keep default None if present) to prevent validation errors
when a Block instance's hash is unset; ensure the type import for Optional is
used and adjust any code that assumes a non-null hash accordingly.
- Around line 150-152: The uvicorn startup in the if __name__ == "__main__"
block binds to all interfaces; update the uvicorn.run invocation (the
uvicorn.run(app, host="0.0.0.0", port=8000") call) to use host "127.0.0.1"
instead of "0.0.0.0" for local/dev runs so the server only listens on localhost;
modify that single argument in the uvicorn.run call accordingly.
- Around line 77-82: The current loop drops valid empty proofs because it uses a
truthiness check; change the condition so it only excludes missing proofs (None)
but keeps empty lists returned by block.get_merkle_proof. In the loop over
block.transactions (where merkle_proofs is built using compute_tx_hash and
block.get_merkle_proof), replace the `if proof:` check with an explicit `if
proof is not None` (or always assign merkle_proofs[tx_hash] = proof) so that
empty proof lists for single-transaction blocks are preserved.
- Around line 52-53: compute_tx_hash duplicates the canonical SHA-256-over-JSON
transaction hashing and must be removed; replace uses of compute_tx_hash with
the Block-provided implementation by calling block.get_tx_hash(i) (which wraps
core/merkle._hash_transactions) when building/serializing blocks (e.g., in
get_block) and delete the compute_tx_hash function to avoid divergent copies.
- Around line 119-122: The call to MerkleTree.verify_proof can receive a None
proof from block.get_merkle_proof which will raise a TypeError; before calling
MerkleTree.verify_proof, check the result of block.get_merkle_proof (variable
proof) and handle the None case (e.g., set verification_status to False or raise
a clear error) using the existing tx_hash and merkle_root values; update the
code paths that assign verification_status so they only call
MerkleTree.verify_proof when proof is not None and otherwise handle/report the
missing proof appropriately.
- Around line 134-147: The /mine endpoint currently creates a new Mempool()
inside mine_block_endpoint which results in empty blocks; change it to use a
shared module-level mempool instance (e.g., create mempool = Mempool() at module
scope) and pass that shared instance into mine_and_process_block instead of
instantiating one per request; update references to pending_nonce_map and
blockchain as needed so they use the same shared state passed into
mine_and_process_block/get_transactions_for_block; also move the deferred
imports for mine_and_process_block and Mempool to the top of the module (or add
ImportError handling) so ImportError is handled at startup rather than at
request time.
In `@core/block.py`:
- Around line 89-93: Replace the on-the-fly recomputation in get_tx_hash with
the precomputed value from the MerkleTree: check the tx_index bounds as
currently done, then return self._merkle_tree.tx_hashes[tx_index] (or None if
out of range) instead of rebuilding the transaction dict and calling _sha256;
keep the same return type Optional[str] and rely on the MerkleTree's tx_hashes
to encapsulate the hashing formula.
In `@core/merkle.py`:
- Around line 26-46: The leaf vs internal hashing lacks domain separation
causing second-preimage risks; modify _hash_transactions to prefix each
serialized transaction with a leaf tag (e.g., "leaf:") before calling _sha256,
update _build_tree to prefix concatenated child hashes with an internal/node tag
(e.g., "node:") before _sha256 (ensure when duplicating the last leaf you still
treat it as a leaf before promotion), and update verify_proof to use the same
"node:" prefix when recomputing parent hashes so verification uses the identical
domain-separated hashing scheme.
- Line 3: Remove the unused Tuple import and modernize type hints in
core/merkle.py by deleting Tuple from the from typing import line, replacing
List and Optional usages with built-in list and union syntax (e.g., list[int] or
MyType | None) throughout the file (update any function signatures, return
annotations, and variable annotations that reference List[...] or Optional[...]
accordingly); ensure no remaining imports from typing are unused and run linters
to confirm Ruff UP035 compliance.
In `@requirements.txt`:
- Line 3: Update the pinned FastAPI dependency in requirements.txt to at least
0.109.1 (preferably the latest stable e.g., 0.129.2) to remediate
CVE-2024-24762; edit the fastapi entry (fastapi==0.109.0) to the new version,
then regenerate your dependency lock (pip freeze / pip-tools) and rebuild/test
the app to ensure compatibility. While updating, also consider bumping uvicorn
to 0.41.0 and pydantic to 2.12.5, run the test suite and smoke tests, and update
any Docker images or CI caches that install these packages.
In `@tests/test_merkle.py`:
- Around line 70-82: Add a new unit test named
test_proof_verification_fails_wrong_tx_hash that mirrors
test_proof_verification_fails_wrong_root but supplies an invalid transaction
hash instead of an invalid root: construct a MerkleTree with the same sample
txs, obtain the valid root via MerkleTree.get_merkle_root() and a valid proof
via MerkleTree.get_proof(0), then call MerkleTree.verify_proof with a tampered
tx hash (e.g. a string of repeated chars) and assert the result is False;
reference the existing test_proof_verification_fails_wrong_root,
MerkleTree.get_proof, MerkleTree.get_merkle_root, and MerkleTree.verify_proof to
locate where to add the new test.
- Around line 10-15: Extend the test_single_transaction in tests/test_merkle.py
to assert the single-leaf proof round-trip: call MerkleTree.get_proof(0) and
assert it returns an empty list, then call MerkleTree.verify_proof(proof,
leaf_hash, root) (using the tx hash as leaf_hash and tree.get_merkle_root() as
root) and assert it returns True; ensure you compute the leaf hash the same way
MerkleTree uses for its leaves so the verification compares the correct values.
api/main.py
Outdated
|
|
||
| app = FastAPI(title="MiniChain API", description="SPV-enabled blockchain API") | ||
|
|
||
| blockchain = Blockchain() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level blockchain singleton is ephemeral — chain state is lost on every server restart.
All mined blocks and transactions are held in memory only. For anything beyond a demo, consider persisting chain state to disk (JSON, SQLite, etc.) and restoring it at startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/main.py` at line 12, The module-level blockchain singleton (variable
blockchain created from class Blockchain) stores state only in memory so the
chain is lost on restart; add persistence by implementing
serialization/deserialization on the Blockchain class (e.g.,
to_dict()/from_dict() or save_to_file()/load_from_file()) to write the chain and
pending transactions to disk (JSON or SQLite) and restore them at startup, then
modify the module to call Blockchain.load_from_file(...) when constructing the
blockchain instance and ensure you save state at shutdown or after mutating
operations (use atomic file writes and error handling in methods like
Blockchain.save_to_file and Blockchain.load_from_file to avoid corruption).
- Add blockchain persistence with save_to_file/load_from_file - Fix BlockResponse.hash to Optional[str] - Remove compute_tx_hash, use block.get_tx_hash() - Fix empty proof check in get_block (use is not None) - Handle None proof in verify_transaction - Use shared mempool in /mine endpoint - Change uvicorn host to 127.0.0.1 - Use precomputed tx_hashes from MerkleTree in get_tx_hash - Modernize type hints in merkle.py (remove unused Tuple import) - Add domain separation for leaf vs internal hashing - Add single transaction proof round-trip test - Add test_proof_verification_fails_wrong_tx_hash - Update FastAPI to 0.129.2, uvicorn to 0.41.0, pydantic to 2.12.5
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/main.py`:
- Around line 70-89: get_block (and likewise get_chain) reads blockchain.chain
without acquiring blockchain._lock, risking concurrent mutation during
iteration; fix by acquiring the lock before accessing the chain (use `with
blockchain._lock:`), make a shallow copy like `chain_copy =
list(blockchain.chain)` inside the locked section, then release the lock and
operate on `chain_copy` when calling block.to_dict(), block.get_tx_hash(), and
block.get_merkle_proof() so you never iterate the live list while /mine may
append.
- Around line 15-23: TransactionResponse currently declares receiver: str which
rejects None for contract-deploy transactions; update the TransactionResponse
model (and any related BlockResponse serialization) to accept receiver:
Optional[str] so contract deployments can use None, and also relax data to
accept either dict or str (e.g., Optional[Union[dict, str]]) since
Transaction.__init__ may set data to a string (contract code); adjust
imports/types accordingly to keep Pydantic validation passing.
- Around line 146-158: The endpoint performs a deferred import of
mine_and_process_block inside mine_block_endpoint which delays failure to
request time; move the import out of the function and import
mine_and_process_block at module level (or refactor the mining logic into a new
core/mining.py and import from there) and update mine_block_endpoint to call the
module-level function directly; if you refactor into core/mining.py, ensure the
function signature accepts dependencies (pass in blockchain, mempool,
pending_nonce_map, and required globals like BURN_ADDRESS and a logger instead
of relying on main.py module scope) and update any calls to
mine_and_process_block accordingly.
- Around line 52-54: Replace the deprecated `@app.on_event`("shutdown") handler
with a FastAPI lifespan context manager: create an asynccontextmanager function
(e.g., lifespan) that yields for startup and calls blockchain.save_to_file() in
a finally block on exit, pass that lifespan to FastAPI(...) when constructing
the app, and remove the shutdown_event function; ensure you import
asynccontextmanager from contextlib and keep the call to
blockchain.save_to_file() inside the teardown section of the lifespan manager.
In `@core/block.py`:
- Around line 9-10: The helper function _sha256 is duplicated in core/block.py
and core/merkle.py; extract this single function into a new shared module (e.g.,
create core/utils.py with def _sha256(data: str) -> str) and remove the
duplicate definitions from both core/block.py and core/merkle.py, then update
both files to import _sha256 from core.utils (ensure the import name matches
existing uses and run tests/linting to confirm no unresolved references).
In `@core/chain.py`:
- Around line 28-58: After deserializing in _load_from_file, validate the
reconstructed chain before accepting it: for each Block in self.chain verify (1)
its previous_hash equals the hash of the prior block, (2) its stored hash
matches a recomputed hash (use the Block method that computes/serializes the
block hash — e.g. Block.compute_hash or equivalent), and (3) the merkle root
(recomputed from Transaction objects) matches whatever the Block stores (use
Block.merkle_root or recompute from Transaction list); on any mismatch log a
clear warning including self._chain_file and reject the loaded file (call
self._create_genesis_block() or raise) instead of silently accepting
corrupted/tampered data.
- Around line 86-89: The except block references temp_file which may not be
assigned if an exception occurs before its creation; fix by declaring temp_file
= None before the try (or assign temp_file earlier) and change the cleanup to
check temp_file is not None and os.path.exists(temp_file) before os.remove,
referencing the same temp_file and keeping the existing
logger.error(self._chain_file) call to avoid a secondary NameError in the
exception handler.
- Around line 38-49: When loading blocks in the chain loader loop, avoid
re-converting transaction timestamps by instantiating Transaction without
letting its constructor re-scale the timestamp and then directly assign the
already-millisecond value from the serialized dict; update the loop that builds
transactions (refer to Transaction and Block in the loader) so you create
Transaction with timestamp bypassed and then set t.timestamp = tx["timestamp"]
(or equivalent) before appending, ensuring merkle/hash consistency used by
add_block and to_dict; also fix save_to_file by declaring temp_file (e.g.,
temp_file = None) before the try block (or initialize it in the except) so the
except path can reference temp_file safely and not mask the original exception.
In `@core/merkle.py`:
- Around line 34-52: _build_tree currently pads the level in-place by mutating
current_level (which is a reference to tree[-1]), causing tree levels (e.g.,
tree[0]) to grow; change the code so you operate on a copy of the last level
before padding (e.g., current_level = tree[-1][:] or list(tree[-1])), then pad
that copy and use it to compute new_level with _sha256(self.NODE_PREFIX +
combined) so the stored tree lists remain unchanged and consistent with
self.tx_hashes (get_proof can still bound-check against self.tx_hashes).
- Around line 10-15: The MerkleProof dataclass is declared but unused; either
remove the MerkleProof class definition or make it the canonical return type for
your proof APIs—e.g., change functions that currently return raw dicts/lists
(such as any get_proof / build_proof / verify_proof utilities) to return a
MerkleProof instance with fields tx_hash, merkle_root, proof, and
verification_status; if you keep it, update the signatures and call sites to
construct and return MerkleProof objects, otherwise delete the MerkleProof
dataclass to eliminate dead code.
In `@tests/test_merkle.py`:
- Around line 6-8: Add a positive test that verifies calculate_merkle_root
returns the same value as constructing a MerkleTree and calling its
get_merkle_root for a non-empty list: create a new test method (e.g.,
test_calculate_merkle_root_matches_tree) that builds a sample transactions list,
computes root1 = calculate_merkle_root(transactions) and root2 =
MerkleTree(transactions).get_merkle_root(), and assert they are equal (use
self.assertEqual); ensure you import or reference MerkleTree and
calculate_merkle_root consistently with the existing tests.
core/block.py
Outdated
Comment on lines
9
to
10
| def _sha256(data: str) -> str: | ||
| return hashlib.sha256(data.encode()).hexdigest() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
_sha256 is duplicated across core/block.py and core/merkle.py.
Both files define an identical _sha256 helper. Consider extracting it into a shared utility module (e.g., core/utils.py) to maintain a single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/block.py` around lines 9 - 10, The helper function _sha256 is duplicated
in core/block.py and core/merkle.py; extract this single function into a new
shared module (e.g., create core/utils.py with def _sha256(data: str) -> str)
and remove the duplicate definitions from both core/block.py and core/merkle.py,
then update both files to import _sha256 from core.utils (ensure the import name
matches existing uses and run tests/linting to confirm no unresolved
references).
- Extract _sha256 to shared core/utils.py - Update block.py and merkle.py to import from utils - Fix _build_tree to use copy of level (avoid mutation) - Remove unused MerkleProof dataclass - Fix temp_file declaration in chain.py save_to_file - Add chain validation in _load_from_file (previous_hash, hash, merkle_root) - Fix transaction timestamp handling in chain loader - Move mining import to module level in api/main.py - Add thread safety in get_block and get_chain (acquire lock) - Replace @app.on_event with lifespan context manager - Fix TransactionResponse for contract deploy (Optional receiver, Union data) - Add test_calculate_merkle_root_matches_tree
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/main.py`:
- Around line 12-21: The module-level instantiation of Blockchain() and
Mempool() causes file I/O on import; move their creation into the lifespan
startup so initialization is explicit and lazy. In the asynccontextmanager
lifespan function, before yield create the instances (Blockchain() and
Mempool()), attach them to the FastAPI app (e.g., app.state.blockchain and
app.state.mempool) so handlers can access them, and keep the existing
blockchain.save_to_file() after yield to persist on shutdown. Update any code
that referenced the module-level symbols (blockchain, mempool) to read from
app.state instead.
- Line 84: Remove the unused local variable by deleting the assignment
"chain_length = len(blockchain.chain)" (or if the length is needed for
side-effects, replace usage by directly referencing len(blockchain.chain) where
required); locate the statement in the code block that references
blockchain.chain and remove that single unused assignment to satisfy Ruff F841.
- Around line 88-94: Rename the unused loop variable `tx` to `_` in the
for-loops that enumerate block.transactions (the loops using `for i, tx in
enumerate(block.transactions):` which call `block.get_tx_hash(i)` and
`block.get_merkle_proof(i)`) to satisfy the Ruff B007 rule; update both
occurrences (the one building `merkle_proofs` and the later similar loop) so the
pattern becomes `for i, _ in enumerate(block.transactions):`.
- Line 9: Extract the mine_and_process_block function out of main.py into a new
core.mining module and update callers to import it from there; move the function
body (including any helper functions it relies on) into core.mining, ensure its
public signature remains unchanged, and remove the import of
mine_and_process_block from main.py in the API module so api's main imports
core.mining.mine_and_process_block instead; verify and update any relative
imports inside the moved code to avoid circular imports and run a quick import
check to ensure both the application entry point and the API import the function
from core.mining.
- Around line 157-167: The pending_nonce_map should be a module-level variable
so its state persists across /mine requests; move the pending_nonce_map
declaration out of mine_block_endpoint to module scope (e.g., define
pending_nonce_map = {} near the top of api/main.py) and update
mine_block_endpoint to use that shared map instead of recreating it; also change
the call result handling to destructure the return from mine_and_process_block
as block, *_ = mine_and_process_block(blockchain, mempool, pending_nonce_map)
and then check block (instead of result[0]) before saving and returning the
block.
In `@core/chain.py`:
- Around line 38-62: The genesis block is not validated because the existing
validation loop starts at range(1, len(self.chain)); update the validation in
the class that builds/validates self.chain to explicitly check the genesis block
first: verify self.chain[0].hash equals a 64-char zero string and
self.chain[0].previous_hash equals "0" and ensure its transactions are as
expected (e.g., empty or conforming to your genesis rules) before proceeding to
the existing loop that validates subsequent blocks (refer to self.chain and the
Block/Transaction construction logic in the shown diff).
- Line 194: add_block currently holds self._lock while calling
self.save_to_file(), causing file I/O under the lock; change the flow so that
inside the critical section (while holding self._lock) you perform validation,
state commit, append the block and then create/serialize the minimal data
snapshot (e.g., a copy of the data dict or the exact payload save_to_file needs)
and release the lock, and then perform the actual file write outside the lock
(either by calling a new helper like _write_serialized_to_file(serialized_data)
or refactoring save_to_file to accept the pre-serialized data); keep references
to self._lock, add_block and save_to_file to locate the change.
- Line 33: Remove the unused local assignment in the _load_from_file method:
delete the line that sets temp_file = None in core.chain._load_from_file since
temp_file is never referenced; ensure no other logic depends on temp_file and
run linters/tests to confirm the unused variable warning (Ruff F841) is
resolved.
- Around line 78-90: The current merkle validation compares
curr_block.merkle_root to a newly constructed Block's merkle_root
(tautological); instead, during reconstruction make the raw block_data available
(e.g., iterate over data.get("chain", []) or keep block_data alongside
curr_block), reconstruct the Block into a variable (Block(...)) and compare
block_data.get("merkle_root") (the persisted/stored value) to the reconstructed
block.merkle_root, and on mismatch call logger.warning and reset self.chain = []
and break; update the loop that builds self.chain so it validates the stored
merkle_root before appending.
In `@core/merkle.py`:
- Around line 49-67: get_proof currently skips siblings when sibling_idx >=
len(level) causing incomplete proofs for the last leaf in odd-length levels;
update MerkleTree.get_proof to emit a self-sibling in that case (use the node's
own hash from level[index] as the sibling hash and set the "position"
appropriately) so the proof matches how _build_tree duplicates last nodes,
ensuring MerkleTree.verify_proof can reconstruct the correct parent; locate
get_proof and change the branch that checks sibling_idx < len(level) to append a
sibling even when out-of-range by using the duplicated node hash.
In `@tests/test_merkle.py`:
- Around line 24-36: The test_two_transactions currently obtains MerkleTree,
root via get_merkle_root, and proofs via get_proof(0) and get_proof(1) but never
verifies them; update the test to call MerkleTree.verify_proof (or verify_proof)
for each leaf using the corresponding proof and the root, asserting that
verification returns True (and also assert that verification fails for a
tampered leaf or proof if desired) so the 2-tx round-trip is fully exercised.
api/main.py
Outdated
Comment on lines
12
to
21
| @asynccontextmanager | ||
| async def lifespan(app: FastAPI): | ||
| yield | ||
| blockchain.save_to_file() | ||
|
|
||
|
|
||
| app = FastAPI(title="MiniChain API", description="SPV-enabled blockchain API", lifespan=lifespan) | ||
|
|
||
| blockchain = Blockchain() | ||
| mempool = Mempool() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level blockchain = Blockchain() triggers file I/O on import — side effect risk.
Importing api.main immediately loads the chain from disk and creates a genesis block if the file doesn't exist. This makes testing harder and can cause surprising behavior when the module is imported for introspection. Consider lazy initialization or explicit startup logic inside the lifespan context manager.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 13-13: Unused function argument: app
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/main.py` around lines 12 - 21, The module-level instantiation of
Blockchain() and Mempool() causes file I/O on import; move their creation into
the lifespan startup so initialization is explicit and lazy. In the
asynccontextmanager lifespan function, before yield create the instances
(Blockchain() and Mempool()), attach them to the FastAPI app (e.g.,
app.state.blockchain and app.state.mempool) so handlers can access them, and
keep the existing blockchain.save_to_file() after yield to persist on shutdown.
Update any code that referenced the module-level symbols (blockchain, mempool)
to read from app.state instead.
- Extract mine_and_process_block to core/mining.py - Move blockchain/mempool creation to lifespan (lazy init) - Remove unused chain_length variable - Rename unused tx to _ in loops - Move pending_nonce_map to module level - Remove unused temp_file in _load_from_file - Add genesis block validation - Fix merkle validation to compare against block_data - Fix add_block to release lock before saving - Fix get_proof to emit self-sibling for odd levels - Update test_two_transactions to verify proofs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
main.py (3)
88-90:⚠️ Potential issue | 🟠 Major
chain.statereference after removing explicitstatewill still work.After fixing
Blockchain()instantiation (removing thestatearg),chain.stateis used at lines 89, 90, 124, 125 etc. SinceBlockchain.__init__creates its ownState(),chain.stateis valid. However, the localstatevariable at line 24 becomes stale and disconnected from the chain's actual state. All references to the standalonestate(lines 31, 89, etc.) should be replaced withchain.state.🐛 Proposed fix — use chain.state consistently
- state = State() - chain = Blockchain(state) + chain = Blockchain() ... def claim_nonce(address): - account = state.get_account(address) + account = chain.state.get_account(address) ... - chain.state.credit_mining_reward(alice_pk, reward=100) - sync_nonce(chain.state, pending_nonce_map, alice_pk) + chain.state.credit_mining_reward(alice_pk, reward=100) + sync_nonce(chain.state, pending_nonce_map, alice_pk)The
chain.statereferences at lines 89, 90, 124, 125, 127, 128 are already correct. Only line 31 uses the stalestatelocal — update it tochain.state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 88 - 90, The local standalone variable state is stale after instantiating Blockchain() which now creates its own State; replace any uses of the standalone state (the variable created earlier) with chain.state so the program always manipulates the chain's authoritative state (e.g., update the initialization that uses state and any subsequent calls such as the one that sets pending_nonce_map entries and any references passed to sync_nonce or chain.state.credit_mining_reward). Ensure all references to the old state variable are removed and only chain.state is referenced (check functions/methods like sync_nonce, chain.state.credit_mining_reward, and any places building pending_nonce_map).
74-79:⚠️ Potential issue | 🔴 CriticalCritical:
_run_nodecall passes 6 arguments but the function accepts only 5 —TypeErrorat runtime.Line 74 passes
(network, state, chain, mempool, pending_nonce_map, claim_nonce)but line 79 defines_run_node(network, chain, mempool, pending_nonce_map, get_next_nonce). The extrastateargument shifts all positional parameters, causingmempool.get_transactions_for_block()to be called on aBlockchainobject, etc.🐛 Proposed fix
- await _run_node(network, state, chain, mempool, pending_nonce_map, claim_nonce) + await _run_node(network, chain, mempool, pending_nonce_map, claim_nonce)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 74 - 79, The call to _run_node passes six positional args (network, state, chain, mempool, pending_nonce_map, claim_nonce) but the definition _run_node(network, chain, mempool, pending_nonce_map, get_next_nonce) only accepts five, shifting parameters and causing runtime TypeErrors; fix by adding the missing state parameter to the _run_node definition (change signature to _run_node(network, state, chain, mempool, pending_nonce_map, get_next_nonce)) so parameter order matches the call and all uses of state, chain, mempool, pending_nonce_map, and get_next_nonce inside the function remain correct.
24-25:⚠️ Potential issue | 🔴 CriticalCritical:
Blockchain(state)passes wrong argument type, and_run_node()call has mismatched argument count.Line 25:
Blockchain.__init__(core/chain.py) expectschain_file: Optional[str]and creates its ownStateinternally. Passingstatesetsself._chain_file = state, causingos.path.exists()in_load_from_file()to raiseTypeError.Line 74: Call to
_run_node()passes 6 arguments (network, state, chain, mempool, pending_nonce_map, claim_nonce) but the function at line 79 accepts only 5 parameters (network, chain, mempool, pending_nonce_map, get_next_nonce), causingTypeError: _run_node() takes 5 positional arguments but 6 were given.🐛 Proposed fixes
- state = State() - chain = Blockchain(state) + chain = Blockchain()- await _run_node(network, state, chain, mempool, pending_nonce_map, claim_nonce) + await _run_node(network, chain, mempool, pending_nonce_map, claim_nonce)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 24 - 25, The code instantiates Blockchain with the wrong argument and calls _run_node with the wrong parameters: replace Blockchain(state) with Blockchain() so Blockchain.__init__ can create its own State (fixes passing a State into chain_file and the os.path.exists TypeError), and update the _run_node call to match its signature by removing the extra argument (pass network, chain, mempool, pending_nonce_map, get_next_nonce) — or if claim_nonce is intended as the nonce provider, rename/pass it as get_next_nonce so the five-argument signature (_run_node(network, chain, mempool, pending_nonce_map, get_next_nonce)) is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/main.py`:
- Around line 71-79: The get_chain endpoint currently accesses the private
synchronization primitive blockchain._lock directly (in get_chain), breaking
encapsulation; add a public, thread-safe accessor on the Blockchain class (for
example a method like get_chain_copy() or a context-manager style method e.g.,
locked_chain() that returns a safe copy or yields the chain under the internal
lock) and update get_chain to call that new public API (e.g., chain_copy =
blockchain.get_chain_copy() or using with blockchain.locked_chain() as chain:
...) instead of reaching into blockchain._lock; apply the same pattern for other
endpoints that reference blockchain._lock (e.g., get_block and
verify_transaction) so callers never touch the private _lock.
- Around line 12-14: Change the module-level annotations so they allow None
until initialization: import typing and annotate blockchain as
Optional[Blockchain] and mempool as Optional[Mempool] (e.g., blockchain:
Optional[Blockchain] = None, mempool: Optional[Mempool] = None), and give
pending_nonce_map an explicit mapping type (e.g., pending_nonce_map: Dict[str,
int] = {}) to make intent explicit to type-checkers; update the declarations for
the symbols blockchain, mempool, and pending_nonce_map accordingly.
- Around line 161-171: The endpoint function mine_block_endpoint calls
blockchain.save_to_file() after mine_and_process_block returns, but
Blockchain.add_block() already persists the chain; remove the redundant
blockchain.save_to_file() call in mine_block_endpoint to avoid duplicate I/O.
Keep the existing logic that calls mine_and_process_block(blockchain, mempool,
pending_nonce_map) and the success/failure response handling; simply delete the
explicit save_to_file() invocation so persistence is handled only by
Blockchain.add_block().
In `@core/chain.py`:
- Around line 95-102: The control flow in the loader is fragile because the
early return only happens when data.get("state") is truthy; fix load logic so
that when a chain is successfully loaded you always log and return regardless of
whether "state" exists: if data contains the chain set self.chain (and set
self.state = State.from_dict(...) only when present), then call
logger.info(f"Loaded chain with {len(self.chain)} blocks from
{self._chain_file}") and return; only fall through to
self._create_genesis_block() when no chain was loaded. Update the block around
the State.from_dict, logger.info, and return to ensure logger.info and the
return execute for a loaded chain even if state is falsy.
- Around line 108-138: The save_to_file method reads self.chain and self.state
without holding self._lock, which can race with add_block (used by
mine_block_endpoint) and produce a corrupted serialization; wrap the
serialization and any uses of self.chain/self.state (building the data dict and
computing len(self.chain) for the log) in the same lock (e.g., with self._lock:)
so the snapshot is taken atomically, then perform file I/O outside or still
under the lock as appropriate, and keep the existing temp-file replace/cleanup
logic unchanged.
- Around line 190-216: The serialization of self.chain is duplicated; extract
the dict-building logic into a new helper method (e.g.,
_serialize_chain_data(self)) that returns the data dict, then centralize file
writing into an unlocked helper (e.g., _save_to_file_unlocked(self, data) which
performs the temp file write/replace and logging without acquiring locks).
Change save_to_file to acquire the lock, call _serialize_chain_data(), then call
_save_to_file_unlocked(data), and change add_block to reuse the same helpers by
calling _serialize_chain_data() and _save_to_file_unlocked(data) while it
already holds the lock (or call save_to_file only if you switch to letting
save_to_file handle locking); ensure method names referenced here (add_block,
save_to_file, _serialize_chain_data, _save_to_file_unlocked) are used so the
duplicated block is removed.
In `@core/mining.py`:
- Around line 20-24: The code reads chain.last_block twice causing a TOCTOU
race; capture chain.last_block once into a local variable (e.g., last_block =
chain.last_block) and then use last_block.index and last_block.hash when
constructing the Block (Block(... index=last_block.index + 1,
previous_hash=last_block.hash, transactions=pending_txs)). Ensure you replace
the two separate accesses to chain.last_block with this single local reference
so index and previous_hash come from the same block.
- Around line 18-26: The code drains transactions up front by calling
mempool.get_transactions_for_block() before mining, so if mine_block(block)
raises MiningExceededError or chain.add_block(mined_block) rejects those
transactions are lost; modify the flow to either (A) fetch transactions without
removing them and only remove them from the mempool after chain.add_block
succeeds, or (B) if get_transactions_for_block() must remove them, catch
exceptions from mine_block() and chain.add_block() and re-add the transactions
back into the mempool in the except handlers; reference functions/classes:
mempool.get_transactions_for_block(), mine_block(), chain.add_block, and the
MiningExceededError exception to implement conditional removal or re-add on
failure.
In `@tests/test_merkle.py`:
- Around line 43-51: Update the test_odd_transaction_count in
tests/test_merkle.py to perform proof round-trip checks: after creating
MerkleTree(txs) and obtaining root via get_merkle_root(), iterate over each leaf
index, call MerkleTree.get_proof(index) (or the project's proof-generation
method) and then call MerkleTree.verify_proof(leaf, proof, root) (or the
project's verification method) asserting verification is True for every leaf;
this will exercise the self-sibling logic in MerkleTree and prevent regressions.
---
Outside diff comments:
In `@main.py`:
- Around line 88-90: The local standalone variable state is stale after
instantiating Blockchain() which now creates its own State; replace any uses of
the standalone state (the variable created earlier) with chain.state so the
program always manipulates the chain's authoritative state (e.g., update the
initialization that uses state and any subsequent calls such as the one that
sets pending_nonce_map entries and any references passed to sync_nonce or
chain.state.credit_mining_reward). Ensure all references to the old state
variable are removed and only chain.state is referenced (check functions/methods
like sync_nonce, chain.state.credit_mining_reward, and any places building
pending_nonce_map).
- Around line 74-79: The call to _run_node passes six positional args (network,
state, chain, mempool, pending_nonce_map, claim_nonce) but the definition
_run_node(network, chain, mempool, pending_nonce_map, get_next_nonce) only
accepts five, shifting parameters and causing runtime TypeErrors; fix by adding
the missing state parameter to the _run_node definition (change signature to
_run_node(network, state, chain, mempool, pending_nonce_map, get_next_nonce)) so
parameter order matches the call and all uses of state, chain, mempool,
pending_nonce_map, and get_next_nonce inside the function remain correct.
- Around line 24-25: The code instantiates Blockchain with the wrong argument
and calls _run_node with the wrong parameters: replace Blockchain(state) with
Blockchain() so Blockchain.__init__ can create its own State (fixes passing a
State into chain_file and the os.path.exists TypeError), and update the
_run_node call to match its signature by removing the extra argument (pass
network, chain, mempool, pending_nonce_map, get_next_nonce) — or if claim_nonce
is intended as the nonce provider, rename/pass it as get_next_nonce so the
five-argument signature (_run_node(network, chain, mempool, pending_nonce_map,
get_next_nonce)) is satisfied.
core/mining.py
Outdated
Comment on lines
45
to
50
| for tx in mined_block.transactions: | ||
| sync_nonce(chain.state, pending_nonce_map, tx.sender) | ||
|
|
||
| result = chain.state.get_account(tx.receiver) if tx.receiver else None | ||
| if isinstance(result, dict): | ||
| deployed_contracts.append(tx.receiver) |
Contributor
There was a problem hiding this comment.
deployed_contracts logic is inverted — it captures all receivers, not actual deployments.
get_account() always returns a dict (auto-creates if missing), so isinstance(result, dict) is always True for any non-None receiver. Meanwhile, actual contract deployments have tx.receiver is None, which this code skips.
Currently unused by callers (block, *_ = ...), but the return value is misleading if anyone relies on it.
- Add Optional/Dict typing annotations for blockchain, mempool, pending_nonce_map - Add public get_chain_copy() method to Blockchain class - Remove redundant blockchain.save_to_file() in mine_block_endpoint - Fix _load_from_file control flow for loaded chain logging - Add _serialize_chain_data() and _save_to_file_unlocked() helpers - Fix save_to_file to use lock properly - Fix mining.py to return transactions on mining failure - Add proof verification for odd_transaction_count test
- Store chain.last_block in local variable last_block to prevent race condition - Use last_block.index and last_block.hash when constructing new Block - Ensures index and previous_hash come from the same block instance
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
core/mining.py (1)
49-54:deployed_contractslogic still captures all receivers, not just contract deployments.
get_account()auto-creates and always returns a dict for any address, soisinstance(result, dict)is alwaysTruewhentx.receiveris set. This means every non-null receiver is appended, not just actual contract deployments (wheretx.receiver is None). Currently harmless since callers discard the value (block, *_ = ...), but misleading if anyone relies on it later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mining.py` around lines 49 - 54, The loop currently appends every non-null tx.receiver to deployed_contracts because get_account() always returns a dict; change the logic in the mined_block.transactions loop so that only actual contract-creation transactions (where tx.receiver is None) contribute to deployed_contracts: detect contract deployments by checking tx.receiver is None and then append the real created contract address (use the transaction field that carries the new contract address, e.g., tx.contract_address or whatever field your Transaction object exposes); if no such field exists, do not append anything. Keep sync_nonce(chain.state, pending_nonce_map, tx.sender) as-is and remove the isinstance(result, dict) check around get_account().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/main.py`:
- Around line 162-171: Remove the unnecessary global declaration in
mine_block_endpoint: delete the line declaring global pending_nonce_map and rely
on in-place mutations of the dict (as performed in sync_nonce) rather than
reassigning it; ensure no reassignment to pending_nonce_map occurs in
mine_block_endpoint or mine_and_process_block—if you need to reassign, change
the code to return the new map instead of using global. Use symbols to locate
the change: mine_block_endpoint, pending_nonce_map, sync_nonce, and
mine_and_process_block.
In `@core/chain.py`:
- Around line 67-70: When detecting an empty chain in the loader (the branch
that calls self._create_genesis_block()), change the logger.info message to
clearly state that a genesis block was created rather than "Loaded chain...";
update the log call (logger.info) to mention the chain file (self._chain_file)
and that the chain was empty so self._create_genesis_block() was invoked (e.g.,
"Chain file contained no blocks; created genesis block from
{self._chain_file}"), referencing self.chain, self._create_genesis_block(),
logger.info and self._chain_file to locate the change.
- Around line 133-146: The log uses len(self.chain) inside
_save_to_file_unlocked which runs outside the lock and can report a
stale/incorrect count; change _save_to_file_unlocked to accept an explicit
block_count (or include the length in the passed-in data dict) and use that
value for the logger instead of len(self.chain), and update all callers
(save_to_file and add_block) to compute and pass the current length while still
holding the lock so the logged count matches the serialized data.
- Line 76: Remove the unnecessary f-string prefixes on the logger.info calls
that have no placeholders; specifically update the logger.info(f"Created new
genesis block after rejecting invalid chain") and the similar logger.info(...)
call at the other occurrence (line with message on line 109) to plain string
literals (logger.info("...")), so the f prefix is removed and the Ruff F541
warning is resolved.
In `@core/mining.py`:
- Around line 26-30: The loop in the mining rollback is redundantly calling
mempool._seen_tx_ids.update(tx_hashes) for every tx and directly manipulates
private attributes; move the single set update call outside the per-transaction
loop and replace direct mutations of mempool._pending_txs and
mempool._seen_tx_ids with a public mempool.return_transactions(txs) (or similar)
method that appends returned transactions and updates seen IDs internally; apply
the same change to the second occurrence around the block referenced by the diff
(the lines corresponding to the second loop where _pending_txs and _seen_tx_ids
are directly modified).
---
Duplicate comments:
In `@core/mining.py`:
- Around line 49-54: The loop currently appends every non-null tx.receiver to
deployed_contracts because get_account() always returns a dict; change the logic
in the mined_block.transactions loop so that only actual contract-creation
transactions (where tx.receiver is None) contribute to deployed_contracts:
detect contract deployments by checking tx.receiver is None and then append the
real created contract address (use the transaction field that carries the new
contract address, e.g., tx.contract_address or whatever field your Transaction
object exposes); if no such field exists, do not append anything. Keep
sync_nonce(chain.state, pending_nonce_map, tx.sender) as-is and remove the
isinstance(result, dict) check around get_account().
Comment on lines
162
to
171
| @app.post("/mine") | ||
| def mine_block_endpoint(): | ||
| global pending_nonce_map | ||
|
|
||
| block, *_ = mine_and_process_block(blockchain, mempool, pending_nonce_map) | ||
|
|
||
| if block: | ||
| return {"message": "Block mined successfully", "block": block.to_dict()} | ||
| else: | ||
| raise HTTPException(status_code=400, detail="Failed to mine block") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary global declaration for pending_nonce_map.
pending_nonce_map is a mutable dict that is only mutated in-place (via pending_nonce_map[address] = ... in sync_nonce), not reassigned. The global declaration is superfluous.
♻️ Proposed fix
`@app.post`("/mine")
def mine_block_endpoint():
- global pending_nonce_map
-
block, *_ = mine_and_process_block(blockchain, mempool, pending_nonce_map)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/main.py` around lines 162 - 171, Remove the unnecessary global
declaration in mine_block_endpoint: delete the line declaring global
pending_nonce_map and rely on in-place mutations of the dict (as performed in
sync_nonce) rather than reassigning it; ensure no reassignment to
pending_nonce_map occurs in mine_block_endpoint or mine_and_process_block—if you
need to reassign, change the code to return the new map instead of using global.
Use symbols to locate the change: mine_block_endpoint, pending_nonce_map,
sync_nonce, and mine_and_process_block.
core/chain.py
Outdated
Comment on lines
67
to
70
| if len(self.chain) == 0: | ||
| self._create_genesis_block() | ||
| logger.info(f"Loaded chain with {len(self.chain)} blocks from {self._chain_file}") | ||
| return |
Contributor
There was a problem hiding this comment.
Misleading log message after creating genesis from empty chain data.
When the loaded chain array is empty, genesis is created, but the log says "Loaded chain with {len(self.chain)} blocks from {self._chain_file}". This is misleading — a genesis block was created, not loaded. Consider using a distinct message like "Chain file contained no blocks, created genesis".
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 69-69: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/chain.py` around lines 67 - 70, When detecting an empty chain in the
loader (the branch that calls self._create_genesis_block()), change the
logger.info message to clearly state that a genesis block was created rather
than "Loaded chain..."; update the log call (logger.info) to mention the chain
file (self._chain_file) and that the chain was empty so
self._create_genesis_block() was invoked (e.g., "Chain file contained no blocks;
created genesis block from {self._chain_file}"), referencing self.chain,
self._create_genesis_block(), logger.info and self._chain_file to locate the
change.
core/chain.py
Outdated
| if genesis.hash != "0" * 64 or genesis.previous_hash != "0": | ||
| logger.warning("Loaded chain has invalid genesis block. Rejecting loaded chain.") | ||
| self._create_genesis_block() | ||
| logger.info(f"Created new genesis block after rejecting invalid chain") |
Contributor
There was a problem hiding this comment.
Remove extraneous f prefix — no placeholders in the string (Ruff F541).
Same issue on line 109.
♻️ Proposed fix
- logger.info(f"Created new genesis block after rejecting invalid chain")
+ logger.info("Created new genesis block after rejecting invalid chain")🧰 Tools
🪛 Ruff (0.15.1)
[error] 76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
[warning] 76-76: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/chain.py` at line 76, Remove the unnecessary f-string prefixes on the
logger.info calls that have no placeholders; specifically update the
logger.info(f"Created new genesis block after rejecting invalid chain") and the
similar logger.info(...) call at the other occurrence (line with message on line
109) to plain string literals (logger.info("...")), so the f prefix is removed
and the Ruff F541 warning is resolved.
core/chain.py
Outdated
Comment on lines
133
to
146
| def _save_to_file_unlocked(self, data): | ||
| temp_file = None | ||
| try: | ||
| temp_file = self._chain_file + ".tmp" | ||
| with open(temp_file, 'w') as f: | ||
| json.dump(data, f, indent=2) | ||
|
|
||
| os.replace(temp_file, self._chain_file) | ||
| logger.info(f"Saved chain with {len(self.chain)} blocks to {self._chain_file}") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to save chain to {self._chain_file}: {e}") | ||
| if temp_file is not None and os.path.exists(temp_file): | ||
| os.remove(temp_file) |
Contributor
There was a problem hiding this comment.
len(self.chain) in the log message is read outside the lock — stale count.
_save_to_file_unlocked is called after the lock is released (both from save_to_file and add_block). By the time line 141 executes, another thread may have appended a block, making the logged count inconsistent with the serialized data. Consider passing the block count as part of the data dict or as a parameter.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 133-133: Missing return type annotation for private function _save_to_file_unlocked
Add return type annotation: None
(ANN202)
[warning] 141-141: Logging statement uses f-string
(G004)
[warning] 143-143: Do not catch blind exception: Exception
(BLE001)
[warning] 144-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 144-144: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/chain.py` around lines 133 - 146, The log uses len(self.chain) inside
_save_to_file_unlocked which runs outside the lock and can report a
stale/incorrect count; change _save_to_file_unlocked to accept an explicit
block_count (or include the length in the passed-in data dict) and use that
value for the logger instead of len(self.chain), and update all callers
(save_to_file and add_block) to compute and pass the current length while still
holding the lock so the logged count matches the serialized data.
core/mining.py
Outdated
Comment on lines
26
to
30
| for tx in pending_txs: | ||
| mempool._pending_txs.append(tx) | ||
| mempool._seen_tx_ids.update(tx_hashes) | ||
| logger.warning("Mining failed, transactions returned to mempool") | ||
| return None, [] |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_seen_tx_ids.update(tx_hashes) is called redundantly inside the per-transaction loop.
The set update with all tx hashes is repeated for every transaction in the loop. It's idempotent but wasteful — move it outside the loop. The same issue exists on lines 58–60.
Additionally, directly manipulating mempool._pending_txs and mempool._seen_tx_ids breaks encapsulation. Consider adding a public mempool.return_transactions(txs) method.
♻️ Proposed fix
except MiningExceededError:
- for tx in pending_txs:
- mempool._pending_txs.append(tx)
- mempool._seen_tx_ids.update(tx_hashes)
+ for tx in pending_txs:
+ mempool._pending_txs.append(tx)
+ mempool._seen_tx_ids.update(tx_hashes)
logger.warning("Mining failed, transactions returned to mempool")
return None, []Apply the same fix to lines 58–60.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/mining.py` around lines 26 - 30, The loop in the mining rollback is
redundantly calling mempool._seen_tx_ids.update(tx_hashes) for every tx and
directly manipulates private attributes; move the single set update call outside
the per-transaction loop and replace direct mutations of mempool._pending_txs
and mempool._seen_tx_ids with a public mempool.return_transactions(txs) (or
similar) method that appends returned transactions and updates seen IDs
internally; apply the same change to the second occurrence around the block
referenced by the diff (the lines corresponding to the second loop where
_pending_txs and _seen_tx_ids are directly modified).
- Remove unnecessary global pending_nonce_map declaration in mine_block_endpoint - Update logger message for empty chain in _load_from_file to clarify genesis block creation - Remove unnecessary f-string prefixes on logger.info calls without placeholders - Fix _save_to_file_unlocked to accept explicit block count to avoid stale length reads - Add return_transactions method to Mempool and use it instead of direct attribute manipulation - Fix redundant mempool._seen_tx_ids.update calls in mining rollback loops
- Fix indentation in chain.py logger.warning call - Remove incorrect contract deployment tracking logic that appended tx.receiver to deployed_contracts list regardless of actual contract creation - The loop now only calls sync_nonce as intended
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #27
🔎 Summary
This PR introduces Merkle Tree–based transaction verification to enable Simplified Payment Verification (SPV) support.
Changes Included
MerkleTreeutility class with:Blockclass to:MerkleTreemerkle_rootinstead of hashing raw transaction lists🌐 API Endpoints Added
GET /verify_transaction?tx_hash=<>&block_index=<>Returns transaction inclusion proof, Merkle path, and verification status.
GET /block/<index>Returns block data including Merkle verification support.
GET /chainReturns the full blockchain.
🧪 Testing
This enhancement aligns the project closer to real-world blockchain architecture and enables light-client style verification.
Summary by CodeRabbit