Bitcoin backend: batch fee estimation requests by darosior · Pull Request #3570 · ElementsProject/lightning

darosior · 2020-03-05T14:03:04Z

EDIT: the scope changed from just batching feerate requests, cf #3570 (comment)

niftynei

something to think about: this PR locks us into the 3 definitions of "urgent, normal, slow" in a harder to change way than the previous method of calling getfeerate 3 times. i can see how this is preferable for a plugin API.

further it canonicalizes 'urgent, normal and slow', while removing the bitcoind standard that we adhered to previously of CONSERVATIVE/2 , ECONOMICAL/100 etc. what guarantees do these new standards require? i.e. if a plugin reports back an urgent feerate, how fast does that mean it'll be mined on average? bitcoind lays these out for their feerate API, and by moving away from them we're opening up some possibility for interpretation. we should add some strong guidelines around the meaning of them in any bitcoin-plugin documentation, and perhaps annotate them in this commit series as well.

darosior · 2020-03-09T22:19:38Z

something to think about: this PR locks us into the 3 definitions of "urgent, normal, slow" in a harder to change way than the previous method of calling getfeerate 3 times. i can see how this is preferable for a plugin API.

imho it's an improvement, this classification is clearer, not backend-specific, and closer to what we use internally.

further it canonicalizes 'urgent, normal and slow', while removing the bitcoind standard that we adhered to previously of CONSERVATIVE/2 , ECONOMICAL/100 etc. what guarantees do these new standards require?

That's not a complete removal, bcli does use the same standards as before. But it gives more flexibility to the plugin if it tries to be smart.

if a plugin reports back an urgent feerate, how fast does that mean it'll be mined on average?

We completely trust the plugin for providing an urgent feerate when asked, and it doesn't change default behaviour at all as we still use CONSERVATIVE/2 for urgent feerate in bcli.

by moving away from them we're opening up some possibility for interpretation. we should add some strong guidelines around the meaning of them in any bitcoin-plugin documentation

Yep, the doc is ready and yet to come once we have the final API.
Of course I shall add that if one wants to write an exotic fee-estimation plugin, it shouldn't take the urgent feerate too lightly as it's crucial...

rustyrussell · 2020-03-10T01:54:37Z

Agree with @niftynei that these differentiations suck. We should expose them by usage.

Currently we use the following:

Mutual close (NORMAL)
Opening (NORMAL)
Unilateral (URGENT)
Minimum acceptable (SLOW / 2)
Maximum acceptable (URGENT * config.max_fee_multiplier)
onchaind: HTLC to wallet, penalty txs, returning our unilateral spend to us (NORMAL).

Perhaps we should expose these directly via the API? Frankly, returning HTLC to wallet (if timed out) should probably be more urgent, whereas the other onchaind cases should be slower.

An even better API would allow an amount and a timeout (in number of blocks), where the amount says how much we'd lose, and the timeout says how many blocks we have.

darosior · 2020-03-10T12:46:19Z

Ok.

I've looked into it and propose the following feerates :

opening
mutual_close
unilateral_close
minimum
maximum
delayed_to_us
htlc
penalty

I propose to also bump the penalty and htlc feerates to URGENT bcli-side.

darosior · 2020-03-11T00:17:30Z

Ok so now the patchset is bigger and breaks some APIs but it's really better this way, I think :).

So, this now changes our feerate tracking from urgent, normal and slow feerates to a "type-based" feerate tracking.
This also batch the fee estimation requests to the Bitcoin backend which will answer with the same estimates as before, so the estimations themselves aren't actually changed.

I propose to also bump the penalty and htlc feerates to URGENT bcli-side.

I'll propose this as an other PR, what do you think of CONSERVATIVE/3 ?
I'll also propose a PR to be able to tweak the mutual close transaction fees since it's part of the types, iirc it was a feature request ?

vasild

Code review about OK. Notice that I am missing some pieces of the big picture.

Some comments follow, feel free to ignore them (except the one about feerate[3]).

darosior · 2020-03-11T11:28:39Z

Thanks for the review @vasild ! Sorry for the obvious fails I was too eager so push that I didn't review the cherry-picked commits of the old version :/

darosior · 2020-03-11T13:29:13Z

Added two commits for handling the max_fee multiplier bcli-side.

niftynei · 2020-03-12T06:21:14Z

#3542 is related to this; see comment regarding desired API for specifying a default feerate policy for fundchannel

darosior · 2020-03-16T14:54:42Z

Corrected the wrong comments (BTC/kVB => sat/kVB)

rustyrussell

Minor comments only.

Ack b712a6f

darosior · 2020-03-23T10:19:47Z

Corrected, and rebased.

vasild · 2020-03-24T15:49:40Z

ACK dcaf75f, light code review

This allows us to set more fine-grained feerate for onchain resolution. We still give it the same feerate for all types, but this will change as we move feerates to bcli.

We kept track of an URGENT, a NORMAL, and a SLOW feerate. They were used for opening (NORMAL), mutual (NORMAL), UNILATERAL (URGENT) transactions as well as minimum and maximum estimations, and onchain resolution. We now keep track of more fine-grained feerates: - `opening` used for funding and also misc transactions - `mutual_close` used for the mutual close transaction - `unilateral_close` used for unilateral close (commitment transactions) - `delayed_to_us` used for resolving our output from our unilateral close - `htlc_resolution` used for resolving onchain HTLCs - `penalty` used for resolving revoked transactions We don't modify our requests to our Bitcoin backend, as the next commit will batch them ! Changelog-deprecated: The "urgent", "slow", and "normal" field of the `feerates` command are now deprecated. Changelog-added: The fields "opening", "mutual_close", "unilateral_close", "delayed_to_us", "htlc_resolution" and "penalty" have been added to the `feerates` command.

