Skip to content
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

Jump to bottom

feat: add fee estimation #118

Merged
merged 11 commits into from
Nov 28, 2022
Merged

feat: add fee estimation #118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8d5afae
add fee estimation
gitferry Nov 23, 2022
09cdf8a
removed tx-fee-default and use tx-fee-max as default
gitferry Nov 23, 2022
f738fe7
add error log
gitferry Nov 24, 2022
c3ca3ba
add estimatesmartfee
gitferry Nov 24, 2022
0de53b5
change fee return condition
gitferry Nov 24, 2022
5fdaa9e
Merge branch 'dev' into submitter/fee-estimation
gitferry Nov 28, 2022
58c9847
modified tx size calculation
gitferry Nov 28, 2022
fd2b46f
add log
gitferry Nov 28, 2022
bce3483
refactor calculating tx size
gitferry Nov 28, 2022
ab04295
change calculating tx size without witness data
gitferry Nov 28, 2022
fc82572
change min tx fee to 200
gitferry Nov 28, 2022
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
25 changes: 22 additions & 3 deletions btcclient/client_wallet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,28 @@ func NewWallet(cfg *config.BTCConfig) (*Client, error) {
return wallet, nil
}

// TODO make it dynamic
func (c *Client) GetTxFee() uint64 {
return uint64(c.Cfg.TxFee.ToUnit(btcutil.AmountSatoshi))
// GetTxFee returns tx fee according to its size
// if tx size is zero, it returns the default tx
// fee in config
func (c *Client) GetTxFee(txSize uint64) uint64 {
defaultFee := uint64(c.Cfg.TxFeeDefault.ToUnit(btcutil.AmountSatoshi))
if txSize == 0 {
return defaultFee
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be called when finding utxos that have sufficient funds

}
feeRate, err := c.Client.EstimateFee(c.Cfg.TargetBlockNum)
if err != nil {
return defaultFee
Copy link
Member

Choose a reason for hiding this comment

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

What is the failure condition on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EstimateFee could fail if the local BTC node does not have sufficient resources to estimate fee.

Copy link
Member

Choose a reason for hiding this comment

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

What resources does it need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example, when FeeEstimator is disabled in the BTC node, or when there are insufficient blocks observed

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}
fee := uint64(feeRateAmount.ToUnit(btcutil.AmountSatoshi)) * txSize
if fee > uint64(c.Cfg.TxFeeMax.ToUnit(btcutil.AmountSatoshi)) ||
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Thanks!

fee < uint64(c.Cfg.TxFeeMax.ToUnit(btcutil.AmountSatoshi)) {
Copy link
Member

Choose a reason for hiding this comment

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

you use c.Cfg.TxFeeMax for both checks.

return defaultFee
Copy link
Member

Choose a reason for hiding this comment

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

What if the defaultFee is more than max or less than min?

}
return fee
}

func (c *Client) GetWalletName() string {
Expand Down
2 changes: 1 addition & 1 deletion btcclient/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type BTCWallet interface {
GetWalletPass() string
GetWalletLockTime() int64
GetNetParams() *chaincfg.Params
GetTxFee() uint64 // in the unit of satoshi
GetTxFee(txSize uint64) uint64 // in the unit of satoshi
ListUnspent() ([]btcjson.ListUnspentResult, error)
ListReceivedByAddress() ([]btcjson.ListReceivedByAddressResult, error)
SendRawTransaction(tx *wire.MsgTx, allowHighFees bool) (*chainhash.Hash, error)
Expand Down
15 changes: 12 additions & 3 deletions config/bitcoin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Default implies that another config must be set and this will be used as a default, but actually the default behavior is that the transaction fee is calculated automatically. Actually, I would have the TxFeeMax be the default fee. For example, we have the following scenarios where we use the default fee:

  1. The fee calculation mechanism returns an error: Use the max fee, the configuration specifies that the user is ok with paying that.
  2. The fee calculation produced a fee that is larger than the maximum fee to pay: Use the max fee, as this is the closest one the user is willing to pay based on the estimation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Thanks!

TxFeeMin btcutil.Amount `mapstructure:"tx-fee-min"` // minimum tx fee, in BTC
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

TxFeeMax btcutil.Amount `mapstructure:"tx-fee-max"` // maximum tx fee, in BTC
Copy link
Member

@vitsalis vitsalis Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

maybe panic if TxFeeMax < TxFeeMin?

TargetBlockNum int64 `mapstructure:"target-block-num"` // for tx fee estimation
Copy link
Member

Choose a reason for hiding this comment

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

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).

NetParams string `mapstructure:"net-params"`
Username string `mapstructure:"username"`
Password string `mapstructure:"password"`
Expand Down Expand Up @@ -50,7 +53,10 @@ func (cfg *BTCConfig) Validate() error {
}

func DefaultBTCConfig() BTCConfig {
feeAmount, err := btcutil.NewAmount(0.00001)
feeAmountDefault, err := btcutil.NewAmount(1e-5)
feeAmountMin, err := btcutil.NewAmount(1e-6)
feeAmountMax, err := btcutil.NewAmount(1e-3)

if err != nil {
panic(err)
}
Expand All @@ -63,7 +69,10 @@ func DefaultBTCConfig() BTCConfig {
WalletName: "default",
WalletCAFile: defaultBtcWalletCAFile,
WalletLockTime: 10,
TxFee: feeAmount,
TxFeeDefault: feeAmountDefault,
TxFeeMin: feeAmountMin,
TxFeeMax: feeAmountMax,
TargetBlockNum: 1,
NetParams: types.BtcSimnet.String(),
Username: "rpcuser",
Password: "rpcpass",
Expand Down
4 changes: 4 additions & 0 deletions sample-vigilante-docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ btc:
ca-file: /bitcoin/rpc.cert
endpoint: host.docker.internal:18556
tx-fee: 2500
tx-fee-default: 2500
tx-fee-min: 1
tx-fee-max: 100000
target-block-num: 1
wallet-endpoint: host.docker.internal:18554
wallet-password: walletpass
wallet-name: default
Expand Down
5 changes: 4 additions & 1 deletion sample-vigilante.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ btc:
no-client-tls: false # use true for bitcoind as it does not support tls
ca-file: $TESTNET_PATH/bitcoin/rpc.cert
endpoint: localhost:18556 # use port 18443 for bitcoind regtest
tx-fee: 2500
tx-fee-default: 2500
tx-fee-min: 1
tx-fee-max: 100000
target-block-num: 1
wallet-endpoint: localhost:18554
wallet-password: walletpass
wallet-name: default
Expand Down
30 changes: 26 additions & 4 deletions submitter/relayer/relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ func (rl *Relayer) ChainTwoTxAndSend(
tx1, recipient, err := rl.buildTxWithData(
utxo,
data1,
false,
)
if err != nil {
return nil, nil, err
Expand All @@ -137,6 +138,7 @@ func (rl *Relayer) ChainTwoTxAndSend(
tx2, _, err := rl.buildTxWithData(
changeUtxo,
data2,
false,
)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return nil, errors.New("insufficient fees")
}

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used when calculating tx size. See here

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we always calculate the transaction size?

Copy link
Member

Choose a reason for hiding this comment

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

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).

) (*wire.MsgTx, btcutil.Address, error) {
log.Logger.Debugf("Building a BTC tx using %v with data %x", utxo.TxID.String(), data)
tx := wire.NewMsgTx(wire.TxVersion)
Expand Down Expand Up @@ -248,9 +251,18 @@ func (rl *Relayer) buildTxWithData(
}
// TODO
// If this will become a dynamic calculation this might lead to a different fee being used in the log and the actual transaction.
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))
Copy link
Member

Choose a reason for hiding this comment

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

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

if !forTxSize {
// to simulate building the tx and get tx size
txSize, err := rl.getTxSize(utxo, data)
if err != nil {
return nil, nil, err
}
txFee := rl.GetTxFee(txSize)
change -= txFee
log.Logger.Debugf("balance of input: %v satoshi, tx fee: %v satoshi, output value: %v",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

int64(utxo.Amount.ToUnit(btcutil.AmountSatoshi)), txFee, change)
}
tx.AddTxOut(wire.NewTxOut(int64(change), changeScript))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?


// sign tx
Expand Down Expand Up @@ -285,6 +297,16 @@ func (rl *Relayer) buildTxWithData(
return tx, changeAddr, nil
}

func (rl *Relayer) getTxSize(utxo *types.UTXO, data []byte) (uint64, error) {
tx, _, err := rl.buildTxWithData(utxo, data, true)
if err != nil {
return 0, err
}
txSize := tx.SerializeSize()
log.Logger.Debugf("tx size is %v", txSize)
return uint64(txSize), nil
}

func (rl *Relayer) sendTxToBTC(tx *wire.MsgTx) (*chainhash.Hash, error) {
log.Logger.Debugf("Sending tx %v to BTC", tx.TxHash().String())
ha, err := rl.SendRawTransaction(tx, true)
Expand Down
8 changes: 4 additions & 4 deletions testutil/mocks/btcclient.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.