⚡ Bolt: Blockchain Chaining and ORM Query Optimization#652
Conversation
💡 What: 1. Implemented HMAC-SHA256 blockchain chaining for community closure confirmations. 2. Optimized GrievanceService creation path to use O(1) cache-first hash lookups. 3. Switched from 'joinedload' to 'selectinload' for audit logs in grievance endpoints. 🎯 Why: 1. Provides tamper-proof integrity for community consensus on issue resolution. 2. Eliminates a redundant database query on every grievance creation. 3. Improves list view performance by avoiding the Cartesian product problem during one-to-many relationship fetching. 📊 Impact: - Grievance creation: Measurable reduction in latency due to O(1) hash resolution. - Grievance list/detail: Faster fetching of audit logs, especially for grievances with many escalation records. - Integrity: Added O(1) cryptographic verification for community closure confirmations. 🔬 Measurement: - Verified via custom test script for O(1) chaining logic. - Benchmarked grievance creation path: reduction in DB roundtrips. - Ran existing blockchain integrity test suite to ensure zero regressions.
|
👋 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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded HMAC-SHA256 integrity chaining for ClosureConfirmations with previous-hash linking and a thread-safe cache; grievance chaining now relies on cached previous hash. Added model columns, migration steps, a verification endpoint, and cache instance for closure confirmations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClosureService as "Closure Service"
participant Cache as "ThreadSafeCache (closure_last_hash_cache)"
participant DB as "Database"
participant HMAC as "HMAC-SHA256"
Client->>ClosureService: Submit closure confirmation
ClosureService->>Cache: Read last_hash
alt cache hit
Cache-->>ClosureService: prev_hash
else cache miss
ClosureService->>DB: Query latest ClosureConfirmation.integrity_hash
DB-->>ClosureService: prev_hash
end
ClosureService->>HMAC: Compute HMAC(secret_key, "grievance_id|user_email|type|prev_hash")
HMAC-->>ClosureService: computed_hash
ClosureService->>DB: Insert ClosureConfirmation(integrity_hash, previous_integrity_hash)
DB-->>ClosureService: commit success
ClosureService->>Cache: Update last_hash = computed_hash
Cache-->>ClosureService: ack
ClosureService-->>Client: Return confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds blockchain-style chaining/verification support for ClosureConfirmation and applies ORM/query-path optimizations to reduce DB load during grievance creation and grievance retrieval.
Changes:
- Switched grievance audit log loading from
joinedloadtoselectinloadin grievance list/detail endpoints. - Added
integrity_hashandprevious_integrity_hashfields (plus migration/index) forClosureConfirmation, and added a new blockchain verification endpoint. - Optimized grievance and closure-confirmation creation chaining by using thread-safe caches to avoid repeated “last hash” DB queries.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/routers/grievances.py | Uses selectinload for audit logs and adds closure-confirmation blockchain verification endpoint. |
| backend/models.py | Adds integrity chaining columns to ClosureConfirmation model. |
| backend/init_db.py | Migrates existing DBs to add new closure-confirmation columns and index. |
| backend/grievance_service.py | Optimizes grievance creation by caching last integrity hash. |
| backend/closure_service.py | Adds closure-confirmation integrity chaining (HMAC) and cache usage. |
| backend/cache.py | Introduces closure_last_hash_cache instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/routers/grievances.py (1)
594-603: Extract the closure-confirmation hash builder into a shared helper.This endpoint has to stay byte-for-byte aligned with
backend/closure_service.pywhen computing the HMAC. Keeping the payload construction duplicated in both places makes future changes easy to miss on one side and will silently break verification. A single helper for “build closure confirmation hash input + compute digest” would remove that drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routers/grievances.py` around lines 594 - 603, The HMAC input/construction logic in grievances.py is duplicated with backend/closure_service.py and must be extracted into a single shared helper (e.g., compute_closure_confirmation_hash or build_closure_confirmation_hmac) so both consumers use identical byte-for-byte behavior; implement the helper in a common utility module, have it take the fields (grievance_id, user_email, confirmation_type, prev_hash) and internally fetch get_auth_config().secret_key, perform the same f"{grievance_id}|{user_email}|{confirmation_type}|{prev_hash}" concatenation and hashlib.sha256 HMAC, then replace the inline logic in both grievances.py and closure_service.py to call this helper.
🤖 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/closure_service.py`:
- Around line 93-100: The current prev_hash read from closure_last_hash_cache
(closure_last_hash_cache.get("last_hash")) is not serialized with hash
generation and db.commit, allowing forks; instead, perform chain-head selection
and update inside a DB-backed lock or cross-process serialization primitive so
only one worker can read the last hash, compute the new integrity hash, insert
the ClosureConfirmation row and commit as an atomic sequence. Concretely:
replace the separate cache read + DB insert flow around ClosureConfirmation with
a DB-level lock (e.g., SELECT ... FOR UPDATE on a chain-head row or a database
advisory lock) so you read the authoritative last hash, compute the new hash,
insert/commit the ClosureConfirmation within that locked transaction, and only
after successful commit update closure_last_hash_cache.set(...) (and avoid
relying on the process-local cache for serialization).
- Around line 102-118: The integrity HMAC currently built from hash_content
(grievance_id|user_email|confirmation_type|prev_hash) and
get_auth_config().secret_key omits stored fields like reason and ties
verification to the auth secret; update the signing in the function that creates
ClosureConfirmation so the canonical payload includes every persisted,
integrity-relevant field (e.g. grievance_id, user_email, confirmation_type,
reason, previous_integrity_hash, timestamp/created_at, and any other stored
metadata) serialized in a stable order (use an explicit delimiter or canonical
JSON) and compute integrity_hash over that payload; replace use of
get_auth_config().secret_key with a dedicated, versioned integrity key (e.g.
integrity_keys.current or a get_integrity_key(version)) and store the integrity
key version alongside ClosureConfirmation so old records can be verified after
rotation.
In `@backend/grievance_service.py`:
- Around line 90-96: The current cache-first head lookup using
grievance_last_hash_cache can produce forked chains under concurrency; modify
the create/insert path that uses grievance_last_hash_cache/get() and
db.query(Grievance.integrity_hash) so the head selection and insert are
serialized at the DB layer: wrap the head-read+insert in a single transaction
and either lock a dedicated chain-head row using SELECT ... FOR UPDATE (or lock
the Grievance head row) before reading integrity_hash, or add a uniqueness
constraint on Grievance.previous_integrity_hash and perform the insert with
retry on unique-constraint violation; ensure grievance_last_hash_cache.set() is
only updated after a successful committed insert and retries refresh the cache
from the DB.
---
Nitpick comments:
In `@backend/routers/grievances.py`:
- Around line 594-603: The HMAC input/construction logic in grievances.py is
duplicated with backend/closure_service.py and must be extracted into a single
shared helper (e.g., compute_closure_confirmation_hash or
build_closure_confirmation_hmac) so both consumers use identical byte-for-byte
behavior; implement the helper in a common utility module, have it take the
fields (grievance_id, user_email, confirmation_type, prev_hash) and internally
fetch get_auth_config().secret_key, perform the same
f"{grievance_id}|{user_email}|{confirmation_type}|{prev_hash}" concatenation and
hashlib.sha256 HMAC, then replace the inline logic in both grievances.py and
closure_service.py to call this helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55114d20-c1f3-42a7-a662-c5fd46e672fc
📒 Files selected for processing (6)
backend/cache.pybackend/closure_service.pybackend/grievance_service.pybackend/init_db.pybackend/models.pybackend/routers/grievances.py
There was a problem hiding this comment.
3 issues found across 6 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/grievances.py">
<violation number="1" location="backend/routers/grievances.py:609">
P2: Use constant-time comparison for HMAC verification instead of `==` to avoid timing side-channel leakage.</violation>
</file>
<file name="backend/grievance_service.py">
<violation number="1" location="backend/grievance_service.py:91">
P1: Removing the DB-validation step from the blockchain cache creates a silent chain-corruption risk. The old code always compared the cached `last_hash`/`last_id` against the actual DB state to detect stale entries. The new code trusts the in-memory cache for up to 1 hour (the TTL in `cache.py`). If any concurrent request, worker, or external process inserts a grievance during that window, subsequent grievances will chain to a stale `prev_hash`, permanently forking the integrity chain.
Consider restoring the DB cross-check, or at minimum verifying `last_id` hasn't changed before trusting the cached hash.</violation>
</file>
<file name="backend/closure_service.py">
<violation number="1" location="backend/closure_service.py:95">
P1: Same chain-forking issue as in `grievance_service.py`: `prev_hash` is sourced from a process-local cache without any DB-level serialization. Concurrent `submit_confirmation` calls can both read the same cached `prev_hash` and produce different rows with identical `previous_integrity_hash`, forking the integrity chain. In multi-worker deployments each process maintains its own cache, making chains globally inconsistent. Use DB-level locking (e.g., `SELECT ... FOR UPDATE` on a chain-head row) within the same transaction to ensure linear chaining.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ): | ||
| prev_hash = db_last_hash or "" | ||
| # Performance Boost: Use thread-safe cache to eliminate DB query for last hash | ||
| prev_hash = grievance_last_hash_cache.get("last_hash") |
There was a problem hiding this comment.
P1: Removing the DB-validation step from the blockchain cache creates a silent chain-corruption risk. The old code always compared the cached last_hash/last_id against the actual DB state to detect stale entries. The new code trusts the in-memory cache for up to 1 hour (the TTL in cache.py). If any concurrent request, worker, or external process inserts a grievance during that window, subsequent grievances will chain to a stale prev_hash, permanently forking the integrity chain.
Consider restoring the DB cross-check, or at minimum verifying last_id hasn't changed before trusting the cached hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/grievance_service.py, line 91:
<comment>Removing the DB-validation step from the blockchain cache creates a silent chain-corruption risk. The old code always compared the cached `last_hash`/`last_id` against the actual DB state to detect stale entries. The new code trusts the in-memory cache for up to 1 hour (the TTL in `cache.py`). If any concurrent request, worker, or external process inserts a grievance during that window, subsequent grievances will chain to a stale `prev_hash`, permanently forking the integrity chain.
Consider restoring the DB cross-check, or at minimum verifying `last_id` hasn't changed before trusting the cached hash.</comment>
<file context>
@@ -87,29 +87,13 @@ def create_grievance(self, grievance_data: Dict[str, Any], db: Session = None) -
- ):
- prev_hash = db_last_hash or ""
+ # Performance Boost: Use thread-safe cache to eliminate DB query for last hash
+ prev_hash = grievance_last_hash_cache.get("last_hash")
+ if prev_hash is None:
+ # Cache miss: Fetch only the last hash from DB
</file context>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Implemented blockchain chaining for ClosureConfirmation and optimized the GrievanceService creation path. Also applied a performance optimization to grievance retrieval by switching to 'selectinload' for audit logs.
PR created automatically by Jules for task 9240946891921980833 started by @RohanExploit
Summary by cubic
Adds HMAC‑SHA256 blockchain chaining for closure confirmations and speeds up grievance reads/writes via cache‑first hash lookups and
selectinloadfor audit logs. Also adds an O(1) verification endpoint and auto‑migration for new hash fields.New Features
ClosureConfirmationwithintegrity_hashand indexedprevious_integrity_hash.GET /closure-confirmation/{confirmation_id}/blockchain-verifyfor O(1) integrity checks.closure_last_hash_cacheto avoid DB reads for the previous hash.Performance
grievance_last_hash_cache) to remove an extra query.audit_logsfromjoinedloadtoselectinloadto avoid Cartesian products and speed up fetches.Written for commit 221ef85. Summary will update on new commits.
Summary by CodeRabbit
New Features
Performance
Chores