From bfa97351053a7917e9a5bea751b95ede980e6116 Mon Sep 17 00:00:00 2001 From: Gijs van Dam Date: Wed, 20 Nov 2024 14:58:29 +0100 Subject: [PATCH 1/4] rpc: minrelayfee check in checkFeeRateSanity This commit includes a minrelayfee check in `checkFeeRateSanity` so that we can err fast if a manually provided feerate doesn't meet minrelayfee. --- rpcserver.go | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 11f9effc4..e8dfd27e8 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -664,17 +664,29 @@ func (r *rpcServer) MintAsset(ctx context.Context, // checkFeeRateSanity ensures that the provided fee rate, in sat/kw, is above // the same minimum fee used as a floor in the fee estimator. -func checkFeeRateSanity(rpcFeeRate uint32) (*chainfee.SatPerKWeight, error) { - feeFloor := uint32(chainfee.FeePerKwFloor) +func checkFeeRateSanity(ctx context.Context, rpcFeeRate chainfee.SatPerKWeight, + lndWallet lndclient.WalletKitClient) (*chainfee.SatPerKWeight, error) { + + feeFloor := chainfee.FeePerKwFloor + minRelayFee, err := lndWallet.MinRelayFee(ctx) + if err != nil { + return nil, err + } switch { // No manual fee rate was set, which is the default. - case rpcFeeRate == 0: + case rpcFeeRate == chainfee.SatPerKWeight(0): return nil, nil - // A manual fee was set but is below a reasonable floor. - case rpcFeeRate < feeFloor: - return nil, fmt.Errorf("manual fee rate below floor: "+ - "(fee_rate=%d, floor=%d sat/kw)", rpcFeeRate, feeFloor) + // A manual fee was set but is below a reasonable floor or minRelayFee. + case rpcFeeRate < feeFloor || rpcFeeRate < minRelayFee: + if rpcFeeRate < feeFloor { + return nil, fmt.Errorf("manual fee rate below floor: "+ + "(fee_rate=%s, floor=%s)", rpcFeeRate.String(), + feeFloor.String()) + } + return nil, fmt.Errorf("feerate does not meet minrelayfee: "+ + "(fee_rate=%s, minrelayfee=%s)", rpcFeeRate.String(), + minRelayFee.String()) // Set the fee rate for this transaction. default: @@ -683,10 +695,12 @@ func checkFeeRateSanity(rpcFeeRate uint32) (*chainfee.SatPerKWeight, error) { } // FundBatch attempts to fund the current pending batch. -func (r *rpcServer) FundBatch(_ context.Context, +func (r *rpcServer) FundBatch(ctx context.Context, req *mintrpc.FundBatchRequest) (*mintrpc.FundBatchResponse, error) { - feeRate, err := checkFeeRateSanity(req.FeeRate) + feeRate, err := checkFeeRateSanity( + ctx, chainfee.SatPerKWeight(req.FeeRate), r.cfg.Lnd.WalletKit, + ) if err != nil { return nil, err } @@ -759,11 +773,13 @@ func (r *rpcServer) SealBatch(ctx context.Context, } // FinalizeBatch attempts to finalize the current pending batch. -func (r *rpcServer) FinalizeBatch(_ context.Context, +func (r *rpcServer) FinalizeBatch(ctx context.Context, req *mintrpc.FinalizeBatchRequest) (*mintrpc.FinalizeBatchResponse, error) { - feeRate, err := checkFeeRateSanity(req.FeeRate) + feeRate, err := checkFeeRateSanity( + ctx, chainfee.SatPerKWeight(req.FeeRate), r.cfg.Lnd.WalletKit, + ) if err != nil { return nil, err } @@ -3117,7 +3133,7 @@ func marshalAddrEventStatus(status address.Status) (taprpc.AddrEventStatus, // complete an asset send. The method returns information w.r.t the on chain // send, as well as the proof file information the receiver needs to fully // receive the asset. -func (r *rpcServer) SendAsset(_ context.Context, +func (r *rpcServer) SendAsset(ctx context.Context, req *taprpc.SendAssetRequest) (*taprpc.SendAssetResponse, error) { if len(req.TapAddrs) == 0 { @@ -3164,7 +3180,9 @@ func (r *rpcServer) SendAsset(_ context.Context, } } - feeRate, err := checkFeeRateSanity(req.FeeRate) + feeRate, err := checkFeeRateSanity( + ctx, chainfee.SatPerKWeight(req.FeeRate), r.cfg.Lnd.WalletKit, + ) if err != nil { return nil, err } From ebd15f2cc3129387bcfc4f39dd270365ab12c847 Mon Sep 17 00:00:00 2001 From: Gijs van Dam Date: Thu, 14 Nov 2024 16:56:40 +0100 Subject: [PATCH 2/4] tapgarden: bump feerate to min relay fee If the fee estimation returns a fee rate lower than the min relay fee, we should use the min relay fee instead. This commit implements this behavior for the minting transaction. --- tapgarden/planter.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tapgarden/planter.go b/tapgarden/planter.go index f6638109b..6d785b754 100644 --- a/tapgarden/planter.go +++ b/tapgarden/planter.go @@ -571,6 +571,32 @@ func (c *ChainPlanter) fundGenesisPsbt(ctx context.Context, batchKey[:], feeRate.FeePerKVByte().String()) } + minRelayFee, err := c.cfg.Wallet.MinRelayFee(ctx) + if err != nil { + return nil, fmt.Errorf("unable to obtain minrelayfee: %w", err) + } + + // If the fee rate is below the minimum relay fee, we'll + // bump it up. + if feeRate < minRelayFee { + switch { + // If a fee rate was manually assigned for this batch, we err + // out, otherwise we silently bump the feerate. + case manualFeeRate != nil: + // This case should already have been handled by the + // `checkFeeRateSanity` of `rpcserver.go`. We check here + // again to be safe. + return nil, fmt.Errorf("feerate does not meet "+ + "minrelayfee: (fee_rate=%s, minrelayfee=%s)", + feeRate.String(), minRelayFee.String()) + default: + log.Infof("Bump fee rate for batch %x to meet "+ + "minrelayfee from %s to %s", batchKey[:], + feeRate.String(), minRelayFee.String()) + feeRate = minRelayFee + } + } + fundedGenesisPkt, err := c.cfg.Wallet.FundPsbt( ctx, genesisPkt, 1, feeRate, -1, ) From 81ee9259b9b9f064a1f6e9ab01eb2450f354f752 Mon Sep 17 00:00:00 2001 From: Gijs van Dam Date: Thu, 14 Nov 2024 16:57:02 +0100 Subject: [PATCH 3/4] tapfreighter: bump feerate to min relay fee If the fee estimation returns a fee rate lower than the min relay fee, we should use the min relay fee instead. This commit implements this behavior for the tapfreighter. --- tapfreighter/chain_porter.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 36d8a78f6..3974343ca 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1137,6 +1137,42 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { return nil, fmt.Errorf("unable to estimate "+ "fee: %w", err) } + + log.Infof("estimated fee rate for parcel:, %s", + feeRate.FeePerKVByte().String()) + } + + minRelayFee, err := p.cfg.Wallet.MinRelayFee(ctx) + if err != nil { + p.unlockInputs(ctx, ¤tPkg) + + return nil, fmt.Errorf("unable to obtain "+ + "minimum relay fee: %w", err) + } + + // If the fee rate is below the minimum relay fee, we'll + // bump it up. + if feeRate < minRelayFee { + switch { + // If a fee rate was manually assigned for this parcel, + // we err out, otherwise we silently bump the feerate. + case addrParcel.transferFeeRate != nil: + // This case should already have been handled by + // the `checkFeeRateSanity` of `rpcserver.go`. + // We check here again to be safe. + p.unlockInputs(ctx, ¤tPkg) + + return nil, fmt.Errorf("feerate does not "+ + "meet minrelayfee: (fee_rate=%s, "+ + "minrelayfee=%s)", feeRate.String(), + minRelayFee.String()) + default: + log.Infof("bump fee rate for parcel to meet "+ + "minrelayfee from %s to %s", + feeRate.FeePerKVByte().String(), + minRelayFee.FeePerKVByte().String()) + feeRate = minRelayFee + } } readableFeeRate := feeRate.FeePerKVByte().String() From b9a64f42b47176c35131d513c66e3d766d6caa49 Mon Sep 17 00:00:00 2001 From: Gijs van Dam Date: Thu, 14 Nov 2024 16:57:48 +0100 Subject: [PATCH 4/4] itest: test min relay fee bump The `testMinRelayFeeBump` itest checks that the minting transaction and a basic send obtain a fee bump when the min relay fee is increased to a value that is higher than the fee estimation. fix --- itest/addrs_test.go | 80 ++++++++++++++++++--- itest/send_test.go | 142 +++++++++++++++++++++++++++++++++++++ itest/test_list_on_test.go | 4 ++ itest/utils.go | 46 ++++++++++++ 4 files changed, 261 insertions(+), 11 deletions(-) diff --git a/itest/addrs_test.go b/itest/addrs_test.go index d951bf2f4..ddedb7227 100644 --- a/itest/addrs_test.go +++ b/itest/addrs_test.go @@ -1078,24 +1078,70 @@ func sendProofUniRPC(t *harnessTest, src, dst *tapdHarness, scriptKey []byte, return importResp } -// sendAssetsToAddr spends the given input asset and sends the amount specified +// sendOptions is a struct that holds a SendAssetRequest and an +// optional error string that should be tested against. +type sendOptions struct { + sendAssetRequest taprpc.SendAssetRequest + errText string +} + +// sendOption is a functional option for configuring the sendAssets call. +type sendOption func(*sendOptions) + +// withReceiverAddresses is an option to specify the receiver addresses for the +// send. +func withReceiverAddresses(addrs ...*taprpc.Addr) sendOption { + return func(options *sendOptions) { + encodedAddrs := make([]string, len(addrs)) + for i, addr := range addrs { + encodedAddrs[i] = addr.Encoded + } + options.sendAssetRequest.TapAddrs = encodedAddrs + } +} + +// withFeeRate is an option to specify the fee rate for the send. +func withFeeRate(feeRate uint32) sendOption { + return func(options *sendOptions) { + options.sendAssetRequest.FeeRate = feeRate + } +} + +// withError is an option to specify the string that is expected in the error +// returned by the SendAsset call. +func withError(errorText string) sendOption { + return func(options *sendOptions) { + options.errText = errorText + } +} + +// sendAsset spends the given input asset and sends the amount specified // in the address to the Taproot output derived from the address. -func sendAssetsToAddr(t *harnessTest, sender *tapdHarness, - receiverAddrs ...*taprpc.Addr) (*taprpc.SendAssetResponse, +func sendAsset(t *harnessTest, sender *tapdHarness, + opts ...sendOption) (*taprpc.SendAssetResponse, *EventSubscription[*taprpc.SendEvent]) { ctxb := context.Background() ctxt, cancel := context.WithTimeout(ctxb, defaultWaitTimeout) defer cancel() - require.NotEmpty(t.t, receiverAddrs) - scriptKey := receiverAddrs[0].ScriptKey + // Create base request that will be modified by options. + options := &sendOptions{} - encodedAddrs := make([]string, len(receiverAddrs)) - for i, addr := range receiverAddrs { - encodedAddrs[i] = addr.Encoded + // Apply all the functional options. + for _, opt := range opts { + opt(options) } + require.NotEmpty(t.t, options.sendAssetRequest.TapAddrs) + + // We need the first address's scriptkey to subscribe to events. + firstAddr, err := address.DecodeAddress( + options.sendAssetRequest.TapAddrs[0], &address.RegressionNetTap, + ) + require.NoError(t.t, err) + scriptKey := firstAddr.ScriptKey.SerializeCompressed() + ctxc, streamCancel := context.WithCancel(ctxb) stream, err := sender.SubscribeSendEvents( ctxc, &taprpc.SubscribeSendEventsRequest{ @@ -1108,9 +1154,12 @@ func sendAssetsToAddr(t *harnessTest, sender *tapdHarness, Cancel: streamCancel, } - resp, err := sender.SendAsset(ctxt, &taprpc.SendAssetRequest{ - TapAddrs: encodedAddrs, - }) + resp, err := sender.SendAsset(ctxt, &options.sendAssetRequest) + if options.errText != "" { + require.ErrorContains(t.t, err, options.errText) + return nil, nil + } + require.NoError(t.t, err) // We'll get events up to the point where we broadcast the transaction. @@ -1123,6 +1172,15 @@ func sendAssetsToAddr(t *harnessTest, sender *tapdHarness, return resp, sub } +// sendAssetsToAddr is a variadic wrapper around sendAsset that enables passsing +// a multitude of addresses. +func sendAssetsToAddr(t *harnessTest, sender *tapdHarness, + receiverAddrs ...*taprpc.Addr) (*taprpc.SendAssetResponse, + *EventSubscription[*taprpc.SendEvent]) { + + return sendAsset(t, sender, withReceiverAddresses(receiverAddrs...)) +} + // fundAddressSendPacket asks the wallet to fund a new virtual packet with the // given address as the single receiver. func fundAddressSendPacket(t *harnessTest, tapd *tapdHarness, diff --git a/itest/send_test.go b/itest/send_test.go index 5d255d9bb..40c499447 100644 --- a/itest/send_test.go +++ b/itest/send_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + "github.com/btcsuite/btcd/btcutil" + "github.com/btcsuite/btcd/wire" "github.com/lightninglabs/taproot-assets/internal/test" "github.com/lightninglabs/taproot-assets/proof" "github.com/lightninglabs/taproot-assets/tapfreighter" @@ -18,7 +20,9 @@ import ( "github.com/lightninglabs/taproot-assets/taprpc/tapdevrpc" unirpc "github.com/lightninglabs/taproot-assets/taprpc/universerpc" "github.com/lightninglabs/taproot-assets/tapsend" + "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/lntest/wait" + "github.com/lightningnetwork/lnd/lnwallet/chainfee" "github.com/stretchr/testify/require" ) @@ -140,6 +144,144 @@ func testBasicSendUnidirectional(t *harnessTest) { wg.Wait() } +// testMinRelayFeeBump tests that if the fee estimation is below the min relay +// fee the feerate is bumped to the min relay fee for both the minting +// transaction and a basic asset send. +func testMinRelayFeeBump(t *harnessTest) { + var ctxb = context.Background() + + const numUnits = 10 + + // Subscribe to receive assent send events from primary tapd node. + events := SubscribeSendEvents(t.t, t.tapd) + + // We will mint assets using the first output and then use the second + // output for the transfer. This ensures a valid fee calculation. + initialUTXOs := []*UTXORequest{ + { + Type: lnrpc.AddressType_NESTED_PUBKEY_HASH, + Amount: 1_000_000, + }, + { + Type: lnrpc.AddressType_NESTED_PUBKEY_HASH, + Amount: 999_990, + }, + } + + // Set the initial state of the wallet of the first node. The wallet + // state will reset at the end of this test. + SetNodeUTXOs(t, t.lndHarness.Alice, btcutil.Amount(1), initialUTXOs) + defer ResetNodeWallet(t, t.lndHarness.Alice) + + // Set the min relay fee to a higher value than the fee rate that will + // be returned by the fee estimation. + lowFeeRate := chainfee.SatPerVByte(1).FeePerKWeight() + highMinRelayFeeRate := chainfee.SatPerVByte(2).FeePerKVByte() + defaultMinRelayFeeRate := chainfee.SatPerVByte(1).FeePerKVByte() + defaultFeeRate := chainfee.SatPerKWeight(3125) + t.lndHarness.SetFeeEstimateWithConf(lowFeeRate, 6) + t.lndHarness.SetMinRelayFeerate(highMinRelayFeeRate) + + // Reset all fee rates to their default value at the end of this test. + defer t.lndHarness.SetMinRelayFeerate(defaultMinRelayFeeRate) + defer t.lndHarness.SetFeeEstimateWithConf(defaultFeeRate, 6) + + // First, we'll make a normal assets with enough units to allow us to + // send it around a few times. + MintAssetsConfirmBatch( + t.t, t.lndHarness.Miner().Client, t.tapd, + []*mintrpc.MintAssetRequest{issuableAssets[0]}, + WithFeeRate(uint32(lowFeeRate)), + WithError("manual fee rate below floor"), + ) + + MintAssetsConfirmBatch( + t.t, t.lndHarness.Miner().Client, t.tapd, + []*mintrpc.MintAssetRequest{issuableAssets[0]}, + WithFeeRate(uint32(lowFeeRate)+10), + WithError("feerate does not meet minrelayfee"), + ) + + rpcAssets := MintAssetsConfirmBatch( + t.t, t.lndHarness.Miner().Client, t.tapd, + []*mintrpc.MintAssetRequest{issuableAssets[0]}, + ) + + genInfo := rpcAssets[0].AssetGenesis + + // Check the final fee rate of the mint TX. + rpcMintOutpoint := rpcAssets[0].ChainAnchor.AnchorOutpoint + mintOutpoint, err := wire.NewOutPointFromString(rpcMintOutpoint) + require.NoError(t.t, err) + + // We check whether the minting TX is bumped to the min relay fee. + AssertFeeRate( + t.t, t.lndHarness.Miner().Client, initialUTXOs[0].Amount, + &mintOutpoint.Hash, highMinRelayFeeRate.FeePerKWeight(), + ) + + // Now that we have the asset created, we'll make a new node that'll + // serve as the node which'll receive the assets. The existing tapd + // node will be used to synchronize universe state. + secondTapd := setupTapdHarness( + t.t, t, t.lndHarness.Bob, t.universeServer, + ) + defer func() { + require.NoError(t.t, secondTapd.stop(!*noDelete)) + }() + + // Next, we'll attempt to complete two transfers with distinct + // addresses from our main node to Bob. + currentUnits := issuableAssets[0].Asset.Amount + + // Issue a single address which will be reused for each send. + bobAddr, err := secondTapd.NewAddr(ctxb, &taprpc.NewAddrRequest{ + AssetId: genInfo.AssetId, + Amt: numUnits, + AssetVersion: rpcAssets[0].Version, + }) + require.NoError(t.t, err) + + // Deduct what we sent from the expected current number of + // units. + currentUnits -= numUnits + + AssertAddrCreated(t.t, secondTapd, rpcAssets[0], bobAddr) + + sendAsset( + t, t.tapd, withReceiverAddresses(bobAddr), + withFeeRate(uint32(lowFeeRate)), + withError("manual fee rate below floor"), + ) + + sendAsset( + t, t.tapd, withReceiverAddresses(bobAddr), + withFeeRate(uint32(lowFeeRate)+10), + withError("feerate does not meet minrelayfee"), + ) + + sendResp, sendEvents := sendAssetsToAddr(t, t.tapd, bobAddr) + + ConfirmAndAssertOutboundTransfer( + t.t, t.lndHarness.Miner().Client, t.tapd, sendResp, + genInfo.AssetId, + []uint64{currentUnits, numUnits}, 0, 1, + ) + + sendInputAmt := initialUTXOs[1].Amount + 1000 + AssertTransferFeeRate( + t.t, t.lndHarness.Miner().Client, sendResp, sendInputAmt, + highMinRelayFeeRate.FeePerKWeight(), + ) + + AssertNonInteractiveRecvComplete(t.t, secondTapd, 1) + AssertSendEventsComplete(t.t, bobAddr.ScriptKey, sendEvents) + + // Close event stream. + err = events.CloseSend() + require.NoError(t.t, err) +} + // testRestartReceiver tests that the receiver node's asset balance after a // single asset transfer does not change if the receiver node restarts. // Before the addition of this test, after restarting the receiver node diff --git a/itest/test_list_on_test.go b/itest/test_list_on_test.go index 151dd7695..e32aaeca2 100644 --- a/itest/test_list_on_test.go +++ b/itest/test_list_on_test.go @@ -81,6 +81,10 @@ var testCases = []*testCase{ name: "basic send unidirectional", test: testBasicSendUnidirectional, }, + { + name: "min relay fee bump", + test: testMinRelayFeeBump, + }, { name: "restart receiver check balance", test: testRestartReceiverCheckBalance, diff --git a/itest/utils.go b/itest/utils.go index 6a2ead3f9..761e9ed99 100644 --- a/itest/utils.go +++ b/itest/utils.go @@ -266,6 +266,8 @@ type MintOptions struct { mintingTimeout time.Duration siblingBranch *mintrpc.FinalizeBatchRequest_Branch siblingFullTree *mintrpc.FinalizeBatchRequest_FullTree + feeRate uint32 + errText string } func DefaultMintOptions() *MintOptions { @@ -292,6 +294,20 @@ func WithSiblingTree(tree mintrpc.FinalizeBatchRequest_FullTree) MintOption { } } +func WithFeeRate(feeRate uint32) MintOption { + return func(options *MintOptions) { + options.feeRate = feeRate + } +} + +// WithError is an option to specify the string that is expected in the error +// returned by the FinalizeBatch call. +func WithError(errorText string) MintOption { + return func(options *MintOptions) { + options.errText = errorText + } +} + func BuildMintingBatch(t *testing.T, tapClient TapdClient, assetRequests []*mintrpc.MintAssetRequest, opts ...MintOption) { @@ -334,9 +350,27 @@ func FinalizeBatchUnconfirmed(t *testing.T, minerClient *rpcclient.Client, if options.siblingFullTree != nil { finalizeReq.BatchSibling = options.siblingFullTree } + if options.feeRate > 0 { + finalizeReq.FeeRate = options.feeRate + } // Instruct the daemon to finalize the batch. batchResp, err := tapClient.FinalizeBatch(ctxt, finalizeReq) + + // If we expect an error, check for it and cancel the batch if it's + // found. + if options.errText != "" { + require.ErrorContains(t, err, options.errText) + cancelBatchKey, err := tapClient.CancelBatch( + ctxt, &mintrpc.CancelBatchRequest{}, + ) + require.NoError(t, err) + require.NotEmpty(t, cancelBatchKey.BatchKey) + return chainhash.Hash{}, nil + } + + // If we don't expect an error, we confirm that the batch has been + // broadcast. require.NoError(t, err) require.NotEmpty(t, batchResp.Batch) require.Len(t, batchResp.Batch.Assets, len(assetRequests)) @@ -443,6 +477,11 @@ func MintAssetsConfirmBatch(t *testing.T, minerClient *rpcclient.Client, tapClient TapdClient, assetRequests []*mintrpc.MintAssetRequest, opts ...MintOption) []*taprpc.Asset { + options := DefaultMintOptions() + for _, opt := range opts { + opt(options) + } + ctxc, streamCancel := context.WithCancel(context.Background()) stream, err := tapClient.SubscribeMintEvents( ctxc, &mintrpc.SubscribeMintEventsRequest{}, @@ -457,6 +496,13 @@ func MintAssetsConfirmBatch(t *testing.T, minerClient *rpcclient.Client, t, minerClient, tapClient, assetRequests, opts..., ) + // If we expect an error, we know that the error has successfully + // occurred during MintAssetUnconfirmed so we don't need to confirm the + // batch and can return here. + if options.errText != "" { + return nil + } + return ConfirmBatch( t, minerClient, tapClient, assetRequests, sub, mintTXID, batchKey, opts...,