`funder` cleanups! #5650

niftynei · 2022-10-07T20:42:47Z

I've been dying to clean up some things in funder that are/were subpar. Here they are!

We used to not send back original lease data in RBF attempts, which meant if you RBF'd a channel w/ a lease on it, the lease request would rather silently disappear. Oops. Now we attempt to satisfy them on RBFs as well.
We didn't used to re-use the utxos from the original tx on RBF, now we do.

I kinda think that's it? That might be it.

cdecker

Excellent cleanups 👍

ACK 3a98aa4

cdecker · 2022-10-12T12:22:50Z

lightningd/dual_open_control.c

+	/* iiuc deserialized gets called multiple times? */
+	if (!payload->rates)
+		payload->rates = tal(payload, struct lease_rates);


Yes, once for each plugin that gets called :-)

cdecker · 2022-10-14T14:30:03Z

plugins/funder.c

+
+	/* Next add available utxos until we surpass the
+	 * requested funds goal */
+	/* FIXME: Update `utxopsbt` to automatically add more inputs? */


Could be an extra RPC method called fundpsbt to mirror bitcoind's API.

We already have a fundpsbt that behaves slightly differently.

cdecker · 2022-10-14T14:35:55Z

Seems there is one json scan that doesn't always match the payload:

`openchannel2` payload did not scan Parsing '{openchannel2:{id:%,channel_id:%,their_funding_msat:%,max_htlc_value_in_flight_msat:%,htlc_minimum_msat:%,funding_feerate_per_kw:%,commitment_feerate_per_kw:%,feerate_our_max:%,feerate_our_min:%,to_self_delay:%,max_accepted_htlcs:%,channel_flags:%,locktime:%,channel_max_msat:': object does not have member channel_max_msat: {"openchannel2":{"id":"0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518","channel_id":"252d1b0a1e57895e84137f28cf19ab2c35847e284c112fefdecc7afeaa5c1de7","their_funding_msat":16777216000,"dust_limit_msat":546000,"max_htlc_value_in_flight_msat":18446744073709551615,"htlc_minimum_msat":0,"funding_feerate_per_kw":7500,"commitment_feerate_per_kw":7500,"feerate_our_max":150000,"feerate_our_min":1875,"to_self_delay":5,"max_accepted_htlcs":483,"channel_flags":1,"locktime":102}}

The offending field is channel_max_msat

We were leaving out the `channel_max_msat` for `openchannel2` when channels are 'wumbo' (the conversion to msat in the json helper overflowed, which resulted in the field not being printed) Changelog-Changed: Plugins: `openchannel2` now always includes the `channel_max_msat`

Parse the channel_max along with everything else.

We're gonna need it for rbf requests/re-negotiations

if they request less than we wanted/accepted FIXME: add a test for this?

If more than one plugin calls `openchannel2`, the payload will get re-freshed/re-parsed.

let's let RBFs know about our lease info!

We're going to need to know what utxos we used if we RBF this channel; so we serialize our inputs and save them to the datastore under the channel_id.

It'd be nice to know which utxos we used previously, so we can rebuild a transaction using them!

Still need to use them to build the PSBT for the rbf however.

We use the saved previous outputs (plus maybe some new ones?) to build a psbt for an RBF request. RBFs utxo reuse is now working so we can unfail the test (and update it to reflect that the lease sticks around through an RBF cycle). Changelog-Fixed: Plugins: `funder` now honors lease requests across RBFs

If for some reason a utxo we used previously is no longer 'unspent', we shouldn't use it for the next transaction.

Let's not leave old state hanging around! Note that this fires for pretty much every/any channel (even if we're not the opener).

niftynei · 2022-10-17T19:57:01Z

Seems there is one json scan that doesn't always match the payload:

`openchannel2` payload did not scan Parsing '{openchannel2:{id:%,channel_id:%,their_funding_msat:%,max_htlc_value_in_flight_msat:%,htlc_minimum_msat:%,funding_feerate_per_kw:%,commitment_feerate_per_kw:%,feerate_our_max:%,feerate_our_min:%,to_self_delay:%,max_accepted_htlcs:%,channel_flags:%,locktime:%,channel_max_msat:': object does not have member channel_max_msat: {"openchannel2":{"id":"0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518","channel_id":"252d1b0a1e57895e84137f28cf19ab2c35847e284c112fefdecc7afeaa5c1de7","their_funding_msat":16777216000,"dust_limit_msat":546000,"max_htlc_value_in_flight_msat":18446744073709551615,"htlc_minimum_msat":0,"funding_feerate_per_kw":7500,"commitment_feerate_per_kw":7500,"feerate_our_max":150000,"feerate_our_min":1875,"to_self_delay":5,"max_accepted_htlcs":483,"channel_flags":1,"locktime":102}}

The offending field is channel_max_msat

Oh fun. We don't include channel_max_msat if wumbo is on because we set it to UINT_MAX, which fails to mulitply by 1000 to convert to sats when adding it to the openchannel2 JSON.

Added a new commit, b6ef966, which now uses a non-overflow-ing number for channel_max_msat!

vincenzopalazzo

ACK ebcc865

niftynei force-pushed the nifty/funder-rbf-ok branch from b0292c7 to 3a98aa4 Compare October 11, 2022 22:30

cdecker self-assigned this Oct 12, 2022

cdecker added this to the v22.11 milestone Oct 12, 2022

cdecker approved these changes Oct 14, 2022

View reviewed changes

niftynei added 13 commits October 17, 2022 14:54

funder: we always pass in channel_max, no need to special case it

2506347

Parse the channel_max along with everything else.

df: put requested_lease onto state, so it persists

dba991a

We're gonna need it for rbf requests/re-negotiations

df: for rbfs, since we know what they asked for, we can abort

c7cbe3b

if they request less than we wanted/accepted FIXME: add a test for this?

openchannel2: may re-use rates

0482ad2

If more than one plugin calls `openchannel2`, the payload will get re-freshed/re-parsed.

df: pass lease data back to funder for rbfs

a5ef12f

let's let RBFs know about our lease info!

funder: rm quote that makes nifty cringe every time she sees it

c6470fa

funder: save utxos of signed txs to datastore

3d8466e

We're going to need to know what utxos we used if we RBF this channel; so we serialize our inputs and save them to the datastore under the channel_id.

funder: pull out previous input list from datastore on RBF

0fbf1b5

It'd be nice to know which utxos we used previously, so we can rebuild a transaction using them!

funder: use previous outputs in count towards available funding

edc907e

Still need to use them to build the PSBT for the rbf however.

funder: filter prev-outs such that we only use still unspent ones

2adb06c

If for some reason a utxo we used previously is no longer 'unspent', we shouldn't use it for the next transaction.

funder: cleanup datastore on state-change/channel failure

ebcc865

Let's not leave old state hanging around! Note that this fires for pretty much every/any channel (even if we're not the opener).

niftynei force-pushed the nifty/funder-rbf-ok branch from 3a98aa4 to ebcc865 Compare October 17, 2022 19:56

niftynei mentioned this pull request Oct 19, 2022

dual funding -> API changes/breakage #5670

Merged

vincenzopalazzo approved these changes Oct 20, 2022

View reviewed changes

cdecker merged commit e008578 into ElementsProject:master Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`funder` cleanups! #5650

`funder` cleanups! #5650

niftynei commented Oct 7, 2022

cdecker left a comment

cdecker Oct 12, 2022

cdecker Oct 14, 2022

niftynei Oct 17, 2022

cdecker commented Oct 14, 2022 •

edited

Loading

niftynei commented Oct 17, 2022

vincenzopalazzo left a comment

funder cleanups! #5650

funder cleanups! #5650

Conversation

niftynei commented Oct 7, 2022

cdecker left a comment

Choose a reason for hiding this comment

cdecker Oct 12, 2022

Choose a reason for hiding this comment

cdecker Oct 14, 2022

Choose a reason for hiding this comment

niftynei Oct 17, 2022

Choose a reason for hiding this comment

cdecker commented Oct 14, 2022 • edited Loading

niftynei commented Oct 17, 2022

vincenzopalazzo left a comment

Choose a reason for hiding this comment

`funder` cleanups! #5650

`funder` cleanups! #5650

cdecker commented Oct 14, 2022 •

edited

Loading