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

lntest/itest: select anchor commitment format and sweeping itests #4073

Merged
merged 9 commits into from
Mar 27, 2020

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Mar 12, 2020

This PR adds integration tests for a select set of integration tests with the channel type set to anchor:

basic funding flow
channel force closure
test multi-hop htlc local force close immediate expiry
test multi-hop htlc receiver chain claim
test multi-hop local force close on-chain htlc timeout
test multi-hop remote force close on-chain htlc timeout
test multi-hop htlc local chain claim
test multi-hop htlc remote chain claim
These are selected since they together test establishing channels of this new type in combination with the previous channel types, sweeping of commitment outputs, and HTLC sweep scenarios.

The goal is to later enable the whole integration test suite for this channel type, but it requires changes to most notably the channel backup format and watchtower.

[desc copied from #4001]

Builds on #4103

@joostjager joostjager force-pushed the anchor-sweep-itest branch 7 times, most recently from 90e35c8 to e9e1434 Compare March 17, 2020 14:36
@joostjager joostjager marked this pull request as ready for review March 17, 2020 14:46
@joostjager joostjager force-pushed the anchor-sweep-itest branch 8 times, most recently from bd16483 to 6844e87 Compare March 18, 2020 15:19
@joostjager
Copy link
Contributor Author

Integration test times out on travis 50 minute max. Need #3924

@joostjager joostjager force-pushed the anchor-sweep-itest branch 3 times, most recently from 0fb5c7c to a852022 Compare March 23, 2020 11:43
@joostjager joostjager added this to the 0.10.0 milestone Mar 23, 2020
@joostjager joostjager changed the title lntest/itest: anchor sweeping lntest/itest: select anchor commitment format and sweeping itests Mar 23, 2020
@joostjager joostjager force-pushed the anchor-sweep-itest branch 7 times, most recently from c4d2927 to f736a3f Compare March 24, 2020 14:03
@joostjager
Copy link
Contributor Author

Timeout issue solved for now because we only test two of the three formats.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Solid, only minor suggestions ✅

sweep/txgenerator.go Show resolved Hide resolved
make/testing_flags.mk Outdated Show resolved Hide resolved

func closeChannelAndAssertType(ctx context.Context, t *harnessTest,
net *lntest.NetworkHarness, node *lntest.HarnessNode,
fundingChanPoint *lnrpc.ChannelPoint, anchors, force bool) *chainhash.Hash {
Copy link
Contributor

Choose a reason for hiding this comment

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

can just take commit type instead of the bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, so the reason that I made it not the commit type, is that this function is also used for coop closes when no anchor is expected (even though the chan type is still anchors).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, because you don't have access to the force bool in assertChannelClosed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that. I could add it probably. But it would also mean that all calls to these assertions need the commit type. Now I can get away by passing in false for anchors for all the existing tests, while when a type is expected, I would want to make sure that the actual channel type is passed in (legacy or tweakless).

lntest/itest/lnd_multi-hop_test.go Show resolved Hide resolved
lntest/itest/lnd_multi-hop_test.go Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nothing major, just a request to use some of our existing helper function in chainfee to make the new commitment-specific fee/weight calculation in the itests easier to follow (and also use methods that are already tested elsewhere).

lntest/itest/lnd_test.go Show resolved Hide resolved
make/testing_flags.mk Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
joostjager and others added 9 commits March 27, 2020 07:41
Fixes a subtle bug where the outer scope predErr was hidden when the
return value of findForceClosedChannel was stored in a newly
defined variable with the same name.
Co-authored-by: Johan T. Halseth <johanth@gmail.com>
Co-authored-by: Johan T. Halseth <johanth@gmail.com>
These tests exercise the different ways of sweeping a commitment, so
we'll cover the modified scripts used for anchor commitments and
spending the anchor itself by both parties.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

still LGTM


func closeChannelAndAssertType(ctx context.Context, t *harnessTest,
net *lntest.NetworkHarness, node *lntest.HarnessNode,
fundingChanPoint *lnrpc.ChannelPoint, anchors, force bool) *chainhash.Hash {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, because you don't have access to the force bool in assertChannelClosed?

@Roasbeef Roasbeef merged commit 0b59ded into lightningnetwork:master Mar 27, 2020
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.

3 participants