Skip to content

Give the record a file it can live in without holding anything - #42

Merged
Wired4ncer merged 8 commits into
mainfrom
feat/storage-layer
Sep 24, 2026
Merged

Wired4ncer merged 8 commits into
mainfrom
feat/storage-layer

Conversation

@Wired4ncer

@Wired4ncer Wired4ncer commented Sep 21, 2026 •

Copy link
Copy Markdown
Owner

Slice 1 of #23: the persistence layer, so that enrolment has somewhere to write spk_ct and #21 has something to read back. Nothing here accepts a request yet; the API and the arming service are the next slice.

What is in it

storage/ — the §3 schema in SQLite: secure_delete=ON, WAL, foreign keys with cascade, day-precision timestamps, no event table (I3). The capability token is minted once and only sha256(token) is stored. spk_ct / label_ct / dest_ct are AEAD under k_store with the column's purpose and the tenant id as associated data, so a ciphertext cannot be read as another column or moved between watches.

The item state machine is enforced by the store, not trusted to the caller: arm only from arming; activate only from armed and only once a routed channel has a confirmed test-fire (I5). Store is also the matching loop's WatchIndex, so tests/test_store.py runs the real Matcher on the file.

One state added to the sketch — armed, between arming and active. The baseline scan finishing and the user confirming the test-fire are minutes to days apart, and a coin spent in between is still a spend the block path must fold in; so armed and paused items are tracked but not alertable. Architecture §3 has the reasoning.

crypto/ — ChaCha20-Poly1305 (RFC 8439), hand-rolled under the #3 policy and checked against the RFC's own vectors (§2.5.2, §2.8.2, A.3, A.5). ChaCha20 moves here from channels/nostr/; a shim keeps the old import working. No new dependency.

What the tests measure rather than assume

The important ones read the database file's raw bytes after the connection closes: the script, label, destination and token are absent from the file; the keyed hash is present; a purge leaves neither the ciphertext nor the token hash behind.

Review

Independently reviewed before this PR was opened. Three findings fixed in the second commit, each reproduced first:

  1. add_outpoint after a concurrent purge_watch raised a foreign-key error out of the block path (INSERT OR IGNORE does not cover FK failures) and nothing above Subscriber.run catches it — one tenant purging at the wrong instant stopped every tenant's watcher. The same window with stop_item left a stopped item owning a coin that would later match OUTGOING. The insert is now conditional on the item existing and being tracked.
  2. The docs prescribed "replay missed blocks, then arm". Against this index that is a no-op — an arming item is invisible. The order is arm first, then replay, for that item only, serialised with the live block path. Corrected before Enrolment API, capability tokens, and the ARMING state machine #23 builds on it.
  3. Smaller: BadTransition for pause/resume from the wrong state; a newer schema version is refused rather than re-stamped; the VACUUM in purge_watch recorded as a known-equivalent mutation (unobservable while secure_delete is on); the threads test renamed to what it actually checks.

Not addressed, noted for the next slice: watch.status (grace/stopped) is not consulted by the index and nothing sets it yet.

Second review (2026-09-24)

A second review after opening found two more defects, both reproduced before fixing; four rounds of independent review of the fixes, each SHIP.

  1. A purge left the tenant's data in -wal. The token hash and spk_ct stayed in the write-ahead log for as long as the connection was open, which in a running service is always; the purge test only looked after close. purge_watch now ends with VACUUM + wal_checkpoint(TRUNCATE) and raises PurgeIncomplete if another connection blocks either -- at once, not after a 5 s busy wait with the store's lock held. finish_purge() is the retry (a second purge_watch cannot be: the rows are gone). Only SQLITE_BUSY/SQLITE_LOCKED are retryable; any other error propagates as itself. The test now also reads the file with the connection open.
  2. Ids were reissued after a purge. Plain INTEGER PRIMARY KEY hands out the highest id again once its row is deleted, so the next tenant got the purged tenant's watch, item and channel ids -- and an id still held in memory by the block path or a delivery would resolve to the wrong tenant, or decrypt the wrong tenant's destination. watch, watch_item, channel and outbox are AUTOINCREMENT now. The cost is sqlite_sequence, a count of ids ever issued; §3 records it.

