Skip to content

cnct: use sweeper in commit resolver#2407

Merged
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
joostjager:commit-resolver-sweeper
Feb 2, 2019
Merged

cnct: use sweeper in commit resolver#2407
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
joostjager:commit-resolver-sweeper

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Jan 3, 2019

Now that the sweeper is available, it isn't necessary anymore for the
commit resolver to craft its own sweep tx. Instead it can offer its
input to the sweeper and wait for the outcome.

When this PR is merged, a follow up is to use sweeper in htlcSuccessResolver too.

@joostjager joostjager force-pushed the commit-resolver-sweeper branch 3 times, most recently from 6376f32 to 55861d9 Compare January 3, 2019 15:15
@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Jan 4, 2019

Before we convert over the REST of the HTLC resolvers, we should first address #1875, otherwise we create more RPC blind spots along the way.

Comment thread contractcourt/contract_resolvers.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even in this case, can't we bypass the nursery and wait out the time lock ourselves here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes that is a next step. I have some code that shows the poc already in #2000 (last 4 commits)

But I want to take small steps at a time.

@joostjager
Copy link
Copy Markdown
Contributor Author

Before we convert over the REST of the HTLC resolvers, we should first address #1875, otherwise we create more RPC blind spots along the way.

I don't think this PR and the same change for htlcSuccessResolver create new blind spots. Their (non-nursery) sweeps were already off the radar.

@joostjager
Copy link
Copy Markdown
Contributor Author

When we start really cutting out the nursery in further PRs, we indeed do create new blind spots which should be addressed first.

@joostjager joostjager force-pushed the commit-resolver-sweeper branch from 55861d9 to 5cefc67 Compare January 17, 2019 12:14
@Roasbeef Roasbeef added this to the 0.6 milestone Jan 22, 2019
@joostjager
Copy link
Copy Markdown
Contributor Author

@Roasbeef ptal. I think this is not blocked by #1875, as we don't create new blind spots in this PR.

Copy link
Copy Markdown
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.

@joostjager love the code simplification here, and should lead a to a more efficient on-chain front print for resolving these when other outputs are being swept concurrently! only minor comments from me

Comment thread contractcourt/commit_sweep_resolver.go Outdated
Comment thread contractcourt/commit_sweep_resolver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

iirc, the spend notification will only trigger after it has one conf, so i'm not sure this second stage does anything? obviously this existed prior and independent of this PR, but perhaps we should consider bumping the number of confirmations

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I indeed think this second stage does nothing, but not sure if we want to change anything to it in this pr. The greater plan is to remove nursery completely and wait out the csv inside the resolver.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah this was left over from when we still did mempool confirmations. IMO there's no harm in leaving it in as we should move away from single conf dispatch in many areas of the codebase.

@joostjager joostjager force-pushed the commit-resolver-sweeper branch from 5cefc67 to 8c05dce Compare January 30, 2019 07:58
@joostjager
Copy link
Copy Markdown
Contributor Author

@cfromknecht ptal

cfromknecht
cfromknecht previously approved these changes Feb 1, 2019
Copy link
Copy Markdown
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 ✅

Roasbeef
Roasbeef previously approved these changes Feb 1, 2019
Copy link
Copy Markdown
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.

With #1875 in, we can now create a report for this resolver as well. If the remote party force close the channel and we have incoming/outgoing HTLCs to it, then we'll report that limbo balance but not the recovered/limbo balance of this non-delayed commitment output. FWIW, this is already the case in w/o this PR so this adds no new holes, but with recent merges makes the existing hole more apparent. Not saying we should block on plugging the existing hole, just want to point it out.

@cfromknecht
Copy link
Copy Markdown
Contributor

@joostjager needs rebase now

Now that the sweeper is available, it isn't necessary anymore for the
commit resolver to craft its own sweep tx. Instead it can offer its
input to the sweeper and wait for the outcome.
@joostjager joostjager dismissed stale reviews from Roasbeef and cfromknecht via e486340 February 1, 2019 08:20
@joostjager joostjager force-pushed the commit-resolver-sweeper branch from 8c05dce to e486340 Compare February 1, 2019 08:20
@joostjager
Copy link
Copy Markdown
Contributor Author

I prefer to do the reporting in a follow up pr.

Rebased.

ptal @cfromknecht @Roasbeef

Copy link
Copy Markdown
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.

LGTM 🦅

@Roasbeef Roasbeef merged commit 8aecccf into lightningnetwork:master Feb 2, 2019
@joostjager joostjager mentioned this pull request Nov 7, 2019
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