Fix: persistence.py function calls in main#61
Open
SIDDHANTCOOKIE wants to merge 8 commits intoStabilityNexus:mainfrom
Open
Fix: persistence.py function calls in main#61SIDDHANTCOOKIE wants to merge 8 commits intoStabilityNexus:mainfrom
SIDDHANTCOOKIE wants to merge 8 commits intoStabilityNexus:mainfrom
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
WalkthroughAdds atomic on-disk persistence for the blockchain via a new persistence module, re-exports Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Persist as Persistence Module
participant Disk as Filesystem
participant Chain as Blockchain
App->>Persist: load(datadir) if provided
Persist->>Disk: read datadir/data.json
Persist->>Persist: _verify_chain_integrity(blocks)
Persist->>Chain: deserialize blocks & restore accounts
Persist-->>App: return Blockchain instance
App->>Chain: normal operations (mining, txs)
App->>Persist: save(chain, datadir) on shutdown
Persist->>Persist: acquire blockchain._lock & serialize snapshot
Persist->>Disk: atomic write datadir/temp -> datadir/data.json (fsync + rename)
Persist-->>App: persistence complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 Coding Plan
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 330-335: Wrap the persistence and network shutdown in a
try/except/finally so network.stop() always executes: call save(chain, datadir)
only if datadir is set, catch and log any exception from save (including
exception details) without re-raising, and then call await network.stop() in the
finally block; reference the existing save(...) call, the datadir check, and
await network.stop() to locate where to implement the try/except/finally.
- Around line 287-289: The current except block that catches (FileNotFoundError,
ValueError) and calls logger.warning should be split: only catch
FileNotFoundError and continue starting fresh (log with file path), but treat
ValueError as fatal — log the error (include exception details and the persisted
filename) and re-raise or exit non‑zero to avoid silently overwriting
corrupted/tampered data; update the block that currently uses "except
(FileNotFoundError, ValueError) as e" and the logger.warning call accordingly so
ValueError is not swallowed.
In `@minichain/persistence.py`:
- Around line 151-153: The write currently uses os.fdopen(fd) and json.dump then
os.replace(tmp_path, filepath) but doesn't fsync, risking loss on crash; update
the write path around the with-block that uses fd/tmp_path/filepath so that
after json.dump you call f.flush() and os.fsync(f.fileno()) (or os.fsync(fd))
before closing/replacing, then after os.replace() open the containing directory
(os.path.dirname(filepath) or '.' if empty) and call os.fsync(dir_fd) on that
directory file descriptor (then close it) to ensure the rename is durable across
crashes.
- Around line 52-53: The two separate calls to _atomic_write_json for
_CHAIN_FILE and _STATE_FILE can produce a torn snapshot if a crash occurs
between them; modify the save procedure to perform a snapshot-id handshake:
write chain and state to their temp/atomic files including a common snapshot_id
(or write them then create a single atomic marker file containing snapshot_id),
then atomically publish the marker (or snapshot_id) only after both writes
succeed; update load() to read the marker and verify the snapshot_id matches in
both _CHAIN_FILE and _STATE_FILE (reject or retry if mismatched). Use the
existing _atomic_write_json and the constants _CHAIN_FILE and _STATE_FILE and
add a single-marker file (or embedded snapshot_id field) to guarantee both files
belong to the same snapshot before accepting them.
In `@tests/test_persistence.py`:
- Around line 104-119: The test test_state_recomputed_from_blocks currently only
round-trips state and won't detect if load() trusts a persisted state file;
modify the test so after calling save(bc, path=self.tmpdir) you explicitly
tamper the persisted state file (e.g., open the saved state JSON under
self.tmpdir and change Alice/Bob balances to bogus values) and write it back,
then call restored = load(path=self.tmpdir) and assert
restored.state.get_account(...) equals bc.state.get_account(...) (not the
tampered file values), ensuring replay-from-blocks behavior is validated while
keeping the test name.
🪄 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: 47ed40aa-4d9e-4404-b721-939b57e5e120
📒 Files selected for processing (4)
main.pyminichain/__init__.pyminichain/persistence.pytests/test_persistence.py
Comment on lines
+104
to
+119
| def test_state_recomputed_from_blocks(self): | ||
| """Balances must be recomputed by replaying blocks, not from a file.""" | ||
| 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.
Test intent and assertions are mismatched.
Line 105 says state must be recomputed from blocks, but this test only checks round-trip equality with persisted state. It won’t catch regressions where loader trusts a tampered state.json.
Suggested correction
- def test_state_recomputed_from_blocks(self):
- """Balances must be recomputed by replaying blocks, not from a file."""
+ def test_state_round_trip_preserved(self):
+ """Balances should round-trip correctly through persistence."""Or keep the current name and add tamper logic to assert replay behavior explicitly.
📝 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_recomputed_from_blocks(self): | |
| """Balances must be recomputed by replaying blocks, not from a file.""" | |
| 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_round_trip_preserved(self): | |
| """Balances should round-trip correctly through persistence.""" | |
| 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"], | |
| ) |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 111-111: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 116-116: 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 104 - 119, The test
test_state_recomputed_from_blocks currently only round-trips state and won't
detect if load() trusts a persisted state file; modify the test so after calling
save(bc, path=self.tmpdir) you explicitly tamper the persisted state file (e.g.,
open the saved state JSON under self.tmpdir and change Alice/Bob balances to
bogus values) and write it back, then call restored = load(path=self.tmpdir) and
assert restored.state.get_account(...) equals bc.state.get_account(...) (not the
tampered file values), ensuring replay-from-blocks behavior is validated while
keeping the test name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/test_persistence.py (1)
103-118:⚠️ Potential issue | 🟡 MinorTest intent and assertions are still mismatched.
The name/docstring says “recomputed from blocks,” but this only checks round-trip equality from persisted state; it does not verify replay behavior.
Proposed adjustment
- def test_state_recomputed_from_blocks(self): - """Balances must be recomputed by replaying blocks, not from a file.""" + def test_state_round_trip_preserved(self): + """Balances should round-trip correctly through persistence."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_persistence.py` around lines 103 - 118, The test test_state_recomputed_from_blocks currently only checks round-trip equality; change it to assert that state is recomputed from blocks by tampering with or removing the persisted state before calling load and verifying restored.state equals bc.state (the recomputed state from blocks) and not the tampered-on-disk state; specifically, use the existing helpers _chain_with_tx, save(path=self.tmpdir) and load(path=self.tmpdir), alter the saved state file in self.tmpdir (or delete it) to an incorrect value, then call load and assert restored.state.get_account(alice_pk)["balance"] == bc.state.get_account(alice_pk)["balance"] and similarly for bob_pk, ensuring the test proves replay-from-blocks rather than trusting persisted state.
🤖 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 165-170: The code opens a directory descriptor into dir_fd with
os.open and calls os.fsync(dir_fd) but if os.fsync raises the current except
block skips os.close(dir_fd) and leaks the descriptor; wrap the fsync/close in a
try/finally (or use contextlib.closing) so os.close(dir_fd) always runs (e.g.,
set dir_fd = os.open(...); try: os.fsync(dir_fd) finally: os.close(dir_fd)) and
keep the existing OSError handling around the whole open/fsync/close sequence.
- Around line 48-55: The snapshot code currently does a shallow copy of
blockchain.state.accounts after releasing the lock, which allows nested account
dicts to be mutated and produce inconsistent snapshots; while holding
blockchain._lock, replace the shallow copy (blockchain.state.accounts.copy())
with a deep copy (e.g., use copy.deepcopy(blockchain.state.accounts) after
importing copy) so the snapshot's "state" is fully frozen at the same point in
time as chain_data (chain derived from [block.to_dict() for block in
blockchain.chain]); ensure the deep copy happens inside the with
blockchain._lock block before constructing the snapshot.
In `@tests/test_persistence.py`:
- Around line 182-187: The test signs tx2 with the wrong private key; ensure the
transaction is signed with the private key that corresponds to tx2.sender (the
new keypair from _make_keypair()). Replace the current unpack and sign usage so
you capture the private key for the new public key (e.g., new_sk, new_pk =
_make_keypair()) and call tx2.sign(new_sk) instead of tx2.sign(alice_sk),
referencing _make_keypair, new_pk, alice_sk, and tx2.sign to locate the change.
---
Duplicate comments:
In `@tests/test_persistence.py`:
- Around line 103-118: The test test_state_recomputed_from_blocks currently only
checks round-trip equality; change it to assert that state is recomputed from
blocks by tampering with or removing the persisted state before calling load and
verifying restored.state equals bc.state (the recomputed state from blocks) and
not the tampered-on-disk state; specifically, use the existing helpers
_chain_with_tx, save(path=self.tmpdir) and load(path=self.tmpdir), alter the
saved state file in self.tmpdir (or delete it) to an incorrect value, then call
load and assert restored.state.get_account(alice_pk)["balance"] ==
bc.state.get_account(alice_pk)["balance"] and similarly for bob_pk, ensuring the
test proves replay-from-blocks rather than trusting persisted state.
🪄 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: 66b9fa6c-5388-4a5d-92ea-caa14f4b719b
📒 Files selected for processing (3)
main.pyminichain/persistence.pytests/test_persistence.py
Contributor
|
@Zahnentferner and @SIDDHANTCOOKIE |
Contributor
Author
why are you deleting and re-commenting the same thing
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
tests/test_persistence.py (1)
103-119:⚠️ Potential issue | 🟡 MinorTest name and docstring don't match actual behavior.
The test is named
test_state_recomputed_from_blockswith docstring claiming "Balances must be recomputed by replaying blocks, not from a file." However:
- The
load()implementation directly assignsraw_accountsfrom the file (line 105 in persistence.py)- This test only verifies round-trip equality, not replay behavior
Either rename the test to
test_state_round_trip_preservedor implement actual tamper detection by modifying the saved state and verifying it's recomputed from blocks.📝 Suggested rename
- def test_state_recomputed_from_blocks(self): - """Balances must be recomputed by replaying blocks, not from a file.""" + def test_state_round_trip_preserved(self): + """Account balances should round-trip correctly through persistence."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_persistence.py` around lines 103 - 119, The test name and docstring are misleading: update the test function test_state_recomputed_from_blocks to reflect its actual behavior (round-trip equality) by renaming it to test_state_round_trip_preserved and change the docstring to state that balances are preserved across save/load; also adjust any references to the old name in the test suite. Alternatively, if you want to enforce recomputation from blocks instead, modify the load() logic (where raw_accounts is assigned) to ignore or validate raw_accounts and then implement a tamper test that alters the saved raw_accounts before loading and asserts balances are recomputed from blocks; reference the test function test_state_recomputed_from_blocks, the load() function, and the raw_accounts variable when making changes.
🤖 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 4-6: The module docstring is out of sync with the implementation:
it describes separate files `blockchain.json` and `state.json` but the code uses
a single `_DATA_FILE = "data.json"`. Update the top-level docstring in
minichain/persistence.py to accurately describe the single-file design (that
`_DATA_FILE` stores both the serialized chain and the accounts/state) and
mention any format/keys used, referencing the `_DATA_FILE` constant and the
persistence class or functions (e.g., Persistence or load/save methods) so
maintainers can find where the file is read/written.
- Around line 122-125: The genesis block validation only checks genesis.index
and genesis.hash but misses genesis.previous_hash; update the check around the
genesis variable (in minichain/persistence.py where genesis = blocks[0]) to also
assert genesis.previous_hash == "0" (or "0" * 64 if the codebase represents it
as 64 zeros) and raise a ValueError with a clear message (e.g., "Invalid genesis
block: bad previous_hash") when this condition fails so malformed genesis blocks
are rejected.
- Around line 184-188: Remove the TOCTOU-prone os.path.exists() check from the
_read_json function and rely on opening the file directly; if open() raises
FileNotFoundError (or other IO errors) let it propagate or catch-and-re-raise
with a clearer message, but do not perform the separate existence check before
calling open() in _read_json.
- Around line 104-105: Validate and deep-copy raw_accounts before assigning to
blockchain.state.accounts: ensure raw_accounts is a dict, iterate its items
(account IDs → account data) and verify required fields like "balance" and
"nonce" exist and are of the expected types, reject or sanitize malformed
entries, then assign a deep copy (e.g., via copy.deepcopy) to
blockchain.state.accounts to avoid sharing the parsed JSON object; perform this
check/assignment in the restore logic where raw_accounts is currently assigned
directly to blockchain.state.accounts.
- Around line 213-215: When loading blocks, avoid overwriting the computed
merkle_root from Block.__init__ with a None from legacy data; change the
assignment using data.get("merkle_root") so that block.merkle_root is only set
when the stored value exists and is not None (e.g., check "merkle_root" in data
and data["merkle_root"] is not None) instead of unconditionally assigning; this
ensures _verify_chain_integrity continues to use the computed merkle_root for
older blocks.
---
Duplicate comments:
In `@tests/test_persistence.py`:
- Around line 103-119: The test name and docstring are misleading: update the
test function test_state_recomputed_from_blocks to reflect its actual behavior
(round-trip equality) by renaming it to test_state_round_trip_preserved and
change the docstring to state that balances are preserved across save/load; also
adjust any references to the old name in the test suite. Alternatively, if you
want to enforce recomputation from blocks instead, modify the load() logic
(where raw_accounts is assigned) to ignore or validate raw_accounts and then
implement a tamper test that alters the saved raw_accounts before loading and
asserts balances are recomputed from blocks; reference the test function
test_state_recomputed_from_blocks, the load() function, and the raw_accounts
variable when making changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b00e318-1baf-41ec-985b-4f5c81a75927
📒 Files selected for processing (2)
minichain/persistence.pytests/test_persistence.py
Comment on lines
+4
to
+6
| Design: | ||
| - blockchain.json holds the full list of serialised blocks | ||
| - state.json holds the accounts dict (includes off-chain credits) |
Contributor
There was a problem hiding this comment.
Docstring describes two-file design but implementation uses single file.
The docstring mentions blockchain.json and state.json as separate files, but the implementation uses a single _DATA_FILE = "data.json" containing both chain and state. This inconsistency could confuse future maintainers.
📝 Proposed fix to update docstring
Design:
- - blockchain.json holds the full list of serialised blocks
- - state.json holds the accounts dict (includes off-chain credits)
+ - data.json holds both the serialised blocks and account state in a single
+ atomic snapshot to prevent torn writes on crash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/persistence.py` around lines 4 - 6, The module docstring is out of
sync with the implementation: it describes separate files `blockchain.json` and
`state.json` but the code uses a single `_DATA_FILE = "data.json"`. Update the
top-level docstring in minichain/persistence.py to accurately describe the
single-file design (that `_DATA_FILE` stores both the serialized chain and the
accounts/state) and mention any format/keys used, referencing the `_DATA_FILE`
constant and the persistence class or functions (e.g., Persistence or load/save
methods) so maintainers can find where the file is read/written.
Contributor
That was by mistake I thought. but the last one is actual. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 101-105: The load() function currently constructs a fresh
Blockchain() and then assigns blockchain.chain and blockchain.state.accounts
without holding blockchain._lock; to fix, acquire the instance's lock
(blockchain._lock) before replacing blockchain.chain and
blockchain.state.accounts (or otherwise ensure the returned instance is
documented as not safe for concurrent use during load); update the load()
implementation to lock around the assignments so they are atomic with respect to
add_block() and mirror the save() locking discipline used elsewhere.
🪄 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: dccf5429-61c8-414d-bffe-a3fb2723ffc2
📒 Files selected for processing (1)
minichain/persistence.py
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
2 participants
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:
The previous pr has no function calls from persistence.py in main.py
so it was originally just dead code with no use at all
this pr fixes that and hardens persistence.py for any mid chain crashes
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI 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
New Features
Tests