Skip to content

Unlock Coins on Anchor TX Broadcast Failure #1348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Unlock Coins on Anchor TX Broadcast Failure #1348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
043d19a
tapfreighter: log wallet's minimum relay fee
ffranr Feb 3, 2025
2b8c3f1
tapfreighter: report final fee rate of anchoring transaction
ffranr Feb 3, 2025
a562518
tapfreighter: add SendStateBroadcastComplete state
ffranr Feb 3, 2025
1ff04d3
tapfreighter: unlock inputs on err if not in broadcast complete state
ffranr Feb 3, 2025
2837237
tapfreighter: unlock asset coins before err on tx broadcast fail
ffranr Feb 3, 2025
cec657e
WIP
ffranr Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion itest/assertions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ func AssertSendEventsComplete(t *testing.T, scriptKey []byte,
stream *EventSubscription[*taprpc.SendEvent]) {

AssertSendEvents(
t, scriptKey, stream, tapfreighter.SendStateWaitTxConf,
t, scriptKey, stream, tapfreighter.SendStateBroadcastComplete,
tapfreighter.SendStateComplete,
)
}
Expand Down
2 changes: 1 addition & 1 deletion itest/psbt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,7 +1388,7 @@ func testPsbtMultiSend(t *harnessTest) {

AssertSendEvents(
t.t, scriptKey1Bytes, sendEvents,
tapfreighter.SendStateWaitTxConf,
tapfreighter.SendStateBroadcastComplete,
tapfreighter.SendStateComplete,
)

Expand Down
33 changes: 32 additions & 1 deletion tapfreighter/chain_porter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,9 @@
"minimum relay fee: %w", err)
}

log.Infof("Min relay fee: %v",
minRelayFee.FeePerKVByte().String())

// If the fee rate is below the minimum relay fee, we'll
// bump it up.
if feeRate < minRelayFee {
Expand Down Expand Up @@ -1388,10 +1391,32 @@
"transaction %v: %w", txHash, err)

case err != nil:
// If the error is due to the min relay fee not being
// met, we'll unlock the inputs we locked for this
// transfer before returning the error.
if strings.Contains(
err.Error(), "min relay fee not met",
) {
p.unlockInputs(ctx, &currentPkg)

Check failure on line 1400 in tapfreighter/chain_porter.go

View workflow job for this annotation

GitHub Actions / Lint check 

multi-line statement should be followed by a newline (whitespace)
}

// We exercise caution by not unlocking the inputs in
// the general error case, in case the transaction was
// somehow broadcasted.
return nil, fmt.Errorf("unable to broadcast "+
"transaction %v: %w", txHash, err)
}

// Set send state to the next state to evaluate.
currentPkg.SendState = SendStateBroadcastComplete
return &currentPkg, nil

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really elegant, adding this extra state here.

log.Infof("Transfer tx broadcast complete")

// With the transaction broadcast, we'll deliver a
// notification via the transaction broadcast response channel.
currentPkg.deliverTxBroadcastResp()
Expand Down Expand Up @@ -1466,6 +1491,9 @@
return
}

// TODO(ffranr): Make use of CheckMempoolAccept to ensure we don't
// unlock inputs for transactions that are in the mempool.

// If we haven't even attempted to broadcast yet, we're still in a state
// where we give feedback to the user synchronously, as we haven't
// created an on-chain transaction that we need to await confirmation.
Expand All @@ -1474,7 +1502,7 @@
// 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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

len(pkg.InputCommitments) > 0 {

for prevID := range pkg.InputCommitments {
Expand Down Expand Up @@ -1505,6 +1533,9 @@
log.Warnf("Unable to unlock input %v: %v", op, err)
}
}

// TODO(ffranr): Remove pending asset transfer and chain tx from the
// database.
}

// logPacket logs the virtual packet to the debug log.
Expand Down
9 changes: 9 additions & 0 deletions tapfreighter/parcel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ const (
// ensure it properly tracks the coins allocated to the anchor output.
SendStateBroadcast

// SendStateBroadcastComplete represents the state where the transfer
// transaction has been broadcast and is either in the mempool or
// confirmed on-chain. At this stage, cancellation cannot rely solely
// on naive coin unlocking.
SendStateBroadcastComplete

// SendStateWaitTxConf is a state in which we will wait for the transfer
// transaction to confirm on-chain.
SendStateWaitTxConf
Expand Down Expand Up @@ -85,6 +91,9 @@ func (s SendState) String() string {
case SendStateBroadcast:
return "SendStateBroadcast"

case SendStateBroadcastComplete:
return "SendStateBroadcastComplete"

case SendStateWaitTxConf:
return "SendStateWaitTxConf"

Expand Down
13 changes: 12 additions & 1 deletion tapfreighter/wallet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/btcsuite/btcd/btcec/v2/schnorr"
"github.com/btcsuite/btcd/btcutil"
"github.com/btcsuite/btcd/btcutil/psbt"
"github.com/btcsuite/btcd/mempool"
"github.com/btcsuite/btcd/txscript"
"github.com/btcsuite/btcd/wire"
"github.com/davecgh/go-spew/spew"
Expand Down Expand Up @@ -1438,11 +1439,21 @@ func (f *AssetWallet) AnchorVirtualTransactions(ctx context.Context,
}

// Final TX sanity check.
err = blockchain.CheckTransactionSanity(btcutil.NewTx(finalTx))
finalBtcUtilTx := btcutil.NewTx(finalTx)
err = blockchain.CheckTransactionSanity(finalBtcUtilTx)
if err != nil {
return nil, fmt.Errorf("anchor TX failed final checks: %w", err)
}

// Report the actual fee rate that will be paid.
//
// Compute the virtual size of the transaction in bytes.
size := mempool.GetTxVirtualSize(finalBtcUtilTx)

// Compute the fee rate in sat/kvb.
actualFeeRate := int64(chainFees) * 1000 / size
log.Infof("Anchor TX final fee rate: %d sat/kvb", actualFeeRate)

anchorTx := &tapsend.AnchorTransaction{
FundedPsbt: anchorPkt,
FinalTx: finalTx,
Expand Down
Loading