Backend blockchain#17
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements two-phase on-chain anchoring (submit + confirm) with scheduler jobs, renames client/server E2E crypto fields, validates and persists message content_hashes, adds logout/flush anchoring triggers, and records design decisions in the transcript. ChangesBlockchain Message Anchoring with Crypto Field Rename
Sequence DiagramsequenceDiagram
participant Client as Client
participant ClientAPI as client-web/api.js
participant Server as FlaskApp
participant DB as MySQL
participant Web3 as Blockchain
Client->>ClientAPI: POST /messages
ClientAPI->>Server: /messages handler (validated JSON)
Server->>DB: INSERT message row
Client->>ClientAPI: logout() -> POST /auth/logout
ClientAPI->>Server: POST /auth/logout (Bearer)
Server->>Server: spawn anchor_pending thread
Server->>DB: SELECT unanchored messages
Server->>DB: INSERT blockchain_records
Server->>DB: UPDATE messages.blockchain_record_id
Server->>Web3: sendTransaction storeData(merkle_root)
alt tx success
Web3->>Server: tx_hash
Server->>DB: UPDATE blockchain_records.tx_hash
else tx pending or failure
Server->>DB: rollback reservation or increment revert_count
end
Server->>Web3: confirm_pending fetch receipts
Web3->>Server: receipt
Server->>DB: UPDATE blockchain_records block_number/timestamp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app/__init__.py`:
- Around line 71-89: The code starts BackgroundScheduler inside create_app()
causing one scheduler per worker (functions
_scheduled_anchor/_scheduled_confirm, app.scheduler) — replace this by ensuring
a single-owner process: either move scheduler startup out of web workers into a
dedicated service/process that imports anchor_pending/confirm_pending, or
implement a production-only single-owner gate (e.g., an explicit env flag like
SCHEDULER_OWNER set on one instance, or a distributed leader-lock using Redis/DB
before starting the scheduler) so only the owner calls scheduler.start(); remove
reliance on WERKZEUG_RUN_MAIN for production.
In `@server/app/auth/routes.py`:
- Around line 162-178: The logout flow has two problems: the background thread
raised NameError because anchor_pending is imported only inside logout() but
used by _anchor_in_context(), and the web client never calls POST /auth/logout
so anchoring never triggers; fix by moving the import of anchor_pending to
module scope (import anchor_pending at top of server/app/auth/routes.py) so
_anchor_in_context and the thread can access it, and update the client API
logout to call POST /auth/logout before clearing sessionStorage (make
api.logout() async and await a POST request to '/auth/logout' then clearToken in
finally), and update any callers of api.logout() to handle the returned promise
to avoid unhandled rejections.
In `@server/app/messages/anchor.py`:
- Around line 66-156: The _run worker currently reads messages with
blockchain_record_id IS NULL then sends the on-chain tx and only afterwards
writes blockchain_records and updates messages, which can cause duplicate
anchors or orphaned tx; modify _run to atomically claim messages before
submitting the external transaction by starting a DB transaction and either (a)
selecting message rows with SELECT ... FOR UPDATE SKIP LOCKED (using the same
cursor and transaction) and setting a temporary claim flag/anchoring state on
messages, or (b) INSERT a blockchain_records row with id/status='pending' and
update messages.blockchain_record_id to that id and COMMIT before calling
contract.functions.storeData, then proceed with the on-chain send and finally
update the blockchain_records row with tx_hash/block_number/block_timestamp and
clear/advance the anchoring state; ensure you use the same unique symbols
(messages table, blockchain_records table, blockchain_record_id column, the _run
function and storeData contract call) and handle DB rollbacks and nonce/gas
error paths so claims are durable and retries are idempotent.
- Around line 196-225: The loop currently treats any fetched transaction receipt
as confirmed; modify the logic after fetching receipt in
server/app/messages/anchor.py to check receipt['status'] (from
w3.eth.get_transaction_receipt) and only proceed to fetch block, update
blockchain_records (the cursor.execute call that sets block_number and
block_timestamp) and call logger.info('Confirmed tx %s at block %s', ...) when
receipt['status'] == 1; if status == 0 (reverted) skip the DB update and instead
log (e.g., logger.warning or logger.info) that the tx reverted and continue to
next row so reverted transactions are not recorded as confirmed. Ensure you
reference the same variables (receipt, block, block_number, row['id'],
row['tx_hash']) and keep existing exception handling around
get_transaction_receipt and get_block.
In `@server/app/messages/routes.py`:
- Around line 80-89: In flush(), before spawning the background Thread, check
the application config flag that controls anchoring (e.g.
app.config.get('ANCHORING_ENABLED') or the Web3 config presence) using the app
object obtained via current_app._get_current_object(); if anchoring is disabled,
return an HTTP error (e.g. 403) and do not start the thread or call
_anchor_in_context, otherwise proceed to spawn the daemon thread as before.
🪄 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: CHILL
Plan: Pro Plus
Run ID: fbab3b60-0b49-4d71-bb10-385f80b381d0
📒 Files selected for processing (7)
client-web/js/api.jsdocs/transcript_sarah.mdrequirements.txtserver/app/__init__.pyserver/app/auth/routes.pyserver/app/messages/anchor.pyserver/app/messages/routes.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/app/messages/anchor.py (1)
116-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed anchoring run nonce from the pending pool
w3.eth.get_transaction_count(account.address)defaults to block_identifier=latest(viaw3.eth.default_block), which excludes pending transactions; this can cause nonce reuse/replacement while prior anchor txs are still pending. Use'pending'for nonce management.Suggested fix
- nonce = w3.eth.get_transaction_count(account.address) + nonce = w3.eth.get_transaction_count(account.address, 'pending')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/messages/anchor.py` around lines 116 - 156, The nonce seeding currently uses w3.eth.get_transaction_count(account.address) which defaults to 'latest' and ignores pending txs, risking nonce reuse; change the initial nonce fetch to w3.eth.get_transaction_count(account.address, 'pending') (where nonce is first assigned) so subsequent contract.functions.storeData(...)/send_raw_transaction calls use a nonce that accounts for pending transactions; keep the existing nonce increment (nonce += 1) behavior unchanged.
♻️ Duplicate comments (1)
server/app/messages/anchor.py (1)
122-168:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCommit the message claim before calling Web3.
This reservation is still not durable enough.
messages.blockchain_record_idis updated but left uncommitted until aftersend_raw_transaction, and theUPDATE ... WHERE id IN (...)can overwrite rows another worker already claimed. If two workers read the same conversation before either commits, both can send a tx and one on-chain spend becomes orphaned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/messages/anchor.py` around lines 122 - 168, The reservation of messages (INSERT into blockchain_records and the UPDATE messages SET blockchain_record_id = %s ... using the cursor) must be made durable before calling Web3; move a db.commit() immediately after the reservation block (after the INSERT and the UPDATE messages) and before building/signing/sending the transaction so the claim is committed and other workers won't double-claim. Keep the existing db.rollback()/logger.exception paths on exceptions and ensure the subsequent UPDATE of blockchain_records with tx_hash (UPDATE blockchain_records SET tx_hash = %s WHERE id = %s) still happens after the chain tx and commits as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app/messages/anchor.py`:
- Around line 223-225: When receipt['status'] != 1, do not just log and
continue; mark the related blockchain_records row as failed and detach or clear
the blockchain_record_id on linked messages so they can be retried or cleaned
up. Specifically, in the block handling the reverted transaction around the
receipt['status'] check (using row['id'] / row['tx_hash'] and the
blockchain_records table), update that blockchain_records row to a terminal
failed state (e.g., status='failed' or failed_at timestamp) and either set
messages.blockchain_record_id = NULL for messages referencing that
blockchain_record_id or call an existing helper like
mark_reservation_failed(row['id']) / detach_messages_from_record(row['id']) to
rollback the reservation.
---
Outside diff comments:
In `@server/app/messages/anchor.py`:
- Around line 116-156: The nonce seeding currently uses
w3.eth.get_transaction_count(account.address) which defaults to 'latest' and
ignores pending txs, risking nonce reuse; change the initial nonce fetch to
w3.eth.get_transaction_count(account.address, 'pending') (where nonce is first
assigned) so subsequent contract.functions.storeData(...)/send_raw_transaction
calls use a nonce that accounts for pending transactions; keep the existing
nonce increment (nonce += 1) behavior unchanged.
---
Duplicate comments:
In `@server/app/messages/anchor.py`:
- Around line 122-168: The reservation of messages (INSERT into
blockchain_records and the UPDATE messages SET blockchain_record_id = %s ...
using the cursor) must be made durable before calling Web3; move a db.commit()
immediately after the reservation block (after the INSERT and the UPDATE
messages) and before building/signing/sending the transaction so the claim is
committed and other workers won't double-claim. Keep the existing
db.rollback()/logger.exception paths on exceptions and ensure the subsequent
UPDATE of blockchain_records with tx_hash (UPDATE blockchain_records SET tx_hash
= %s WHERE id = %s) still happens after the chain tx and commits as currently
implemented.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4108c5a3-ab15-4e7a-9a34-d8c047628d82
📒 Files selected for processing (4)
client-web/js/api.jsserver/app/auth/routes.pyserver/app/messages/anchor.pyserver/app/messages/routes.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app/messages/anchor.py`:
- Around line 223-240: The current revert handler in anchor.py immediately nulls
messages.blockchain_record_id and deletes the blockchain_records row which
requeues the same batch; instead update the existing blockchain_records row to
record the failure (e.g., set status='failed' or increment retry_count and set
next_retry_at/backoff) and leave messages.blockchain_record_id pointing to that
record so anchor_pending() ignores it until retry conditions are met. Modify the
exception branch in the receipt status != 1 block (the code that currently runs
cursor.execute('UPDATE messages SET blockchain_record_id = NULL ...') and DELETE
FROM blockchain_records) to perform an UPDATE on blockchain_records (status,
retry_count, next_retry_at) and avoid nulling messages.blockchain_record_id, and
also ensure anchor_pending() logic is changed to filter out failed records or
defer until next_retry_at. Ensure DB commit/rollback and cursor.close() remain
intact.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 68a0fe6f-26b5-44df-9915-f50226253740
📒 Files selected for processing (1)
server/app/messages/anchor.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app/messages/anchor.py`:
- Line 224: Handle potential NULL revert_count by treating None as 0 before
incrementing: replace the direct addition new_count = row['revert_count'] + 1
with code that safely coalesces row['revert_count'] (e.g., current =
row.get('revert_count') or 0 or current = int(row['revert_count'] or 0)) and
then set new_count = current + 1 so that new_count calculation in anchor.py
never raises TypeError when row['revert_count'] is NULL.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b79ab263-42f3-40e9-8da9-c9470e3cff4b
📒 Files selected for processing (1)
server/app/messages/anchor.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/app/messages/anchor.py (1)
33-42: 💤 Low valueGuard against empty input to avoid
IndexError.If
hex_hashesis empty,layer[0]on line 42 raisesIndexError. While current callers ensure non-empty input, a defensive check would prevent future misuse.🛡️ Proposed fix
def _merkle_root(hex_hashes): + if not hex_hashes: + raise ValueError("Cannot compute Merkle root of empty list") layer = [bytes.fromhex(h.removeprefix('0x')) for h in hex_hashes]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/messages/anchor.py` around lines 33 - 42, The _merkle_root function can IndexError when hex_hashes is empty; add a defensive check at the start of _merkle_root to validate hex_hashes is non-empty and raise a clear ValueError (or another appropriate exception) with a descriptive message (e.g., "hex_hashes must be a non-empty iterable") before using layer and accessing layer[0]; this keeps the existing logic (removeprefix, bytes.fromhex, Web3.keccak) intact but prevents silent IndexError for future callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/app/messages/anchor.py`:
- Around line 33-42: The _merkle_root function can IndexError when hex_hashes is
empty; add a defensive check at the start of _merkle_root to validate hex_hashes
is non-empty and raise a clear ValueError (or another appropriate exception)
with a descriptive message (e.g., "hex_hashes must be a non-empty iterable")
before using layer and accessing layer[0]; this keeps the existing logic
(removeprefix, bytes.fromhex, Web3.keccak) intact but prevents silent IndexError
for future callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0273e7be-91c4-4000-abaa-79323cddbfc5
📒 Files selected for processing (1)
server/app/messages/anchor.py
Aoibheannmangan
approved these changes
May 24, 2026
This was referenced May 24, 2026
Merged
Merged
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.
this nearly killed me
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Maintenance