Skip to content

fix: keep Goat RPC and the network loop out of SQLite write locks - #533

Open
KSlashh wants to merge 3 commits into
devfrom
audit/20-21-storage-locks
Open

KSlashh wants to merge 3 commits into
devfrom
audit/20-21-storage-locks

Conversation

@KSlashh

@KSlashh KSlashh commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

Audit items M20, M21 and M22. Open against dev.

Event watcher (M21). Each block range ran in one deferred transaction that read, then called esplora and Goat (one debug_traceTransaction per swap event), then wrote. Under WAL, a commit by anyone else in between made the first write fail with BUSY_SNAPSHOT and the whole range ran again. Once the write went through, the lock was held through the rest of the RPC calls, so engine and inbox writes waited behind them. Anyone can create a swap escrow, so the number of traces is attacker-controlled.

A range now has two parts:

  • Fetch: query the subgraph, trace the swap transactions, and build the instances that the BridgeIns of the range need. All of this happens outside any transaction; the database is only read, on a pool connection that is released before the build.
  • Apply: run in a BEGIN IMMEDIATE transaction with no I/O and write in the same entity order as before.

The instance a BridgeIn needs comes from the BridgeInRequest of the same range or from the stored record. If that record appears between fetch and apply, the apply bails and the range is retried.

Network loop (M20). The swarm loop wrote the inbox row and the peer timestamp synchronously, with a 120 s busy timeout. Any long write lock froze gossipsub validation, heartbeats, GetGraph and publishing. Those writes now go to a writer task through a bounded queue of 64 rows and 64 MiB. The gossipsub verdict is reported after the row is written:

  • Accept when the row is recorded.
  • Ignore when the write fails; the sender's outbox sends the message again.

When the queue is full, the swarm reports Ignore at once and counts inbox_queue_full. Peer timestamp refreshes are skipped while the writer is behind.

Engine gauges (M22). A state whose row count dropped to zero was no longer reported, so its gauge kept the last value. Now every label seen before and missing from the current count is set to 0.

Review notes

  • A message whose write takes longer than gossipsub's mcache window (about history_length heartbeats) is still recorded, but this node no longer forwards it. Other peers and the outbox cover it.
  • Ranges are applied in the same order as before, one transaction per range.

Tests: a missing instance is built from the request of the range or from its record; a BridgeIn whose instance was not built retries the range; the inbox writer records off the network loop and hands back the verdict; a full queue refuses instead of waiting; an emptied state's gauge goes back to zero. Node, engine and client tests, workspace clippy with -D warnings and fmt pass.

The test coverage and validation notes above are from the zkm PR notes. Branch scope and diff formatting were checked locally; builds and tests were not rerun during publication.

This branch has not been deployed

No deployments
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