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.

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

multi: switch to sat/kw fees #1644

Merged
merged 4 commits into from Aug 10, 2018

Conversation

Projects
None yet
4 participants
@wpaulino
Collaborator

wpaulino commented Jul 28, 2018

In this PR, we modify our FeeEstimator interface to return an estimated fee rate in sat/kw. Recently, due to low fees on the network, users have been experiencing failures broadcasting transactions due to not meeting specific fee requirements. This was happening more often than not, as the estimated fee returned by backend nodes (bitcoind and btcd) only takes into account vbytes, rather than weight. The fees returned are also expressed in sat/kb, so we must take care that we do not lose precision while converting to sat/kw. In the event that this happens, a fee floor of 253 sat/kw has been added. This fee rate originates from bitcoind rounding up the conversion from weight to vbytes.

Fixes #1571.
Fixes #1610.

@Roasbeef

Did an initial pass through, and the set of changes reads well to me. We'll need to follow up with an additional few rounds of testing to ensure that we're not introducing some weird fee regression as a side effect of this PR.

I also updated the PR description to include the two issues that this directly affects. Once this goes in, I consider those to issues fixed for future users. However, for existing users that have already run into these issues, we may want to create a follow up patch (a component of the utxo pool really) which will simply re-sign if the fee is too low.

@Roasbeef Roasbeef removed the needs review label Jul 31, 2018

b.minFeeRate = SatPerVByte(relayFee / 1000)
// The fee rate is expressed in sat/kb, so we'll manually convert it to
// our desired sat/kw rate.
minRelayFeePerKw := SatPerVByte(relayFee / 1000).FeePerKWeight()

This comment has been minimized.

@halseth

halseth Jul 31, 2018

Collaborator

This will lose some precision.

This comment has been minimized.

@Roasbeef

Roasbeef Aug 1, 2018

Member

Why can't we just convert directly to sat/kw and skip the vbyte hop?

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

Fixed.

// EstimateFeePerVSize takes in a target for the number of blocks until an
// initial confirmation and returns the estimated fee expressed in
// satoshis/vbyte.
// EstimateFeePer takes in a target for the number of blocks until an initial

This comment has been minimized.

@halseth

halseth Jul 31, 2018

Collaborator

update name

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

Fixed.

satPerByte = b.minFeeRate
// Since we use fee rates in sat/kw internally, we'll convert the
// estimated fee rate from its sat/kb representation to sat/kw.
satPerKw := SatPerVByte(satPerKB / 1000).FeePerKWeight()

This comment has been minimized.

@halseth

halseth Jul 31, 2018

Collaborator

should probably create a SatPeKB->SatPerKw method to do this conversion without precision loss.

This comment has been minimized.

@halseth

halseth Aug 1, 2018

Collaborator

Actually, I suggest renaming SatPerVByte->SatPerKVByte, then we can do the conversion as is done here (removing the division by 1000 ofc)

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

Fixed.

lnwallet.OfferedHtlcSuccessWitnessSize,
)
weightEstimator.AddP2WKHOutput()
totalWeight := weightEstimator.Weight()

This comment has been minimized.

@halseth

halseth Jul 31, 2018

Collaborator

The chaining previously used read pretty nicely IMO :)

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

Fixed.

paymentMap := map[string]int64{in.Addr: in.Amount}
txid, err := r.sendCoinsOnChain(paymentMap, feeRate)
txid, err := r.sendCoinsOnChain(paymentMap, feePerVByte)

This comment has been minimized.

@halseth

halseth Jul 31, 2018

Collaborator

sendCoinsOnChain should also be modified to take sat/kw. I think it will be converted to sat/KB at some point, where which we can do the conversion.

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

Fixed.

@Roasbeef Roasbeef added this to the 0.5 milestone Aug 1, 2018

@wpaulino wpaulino force-pushed the wpaulino:kw-fees branch from 2cb6e2f to af278ed Aug 1, 2018

@cfromknecht

nice set of changes! conversion looks solid, my comments are mostly about making the units more explicit

// EstimateFee takes in a target for the number of blocks until an
// initial confirmation and returns the estimated fee expressed in
// sat/kw.
EstimateFee(numBlocks uint32) (SatPerKWeight, error)

This comment has been minimized.

@cfromknecht

cfromknecht Aug 4, 2018

Collaborator

change name to EstimateFeePerKW/EstimateFeePerKWeight

This comment has been minimized.

@wpaulino

wpaulino Aug 9, 2018

Collaborator

Fixed.

