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

contractcourt: settle invoice when claiming HTLC on-chain #2075

Merged

Conversation

3 participants
@wpaulino
Copy link
Collaborator

wpaulino commented Oct 19, 2018

In this PR, we extend the htlcSuccessResolver to settle the invoice,
if any, of the corresponding on-chain HTLC sweep. This ensures that the
invoice state is consistent as when claiming the HTLC "off-chain".

Partly addresses #2032 (the receiver side).

@Roasbeef Roasbeef requested a review from joostjager Oct 19, 2018

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch 2 times, most recently from 2156b9d to 08746aa Oct 19, 2018

Show resolved Hide resolved witness_beacon.go Outdated
Show resolved Hide resolved contractcourt/contract_resolvers.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch 3 times, most recently from 26e6a97 to c647d55 Oct 22, 2018

Show resolved Hide resolved contractcourt/contract_resolvers.go Outdated
Show resolved Hide resolved contractcourt/contract_resolvers.go Outdated
if !invoicesResp.Invoices[0].Settled {
t.Fatalf("expected invoice to be settled")
}

This comment has been minimized.

Copy link
@joostjager

joostjager Oct 23, 2018

Collaborator

You could consider adding test coverage on the unit level. In general, I myself prefer to test things only in an integration test if there is really no other option.

I have an (unmerged) contract resolver test for the outgoingcontestresolver, 33c348b.

But I can also imagine that you consider this too much effort in comparison to the size of the change.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Dec 11, 2018

Author Collaborator

I opted for the integration test change as we're interested in making sure that the invoice state is reflected correctly within the database. I guess we could have this done at the unit test level though. Let me know if you'd like to proceed with that instead.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 11, 2018

Collaborator

We test lots of database behaviour in unit tests. I'd go for that. Integration tests can be so expensive in maintenance over their full lifetimes. Checking flakes, dirty time.sleeps, etc. Maybe ask a second person for an opinion.

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch from c647d55 to 0b5a9e9 Dec 11, 2018

@Roasbeef Roasbeef added this to Blocked in High Priority Dec 18, 2018

@Roasbeef Roasbeef moved this from Blocked to Needs review in High Priority Dec 18, 2018

Show resolved Hide resolved lnd_test.go Outdated
Show resolved Hide resolved lnd_test.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch 3 times, most recently from a5252ba to ef5c3a8 Jan 16, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

wpaulino commented Jan 16, 2019

Comments addressed, PTAL @Roasbeef @joostjager

Show resolved Hide resolved channeldb/invoices.go Outdated
Show resolved Hide resolved contractcourt/contract_resolvers.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch from ef5c3a8 to 7f672a5 Jan 22, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 22, 2019

I still think a unit test would've been more appropriate for this PR (see earlier comment above), but if a second reviewer approves, I am good.

Other than that, LGTM

@Roasbeef
Copy link
Member

Roasbeef left a comment

One style related nit, but other than that, LGTM 🐲

Agree with Joost here that this package could certainly use additional unit tests. I don't think it's worth blocking the merge of this PR as this section is already covered with additional unit tests. Further down the release timeline, if we're in a testing freeze with time to spare, we can circle back and bolster the unit tests in this package. This may be something we want to do in-between 0.6 and the next major release as well, while we gather PRs for the next milestone.

Show resolved Hide resolved contractcourt/channel_arbitrator.go
Show resolved Hide resolved contractcourt/htlc_success_resolver.go

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch from 7f672a5 to d712219 Jan 23, 2019

cnct: extract relaunching of resolvers to method
Co-authored-by: Joost Jager <joost.jager@gmail.com>

wpaulino and others added some commits Jan 23, 2019

contractcourt+server: settle invoice for on-chain HTLC sweep
In this commit, we extend the htlcSuccessResolver to settle the invoice,
if any, of the corresponding on-chain HTLC sweep. This ensures that the
invoice state is consistent as when claiming the HTLC "off-chain".
lnd_test: extend chain claim test with invoice settled check
In this commit, we extend the remote/receiver chain claim integration
test to assert that the on-disk representation of the invoice on the
receiving side (Carol) is marked as settled due to the claiming the HTLC
on-chain.
lnwallet: add HtlcPoint() resolution helper methods
Co-authored-by: Joost Jager <joost.jager@gmail.com>
cnct: make original htlc amt available for contract resolvers
Previously, contract resolvers that needed to publish a second level tx,
did not have access to the original htlc amount.

This commit reconstructs this amount from data that is already persisted
in arbitrator log.

Co-authored-by: Joost Jager <joost.jager@gmail.com>

@wpaulino wpaulino force-pushed the wpaulino:settle-invoice-on-chain-sweep branch from d712219 to 016add6 Jan 23, 2019

@Roasbeef Roasbeef merged commit 6c610d9 into lightningnetwork:master Jan 24, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 56.172%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

High Priority automation moved this from Needs review to Done Jan 24, 2019

@wpaulino wpaulino deleted the wpaulino:settle-invoice-on-chain-sweep branch Jan 24, 2019

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.