Configurable fee target by araspitzu · Pull Request #1066 · ACINQ/eclair

araspitzu · 2019-07-08T09:20:18Z

Summary of changes

Rationale

Fee computation is a sensitive topic and directly affects the profitability of node operators, thus saving as much as possible is a priority and a nice-to-have feature. Currently eclair uses a very conservative estimation for most of the transactions, resulting in unnecessary high fees under certain scenarios; this PR aims at making this behavior configurable and give the node operators the opportunity to fine tune on chain transaction costs.

High level overview

There is a new configuration block that can be used to define default values for the block target used in the fee calculation for various transactions, currently supported are types: "funding", "commitment", "mutual-close" and "claim-main". Eclair will target the block number defined there when computing the fees, this means that according to the current feerate (still taken from the usual fee providers) we calculate the absolute transaction fee trying to target the confirmation on the N-th block where N is user defined default.

funding is the block target for the funding transaction (you can override this when opening a channel)
commitment is the block target using when computing the commitment transaction, this is used when force closing the channel.
mutual-close block target used when negotiating the closing transaction fee with the remote peer.
claim-main block target is used to compute fee for the claim-main transaction, this used to spend our main channel output to our on-chain wallet

Implementation details

The claim-main block target is applied to both the claim-main-delayed and claim-main transaction, those are logically equal and none of them is competing with the counterparty for confirmation.

We still don't provide configurable block target for htlc-2nd stage transactions and main penalty transaction, this is due to the fact that in these scenarios we're competing with the counterparty and we
must act swiftly.

Transactions and block target

Funding transaction: blockTarget=funding
Commitment transaction: blockTarget=commitment
HTLC-success transaction: blockTarget=2 (hardcoded)
HTLC-timeout transaction: blockTarget=2 (hardcoded)
Main penalty transaction: blockTarget=2 (hardcoded)
HTLC penalty transaction: blockTarget=2 (hardcoded)
Mutual close transaction: blockTarget=mutual-close ( must be >= commitment)
Claim main output transaction (remote unilateral-close): blockTarget=claim-main
Claim main delayed transaction (local unilateral-close): blockTarget=claim-main
Claim htlc-delayed transaction: blockTarget=claim-main

Fixes #1028

…ove unused 'smartfeeNBlocks'

# Conflicts: # eclair-core/src/test/scala/fr/acinq/eclair/io/PeerSpec.scala

codecov-io · 2019-07-09T12:45:57Z

Codecov Report

Merging #1066 into master will increase coverage by 8.79%.
The diff coverage is 87.71%.

@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage    73.7%   82.49%   +8.79%     
==========================================
  Files         126       99      -27     
  Lines        8458     7581     -877     
  Branches      321      300      -21     
==========================================
+ Hits         6234     6254      +20     
+ Misses       2224     1327     -897

Impacted Files	Coverage Δ
...la/fr/acinq/eclair/transactions/Transactions.scala	`96.28% <ø> (ø)`	⬆️
.../main/scala/fr/acinq/eclair/channel/Register.scala	`72% <ø> (ø)`	⬆️
...eclair/blockchain/fee/BitcoinCoreFeeProvider.scala	`84.61% <100%> (+1.28%)`	⬆️
...cinq/eclair/blockchain/fee/SmoothFeeProvider.scala	`100% <100%> (ø)`	⬆️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala	`84.26% <100%> (+0.74%)`	⬆️
...acinq/eclair/blockchain/fee/BitgoFeeProvider.scala	`95.65% <100%> (+41.1%)`	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	`94.45% <100%> (+0.02%)`	⬆️
.../eclair/blockchain/fee/EarnDotComFeeProvider.scala	`100% <100%> (ø)`	⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	`75.23% <100%> (ø)`	⬆️
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala	`67.21% <100%> (+0.18%)`	⬆️
... and 34 more

pm47 · 2019-07-11T13:48:32Z

We still don't provide configurable block target for htlc-2nd stage transactions and main penalty transaction, this is due to the fact that in these scenarios we're competing with the counterparty and we
must act swiftly.