Also: journal_mode=WAL is checked rather than assumed, and store.py now states the one remaining gap -- outside a purge, a deleted row's old page (a spent coin's outpoint_hmac, later a completed outbox row) stays in -wal until the log wraps. Keyed hashes, not ciphertexts.

Numbers

505 tests. 30 mutations for the new modules, all caught, none recorded as survivors; 106 in the sweep. The VACUUM mutation is caught only because a test sees that it ran -- what it cleans beyond secure_delete is still untested, and the mutation is named to say so.

🤖 Generated with Claude Code

Wired4ncer and others added 3 commits September 21, 2026 15:29
Slice 1 of #23: the persistence layer, so that enrolment has somewhere to write
`spk_ct` and #21 has something to read back.

`storage/` is the §3 schema in SQLite -- secure_delete on, WAL, foreign keys, day-
precision timestamps, no event table -- with the item state machine enforced by the
store rather than trusted to the caller: `arm` only from `arming`, `activate` only
from `armed` and only once a *routed* channel has a confirmed test-fire. `Store`
is also the matching loop's `WatchIndex`, so the real `Matcher` runs on the file.

One state added to the sketch: `armed`, between `arming` and `active`. The baseline
scan finishing and the user confirming the test-fire are minutes to days apart, and
a coin spent in between is still a spend the block path must fold in -- so `armed`
(and `paused`) items are tracked but not alertable.

`crypto/` is ChaCha20-Poly1305 (RFC 8439) for the `*_ct` columns, hand-rolled under
the #3 policy and checked against the RFC's own vectors (§2.5.2, §2.8.2, A.3, A.5).
The associated data is the column's purpose and the tenant id, so a ciphertext cannot
be read as another column or moved to another watch. ChaCha20 moves here from the
Nostr package; a shim keeps the old import working.

The tests that matter read the database file's raw bytes: the script, label,
destination and token are measured absent, not assumed encrypted. 18 mutations
added, all caught; 496 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on the storage slice, all reproduced by the reviewer:

- `add_outpoint` after a concurrent `purge_watch` raised a foreign-key error out of
  the block path -- `INSERT OR IGNORE` does not cover FK failures -- and nothing above
  `Subscriber.run` catches it, so one tenant nuking at the wrong instant stopped every
  tenant's watcher. The same window with `stop_item` left a stopped item owning a coin
  that would later match OUTGOING. The insert is now conditional on the item existing
  and being tracked; both become no-ops.
- The docs prescribed "replay the missed blocks, then arm". Against this index that is
  a no-op: an arming item is invisible, so the replay folds in nothing. The order is
  arm first, then replay -- for that item only, serialised with the live block path.
  Corrected in the `arm` docstring and architecture §3 before #23 builds on it.
- `pause`/`resume` from the wrong state raised `NotActivatable`; now `BadTransition`,
  which `NotActivatable` subclasses.
- A file stamped with a newer schema version is refused, not silently re-stamped.
- The VACUUM in `purge_watch` is unobservable while secure_delete is on; recorded as a
  known-equivalent mutation with the argument rather than left to be rediscovered.
- The threads test claimed to exercise the lock and did not; renamed to what it checks.
- Stale docstrings in crypto/ pointing at NIP-44 as the only authenticator.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
96, per `tools/mutate.py --list`. Issue #34 is right that this number does not
belong in a comment; until it moves, at least make it the tool's number.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things the docs have been getting wrong.

CONTRIBUTING said the runtime has exactly one third-party dependency. That stopped
being true on 2026-08-12, when #3 accepted coincurve and websocket-client; SECURITY.md
was amended then and this was not. It now states three, points at the table that says
what each is trusted with, and carries the rule #3 actually settled -- which is not a
count but a shape: impossible or dangerous in Python, outside the alert path, and no
published vectors we could check ourselves.

