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

Add a new min-funding config parameter for private channels #2203

Merged
merged 4 commits into from
Mar 11, 2022

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Mar 10, 2022

Add the ability to set a different min-funding limit for private and public channels. See issue "Private channels should have their own dedicated 'min-funding-satoshis' setting" #2077.

If channel.private.enable-alt = true then the channel.private.min-funding-satoshis value will be used for private channels, otherwise the channel.min-funding-satoshis will be used for both public and private channels.

A few questions:

  • previous min-funding-satoshis tests compared assert(error.toAscii === Error(...).toAscii) while other tests in WaitForOpenChannelStateSpec left off the .toAscii. Is there a reason to use the .toAscii in the assert here?
  • Issue 2077 mentioned we should document that users should not set unsafely low min-funding limits for private channels. Only if users set their public channels to have higher limits then they might allow private channels with a lower, but still safe, min-funding-satoshis. Should I add documentation to that effect in docs\Configure.md ? or in the release notes ?

@remyers
Copy link
Contributor Author

remyers commented Mar 10, 2022

My rational for the channel.private.enable-alt is that when people upgrade their node it should default to have the same min-funding behavior as before. If we just added a new setting for private channels then it would use whatever default was in the reference.conf and that would potentially be very different than what the user expects their private channels to use.

I'm not completely happy with the enable-alt as the name for that parameters - open to suggestions.

@remyers remyers changed the title Minfundingprivate new min-funding config parameter for private channels Mar 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #2203 (6534977) into master (f3c6c78) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2203      +/-   ##
==========================================
- Coverage   83.89%   83.88%   -0.02%     
==========================================
  Files         186      186              
  Lines       13937    13940       +3     
  Branches      545      580      +35     
==========================================
  Hits        11693    11693              
- Misses       2244     2247       +3     
Impacted Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 92.77% <100.00%> (+0.05%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 87.81% <100.00%> (+0.08%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 94.97% <100.00%> (ø)
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 93.10% <0.00%> (-3.45%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 72.84% <0.00%> (-1.33%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 85.60% <0.00%> (-0.76%) ⬇️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.76% <0.00%> (+0.54%) ⬆️

@thomash-acinq
Copy link
Member

I understand your rationale for channel.private.enable-alt but I really don't like the result.
An alternative would be to rename min-funding-satoshis to min-public-funding-satoshis and add a deprecation message for min-funding-satoshis, that way everyone using this parameter will see the message and will be forced to manually change it.

@pm47
Copy link
Member

pm47 commented Mar 10, 2022

@thomash-acinq beat me to it, indeed it's the way to go:

@remyers
Copy link
Contributor Author

remyers commented Mar 11, 2022

I understand your rationale for channel.private.enable-alt but I really don't like the result. An alternative would be to rename min-funding-satoshis to min-public-funding-satoshis and add a deprecation message for min-funding-satoshis, that way everyone using this parameter will see the message and will be forced to manually change it.

I agree. As long as we mention both new options in the deprecation message that should ensure they know to set both the public and private min funding values.

@remyers remyers marked this pull request as ready for review March 11, 2022 10:37
Comment on lines 85 to 86
min-public-funding-satoshis = 100000 // high public minimums are attractive to pathfinding heuristics
min-private-funding-satoshis = 100000 // safe private minimums increase the cost of spamming and flood-and-loot attacks
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these comments are really helpful. Larger channels are more attractive but that doesn't have anything to do with the minimum we set here, a small channel is more attractive than no channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed them in 6534977. Sounds like its too nuanced to explain in comments.

@remyers remyers merged commit f14300e into ACINQ:master Mar 11, 2022
@thomash-acinq
Copy link
Member

I've just remembered about the release notes, can you update them too?

@pm47
Copy link
Member

pm47 commented Mar 15, 2022

Also, for PR names and squashed commit messages please use a short sentence starting by a verb with an uppercase.

@remyers remyers changed the title new min-funding config parameter for private channels Add a new min-funding config parameter for private channels Mar 17, 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.

4 participants