⚡ Bolt: Optimized blockchain integrity system with last-hash caching#501
⚡ Bolt: Optimized blockchain integrity system with last-hash caching#501RohanExploit wants to merge 1 commit intomainfrom
Conversation
Identified and implemented two major performance improvements for the blockchain-style integrity feature:
1. Verification Optimization: Added `previous_integrity_hash` column to the `Issue` model and indexed it. This allows the `/api/issues/{id}/blockchain-verify` endpoint to validate integrity in O(1) time with a single database lookup, instead of needing an additional query to find the parent record.
2. Creation Optimization: Introduced `blockchain_last_hash_cache` in `backend/cache.py`. The `create_issue` endpoint now retrieves the parent hash from cache, avoiding an expensive `ORDER BY id DESC` database query on every report submission.
These changes measurably reduce database I/O and latency for core civic reporting flows.
- Added `previous_integrity_hash` to `backend/models.py`
- Implemented `blockchain_last_hash_cache` in `backend/cache.py`
- Optimized `create_issue` and `verify_blockchain_integrity` in `backend/routers/issues.py`
- Added database migration for the new column in `backend/init_db.py`
- Updated `tests/test_blockchain.py` to verify the new optimizations.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for fixmybharat canceled.
|
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
📝 WalkthroughWalkthroughThe pull request implements blockchain-like integrity chaining for issues. It adds a Changes
Sequence DiagramssequenceDiagram
participant Client
participant Router as Issue Router
participant Cache as Cache Layer
participant Database
rect rgba(100, 200, 150, 0.5)
Note over Client,Database: Issue Creation Flow with Previous Hash Linking
end
Client->>Router: POST /issues (create new issue)
Router->>Cache: Get blockchain_last_hash_cache
alt Cache hit
Cache-->>Router: Return previous_hash
else Cache miss
Router->>Database: Query last integrity_hash
Database-->>Router: Return hash
Router->>Cache: Populate cache with hash
end
Router->>Router: Compute integrity_hash for new issue
Router->>Database: Create Issue with previous_integrity_hash
Database-->>Router: Issue created
Router->>Cache: Update cache with new integrity_hash
Router-->>Client: Return created issue
sequenceDiagram
participant Client
participant Router as Issue Router
participant Database
participant Verifier as Verification Logic
rect rgba(150, 150, 200, 0.5)
Note over Client,Verifier: Blockchain Integrity Verification Flow
end
Client->>Router: GET /verify-blockchain/{issue_id}
Router->>Database: Query issue with previous_integrity_hash
Database-->>Router: Return current_issue
alt current_issue has previous_integrity_hash stored
Router->>Verifier: Use stored previous_integrity_hash
else Legacy record (no stored hash)
Router->>Database: Query last issue hash
Database-->>Router: Return previous hash
end
Verifier->>Verifier: Recompute hash using previous + current data
Verifier->>Verifier: Compare computed vs stored integrity_hash
Verifier-->>Router: Validation result
Router-->>Client: Return is_valid, chain verification
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/routers/issues.py">
<violation number="1" location="backend/routers/issues.py:176">
P1: Using a process-local last-hash cache to build `previous_integrity_hash` can create incorrect parent links (forked chain) across workers/processes.</violation>
<violation number="2" location="backend/routers/issues.py:646">
P2: Integrity verification is weakened by trusting `previous_integrity_hash` directly instead of validating against the canonical previous issue hash.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
| prev_hash = prev_issue[0] if prev_issue and prev_issue[0] else "" | ||
| # Optimization: Use cache for the last hash to maintain the chain with near-zero overhead | ||
| prev_hash = blockchain_last_hash_cache.get("last_integrity_hash") |
There was a problem hiding this comment.
P1: Using a process-local last-hash cache to build previous_integrity_hash can create incorrect parent links (forked chain) across workers/processes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/issues.py, line 176:
<comment>Using a process-local last-hash cache to build `previous_integrity_hash` can create incorrect parent links (forked chain) across workers/processes.</comment>
<file context>
@@ -172,13 +172,18 @@ async def create_issue(
- )
- prev_hash = prev_issue[0] if prev_issue and prev_issue[0] else ""
+ # Optimization: Use cache for the last hash to maintain the chain with near-zero overhead
+ prev_hash = blockchain_last_hash_cache.get("last_integrity_hash")
+
+ if prev_hash is None:
</file context>
| if current_issue.previous_integrity_hash is not None: | ||
| prev_hash = current_issue.previous_integrity_hash | ||
| else: | ||
| # Fallback for legacy records without previous_integrity_hash column populated | ||
| prev_issue_hash = await run_in_threadpool( | ||
| lambda: db.query(Issue.integrity_hash).filter(Issue.id < issue_id).order_by(Issue.id.desc()).first() | ||
| ) | ||
| prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else "" |
There was a problem hiding this comment.
P2: Integrity verification is weakened by trusting previous_integrity_hash directly instead of validating against the canonical previous issue hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/issues.py, line 646:
<comment>Integrity verification is weakened by trusting `previous_integrity_hash` directly instead of validating against the canonical previous issue hash.</comment>
<file context>
@@ -620,24 +629,28 @@ def get_user_issues(
-
- prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else ""
+ # Use stored previous hash if available, otherwise fall back to DB lookup (for legacy records)
+ if current_issue.previous_integrity_hash is not None:
+ prev_hash = current_issue.previous_integrity_hash
+ else:
</file context>
| if current_issue.previous_integrity_hash is not None: | |
| prev_hash = current_issue.previous_integrity_hash | |
| else: | |
| # Fallback for legacy records without previous_integrity_hash column populated | |
| prev_issue_hash = await run_in_threadpool( | |
| lambda: db.query(Issue.integrity_hash).filter(Issue.id < issue_id).order_by(Issue.id.desc()).first() | |
| ) | |
| prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else "" | |
| prev_issue_hash = await run_in_threadpool( | |
| lambda: db.query(Issue.integrity_hash).filter(Issue.id < issue_id).order_by(Issue.id.desc()).first() | |
| ) | |
| prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else "" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/models.py (1)
152-152: Align model column type with migration schema definition.Line 152 uses unbounded
String, while the migration at line 73 ofbackend/init_db.pyexplicitly createsVARCHAR(255). This causes schema drift: fresh environments usingcreate_all()will differ from migrated environments. UseString(255)in the model to ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models.py` at line 152, The model column previous_integrity_hash in backend/models.py is declared as an unbounded String which conflicts with the migration that defines this field as VARCHAR(255); update the Column declaration for previous_integrity_hash to use a bounded String(255) so the SQLAlchemy model matches the migration schema and prevents schema drift (locate the previous_integrity_hash Column in the class definition and change its type argument accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routers/issues.py`:
- Around line 176-185: The current code reads prev_hash from
blockchain_last_hash_cache and then computes/creates a new block without global
serialization, causing TOCTOU forks; replace this local-cache-only pattern by
acquiring a DB-backed chain-head lock/row when generating a new issue: within a
DB transaction select the chain-head row (or a dedicated single-row table) FOR
UPDATE to read the authoritative parent hash (instead of the cache), compute and
persist the new Issue (using Issue.integrity_hash logic), update the chain-head
row to point to the new hash, commit, and only then update
blockchain_last_hash_cache with the committed value; ensure the code paths
referencing prev_hash, blockchain_last_hash_cache, and the
run_in_threadpool/Issue.integrity_hash query use this transactional lock to
serialize head assignment across processes.
In `@tests/test_blockchain.py`:
- Line 100: Replace the equality comparison asserting a boolean on the response
with a direct truth assertion: change the assertion that checks
response.json()["is_valid"] == True to assert response.json()["is_valid"] so the
test uses a direct truthy check on the response value (refer to the assertion
that inspects response.json()["is_valid"] in tests/test_blockchain.py).
- Around line 67-95: The test test_blockchain_previous_hash_stored is flaky
because the module-global blockchain_last_hash_cache can retain state across
tests; before creating issues (or in test setup) clear or reset
blockchain_last_hash_cache from the blockchain module so the cache is empty
(e.g., import the symbol blockchain_last_hash_cache and reinitialize/clear it)
to ensure previous_integrity_hash is computed deterministically for the test.
---
Nitpick comments:
In `@backend/models.py`:
- Line 152: The model column previous_integrity_hash in backend/models.py is
declared as an unbounded String which conflicts with the migration that defines
this field as VARCHAR(255); update the Column declaration for
previous_integrity_hash to use a bounded String(255) so the SQLAlchemy model
matches the migration schema and prevents schema drift (locate the
previous_integrity_hash Column in the class definition and change its type
argument accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c0bd8b3-1286-4076-93da-1ee81fd3f351
📒 Files selected for processing (5)
backend/cache.pybackend/init_db.pybackend/models.pybackend/routers/issues.pytests/test_blockchain.py
| prev_hash = blockchain_last_hash_cache.get("last_integrity_hash") | ||
|
|
||
| if prev_hash is None: | ||
| # Cache miss: Fetch only the last hash from DB | ||
| prev_issue = await run_in_threadpool( | ||
| lambda: db.query(Issue.integrity_hash).order_by(Issue.id.desc()).first() | ||
| ) | ||
| prev_hash = prev_issue[0] if prev_issue and prev_issue[0] else "" | ||
| blockchain_last_hash_cache.set(data=prev_hash, key="last_integrity_hash") | ||
|
|
There was a problem hiding this comment.
Chain fork risk under concurrent issue creation (TOCTOU on prev_hash).
Line 176 reads cached parent hash and Line 215 updates cache only after persistence. Concurrent requests can compute different new blocks from the same parent, breaking linear chain guarantees.
🔧 Suggested fix (serialize chain head assignment in app process)
+import asyncio
+
+# Serialize blockchain head updates inside this worker process
+_blockchain_chain_lock = asyncio.Lock()
...
- prev_hash = blockchain_last_hash_cache.get("last_integrity_hash")
-
- if prev_hash is None:
- prev_issue = await run_in_threadpool(
- lambda: db.query(Issue.integrity_hash).order_by(Issue.id.desc()).first()
- )
- prev_hash = prev_issue[0] if prev_issue and prev_issue[0] else ""
- blockchain_last_hash_cache.set(data=prev_hash, key="last_integrity_hash")
-
- hash_content = f"{description}|{category}|{prev_hash}"
- integrity_hash = hashlib.sha256(hash_content.encode()).hexdigest()
+ async with _blockchain_chain_lock:
+ prev_hash = blockchain_last_hash_cache.get("last_integrity_hash")
+ if prev_hash is None:
+ prev_issue = await run_in_threadpool(
+ lambda: db.query(Issue.integrity_hash).order_by(Issue.id.desc()).first()
+ )
+ prev_hash = prev_issue[0] if prev_issue and prev_issue[0] else ""
+ blockchain_last_hash_cache.set(data=prev_hash, key="last_integrity_hash")
+
+ hash_content = f"{description}|{category}|{prev_hash}"
+ integrity_hash = hashlib.sha256(hash_content.encode()).hexdigest()
new_issue = Issue(
...
integrity_hash=integrity_hash,
previous_integrity_hash=prev_hash
)
- await run_in_threadpool(save_issue_db, db, new_issue)
-
- blockchain_last_hash_cache.set(data=integrity_hash, key="last_integrity_hash")
+ await run_in_threadpool(save_issue_db, db, new_issue)
+ blockchain_last_hash_cache.set(data=integrity_hash, key="last_integrity_hash")For multi-worker deployments, use a DB-backed lock/chain-head row so serialization is global, not per process.
Also applies to: 214-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routers/issues.py` around lines 176 - 185, The current code reads
prev_hash from blockchain_last_hash_cache and then computes/creates a new block
without global serialization, causing TOCTOU forks; replace this
local-cache-only pattern by acquiring a DB-backed chain-head lock/row when
generating a new issue: within a DB transaction select the chain-head row (or a
dedicated single-row table) FOR UPDATE to read the authoritative parent hash
(instead of the cache), compute and persist the new Issue (using
Issue.integrity_hash logic), update the chain-head row to point to the new hash,
commit, and only then update blockchain_last_hash_cache with the committed
value; ensure the code paths referencing prev_hash, blockchain_last_hash_cache,
and the run_in_threadpool/Issue.integrity_hash query use this transactional lock
to serialize head assignment across processes.
| def test_blockchain_previous_hash_stored(client, db_session): | ||
| # Create first issue | ||
| hash1 = hashlib.sha256(b"First").hexdigest() | ||
| issue1 = Issue(description="First", category="Road", integrity_hash=hash1) | ||
| db_session.add(issue1) | ||
| db_session.commit() | ||
|
|
||
| # Create second issue via API (to test logic in create_issue) | ||
| # Mocking background tasks to avoid errors | ||
| from unittest.mock import patch | ||
| with patch("backend.routers.issues.process_action_plan_background"), \ | ||
| patch("backend.routers.issues.create_grievance_from_issue_background"): | ||
| response = client.post( | ||
| "/api/issues", | ||
| data={ | ||
| "description": "Second issue description", | ||
| "category": "Garbage", | ||
| "latitude": 10.0, | ||
| "longitude": 20.0 | ||
| } | ||
| ) | ||
|
|
||
| assert response.status_code == 201 | ||
| issue_id = response.json()["id"] | ||
|
|
||
| # Verify issue in DB has previous_integrity_hash | ||
| issue2 = db_session.query(Issue).filter(Issue.id == issue_id).first() | ||
| assert issue2.previous_integrity_hash == hash1 | ||
|
|
There was a problem hiding this comment.
Reset blockchain cache in test setup to avoid order-dependent failures.
This test assumes cache state is empty, but blockchain_last_hash_cache is module-global and can leak state from other tests, making previous_integrity_hash assertions flaky.
✅ Suggested test hardening
from backend.models import Issue
+from backend.cache import blockchain_last_hash_cache
...
def test_blockchain_previous_hash_stored(client, db_session):
+ blockchain_last_hash_cache.invalidate("last_integrity_hash")
+
# Create first issue
hash1 = hashlib.sha256(b"First").hexdigest()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_blockchain.py` around lines 67 - 95, The test
test_blockchain_previous_hash_stored is flaky because the module-global
blockchain_last_hash_cache can retain state across tests; before creating issues
(or in test setup) clear or reset blockchain_last_hash_cache from the blockchain
module so the cache is empty (e.g., import the symbol blockchain_last_hash_cache
and reinitialize/clear it) to ensure previous_integrity_hash is computed
deterministically for the test.
| # (Implicitly tested if it returns is_valid=True and we know we didn't mock the internal DB query in the router) | ||
| response = client.get(f"/api/issues/{issue_id}/blockchain-verify") | ||
| assert response.status_code == 200 | ||
| assert response.json()["is_valid"] == True |
There was a problem hiding this comment.
Use direct truth assertion instead of == True.
Prefer a direct truthy check on Line 100 for clarity and lint compliance.
✏️ Suggested fix
- assert response.json()["is_valid"] == True
+ assert response.json()["is_valid"]📝 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.
| assert response.json()["is_valid"] == True | |
| assert response.json()["is_valid"] |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 100-100: Avoid equality comparisons to True; use response.json()["is_valid"]: for truth checks
Replace with response.json()["is_valid"]
(E712)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_blockchain.py` at line 100, Replace the equality comparison
asserting a boolean on the response with a direct truth assertion: change the
assertion that checks response.json()["is_valid"] == True to assert
response.json()["is_valid"] so the test uses a direct truthy check on the
response value (refer to the assertion that inspects response.json()["is_valid"]
in tests/test_blockchain.py).
There was a problem hiding this comment.
Pull request overview
This PR optimizes the issue “blockchain integrity” chaining by (1) persisting each issue’s parent hash in the issues table and (2) caching the most recent integrity hash in-memory to reduce DB round-trips during issue creation and verification.
Changes:
- Add
previous_integrity_hashto theIssuemodel and DB migration, including an index. - Update issue creation to cache and reuse the last integrity hash, and store
previous_integrity_hashon new issues. - Update blockchain verification to use
previous_integrity_hash(with a legacy fallback), and add a test to assert the parent hash is stored when creating via the API.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
backend/routers/issues.py |
Uses blockchain_last_hash_cache for parent-hash lookup on create; verification reads previous_integrity_hash to avoid an extra query. |
backend/models.py |
Adds previous_integrity_hash column to Issue. |
backend/init_db.py |
Migrates previous_integrity_hash column (VARCHAR(255)) and adds an index. |
backend/cache.py |
Introduces blockchain_last_hash_cache singleton cache. |
tests/test_blockchain.py |
Updates existing blockchain tests and adds a new API-level test for previous_integrity_hash persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # Cache miss: Fetch only the last hash from DB | ||
| prev_issue = await run_in_threadpool( | ||
| lambda: db.query(Issue.integrity_hash).order_by(Issue.id.desc()).first() |
There was a problem hiding this comment.
create_issue now relies on blockchain_last_hash_cache for prev_hash, but the DB fallback query on cache miss selects the most recent row regardless of whether Issue.integrity_hash is NULL. Since there are other code paths that insert issues without integrity_hash (e.g., voice/telegram), prev_hash can become nondeterministic: cache hit continues the chain from the last hashed issue, while a cache miss after such inserts resets the chain to "". Update the cache-miss query to fetch the latest non-null integrity_hash (and consider aligning any other issue-creation paths with this rule) so cache hits and misses compute the same parent hash.
| # Cache miss: Fetch only the last hash from DB | |
| prev_issue = await run_in_threadpool( | |
| lambda: db.query(Issue.integrity_hash).order_by(Issue.id.desc()).first() | |
| # Cache miss: Fetch only the last *non-null* hash from DB | |
| prev_issue = await run_in_threadpool( | |
| lambda: db.query(Issue.integrity_hash) | |
| .filter(Issue.integrity_hash.isnot(None)) | |
| .order_by(Issue.id.desc()) | |
| .first() |
| def test_blockchain_previous_hash_stored(client, db_session): | ||
| # Create first issue | ||
| hash1 = hashlib.sha256(b"First").hexdigest() | ||
| issue1 = Issue(description="First", category="Road", integrity_hash=hash1) | ||
| db_session.add(issue1) | ||
| db_session.commit() | ||
|
|
||
| # Create second issue via API (to test logic in create_issue) | ||
| # Mocking background tasks to avoid errors | ||
| from unittest.mock import patch | ||
| with patch("backend.routers.issues.process_action_plan_background"), \ | ||
| patch("backend.routers.issues.create_grievance_from_issue_background"): | ||
| response = client.post( | ||
| "/api/issues", | ||
| data={ | ||
| "description": "Second issue description", | ||
| "category": "Garbage", | ||
| "latitude": 10.0, | ||
| "longitude": 20.0 | ||
| } | ||
| ) | ||
|
|
||
| assert response.status_code == 201 | ||
| issue_id = response.json()["id"] | ||
|
|
||
| # Verify issue in DB has previous_integrity_hash | ||
| issue2 = db_session.query(Issue).filter(Issue.id == issue_id).first() | ||
| assert issue2.previous_integrity_hash == hash1 | ||
|
|
There was a problem hiding this comment.
This test can become order-dependent because blockchain_last_hash_cache is a module-level singleton and is not cleared between tests. If any other test calls /api/issues before this one, the cache may contain a stale hash and create_issue will skip the DB lookup, causing issue2.previous_integrity_hash to differ from hash1. Clear blockchain_last_hash_cache (e.g., in the db_session/client fixture setup or at the start of this test) to make the assertion deterministic.
This PR optimizes the blockchain integrity feature by caching the last hash and storing the parent hash directly in the issue record. This reduces DB roundtrips and improves overall system performance.
PR created automatically by Jules for task 15979771567181896565 started by @RohanExploit
Summary by cubic
Speeds up blockchain integrity by caching the last hash and storing each issue’s previous hash. This cuts DB reads on create and verify and reduces latency.
New Features
Migration
Written for commit fe7744e. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Performance