fix: make faucet drip rate limit atomic

[saim256]

Summary

• Fixes the live root faucet.py /faucet/drip TOCTOU rate-limit race from Bug: Faucet rate limiting TOCTOU race condition enables draining #4887.
• Moves the IP check, wallet check, and drip insert into one SQLite BEGIN IMMEDIATE write transaction.
• Adds a concurrent endpoint regression that sends 8 simultaneous drip requests for the same wallet and verifies only one succeeds and only one row is recorded.

Validation

• python -m pytest tests\test_legacy_faucet_json_validation.py -q -> 4 passed
• python -m pytest tests\test_legacy_faucet_json_validation.py tests\test_faucet.py -q -> 8 passed
• python -m py_compile faucet.py tests\test_legacy_faucet_json_validation.py -> passed
• git diff --check -> passed
• python tools\bcos_spdx_check.py --base-ref origin/main -> OK

No live faucet, production wallet, or destructive testing was performed.

Wallet: RTC253255d034065a839cd421811ec589ae5b694ffc

Fixes #4887

[ethever]

Reviewed the live root faucet.py fix for the TOCTOU rate-limit race. The important property is present: create_drip_atomically() starts a SQLite BEGIN IMMEDIATE transaction before reading both the IP and wallet last-drip rows, and it inserts the new drip before committing. That serializes competing /faucet/drip requests through one write transaction, so a second request observes the first committed drip before it can pass the rate-limit checks.

Validation on PR head 15c8216:

• git diff --check origin/main...HEAD -> passed
• python3 -m py_compile faucet.py tests/test_legacy_faucet_json_validation.py -> passed
• python3 tools/bcos_spdx_check.py --base-ref origin/main -> OK
• uv run --no-project --with pytest --with flask --with flask-cors --with requests python -m pytest tests/test_legacy_faucet_json_validation.py tests/test_faucet.py -q -> 8 passed

Small environment note: the combined pytest command needs requests available because tests/test_faucet.py imports tools/testnet_faucet.py. With requests included, the listed tests pass cleanly.

Non-blocking hardening suggestion: the new concurrency regression uses the same wallet and the default test-client IP for all 8 requests, so either the IP check alone or the wallet check alone would make that specific test pass. I would add two narrower regressions as follow-up coverage: same IP with different wallets should allow only one success, and same wallet with different X-Forwarded-For/REMOTE_ADDR values should also allow only one success. That would lock both promised dimensions independently.

No blocking issues found; approving.

[508704820]

Code Review: Make faucet drip rate limit atomic

Summary

Same vulnerability as #4887 (TOCTOU race in faucet rate limiting). Different approach from #4889.

Comparison with #4889

Aspect	#4889 (Xeophon)	#4891 (saim256)
Transaction type	BEGIN IMMEDIATE	BEGIN EXCLUSIVE
Refactoring	Rewrote drip() inline	Extracted reusable helper functions
Code structure	Single large function	Cleaner: can_drip_from_cursor(), _drip_atomic()
Wallet validation	Added RTC prefix	Separate PR (#4892)
Backward compat	Kept old functions	Kept old functions as wrappers

Positive ✅

1. BEGIN EXCLUSIVE is even stronger than BEGIN IMMEDIATE — no other connection can read or write
2. Better code organization — extracted helpers are reusable
3. Backward-compatible — old can_drip() still works for non-atomic use cases
4. Added RTC wallet validation (fix: accept native RTC faucet wallets #4892)

Minor note 🔍

BEGIN EXCLUSIVE is stricter than necessary — BEGIN IMMEDIATE (write lock only) is sufficient since we only need to prevent concurrent writes, not reads. But EXCLUSIVE is also fine for this use case.

Both PRs fix the same bug correctly. #4891 has cleaner code organization.

Review quality: Comparative review

[shuibui]

Code Review: Security Fix

Good security fix. Atomic rate limiting / fail-closed patterns are correct.

Verdict: Approve.

[loganoe]

Reviewed the faucet TOCTOU fix. The rate-limit checks and drip insert now run under one SQLite BEGIN IMMEDIATE transaction on the same connection, which closes the gap between can_drip() and record_drip(). The concurrent regression verifies one success and seven rate-limit responses for eight simultaneous requests.

Validation run locally:

• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. /tmp/rustchain-flask-venv/bin/python -m pytest -q tests/test_legacy_faucet_json_validation.py tests/test_faucet.py
• /tmp/rustchain-flask-venv/bin/python -m py_compile faucet.py tests/test_legacy_faucet_json_validation.py

[shuibui]

Code Review: Approve

Good fix.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix.

**Verdict: Approve.

[TJCurnutte]

Verified review

Approved for PR #4891 at head 15c8216d82607148e601c766d1a5d82b36f85e08.

I inspected the PR metadata/diff for: fix: make faucet drip rate limit atomic.
Changed scope: faucet.py (+75/-38), tests/test_legacy_faucet_json_validation.py (+28/-0).

Local validation

• git diff --check origin/main...HEAD — pass
• python3 tools/bcos_spdx_check.py --base-ref origin/main — pass
• python3 -B -m py_compile faucet.py tests/test_legacy_faucet_json_validation.py — pass
• python3 -B -m pytest tests/test_legacy_faucet_json_validation.py -q — pass
• added-line secret pattern scan — pass

Review reasoning

• The diff is focused on the stated security/validation/bugfix scope and is small enough to review in this bounty tick.
• Whitespace, SPDX, syntax/static, and added-line secret checks passed for this exact head.
• I did not find a blocker in the changed files or validation output.

Boundary: this review certifies the current diff/head only; payout/merge is not assumed.

[himanalot]

Reviewed the root faucet.py rate-limit changes and the concurrent regression test. The drip route now performs the IP check, wallet check, and insert under a single SQLite BEGIN IMMEDIATE write transaction, so concurrent requests serialize before the insert and losers observe the recorded drip. Existing read helpers are preserved through cursor/timestamp helper extraction.

I do not see a blocking issue in this patch. Approved.

[guangningsun]

PR Review — Standard Quality ✓

PR: #4891 — Fix: make faucet drip rate limit atomic

What I reviewed

• faucet.py
• tests/test_legacy_faucet_json_validation.py

Observations

1. Atomic rate limiting prevents faucet drain via race conditions — without atomic operations, concurrent requests could bypass the rate limit. Using database transactions with proper locking ensures the rate limit is enforced correctly even under concurrent load.

2. New tests using ThreadPoolExecutor validate concurrent drip requests — testing rate limiting under concurrent load is exactly the right way to verify the fix works.

3. SQLite concurrent access is notoriously tricky — proper use of transactions and isolation levels is essential for correct behavior.

LGTM.

Bounty: #2782
Disclosure: I received RTC compensation for this review.