tapchannel: fix immediate force-close proof sync and sweep anchoring#2133
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of asset channel force-closes, particularly in scenarios where a channel is closed immediately after funding. It introduces mechanisms to re-sync proofs and re-anchor outputs correctly, while also hardening the code against potential panics during anchor resolution. These changes ensure that assets remain recoverable and spendable even under edge-case timing conditions. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for immediate force-closing of asset channels by adding a new test case and updating the auxiliary sweeper and commitment logic to handle the necessary proof metadata and script key synchronization. The review comment regarding the robustness of the confirmation fallback logic in waitForTransferTxConf is valid and provides an actionable improvement, so it has been retained.
| if startHeight == 0 || startHeight > currentHeight { | ||
| startHeight = currentHeight | ||
| } |
There was a problem hiding this comment.
The fallback confirmation logic might not be robust enough if no height hint is provided. If scanHeight is 0, startHeight is set to currentHeight, meaning only the current block is scanned. If the transaction was confirmed in a recent block (e.g., currentHeight - 1) and the primary notification was missed, this fallback will not find it, as scanHeight will be updated to currentHeight + 1 and the missed block will never be scanned.
To make this fallback more reliable, consider scanning a few recent blocks when no height hint is available.
if startHeight == 0 {
const rescanDepth = 6
if currentHeight > rescanDepth {
startHeight = currentHeight - rescanDepth
} else {
startHeight = 1
}
} else if startHeight > currentHeight {
startHeight = currentHeight
}
4899471 to
9093b73
Compare
|
@GeorgeTsagk I had a failure in the CI run of #2130. Claude & Copilot pointed me towards this PR as the likely cause https://github.com/lightninglabs/taproot-assets/actions/runs/26522178163/job/78117493175?pr=2130 EDIT: Since this is not merged it indicates that the failure is due to flakiness, possibly fixed by sergey's PR #2060 |
9093b73 to
eb8e514
Compare
|
@jtobin: review reminder |
bf189cc to
c09446d
Compare
|
|
||
| const ( | ||
| commitSweepWitnessScriptType tlv.Type = 31337 | ||
| commitSweepControlBlockType tlv.Type = 31339 |
There was a problem hiding this comment.
You have some magic numbers here. Want to add comment to explain the reason?
| "funded psbt outputs (len=%d)", | ||
| vOut.AnchorOutputIndex, outputCount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This is good to prevent panic on out-of-range indices
| } | ||
| if matchIdx == -1 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The matching logic here is subtle and challenging to understand. Think it could use some comments explaining why, for example we skip when fallbackCount > 1 rather than erroring.
Should this log a warning when it skips? Silent skips can be hard to debug if a sweep fails
c09446d to
6e66ff2
Compare
| // and then force closed immediately after the funding transaction confirms, | ||
| // without any in-channel asset movement. | ||
| func testCustomChannelsImmediateClose(ctx context.Context, | ||
| net *itest.IntegratedNetworkHarness, t *ccHarnessTest) { |
There was a problem hiding this comment.
Looks good to validate the N=0 force-close path end to end
| totalFee := commitFee | ||
| if lndOpenChan.ChanType.HasAnchors() { | ||
| totalFee += 2 * lnwallet.AnchorSize | ||
| } |
There was a problem hiding this comment.
the totalFee here represents total reserved sats not just miner fees. the calculation is good, is there a more descriptive name for it?
| // transaction in the mempool sweeping commitment outputs. | ||
| // We wait for at least one tx as other background txns can race in. | ||
| charlieJusticeTxid, err := waitForAtLeastNTxsInMempool( | ||
| net.Miner, 1, time.Second*60, |
There was a problem hiding this comment.
Any particular reason to increase to 60s timeout here?
| } | ||
| } | ||
|
|
||
| func syncActiveOutputProofsFromVPackets( |
There was a problem hiding this comment.
the new functions here could use header comments
| // commitment at height 0 — i.e. an immediate post-funding force close, | ||
| // before the channel_ready commit-point rotation. | ||
| func isInitialCommitState(req lnwallet.ResolutionReq) bool { | ||
| return req.CommitHeight.UnwrapOr(1) == 0 |
There was a problem hiding this comment.
The UnwrapOr(1) default means if CommitHeight is not set, we assume it's NOT the initial state with a default value of 1. This is a safe default that should be documented.
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
I don't think you need the special-case reconstruction stuff for commit height 0 anymore.
I think that idea can be summarized as: "the data at commit height 0 is possibly bad, so before doing anything, reconstruct it to ensure it's good." But IIUC the other fixes ensure that the initial commit data is good, so it should be safe to trust.
N.b., the other fixes being:
- lnd should populate negotiated CsvDelay and DustLimit before tapd constructs the initial commitment blob (lightningnetwork/lnd#10804),
- tapd should build initial commitment blobs with actual/nonzero BTC balances (in 5275b38), and
- tapd should walk the commitment tx to figure out where assets actually are/aren't during force close (also in 5275b38).
Is there still anything that the initial commit height special-case machinery accomplishes that those fixes don't?
If not, I vote to cut this one down: include just those latter two tapd fixes, and then add the itest in a separate PR. The tapd-side fixes don't depend on the lnd change, so we can merge/release them without having to wait for an lnd release. Eh, eh?
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
6e66ff2 to
45c60da
Compare
|
Thanks for the review @kaldun-tech & @jtobin . Applied your recommendations and also split the itest/lnd bump part to a separate PR #2177 |
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
I still think this should be stripped down further. 😅
There's still machinery in here to work around the issue that lightningnetwork/lnd#10804 should ultimately fix by populating the initial aux channel state appropriately, but the cure is worse than the disease, IMO.
The additional TLV records, the skipOutputProofVerify bool threaded through everything, the added CommitWitness{Script, ControlBlock} fields -- I don't think any of that will be necessary when the upstream PR lands. IIUC, the initial aux channel state will be populated properly, and then there'll be nothing new on tapd's end to do with it. The porter's normal verification procedure should just work on its own.
If we can safely assume that lnd gives us correct initial negotiated values, then the diff of the purely tapd-side fixes (for init. balances and commit tx asset locations/nonlocations) shrinks to something like (closer to like +120/-50 ish):
https://gist.github.com/jtobin/50132672406173b31ee328205175f673
Essentially I argue that "lnd gives us incorrect negotiated initial values" is an lnd problem, so the fix for it should be in lnd. Fixing it in tapd feels too gnarly.
jtobin ducks
45c60da to
c55c913
Compare
c55c913 to
79cac9f
Compare
fa7f4d8
github-project-automation
Bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
tapchannel: fix immediate force-close proof sync and sweep anchoring
4 participants
Description
This PR fixes immediate post-funding (N=0) force-close recovery by correctly re-syncing active vPacket outputs with re-anchored proofs when anchor indices are stale, and hardens anchor resolution to return explicit errors instead of panicking on out-of-range indices.
It also adds an integration test that force-closes right after funding confirmation and verifies the swept assets remain spendable via an onward transfer to a third node.Depends on lightningnetwork/lnd#10804These last parts of the PR now moved to #2177