Skip to content

sqldb: prevent pool access from transaction callbacks - #11347

Open
ziggie1984 wants to merge 2 commits into
lightningnetwork:masterfrom
ziggie1984:fix-exectx-callback-scope
Open

ziggie1984 wants to merge 2 commits into
lightningnetwork:masterfrom
ziggie1984:fix-exectx-callback-scope

Conversation

@ziggie1984

Copy link
Copy Markdown
Collaborator

Change Description

Fixes #11346 by using the transaction-bound query object in
InvoicesSettledSince, avoiding a second pool connection from inside
ExecTx.

Adds the txscope analyzer to prevent transaction callbacks from accessing
their originating executor, while still allowing access to a different DB.

Steps to Test

go test ./invoices -run '^TestInvoicesSettledSinceSingleConnection$'
make lint-native

Fetch settled AMP sub-invoices through the query object supplied to the transaction callback. Using the store's pool here can wait for a second connection while the transaction retains the first one, deadlocking a constrained pool.

Add a single-connection SQLite regression test so any pool access from this transaction fails instead of going unnoticed.
Add the txscope analysis plugin to prevent ExecTx callbacks from accessing the executor that opened their transaction. Detect direct uses, alias chains, conversions, helper arguments, and nested transactions while permitting calls through a distinct database.

Require inline callbacks so the check cannot be bypassed through a named function, and enable the analyzer in the repository lint configuration.
@ziggie1984 ziggie1984 self-assigned this Oct 17, 2026
@ziggie1984 ziggie1984 added this to v0.21 Oct 17, 2026
@ziggie1984 ziggie1984 moved this to In progress in v0.21 Oct 17, 2026
@github-actions github-actions Bot added the severity-high Requires knowledgeable engineer review label Oct 17, 2026
@github-actions

Copy link
Copy Markdown

🟠 PR Severity: HIGH

gh pr view | 8 files | 467 lines changed

🟠 High (1 file)
  • invoices/sql_store.go - invoices/* package (invoice management and settlement)
🟡 Medium (1 file)
  • .golangci.yml - repo-wide lint configuration change
🟢 Low (6 files)
  • invoices/sql_store_tx_test.go - test-only change
  • tools/linters/go.mod - tooling dependency module file
  • tools/linters/go.sum - tooling dependency checksum file
  • tools/linters/testdata/src/txscope/txscope.go - linter testdata fixture
  • tools/linters/txscope.go - new custom linter implementation
  • tools/linters/txscope_test.go - test-only change

Analysis

The highest-severity file is invoices/sql_store.go, a one-line change in the invoices/* package, which maps to HIGH. The bulk of the diff (302 + 102 lines) is a new custom linter (tools/linters/txscope.go and its testdata) intended to enforce transaction-scope rules, which falls under tools/* (LOW). The .golangci.yml change wires the new linter into CI and is a general repo config change (MEDIUM). No bump thresholds were met: 6 non-test/non-generated files changed (≤20) and 418 non-test lines changed (≤500), and no CRITICAL package is touched. Overall severity: HIGH, driven solely by the invoices/sql_store.go edit.


To override, add a severity-override-{critical,high,medium,low} label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog severity-high Requires knowledgeable engineer review

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

[bug]: two SubscribeInvoices with settle_index deadlock the SQLite connection pool

1 participant