fix: prevent race condition in UTXO locking with mutex#128
fix: prevent race condition in UTXO locking with mutex#128GautamKumarOffical wants to merge 4 commits into
Conversation
Move idempotency check and UTXO selection inside the database transaction to prevent concurrent requests from selecting the same UTXOs. SQLite's transaction isolation ensures that once a transaction begins, no other connection can see uncommitted changes or select the same outputs until commit or rollback. Closes tari-project#125 Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request modifies the FundLocker to start a database transaction before checking idempotency and selecting UTXOs, and adds unit tests to verify idempotency behavior. However, the reviewer correctly identifies that SQLite's default transaction behavior is DEFERRED, which only acquires a SHARED lock on read operations and does not prevent concurrent transactions from selecting the same UTXOs. To fix this race condition, the reviewer suggests initiating an IMMEDIATE transaction instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Start the transaction FIRST to prevent race conditions. | ||
| // SQLite's transaction isolation ensures that once we begin, | ||
| // no other connection can see our uncommitted changes or select | ||
| // the same UTXOs until we commit or rollback. | ||
| let transaction = conn.transaction()?; |
There was a problem hiding this comment.
SQLite's default transaction behavior is DEFERRED. A deferred transaction does not acquire any database locks until the first write operation. Since the idempotency check and UTXO selection are read operations, they only acquire a SHARED lock, allowing concurrent connections to read the same UTXOs.
When both connections subsequently attempt to write, one will succeed and the other will block/retry. However, because lock_output does not verify that rows were actually updated (it executes UPDATE ... WHERE status = 'Unspent'), the second transaction will silently succeed even though it updated 0 rows (since the first transaction already locked them). This results in both transactions believing they successfully locked the same UTXOs.
To prevent this race condition, start an IMMEDIATE transaction to acquire a RESERVED lock immediately, blocking concurrent write-intent transactions from starting until this one commits or rolls back.
| // Start the transaction FIRST to prevent race conditions. | |
| // SQLite's transaction isolation ensures that once we begin, | |
| // no other connection can see our uncommitted changes or select | |
| // the same UTXOs until we commit or rollback. | |
| let transaction = conn.transaction()?; | |
| // Start an IMMEDIATE transaction to prevent race conditions. | |
| // This acquires a RESERVED lock immediately, ensuring no other concurrent | |
| // transaction can select or lock the same UTXOs. | |
| let transaction = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; |
SQLite's default DEFERRED transaction only acquires a SHARED lock on reads, which doesn't prevent concurrent transactions from selecting the same UTXOs. Use IMMEDIATE transaction behavior to acquire a RESERVE lock upfront, preventing the race condition. Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
Good catch. Updated to use |
SWvheerden
left a comment
There was a problem hiding this comment.
wont work, you need a mutex
SQLite IMMEDIATE transactions only prevent concurrent access at the connection level. When using a connection pool, two concurrent requests could get different connections and both start IMMEDIATE transactions, allowing them to select the same UTXOs before either commits. Adding an in-process mutex ensures only one thread can execute the UTXO selection and locking logic at a time, preventing double-selection even with connection pooling. Closes tari-project#125 Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
Good catch. Added a Mutex<()> to serialize concurrent UTXO selection attempts. The IMMEDIATE transaction alone isn't enough when using a connection pool — two concurrent requests could get different connections and both read the same unspent outputs. The mutex ensures only one thread can execute the critical section at a time, preventing double-selection even with pooling. Also added a test to verify the mutex behavior. |
| // Acquire mutex to serialize concurrent UTXO selection attempts. | ||
| // Without this, two concurrent requests could both read the same | ||
| // unspent outputs before either commits, leading to double-selection. | ||
| let _guard = self.lock.lock().map_err(|e| anyhow::anyhow!("Lock poisoned: {}", e))?; |
There was a problem hiding this comment.
you can move the mutex below the idempotency, allow this check to be done safely and quickly.
then after check you need the mutex and you need to check the idempotency again.
There was a problem hiding this comment.
Done. Moved the idempotency check before the mutex as a fast path — it now reads from the DB without blocking other operations. If a match is found, we return immediately. Otherwise, we acquire the mutex and re-check idempotency inside the IMMEDIATE transaction to handle the race between the fast check and acquiring the lock.
Re-check idempotency inside the mutex+transaction to handle the race between the fast path check and acquiring the mutex. Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
Hey @SWvheerden, I've addressed the mutex feedback. The race condition should be fixed now. Could you take another look? Thanks! |
Problem
SQLite's IMMEDIATE transactions only prevent concurrent access at the connection level. When using a connection pool, two concurrent requests could get different connections and both start IMMEDIATE transactions, allowing them to select the same UTXOs before either commits.
Solution
Added an in-process
Mutex<()>toFundLockerthat serializes the entire UTXO selection and locking logic. This ensures only one thread can execute the critical section at a time, preventing double-selection even with connection pooling.Changes
std::sync::Mutex<()>toFundLockerstructlock()methodtest_fund_locker_mutex_prevents_concurrent_selectionto verify mutex behaviorTesting
Closes #125
Signed-off-by: Gautam Kumar gautamkumarofficial@users.noreply.github.com