Check confirmation/existence via scripthash history, not verbose replies (B-41)
poll_once and reconcile.py's existence check both called
blockchain.transaction.get(txid, verbose=True). Several Electrum server
implementations and versions reject the verbose flag outright
("verbose transactions are currently unsupported"), which would have
meant no confirmations and no reconciliation ever running against such
a server, read as a plain transport error. reconcile.py additionally
decided whether to abandon a transaction - releasing its funds - by
substring-matching the error text ("missing", "not found", ...), which
only works against ElectrumX's specific wording.
Both now ask blockchain.scripthash.get_history for the address that
owns every input of the transaction (a user's own address for a
bet/withdrawal, the pool address for a payout) and look for the txid in
the result: present with height > 0 means confirmed, present with
height <= 0 means still in the mempool, absent means the server
doesn't know it. get_history is a plain, universally-supported Electrum
method, and "not in the list" replaces the old substring-matching
entirely - no more guessing at error wording to decide whether to
release funds. History is cached per scripthash within one pass, since
every "payout" row shares the same pool address.
New app/tx/pending_address.py factors out own_address_for (the
address derivation was previously duplicated informally inside
tx/broadcast.py's signing context) so confirmation.py and reconcile.py
share one definition instead of two that could compute different
addresses for the same row.
tests/unit/test_confirmation.py and test_reconcile.py needed real User
rows and a master-key bootstrap they didn't have before, since address
derivation is now exercised for real rather than assumed. Suite grows
from 217 to 222 tests. BUGS.md moves B-41 to Previously fixed - no
Medium-severity finding remains open.
This commit is contained in:
@@ -1,11 +1,11 @@
|
||||
# Known bugs
|
||||
|
||||
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
|
||||
7 medium, 8 low), listed below as B-41 … B-49. B-25 through B-40 are fixed (see "Previously
|
||||
fixed" below) — no Critical-severity finding remains open; the other 9 are Medium/Low.
|
||||
7 medium, 8 low), listed below as B-42 … B-49. B-25 through B-41 are fixed (see "Previously
|
||||
fixed" below) — no Critical- or Medium-severity finding remains open; the other 8 are Low/hygiene.
|
||||
The 139-test suite was green at the time of the audit, so none of these were caught by existing
|
||||
coverage — every fix lands with a regression test (the sixteen fixes so far brought the suite
|
||||
from 139 to 217).
|
||||
coverage — every fix lands with a regression test (the seventeen fixes so far brought the suite
|
||||
from 139 to 222).
|
||||
|
||||
The recurring pattern across the open findings is worth stating once: the code is rigorous
|
||||
about the failure modes that have actually been hit, and silent about the ones that have not.
|
||||
@@ -16,29 +16,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### B-41 — Confirmation/reconciliation logic depends on `verbose=True`, which is not universally supported
|
||||
|
||||
`poll_once` and `reconcile._tx_exists_on_chain` call `blockchain.transaction.get(txid, True)`.
|
||||
Several Electrum server implementations and versions reject the verbose flag ("verbose
|
||||
transactions are currently unsupported"). Falling back onto such a server means **no
|
||||
confirmations, no reconciliation** — and the code would read that as a transport error and stay
|
||||
silent. (`bump_fee`'s own `verbose=True` call was removed as part of the B-40 fix — it now reads
|
||||
the raw transaction and parses the output value with `embit` instead, so bumps are unaffected by
|
||||
this finding.)
|
||||
|
||||
Related: `reconcile.py:83` decides whether to **abandon a transaction** by substring-matching
|
||||
the error text (`"missing"`, `"not found"`, `"no such"`, `"unknown"`). It works against
|
||||
ElectrumX; it is fragile as the basis for a decision that releases funds.
|
||||
|
||||
**Proposed fix.** Use `blockchain.transaction.get_merkle` (or the scripthash history) for
|
||||
confirmation and existence checks — both are portable and give the confirming height directly.
|
||||
Probe verbose support once at connect time and record it on the client, so an unsupported
|
||||
server is detected loudly at session start rather than silently mid-operation.
|
||||
|
||||
---
|
||||
|
||||
## Low / hygiene
|
||||
|
||||
### B-42 — `/docs` exposed in production
|
||||
@@ -121,9 +98,10 @@ already does.
|
||||
- **B-38** — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling
|
||||
- **B-39** — SQLite ran without WAL or a `busy_timeout`, so a writer could block every reader and a second writer failed immediately instead of waiting
|
||||
- **B-40** — `bump_fee` held a DB session open across N slow network calls, and computed a prevout's value from a server-reported float instead of an exact integer
|
||||
- **B-41** — confirmation/reconciliation depended on a verbose `blockchain.transaction.get` reply many Electrum servers reject, and abandonment relied on fragile substring-matching of an error message
|
||||
|
||||
See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the
|
||||
B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39/B-40 fixes). Suite grew from 139 to 217 tests over the sixteen.
|
||||
B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39/B-40/B-41 fixes). Suite grew from 139 to 222 tests over the seventeen.
|
||||
|
||||
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python
|
||||
module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
|
||||
|
||||
Reference in New Issue
Block a user