The README's status table said enrolment, payment and delivery were all 'not started'.
Delivery has been built since #32 and #35, and enrolment now has its storage layer.
Split into four rows, each saying what is *not* proven -- neither channel has yet
delivered an alert raised by the engine, because nothing can enrol.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@Wired4ncer Wired4ncer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Independent review — fix two things before merge

Reviewed at 44587b1. All 500 tests pass against this branch's own source. The ChaCha20-Poly1305 transcription matches RFC 8439 on reading, and it is checked against the RFC vectors. The state machine, the add_outpoint race fix, the armed state and the "arm first, then replay" order all hold up. The docs changes match the code.

1. Purged data stays on disk while the connection is open

After purge_watch, the -wal file still holds the purged tenant's token hash and spk_ct. Reproduced with the connection still open: both present, WAL at 156,592 bytes. They only go away when the last connection closes and SQLite deletes the WAL. A long-running service rarely closes it.

test_purge_removes_the_tenant_and_the_bytes reads the bytes only after Store is closed, so it cannot see this.

Fix: after the VACUUM in purge_watch (store.py:235), run PRAGMA wal_checkpoint(TRUNCATE) and check that its busy column is 0. Verified: the WAL goes to 0 bytes and neither value remains in the file. The test should also read the bytes before closing.

2. Row ids are reused after a purge

watch, watch_item and channel use INTEGER PRIMARY KEY without AUTOINCREMENT, so SQLite reuses the highest id once that row is deleted. Reproduced: purge watch 2 (item 1), create a new watch, and it gets watch id 2 and item id 1 again.

Why it matters:

  • The add_outpoint docstring already says the loop can hold an item id while the API thread purges it. That id can now belong to another tenant. The status IN (tracked) guard only helps while the new item is still arming.
  • A delivery holding an old channel id would call channel_dest and decrypt the new tenant's destination. That sends one tenant's alert to another tenant.
  • The AAD binds each ciphertext to a tenant id, and that binding is weaker when ids are not unique over time.

Fix: add AUTOINCREMENT to those three tables. Verified that it stops reuse. Side effect: sqlite_sequence then stores how many rows were ever created, which roughly tells how many tenants the service has had. That is probably acceptable, but it belongs in §3.

Minor (fine as follow-ups)

  • Swaps within one tenant. The AAD is purpose + watch_id only, so someone with write access to the file could swap spk_ct/label_ct between two items of the same tenant, or dest_ct between its channels. Nothing ties spk_ct to spk_hmac either. Adding the item or channel id to the AAD closes it. The PR only claims per-tenant binding, so this is not a false claim.
  • Lengths are visible. Ciphertext length shows plaintext length. For spk_ct that shows the script type (22 vs 34 bytes); for label_ct, the label length. Pad before sealing if that matters for the threat model.
  • CONTRIBUTING, "no transitive requirements". True on CPython, but pyzmq requires cffi on PyPy (implementation_name == "pypy").
  • Mutation count. The PR body says 20 mutations were added and the first commit message says 18. The CI comment's 96 comes from the tool and is right.

Wired4ncer and others added 4 commits September 24, 2026 10:52
Second review of this PR, both reproduced before fixing:

- After `purge_watch` the tenant's token hash and `spk_ct` were still in `-wal`: WAL mode
  keeps every page as written, and only closing the last connection deletes the log -- which
  a running service never does. The purge test only looked after close, so it could not see
  it. The purge now ends with `wal_checkpoint(TRUNCATE)` and raises `PurgeIncomplete` if a
  reader kept the checkpoint from finishing, rather than letting the caller say "gone". The
  test now also reads the bytes with the connection open.
