Unlock Coins on Anchor TX Broadcast Failure #1348

ffranr · 2025-02-03T16:13:05Z

This PR ensures that asset coins are properly unlocked if an error occurs during the anchor transaction broadcast process. Previously, failed broadcasts could leave coins locked, causing send attempts to be stuck.

Key changes:

Unlock asset coins before returning an error when publishing an anchor transaction.
Ensure inputs are unlocked if an error occurs before reaching the broadcast complete state.
Introduce SendStateBroadcastComplete to better define when a transfer can be canceled.
Improve logging by reporting the final fee rate of the anchoring transaction and logging the wallet's minimum relay fee for better debugging.

coveralls · 2025-02-03T16:29:39Z

Pull Request Test Coverage Report for Build 13379509732

Details

0 of 28 (0.0%) changed or added relevant lines in 4 files are covered.
32 unchanged lines in 6 files lost coverage.
Overall coverage remained the same at 41.014%

Changes Missing Coverage	Changed/Added Lines	%
itest/assertions.go	1	0.0%
tapfreighter/parcel.go	2	0.0%
tapfreighter/wallet.go	8	0.0%
tapfreighter/chain_porter.go	17	0.0%

Files with Coverage Reduction	New Missed Lines	%
tapgarden/planter.go	2	64.98%
tappsbt/create.go	2	53.22%
tapfreighter/chain_porter.go	4	0.0%
tapgarden/caretaker.go	4	68.11%
commitment/tap.go	5	83.64%
universe/interface.go	15	50.65%

Totals
Change from base Build 13373496688:	0.0%
Covered Lines:	27339
Relevant Lines:	66657

💛 - Coveralls

gijswijs

I think we need to rethink this at a few levels. I think this might be a good time to introduce a failing state that properly handles releasing coins, releasing acnhor tx inputs and cleans up any lingering parcels. I've made some comments throughout where I see issues with this.

There are some points where I think the specific error message we get back from WalletKit informs us on how to proceed. I've mentioned this throughout as well.

Thirdly, we could also use a RPC endpoint that allows you to cancel stuck parcels. But mayby that's something for another PR.

gijswijs · 2025-02-03T16:27:14Z

tapfreighter/chain_porter.go

+	// At this stage, the transaction has been broadcast to the network.
+	// From this point forward, the transfer cancellation methodology
+	// changes.
+	case SendStateBroadcastComplete:


This is really elegant, adding this extra state here.

gijswijs · 2025-02-04T14:39:10Z

tapfreighter/chain_porter.go

@@ -1388,10 +1391,24 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) {
 				"transaction %v: %w", txHash, err)

 		case err != nil:
+			// Unlock the inputs we locked for this transfer before
+			// returning the error.
+			p.unlockInputs(ctx, &currentPkg)


Are we sure that every error we get back from PublishTransaction warrants calling unlockInputs. I'm not 100% sure that we couldn't get into a state where we do have a tx in the mempool, but still unlock the inputs.

We could explicitly check whether the error message contains "min relay fee not met" to specifically address this coin-locking issue when the wallet is bitcoind.

I agree that unlocking for a general error might be too risky. In a future update, we could consider checking the mempool from unlockInputs.

gijswijs · 2025-02-04T16:29:33Z

tapfreighter/chain_porter.go

@@ -1474,7 +1491,7 @@ func (p *ChainPorter) unlockInputs(ctx context.Context, pkg *sendPackage) {
 	// sanity-check that we have known input commitments to unlock, since
 	// that might not always be the case (for example if another party
 	// contributes inputs).
-	if pkg.SendState < SendStateStorePreBroadcast &&
+	if pkg.SendState < SendStateBroadcastComplete &&


If we remove the lease on assets when we are in state SendStateStorePreBroadcast or SendStateBroadcast, we don't clean up the parcel. Which seems wrong to me. This wasn't fully the case in the old situation because we only released coins when the parcel wasn't persisted to disk. So at least upon restart, the parcel was lost, and the assets would truly be free again. In this PR, we get situations were we remove the lease on coins, but still have parcels persisted to disk, that are loaded after a restart.

A similar problem occurs a few lines down (and wasn't introduced by this PR, but should still be handled)

taproot-assets/tapfreighter/chain_porter.go

Line 1520 in e0c9661

err := p.cfg.Wallet.UnlockInput(ctx, op)

In states SendStateStorePreBroadcast and SendStateBroadcast (which are the final two states that call into unlockInputs) we try to release the inputs of the anchor tx but we this leaves us with a situation where we still have the parcel. This PR changes the behavior and at least releases the assets, but we are still stuck with a pending parcel.

Also, we should inform the user what to do if p.cfg.Wallet.UnlockInput fails, because now he has a potential asset burning tx in his lnd node that we couldn't cancel.

I agree that we should cleanup the pending parcel as well.

Also, we should inform the user what to do if p.cfg.Wallet.UnlockInput fails, because now he has a potential asset burning tx in his lnd node that we couldn't cancel.

I don't think we currently call unlockInputs in state broadcast complete or beyond. So LND won't have a burning tx.

Perhaps, in the worst case, if we don't cancel the pending parcel then upon resumption, the asset coins would have been spent and the transaction would fail because the UTXO would have been spent.

Roasbeef · 2025-02-04T19:45:33Z

I think before broadcast, we can call into CheckMempoolAccept, then only broadcast if that returns that it's likely to be accepted.

Here's an example in lnd: https://github.com/lightningnetwork/lnd/blob/6bf895aeb929fb64265bfb226f796392921d8d3a/lnwallet/btcwallet/btcwallet.go#L1928-L1957

A user encountered an error where the anchor transaction fee was reported to be lower than the wallet's minimum relay fee. To provide more context, log the minimum relay fee as reported by the wallet.

The absolute fee is already reported. This change calculates and reports the final fee rate using the virtual size of the completed transaction.

Introduce a new send state to act as a threshold for transfer cancellation handling. After this state, the anchor transaction may already be in the mempool or confirmed on-chain. Before reaching this point, however, the broadcast may have failed, requiring different handling.

Update unlock logic to ensure that errors encountered during the anchor tx broadcast process, but before reaching the broadcast complete state, result in coins being unlocked. This prevents assets from remaining locked in case of intermediate failures.

Ensure locked asset coins are released before returning an error when attempting to publish the anchor transaction. Without this change, if the chain bridge logic determines the anchor transaction should not be broadcast, the coins remain locked, leaving the send attempt stuck in limbo.

lightninglabs-deploy · 2025-02-18T21:17:37Z

@ffranr, remember to re-request review from reviewers when ready

ffranr added bug Something isn't working transfers sending fees logging labels Feb 3, 2025

ffranr requested review from gijswijs and GeorgeTsagk February 3, 2025 16:13

ffranr self-assigned this Feb 3, 2025

ffranr force-pushed the unlock-coins-on-broadcast-fail branch 2 times, most recently from 3d7d78e to 8dba8e6 Compare February 3, 2025 16:27

ffranr force-pushed the unlock-coins-on-broadcast-fail branch from 8dba8e6 to e0c9661 Compare February 3, 2025 16:35

ffranr mentioned this pull request Feb 3, 2025

[bug]: On signet, min-relay-fee is not met if fee is not specified. Fee will be 1 sat short #1171

Closed

gijswijs requested changes Feb 4, 2025

View reviewed changes

ffranr force-pushed the unlock-coins-on-broadcast-fail branch from e0c9661 to 19edeee Compare February 4, 2025 19:57

ffranr removed the request for review from GeorgeTsagk February 5, 2025 13:39

ffranr added 6 commits February 17, 2025 21:03

tapfreighter: log wallet's minimum relay fee

043d19a

A user encountered an error where the anchor transaction fee was reported to be lower than the wallet's minimum relay fee. To provide more context, log the minimum relay fee as reported by the wallet.

tapfreighter: report final fee rate of anchoring transaction

2b8c3f1

The absolute fee is already reported. This change calculates and reports the final fee rate using the virtual size of the completed transaction.

WIP

cec657e

ffranr force-pushed the unlock-coins-on-broadcast-fail branch from 19edeee to cec657e Compare February 17, 2025 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unlock Coins on Anchor TX Broadcast Failure #1348

Unlock Coins on Anchor TX Broadcast Failure #1348

ffranr commented Feb 3, 2025

coveralls commented Feb 3, 2025 •

edited

Loading

gijswijs left a comment

gijswijs Feb 3, 2025

gijswijs Feb 4, 2025

ffranr Feb 4, 2025

gijswijs Feb 4, 2025

gijswijs Feb 4, 2025

ffranr Feb 4, 2025

ffranr Feb 4, 2025

Roasbeef commented Feb 4, 2025

lightninglabs-deploy commented Feb 18, 2025

Unlock Coins on Anchor TX Broadcast Failure #1348

Are you sure you want to change the base?

Unlock Coins on Anchor TX Broadcast Failure #1348

Conversation

ffranr commented Feb 3, 2025

coveralls commented Feb 3, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13379509732

Details

💛 - Coveralls

gijswijs left a comment

Choose a reason for hiding this comment

gijswijs Feb 3, 2025

Choose a reason for hiding this comment

gijswijs Feb 4, 2025

Choose a reason for hiding this comment

ffranr Feb 4, 2025

Choose a reason for hiding this comment

gijswijs Feb 4, 2025

Choose a reason for hiding this comment

gijswijs Feb 4, 2025

Choose a reason for hiding this comment

ffranr Feb 4, 2025

Choose a reason for hiding this comment

ffranr Feb 4, 2025

Choose a reason for hiding this comment

Roasbeef commented Feb 4, 2025

lightninglabs-deploy commented Feb 18, 2025

coveralls commented Feb 3, 2025 •

edited

Loading