Skip to content

fix(utxo): wrap mempool_clear_expired in BEGIN IMMEDIATE - #8181

Open
rebel117 wants to merge 1 commit into
Scottcjn:mainfrom
rebel117:fix-8176-mempool-clear-expired-begin-immediate
Open

fix(utxo): wrap mempool_clear_expired in BEGIN IMMEDIATE#8181
rebel117 wants to merge 1 commit into
Scottcjn:mainfrom
rebel117:fix-8176-mempool-clear-expired-begin-immediate

Conversation

@rebel117

@rebel117 rebel117 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

mempool_clear_expired() performed a SELECT to find expired transactions, then looped through doing two DELETEs per row without holding a write lock. A concurrent mempool_add() or apply_transaction() could interleave between the SELECT and the DELETEs, producing orphan utxo_mempool_inputs rows or inconsistent mempool state.

This is the same bug class that was already fixed in mempool_remove() (BUG-1), but mempool_clear_expired() was left out.

Fix

Wrap the SELECT + DELETEs in BEGIN IMMEDIATE, matching the pattern already established in mempool_add() (L1043) and mempool_remove() (L1232):

  • Acquire the write lock before the SELECT so the expiry scan and the deletes are atomic
  • conn.commit() after all deletes complete
  • ROLLBACK in the except block for error paths
  • The no such table early-return now does ROLLBACK before returning 0

Testing

Updated test_utxo_mempool_concurrent_stress_poc.py (the existing B2 PoC) so its assertions verify the fix rather than the bug, and added two regression tests:

  • test_clear_expired_removes_inputs_and_tx_atomically — verifies both the tx row and its input rows are gone after expiry clear (no orphans)
  • test_clear_expired_handles_missing_table — verifies graceful 0-return on a fresh DB without tables
test_utxo_mempool_concurrent_stress_poc.py ....                            [100%]
test_utxo_db.py ............................................................. [100%]
test_mempool_rollback.py .                                                  [100%]
100 passed

Fixes #8176

The clear_expired SELECT and subsequent DELETEs ran without a write
lock, so a concurrent mempool_add could interleave between them and
leave orphan utxo_mempool_inputs rows (persistent UTXO locks / DoS).

Now matches the pattern already used in mempool_add() and
mempool_remove() — acquire BEGIN IMMEDIATE before the SELECT, commit
after the DELETEs, rollback on error.

Also updated the B2 PoC test to verify the fix instead of the bug,
and added a regression test for atomic input+tx removal.

Fixes Scottcjn#8176
@rebel117

rebel117 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

PR #8181 opened. mempool_clear_expired() now acquires BEGIN IMMEDIATE before the expiry scan + deletes, matching mempool_add/mempool_remove. 100 tests pass.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines labels Aug 3, 2026

@FlintLeng FlintLeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: UTXO Mempool Race Condition — BEGIN IMMEDIATE

Reviewed on: 2026-08-04

Summary

Fixes a SQLite concurrency race in mempool_clear_expired(). The original code SELECTed expired tx IDs without a write lock, then looped through DELETEs — a concurrent mempool_add() or apply_transaction() could interleave between the SELECT and DELETEs, leaving orphan utxo_mempool_inputs rows that permanently hold UTXO locks.

Root Cause Analysis ✅

The comment correctly identifies the pattern: BEGIN IMMEDIATE is already used by mempool_remove() and mempool_add() in this codebase. The absence of it in mempool_clear_expired() was an oversight, not a deliberate design choice. The diff shows the fix is consistent with the established pattern across the three mempool mutation functions.

Code Quality ✅

Structure is clean:

conn.execute("BEGIN IMMEDIATE")
try:
    expired = conn.execute(SELECT ...)
    for row in expired:
        conn.execute(DELETE inputs)
        conn.execute(DELETE main)
    conn.commit()
except:
    conn.execute("ROLLBACK")
    raise
  • Rollback in except block prevents half-committed state on any failure — correct.
  • The "no such table" path (fresh DB) returns 0 after rollback — graceful degradation.
  • The original else: count = 0 ... return count was structurally confusing (return inside a loop with a deferred return after it). Flattening it into sequential code is clearer.

The nested else on try/except/else was particularly confusing in the original. The else clause only ran if no exception occurred, making the flow: try (SELECT) → except (no table) → else (loop+commit). The new code removes this ambiguity entirely.

Test Coverage ✅

  • Regression test TestClearExpiredAtomicity directly verifies both utxo_mempool and utxo_mempool_inputs rows are deleted atomically — tests the actual failure mode (orphan inputs), not just the happy path.
  • test_clear_expired_handles_missing_table tests the graceful degradation path.
  • The PoC tests (B1/B2) flipping from assertFalseassertTrue is an honest way to document the fix — acceptable in a regression test suite.

Minor Notes

  1. import time as _time inside the test method shadows the module-level time — minor style nits, harmless in a test.

  2. conn.execute("ROLLBACK") in the except block can itself raise (e.g., if the transaction already committed or if the connection is broken). Wrapped in another nested try/except — correct.

Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e

✅ LGTM — clean, correct race condition fix with solid regression coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] mempool_clear_expired() missing BEGIN IMMEDIATE -- concurrent mempool corruption (B2)

2 participants