cnct: use sweeper in commit resolver #2407
Conversation
joostjager
force-pushed
the
commit-resolver-sweeper
branch
3 times, most recently
from
6376f32 to
55861d9
Compare
|
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. |
contractcourt/contract_resolvers.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Otherwise we are dealing with a local commitment transaction and the |
There was a problem hiding this comment.
Even in this case, can't we bypass the nursery and wait out the time lock ourselves here?
There was a problem hiding this comment.
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.
I don't think this PR and the same change for |
|
When we start really cutting out the nursery in further PRs, we indeed do create new blind spots which should be addressed first. |
joostjager
force-pushed
the
commit-resolver-sweeper
branch
from
55861d9 to
5cefc67
Compare
There was a problem hiding this comment.
@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
| sweepTXID := c.sweepTx.TxHash() | ||
| sweepingScript := c.sweepTx.TxOut[0].PkScript | ||
| sweepTXID := sweepTx.TxHash() | ||
| sweepingScript := sweepTx.TxOut[0].PkScript |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
commit-resolver-sweeper
branch
from
5cefc67 to
8c05dce
Compare
|
@cfromknecht ptal |
There was a problem hiding this comment.
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.
|
@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
force-pushed
the
commit-resolver-sweeper
branch
from
8c05dce to
e486340
Compare
|
I prefer to do the reporting in a follow up pr. Rebased. ptal @cfromknecht @Roasbeef |
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
htlcSuccessResolvertoo.