This adapts our fee estimations requests to the Bitcoin backend to the new semantic, and batch the requests. This makes our request for fees much simpler, and leaves some more flexibility for a plugin to do something smart (it could still lie before but now it's explicit, at least.) as we don't explicitly request estimation for a specific mode and a target. Changelog-Changed: We now batch the requests for fee estimation to our Bitcoin backend. Changelog-Changed: We now get more fine-grained fee estimation from our Bitcoin backend.

We keep the same behaviour as lightningd before.

Changelog-Changed: "htlc_timeout_satoshis" and "htlc_success_satoshis" fields have been added to the `feerates` command.

That way we pass the real min_acceptable (SLOW/2) and max_acceptable (URGENT * 10) feerates to lightningd.

darosior · 2020-03-27T12:36:20Z

Re-rebased after #3572 was merged.

rustyrussell · 2020-03-30T09:47:09Z

Ack eff7241

darosior requested a review from cdecker as a code owner March 5, 2020 14:03

darosior force-pushed the bitcoind_getfeerate_batch branch 2 times, most recently from 506703c to 79ace3e Compare March 5, 2020 17:06

niftynei reviewed Mar 5, 2020

View reviewed changes

Comment thread lightningd/bitcoind.c Outdated

Comment thread lightningd/bitcoind.c Outdated

Comment thread lightningd/bitcoind.c Outdated

Comment thread plugins/bcli.c

Comment thread lightningd/bitcoind.c

darosior force-pushed the bitcoind_getfeerate_batch branch 2 times, most recently from d02b8bb to 256cffb Compare March 6, 2020 09:36

darosior mentioned this pull request Mar 8, 2020

Bitcoin backend plugins planning #3354

Closed

darosior force-pushed the bitcoind_getfeerate_batch branch from 256cffb to 9e4102e Compare March 11, 2020 00:17

darosior commented Mar 11, 2020

View reviewed changes

Comment thread lightningd/chaintopology.c

darosior commented Mar 11, 2020

View reviewed changes

Comment thread lightningd/json.c

vasild reviewed Mar 11, 2020

View reviewed changes

darosior force-pushed the bitcoind_getfeerate_batch branch from 9e4102e to 6cda3d5 Compare March 11, 2020 11:28

darosior force-pushed the bitcoind_getfeerate_batch branch from 6cda3d5 to a48ef0e Compare March 11, 2020 12:24

darosior force-pushed the bitcoind_getfeerate_batch branch 4 times, most recently from c7c25c4 to 6dcf600 Compare March 11, 2020 21:42

darosior force-pushed the bitcoind_getfeerate_batch branch from 6dcf600 to fe98530 Compare March 12, 2020 11:10

darosior mentioned this pull request Mar 16, 2020

Bump htlc resolution transactions feerate #3592

Merged

darosior commented Mar 16, 2020

View reviewed changes

Comment thread lightningd/bitcoind.c Outdated

darosior force-pushed the bitcoind_getfeerate_batch branch from fe98530 to b712a6f Compare March 16, 2020 14:54

rustyrussell approved these changes Mar 23, 2020

View reviewed changes

Comment thread lightningd/chaintopology.c

Comment thread lightningd/chaintopology.c Outdated

Comment thread plugins/libplugin.c Outdated

Comment thread plugins/libplugin.c

darosior force-pushed the bitcoind_getfeerate_batch branch from b712a6f to dcaf75f Compare March 23, 2020 10:19

darosior added 8 commits March 27, 2020 13:21

onchaind: a feerate per transaction type

05f6a69

This allows us to set more fine-grained feerate for onchain resolution. We still give it the same feerate for all types, but this will change as we move feerates to bcli.

chaintopology: add delayed_to_us, htlc, and penalty feerates

cfba7cc

bcli: adapt interface to the new fees estimation interface

a2f4c9d

We keep the same behaviour as lightningd before.

chaintopology: Add the cost of HTLCs transactions to json_feerates

8b23168

Changelog-Changed: "htlc_timeout_satoshis" and "htlc_success_satoshis" fields have been added to the `feerates` command.

libplugin: allow u32 options

54f10d2

bcli: register --dev-max-fee-multiplier on our side

eff7241

That way we pass the real min_acceptable (SLOW/2) and max_acceptable (URGENT * 10) feerates to lightningd.

darosior force-pushed the bitcoind_getfeerate_batch branch from dcaf75f to eff7241 Compare March 27, 2020 12:35

rustyrussell merged commit 610aa0b into ElementsProject:master Mar 30, 2020

darosior deleted the bitcoind_getfeerate_batch branch March 30, 2020 11:47

This was referenced Mar 30, 2020

Adapt Sauron to the new lightningd lightningd/plugins#95

Merged

Adapt to new lightningd API clightning4j/esplora_clnd_plugin#4

Merged

Trusted coin: mind the latest API break fiatjaf/lightningd-gjson-rpc#3

Closed

Conversation

darosior commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darosior commented Mar 9, 2020

Uh oh!

rustyrussell commented Mar 10, 2020

Uh oh!

darosior commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild 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!

darosior commented Mar 11, 2020

Uh oh!

darosior commented Mar 11, 2020

Uh oh!

niftynei commented Mar 12, 2020

Uh oh!

Uh oh!

darosior commented Mar 16, 2020

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darosior commented Mar 23, 2020

Uh oh!

vasild commented Mar 24, 2020

Uh oh!

darosior commented Mar 27, 2020

Uh oh!

rustyrussell commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

darosior commented Mar 5, 2020 •

edited

Loading

darosior commented Mar 10, 2020 •

edited

Loading

darosior commented Mar 11, 2020 •

edited

Loading