- `watch`, `watch_item` and `channel` were plain `INTEGER PRIMARY KEY`, so SQLite reissued
  the highest id once its row was deleted: purge the newest tenant and the next one got the
  same watch, item and channel ids. An id still held in memory by the block path or a
  delivery would then resolve to the wrong tenant -- `channel_dest` would decrypt someone
  else's destination. They are `AUTOINCREMENT` now. The price is `sqlite_sequence`, a
  counter of ids ever issued; the no-event-table test names it and pins its two columns.

Schema stays at version 1: nothing has enrolled, so no file exists to migrate.

5 mutations added, all caught; 101 in the sweep. 502 tests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Independent review of the previous commit said SHIP, with findings; the cheap ones are
fixed here rather than carried:

- A `PurgeIncomplete` purge had no way to finish: the rows were already gone, so a second
  `purge_watch` raised `UnknownRow` -- which a caller would read as "already done" with the
  data still in `-wal`. `finish_purge()` is now the retry, and the exception says so.
- The truncating checkpoint waited out sqlite3's 5 s busy handler with the store's lock
  held, so one reader stalled the matching thread for 5 s. It now runs with busy_timeout=0
  and fails at once; the timeout is restored in `finally`. The test pins "under 1 s".
- `outbox.id` had the same reissue bug, worse: its rows are deleted on every delivery.
  `AUTOINCREMENT` now, while schema version 1 is still free to change.
- `journal_mode=WAL` is checked, not assumed; the purge's guarantee depends on it.
- store.py claimed a deleted row is overwritten. Outside a purge the WAL keeps its old page
  until the log wraps -- keyed hashes (`outpoint_hmac`, later outbox rows), not ciphertexts.
  Stated now instead.
- The purge test also checks `label_ct` and `dest_ct`; `file_bytes`' docstring fixed.

Not changed: a VACUUM that fails after the delete commits leaves the same "rows gone, file
not clean" state; `finish_purge` covers the WAL half of it, and VACUUM failing is rare.

3 mutations added, 2 re-anchored; 104 in the sweep. 503 tests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Round 2 of the independent review said SHIP; these are its remaining findings.

- A VACUUM that failed after the delete committed came out as a raw OperationalError, so
  the caller was never told to call `finish_purge` -- and a second `purge_watch` raises
  `UnknownRow`, the trap the last commit closed for the checkpoint. VACUUM and the
  checkpoint are now one `_scrub` step under busy_timeout=0; any OperationalError from it
  is `PurgeIncomplete`, and `finish_purge` retries both. The busy_timeout=0 moved inside
  the `try`, so nothing can leave it set.
- `finish_purge`'s docstring said it returns "once the log is empty", as if it waits. It
  makes one attempt.
- The WAL guard refused `file::memory:` URIs (mode "memory"); allowed, since there is no file.
  `""` (SQLite's private temp file) stays refused, and the comment says so.
- Two over-width lines rewrapped.

1 mutation added, 3 re-anchored; 105 in the sweep. 504 tests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Round 3 of the independent review (SHIP) found `_scrub` mapped every OperationalError to
`PurgeIncomplete`, whose instruction is "call finish_purge again". A statement left open, a
full disk or a read-only file fails the same way forever, so a caller following that would
loop without seeing why. Only SQLITE_BUSY and SQLITE_LOCKED (primary code, so extended
variants count) become `PurgeIncomplete` now; anything else propagates as itself. An error
raised by the sqlite3 module itself carries no code and counts as permanent.

`finish_purge`'s docstring now says what it costs -- a full VACUUM under the store's lock --
so it does not end up on a timer. The writer-blocks test's docstring no longer claims to
reproduce the post-DELETE VACUUM failure, which a test cannot stage.

The VACUUM mutation is no longer a recorded survivor, but only because the new open-cursor
test sees *that* VACUUM ran. What it cleans beyond secure_delete is still untested; the
mutation is renamed to say so (round 4 of the review, SHIP), so the sweep is not read as
proof of the page guarantee.

1 mutation added, 1 re-anchored, 1 renamed; 106 in the sweep. 505 tests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Wired4ncer
Wired4ncer merged commit cc62313 into main Sep 24, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant