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: lower outgoing htlc broadcast delta #2887

Merged
merged 3 commits into from May 3, 2019

Conversation

Projects
None yet
4 participants
@joostjager
Copy link
Collaborator

commented Apr 4, 2019

Builds on top of #2879.

We create outgoing htlcs and accept incoming htlcs within the go-to-chain window, risking triggering a force close. This PR updates the block deltas to make sure that we don't trigger a force close ourselves.

The main change is lowering the broadcast delta for outgoing htlcs. It can be lower than the delta for incoming htlcs, as we don't risk not being able to settle with the preimage if the tx(es) don't confirm in time.

The spec even suggests a delta of -1 for outgoing htlcs (https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#cltv_expiry_delta-selection). Meaning that we only go to chain in the block after the block where the htlc expires.

This PR doesn't lower it to -1, but uses 0 instead to prevent confusion around the negative number. This means that we go to chain when an outgoing htlc expires.

OutgoingCltvRejectDelta is a few blocks more than OutgoingBroadcastDelta. This is to prevent us forwarding an htlc that would bring us inside the broadcast window. We would probably receive a settle or fail quickly, but if a new block arrives in the mean time, we'd force close the channel.

FinalCltvRejectDelta could have been equal to IncomingBroadcastDelta, because as an exit hop we can immediately decide what to do with the incoming htlc.

This PR does increase FinalCltvRejectDelta a bit because for hodl invoices we do not decide immediately. Eventually, for hodl invoices, there will be automatic cancellation to prevent channel closure. This automatic cancellation should trigger some blocks before we go to chain. We can probably use the same value FinalCltvRejectDelta and the delta at which we cancel back hodl invoice htlcs.

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2019

Comments from @halseth

  • Add unit test to assert spec compliance of delta constant
  • Reword comment on IncomingBroadcastDelta in chain_arbitrator.go
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2019

Reminder: Also need to check the outgoing expiry when we are the sender of the payment. In that case, HtlcSatisfyPolicy isn't called.

@joostjager joostjager force-pushed the joostjager:asym-broadcast-delta branch from 5f70e4f to 66e92e8 Apr 6, 2019

@cfromknecht cfromknecht added this to the 0.7 milestone Apr 11, 2019

@Roasbeef Roasbeef modified the milestones: 0.7, 0.6.1 Apr 12, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

# github.com/lightningnetwork/lnd [github.com/lightningnetwork/lnd.test]
./lnd_test.go:9473:33: constant -4 overflows uint32

@joostjager joostjager force-pushed the joostjager:asym-broadcast-delta branch 4 times, most recently from db69a27 to 2f9e758 Apr 19, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 19, 2019

@cfromknecht ptal

Implemented policy check for local htlcs.

Didn't implement unit test to assert spec compliance. It doesn't look very useful to me to just check that a constant has a value. Instead I added extensive comments to the config constants which should make future devs think about what these values mean.

@cfromknecht
Copy link
Collaborator

left a comment

excellent descriptions for the broadcast deltas 💯 apart from the one typo i think this is gtg

Show resolved Hide resolved htlcswitch/switch.go
Show resolved Hide resolved htlcswitch/switch_test.go Outdated

@joostjager joostjager force-pushed the joostjager:asym-broadcast-delta branch from 2f9e758 to 11f5ff2 Apr 26, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 26, 2019

@cfromknecht
Copy link
Collaborator

left a comment

LGTM ⚡️

@halseth

halseth approved these changes May 3, 2019

Copy link
Collaborator

left a comment

LGTM 🥇

Show resolved Hide resolved config.go Outdated
Show resolved Hide resolved htlcswitch/link.go Outdated
Show resolved Hide resolved config.go

@joostjager joostjager force-pushed the joostjager:asym-broadcast-delta branch from 11f5ff2 to 4c9a3d6 May 3, 2019

joostjager added some commits Apr 4, 2019

htlcswitch: reorder policy checks
This commit reorders the policies check as a preparation for splitting
the checks in separate sets for the incoming and outgoing htlc.

@joostjager joostjager force-pushed the joostjager:asym-broadcast-delta branch from 4c9a3d6 to f5f6a52 May 3, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

@halseth ptal

@Roasbeef Roasbeef merged commit 649991c into lightningnetwork:master May 3, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 60.164%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.