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

cnct+htlcswitch+invoices: move invoice parameter check to registry #2913

Merged
merged 14 commits into from May 16, 2019

Conversation

@joostjager
Copy link
Collaborator

@joostjager joostjager commented Apr 8, 2019

This PR simplifies the link implementation by moving out all references to invoices. As an exit hop, the link offers its htlc to the invoice registry. The invoice registry decides how and when the link should respond to the htlc.

Checking of the cltv delta and amount is now performed in the invoice registry. This concentrates the logic around invoice acceptance and settling in the invoice registry. It increases maintainability and is a also preparatory step for implementation of multi path payments.

In addition to that, moving the htlc parameter check to the invoice registry allows us to apply the exact same logic for on-chain resolution of an incoming htlc. This fixes some unaddressed edge cases:

  • On-chain resolution of an incoming htlc for a canceled invoice. Previously the htlc would still be settled even though the invoice database indicates that we should reject payments to the invoice. The htlc was pulled but the invoice stayed in the canceled stayed.

  • On-chain resolution of an incoming htlc for an open invoice. Previously the invoice was marked settled as long as the htlc hadn't expired yet. But if the number of remaining blocks until expiry is low, it could
    happen that we couldn't pull the htlc in the end.

  • On-chain resolution of an incoming htlc for a hodl invoice. Previously the hodl invoice would move to the accepted state regardless of the number of remaining blocks until htlc expiry. It could be that invoices move to the accepted state without meeting the invoice min cltv delta requirement. This violates the assumption of the user that there are still enough blocks left when the accepted state transition is received.

To reduce the number of execution paths for incoming htlc resolution, this PR modifies the channel arbitrator to always send htlcs via the incoming contest resolver. This allows us to perform the check with the invoice registry in a single location.

Furthermore this change makes it unnecessary to already determine earlier in the process whether an incoming contest or a success resolver is needed. This simplifies channel and channel_arbitrator.

@joostjager joostjager force-pushed the hodl-restart-fix branch 3 times, most recently from 4b59b01 to aaa41e7 Apr 9, 2019
@joostjager joostjager changed the title Hodl restart fix cnct+htlcswitch+invoices: move invoice parameter to check registry Apr 9, 2019
@joostjager joostjager changed the title cnct+htlcswitch+invoices: move invoice parameter to check registry cnct+htlcswitch+invoices: move invoice parameter check to registry Apr 9, 2019
@joostjager joostjager force-pushed the hodl-restart-fix branch from aaa41e7 to 5358fa0 Apr 9, 2019
@joostjager joostjager marked this pull request as ready for review Apr 9, 2019
@joostjager joostjager changed the title cnct+htlcswitch+invoices: move invoice parameter check to registry cnct+htlcswitch+invoices: move invoice parameter check to registry [wip] Apr 9, 2019
@cfromknecht cfromknecht added this to the 0.7 milestone Apr 11, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Apr 11, 2019

Linking #2935. This may make it easier to handle hodl invoices and incoming preimages in the contract resolvers.

Loading

@joostjager joostjager force-pushed the hodl-restart-fix branch 2 times, most recently from a3d673d to 6a9924e Apr 17, 2019
@joostjager joostjager force-pushed the hodl-restart-fix branch 5 times, most recently from ff5227b to 89a8545 Apr 17, 2019
@Roasbeef Roasbeef requested a review from cfromknecht Apr 26, 2019
@joostjager joostjager force-pushed the hodl-restart-fix branch from 89a8545 to 6d58b4e May 6, 2019
@joostjager joostjager changed the title cnct+htlcswitch+invoices: move invoice parameter check to registry [wip] cnct+htlcswitch+invoices: move invoice parameter check to registry May 6, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

great work @joostjager, completed an initial round of review

Loading

contractcourt/interfaces.go Show resolved Hide resolved
Loading
server.go Outdated Show resolved Hide resolved
Loading
lnwallet/channel.go Show resolved Hide resolved
Loading
Loading
Loading
htlcswitch/link.go Show resolved Hide resolved
Loading
htlcswitch/link.go Outdated Show resolved Hide resolved
Loading
htlcswitch/link.go Show resolved Hide resolved
Loading
htlcswitch/link.go Outdated Show resolved Hide resolved
Loading
@@ -2868,8 +2782,12 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
// Notify the invoiceRegistry of the exit hop htlc. If we crash right
// after this, this code will be re-executed after restart. We will
// receive back a resolution event.
blocksToExpiry := int32(pd.Timeout) - int32(heightNow)
Copy link
Collaborator

