btcclient+btcjson: defaultMaxFeeRate to BTC/kvB #2142
Conversation
btcjson/chainsvrcmds.go
Outdated
| @@ -895,7 +895,7 @@ func NewSendRawTransactionCmd(hexTx string, allowHighFees *bool) *SendRawTransac | |||
| // sendrawtransaction JSON-RPC command to a bitcoind node. | |||
| // | |||
| // A 0 maxFeeRate indicates that a maximum fee rate won't be enforced. | |||
| func NewBitcoindSendRawTransactionCmd(hexTx string, maxFeeRate int32) *SendRawTransactionCmd { | |||
| func NewBitcoindSendRawTransactionCmd(hexTx string, maxFeeRate float64) *SendRawTransactionCmd { | |||
There was a problem hiding this comment.
Should we add a note in the Godoc comment above what the unit for the maxFeeRate is?
There was a problem hiding this comment.
Perhaps we can even make a type alias to float64 of the unit? So something like type SatPerVByte = float64. We have some of this in lnd, but it isn't in a sub-module yet, so we'd need to bring in the entire repo. Alternatively, we can just copy over the files/definitions somewhere in btcutil, as they're super useful if you're doing any on chain stuff.
There was a problem hiding this comment.
I added a comment about maxFeeRate unit.
714e994
I will rebase after review.
We have some of this in lnd
Would it be possible to tell me where in lnd the above mentioned file is located?
I assume that adding a type alias for BTC/kvB will be handled in a separate PR once it is decided, as it requires an update to btcutil.
There was a problem hiding this comment.
The fee rate types are here: https://github.com/lightningnetwork/lnd/blob/60bc30dd08bb6abb39d7f8481ebd990d587e5469/lnwallet/chainfee/rates.go.
Yes, because of the Go modules, there first needs to be a PR in btcutil, then after merging that a tag is pushed and then the code can be used in this file here.
But maybe, because bitcoind uses its own fee rate type (BTC/kvB instead of sat/kWU we use in lnd), it would be okay to define the type in this package directly, as AFAIK it's the only place where we actually use BTC/kvB.
It looks like the fee rate is used for fundrawtransaction and sendrawtransaction currently.
There was a problem hiding this comment.
Thank you for sharing.
Yes, I agree with adding the type alias here.
It is used by TestMempoolAccept, fundrawtransaction and SendRawTransaction, so I am replacing their definitions with this type alias.
bd5af2b
YusukeShimizu
force-pushed
the
feeRate-BTCkvB
branch
from
d65e7c4
to
bd5af2b
Compare
Pull Request Test Coverage Report for Build 8369564008Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
defaultMaxFeeRate was set to 1e8 / 10(sat/kb) as a parameter. But BTC/kvB is the expected value, so the units was wrong. This commit updates defaultMaxFeeRate to BTC/kvB and sets it to 0.1, which is the default value of Bitcoin Core. This commit also updates the comment to reflect the change. Because maxFeeRate sanity check has been added in bitcoin core v27.0 or later, sendrawtransaction cannot be executed without this change.
Added type alias BTC/kvB to explicitly indicate that it represents the fee in BTC for a transaction size of 1 kB. Because bitcoind uses its own fee rate type (BTC/kvB instead of sat/kWU we use in lnd), define the type in btcjson package, as it's the only place where we actually use BTC/kvB.
YusukeShimizu
force-pushed
the
feeRate-BTCkvB
branch
from
bd5af2b
to
80b27f5
Compare
|
I have squashed it. |
defaultMaxFeeRatewas set to 1e8 / 10(sat/kb) as a parameter.But BTC/kvB is the expected value, so the units was wrong.
This updates
defaultMaxFeeRateto BTC/kvB and sets it to 0.1, which is the default value of Bitcoin Core.Because
maxFeeRatesanity check has been added in bitcoin core v27.0 or later, sendrawtransaction cannot be executed without this change.Fixes lightningnetwork/lnd#8571