Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openingd: Allow users to set the enforced reserve for their counterparty (zeroreserve) #5315

Merged
merged 16 commits into from
Sep 21, 2022

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jun 9, 2022

Based on top of #5275, this PR starts at commit f7d0827

Until now we always applied the default reserve of 1% of the funding amount, however there has been considerable demand to being able to adjust these values. In particular zeroreserve, i.e., not enforcing any minimal amount in the channel from the counterparty, has been an often requested feature. This PR implements the zeroreserve proposal, in two variants (see second to last commit):

  • The first variant implements the prescribed checks from the spec, ensuring that both the remote and the local fees are above the dust limit, resulting in some dust and at least one forced output for both sides should they have ever reached a sufficiently high balance
  • The second variant skips the prescribed checks, allowing true 0 reserve, and users can drain their channels completely. This is a more trusted setup since the party that gets permission from the other side to fully drain their channel can replay an old state without repercussions

The implementation itself is done via a new reserve argument to the single funding RPC commands, and by adding a new reserve return argument to the openchannel hook. The values are absolute satoshi values, not percentages.

Using arguments and hooks allows the caller or a plugin to selectively opt into allowing the counterparty to drain the channel, based on the nodeid, amount or any other information in the openchannel hook (potentially including dynamic lookups). I think this is the best way to allow users to selectively opt-in without them automatically allowing everybody.

Either way a stern warning about the trust involved should be added to the docs.

Open questions

  • I guess the big one is do we feel ok allowing sub-dust reserves, should it be a user option, and if it's ok, should we propose a spec amendment removing those checks?
    • Amending the spec may require a featurebit since at the moment we actually enforce the sub-dust check for the reserve our peer imposes on us too, making this a breaking change that could fail the channel funding flow. Then again, we presume some level of coordination and trust before attempting to open a zeroreserve channel, so that breakage might be ok.

@cdecker cdecker added this to the v0.12 milestone Jun 9, 2022
@cdecker cdecker marked this pull request as draft June 9, 2022 15:19
@cdecker cdecker force-pushed the zeroreserve branch 3 times, most recently from 2c5c6c9 to e6f87a3 Compare June 9, 2022 15:57
@cdecker cdecker force-pushed the zeroreserve branch 3 times, most recently from 1cb92f7 to f71cff3 Compare July 1, 2022 12:21
@cdecker cdecker marked this pull request as ready for review July 1, 2022 12:47
@cdecker cdecker force-pushed the zeroreserve branch 12 times, most recently from d139a7c to 811db90 Compare July 7, 2022 10:11
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, generally approve of this, but I think we could replace the dev switches with a generic channel-size check. You can defer that change until next release if you want though?

We should mention the ability to zeroreserve in the Changelog, if not explicitly called already (I didn't see it, but then GH doesn't exactly highlight commit msgs!)

lightningd/opening_control.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
lightningd/lightningd.h Show resolved Hide resolved
contrib/pyln-client/pyln/client/lightning.py Show resolved Hide resolved
@cdecker cdecker force-pushed the zeroreserve branch 5 times, most recently from 4a8f4eb to 25914c2 Compare August 17, 2022 07:43
@cdecker cdecker force-pushed the zeroreserve branch 2 times, most recently from 6788f83 to 8ac3eb7 Compare September 12, 2022 12:24
@cdecker cdecker force-pushed the zeroreserve branch 3 times, most recently from 315813e to 561ccc9 Compare September 19, 2022 16:42
Changelog-Added: JSON-RPC: `fundchannel`, `multifundchannel` and `fundchannel_start` now accept a `reserve` parameter to indicate the absolute reserve to impose on the peer.
Changelog-Added: plugin: The `openchannel` hook may return a custom absolute `reserve` value that the peer must not dip below.
In the case of the local channel we set the estimation to the exact
value spendable, which is important when we want to drain a channel,
because there we actually want to get the last msat.
This is incompatible with the spec as it removes the enforcement for
reserves being above dust, but from what I can see from other
implementations it seems that others have allowed this as well.

This commit just guards the necessary changes with compilation guards, so
we can decide either way quickly. This part of the PR is not intended
to be final, just as a discussion basis.
Technically this is a non-conformance with the spec, hence the `dev`
flag to opt-in, however I'm being told that it is also implemented in
other implementations. I'll follow this up with a proposal to the spec
to remove the checks we now bypass.
This check, while in line with the specification, would cause issues
in mixed setups when the funder or fundee allows dust reserves, but
the counterparty does not. It is not an issue for the non-dust reserve
node since in this case it's the peer giving us more flexibility not
the other way around.
Technically this is a non-conformance with the spec, hence the `dev`
flag to opt-in, however I'm being told that it is also implemented in
other implementations. I'll follow this up with a proposal to the spec
to remove the checks we now bypass.
Tests that we can have zeroreserve nodes accept and open new channels
with non-zeroreserve nodes, without throwing errors
It means we consume the channel completely to the best of our
knowledge, so let that through.

Changelog-Fixed: pay: Squeezed out the last `msat` from our local view of the network
This just shows we need to add that constraint.

Reported-by: Rusty Russell <@rustyrussell>
The scale variants weren't usable since they use `double` as a scale
factor, which in turn causes imprecisions.
Prior to this we might end up with a commitment transaction without
any outputs, if combined with `--dev-allowdustreserve`. Otherwise the
reserve being larger than dust means the funder could not drop its
direct output to be below dust.

Reported-by: Rusty Russell <@rustyrussell>
@cdecker
Copy link
Member Author

cdecker commented Sep 21, 2022

ACK afed054

@cdecker cdecker merged commit 774d16a into ElementsProject:master Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants