Rework of fee estimation #102

nepet · 2022-07-13T14:11:22Z

This is a complete rework of the fee estimation. This pr introduces an Estimator interface to allow for lnds internal and bitcoind fee estimation. The following two estimator implementations are provided:

LndEstimator: Uses lnds internal fee estimation.
GBitcoindEstimator: This estimator uses the gbitcoin api from glightning to access the bitcoind backend. The estimator mimics the behavior that lnd is using to provide a common way for fee estimation. We can set a fallback fee rate and have a constant floor fee rate of 253 sat/kw

The BitcoinOnchain service uses the new estimators that return the fee rate in sat/kw. Also a fallbackFeeRateSatPerKw and a floorFeeRateSatPerKw are provided while the former can be set on service creation, the later is a constant fixed to 275 sat/kw which equals 1.1 sat/vb.
If the fee rate returned by the estimator is below the floor, the floor will be used for estimation. The fallback is used if the estimator does return an error or an 0 fee rate.

This PR includes PRs #100 and #79

Please do not merge this PR until ElementsProject/glightning#1 got merged as it depends on my fork nepet/glightning@gbitcoin/add-public-request right now

kornpow · 2022-07-13T20:25:15Z

Im still running into the fee agreement issue while running this PR:
2022/07/13 14:19:14 rpc error: code = Unknown desc = Fee is too damn high. Max expected: 717 Received 6403
Im running LND.
Though from the discord I think this is being actively worked on.

kornpow · 2022-07-14T17:30:36Z

maybe more useful logs

2022/07/14 11:29:21 [INFO] Could not fetch on-chain fee from lnd: rpc error: code = DeadlineExceeded desc = context deadline exceeded
2022/07/14 11:29:21 [DEBUG] Estimated fee is 0, using fallback fee 0
2022/07/14 11:29:21 [INFO] Estimated fee rate is below floor of 275 sat/kw, take floor instead
2022/07/14 11:29:21 [DEBUG] Using a fee rate of 1.10 sat/vb for a total fee of 239
2022/07/14 11:29:21 [INFO] [FSM] Action failure Fee is too damn high. Max expected: 717 Received 6180
2022/07/14 11:29:21 [DEBUG] [FSM] event:id: dae7620ca9b5d18d89fa6f49aa872c1dd760f68bacd738b6dfb3b187bad57863, Event_ActionFailed on State_SwapOutSender_PayFeeInvoice
2022/07/14 11:29:21 [DEBUG] [FSM] Canceling because of Fee is too damn high. Max expected: 717 Received 6180

kornpow · 2022-07-14T21:34:24Z

I was able to perform a swap out on btc chain using this PR, and didnt hit the fee too high bug this time around

wtogami · 2022-07-14T22:56:11Z

This test error is a timeout that I suspect might be harmless? Not sure. In any case this is an improvement for LND which earlier today had broken fee estimation so it's worth testing for LND users.

nepet · 2022-07-14T23:42:22Z

Nono, I kicked ci a few times with the same error. Give me some time to understand and fix it! It now failed in the other direction concerning cln and lnd.

wtogami · 2022-07-15T00:23:30Z

Maybe the timeouts are not new? Between CLN and LND with #102 yesterday I saw some timeouts.

There's also a known bug with CLN v0.11.2 where payments can break entirely while the channel works for custommessage and ping. If we figure out a way to reproduce THAT it would be helpful.

We want to have an estimator interface that allows for multiple backend implementations to provide the same fee estimation for the cln plugin as for the lnd daemon.

nepet · 2022-07-17T18:55:54Z

If this is tested on signet/mainnet I squash the commit to make it ready to be merged

nepet · 2022-07-17T22:03:46Z

Squashed the fixups and removed the draft status.

wtogami · 2022-07-17T23:40:57Z

Visually inspecting the changes today may be confusing due to the subsequent rebase on top of master.

https://github.com/ElementsProject/peerswap/compare/test-83-97-102..test-102-jul17
https://github.com/ElementsProject/peerswap/compare/795c23a4999be67252b3d2ad227a7ae0ae757cbe..3f9864b9c49c99d2b19c87274be10f2f73c2a743
795c23a was #102 as of July 14th.
3f9864b is master with #102 of July 17th. This allows you to more easily see the changes since we last tested this.

wtogami

$ grep -r 'CONSERVATIVE'
onchain/estimator.go:	// must be one of "UNSET", "ECONOMICAL" or "CONSERVATIVE" and defaults to
onchain/estimator.go:	// "CONSERVATIVE".
onchain/estimator_test.go:		"CONSERVATIVE",

Should these be changed to ECONOMICAL as well?

wtogami · 2022-07-18T00:03:21Z

04dee90
This commit is the main change since yesterday that needs testing and review.

wtogami · 2022-07-18T00:18:14Z

If you test be sure you merge #102 into master or use 3f9864b which did so.

wtogami · 2022-07-18T09:03:31Z

ACK 04dee90

Please add one more trivial commit to get rid of the last CONSERVATIVE and merge. This is certainly a needed improvement. It would be easier for everyone to test merged.

…nterface. We use gbitcoin as our RPC proxy for bitcoind connection and calls. This estimator mimics lnds bitcoind estimator such that we will get reliable and same results for Peerswap on lnd and Peerswap on cln.

The LndEstimator uses lnds walletkit RPC EstimateFee call to fetch the fee estimation from lnds internal estimator.

We now use the Estimator interface in BitcoinOnchain to estimate the fees. The GetFee command now also has a fallback and floor fee rate where the floor is set to 1.1 sat/vb.

… estimation

…imation

This uses the current head of master that includes a PR that enables the use of the "getmempoolinfo" call.

We had no documentation on how the stubs were built. Now we can use the Makefile for stub generation.

The lnd fee estimator returns a static fee rate if lnd is used on the regtest network. In order to match this behavior for integration tests. We add the static RegtestFeeEstimator to the cln plugin that is used if bitcoind is configured to use regtest.

nepet · 2022-07-18T09:13:53Z

$ grep -r 'CONSERVATIVE'
onchain/estimator.go:	// must be one of "UNSET", "ECONOMICAL" or "CONSERVATIVE" and defaults to
onchain/estimator.go:	// "CONSERVATIVE".
onchain/estimator_test.go:		"CONSERVATIVE",

Should these be changed to ECONOMICAL as well?

Changed commits: f1fafe3, 6075df5, to address the change from "CONSERVATIVE" to "ECONOMICAL" in tests and to remove the default part in the comment.

nepet added this to the v0.3.0 milestone Jul 13, 2022

wtogami added the priority label Jul 13, 2022

nepet force-pushed the fix-fee branch from cd37fd1 to 159c777 Compare July 14, 2022 18:48

huilder and others added 3 commits July 17, 2022 20:54

Rename GetSatPerKW function. Fix LND fee unit conversion

75aee49

add minimum feerate of 1.1 sat/vbyte

3a09638

onchain: Add estimator interface that can be used for multiple backends.

ea1089f

We want to have an estimator interface that allows for multiple backend implementations to provide the same fee estimation for the cln plugin as for the lnd daemon.

nepet force-pushed the fix-fee branch from 22faddd to 30d37ae Compare July 17, 2022 18:54

nepet force-pushed the fix-fee branch from 30d37ae to 04dee90 Compare July 17, 2022 22:02

nepet changed the title ~~draft: Rework of fee estimation~~ Rework of fee estimation Jul 17, 2022

nepet requested a review from wtogami July 17, 2022 22:11

This was referenced Jul 17, 2022

Fix lnd estimate fee #100

Closed

Rename GetSatPerKW function. Fix LND fee unit conversion #79

Closed

wtogami reviewed Jul 18, 2022

View reviewed changes

nepet force-pushed the fix-fee branch from 04dee90 to 20a6818 Compare July 18, 2022 09:06

nepet added 4 commits July 18, 2022 11:10

onchain: Add tests for GBitcoinEstimator

6075df5

onchain: Add LndEstimator to estimate the fee in a lnd setup.

4ea3884

The LndEstimator uses lnds walletkit RPC EstimateFee call to fetch the fee estimation from lnds internal estimator.

onchain: Rework of the BitcoinOnchain service to use the Estimator

509857f

We now use the Estimator interface in BitcoinOnchain to estimate the fees. The GetFee command now also has a fallback and floor fee rate where the floor is set to 1.1 sat/vb.

nepet added 6 commits July 18, 2022 11:10

plugin: Fix the service creation to use the GBitcoinEstimator for fee…

b64e841

… estimation

peerswaplnd: Fix service creation to use the LndEstimator for fee est…

1fabf54

…imation

dep: Update glightning to 49a9a8e

5076ad8

This uses the current head of master that includes a PR that enables the use of the "getmempoolinfo" call.

protos: Add Makefile for stub generation

81bcdac

We had no documentation on how the stubs were built. Now we can use the Makefile for stub generation.

testframework: Use ECONOMICAL fee rate estimation on lnd node

1a0f4f3

nepet force-pushed the fix-fee branch from 20a6818 to bdd3254 Compare July 18, 2022 09:11

nepet merged commit e563b00 into ElementsProject:master Jul 18, 2022

openoms mentioned this pull request Dec 30, 2023

[NEW] add PeerSwap raspiblitz/raspiblitz#3202

Draft

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of fee estimation #102

Rework of fee estimation #102

nepet commented Jul 13, 2022

kornpow commented Jul 13, 2022

kornpow commented Jul 14, 2022

kornpow commented Jul 14, 2022

wtogami commented Jul 14, 2022

nepet commented Jul 14, 2022

wtogami commented Jul 15, 2022

nepet commented Jul 17, 2022

nepet commented Jul 17, 2022

wtogami commented Jul 17, 2022

wtogami left a comment

wtogami commented Jul 18, 2022

wtogami commented Jul 18, 2022

wtogami commented Jul 18, 2022

nepet commented Jul 18, 2022

Rework of fee estimation #102

Rework of fee estimation #102

Conversation

nepet commented Jul 13, 2022

kornpow commented Jul 13, 2022

kornpow commented Jul 14, 2022

kornpow commented Jul 14, 2022

wtogami commented Jul 14, 2022

nepet commented Jul 14, 2022

wtogami commented Jul 15, 2022

nepet commented Jul 17, 2022

nepet commented Jul 17, 2022

wtogami commented Jul 17, 2022

wtogami left a comment

Choose a reason for hiding this comment

wtogami commented Jul 18, 2022

wtogami commented Jul 18, 2022

wtogami commented Jul 18, 2022

nepet commented Jul 18, 2022