@@ -56,13 +67,13 @@ type FeeEstimator interface {
type StaticFeeEstimator struct {
// FeeRate is the static fee rate in satoshis-per-vbyte that will be
// returned by this fee estimator.
FeeRate SatPerVByte
FeeRate SatPerKWeight

This comment has been minimized.

@cfromknecht

cfromknecht Aug 4, 2018

Collaborator

while we're changing this, can we set the field name to FeePerKW, so the units are clear in other parts of the code base? would recommend doing the same for btcd and bitcoind estimators

This comment has been minimized.

@wpaulino

wpaulino Aug 9, 2018

Collaborator

Fixed.

@@ -37,13 +37,13 @@ const (
defaultBitcoinBaseFeeMSat = lnwire.MilliSatoshi(1000)
defaultBitcoinFeeRate = lnwire.MilliSatoshi(1)
defaultBitcoinTimeLockDelta = 144
defaultBitcoinStaticFeeRate = lnwallet.SatPerVByte(50)
defaultBitcoinStaticFeeRate = lnwallet.SatPerKVByte(50 * 1000)

This comment has been minimized.

@cfromknecht

cfromknecht Aug 4, 2018

Collaborator

would change these to be explicitly in SatPerKWeight

This comment has been minimized.

@halseth

halseth Aug 6, 2018

Collaborator

Agreed, but a comment with the feerate in sat/b.

This comment has been minimized.

@wpaulino

wpaulino Aug 9, 2018

Collaborator

Fixed.

@@ -37,13 +37,13 @@ const (
defaultBitcoinBaseFeeMSat = lnwire.MilliSatoshi(1000)
defaultBitcoinFeeRate = lnwire.MilliSatoshi(1)
defaultBitcoinTimeLockDelta = 144
defaultBitcoinStaticFeeRate = lnwallet.SatPerVByte(50)
defaultBitcoinStaticFeeRate = lnwallet.SatPerKVByte(50 * 1000)

This comment has been minimized.

@halseth

halseth Aug 6, 2018

Collaborator

Agreed, but a comment with the feerate in sat/b.

case feePerByte != 0:
return lnwallet.SatPerVByte(feePerByte), nil
return lnwallet.SatPerKVByte(feePerByte).FeePerKWeight(), nil

This comment has been minimized.

@halseth

halseth Aug 6, 2018

Collaborator

must be multiplied by 1000.

This comment has been minimized.

@wpaulino

wpaulino Aug 9, 2018

Collaborator

Nice catch! Don't blindly search and replace 🤫

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Aug 9, 2018

Needs rebase.

@wpaulino wpaulino force-pushed the wpaulino:kw-fees branch from af278ed to 1f2bb9b Aug 9, 2018

// By default, we'll use the backend node's minimum relay fee as the
// minimum fee rate we'll propose for transacations. However, if this
// happens to be lower than our fee floor, we'll enforce that instead.

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

👍

AddP2PKHInput().
AddP2WKHOutput().VSize()
totalFees := feePerVSize.FeeForVSize(int64(totalVSize))
var weightEstimator lnwallet.TxWeightEstimator

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nit: could keep the pattern, only changin to Weight()

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Fixed.

defaultLitecoinDustLimit = btcutil.Amount(54600)
// defaultBitcoinStaticFeePerKW is the fee rate of 50 sat/vbyte

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

👍

defaultLitecoinDustLimit = btcutil.Amount(54600)
// defaultBitcoinStaticFeePerKW is the fee rate of 50 sat/vbyte
// expressed in sat/kw.
//defaultBitcoinStaticFeePerKW = lnwallet.SatPerKVByte(50 * 1000)

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

remove

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Fixed.

if err != nil {
msg.err <- err
return
}
// If the converted fee-per-kw is below the current widely used policy

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nice 😎

quit chan struct{}
}
func (m *mockFeeEstimator) EstimateFeePerVSize(numBlocks uint32) (lnwallet.SatPerVByte, error) {
func (m *mockFeeEstimator) EstimateFeePerKW(numBlocks uint32) (lnwallet.SatPerKWeight, error) {

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nit: wrap line?

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Fixed.

@@ -172,7 +172,7 @@ func (n *NetworkHarness) SetUp(lndArgs []string) error {
PkScript: addrScript,
Value: btcutil.SatoshiPerBitcoin,
}
if _, err := n.Miner.SendOutputs([]*wire.TxOut{output}, 30); err != nil {
if _, err := n.Miner.SendOutputs([]*wire.TxOut{output}, 7500); err != nil {

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nit: wrap line

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Fixed.

@@ -1159,7 +1159,7 @@ func (n *NetworkHarness) sendCoins(ctx context.Context, amt btcutil.Amount,
PkScript: addrScript,
Value: int64(amt),
}
if _, err := n.Miner.SendOutputs([]*wire.TxOut{output}, 30); err != nil {
if _, err := n.Miner.SendOutputs([]*wire.TxOut{output}, 7500); err != nil {

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nit: wrap line?

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Fixed.

wpaulino added some commits Jul 28, 2018

lnwallet: switch fee estimation to return sat/kw fee rates
In this commit, we modify our FeeEstimator interface to return an
estimated fee rate in sat/kw. Recently, due to low fees on the network,
users have been experiencing failures broadcasting transactions due to
not meeting specific fee requirements. This was happening more often
than not, as the estimated fee returned by backend nodes (bitcoind and
btcd) only takes into account vbytes, rather than weight. The fees
returned are also expressed in sat/kb, so we must take care that we do
not lose precision while converting to sat/kw. In the event that this
happens, a fee floor of 253 sat/kw has been added. This fee rate
originates from bitcoind rounding up the conversion from weight to
vbytes.
multi: switch to transaction weight to calculate transaction fees
Due to a recent change within the codebase to return estimated fee rates
in sat/kw, this commit ensures that we use this fee rate properly by
calculing a transaction's fees using its weight. This includes all of
the different transactions that are created within lnd (funding, sweeps,
etc.). On-chain transactions still rely on a sat/vbyte fee rate since it's
required by btcwallet.
rpcserver: convert sat/vbyte fee rates input by the user to sat/kw
In this commit, we explicitly convert sat/vbyte fee rates input by the
user to sat/kw. We do this as users are typically more accustomed to
sat/vbyte fee rates, rather than sat/kw.

@wpaulino wpaulino force-pushed the wpaulino:kw-fees branch from 1f2bb9b to 336f1c1 Aug 10, 2018

@Roasbeef

LGTM 🍡

@Roasbeef Roasbeef merged commit d2612e5 into lightningnetwork:master Aug 10, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 54.546%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wpaulino wpaulino deleted the wpaulino:kw-fees branch Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment