Configurable transaction confirmation target by araspitzu · Pull Request #1083 · ACINQ/eclair

araspitzu · 2019-07-20T14:30:24Z

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

…emove unused 'smartfeeNBlocks'

sstone

Nice to get rid of all these calls to Globals !

eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala

…ets` to fr.acinq.blockchain.fee.FeeEstimator.

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/fee/FeeEstimator.scala

eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala

pm47 · 2019-07-23T09:38:10Z

HTLC-success transaction: blockTarget=2 (hardcoded)
HTLC-timeout transaction: blockTarget=2 (hardcoded)
Main penalty transaction: blockTarget=2 (hardcoded)
HTLC penalty transaction: blockTarget=2 (hardcoded)

@t-bast I think this is the value we were looking for for #1079.

t-bast · 2019-07-23T09:47:01Z

@t-bast I think this is the value we were looking for for #1079.

Do you think we should hard-code this for 1079? I think it's better to keep it configurable, because this is linked to the amount of time you can expect to be safely offline, so if you are on a not-so-reliable node (raspberry pi for example) you might want to use a value higher than 2 here.

pm47 · 2019-07-23T09:51:57Z

Do you think we should hard-code this for 1079?

Not necessarily, it's just that now we have a rationale for the default value. OTOH changing the default commitment target is definitely risky.

pm47

Nice change. As a side effect, with the removal of global access to feerates we are one step closer to being able to run tests in parallel.

I think we should probably get rid of the global Alice and Bob parameters. They are not needed since we can always retrieve the actual parameters from TestActorRef.underlyingActor.nodeParams. Maybe we could replace them with methods like makeAliceParams to discourage using them directly. NB: that would be a different PR.

eclair-core/src/main/resources/reference.conf

eclair-core/src/main/scala/fr/acinq/eclair/Setup.scala

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/fee/FeeProvider.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

pm47 · 2019-07-23T10:03:00Z

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala


+    def firstClosingFee(commitments: Commitments, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, feeEstimator: FeeEstimator, feeTargets: FeeTargets)(implicit log: LoggingAdapter): Satoshi = {
+      // we "MUST set fee_satoshis less than or equal to the base fee of the final commitment transaction"
+      val blockTarget = Math.max(feeTargets.mutualCloseBlockTarget, feeTargets.commitmentBlockTarget)


We need to compare feerates, not block targets.

We are comparing feerates, this code is the result of adapting:

// no need to use a very high fee here, so we target 6 blocks; also, we "MUST set fee_satoshis less than or equal to the base fee of the final commitment transaction" val feeratePerKw = Math.min(Globals.feeratesPerKw.get.blocks_6, commitments.localCommit.spec.feeratePerKw)

into

// we "MUST set fee_satoshis less than or equal to the base fee of the final commitment transaction" val blockTarget = Math.max(feeTargets.mutualCloseBlockTarget, feeTargets.commitmentBlockTarget) val feeratePerKw = feeEstimator.getFeeratePerKw(blockTarget)

It's just a different way to extract the feerate, note that the Math.min of the 2 feerates has become Math.max of the confirmation target because the higher the target the lower the feerate.

I understand what you did, but it doesn't work: there is no guarantee that the resulting fee will be lesser than the one used in the last commitment, because fees may vary widely. We must compare to commitments.localCommit.spec.feeratePerKw.

That's correct the resulting feerate must be compared with the one used in the commitment transaction and it's not the case currently.

In 419bf60 i used Math.min(feeEstimator.getFeeratePerKw(blockTarget), commitments.localCommit.spec.feeratePerKw) to do the proper check.

Right, but the line above with the max on block targets is misleading. This conveys the intent better:

val requestedFeerate = feeEstimator.getFeeratePerKw(feeTargets.mutualCloseBlockTarget) // we "MUST set fee_satoshis less than or equal to the base fee of the final commitment transaction" val feeratePerKw = Math.min(requestedFeerate, commitments.localCommit.spec.feeratePerKw)

Sounds good to me, but note that with this change if mutualCloseBlockTarget < commitmentBlockTarget we are (likely) to make a spec violation, I propose to have a check done in the NodeParam to avoid that.

@pm47 Nevermind my comment above, the suggested change is correct.

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

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala

Add a warning to commitment confirmation target configuration key Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>

codecov-io · 2019-07-23T10:52:48Z

Codecov Report

Merging #1083 into master will increase coverage by 0.05%.
The diff coverage is 85.39%.

@@            Coverage Diff             @@
##           master    #1083      +/-   ##
==========================================
+ Coverage    82.7%   82.76%   +0.05%     
==========================================
  Files         101      101              
  Lines        7621     7652      +31     
  Branches      293      309      +16     
==========================================
+ Hits         6303     6333      +30     
- Misses       1318     1319       +1

Impacted Files	Coverage Δ
.../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.78% <100%> (+0.87%)`	⬆️
...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%> (+0.3%)`	⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala	`87.02% <100%> (ø)`	⬆️
...a/fr/acinq/eclair/blockchain/fee/FeeProvider.scala	`60% <44.44%> (-40%)`	⬇️
... and 8 more

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala # eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

araspitzu · 2019-07-23T11:41:24Z

Latest commit 1c21959 makes use of the feeEstimator from the underlying actor in all the test

araspitzu · 2019-07-23T15:41:01Z

Summary of latest changes:

All on-chain fee related configurations have been grouped together, both in the configuration file and in NodeParams under the OnChainFeeConf object.
There are now helper functions to extract the on-chain fee conf during the test (this avoids lengthy alice.underlyingActor.nodeParams.feeEstimator)
Default confirmation target for claim transactions is now 12 blocks

eclair-core/src/main/resources/reference.conf

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/StateTestsHelperMethods.scala

eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

…he commitment fee and the configured fee target

…, move comment closer to relevant line.

pm47 · 2019-07-25T15:13:58Z

eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala

                      pingDisconnect: Boolean,
-                      maxFeerateMismatch: Double,
-                      updateFeeMinDiffRatio: Double,
+                      onChainFeeConf: OnChainFeeConf,


Last nit: onChainFeeConf is lost in between peer-specific params, can you move it somewhere where it makes more sense?

Done in 36a8d5e

araspitzu · 2019-07-25T16:04:31Z

Sorry for discarding the review but commit 36a8d5e set the wrong maxFeerateMismatch on TestConstant.Bob, it has been fixed in 8f1bcc0

araspitzu added 9 commits July 19, 2019 17:52

Use feeEstimator in NodeParams with custom confirmationTarget

4293514

Introduce FeeConf object and config block for confirmation targets, r…

5f4ee9a

…emove unused 'smartfeeNBlocks'

Use a custom confirmation target for commitment transaction

527499b

Use a custom confirmation target for funding transaction

dade6fd

Use custom confirmation target for mutual close transaction

3e4ad7a

Use custom confirmation target for claim transactions

214494b

Add confirmation target block 144

de55d92

Fix bitgo fee provider test

83763ef

Add copyright header to FeeEstimator.scala

7ec7e3a

araspitzu mentioned this pull request Jul 20, 2019

Configurable fee target #1066

Closed

sstone requested changes Jul 22, 2019

View reviewed changes

Reorg parametres order, pass FeeEstimator from Setup, move `FeeTarg…

