From 043d19ae8bbf983e71faae6e45454bb82d4df97e Mon Sep 17 00:00:00 2001 From: ffranr Date: Mon, 3 Feb 2025 13:55:02 +0000 Subject: [PATCH 1/6] tapfreighter: log wallet's minimum relay fee 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/chain_porter.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index e58bfe671..589ce322f 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1178,6 +1178,9 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { "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 { From 2b8c3f1b7645f1fb1a211b043712160d60971546 Mon Sep 17 00:00:00 2001 From: ffranr Date: Mon, 3 Feb 2025 15:33:08 +0000 Subject: [PATCH 2/6] tapfreighter: report final fee rate of anchoring transaction The absolute fee is already reported. This change calculates and reports the final fee rate using the virtual size of the completed transaction. --- tapfreighter/wallet.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tapfreighter/wallet.go b/tapfreighter/wallet.go index baff3a3bb..63f822da4 100644 --- a/tapfreighter/wallet.go +++ b/tapfreighter/wallet.go @@ -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" @@ -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, From a5625187830131bc43bf45ef7deef94f3e77ab5e Mon Sep 17 00:00:00 2001 From: ffranr Date: Mon, 3 Feb 2025 15:57:07 +0000 Subject: [PATCH 3/6] tapfreighter: add SendStateBroadcastComplete state 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. --- itest/assertions.go | 2 +- itest/psbt_test.go | 2 +- tapfreighter/chain_porter.go | 10 ++++++++++ tapfreighter/parcel.go | 9 +++++++++ 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/itest/assertions.go b/itest/assertions.go index 5828c4365..ed5d584fb 100644 --- a/itest/assertions.go +++ b/itest/assertions.go @@ -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, ) } diff --git a/itest/psbt_test.go b/itest/psbt_test.go index 00512b8b4..6104c355f 100644 --- a/itest/psbt_test.go +++ b/itest/psbt_test.go @@ -1388,7 +1388,7 @@ func testPsbtMultiSend(t *harnessTest) { AssertSendEvents( t.t, scriptKey1Bytes, sendEvents, - tapfreighter.SendStateWaitTxConf, + tapfreighter.SendStateBroadcastComplete, tapfreighter.SendStateComplete, ) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 589ce322f..92280c4fa 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1395,6 +1395,16 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { "transaction %v: %w", txHash, err) } + // Set send state to the next state to evaluate. + currentPkg.SendState = SendStateBroadcastComplete + return ¤tPkg, nil + + // At this stage, the transaction has been broadcast to the network. + // From this point forward, the transfer cancellation methodology + // changes. + case SendStateBroadcastComplete: + log.Infof("Transfer tx broadcast complete") + // With the transaction broadcast, we'll deliver a // notification via the transaction broadcast response channel. currentPkg.deliverTxBroadcastResp() diff --git a/tapfreighter/parcel.go b/tapfreighter/parcel.go index efe5f9157..82115fd65 100644 --- a/tapfreighter/parcel.go +++ b/tapfreighter/parcel.go @@ -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 @@ -85,6 +91,9 @@ func (s SendState) String() string { case SendStateBroadcast: return "SendStateBroadcast" + case SendStateBroadcastComplete: + return "SendStateBroadcastComplete" + case SendStateWaitTxConf: return "SendStateWaitTxConf" From 1ff04d3bc500e0dbb168e5d45486a9fb82b7e8ac Mon Sep 17 00:00:00 2001 From: ffranr Date: Mon, 3 Feb 2025 15:57:44 +0000 Subject: [PATCH 4/6] tapfreighter: unlock inputs on err if not in broadcast complete state 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. --- tapfreighter/chain_porter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 92280c4fa..31b88e480 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1487,7 +1487,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 && len(pkg.InputCommitments) > 0 { for prevID := range pkg.InputCommitments { From 2837237b9eb06d85b8a3b32f867047defd185d2f Mon Sep 17 00:00:00 2001 From: ffranr Date: Mon, 3 Feb 2025 15:59:36 +0000 Subject: [PATCH 5/6] tapfreighter: unlock asset coins before err on tx broadcast fail 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. --- tapfreighter/chain_porter.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 31b88e480..38db68986 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1391,6 +1391,18 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { "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, ¤tPkg) + } + + // 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) } From cec657e62d4a4c514ba7ee58dbf98f96bcf6e096 Mon Sep 17 00:00:00 2001 From: ffranr Date: Tue, 4 Feb 2025 19:56:05 +0000 Subject: [PATCH 6/6] WIP --- tapfreighter/chain_porter.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 38db68986..db51bae34 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1491,6 +1491,9 @@ func (p *ChainPorter) unlockInputs(ctx context.Context, pkg *sendPackage) { 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. @@ -1530,6 +1533,9 @@ func (p *ChainPorter) unlockInputs(ctx context.Context, pkg *sendPackage) { 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.