aiolabs/satmachineadmin

Fork 0

v2 follow-up: review-cycle findings (HIGH/MEDIUM/NITS) after fix bundle 1 #11

New issue

Closed

opened 2026-05-14 15:39:03 +00:00 by padreug · 2 comments

padreug commented

2026-05-14 15:39:03 +00:00

Owner

Single tracking issue for review findings on the v2-bitspire branch that were deferred after fix bundle 1 shipped (commit 3ede66f, which closed the wallet-IDOR security finding + concurrency H1/H2/H3 + added the retry endpoint).

A formal security pass + architectural review ran on the branch at the P3e checkpoint (10 commits at that point). Fix bundle 1 addressed the HIGH-severity security issue and three load-bearing concurrency findings. Everything else from those two reviews lives here for follow-up.

HIGH-priority remaining (architectural review)

H4. Decouple invoice listener from distribution

tasks._handle_payment awaits process_settlement synchronously. Each settlement does N+M internal create_invoice/pay_invoice round-trips (N = LP count, M = split legs + super fee). A slow machine (50 LPs, stalled internal payments) freezes the global LNbits invoice queue for every extension on the node, not just satmachineadmin.

Fix: spawn process_settlement on a task — asyncio.create_task(process_settlement(settlement.id)) — and let the listener move on. The new claim-based concurrency guards already prevent double-processing on listener re-fires, so this is safe.

Files: tasks.py (line ~85).

H5. Fallback path silently strands sats

bitspire._parse_fallback sets exchange_rate=0.0 when bitSpire's Payment.extra is absent. distribution._pay_dca_distributions then logs a warning and leaves net_sats orphaned in the machine wallet — but the settlement is still marked 'processed'. From the operator's dashboard, this looks identical to a clean process; they don't know that 258k sats are stuck and need manual reconciliation.

Fix: either mark such settlements as 'partial' instead of 'processed', or write a dca_payments row with leg_type='dca', status='skipped', amount_sats=settlement.net_sats, error_message='no exchange rate'. The skipped-leg approach is consistent with M8 below (super-fee leg skipped because no super wallet).

Awaits aiolabs/lamassu-next#44 for the cleaner long-term fix (bitSpire populates Payment.extra so fallback is unreachable in practice).

Files: bitspire.py, distribution.py.

H6. Partial-dispense recomputes split using current `super_fee_pct`

distribution.apply_partial_dispense_and_redistribute reads super_fee_pct from super_config at the time of partial-dispense, breaking the "absolute fields are the source of truth" invariant the schema and the v2 design were built on. If super raises the rate between landing and partial-dispense, the operator's slice shrinks unfairly.

Fix: re-derive the new platform/operator split from the ratio of the original platform_fee_sats to commission_sats on the settlement row, not from the current super_config:

original_platform_ratio = (
    settlement.platform_fee_sats / settlement.commission_sats
    if settlement.commission_sats > 0
    else 0.0
)
new_platform = round(new_commission * original_platform_ratio)
new_operator = new_commission - new_platform

Files: distribution.py (around line 260-264).

MEDIUM findings

M1. `calculate_distribution` filter threshold unit ambiguity

min_balance_threshold=0.01 in calculations.py is "0.01 of whatever the caller's client_balances dict says it is" (fiat in our case). Docstring example uses fiat literals; comment in distribution.py doesn't anchor the unit. Make the threshold a caller-supplied named arg or rename to min_fiat_balance and document.

M2. DCA leg `amount_fiat` can slightly exceed remaining balance

_pay_dca_distributions does cap_sats = int(remaining_fiat * exchange_rate) then amount_fiat = round(amount_sats / exchange_rate, 2). The floor at cap + round at fiat can push amount_fiat slightly above remaining_fiat, putting the LP at a small negative balance in get_client_balance_summary. Use floor on amount_fiat.

M3. N+1 query in DCA distribution

_pay_dca_distributions calls get_client_balance_summary(client.id) inside a loop over flow-mode clients. Each call does two queries + a get_machine. For a machine with 50 LPs that's 150 DB round-trips per settlement. Replace with a single grouped JOIN that returns (client_id, balance) per active flow-mode LP at the machine. Same applies to any operator dashboard endpoint that hits /balance per LP row.

M4. `char(10)` newline embed is SQLite-only idiom

apply_partial_dispense and append_settlement_note in crud.py use char(10) to embed newlines into the notes blob. SQLite supports char(10) but PG idiom is chr(10). Both happen to accept char(10), but cleaner is to construct the joined string in Python and pass as a single parameter — DB-agnostic.

M5. `hourly_transaction_polling` is dead