@cfromknecht cfromknecht May 7, 2019

Choose a reason for hiding this comment

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

not sure that converting to int32 helps here. it's still susceptible to underflow, which would later satisfy either of the equations: blocksTilExpiry >= finalExpiry or blocksTilExpiry >= finalRejectDelta. however, checking that pd.Timeout > heightNow only needs to be on the first accept, which goes in line with my other comment passing both distinctly into NotifyExitHopHtlc and validating this in the invoice registry

Loading

Copy link
Collaborator Author

@joostjager joostjager May 7, 2019

Choose a reason for hiding this comment

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

Suppose pd.Timeout is 3 and heightNow is 5. blocksToExpiry would then be -2 and blocksTilExpiry >= finalExpiry = -2 >= finalExpiry would be false?

Loading

Copy link
Collaborator Author

@joostjager joostjager May 7, 2019

Choose a reason for hiding this comment

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

But this check has now indeed been moved to invoice registry.

Loading

Copy link
Collaborator

@cfromknecht cfromknecht May 8, 2019

Choose a reason for hiding this comment

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

The case I'm thinking about is when pd.Timeout is very high (as a unit32), like 0x80000000. In this example, blocksTilExpiry is now 2147483643 which exceeds any reasonable finalExpriy

Loading

Copy link
Collaborator Author

@joostjager joostjager May 8, 2019

Choose a reason for hiding this comment

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

Rewritten the comparison without subtraction

Loading

// sender can sweep it, so we'll end our lifetime. Here we deliberately
// forego the chance that the sender doesn't sweep and we already have
// or will learn the preimage. Otherwise the resolver could potentially
// stay active indefinitely and the channel will never close properly.
Copy link
Collaborator

@cfromknecht cfromknecht May 7, 2019

Choose a reason for hiding this comment

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

are we sure we want to do this? seems like if the htlc is unswept by either party the channel should remain unclosed. perhaps there's an argument to be made that in this case we should offload to the stray output pool, to decouple it from the closing of the channel. but that seems easier to add on later if the contract hasn't been closed, than trying to resurrect everything after the resolver state has been cleaned up

Loading

Copy link
Collaborator Author

@joostjager joostjager May 7, 2019

Choose a reason for hiding this comment

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

It was already like that, I just clarified the rationale in the comment. Would you like to change that behaviour in this PR? I am not sure how users will react to these stuck channels, as long as we don't have a stray output pool.

Btw, for exit hops this check is actually unnecessary, because further down NotifyExitHopHtlc will also return a cancel action. For exit hops we really want to close the contract, because accepting it would violate the invoice final cltv requirement?

Loading

Copy link
Collaborator

@cfromknecht cfromknecht May 15, 2019

Choose a reason for hiding this comment

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

@joostjager doesn't need to be done in this pr, that is a bigger change and currently we are keeping the existing behavior.

I am not sure how users will react to these stuck channels

i'm not sure either, though imo it'd be preferable to know that we are possibly leaving money on the table, especially since the preimage could come in shortly after the expiry. it's also possible that this whole flow can be delayed due to fee spikes, and in that case it seems like it'd be premature to give up on the HTLC, though i admit i haven't worked through all the edge cases

accepting it would violate the invoice final cltv requirement

i'm not positive what the correct behavior is either. in the case of a regular invoice, we will already know the preimage and the resolver will convert itself to a success resolver and we no longer have this issue. if we have a hold invoice, "learning" the preimage after the expiry could only be done by the preimage being provided externally via rpc, so perhaps that constitutes some explicit consent that the user in fact wants to settle the invoice even if the expiry has expired. what do you think?

Loading

Copy link
Collaborator Author

@joostjager joostjager May 15, 2019

Choose a reason for hiding this comment

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

this whole flow can be delayed due to fee spikes, and in that case it seems like it'd be premature to give up on the HTLC

Maybe, but in the other hand, no one is losing money. The htlc is canceled back and we never marked it as settled, so no pizza has been delivered yet.