There are actually 3 stages transactions in the case of a local commit. The last stage (ClaimDelayedOutputTx) doesn't compete with remote txes, so we could also make the fees customizable. Maybe claim-htlc-delayed?

pm47 · 2019-07-11T13:49:40Z

How about funding, mutual-close and unilateral-close? Is that too vague?

araspitzu · 2019-07-11T14:04:25Z

We still don't provide configurable block target for htlc-2nd stage transactions and main penalty transaction, this is due to the fact that in these scenarios we're competing with the counterparty and we
must act swiftly.

There are actually 3 stages transactions in the case of a local commit. The last stage (ClaimDelayedOutputTx) doesn't compete with remote txes, so we could also make the fees customizable. Maybe claim-htlc-delayed?

Yes correct, the claim transaction spending the HTLC-2nd stage back to our wallet isn't competing with anyone and we don't need to have fast confirmation so we can let the user customize it, will add.

pm47 · 2019-07-11T13:53:32Z

      Transactions.weight2fee(feeratePerKw, closingWeight)
    }

+    def firstClosingFee(nodeParams: NodeParams, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {


Suggested change

def firstClosingFee(nodeParams: NodeParams, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {

def firstClosingFee(feeTargets: FeeTargets, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {

Agree with the proposed change, and let's move it to last position for consistency.

In 63becf3 we don't pass NodeParams anymore and all the feeTargets parameters have been moved to the last position

pm47 · 2019-07-11T13:56:07Z

      * @return a list of transactions (one per HTLC that we can claim)
      */
-    def claimRemoteCommitTxOutputs(keyManager: KeyManager, commitments: Commitments, remoteCommit: RemoteCommit, tx: Transaction)(implicit log: LoggingAdapter): RemoteCommitPublished = {
+    def claimRemoteCommitTxOutputs(feeTargets: FeeTargets, keyManager: KeyManager, commitments: Commitments, remoteCommit: RemoteCommit, tx: Transaction)(implicit log: LoggingAdapter): RemoteCommitPublished = {


Nit: I'm not sure why but I wouldn't use feeTargets as first parameter. Maybe between keyManager and commitments?

How about moving it to last position (and update the javadoc)?

Done in 63becf3

pm47 · 2019-07-11T13:59:58Z

+    case 12 => blocks_12
+    case 36 => blocks_36
+    case 72 => blocks_72
+    case _ => throw new IllegalArgumentException(s"can't choose ratePerBlock=$blockTarget")


How about:

def getRatePerBlock(blockTarget: Int): Long = blockTarget match { case i if i <=1 => block_1 case i if i <= 2 => blocks_2 case i if i <= 6 => blocks_6 case i if i <= 12 => blocks_12 case i if i <= 36 => blocks_36 case i if i <= 72 => blocks_72 case _ => blocks_144 }

A bit more lax to reduce support requests?

I don't like silently choosing a different feerate for the user, this can result in unexpected delays. The current block of code is wrong though (it would fail for blockTarget = 144).

Commit 5195614 fixes the problem but it does not introduce lax behavior.

I'd rather agree with @pm47 here: onchain fee estimation is very unreliable and it gets worse as the confirmation target goes up, so forcing users to use arbitrary targets seems a bit too strict.
I think we should be able to accept any confirmation target and convert it to a feerate

… of `NodeParams`

araspitzu · 2019-07-11T14:41:32Z

We still don't provide configurable block target for htlc-2nd stage transactions and main penalty transaction, this is due to the fact that in these scenarios we're competing with the counterparty and we
must act swiftly.

There are actually 3 stages transactions in the case of a local commit. The last stage (ClaimDelayedOutputTx) doesn't compete with remote txes, so we could also make the fees customizable. Maybe claim-htlc-delayed?

Yes correct, the claim transaction spending the HTLC-2nd stage back to our wallet isn't competing with anyone and we don't need to have fast confirmation so we can let the user customize it, will add.

On a second thought, do we really want to offer so much flexibility? Considering that all the claim-* transactions that we do don't compete with the counter party the PR uses the same blockTarget (namely claim-main) for all of them, i struggle to see a use case for differentiating blockTarget between different claim transactions.

pm47 · 2019-07-11T15:41:59Z

    }

-    def firstClosingFee(nodeParams: NodeParams, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {
+    def firstClosingFee(commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, feeTargets: FeeTargets)(implicit log: LoggingAdapter): Satoshi = {


This make the two firstClosingFee methods very similar. Would it be a food idea to merge them?

I split them to facilitate testing, in this test i needed to compute the expected fee for the first closing signed negotiation.

araspitzu · 2019-07-12T11:41:20Z

How about funding, mutual-close and unilateral-close? Is that too vague?

I like this suggestion, unilateral-close will be used in the commitment and is what users will expect to use when doing an unilateral close. Also I'd like to propose changing claim-main to claim-transaction which is a bit more generic and applies more broadly to all our claim transactions.

sstone

The way we estimate fees and use these estimates in helper functions is still ugly. A potential improvement would be to add a fee estimator to NodeParams, something simple like

trait FeeEstimator {
  def getFeeratePerKb(target: Int) : Long
  def getFeeratePerKw(target: Int) : Long
}

implement it with call to Globals in Setup, and with a basic estimator that returns fix values in unit tests whenever possible, and use it to get actual fee rates that would be explicitly passed to helper functions (no more calls to Globals).
This could be done first as a refactoring that does not change how we set fees, and then we could add additional configuration options. WDYT ?

sstone · 2019-07-19T08:33:41Z

+    case 12 => blocks_12
+    case 36 => blocks_36
+    case 72 => blocks_72
+    case _ => throw new IllegalArgumentException(s"can't choose ratePerBlock=$blockTarget")


I'd rather agree with @pm47 here: onchain fee estimation is very unreliable and it gets worse as the confirmation target goes up, so forcing users to use arbitrary targets seems a bit too strict.
I think we should be able to accept any confirmation target and convert it to a feerate

sstone · 2019-07-19T08:37:27Z

    }

-    val localFeeratePerKw = Globals.feeratesPerKw.get.blocks_2
+    val localFeeratePerKw = Globals.feeratesPerKw.get.getRatePerBlock(feeTargets.commitmentBlockTarget)


If we are to revisit this I'd rather pass an actual fee rate to this method instead of calling Globals.feeratesPerKw

araspitzu · 2019-07-19T10:15:14Z

The way we estimate fees and use these estimates in helper functions is still ugly. A potential improvement would be to add a fee estimator to NodeParams, something simple like
trait FeeEstimator {
  def getFeeratePerKb(target: Int) : Long
  def getFeeratePerKw(target: Int) : Long
}
implement it with call to Globals in Setup, and with a basic estimator that returns fix values in unit tests whenever possible, and use it to get actual fee rates that would be explicitly passed to helper functions (no more calls to Globals).
This could be done first as a refactoring that does not change how we set fees, and then we could add additional configuration options. WDYT ?

I agree that we should avoid using Globals during test, and having a trait passed to NodeParams seems the way to go to refactor this, it will be readily available and we can subclass it with a fixed-value implementation during the test. ACK.

araspitzu · 2019-07-19T10:19:06Z

I'd rather agree with @pm47 here: onchain fee estimation is very unreliable and it gets worse as the confirmation target goes up, so forcing users to use arbitrary targets seems a bit too strict.
I think we should be able to accept any confirmation target and convert it to a feerate

I like this proposal too but i thought it's a huge change, if we are going to accept any block target the current structure of the FeeProvider will change almost completely.

sstone · 2019-07-19T11:33:39Z

Why would it be a huge change ? we could compute a fee rate for any target using the same pattern as above:

// stores fee rate in satoshi/kb (1 kb = 1000 bytes)
case class FeeratesPerKB(block_1: Long, blocks_2: Long, blocks_6: Long, blocks_12: Long, blocks_36: Long, blocks_72: Long) {
  require(block_1 > 0 && blocks_2 > 0 && blocks_6 > 0 && blocks_12 > 0 && blocks_36 > 0 && blocks_72 > 0, "all feerates must be strictly greater than 0")

  def feeRatePerKb(target: Int) = target match {
    case 1 => block_1
    case 2 => blocks_2
    case t if t <= 6 => blocks_6
    case t if t <= 12 => blocks_12
    case t if t <= 36 => blocks_36
    case _ => blocks_72
  }
}

then our estimator is simply:

  val feeEstimator = new FeeEstimator {
    override def getFeeratePerKb(target: Int): Long = Globals.feeratesPerKB.get().feeRatePerKb(target)

    override def getFeeratePerKw(target: Int): Long = Globals.feeratesPerKw.get().feeRatePerKw(target)
  }

This way we can hide how our fee provider is implemented

araspitzu · 2019-07-20T14:31:13Z

Closed in favor of #1083

araspitzu added 4 commits July 5, 2019 12:27

Add FeeConf to NodeParams, add config block for fee configuratio, rem…

12005af

…ove unused 'smartfeeNBlocks'

Use a custom block target for commitment transaction

381c07f

Use a custom block target for funding transaction

0fbf708

Use custom block target for mutual close transaction

f0627fd

araspitzu force-pushed the custom_fee_target branch from c414f8c to f0627fd Compare July 8, 2019 13:37

ACINQ deleted a comment from codecov-io Jul 8, 2019

araspitzu added 3 commits July 8, 2019 17:42

Use custom fee target for claim main output transaction

4d1a02e

Set fee target defaults to existing values

5c5d857

Use claimP2WPKHOutputWeight to estimate claim transaction fees in test

59d96de

araspitzu force-pushed the custom_fee_target branch from 1af515c to 59d96de Compare July 9, 2019 08:51

araspitzu marked this pull request as ready for review July 9, 2019 10:43

ACINQ deleted a comment from codecov-io Jul 9, 2019

araspitzu force-pushed the custom_fee_target branch from add5e2c to 0173668 Compare July 9, 2019 11:01

araspitzu added 2 commits July 9, 2019 14:23

Add block target 144

7d0ecb6

Fix bitgo fee provider spec

8099297

araspitzu force-pushed the custom_fee_target branch from 0173668 to 8099297 Compare July 9, 2019 12:24

Merge branch 'master' into custom_fee_target

06f6995

# Conflicts: # eclair-core/src/test/scala/fr/acinq/eclair/io/PeerSpec.scala

araspitzu requested review from pm47 and sstone July 9, 2019 12:47

Merge branch 'master' into custom_fee_target

94a55ad

pm47 reviewed Jul 11, 2019

View reviewed changes

araspitzu added 2 commits July 11, 2019 16:21

Allow choosing blockTarget = 144

5195614

Move 'feerates' parameter to last position, pass FeeTargets instead…

63becf3

… of `NodeParams`

pm47 reviewed Jul 11, 2019

View reviewed changes

araspitzu mentioned this pull request Jul 15, 2019

Feature request: better (manual) fee control #1028

Closed

sstone requested changes Jul 19, 2019

View reviewed changes

araspitzu closed this Jul 20, 2019

araspitzu deleted the custom_fee_target branch November 18, 2019 14:32

	def firstClosingFee(nodeParams: NodeParams, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {
	def firstClosingFee(feeTargets: FeeTargets, commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector)(implicit log: LoggingAdapter): Satoshi = {

Conversation

araspitzu commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Rationale

High level overview

Implementation details

Transactions and block target

Uh oh!

codecov-io commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pm47 commented Jul 11, 2019

Uh oh!

pm47 commented Jul 11, 2019

Uh oh!

araspitzu commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araspitzu commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araspitzu commented Jul 12, 2019

Uh oh!

sstone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araspitzu commented Jul 19, 2019

Uh oh!

araspitzu commented Jul 19, 2019

Uh oh!

sstone commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

araspitzu commented Jul 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

araspitzu commented Jul 8, 2019 •

edited

Loading

codecov-io commented Jul 9, 2019 •

edited

Loading

araspitzu commented Jul 11, 2019 •

edited

Loading

sstone commented Jul 19, 2019 •

edited

Loading