We should avoid taking the lock of a mutex inside transaction.
Currently we also take this lock in other places and there is a
chance that in case the application lock aquires the lock but
all transactions are already blocked waiting for the mutex to
unlock, we end up in a deadlock.
The DisconnectBlockAtHeight method was modifying the rejectCache and
chanCache without holding the cacheMu lock. This caused races with
other operations that properly held the lock, such as AddChannelEdge
which modifies the caches in its OnCommit callback while the batch
scheduler holds cacheMu.
Fix by acquiring cacheMu before removing channels from the caches.
Both NodeKey1 and NodeKey2 methods had the same race condition as the
Node.PubKey method, where concurrent calls could race to write to the
cached fields.
Remove the caching for the same reasons: parsing overhead is minimal
and doesn't justify the complexity and race risk.
The PubKey method had a race condition where concurrent calls could
all pass the nil check and race to write to the cached pubKey field.
This is a classic check-then-act race.
Remove the caching entirely to fix the race. The overhead of parsing
a public key is minimal and doesn't justify the added complexity and
race risk of caching.
This commit fixes a race condition where multiple goroutines call
SetSourceNode concurrently during startup, causing sql.ErrNoRows
errors. The race occurs when multiple code paths (setSelfNode,
createNewHiddenService, RPC updates) read the same old timestamp,
independently increment it to the same new value (T+1), and race to
write.
The fix uses the new UpsertSourceNode SQL query (without strict
timestamp constraint) instead of UpsertNode. This allows
last-write-wins semantics for our own node, ensuring all parameter
changes persist even when timestamps collide.
Refactored sql_store.go for reusability:
- upsertNodeAncillaryData: common logic for features/addresses/extras
- populateNodeParams: common parameter building with callback pattern
- buildNodeUpsertParams: builds params for strict UpsertNode
- buildSourceNodeUpsertParams: builds params for lenient UpsertSourceNode
- upsertSourceNode: new function using lenient query
Updated TestSetSourceNodeSameTimestamp to verify that concurrent
updates with the same timestamp now succeed and parameter changes
persist.
Fixes the itest error:
"unable to upsert source node: upserting node(...): sql: no rows in
result set"
This query is less strict in terms of the latest update timestamp field.
We want to be less strict with our own node data since we always want
our own updates recorded.
This commit adds TestSetSourceNodeSameTimestamp to demonstrate the
current behavior when SetSourceNode is called with the same last update
timestamp. The test reveals a difference between the SQL and bbolt
implementations:
- SQL store returns sql.ErrNoRows when attempting to update with the
same timestamp, as the upsert query's UPDATE clause requires the new
timestamp to be strictly greater than the existing one
- bbolt store silently ignores stale updates and returns no error
This behavior is important to document because our own node
announcements may change quickly with the same timestamp, unlike
announcements from other nodes where same timestamp typically means
identical parameters.
We now allow the mission control manager to skip over deserializable
errors. We cannot repair this these results but we just skip over
it so we can startup properly.
When fetchAll() encounters entries that fail to deserialize, in
addition to skipping them, now also:
- Delete the corrupted entries from the database
- Remove them from the in-memory keysMap and keys tracking structures
This prevents corrupted entries from:
- Being counted toward maxRecords, which would cause valid entries
to be pruned prematurely
- Persisting in the database indefinitely
- Causing inaccurate entry counts in startup logs
This commit enhances the integration test to validate the LSP heuristic
end-to-end with real network topology and payment probing.
Network topology additions:
- Added Frank node as a private destination
- Created multi-LSP test scenario with Bob, Eve, and Dave as LSPs
New test cases:
1. "probe based estimate, public target with public hop hints"
- Validates Rule 1: public invoice target routes directly
- Even with public hop hints, direct routing is used
- Expected: standard single-hop fees
2. "probe based estimate, multiple different public LSPs"
- Validates multi-LSP worst-case selection
- Frank has routes through Bob (low fee), Eve (HIGH fee), Dave (medium)
- Expected: Eve's worst-case fees (most expensive)
- Tests griefing protection (max 3 LSP probes)
This commit implements a comprehensive LSP (Lightning Service Provider)
detection heuristic and updates the payment probing logic to handle
multiple LSPs with worst-case fee estimation.
Key changes:
1. LSP Detection Heuristic (isLSP function):
Implements three rules to detect LSP setups:
- Rule 1: If invoice target is public → NOT an LSP (route directly)
- Rule 2: If at least one destination hop is public → IS an LSP
- Rule 3: If all destination hops are private → NOT an LSP
2. LSP Route Preparation (prepareLspRouteHints function):
- Groups route hints by unique public LSP nodes
- Filters out non-LSP routes based on the heuristic
- Tracks worst-case fees and CLTV delays for each LSP
- Returns adjusted route hints with LSP hop stripped
3. Multi-LSP Probing (probePaymentRequest updates):
- Probes up to 3 unique LSPs maximum (griefing protection)
- Selects the WORST-CASE (most expensive) route for conservative
fee estimation
- Adds comprehensive debug logging for worst-case selection process
- Properly formats vertex logging using %v (calls Vertex.String())
The worst-case approach ensures users won't be surprised by higher fees
when the actual payment is sent, providing a more conservative and
reliable fee estimate.
This commit also adds extensive unit test coverage for the LSP detection
heuristic and route preparation logic.
TestIsLsp:
- Edge cases: empty route hints, nil scenarios
- Rule 1: Public invoice target (3 tests)
- Rule 2: All private destination hops (4 tests)
- Rule 3: At least one public destination hop (6 tests)
TestPrepareLspRouteHints:
- LSP grouping and filtering logic
- Worst-case fee selection across route hints
- Worst-case CLTV delta tracking
- Adjusted route hints validation (LSP hop stripped)
- Multi-LSP scenarios with different fees
This commit adds the HasNode function to the RouterBackend struct,
which checks if a node exists in the graph (i.e., has public channels).
This function is needed by the LSP detection heuristic to determine
if a node is publicly reachable.
The function is wired up in rpcserver.go to query the graph database.
Replace hardcoded WithGlobalLock assignment with configurable
options wallet postgres backends. Also add the WithGlobalLock
option to the channeldb table for postgres backends.
Defaults:
- channeldb: false (allow concurrent access)
- wallet: true (maintain safe single-writer behavior)
Users can now override these defaults via:
- db.postgres.channeldb-with-global-lock
- db.postgres.walletdb-with-global-lock
This gives operators flexibility while maintaining safe defaults
until full native SQL migration is complete.
Moreover exclude db.postgres.walletdb-with-global-lock check
in the sample config file script. We cannot easily check the
correct default because we set it later in the LND startup
sequence so we exclude it.
Add two configuration options to control global lock usage for
different postgres database backends:
- ChannelDBWithGlobalLock: for channeldb access (default: false)
- WalletDBWithGlobalLock: for wallet database access (default: true)
These allow fine-grained control over which databases use global
locks, rather than hardcoding the behavior. This is a temporary
measure until the revocation log and wallet are migrated to native
SQL and become fully concurrent-safe.
When there is only one of the tls pairs (key/certificate) and the
other is missing, the TLS manager currently assumes it exists
and ignore generating them. This results in error propgated to user
that the other tls pair file is missing/not found.
Modifiers of the node announcement may add duplicate addresses, which we
remove here after the modifications were applied. This also ensures that
any previously added duplicate addresses are removed as well.
Introduce `CommitTxBlockHeight` field to the `ResolutionReq` structure
and related methods. This field records the block height where a
commitment transaction has confirmed.
ziggie