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: commit sweep without nursery #3644

Merged
merged 8 commits into from
Nov 15, 2019

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Oct 29, 2019

This PR (finally) removes the dependency on the legacy utxo nursery from the commit sweep resolver.

The reason for doing this now, is that the new anchor output commitment format will also csv-lock the to_remote output. This requires changes and testing in this area and it seems like a good opportunity to clean up this technical debt.

Subtask of #3587

@joostjager joostjager added chain handling commitments Commitment transactions containing the state of the channel contracts incomplete WIP PR, not fully finalized, but light review possible labels Oct 29, 2019
@joostjager joostjager added this to the 0.9.0 milestone Oct 29, 2019
@joostjager joostjager removed the request for review from Roasbeef October 29, 2019 16:29
@joostjager joostjager self-assigned this Oct 29, 2019
@joostjager
Copy link
Contributor Author

joostjager commented Oct 29, 2019

Note to self: reporting in PendingChannels (no more nursery report)

@joostjager
Copy link
Contributor Author

To do: add commit sweep resolver unit test

@joostjager joostjager force-pushed the commit-sweep-no-nursery branch 2 times, most recently from bed44a0 to fad80f9 Compare October 31, 2019 12:01
@joostjager
Copy link
Contributor Author

Unit test added, ready for review

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.

Awesome change, I can see this making it much simpler to add the new commitment ouputs later, and nice to have more of the sweeping being done by the sweeper 👍

contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
contractcourt/channel_arbitrator.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
contractcourt/commit_sweep_resolver_test.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver_test.go Show resolved Hide resolved
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.

LGTM 💹

contractcourt/channel_arbitrator.go Outdated Show resolved Hide resolved
contractcourt/channel_arbitrator.go Outdated Show resolved Hide resolved
contractcourt/channel_arbitrator.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver_test.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver_test.go Outdated Show resolved Hide resolved
@joostjager
Copy link
Contributor Author

Rebased on top of resolver constructors

@joostjager joostjager removed the incomplete WIP PR, not fully finalized, but light review possible label Nov 12, 2019
Unify resolver specific log statements. Leaves modification of
the other resolvers for a later moment when it can be combined with a
real change.
This commit prepares for the commit sweep resolver to report on its
state.
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM. Nice commit structure 🤘

contractcourt/channel_arbitrator.go Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
contractcourt/commit_sweep_resolver.go Outdated Show resolved Hide resolved
MaturityDelay: 3,
}

ctx := newCommitSweepResolverTestContext(t, &res)
Copy link
Contributor

Choose a reason for hiding this comment

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

check the report at various times during the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check added

The channel arbitrator no longer passes the direct commitment output to
the nursery for incubation. Instead the resolver itself will await the
csv lock if any.

The reason to change this now is to prevent having to deal with the
(legacy) nursery code for a planned anchor outputs related change to the
commit sweep resolver (also csv lock to_remote).

It is no problem if there are any lingering incubating outputs at the
time of upgrade. This just means that the output will be offered twice
to the sweeper and this doesn't hurt.
Now that the commit sweep resolver is no longer relying on the nursery,
all code associated with commit sweeping can be removed.
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.

LGTM 🎩

@halseth halseth merged commit 840051c into lightningnetwork:master Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chain handling commitments Commitment transactions containing the state of the channel contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants