multi: anchor fee test coverage #605

jharveyb · 2023-10-19T23:22:25Z

Fixes #671

Verifies fixes for various issues with fee estimation.

There are also some issues with cleaning up caretaker state if the caretaker returns an error, which is not recoverable from inside the caretaker.

Batch finalization could be manually reattempted by the user.

Also added extra logging of fee rates and transaction weights.

Should also change units exposed to the user.

In line with our other software, this flag could be called --sat_per_vbyte and be measured in sat/vB. We use the term fee_rate a bit differently in LND.

jharveyb · 2023-10-25T17:17:58Z

Will split out the caretaker shutdown issue discovered here into a separate PR. I think we can move testing for that fix into the existing unit tests for the planter, where we trigger the error with a different mock wallet anchor that always fails.

jharveyb · 2023-11-22T18:42:18Z

Split caretaker fixes into #693 , the fee estimation itest here depends on fixes added there.

jharveyb · 2023-12-06T20:05:34Z

I think I've narrowed this down to possible issues with inputs that are older types like P2PKH.

In the minter we don't adjust the fee, but we still record it. In the freighter, we use a weight estimator unique to this repo to calculate TX weight, and then adjust the change output.

We could instead reuse weight & fee logic that IIUC is already being used as part of FundPsbt():

https://github.com/btcsuite/btcwallet/blob/5df09dd4335865dde2a9a6d94a26a7f5779af825/wallet/txsizes/size.go#L203

Most of this logic is already publicly exposed by these packages. We could also then unify fee estimation across the minter and freighter.

I think the old user issues about bad fee estimates on testnet during minting were just due to bad fee estimates.

jharveyb · 2023-12-06T20:10:31Z

Acceptance Criteria:

new weight and fee estimator that reuses code from btcwallet
itest that exercises the new manual feerate sanity-checking logic in the relevant RPC calls
update RPC calls to use sat/vB
itest that exercises correct feerates on minting and transfer when backing wallet only has old (NP2WKH, P2WKH) input types

dstadulis · 2023-12-11T16:51:27Z

There's a round issue that has caused the tests to fail. @jharveyb has identified root cause and addressing it.

jharveyb · 2023-12-11T17:00:32Z

Specifically this unit conversion from sat/kw to sat/vB when constructing the FundPsbt call:

taproot-assets/wallet_anchor.go

Line 61 in 564795a

SatPerVbyte: uint64(feeRate.FeePerKVByte()) / 1000,

Where e.x. a feerate of 3125 sat/kw or 12.5 sat/vB is rounded down to 12 sat/vB.

jharveyb · 2023-12-12T19:18:46Z

Pushed up my latest state; discovered a separate issue about dust change handling in the freighter.

jharveyb · 2023-12-14T16:47:25Z

Made tracking issue for dusty change: #738. Going to defer that for a separate PR.

jharveyb · 2023-12-18T17:15:24Z

From offline discussion: the standard for fee unit handling is to use sat/kW on the RPC interface, but expose sat/vB as the unit on the CLI. We then convert from sat/vB to sat/kW before making our RPC call.

guggero

Very nice improvements and test additions!
Needs one change for the RPC to remain at sat/kw, the rest looks good 💯

tapgarden/caretaker.go

tapfreighter/wallet.go

itest/assertions.go

itest/utils.go

rpcserver.go

taprpc/taprootassets.proto

cmd/tapcli/assets.go

ffranr

Great itest and clear changes. Nice work.

I can only think of small things that might need addressing. My second review should be quick.

tapgarden/caretaker.go

tapgarden/mock.go

tapfreighter/wallet.go

rpcserver.go

itest/fee_estimation_test.go

itest/utils.go

ffranr · 2023-12-19T15:23:04Z

itest/utils.go

