Repository navigation
Conversation
a1b5ec5 to
c74ac6c
Compare
ee1bbba to
5b4bd5b
Compare
5b4bd5b to
c49a43e
Compare
|
@starius: review reminder |
c49a43e to
56816b7
Compare
2f594a8 to
205077e
Compare
|
@starius: review reminder |
ad14247 to
5b76079
Compare
73738a5 to
1384a6e
Compare
GustavoStingelin
left a comment
There was a problem hiding this comment.
Nice work! For me, it looks good to go this way.
|
Low, narrow regression: reconciliation trusts a mapping that can be stale. It decides ownership from deposits.swap_hash. The v0.31.3 migration MigrateDepositSwapHash mapped each reused |
I think this is related to fix #1251 |
|
Thanks, the mixed case is fixed. One variant is still open: if every selected deposit was unconfirmed and gets replaced, nothing proves the swap unfunded, so it stays pending forever. This is a regression against master. Scenario: loop-in S selects only unconfirmed deposit X, or several outputs of one unconfirmed funding tx. The user RBF-bumps the funding tx, keeping the static-address output, and the replacement is mined in the next block, which is the usual result of a fee bump. lnd drops the original funding tx once the replacement is mined, and the server, which re-checks the selected outpoints on every block while waiting on confirmation risk, sends a risk rejection. On the client, Effect: S never reaches A possible proof without new schema: treat an original as conflicted when the stored deposit is still unconfirmed ( |
1384a6e to
a957b74
Compare
Keep pending deposits owned while historical HTLC confirmations recover. Release only the calling swap's claims, after an unspent original or confirmed conflict rules out a confirmed HTLC. Retry temporary wallet failures and finish old swaps after their deposits are reused. Claim ownership before initialization. Before startup releases a stale legacy mapping, recover its unique pending or paid owner and fail closed on ambiguous claims. Preserve current mappings during valid retries. Persist unconfirmed funding parents before signing so confirmed wallet replacements can prove a conflict after restart. Missing history or an absent legacy snapshot never proves the old HTLC cannot confirm. Retain replacement expiry-sweep tracking and regression coverage for crashes, funding replacement, reuse and delayed notifications.
Cover per-deposit address ownership and operation-specific change outputs across the shared SQL persistence boundary. Reconstruct the deposit, loop-in, and withdrawal stores to verify ownership and change metadata survive restart.
The sweep request handler only compared the number of prevouts the server sent against the number of sweep inputs. A list of the right length could still contain duplicate outpoints or reference outpoints the sweep doesn't spend. The prevout fetcher then returns nil for an input and NewTxSigHashes panics on the nil dereference, taking down loopd on a malformed server request. Reject duplicate prevouts while building the prevout map and require a prevout for every sweep input before computing sighashes.
Use each selected deposit's address parameters when checking quote expiry. Also check the actual server HTLC expiry against every deposit's CSV expiry and safety buffer before sharing signatures. Reject unsafe contracts, abandon their server attempt, and cancel their invoice.
Rename the legacy key family and address parameter alias, reconcile root terminology, document script map keys and consolidate wallet UTXO listing. Keep key-family values and underlying script parameters unchanged. Share address decoding and script reconstruction, convert identical SQL rows directly, and use per-deposit expiry for autoloop. Clarify method and test invariants, include scripts in missing-parameter errors, preallocate store results and update CLI and RPC documentation.
Keep address creation separate from wallet funding. Require wallet:fund for the funding RPC. Keep static deposit as a CLI helper that creates an address and prints an lncli sendcoins command for manual funding. Report funding timeouts and cancellations as unknown outcomes, retaining the address and advising callers to inspect wallet transactions first.
Require one legacy address when deposits exist before migration 23 assigns ownership. Use a regular CHECK-constrained guard table on both backends without an additional PostgreSQL TEMPORARY privilege. Cover backfills, ambiguous ownership, restricted roles, rollback and retry. Document recovery only for a verified rolled-back dirty migration 23, retaining the migration framework's existing dirty-version behavior.
Encode each persisted P2TR script using an explicit network in a free helper. Address rendering does not need the manager or reconstruction of the signing keys and script tree, and does not imply an ownership check.
Quote selected deposits with one exact active-set lookup, retaining presence, state, duplicate and expiry checks without rendering history. Refresh deposits before taking the unspent response snapshot and filter against active Deposited records. Historical database rows cannot revive outputs removed during reconciliation or return stale confirmations.
Keep the lowest-ID active address alongside the script index. Publish it only after successful activation and update it under the index mutex. Serve public root lookups from the cache with independent parameter, script and public-key copies. Cover restart recovery, failed imports and caller mutation so caching preserves readiness and value ownership.
Read wallet UTXOs outside the map lock, then match their scripts directly against the active address index under a short lock. This removes the full-index allocation and copy on each poll while preserving confirmation bounds and allowing address activation during the wallet RPC. Test filtering across addresses and verify wallet RPCs do not hold the map lock.
Load persisted addresses in bounded pages using an ascending ID cursor during startup and root recovery. Read wallet watches once and publish the complete runtime index only after every page and import succeeds. The existing bulk-read API remains available for compatibility. Test sparse IDs, page boundaries, failed-page recovery and restart behavior, including the SQL query on SQLite and PostgreSQL.
Replace the issuance mutex with a context-aware gate so queued callers can cancel without disturbing the current issuer. Keep root creation, recovery and derived issuance serialized and retryable. A private mutex now explicitly guards the active script index and root. Test canceled root and derived waiters, subsequent progress and concurrent first callers sharing one root.
Remove the blank line before the closing brace flagged by CI. This is formatting only; test behavior is unchanged.
Compare lnd counters with persisted address and HTLC indices, verify wallet identity and advance only lagging families before activation. Preserve equal or ahead counters without creating addresses. Allow restoration beyond the manager deadline with RPC timeouts, cancellation and progress logs. Finish listener initialization before shutdown teardown to avoid leaking REST or racing wait groups. Cover restoration, interrupted restarts and shutdown order.
A restored wallet may not have derived a static address key. Pubkey lookup then misses and can fall back to locator (0, 0), producing an invalid expiry signature. Pass the deposit locator and omit PubKey so lndclient sends the locator over the wire. Cover root, receive and change deposits.
Document fresh receive-address derivation, lazy seed initialization, funding-address lookup hardening, and the swap:execute permission required by address creation. Regenerate the CLI, gRPC, Swagger, and man-page documentation and add feature, breaking-change, and recovery release notes.
Cost polling can retain a fee share before discovery stores another static deposit from the same transaction. Different scripts caused the first deposit to receive the entire fee permanently. Recognize every owned static output through the active address index before allocating the fee. Preserve value-based shares, remainder placement, and existing change and unknown-fee behavior.
Recognize legacy root change alongside derived change and preserve accounting when both outputs pay the root. Reuse the persisted change address across fee bumps and restarts while retaining old transactions and creating fresh signing sessions. Compute withdrawal values once. Cover legacy and derived change, root destinations, repeated fee bumps, full withdrawals and unrelated outputs.
a957b74 to
8400ee7
Compare
|
Addressed the two reports in the stale-owner comment and the all-unconfirmed funding replacement comment.
I kept missing wallet history conservative. The proposed absence proof is not sufficient: btcwallet can remove unmined transactions after failed rebroadcasts, and WalletKit.RemoveTransaction can delete an unconfirmed transaction and its descendants without proving a mined conflict. Tests ensure missing history, missing legacy snapshots, mempool/shallow conflicts, changed/unsynced tips, and the original funding transaction do not justify the new conflict proof. Older swaps without a retained snapshot, replacements not visible to lnd, or replacement of an uncaptured ancestor remain monitored. Those cases are not claimed as automatically repaired. Both changes are folded into the existing lifecycle recovery commit, with the PR at Nautilus #1660 is pinned to this version. The new funding-replacement regression fails at the expected stuck-swap assertion with the previous Loop pin and passes with this fix. Withdrawal RBF and fractional multi-address loop-in with change also pass locally, along with Nautilus's static-address race suite and lint. Both PRs are pushed for CI. |
| @@ -0,0 +1,21 @@ | |||
| -- Existing deposits must belong to one unambiguous legacy address. A guard | |||
There was a problem hiding this comment.
Now there is migration 23 in the master. We need to rename migrations from this PR from (23,24,25) to (24,25,26).
| func (m *Manager) withdrawalChangeAddress(ctx context.Context, | ||
| deposits []*deposit.Deposit) (*address.AddressParameters, error) { | ||
|
|
||
| d := deposits[0] |
There was a problem hiding this comment.
Must fix: an open-channel request with no deposits crashes loopd. deposits[0] here panics on an empty slice, and loopd has no panic recovery.
- How it happens:
- An authenticated
StaticOpenChannelcall setsFundMax=truewhile no confirmed deposits exist, so the selection is empty (openchannel/manager.go#L298-L327). - It passes an absurd
SatPerVbyte, for example18446744073709550616.SatPerVbyte * 1000wraps to a negative fee rate (#L265-L267), soCalculateWithdrawalTxValuesreturns positive funding and change amounts with no inputs. - Once a peer accepts the channel, the
PsbtFundstep callsCreateFinalizedWithdrawalTxwith the empty slice (#L654), and the positive change reaches this helper.
- An authenticated
- Result: an index-out-of-range panic in the gRPC handler goroutine, and loopd exits.
- Master: has the same overflow and the same empty fundmax path, but doesn't crash. The index came with this helper.
- Fix: reject an empty deposit list in
CreateFinalizedWithdrawalTx(or here), reject an empty fundmax selection inOpenChannel, and boundSatPerVbytebefore multiplying.
| } | ||
| candidates := claims[d.OutPoint] | ||
| if len(candidates) > 1 { | ||
| return fmt.Errorf("ambiguous live owners for "+ |
There was a problem hiding this comment.
Must fix: this error can stop loopd from ever starting again, in a normal database.
- How it happens:
- Swaps B1 and B2 each use deposit D, miss their payment deadline and release D. Both stay pending until their HTLC timeout, and both still list D in
DepositOutpoints. - A newer swap A' uses D (
CreateLoopInmaps D to A') and then fails. - loopd stops while a fourth swap is starting, after D was locked but before
CreateLoopIn. D staysLoopingInand still maps to A', becauseUpdateDepositnever writesswap_hash.
- Swaps B1 and B2 each use deposit D, miss their payment deadline and release D. Both stay pending until their HTLC timeout, and both still list D in
- Result: on restart A' is
Failed, so D counts as at risk, and both B1 and B2 match as live claimants. This returns an error,recoverLoopInsfails, and loopd gives up with "static address loop-in manager not ready". B1 and B2 can't progress while loopd is down, so every restart fails the same way. - Before: 1384a6e released D, and master swept it at expiry.
- Fix: rank claimants by swap id and only consider those newer than the mapped owner. In a normal DB there are none, because every
CreateLoopInremaps D. Never fail startup here: if ownership is still unclear, log it and leave D alone.
| event, target = deposit.OnLoopedIn, deposit.LoopedIn | ||
| default: | ||
| if slices.Contains(PendingStates, ownerState) { | ||
| if err := m.cfg.DepositManager.RestoreLoopInClaim( |
There was a problem hiding this comment.
Should fix: a restored owner never releases the deposit.
- How it happens:
- Reconciliation picks a live claimant B for deposit D and calls
RestoreLoopInClaim, which only sets B as D's in-memory owner. - B's recovered state machine gets its deposits from
DepositsForSwapHash(B). That skips D, because D's storedswap_hashstill points to the old swap.activeDepositsForLoopInonly falls back toDepositOutpointswhen B has no mapped deposits left. - So for any multi-deposit B, B's release and finalize never touch D.
- Reconciliation picks a live claimant B for deposit D and calls
- Result: D stays
LoopingIn, with its expiry sweep held back, until B reaches a final state and loopd restarts again, which can be up to about 1,500 blocks. 1384a6e released D right away in the normal case. - Fix: with the id ranking suggested at L846, restoring is only needed in the legacy case. When it is, also add D to the owner's recovered deposits, or rewrite the stored mapping.
| } | ||
| return fsm.NoOp, false | ||
| } | ||
| if !canReleaseDeposits("risk rejection") { |
There was a problem hiding this comment.
Should fix: a risk-rejected RBF swap now waits about a week to fail.
- How it happens:
- Every selected deposit is unconfirmed, and its funding tx is RBF-replaced. The server re-checks the outpoints on each block and sends a risk rejection right after the replacement is mined.
- At that point the replacement has 1 confirmation and the conflict proof needs 3, so
canReleaseDepositsis false and this returnsNoOp. - Two blocks later the per-block check (#L1387) passes and releases the deposits, but the swap keeps monitoring until the HTLC timeout (#L1406-L1440).
- Result: the swap stays pending for about 1,000 blocks after its deposits are free. Autoloop counts it as in flight (default limit 1) and keeps the peer marked busy. Master failed it immediately on the rejection.
- Fix: once a rejection is recorded, end the swap (
HandleError) as soon as the release check passes, instead of waiting for the timeout.
This is PR 1 of 3 in the Static Address multi-address stack.
Static Address previously treated one legacy/root address as the owner of every
deposit. This PR introduces fresh receive and operation-specific change
addresses while preserving the address parameters that own each deposit.
Loop-ins and withdrawals can consequently spend deposits received across
multiple derived addresses, with each input signed and proven using its actual
address parameters.
Key Changes
across restarts.
address.
loop-ins and partial withdrawals.
errors for the expected Taproot output key.
of reconstructing and scanning every persisted address.
paying the same destination script cannot finalize a deposit.
loop static depositas a helper that creates a fresh address andprints an
lncli sendcoinscommand for manual funding.RPC and Compatibility Changes
NewStaticAddressnow derives a fresh receive address on every request andnever funds it. Callers must not assume repeated requests are idempotent or
return the same address.
Wallet funding is handled by the separate
FundStaticAddressRPC, exposed asPOST /v1/staticaddr/fund. It creates and funds a fresh address whensend_coins_request.addris empty, or funds a known existing static addresswhen it is set. The CLI does not call this funding endpoint:
loop static depositcallsNewStaticAddressand prints anlncli sendcoinscommandinstead of broadcasting a transaction.
loop static newkeeps its address-onlyoutput.
Funding requires
wallet:fundin addition toswap:executeandloop:in,or an explicit URI grant for
FundStaticAddress. Existing execute/in andNewStaticAddressURI macaroons cannot authorize funding. The default localLoop macaroon is regenerated on startup when permissions change; distributed
or custom funding macaroons must be explicitly updated.
Because address creation mutates wallet and database state, the RPC permission
changes from
swap:readtoswap:execute. Operators using custom scopedmacaroons must rebake them accordingly.
StaticAddressSummaryResponse.static_addressremains populated with thelegacy/root address for wire compatibility, but is deprecated and must not be
treated as the current receive address. Call
NewStaticAddressto derive afresh address.
Database and Recovery
The database now records:
Migrations backfill existing records using the legacy/root address. Store
reconstruction restores deposit ownership and loop-in and withdrawal change
metadata after restart.
Stacked PRs
validates and follows the transactions that actually replace multi-address
withdrawals.
persists the confirmed HTLC output so recovery can rebuild the exact
server-published transaction.
The address startup and lookup hardening previously isolated in
#1214 has been folded into
this PR, and #1214 is now closed.
Testing
Multi-address integration tests are in
lightninglabs/nautilus#1660,
currently at
52975e8f.Its Loop client,
looprpc, andswapserverrpcmodules all pin this PR'scurrent head
8400ee7f.Integration CI run.
go test ./...go vet ./...go test -race ./staticaddr/address ./loopdmake docs-checkgit diff --checkRelease Notes
Release notes document the new address behavior, RPC permission change,
deprecated summary field, migration compatibility, and address lookup
hardening.