if we have a hold invoice, "learning" the preimage after the expiry could only be done by the preimage being provided externally via rpc, so perhaps that constitutes some explicit consent that the user in fact wants to settle the invoice even if the expiry has expired.

It is about accepting the invoice. For hodl invoices, the preimage isn't needed to accept. If we would accept, the user should check the height when the accepted event comes because there may be fewer blocks left than expected.

Also for regular invoices, there is a risk in marking the invoice settled when there are not enough blocks left and we still need to wait for confirmation of a tx spending the commit output. We normally assume that we need the final cltv delta blocks to get those confirmed. This is also the criterium for settling invoices in the link.

Previously we would only mark the invoice as settled when the success resolver completes. This is safer, but can also be an unnecessary delay in case there are plenty of blocks left. It will take longer for the pizza to be delivered because of a channel failure that the payer has nothing to do with.

Loading

@joostjager joostjager force-pushed the hodl-restart-fix branch from 6d58b4e to dc54add May 7, 2019
lnwallet/channel.go Show resolved Hide resolved
Loading
contractcourt/channel_arbitrator.go Show resolved Hide resolved
Loading
contractcourt/channel_arbitrator.go Show resolved Hide resolved
Loading
lnd_multihoplocalchainclaim_test.go Outdated Show resolved Hide resolved
Loading
lnd_multihoplocalchainclaim_test.go Outdated Show resolved Hide resolved
Loading
@@ -99,7 +99,10 @@ func TestInvoiceWorkflow(t *testing.T) {
// now have the settled bit toggle to true and a non-default
// SettledDate
payAmt := fakeInvoice.Terms.Value * 2
if _, err := db.AcceptOrSettleInvoice(paymentHash, payAmt); err != nil {
_, err = db.AcceptOrSettleInvoice(
Copy link
Member

@Roasbeef Roasbeef May 11, 2019

Choose a reason for hiding this comment

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

I think this commit could've easily been split up into multiple commits. There's a lot going on in this commit across multiple files and packages.

Loading

Copy link
Collaborator Author

@joostjager joostjager May 13, 2019

Choose a reason for hiding this comment

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

How would you do this easily? This commit is a set of changes that are all interconnected. Leaving some out will not result in a working lnd. I tried to split off all independent changes already. Also, I want to fix tests in the same commit that breaks them, so those can't be moved out.

Loading

Copy link
Member

@Roasbeef Roasbeef May 14, 2019

Choose a reason for hiding this comment

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

Yeah there's a tradeoff w.r.t having them always compile vs having them make smaller changes at a time with an eye for ease of review. IMO things are more likely to slip by with larger commits that make modifications over several packages/files. Isolated commits also allow the committer to tell a narrative in a sense, with the progression of each commit.

As it's already received some attention as is, I think it's too late at this point. However, as an example, the AcceptOrSettleInvoice change could've been added in a distinct commit that just left all the logic in place, but used a nil value for the function closure in the link. A follow up commit could then remove the old verification logic and add the height based verification to that new closure.

Loading

Copy link
Collaborator Author

@joostjager joostjager May 14, 2019

Choose a reason for hiding this comment

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

Ok, that is a good example indeed. Could have been a separate commit. Not sure about committing non-building code though.

Loading

invoices/invoiceregistry.go Outdated Show resolved Hide resolved
Loading
invoices/invoiceregistry.go Show resolved Hide resolved
Loading
lnd_multihopreceiverchainclaim_test.go Outdated Show resolved Hide resolved
Loading
lnd_multihopreceiverchainclaim_test.go Outdated Show resolved Hide resolved
Loading
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 13, 2019

Then the commitment tx is published and mined, after which the resolver takes over. At that point there are just 39 blocks left. According to the new (consistent) logic introduced in this PR, that is not enough and the htlc is canceled.

Why should the HTLC be cancelled? It should be swept since Carol has the preimage.

See explanation in comments above. Having the preimage for an invoice doesn't always mean we should settle the htlc. At least that is the idea introduced in this PR.

Loading

@joostjager joostjager force-pushed the hodl-restart-fix branch from fe71137 to a3d22c4 May 13, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 13, 2019

Loading

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM on the code changes 🐣

Starting to run this on some testnet nodes, also thinking about how to properly evaluate it in the wild before rolling out. It's mostly code movement, but existing code pathways are also modified.

Loading

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

@joostjager did another pass, looking good! a couple non-blocking comments, otherwise LGTM 🌪

One step closer to multi-path payments!

Loading

@@ -0,0 +1,352 @@
// +build rpctest
Copy link
Collaborator

@cfromknecht cfromknecht May 15, 2019

Choose a reason for hiding this comment

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

imo it'd be preferable to start moving these all into the lntest package, tho i suspect there are a bunch of dependencies either in lnd or just helper functions that would break if we don't do the move all at once. is that the case?

Loading

Copy link
Collaborator Author

@joostjager joostjager May 15, 2019

Choose a reason for hiding this comment

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

It needs to be done all at once, commit added.

Loading

// Wait for carol to mark invoice as accepted. There is a small gap to
// bridge between adding the htlc to the channel and executing the exit
// hop logic.
for {
Copy link
Collaborator

@cfromknecht cfromknecht May 15, 2019

Choose a reason for hiding this comment

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

this block might make a good helper function, since it's used 3+ times

Loading

Copy link
Collaborator Author

@joostjager joostjager May 15, 2019

Choose a reason for hiding this comment

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

extracted

Loading

@joostjager joostjager force-pushed the hodl-restart-fix branch 4 times, most recently from 49f76db to 154f688 May 15, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 15, 2019

Loading

@joostjager joostjager requested a review from cfromknecht May 15, 2019
joostjager and others added 14 commits May 15, 2019
One of the first things the incoming contest resolver does is checking
if the preimage is available and if it is, convert itself into a success
resolver.

This behaviour makes it unnecessary to already determine earlier in the
process whether an incoming contest or a success resolver is needed.

By having all incoming htlcs go through the incoming contest resolver,
the number of execution paths is reduced and it becomes easier to
ascertain that the implemented logic is correct.

The only functional change in this commit is that a forwarded htlc for
which is the preimage is known, is no longer settled when the htlc is
already expired. Previously a success resolver would be instantiated
directly, skipping the expiry height check.

This created a risk that the success resolver would never finish,
because an expired htlc could already have been swept by the remote
party and there is no detection of this remote spend in the success
resolver currently.

With the new change, the general direction that an expired htlc
shouldn't be settled and instead given up on is implemented more
consistently.

This commit prepares for fixing edges cases related to hodl
invoice on-chain resolution.
Now that the success resolver preimage field is always populated by the
incoming contest resolver, preimage lookups earlier in the
process (channel and channel arbitrator) can mostly be removed.
New behaviour of the chain notifier to always send the current block
immediately after registration takes away the need to make a separate
GetBestBlock call on ChainIO.
The former tryApplyPreimage function silently ignored invalid preimages.
This could mask potential bugs. This commit makes the logic stricter and
generates an error in case an unexpected mismatch occurs.
This commit isolates preimages of forwarded htlcs from invoice
preimages. The reason to do this is to prevent the incoming contest
resolver from settling exit hop htlcs for which the invoice isn't marked
as settled.
Create an interface type to be able to mock the registry in unit tests.
This commit splits out several integration tests that will be modified
in a follow up commit to separate files. The current lnd_test.go file is
10k+ loc which makes it harder to handle by tools and it isn't good for
overview either.
This commit is the final step in making the link unaware of invoices. It
now purely offers the htlc to the invoice registry and follows
instructions from the invoice registry about how and when to respond to
the htlc.

The change also fixes a bug where upon restart, hodl htlcs were
subjected to the invoice minimum cltv delta requirement again. If the
block height has increased in the mean while, the htlc would be canceled
back.

Furthermore the invoice registry interaction is aligned between link and
contract resolvers.
This commit refactors test code around channel restoration in unit
tests to make it easier to use.
This commit adds a test that covers the hodl invoice behaviour after a
link restart.
@joostjager joostjager force-pushed the hodl-restart-fix branch 2 times, most recently from cdc834f to 570f9ca May 16, 2019
@joostjager joostjager merged commit 27ae22f into lightningnetwork:master May 16, 2019
2 checks passed
Loading
@joostjager joostjager deleted the hodl-restart-fix branch May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants