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

config: improve management of htlc amount lower limit #3697

Merged
merged 10 commits into from Dec 11, 2019

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Nov 9, 2019

The minimum htlc amount is set when a channel is opened and thereafter it isn't possible anymore to change it. The other end of the channel will never be able to advertize a htlc_minimum_msat forwarding policy below this lower bound.

Until now, the default value was 1000 msat. Users who are not aware of the importance of this minimum value when opening the channel, may inadvertently contribute to a Lightning Network that is not able to carry sub-satoshi payments. Also when accepting a channel open request, this default value is used.

In this commit, the minimum htlc amount is lowered to 1 msat. Changing the default will not force nodes to forward lower amount payments. If the forward doesn't satisfy the node's htlc_minimum_msat policy of the outgoing channel, the htlc is still canceled back. It
remains possible to set htlc_minimum_msat to any desired value and will still default to 1000 msat.

This commit improves the capability of the network to facilitate sub-satoshi (streaming) micro payments without reducing the level of control that node operators have over the htlcs they forward.

Furthermore the rpc UpdateChanPolicy is extended with the ability set the min htlc value.

@joostjager joostjager self-assigned this Nov 9, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 3 times, most recently from 9351bc2 to 30363dd Nov 9, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 13, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 13, 2019
@joostjager joostjager added the v0.9.0 label Nov 13, 2019
@joostjager joostjager requested review from Roasbeef and wpaulino Nov 13, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 13, 2019
Copy link
Collaborator

wpaulino left a comment

Should we consider allowing this value to be modified through UpdateChannelPolicy as a follow-up?

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 13, 2019

The default effect seems real:

image

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 13, 2019

Should we consider allowing this value to be modified through UpdateChannelPolicy as a follow-up?

So this value isn't the regular min_htlc that is advertized on the network. This is the absolute minimum that a channel will ever see in its life. After opening, it cannot be changed anymore.

The benefit of setting this to anything other than 0 (or 1) msat seems very limited and it does reduce the flexibility.

Also, if node A sets the minimum on channel open, this has consequences for node B. node B won't be able to forward anything below A's min_htlc channel parameters. It won't affect A's policy towards B.

For that reason, LND nodes connected to CL advertize a min_htlc channel policy of 0. CL has no minimum by default (or maybe it isn't even possible to set it).

Adding min_htlc setting to UpdateChannelPolicy is useful, but it isn't linked to this PR as much as one might expect.

@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch from 30363dd to 04aa7a6 Nov 15, 2019
@joostjager joostjager requested review from cfromknecht and halseth as code owners Nov 15, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 2 times, most recently from 8232001 to f46d5f9 Nov 15, 2019
@joostjager joostjager removed request for cfromknecht and halseth Nov 15, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch from f46d5f9 to c2baace Nov 17, 2019
@joostjager joostjager requested a review from wpaulino Nov 18, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 18, 2019

@wpaulino re-requesting review because I changed significantly more to separate fwd policy from the min htlc channel parameter.

@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 2 times, most recently from e37799e to 8e5a9f2 Nov 19, 2019
@joostjager joostjager changed the title config: lower default minimum htlc amount config: lower default minimum htlc amount and allow setting min_htlc forwarding Nov 19, 2019
@joostjager joostjager changed the title config: lower default minimum htlc amount and allow setting min_htlc forwarding config: improve management of htlc amount lower limit Nov 19, 2019
htlcswitch/link.go Show resolved Hide resolved
pilot.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
routing/localchans/manager.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 3 times, most recently from 2580009 to 8fcec6f Nov 22, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch from 8fcec6f to f5a6ddf Nov 26, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 2 times, most recently from f8281d1 to 60cc0a4 Dec 4, 2019
@@ -2209,6 +2209,12 @@ func (f *fundingManager) addToRouterGraph(completeChan *channeldb.OpenChannel,
// need to determine the smallest HTLC it deems economically relevant.
fwdMinHTLC := completeChan.LocalChanCfg.MinHTLC

// We don't necessarily want to go as low as the remote party

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 7, 2019

Member

Not clear to me why we need to clamp this...

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 7, 2019

Author Collaborator

This is the change that you requested. Not to just use the remote's min htlc in our forwarding policy, but apply our default instead. I'd rather not make this change though and stay closer to what people are used to. Let me know your final opinion on it.

//
// All forwarded payments are subjected to the min htlc constraint of
// the routing policy of the outgoing channel. This implicitly controls
// the minimum htlc value on the incoming channel too.
defaultBitcoinMinHTLCInMSat = lnwire.MilliSatoshi(1000)
defaultBitcoinMinHTLCInMSat = lnwire.MilliSatoshi(0)

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 7, 2019

Member

Why would accepting a zero amount HTLC ever be useful? I'm not even sure we have any tests to exercise if it'll even flow as normal for a point-to-point (direct, no hops) payment.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 7, 2019

Author Collaborator

The reason for this is pretty far-fetched. It would prepare for a future where 0 sat payments are possible. It seemed safe to me, as CL is also setting this value to zero. But the direct payment case I didn't consider. Changed to 1 msat.

lnrpc/rpc.proto Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 2 times, most recently from 9b2fcc3 to a28c4c1 Dec 7, 2019
@joostjager joostjager requested a review from Roasbeef Dec 7, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 10, 2019

Needs rebase.

@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch from a28c4c1 to 1326915 Dec 10, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Dec 10, 2019

rebased

@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch 2 times, most recently from 5263faa to e425022 Dec 10, 2019
joostjager added 10 commits Nov 22, 2019
@joostjager joostjager force-pushed the joostjager:lower-min-htlc branch from e425022 to 61e114f Dec 10, 2019
@Roasbeef Roasbeef merged commit 70c5fe3 into lightningnetwork:master Dec 11, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 62.183%
Details
v0.9.0-beta automation moved this from Needs Review to Done Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
3 participants
You can’t perform that action at this time.