persist blockchain across sessions via save/load#56
Closed
aniket866 wants to merge 4 commits intoStabilityNexus:mainfrom
Closed
persist blockchain across sessions via save/load#56aniket866 wants to merge 4 commits intoStabilityNexus:mainfrom
aniket866 wants to merge 4 commits intoStabilityNexus:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds JSON-based persistence: new Changes
Sequence DiagramsequenceDiagram
participant Client as Client/User
participant Persist as Persistence\n(save/load)
participant FS as Filesystem
participant Deser as Deserializer
participant Blockchain as Blockchain\n(instance)
Client->>Persist: save(blockchain, path)
Persist->>FS: write "blockchain.json"
Persist->>FS: write "state.json"
FS-->>Persist: write ack
Client->>Persist: load(path)
Persist->>FS: read "blockchain.json"
FS-->>Persist: chain JSON
Persist->>FS: read "state.json"
FS-->>Persist: state JSON
Persist->>Deser: deserialize blocks & transactions
Deser-->>Persist: reconstructed Blocks
Persist->>Blockchain: instantiate and assign chain/state
Blockchain-->>Persist: ready instance
Persist-->>Client: return Blockchain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 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: 7
🤖 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 75-88: The function currently validates raw_blocks but assigns
raw_accounts directly; add a validation step before creating blockchain.state to
ensure raw_accounts is a dict (and non-empty if desired), e.g. verify
isinstance(raw_accounts, dict) and raise a ValueError with context including
chain_path if the check fails; then proceed to create State via
State.__new__(State) and assign blockchain.state.accounts = raw_accounts and
blockchain.state.contract_machine = ContractMachine(blockchain.state) as before,
referencing the existing symbols raw_accounts, State, ContractMachine,
blockchain.state, and chain_path to locate the insertion point.
- Around line 79-81: Move the standard-library import "threading" out of the
local scope and place it at module-level with the other imports (so remove the
inline "import threading" inside the function) and update any references to
threading.Thread or threading.Lock in this module accordingly; specifically,
remove the local import in the function whereState and ContractMachine are used
and add a single top-level "import threading" next to the other imports so the
module uses the shared top-level threading name.
- Line 109: Remove the unnecessary explicit "r" mode when opening files: locate
the with open(filepath, "r", encoding="utf-8") as f usage in persistence.py (the
open call that reads the file into f) and change it to with open(filepath,
encoding="utf-8") as f so the default read mode is used and functionality
remains identical.
In `@tests/test_persistence.py`:
- Line 116: The test unpacks alice_pk and bob_pk from self._chain_with_tx() but
never uses them; change the unpacking to prefix unused variables with
underscores (e.g., bc, _alice_pk, _bob_pk) to signal they are intentionally
ignored and avoid lint warnings—update the tuple assignment where
self._chain_with_tx() is called in the test to use the underscore-prefixed
names.
- Line 71: Update the zip call in the test loop that iterates over bc.chain and
restored.chain to use strict=True so the comparison fails if lengths differ;
specifically modify the loop in tests/test_persistence.py (the for original,
loaded in zip(bc.chain, restored.chain) loop) to pass strict=True to zip() to
enforce equal-length iteration.
- Around line 24-25: The setUp method creates a temp directory (self.tmpdir) but
never removes it; add a tearDown method that checks for and removes self.tmpdir
(e.g., using shutil.rmtree) to clean up after each test. Implement tearDown in
the same test class that defines setUp, guard against missing/None self.tmpdir,
and handle exceptions (or ignore errors) so test teardown never fails.
- Around line 169-170: The test test_contract_storage_preserved currently only
asserts restored.state.get_account(contract_addr)["code"] equals code; add an
assertion to also verify the contract storage survived the save/load cycle by
fetching the account via restored.state.get_account(contract_addr) and asserting
that its storage (e.g. contract["storage"]["hits"] or the appropriate storage
key) equals 1, ensuring storage["hits"] is present and has the expected value
after restore.
🪄 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: 66734a3a-c93b-4296-bffc-fe3db8634f61
📒 Files selected for processing (3)
minichain/__init__.pyminichain/persistence.pytests/test_persistence.py
11 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
minichain/persistence.py (2)
113-113: 🧹 Nitpick | 🔵 TrivialRemove unnecessary mode argument.
The
"r"mode is the default foropen()and can be omitted.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/persistence.py` at line 113, Remove the redundant explicit read mode in the open() call: change the call using open(filepath, "r", encoding="utf-8") to open(filepath, encoding="utf-8") (the default mode is "r"); update the use found where the file is opened with the variable filepath so only encoding is passed.
83-85: 🧹 Nitpick | 🔵 TrivialMove
threadingimport to module level.The
threadingimport is a standard library module with no circular import concerns. Moving it to the module level with other imports improves clarity and follows Python conventions.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/persistence.py` around lines 83 - 85, Move the local import of the standard-library module threading to the module-level imports in minichain/persistence.py (alongside other imports like State and ContractMachine) to follow Python conventions; update any references to threading in the file (e.g., inside the Persistence class or functions that currently import it locally) to use the top-level import instead.
🤖 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 312-318: Replace the current exception logging in the save block
to preserve the traceback: inside the try/except around os.makedirs(DATA_DIR,
exist_ok=True) and save(chain, path=DATA_DIR), change the logger.error call that
references the exception variable to logger.exception so the stack trace for the
failure is recorded (keep the same message context mentioning DATA_DIR/chain
length); leave await network.stop() as-is.
- Around line 259-270: The current broad except in the chain load block masks
unexpected errors; update the try/except around load(path=DATA_DIR) to catch
only the expected exceptions (e.g., FileNotFoundError, ValueError, and
json.JSONDecodeError) and fall back to creating a new Blockchain() for those
cases, while letting other exceptions propagate; ensure json is imported if
using json.JSONDecodeError and keep the logger.warning call to report the
specific exception for the handled cases, referencing load(), Blockchain,
DATA_DIR, chain_file, and logger.
---
Duplicate comments:
In `@minichain/persistence.py`:
- Line 113: Remove the redundant explicit read mode in the open() call: change
the call using open(filepath, "r", encoding="utf-8") to open(filepath,
encoding="utf-8") (the default mode is "r"); update the use found where the file
is opened with the variable filepath so only encoding is passed.
- Around line 83-85: Move the local import of the standard-library module
threading to the module-level imports in minichain/persistence.py (alongside
other imports like State and ContractMachine) to follow Python conventions;
update any references to threading in the file (e.g., inside the Persistence
class or functions that currently import it locally) to use the top-level import
instead.
🪄 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: d8890ee0-41ff-44a4-8bf4-b45c069b4164
📒 Files selected for processing (3)
.gitignoremain.pyminichain/persistence.py
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 `@main.py`:
- Around line 333-336: The persistence layer is not crash-safe: the save() call
in main.py writes blockchain.json and state.json sequentially and can leave
partial/corrupt files if the process crashes; update minichain/persistence.py to
perform atomic writes by writing each output to a temporary file (e.g.,
blockchain.json.tmp / state.json.tmp) and then atomically replacing the target
using os.replace(), ensuring any directory creation and fsyncs as needed, and
update the save() usage (or its implementation) so callers like the save(chain,
path=DATA_DIR) invocation remain unchanged; if you prefer the demo route, add a
short note in the README indicating that corrupted node_data/ can be recovered
by deleting the directory.
- Around line 89-90: Remove the duplicate/inaccurate log: keep the accurate
logger.info call that uses mineable_txs and the emoji, and delete the earlier
logger.info that references pending_txs; update any nearby comments if needed to
reflect that the message reports the number of mineable (valid) transactions for
mined_block (references: logger.info, mined_block, pending_txs, mineable_txs).
- Line 23: Remove the unused import of the re module from main.py by deleting
the "import re" statement; ensure no other code references the re symbol and run
linters/tests to confirm no side effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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:
Fixes #57
screen-capture.41.online-video-cutter.com.mp4
MiniChain Persistence Test — 6 Steps
Step 1 — Start the node
Note your address printed on screen.
Step 2 — Send coins
Step 3 — Mine the block
Expected output:
Step 4 — Check the chain
Expected output:
Step 5 — Quit (saves chain to disk)
Expected output:
Step 6 — Restart and verify
Expected output:
Then type:
Same 2 blocks appear — persistence is working.
Reset (start fresh)
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: Chatgpt
Checklist
Summary by CodeRabbit