feat: add sqlite-backed chain persistence#82
Open
arunabha003 wants to merge 5 commits intoStabilityNexus:mainfrom
Open
feat: add sqlite-backed chain persistence#82arunabha003 wants to merge 5 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 5 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughPersisted snapshots moved from atomic JSON files to a SQLite-backed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI / Test Harness"
participant Node as "run_node (main)"
participant Persist as "minichain.persistence"
participant DB as "data.db (SQLite)"
CLI->>Node: start node(datadir)
Node->>Persist: persistence_exists(datadir)?
alt exists
Persist->>DB: open & read blocks, accounts, metadata
DB-->>Persist: snapshot rows
Persist-->>Node: Blockchain restored
else not exists
Node-->>Node: initialize fresh Blockchain
end
CLI->>Node: submit tx / fund / mine
Node->>Persist: save(blockchain, datadir)
Persist->>DB: write/replace blocks, accounts, metadata
DB-->>Persist: ack
Node-->>CLI: shutdown
CLI->>Node: restart node(datadir)
Node->>Persist: persistence_exists(datadir)?
Persist->>DB: read snapshot
DB-->>Persist: snapshot
Persist-->>Node: restored blockchain state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/persistence.py`:
- Around line 89-91: The code assigns raw_accounts directly into
blockchain.state.accounts after calling _verify_chain_integrity(blocks), which
allows tampered account snapshots to be trusted; instead either rebuild the
state from the verified blocks or verify a signed/state-root against the
snapshot: update the load path that currently does blockchain = Blockchain();
blockchain.chain = blocks; blockchain.state.accounts = raw_accounts to (a)
reconstruct state by replaying blocks/transactions using the existing state
application logic (e.g., the same functions that apply transactions/mining
rewards used in main.py) and then replace blockchain.state.accounts with the
reconstructed state, or (b) persist and load a verifiable state root/signature
alongside the snapshot and check that the computed state root from the
reconstructed state matches before accepting raw_accounts (use the
_verify_chain_integrity and a new compute_state_root or signature verification
routine to compare). Ensure references to Blockchain, blockchain.state.accounts,
raw_accounts, and _verify_chain_integrity are used so the change replaces the
unsafe direct assignment with state reconstruction or state-root verification.
- Around line 81-91: The loader currently only checks top-level types but lets
malformed individual rows (e.g. non-dict block/account entries or
Block.from_dict KeyError/TypeError) propagate as non-ValueError exceptions;
change load to validate and normalize per-row failures: iterate raw_blocks,
ensure each block entry is a dict, call _deserialize_block inside a try/except
that catches KeyError/TypeError/ValueError and re-raises a ValueError with
context if deserialization fails; similarly iterate raw_accounts.items(), ensure
each account value is a dict (or else raise ValueError), and only after all rows
validate and deserialize assign blockchain.chain and blockchain.state.accounts
(use Blockchain and _deserialize_block as anchors). This ensures
malformed-but-JSON-valid rows become ValueError and are handled as recoverable
corruption.
🪄 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: d81bb7af-99ca-462b-af97-806b978b209e
📒 Files selected for processing (4)
main.pyminichain/persistence.pytests/test_persistence.pytests/test_persistence_runtime.py
minichain/persistence.py
Outdated
Comment on lines
89
to
91
| blockchain = Blockchain() | ||
| blockchain.chain = blocks | ||
| blockchain.state.accounts = raw_accounts |
Contributor
There was a problem hiding this comment.
Line 91 trusts an unauthenticated accounts snapshot.
After _verify_chain_integrity(blocks), Line 91 assigns raw_accounts wholesale. Editing one accounts.account_json row can rewrite balances/nonces on startup without changing any block, and the chain cannot prove that state because mining rewards are applied outside block payloads in main.py, Lines 87-90 and 149-151. Please persist enough information to rebuild state on load, or add a verifiable state root/signature before accepting accounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/persistence.py` around lines 89 - 91, The code assigns raw_accounts
directly into blockchain.state.accounts after calling
_verify_chain_integrity(blocks), which allows tampered account snapshots to be
trusted; instead either rebuild the state from the verified blocks or verify a
signed/state-root against the snapshot: update the load path that currently does
blockchain = Blockchain(); blockchain.chain = blocks; blockchain.state.accounts
= raw_accounts to (a) reconstruct state by replaying blocks/transactions using
the existing state application logic (e.g., the same functions that apply
transactions/mining rewards used in main.py) and then replace
blockchain.state.accounts with the reconstructed state, or (b) persist and load
a verifiable state root/signature alongside the snapshot and check that the
computed state root from the reconstructed state matches before accepting
raw_accounts (use the _verify_chain_integrity and a new compute_state_root or
signature verification routine to compare). Ensure references to Blockchain,
blockchain.state.accounts, raw_accounts, and _verify_chain_integrity are used so
the change replaces the unsafe direct assignment with state reconstruction or
state-root verification.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
minichain/persistence.py (1)
101-105:⚠️ Potential issue | 🟠 MajorThe accounts snapshot is still unauthenticated.
After
_verify_chain_integrity(blocks), Line 105 installsnormalized_accountswholesale. Editing oneaccounts.account_jsonrow can still rewrite balances or nonces on startup without touching any block. Please rebuild state from the verified chain or verify a persisted state root/signature before assigningblockchain.state.accounts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/persistence.py` around lines 101 - 105, The code assigns normalized_accounts directly to blockchain.state.accounts after calling _verify_chain_integrity(blocks), leaving the persisted snapshot unauthenticated; instead either (A) rebuild the state from the verified chain by creating the Blockchain instance, setting blockchain.chain = blocks and then calling the state-reconstruction routine (e.g., a method like Blockchain.rebuild_state_from_chain or iterating blocks and applying each block/transaction via State.apply_transaction) to derive blockchain.state.accounts, or (B) if you must trust a stored snapshot, verify its integrity first by checking the persisted state root/signature against the chain (e.g., verify_state_root(normalized_accounts) or validate signature) before assigning to blockchain.state.accounts; update the code around _verify_chain_integrity, Blockchain, blockchain.chain, blockchain.state.accounts and normalized_accounts to implement one of these secure flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/persistence.py`:
- Around line 205-211: The code calls _initialize_schema before reading which
will silently create missing tables; instead, stop creating schema during load:
remove or skip calling _initialize_schema in the loading path and add an
explicit existence check for the required tables (accounts, blocks) prior to
executing the SELECTs in the load routine (the block that runs
conn.execute("SELECT block_json FROM blocks ...") and conn.execute("SELECT
address, account_json FROM accounts ...")). If a table is missing, raise an
explicit error (or re-raise sqlite OperationalError) indicating database
corruption/missing table rather than recreating an empty table; alternatively
catch the SELECT failure and convert it to a clear corruption error mentioning
the missing table names so startup fails fast.
- Around line 95-100: The loop that builds normalized_accounts from raw_accounts
must validate the full account schema before accepting entries: in the block
that iterates raw_accounts (where normalized_accounts and path are used), ensure
each account dict contains required keys "balance" (int), "nonce" (int), "code"
(str or None), and "storage" (dict), and that no extra malformed types are
present; implement or call a small helper like validate_account_schema(account,
path, address) that raises ValueError including path/address on any mismatch,
and only then assign normalized_accounts[address] = account so corrupted
snapshots are rejected early and State (which expects balance/nonce/code/storage
types) never receives invalid data.
---
Duplicate comments:
In `@minichain/persistence.py`:
- Around line 101-105: The code assigns normalized_accounts directly to
blockchain.state.accounts after calling _verify_chain_integrity(blocks), leaving
the persisted snapshot unauthenticated; instead either (A) rebuild the state
from the verified chain by creating the Blockchain instance, setting
blockchain.chain = blocks and then calling the state-reconstruction routine
(e.g., a method like Blockchain.rebuild_state_from_chain or iterating blocks and
applying each block/transaction via State.apply_transaction) to derive
blockchain.state.accounts, or (B) if you must trust a stored snapshot, verify
its integrity first by checking the persisted state root/signature against the
chain (e.g., verify_state_root(normalized_accounts) or validate signature)
before assigning to blockchain.state.accounts; update the code around
_verify_chain_integrity, Blockchain, blockchain.chain, blockchain.state.accounts
and normalized_accounts to implement one of these secure flows.
🪄 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: 1428837a-2ae0-4bc6-b265-548336f6cc0e
📒 Files selected for processing (2)
minichain/persistence.pytests/test_persistence.py
Comment on lines
+95
to
100
| normalized_accounts = {} | ||
| for address, account in raw_accounts.items(): | ||
| if not isinstance(address, str) or not isinstance(account, dict): | ||
| raise ValueError(f"Invalid accounts data in '{path}'") | ||
| normalized_accounts[address] = account | ||
|
|
Contributor
There was a problem hiding this comment.
Validate the full account schema before accepting it.
{} or {"storage": []} passes the current dict check, but State assumes balance/nonce are integers, code is str | None, and storage is a dict. That lets a corrupted snapshot boot and fail later on the first account read.
🛡️ Suggested validation
normalized_accounts = {}
for address, account in raw_accounts.items():
if not isinstance(address, str) or not isinstance(account, dict):
raise ValueError(f"Invalid accounts data in '{path}'")
+ missing = {"balance", "nonce", "code", "storage"} - account.keys()
+ if missing:
+ raise ValueError(f"Invalid accounts data in '{path}'")
+ if (
+ type(account["balance"]) is not int
+ or type(account["nonce"]) is not int
+ or (account["code"] is not None and not isinstance(account["code"], str))
+ or not isinstance(account["storage"], dict)
+ ):
+ raise ValueError(f"Invalid accounts data in '{path}'")
normalized_accounts[address] = account🧰 Tools
🪛 Ruff (0.15.7)
[warning] 98-98: Prefer TypeError exception for invalid type
(TRY004)
[warning] 98-98: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/persistence.py` around lines 95 - 100, The loop that builds
normalized_accounts from raw_accounts must validate the full account schema
before accepting entries: in the block that iterates raw_accounts (where
normalized_accounts and path are used), ensure each account dict contains
required keys "balance" (int), "nonce" (int), "code" (str or None), and
"storage" (dict), and that no extra malformed types are present; implement or
call a small helper like validate_account_schema(account, path, address) that
raises ValueError including path/address on any mismatch, and only then assign
normalized_accounts[address] = account so corrupted snapshots are rejected early
and State (which expects balance/nonce/code/storage types) never receives
invalid data.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
minichain/persistence.py (2)
101-105:⚠️ Potential issue | 🟠 MajorStill accepting an unauthenticated account snapshot.
_verify_chain_integrity(blocks)only proves the block list. The next lines trustnormalized_accountswholesale, so editing oneaccounts.account_jsonrow can rewrite balances/nonces on startup without touching any block. Please reconstruct state from verified data or validate it against a verifiable state root/signature before assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/persistence.py` around lines 101 - 105, The code calls _verify_chain_integrity(blocks) but then blindly assigns normalized_accounts into blockchain.state.accounts (via Blockchain and blockchain.chain), allowing tampered accounts.account_json to overwrite state; instead reconstruct or validate state: derive the state by replaying transactions from the now-verified blocks into a fresh Blockchain/state object (e.g., create Blockchain(), set blockchain.chain = blocks, then call the existing state-reconstruction/replay method to build accounts from blocks), or verify normalized_accounts against a signed/state root stored in the verified blocks (compare computed state root or signature to the block header) before assigning to blockchain.state.accounts; update the code paths referencing blockchain.state.accounts and normalized_accounts accordingly so only reconstructed/verified state is used.
95-99:⚠️ Potential issue | 🟠 MajorStill missing full account schema validation.
This loop only checks
dict, but the restored state is later used as{"balance": int, "nonce": int, "code": str | None, "storage": dict}. Corrupted rows like{}or{"storage": []}will still load and then fail on first account access.🛡️ Suggested guardrail
normalized_accounts = {} for address, account in raw_accounts.items(): if not isinstance(address, str) or not isinstance(account, dict): raise ValueError(f"Invalid accounts data in '{path}'") + missing = {"balance", "nonce", "code", "storage"} - account.keys() + if missing: + raise ValueError(f"Invalid accounts data in '{path}'") + if ( + type(account["balance"]) is not int + or type(account["nonce"]) is not int + or (account["code"] is not None and not isinstance(account["code"], str)) + or not isinstance(account["storage"], dict) + ): + raise ValueError(f"Invalid accounts data in '{path}'") normalized_accounts[address] = account🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/persistence.py` around lines 95 - 99, The loop that builds normalized_accounts from raw_accounts only checks types of address and account but must validate the full account schema used elsewhere; inside the same loop (in persistence.py where normalized_accounts, raw_accounts and path are referenced) verify each account has keys "balance" and "nonce" as ints, "code" as either str or None, and "storage" as a dict (and reject other types/structures like empty dict or list storage), raising ValueError(f"Invalid account data in '{path}' for address {address}") on any mismatch so corrupted rows are rejected before being returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/persistence.py`:
- Around line 215-236: The load() function must validate the persisted
metadata.chain_length against the actual number of blocks returned to detect
truncated snapshots: after reading block_rows/account_rows and parsing chain,
read the metadata row (metadata.chain_length) that save() writes and compare
metadata.chain_length to len(chain); if they differ raise ValueError (e.g.,
"Invalid SQLite persistence data" or "Invalid persisted JSON payload") so
startup rejects rolled-back/truncated DBs; update the logic in load() (near
calls to _require_schema, the block_rows/account_rows parsing, and before
returning the {"chain": chain, "state": state}) to fetch and validate
metadata.chain_length and reference save(), load(), and metadata.chain_length
for locating the change.
In `@tests/test_persistence.py`:
- Around line 94-105: Update test_state_snapshot_preserved to assert the entire
restored account objects instead of only balances: when comparing
restored.state.get_account(alice_pk) and restored.state.get_account(bob_pk) to
bc.state.get_account(...), assert the full dicts (nonce, balance, code, storage,
etc.) so regressions in nonce/code/storage are caught; use the existing
save/load cycle and the same get_account calls to locate where to change the
asserts in test_state_snapshot_preserved (note related
test_loaded_chain_can_add_new_block uses a fresh sender and does not exercise
the restored nonce).
---
Duplicate comments:
In `@minichain/persistence.py`:
- Around line 101-105: The code calls _verify_chain_integrity(blocks) but then
blindly assigns normalized_accounts into blockchain.state.accounts (via
Blockchain and blockchain.chain), allowing tampered accounts.account_json to
overwrite state; instead reconstruct or validate state: derive the state by
replaying transactions from the now-verified blocks into a fresh
Blockchain/state object (e.g., create Blockchain(), set blockchain.chain =
blocks, then call the existing state-reconstruction/replay method to build
accounts from blocks), or verify normalized_accounts against a signed/state root
stored in the verified blocks (compare computed state root or signature to the
block header) before assigning to blockchain.state.accounts; update the code
paths referencing blockchain.state.accounts and normalized_accounts accordingly
so only reconstructed/verified state is used.
- Around line 95-99: The loop that builds normalized_accounts from raw_accounts
only checks types of address and account but must validate the full account
schema used elsewhere; inside the same loop (in persistence.py where
normalized_accounts, raw_accounts and path are referenced) verify each account
has keys "balance" and "nonce" as ints, "code" as either str or None, and
"storage" as a dict (and reject other types/structures like empty dict or list
storage), raising ValueError(f"Invalid account data in '{path}' for address
{address}") on any mismatch so corrupted rows are rejected before being
returned.
🪄 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: d194e02a-c119-4c60-80e2-8459d37e8bf5
📒 Files selected for processing (2)
minichain/persistence.pytests/test_persistence.py
Comment on lines
+94
to
105
| def test_state_snapshot_preserved(self): | ||
| bc, alice_pk, bob_pk = self._chain_with_tx() | ||
| save(bc, path=self.tmpdir) | ||
|
|
||
| restored = load(path=self.tmpdir) | ||
| # Alice started with 100, sent 30 → 70 | ||
| self.assertEqual( | ||
| restored.state.get_account(alice_pk)["balance"], | ||
| bc.state.get_account(alice_pk)["balance"], | ||
| ) | ||
| # Bob received 30 | ||
| self.assertEqual( | ||
| restored.state.get_account(bob_pk)["balance"], | ||
| bc.state.get_account(bob_pk)["balance"], | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert the full restored account state.
test_loaded_chain_can_add_new_block() now avoids Alice’s restored nonce by using a fresh sender, so this balance-only check is the remaining coverage on Alice’s snapshot. Comparing the full account dict here would keep nonce/code/storage regressions visible.
💡 Minimal test hardening
self.assertEqual(
- restored.state.get_account(alice_pk)["balance"],
- bc.state.get_account(alice_pk)["balance"],
+ restored.state.get_account(alice_pk),
+ bc.state.get_account(alice_pk),
)
self.assertEqual(
- restored.state.get_account(bob_pk)["balance"],
- bc.state.get_account(bob_pk)["balance"],
+ restored.state.get_account(bob_pk),
+ bc.state.get_account(bob_pk),
)📝 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
| def test_state_snapshot_preserved(self): | |
| bc, alice_pk, bob_pk = self._chain_with_tx() | |
| save(bc, path=self.tmpdir) | |
| restored = load(path=self.tmpdir) | |
| # Alice started with 100, sent 30 → 70 | |
| self.assertEqual( | |
| restored.state.get_account(alice_pk)["balance"], | |
| bc.state.get_account(alice_pk)["balance"], | |
| ) | |
| # Bob received 30 | |
| self.assertEqual( | |
| restored.state.get_account(bob_pk)["balance"], | |
| bc.state.get_account(bob_pk)["balance"], | |
| ) | |
| def test_state_snapshot_preserved(self): | |
| bc, alice_pk, bob_pk = self._chain_with_tx() | |
| save(bc, path=self.tmpdir) | |
| restored = load(path=self.tmpdir) | |
| self.assertEqual( | |
| restored.state.get_account(alice_pk), | |
| bc.state.get_account(alice_pk), | |
| ) | |
| self.assertEqual( | |
| restored.state.get_account(bob_pk), | |
| bc.state.get_account(bob_pk), | |
| ) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 98-98: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 102-102: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_persistence.py` around lines 94 - 105, Update
test_state_snapshot_preserved to assert the entire restored account objects
instead of only balances: when comparing restored.state.get_account(alice_pk)
and restored.state.get_account(bob_pk) to bc.state.get_account(...), assert the
full dicts (nonce, balance, code, storage, etc.) so regressions in
nonce/code/storage are caught; use the existing save/load cycle and the same
get_account calls to locate where to change the asserts in
test_state_snapshot_preserved (note related test_loaded_chain_can_add_new_block
uses a fresh sender and does not exercise the restored nonce).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/persistence.py`:
- Around line 49-53: The current snapshot uses
json.loads(json.dumps(blockchain.state.accounts)) to deep-copy state which is
less efficient; replace the JSON round-trip with copy.deepcopy to produce an
in-memory deep copy of blockchain.state.accounts while holding blockchain._lock
before calling _save_snapshot_to_sqlite(db_path, {"chain": chain_data, "state":
state_data}); update the import to include copy and use
copy.deepcopy(blockchain.state.accounts) and leave chain_data creation via
[block.to_dict() for block in blockchain.chain] as-is (unless you also want to
validate JSON serializability, in which case keep the round-trip).
- Around line 257-259: The open call in _read_legacy_json unnecessarily passes
the default mode "r"; remove the explicit "r" argument so the function uses
open(filepath, encoding="utf-8") when reading JSON, keeping json.load(f)
unchanged.
In `@tests/test_persistence.py`:
- Around line 122-135: The test test_broken_linkage_rejected currently sets
payload["previous_hash"] to "0" * 64 + "ff" which produces a 66-character string
and triggers length validation instead of linkage validation; update the
modification in the test to set payload["previous_hash"] to a 64-character hex
string that is different from the real previous hash (e.g., change one hex
digit) so the value has valid length but incorrect linkage, then save the
updated block_json and assert load(path=self.tmpdir) raises ValueError as
intended (locate the change around payload["previous_hash"] in
test_broken_linkage_rejected).
🪄 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: d6b03bbb-872a-4948-8e57-f565fa23ba1d
📒 Files selected for processing (2)
minichain/persistence.pytests/test_persistence.py
Comment on lines
+49
to
+53
| with blockchain._lock: | ||
| chain_data = [block.to_dict() for block in blockchain.chain] | ||
| state_data = copy.deepcopy(blockchain.state.accounts) | ||
| state_data = json.loads(json.dumps(blockchain.state.accounts)) | ||
|
|
||
| snapshot = { | ||
| "chain": chain_data, | ||
| "state": state_data | ||
| } | ||
|
|
||
| _atomic_write_json(os.path.join(path, _DATA_FILE), snapshot) | ||
| _save_snapshot_to_sqlite(db_path, {"chain": chain_data, "state": state_data}) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using copy.deepcopy() instead of JSON round-trip.
The json.loads(json.dumps(...)) pattern works but is less efficient than copy.deepcopy() for in-memory deep copying. However, if the intent is to validate JSON serializability at save time, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/persistence.py` around lines 49 - 53, The current snapshot uses
json.loads(json.dumps(blockchain.state.accounts)) to deep-copy state which is
less efficient; replace the JSON round-trip with copy.deepcopy to produce an
in-memory deep copy of blockchain.state.accounts while holding blockchain._lock
before calling _save_snapshot_to_sqlite(db_path, {"chain": chain_data, "state":
state_data}); update the import to include copy and use
copy.deepcopy(blockchain.state.accounts) and leave chain_data creation via
[block.to_dict() for block in blockchain.chain] as-is (unless you also want to
validate JSON serializability, in which case keep the round-trip).
Comment on lines
+257
to
259
| def _read_legacy_json(filepath: str) -> dict[str, Any]: | ||
| with open(filepath, "r", encoding="utf-8") as f: | ||
| return json.load(f) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Remove redundant mode argument.
The "r" mode is the default for open(). This is a nitpick from static analysis.
♻️ Suggested fix
def _read_legacy_json(filepath: str) -> dict[str, Any]:
- with open(filepath, "r", encoding="utf-8") as f:
+ with open(filepath, encoding="utf-8") as f:
return json.load(f)📝 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
| def _read_legacy_json(filepath: str) -> dict[str, Any]: | |
| with open(filepath, "r", encoding="utf-8") as f: | |
| return json.load(f) | |
| def _read_legacy_json(filepath: str) -> dict[str, Any]: | |
| with open(filepath, encoding="utf-8") as f: | |
| return json.load(f) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 258-258: Unnecessary mode argument
Remove mode argument
(UP015)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/persistence.py` around lines 257 - 259, The open call in
_read_legacy_json unnecessarily passes the default mode "r"; remove the explicit
"r" argument so the function uses open(filepath, encoding="utf-8") when reading
JSON, keeping json.load(f) unchanged.
Comment on lines
122
to
+135
| def test_broken_linkage_rejected(self): | ||
| """Loading a chain with broken previous_hash linkage must raise.""" | ||
| bc, _, _ = self._chain_with_tx() | ||
| save(bc, path=self.tmpdir) | ||
| db_path = os.path.join(self.tmpdir, DB_FILE) | ||
| with sqlite3.connect(db_path) as conn: | ||
| row = conn.execute("SELECT block_json FROM blocks WHERE height = 1").fetchone() | ||
| payload = json.loads(row[0]) | ||
| payload["previous_hash"] = "0" * 64 + "ff" | ||
| conn.execute( | ||
| "UPDATE blocks SET block_json = ? WHERE height = 1", | ||
| (json.dumps(payload),), | ||
| ) | ||
| with self.assertRaises(ValueError): | ||
| load(path=self.tmpdir) |
Contributor
There was a problem hiding this comment.
Test uses invalid hash length instead of wrong hash value.
Line 129 sets previous_hash to "0" * 64 + "ff" which is 66 characters, not a valid 64-character hash. This might cause rejection for hash length validation rather than linkage validation. Use a valid-length but wrong hash to properly test linkage rejection.
🧪 Suggested fix
payload = json.loads(row[0])
- payload["previous_hash"] = "0" * 64 + "ff"
+ payload["previous_hash"] = "f" * 64 # Valid length, wrong value
conn.execute(🧰 Tools
🪛 Ruff (0.15.7)
[warning] 134-134: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_persistence.py` around lines 122 - 135, The test
test_broken_linkage_rejected currently sets payload["previous_hash"] to "0" * 64
+ "ff" which produces a 66-character string and triggers length validation
instead of linkage validation; update the modification in the test to set
payload["previous_hash"] to a 64-character hex string that is different from the
real previous hash (e.g., change one hex digit) so the value has valid length
but incorrect linkage, then save the updated block_json and assert
load(path=self.tmpdir) raises ValueError as intended (locate the change around
payload["previous_hash"] in test_broken_linkage_rejected).
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.
Addressed Issues:
Summary
save(path)/load(path)APIpersistence_exists(path)and wire node startup/shutdown to the new SQLite persistence pathdata.jsonloading is still supported for backward compatibilityValidation
python -m pytest -q--datadirdata.dbis written--datadirAI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Refactor
Tests