f0649dd

…ets` to fr.acinq.blockchain.fee.FeeEstimator.

sstone requested changes Jul 23, 2019

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/fee/FeeEstimator.scala Show resolved Hide resolved

eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala Show resolved Hide resolved

Formatting

a04eed8

pm47 requested changes Jul 23, 2019

View reviewed changes

Update eclair-core/src/main/resources/reference.conf

45100ad

Add a warning to commitment confirmation target configuration key Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>

araspitzu added 3 commits July 23, 2019 12:59

Use underlyingActor to access NodeParams and feeEstimator during test

7e31ebb

Change position order of receiveFee parameters

c66b3b8

Merge branch 'master' into custom_confirmation_target

d577d07

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala # eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

Use feeEstimator from underlyingActor

1c21959

araspitzu force-pushed the custom_confirmation_target branch from 8856051 to 1c21959 Compare July 23, 2019 14:01

araspitzu added 3 commits July 23, 2019 17:34

Group together on-chain fees related configurations

b652fb6

Use helper functions to get 'OnChainFeeConf' during test

4cf6359

Use block target = 12 as default for claim transactions

a75a931

pm47 requested changes Jul 23, 2019

View reviewed changes

sstone requested changes Jul 23, 2019

View reviewed changes

eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala Outdated Show resolved Hide resolved

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala Outdated Show resolved Hide resolved

araspitzu added 7 commits July 24, 2019 10:01

Move default-feerates configuration block inside on-chain-fees, renaming

f768ec5

Remove helper methods in favor of pimp pattern

9da4996

Merge branch 'master' into custom_confirmation_target

e16763d

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

Rename default-feerates section, remove blank line

5db1271

Move OnChainFeeConf to FeeEstimator.scala

d240dfe

When computing the first mutual-closing fee use the minimum between t…

419bf60

…he commitment fee and the configured fee target

Do not compare confirmation targets when choosing a 'firstClosingFee'…

f350b87

…, move comment closer to relevant line.

pm47 reviewed Jul 25, 2019

View reviewed changes

pm47 previously approved these changes Jul 25, 2019

View reviewed changes

Move OnChainFeeConf value close to other on-chain fee related value

8f1bcc0

araspitzu dismissed pm47’s stale review via 8f1bcc0 July 25, 2019 16:03

araspitzu force-pushed the custom_confirmation_target branch from 36a8d5e to 8f1bcc0 Compare July 25, 2019 16:03

sstone approved these changes Jul 25, 2019

View reviewed changes

araspitzu merged commit 131f50a into master Jul 25, 2019

araspitzu deleted the custom_confirmation_target branch July 25, 2019 17:06

pm47 mentioned this pull request Aug 29, 2019

Make tests run in parallel #1112

Merged

Conversation

araspitzu commented Jul 20, 2019

Summary of changes

Rationale

High level overview

Implementation details

Transactions and block target

Uh oh!

sstone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pm47 commented Jul 23, 2019

Uh oh!

t-bast commented Jul 23, 2019 • edited by araspitzu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pm47 commented Jul 23, 2019

Uh oh!

pm47 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

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!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

araspitzu commented Jul 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 25, 2019

Uh oh!

t-bast commented Jul 23, 2019 •

edited by araspitzu

Loading

pm47 left a comment •

edited

Loading

pm47 Jul 25, 2019 •

edited

Loading

codecov-io commented Jul 23, 2019 •

edited

Loading

araspitzu commented Jul 23, 2019 •

edited

Loading