+
+// ResetNodeWallet sets the wallet state of the given node to own 100 P2TR UTXOs
+// of BTC, which matches the wallet state when initializing the itest harness.
+func ResetNodeWallet(t *harnessTest, wallet *node.HarnessNode) {


Maybe we should call this function when initializing the itest harness to be certain that it's the default state.

jharveyb · 2023-12-20T19:47:08Z

#605 (comment)

(Can't reply to comment directly) - the rounding up here is a difference of 3 sat/kw or 12 sat/kvB; so extremely small, even for a hypothetical large transfer TX.

This is also what we do in Pool:

https://github.com/lightninglabs/pool/blob/59c7c2b40009503851de6da3b3f9b4d90b34e208/cmd/pool/account.go#L172

Without the rounding, the TX would get rejected later on by LND before broadcast anyways, so the alternative (no rounding) is not allowing the user to specify a fee below 2 sat/vB.

jharveyb · 2023-12-21T01:37:12Z

Updated, would like to resolve the detail on possible inputs for transfers that don't have WitnessUtxo set.

jharveyb · 2023-12-21T20:33:02Z

Made final updates re: non-witness inputs, ready for final approval.

guggero

Nice, this is very close! CLI needs to be updated since the parseFeeRate() is used for other commands too. Other than that this looks good.

tapgarden/mock.go

tapfreighter/wallet.go

cmd/tapcli/assets.go

lightninglabs-deploy · 2023-12-29T18:23:57Z

@guggero: review reminder
@ffranr: review reminder

ffranr

🎆

tapfreighter/wallet.go

In this commit, we add extra logging of fee rates to the minter, and also remove a reimplementation of psbt.GetTxFee(). We also deduplicate some dummy PkScript generation.

In this commit, we remove the amount of tap-specific code handling fee estimation by replacing EstimateWeight() with EstimateFee(). This is a thin wrapper around txsizes.EstimateVirttualSize(), and is very similar to the implementation in txauthor/NewUnsignedTransaction.

In this commit, we add a new itest that exercises the minter and freighter when using multiple input types like P2WKH and various fee rates.

dstadulis added this to the v0.4 milestone Nov 14, 2023

jharveyb mentioned this pull request Nov 14, 2023

[feature]: further test coverage for manual fee rates and fee rate estimation #671

Closed

5 tasks

jharveyb force-pushed the fee_estimation_testing branch from 4b0bcd5 to a8c6988 Compare November 14, 2023 21:17

jharveyb force-pushed the fee_estimation_testing branch from a8c6988 to 93fb244 Compare November 22, 2023 18:41

jharveyb changed the base branch from main to caretaker_stop_fixes November 22, 2023 18:41

jharveyb force-pushed the fee_estimation_testing branch from 93fb244 to 1a530c0 Compare November 22, 2023 18:48

jharveyb force-pushed the caretaker_stop_fixes branch from 5ae0a56 to 83d5141 Compare November 22, 2023 18:48

jharveyb force-pushed the caretaker_stop_fixes branch 4 times, most recently from 93ca166 to c19f2d6 Compare December 4, 2023 21:30

dstadulis assigned jharveyb Dec 5, 2023

jharveyb force-pushed the fee_estimation_testing branch from 1a530c0 to 372e47e Compare December 6, 2023 18:37

jharveyb force-pushed the caretaker_stop_fixes branch from c19f2d6 to 2b6900c Compare December 6, 2023 18:37

jharveyb force-pushed the fee_estimation_testing branch from 372e47e to 6af7724 Compare December 6, 2023 18:39

jharveyb force-pushed the caretaker_stop_fixes branch from 2b6900c to 05b82da Compare December 8, 2023 19:15

jharveyb force-pushed the fee_estimation_testing branch from 6af7724 to 935db82 Compare December 12, 2023 19:14

jharveyb changed the base branch from caretaker_stop_fixes to main December 14, 2023 00:57

jharveyb mentioned this pull request Dec 14, 2023

[bug]: Handle possibility of transfer change outputs being below dust threshold #738

Closed

4 tasks

jharveyb force-pushed the fee_estimation_testing branch from 935db82 to b646d2f Compare December 14, 2023 01:38

jharveyb force-pushed the fee_estimation_testing branch 2 times, most recently from 5f4543e to 9b7d58d Compare December 14, 2023 17:33

jharveyb marked this pull request as ready for review December 14, 2023 17:56

guggero reviewed Dec 19, 2023

View reviewed changes

ffranr reviewed Dec 19, 2023

View reviewed changes

jharveyb force-pushed the fee_estimation_testing branch from 92957ce to e1e20fd Compare December 21, 2023 01:26

jharveyb mentioned this pull request Dec 21, 2023

Send: improve PSBT funding by using new lnd RPC #118

Closed

jharveyb force-pushed the fee_estimation_testing branch from e1e20fd to 7ee40f5 Compare December 21, 2023 20:32

jharveyb requested review from ffranr and guggero December 21, 2023 20:33

guggero reviewed Dec 22, 2023

View reviewed changes

tapgarden/mock.go Show resolved Hide resolved

tapfreighter/wallet.go Outdated Show resolved Hide resolved

tapfreighter/wallet.go Outdated Show resolved Hide resolved

tapfreighter/wallet.go Outdated Show resolved Hide resolved

cmd/tapcli/assets.go Show resolved Hide resolved

jharveyb force-pushed the fee_estimation_testing branch from 7ee40f5 to 1d8b487 Compare December 22, 2023 17:54

jharveyb requested a review from guggero December 22, 2023 17:56

guggero approved these changes Jan 3, 2024

View reviewed changes

ffranr approved these changes Jan 8, 2024

View reviewed changes

tapfreighter/wallet.go Outdated Show resolved Hide resolved

jharveyb added 7 commits January 8, 2024 22:29

multi: remove GetTxFee(), improve fee logging

8cbf9aa

In this commit, we add extra logging of fee rates to the minter, and also remove a reimplementation of psbt.GetTxFee(). We also deduplicate some dummy PkScript generation.

tapgarden: use valid PkScript for mock funded PSBT

d733b81

tapfreighter: fail early on dusty change outputs

f848070

itest: fix typo

2267c60

itest: add fee estimation + input types itest

b8624ed

In this commit, we add a new itest that exercises the minter and freighter when using multiple input types like P2WKH and various fee rates.

tapcli+rpcserver: switch manual fee unit to sat/vB

9a47eb8

jharveyb force-pushed the fee_estimation_testing branch from 1d8b487 to 9a47eb8 Compare January 9, 2024 03:29

jharveyb added this pull request to the merge queue Jan 9, 2024

Merged via the queue into main with commit 5a25a46 Jan 9, 2024
14 checks passed

guggero deleted the fee_estimation_testing branch January 9, 2024 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: anchor fee test coverage #605

multi: anchor fee test coverage #605

jharveyb commented Oct 19, 2023 •

edited by dstadulis

Loading

jharveyb commented Oct 25, 2023

jharveyb commented Nov 22, 2023

jharveyb commented Dec 6, 2023

jharveyb commented Dec 6, 2023 •

edited

Loading

dstadulis commented Dec 11, 2023

jharveyb commented Dec 11, 2023

jharveyb commented Dec 12, 2023

jharveyb commented Dec 14, 2023

jharveyb commented Dec 18, 2023

guggero left a comment

ffranr left a comment

ffranr Dec 19, 2023

jharveyb commented Dec 20, 2023

jharveyb commented Dec 21, 2023

jharveyb commented Dec 21, 2023

guggero left a comment

lightninglabs-deploy commented Dec 29, 2023

ffranr left a comment

multi: anchor fee test coverage #605

multi: anchor fee test coverage #605

Conversation

jharveyb commented Oct 19, 2023 • edited by dstadulis Loading

jharveyb commented Oct 25, 2023

jharveyb commented Nov 22, 2023

jharveyb commented Dec 6, 2023

jharveyb commented Dec 6, 2023 • edited Loading

dstadulis commented Dec 11, 2023

jharveyb commented Dec 11, 2023

jharveyb commented Dec 12, 2023

jharveyb commented Dec 14, 2023

jharveyb commented Dec 18, 2023

guggero left a comment

Choose a reason for hiding this comment

ffranr left a comment

Choose a reason for hiding this comment

ffranr Dec 19, 2023

Choose a reason for hiding this comment

jharveyb commented Dec 20, 2023

jharveyb commented Dec 21, 2023

jharveyb commented Dec 21, 2023

guggero left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Dec 29, 2023

ffranr left a comment

Choose a reason for hiding this comment

jharveyb commented Oct 19, 2023 •

edited by dstadulis

Loading

jharveyb commented Dec 6, 2023 •

edited

Loading