tasks.py has a no-op hourly_transaction_polling stub but __init__.py still spawns a permanent task for it. Delete the function + the task spawn + the import. (See fix bundle 3 below for the cleanup-cluster proposal.)

M6. Placeholder debug log in `init.py`

__init__.py:12-15 carries the scaffolding template's "you can debug in your extension using…" log line. Useful during scaffolding; now noise on every boot. Replace with a meaningful "satmachineadmin v2 loaded" line or delete.

M7. Catch-all stub in `views_api.py` is unauthenticated

views_api.v2_in_progress_stub returns 503 for any unmatched /api/v1/dca/* request without auth. Leaks the extension's existence to unauthenticated probes. With P3a-e all shipped, the stale "landing in P2+" comment is misleading; delete the stub entirely (P3f and P6 will land their own endpoints).

M8. Super-fee leg silently no-ops when wallet unset

_pay_super_fee warns to logs and returns when super_fee_wallet_id is None, but the settlement still progresses to 'processed' and the platform_fee_sats value remains in the machine wallet. Operator audit doesn't reflect it. Same fix as H5 — record a dca_payments row with leg_type='super_fee', status='skipped'.

M9. `settle_lp_balance` design note (not a bug)

distribution.settle_lp_balance records its leg with settlement_id=None since it's not tied to a bitSpire settlement. get_client_balance_summary correctly counts both dca + settlement leg_types against the LP balance, so the math is consistent. Worth documenting in the model comment that settlement legs are intentionally independent of any parent dca_settlements row.

M10. `UpdateData` models can't unset* fields

All Update*Data Pydantic models use Optional[str] = None with None meaning "no change" in the CRUD update. Operators can never clear super_fee_wallet_id (or other fields) once set — only overwrite. Adopt a sentinel pattern (e.g. accept "" as "clear") or use Pydantic's Field(...) with a discriminated Unset/SetToValue wrapper.

M11. `replace_commission_splits` is not atomic

crud.py does DELETE + N INSERTs without a transaction. A crash between DELETE and the last INSERT leaves the operator with a partial ruleset that doesn't sum to 1.0. Wrap in a transaction.

M12. `dca_settlements.fiat_amount` defaults to 0.0 in fallback path

Settlement still proceeds with fiat_amount=0.0 + exchange_rate=0 (skips DCA distribution per H5). Column is NOT NULL DECIMAL(10,2), so the zero is a valid number that aggregates into operator reports as "0 GTQ on this tx" — operators see a settlement row with zero fiat and don't know it represents an unprocessed transaction. Either make fiat_amount nullable or set status to 'errored' immediately on fallback path.

NITS

N1. bitspire._parse_extra falls back to _parse_fallback when extra is malformed, but the caller-facing (data, used_fallback) tuple says used_fallback=False. Inconsistent. Reflect actual fallback usage in both data.used_fallback_split and the returned tuple.
N2. _pay_internal uses datetime.now() (naive); settle_lp_balance uses datetime.now(timezone.utc). Pick one.
N3. views_api.py catch-all comment references P2+ but P3a-e have shipped (see M7).
N4. __init__.py:8 imports hourly_transaction_polling even though it's a no-op (see M5).
N5. transaction_processor.py (1274 lines) is orphaned. Delete.
N6. migrations.py mixes v1 m001-m004 with v2 m005-m007. Collapse to a single m001_initial_v2_schema before any operator with v1 data exists in the wild (per CLAUDE.md, "production ready" is stretched here).
N7. calculations.calculate_commission docstring still says "Lamassu transaction"; rename to "Lamassu-formula" since it's now reused via bitspire._parse_fallback.
N8. calculations.allocate_operator_split_legs parameter leg_pcts: list should be list[float] for clarity; return -> list should be -> list[int].
N9. crud.append_settlement_note has from datetime import timezone inline — hoist to module-level imports.
N10. CreateMachineData.commission_in_unit_range and UpdateMachineData.commission_in_unit_range are identical validators. Factor into a shared PctField type or function.
N11. bitspire.parse_settlement warning log lacks payment_hash[:12] — useful for grep'ing logs when the operator knows the hash but not the npub.
N12. _payment_tag(machine) writes the full Nostr npub on every Payment.tag. Consider truncating or using machine.id (shorter, opaque, still uniquely identifies).

Reviewer-proposed fix bundles (already partially shipped)

Fix bundle 1 — security + concurrency ✅ SHIPPED (commit `3ede66f`)

Wallet-ownership check on machine create/update (closes the security finding).
DB UNIQUE index on dca_machines.wallet_id (defence-in-depth).
claim_settlement_for_processing optimistic-lock pattern.
reset_settlement_for_retry + new POST /api/v1/dca/settlements/{id}/retry endpoint.
Closes H1 + H2 + H3 from the architectural review.

Fix bundle 2 — Decouple listener + audit skipped legs ⬜ TODO

asyncio.create_task(process_settlement(...)) in tasks._handle_payment (closes H4).
Write dca_payments rows with status='skipped' in _pay_super_fee / _pay_dca_distributions when sats are stranded (closes H5 + M8).
Adds 'skipped' to the documented leg-status enum.

Fix bundle 3 — Dead-code purge ⬜ TODO

Delete views_api.v2_in_progress_stub + its catch-all route (M7 + N3).
git rm transaction_processor.py (N5; 1274 lines of dead Lamassu code).
Collapse m001..m007 into a single m001_initial_v2_schema (N6).
Delete hourly_transaction_polling + its __init__.py spawn + import (M5 + N4).
Replace the placeholder debug log in __init__.py (M6).
~1400 lines of dead/stale code gone in one PR.

Standalone — H6 ⬜ TODO

Preserve original split ratio on partial-dispense recompute.

What the review confirmed is RIGHT (preserve)

Absolute platform_fee_sats / operator_fee_sats on dca_settlements (not percentages). This is the load-bearing design choice for the post-v1 customer-discount engine — keeping it as audit-grade absolutes makes the discount engine ship without a breaking migration. Documented in migrations.py:300-306.
payment_hash as idempotency key (not bitspire_event_id). LN payment_hash is globally unique and always present.
404 (not 403) on cross-operator probes. Uniform across _machine_owned_by/_client_owned_by helpers.
Two-stage split with last-leg-absorbs-rounding. Well-tested in test_two_stage_split.py.

aiolabs/satmachineadmin#9 — v2 epic
aiolabs/satmachineadmin#10 — future dedicated audit table
aiolabs/lamassu-next#44 — Payment.extra split metadata (turns off the fallback path)

Single tracking issue for review findings on the v2-bitspire branch that were deferred after **fix bundle 1** shipped (commit `3ede66f`, which closed the wallet-IDOR security finding + concurrency H1/H2/H3 + added the retry endpoint). A formal security pass + architectural review ran on the branch at the P3e checkpoint (10 commits at that point). Fix bundle 1 addressed the HIGH-severity security issue and three load-bearing concurrency findings. Everything else from those two reviews lives here for follow-up. --- ## HIGH-priority remaining (architectural review) ### H4. Decouple invoice listener from distribution `tasks._handle_payment` awaits `process_settlement` synchronously. Each settlement does N+M internal `create_invoice`/`pay_invoice` round-trips (N = LP count, M = split legs + super fee). A slow machine (50 LPs, stalled internal payments) freezes the *global* LNbits invoice queue for **every extension** on the node, not just satmachineadmin. **Fix:** spawn `process_settlement` on a task — `asyncio.create_task(process_settlement(settlement.id))` — and let the listener move on. The new claim-based concurrency guards already prevent double-processing on listener re-fires, so this is safe. **Files:** `tasks.py` (line ~85). ### H5. Fallback path silently strands sats `bitspire._parse_fallback` sets `exchange_rate=0.0` when bitSpire's `Payment.extra` is absent. `distribution._pay_dca_distributions` then logs a warning and leaves `net_sats` orphaned in the machine wallet — but the settlement is still marked `'processed'`. From the operator's dashboard, this looks identical to a clean process; they don't know that 258k sats are stuck and need manual reconciliation. **Fix:** either mark such settlements as `'partial'` instead of `'processed'`, or write a `dca_payments` row with `leg_type='dca'`, `status='skipped'`, `amount_sats=settlement.net_sats`, `error_message='no exchange rate'`. The skipped-leg approach is consistent with M8 below (super-fee leg skipped because no super wallet). Awaits `aiolabs/lamassu-next#44` for the cleaner long-term fix (bitSpire populates `Payment.extra` so fallback is unreachable in practice). **Files:** `bitspire.py`, `distribution.py`. ### H6. Partial-dispense recomputes split using *current* `super_fee_pct` `distribution.apply_partial_dispense_and_redistribute` reads `super_fee_pct` from `super_config` at the time of partial-dispense, breaking the "absolute fields are the source of truth" invariant the schema and the v2 design were built on. If super raises the rate between landing and partial-dispense, the operator's slice shrinks unfairly. **Fix:** re-derive the new platform/operator split from the *ratio* of the original `platform_fee_sats` to `commission_sats` on the settlement row, not from the current super_config: ```python original_platform_ratio = ( settlement.platform_fee_sats / settlement.commission_sats if settlement.commission_sats > 0 else 0.0 ) new_platform = round(new_commission * original_platform_ratio) new_operator = new_commission - new_platform ``` **Files:** `distribution.py` (around line 260-264). --- ## MEDIUM findings ### M1. `calculate_distribution` filter threshold unit ambiguity `min_balance_threshold=0.01` in `calculations.py` is "0.01 of whatever the caller's `client_balances` dict says it is" (fiat in our case). Docstring example uses fiat literals; comment in `distribution.py` doesn't anchor the unit. Make the threshold a caller-supplied named arg or rename to `min_fiat_balance` and document. ### M2. DCA leg `amount_fiat` can slightly exceed remaining balance `_pay_dca_distributions` does `cap_sats = int(remaining_fiat * exchange_rate)` then `amount_fiat = round(amount_sats / exchange_rate, 2)`. The floor at cap + round at fiat can push `amount_fiat` slightly above `remaining_fiat`, putting the LP at a small negative balance in `get_client_balance_summary`. Use floor on `amount_fiat`. ### M3. N+1 query in DCA distribution `_pay_dca_distributions` calls `get_client_balance_summary(client.id)` inside a loop over flow-mode clients. Each call does two queries + a `get_machine`. For a machine with 50 LPs that's 150 DB round-trips per settlement. Replace with a single grouped JOIN that returns `(client_id, balance)` per active flow-mode LP at the machine. Same applies to any operator dashboard endpoint that hits `/balance` per LP row. ### M4. `char(10)` newline embed is SQLite-only idiom `apply_partial_dispense` and `append_settlement_note` in `crud.py` use `char(10)` to embed newlines into the `notes` blob. SQLite supports `char(10)` but PG idiom is `chr(10)`. Both happen to accept `char(10)`, but cleaner is to construct the joined string in Python and pass as a single parameter — DB-agnostic. ### M5. `hourly_transaction_polling` is dead `tasks.py` has a no-op `hourly_transaction_polling` stub but `__init__.py` still spawns a permanent task for it. Delete the function + the task spawn + the import. (See fix bundle 3 below for the cleanup-cluster proposal.) ### M6. Placeholder debug log in `__init__.py` `__init__.py:12-15` carries the scaffolding template's "you can debug in your extension using…" log line. Useful during scaffolding; now noise on every boot. Replace with a meaningful "satmachineadmin v2 loaded" line or delete. ### M7. Catch-all stub in `views_api.py` is unauthenticated `views_api.v2_in_progress_stub` returns 503 for any unmatched `/api/v1/dca/*` request without auth. Leaks the extension's existence to unauthenticated probes. With P3a-e all shipped, the stale "landing in P2+" comment is misleading; delete the stub entirely (P3f and P6 will land their own endpoints). ### M8. Super-fee leg silently no-ops when wallet unset `_pay_super_fee` warns to logs and returns when `super_fee_wallet_id` is None, but the settlement still progresses to `'processed'` and the `platform_fee_sats` value remains in the machine wallet. Operator audit doesn't reflect it. Same fix as H5 — record a `dca_payments` row with `leg_type='super_fee', status='skipped'`. ### M9. `settle_lp_balance` design note (not a bug) `distribution.settle_lp_balance` records its leg with `settlement_id=None` since it's not tied to a bitSpire settlement. `get_client_balance_summary` correctly counts both `dca` + `settlement` leg_types against the LP balance, so the math is consistent. Worth documenting in the model comment that `settlement` legs are intentionally independent of any parent `dca_settlements` row. ### M10. `Update*Data` models can't *unset* fields All `Update*Data` Pydantic models use `Optional[str] = None` with `None` meaning "no change" in the CRUD update. Operators can never clear `super_fee_wallet_id` (or other fields) once set — only overwrite. Adopt a sentinel pattern (e.g. accept `""` as "clear") or use Pydantic's `Field(...)` with a discriminated `Unset`/`SetToValue` wrapper. ### M11. `replace_commission_splits` is not atomic `crud.py` does DELETE + N INSERTs without a transaction. A crash between DELETE and the last INSERT leaves the operator with a partial ruleset that doesn't sum to 1.0. Wrap in a transaction. ### M12. `dca_settlements.fiat_amount` defaults to 0.0 in fallback path Settlement still proceeds with `fiat_amount=0.0` + `exchange_rate=0` (skips DCA distribution per H5). Column is `NOT NULL DECIMAL(10,2)`, so the zero is a valid number that aggregates into operator reports as "0 GTQ on this tx" — operators see a settlement row with zero fiat and don't know it represents an unprocessed transaction. Either make `fiat_amount` nullable or set status to `'errored'` immediately on fallback path. --- ## NITS - **N1.** `bitspire._parse_extra` falls back to `_parse_fallback` when extra is malformed, but the caller-facing `(data, used_fallback)` tuple says `used_fallback=False`. Inconsistent. Reflect actual fallback usage in both `data.used_fallback_split` and the returned tuple. - **N2.** `_pay_internal` uses `datetime.now()` (naive); `settle_lp_balance` uses `datetime.now(timezone.utc)`. Pick one. - **N3.** `views_api.py` catch-all comment references P2+ but P3a-e have shipped (see M7). - **N4.** `__init__.py:8` imports `hourly_transaction_polling` even though it's a no-op (see M5). - **N5.** `transaction_processor.py` (1274 lines) is orphaned. Delete. - **N6.** `migrations.py` mixes v1 m001-m004 with v2 m005-m007. Collapse to a single `m001_initial_v2_schema` before any operator with v1 data exists in the wild (per CLAUDE.md, "production ready" is stretched here). - **N7.** `calculations.calculate_commission` docstring still says "Lamassu transaction"; rename to "Lamassu-formula" since it's now reused via `bitspire._parse_fallback`. - **N8.** `calculations.allocate_operator_split_legs` parameter `leg_pcts: list` should be `list[float]` for clarity; return `-> list` should be `-> list[int]`. - **N9.** `crud.append_settlement_note` has `from datetime import timezone` inline — hoist to module-level imports. - **N10.** `CreateMachineData.commission_in_unit_range` and `UpdateMachineData.commission_in_unit_range` are identical validators. Factor into a shared `PctField` type or function. - **N11.** `bitspire.parse_settlement` warning log lacks `payment_hash[:12]` — useful for grep'ing logs when the operator knows the hash but not the npub. - **N12.** `_payment_tag(machine)` writes the full Nostr npub on every Payment.tag. Consider truncating or using `machine.id` (shorter, opaque, still uniquely identifies). --- ## Reviewer-proposed fix bundles (already partially shipped) ### Fix bundle 1 — security + concurrency ✅ SHIPPED (commit `3ede66f`) - Wallet-ownership check on machine create/update (closes the security finding). - DB UNIQUE index on `dca_machines.wallet_id` (defence-in-depth). - `claim_settlement_for_processing` optimistic-lock pattern. - `reset_settlement_for_retry` + new `POST /api/v1/dca/settlements/{id}/retry` endpoint. - Closes H1 + H2 + H3 from the architectural review. ### Fix bundle 2 — Decouple listener + audit skipped legs ⬜ TODO - `asyncio.create_task(process_settlement(...))` in `tasks._handle_payment` (closes H4). - Write `dca_payments` rows with `status='skipped'` in `_pay_super_fee` / `_pay_dca_distributions` when sats are stranded (closes H5 + M8). - Adds `'skipped'` to the documented leg-status enum. ### Fix bundle 3 — Dead-code purge ⬜ TODO - Delete `views_api.v2_in_progress_stub` + its catch-all route (M7 + N3). - `git rm transaction_processor.py` (N5; 1274 lines of dead Lamassu code). - Collapse `m001..m007` into a single `m001_initial_v2_schema` (N6). - Delete `hourly_transaction_polling` + its `__init__.py` spawn + import (M5 + N4). - Replace the placeholder debug log in `__init__.py` (M6). - ~1400 lines of dead/stale code gone in one PR. ### Standalone — H6 ⬜ TODO - Preserve original split ratio on partial-dispense recompute. --- ## What the review confirmed is RIGHT (preserve) - **Absolute `platform_fee_sats` / `operator_fee_sats`** on `dca_settlements` (not percentages). This is the load-bearing design choice for the post-v1 customer-discount engine — keeping it as audit-grade absolutes makes the discount engine ship without a breaking migration. Documented in `migrations.py:300-306`. - **`payment_hash` as idempotency key** (not `bitspire_event_id`). LN payment_hash is globally unique and always present. - **404 (not 403) on cross-operator probes.** Uniform across `_machine_owned_by`/`_client_owned_by` helpers. - **Two-stage split with last-leg-absorbs-rounding.** Well-tested in `test_two_stage_split.py`. --- ## Related - `aiolabs/satmachineadmin#9` — v2 epic - `aiolabs/satmachineadmin#10` — future dedicated audit table - `aiolabs/lamassu-next#44` — `Payment.extra` split metadata (turns off the fallback path)