feat: add fee estimation #118

gitferry · 2022-11-23T13:55:02Z

This PR is to add fee estimation. In particular, this PR changes the way it gets tx fee to via EstimateFee RPC call rather than via config.

The EstimateFee RPC call returns tx fee rate (BTC/byte). To estimate tx fee, this PR adds getTxSize to calculate tx size with simulated tx construction without adding tx fee.

Note that EstimateFee is deprecated, so it should be replaced by EstimateSmartFee after btcd upgrade to v0.23. See #117.

vitsalis

Thanks for this! Some comments

vitsalis · 2022-11-23T14:05:33Z

btcclient/client_wallet.go

+func (c *Client) GetTxFee(txSize uint64) uint64 {
+	defaultFee := uint64(c.Cfg.TxFeeDefault.ToUnit(btcutil.AmountSatoshi))
+	if txSize == 0 {
+		return defaultFee


Should this be a panic condition? Someone calling with an empty transaction size.

This will be called when finding utxos that have sufficient funds

vitsalis · 2022-11-23T14:06:11Z

btcclient/client_wallet.go

+	}
+	feeRateAmount, err := btcutil.NewAmount(feeRate)
+	if err != nil {
+		return defaultFee


Would this having an error mean that EstimateFee returned something that is not a proper fee rate? Maybe we should panic here as well.

Yeah, this err would mean feeRate is very wrong. We should panic here. Thanks for pointing out!

vitsalis · 2022-11-23T14:07:09Z

btcclient/client_wallet.go

+		return defaultFee
+	}
+	fee := uint64(feeRateAmount.ToUnit(btcutil.AmountSatoshi)) * txSize
+	if fee > uint64(c.Cfg.TxFeeMax.ToUnit(btcutil.AmountSatoshi)) ||


We can maybe create a helper method i.e. feeApplicable(fee, c.Cfg.TxFeeMax, c.Cfg.TxFeeMin) for this check.

Good point. Thanks!

vitsalis · 2022-11-23T14:07:24Z

btcclient/client_wallet.go

+	}
+	fee := uint64(feeRateAmount.ToUnit(btcutil.AmountSatoshi)) * txSize
+	if fee > uint64(c.Cfg.TxFeeMax.ToUnit(btcutil.AmountSatoshi)) ||
+		fee < uint64(c.Cfg.TxFeeMax.ToUnit(btcutil.AmountSatoshi)) {


you use c.Cfg.TxFeeMax for both checks.

vitsalis · 2022-11-23T14:08:42Z

config/bitcoin.go

@@ -17,7 +17,10 @@ type BTCConfig struct {
 	WalletName        string                    `mapstructure:"wallet-name"`
 	WalletCAFile      string                    `mapstructure:"wallet-ca-file"`
 	WalletLockTime    int64                     `mapstructure:"wallet-lock-time"` // time duration in which the wallet remains unlocked, in seconds
-	TxFee             btcutil.Amount            `mapstructure:"tx-fee"`           // BTC tx fee, in BTC
+	TxFeeDefault      btcutil.Amount            `mapstructure:"tx-fee-default"`   // default BTC tx fee, in BTC
+	TxFeeMin          btcutil.Amount            `mapstructure:"tx-fee-min"`       // minimum tx fee, in BTC


Why would we want a minimum transaction fee? Personally I would always set this to 0

Having 0 might cause an error saying the tx has insufficient priority. I'm not sure of the exact min fee we should set tho.

I suggest we remove this attribute alltogether. Why would someone want to set the minimum fee into anything other than the minimum value possible (e.g. 1)?

The estimation might return us an extremely low tx fee which is not reasonable. We set a reasonable tx fee here to prevent this case. It could be 1, 10, or 100.

vitsalis · 2022-11-23T14:12:37Z

config/bitcoin.go

+	TxFeeDefault      btcutil.Amount            `mapstructure:"tx-fee-default"`   // default BTC tx fee, in BTC
+	TxFeeMin          btcutil.Amount            `mapstructure:"tx-fee-min"`       // minimum tx fee, in BTC
+	TxFeeMax          btcutil.Amount            `mapstructure:"tx-fee-max"`       // maximum tx fee, in BTC
+	TargetBlockNum    int64                     `mapstructure:"target-block-num"` // for tx fee estimation


Maybe add in the comment that this specifies a parameter into how soon we want the transaction to be included (e.g. 1 specifying next block).

vitsalis · 2022-11-23T14:14:24Z

submitter/relayer/relayer.go

@@ -195,7 +197,7 @@ func (rl *Relayer) PickHighUTXO() (*types.UTXO, error) {
 	}

 	// TODO: consider dust, reference: https://www.oreilly.com/library/view/mastering-bitcoin/9781491902639/ch08.html#tx_verification
-	if uint64(amount.ToUnit(btcutil.AmountSatoshi)) < rl.GetTxFee()*2 {
+	if uint64(amount.ToUnit(btcutil.AmountSatoshi)) < rl.GetTxFee(0)*2 {


This will return the default fee, without considering the transaction size.

Yes, this is to ensure that we have sufficient balance in this utxo as we are considering max fee as default

Then why don't we use the max fee config directly? This adds further complexity to the GetTxFee function by adding a special case with a 0 input which is not clear.

vitsalis · 2022-11-23T14:15:09Z

submitter/relayer/relayer.go

@@ -219,6 +221,7 @@ func (rl *Relayer) PickHighUTXO() (*types.UTXO, error) {
 func (rl *Relayer) buildTxWithData(
 	utxo *types.UTXO,
 	data []byte,
+	forTxSize bool,


Why do we add this parameter? We should always try to estimate the fee.

This is used when calculating tx size. See here

Why don't we always calculate the transaction size?

Ah I see what's happening. I suggest that we don't use this function again with the dubious flag, but just create a copy of the transaction we have already built and add a dummy change output. Then calculate the transaction size based on that. A cyclic dependency between the functions based on a flag is going to come bite us in the future (I had to spend some time reading the code to understand what was happening).

vitsalis · 2022-11-23T14:17:33Z

submitter/relayer/relayer.go

+		}
+		txFee := rl.GetTxFee(txSize)
+		change -= txFee
+		log.Logger.Debugf("balance of input: %v satoshi, tx fee: %v satoshi, output value: %v",


This log appeared before but now it will only appear if forTxSize is set to false.

Yep. The log shouldn't appear when calculating tx size

vitsalis · 2022-11-23T14:18:57Z

submitter/relayer/relayer.go

+		change -= txFee
+		log.Logger.Debugf("balance of input: %v satoshi, tx fee: %v satoshi, output value: %v",
+			int64(utxo.Amount.ToUnit(btcutil.AmountSatoshi)), txFee, change)
+	}
 	tx.AddTxOut(wire.NewTxOut(int64(change), changeScript))


The above estimation uses a transaction without the change output and therefore it will have a lesser size leading to an estimation based on a lower-sized transaction than the one that is actually sent.

Actually, the output is included. Only that the change will minus tx fee (we don't know the tx fee yet) when calculating tx size

I don't see tx.AddTxOut with an empty change (or a dummy change etc) for adding the second output in the transaction. I only see a single tx.AddTxOut for the OP_RETURN. Am I missing something?

gitferry · 2022-11-23T16:21:58Z

Hi @vitsalis, thanks for your reviews which I have replied/addressed. In particular, I removed tx-fee-default and use tx-fee-max as default tx fee. Please see the updates.

vitsalis · 2022-11-28T08:53:15Z

submitter/relayer/relayer.go

@@ -195,7 +197,7 @@ func (rl *Relayer) PickHighUTXO() (*types.UTXO, error) {
 	}

 	// TODO: consider dust, reference: https://www.oreilly.com/library/view/mastering-bitcoin/9781491902639/ch08.html#tx_verification
-	if uint64(amount.ToUnit(btcutil.AmountSatoshi)) < rl.GetTxFee()*2 {
+	if uint64(amount.ToUnit(btcutil.AmountSatoshi)) < rl.GetTxFee(0)*2 {


Then why don't we use the max fee config directly? This adds further complexity to the GetTxFee function by adding a special case with a 0 input which is not clear.

vitsalis · 2022-11-28T08:53:55Z

submitter/relayer/relayer.go

@@ -219,6 +221,7 @@ func (rl *Relayer) PickHighUTXO() (*types.UTXO, error) {
 func (rl *Relayer) buildTxWithData(
 	utxo *types.UTXO,
 	data []byte,
+	forTxSize bool,


Why don't we always calculate the transaction size?

vitsalis · 2022-11-28T08:55:50Z

submitter/relayer/relayer.go

+		change -= txFee
+		log.Logger.Debugf("balance of input: %v satoshi, tx fee: %v satoshi, output value: %v",
+			int64(utxo.Amount.ToUnit(btcutil.AmountSatoshi)), txFee, change)
+	}
 	tx.AddTxOut(wire.NewTxOut(int64(change), changeScript))


I don't see tx.AddTxOut with an empty change (or a dummy change etc) for adding the second output in the transaction. I only see a single tx.AddTxOut for the OP_RETURN. Am I missing something?

vitsalis · 2022-11-28T08:57:17Z

submitter/relayer/relayer.go

-	change := uint64(utxo.Amount.ToUnit(btcutil.AmountSatoshi)) - rl.GetTxFee()
-	log.Logger.Debugf("balance of input: %v satoshi, tx fee: %v satoshi, output value: %v",
-		int64(utxo.Amount.ToUnit(btcutil.AmountSatoshi)), rl.GetTxFee(), change)
+	change := uint64(utxo.Amount.ToUnit(btcutil.AmountSatoshi))


If forTxSize is true, then the change will be all the funds of the UTXO.

vitsalis · 2022-11-28T09:00:13Z

submitter/relayer/relayer.go

@@ -219,6 +221,7 @@ func (rl *Relayer) PickHighUTXO() (*types.UTXO, error) {
 func (rl *Relayer) buildTxWithData(
 	utxo *types.UTXO,
 	data []byte,
+	forTxSize bool,


Ah I see what's happening. I suggest that we don't use this function again with the dubious flag, but just create a copy of the transaction we have already built and add a dummy change output. Then calculate the transaction size based on that. A cyclic dependency between the functions based on a flag is going to come bite us in the future (I had to spend some time reading the code to understand what was happening).

gitferry · 2022-11-28T14:15:21Z

Thanks @vitsalis for your review. I have improved the way we calculate tx size and addressed other comments. Please see the updates. Thanks!

vitsalis

Nice! Minor comments:

vitsalis · 2022-11-28T14:14:03Z

submitter/relayer/utils.go

+
+func calTxSize(tx *wire.MsgTx, utxo *types.UTXO, changeScript []byte, isSegWit bool, privkey *btcec.PrivateKey) (uint64, error) {
+	tx.AddTxOut(wire.NewTxOut(dummyChangeValue, changeScript))
+	if !isSegWit {


This if/else based on isSegwit already happens in the buildTxWithData. Can it be moved to a util function to avoid code duplication?

vitsalis · 2022-11-28T14:14:49Z

submitter/relayer/utils.go

 	"github.com/btcsuite/btcutil"
 )

+const dummyChangeValue int64 = 2500000000


The change value can be the amount of tokens contained in the UTXO. This would be a good value to estimate something that is close to the actual change value.

vitsalis · 2022-11-28T14:15:17Z

submitter/relayer/utils.go

@@ -15,3 +21,42 @@ func isSegWit(addr btcutil.Address) (bool, error) {
 		return false, errors.New("non-supported address type")
 	}
 }
+
+func calTxSize(tx *wire.MsgTx, utxo *types.UTXO, changeScript []byte, isSegWit bool, privkey *btcec.PrivateKey) (uint64, error) {
+	tx.AddTxOut(wire.NewTxOut(dummyChangeValue, changeScript))


Maybe instead of modifying the transaction directly, a copy could be made?

vitsalis · 2022-11-28T14:20:24Z

config/bitcoin.go

@@ -17,7 +17,9 @@ type BTCConfig struct {
 	WalletName        string                    `mapstructure:"wallet-name"`
 	WalletCAFile      string                    `mapstructure:"wallet-ca-file"`
 	WalletLockTime    int64                     `mapstructure:"wallet-lock-time"` // time duration in which the wallet remains unlocked, in seconds
-	TxFee             btcutil.Amount            `mapstructure:"tx-fee"`           // BTC tx fee, in BTC
+	TxFeeMin          btcutil.Amount            `mapstructure:"tx-fee-min"`       // minimum tx fee, in BTC
+	TxFeeMax          btcutil.Amount            `mapstructure:"tx-fee-max"`       // maximum tx fee, in BTC


maybe panic if TxFeeMax < TxFeeMin?

add fee estimation

8d5afae

gitferry requested a review from vitsalis November 23, 2022 13:55

vitsalis reviewed Nov 23, 2022

View reviewed changes

gitferry force-pushed the submitter/fee-estimation branch from dbbcf30 to 2d29aa8 Compare November 23, 2022 16:20

removed tx-fee-default and use tx-fee-max as default

09cdf8a

gitferry force-pushed the submitter/fee-estimation branch from 2d29aa8 to 09cdf8a Compare November 23, 2022 16:23

gitferry added 3 commits November 24, 2022 17:28

add error log

f738fe7

add estimatesmartfee

c3ca3ba

change fee return condition

0de53b5

vitsalis reviewed Nov 28, 2022

View reviewed changes

Merge branch 'dev' into submitter/fee-estimation

5fdaa9e

gitferry force-pushed the submitter/fee-estimation branch 3 times, most recently from f47c932 to 56b0189 Compare November 28, 2022 13:51

modified tx size calculation

58c9847

gitferry force-pushed the submitter/fee-estimation branch from 56b0189 to 58c9847 Compare November 28, 2022 14:08

vitsalis approved these changes Nov 28, 2022

View reviewed changes

vitsalis reviewed Nov 28, 2022

View reviewed changes

gitferry added 2 commits November 28, 2022 22:25

add log

fd2b46f

refactor calculating tx size

bce3483

gitferry force-pushed the submitter/fee-estimation branch from 138de58 to bce3483 Compare November 28, 2022 15:38

gitferry added 2 commits November 28, 2022 23:49

change calculating tx size without witness data

ab04295

change min tx fee to 200

fc82572

gitferry force-pushed the submitter/fee-estimation branch from c39cf7e to fc82572 Compare November 28, 2022 16:04

gitferry merged commit 4a0a759 into dev Nov 28, 2022

gitferry deleted the submitter/fee-estimation branch November 28, 2022 16:14

vitsalis mentioned this pull request Feb 7, 2023

Submitter: hardcoded txfee #42

Closed

feat: add fee estimation #118

feat: add fee estimation #118

Conversation

gitferry commented Nov 23, 2022

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Nov 28, 2022

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitsalis Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

vitsalis Nov 28, 2022 •

edited

Loading