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

rpc+contractcourt: merge the contractResolver state into the pendingchannels RPC response #1875

Merged
merged 5 commits into from Feb 2, 2019

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager force-pushed the resolverstate branch 3 times, most recently from 6a98580 to 3d99ff9 Compare September 11, 2018 14:05
@Roasbeef
Copy link
Member

Roasbeef commented Dec 4, 2018

I'd like to pick this back up, with the exclusion of the commit to detect spends in the nursery. Since then, we've created the Sweeper which itself will handle this notification case. It still stands that as is, the API is blind to these HTLCs that haven't yet timed out causing much user confusion and an inability to properly track all funds throughout the timeout cycle. Even after the new sweeper changes have been merged, we'll still need this for the lifetime that the current nursery exists (and the resolvers will still report directly back to the API).

@Roasbeef Roasbeef added rpc Related to the RPC interface P2 should be fixed if one has time needs review PR needs review by regular contributors bug fix labels Dec 4, 2018
@Roasbeef Roasbeef added this to Blocked in High Priority Dec 18, 2018
@joostjager joostjager force-pushed the resolverstate branch 3 times, most recently from 69a35fc to 288652f Compare January 7, 2019 10:51
@joostjager
Copy link
Collaborator Author

Some observations on the current reporting situation:

  • When the channel is fully closed, the close summary doesn't show any information on the recovered balance. Just the settled balance at the time of closure.
  • During pending close, only recovered balance from locked outputs is reported.
  • During pending close, recovered balance stays in the report even when contract resolvers have already finished. The data is kept in the utxonursery graduation table.
  • Some nursery report fields are unused. I removed them in commit utxonursery: remove unused report fields.

Is this the desired behaviour? Ideally I think you'd want to see:

  • In addition to the settled balance, report in the channel close summary: recovered balance, lost (to remote) balance and chain fees. These should add up to the settled balance.
  • During pending close, show limbo balance, recovered balance, lost (to remote) balance and chain fees. All reported per output. This should again add up to the settled balance. Contrary to current behaviour, also add commitment no delay outputs as recovered balance.
  • Open question is how to attribute the on-chain sweeper fee to the various inputs. One option is to make some kind of proportional calculation it the sweeper and report the per input fee back as part of the sweeper result struct.

Implementation implications:

  • Currently the channel close summary is already written before the contract resolvers finish their work. This order would need to change.
  • The point that we are working towards is to get rid of utxonursery completely. That means that all reporting should originate from the contract resolvers. Currently contract resolvers disappear after the have finished. At that point, the information on recovered/lost/fee balances is lost. To be able to keep reporting this, it should be persisted somewhere else (briefcase?). Or alternatively, contract resolvers should be kept alive until all of them have finished.
  • How to make a small first step? An option is to remove UtxoNursery.NurseryReport() and pull all data from the contract resolvers. If we don't want the changeset to get big, this would mean that reporting on recovered balance during pending close would disappear. I doubt this is a problem, as recovered balance is currently already incomplete. In exchange for this, we'd get accurate limbo balance reporting, because the source is now the contract resolver that has full information on what is going on.

@Roasbeef
Copy link
Member

Roasbeef commented Jan 8, 2019

When the channel is fully closed, the close summary doesn't show any information on the recovered balance. Just the settled balance at the time of closure.

Yes we atm we only record the fully settled amount, as depending on a number of parameters (if it's batched, the fee rate, etc) we may actually sweep less than the total amount settled at closing time.

During pending close, only recovered balance from locked outputs is reported.

We report the CSV outputs, and HTLC outputs as they transition from stage 1 to stage 2. We also report the total limbo balance, and the amount of coins recovered at that given instance.

During pending close, recovered balance stays in the report even when contract resolvers have already finished. The data is kept in the utxonursery graduation table.

Yep, it's part of the base report. Only once all the outputs that were created by a channel have been resolved do we no longer report on it.

In addition to the settled balance, report in the channel close summary: recovered balance, lost (to remote) balance and chain fees. These should add up to the settled balance.

Agree this would be a good addition in order to ensure users have access to this data without doing any manual block chain crawling. I think it's out of the scope for this PR though, which should be focused on fixing the egregious blind spots.

During pending close, show limbo balance, recovered balance, lost (to remote) balance and chain fees. All reported per output. This should again add up to the settled balance. Contrary to current behavior, also add commitment no delay outputs as recovered balance.

All this but chain fees and the non delay output are already reported. Agree it would be nice to also show the confirmation status of the non-delay output as well. I don't think this would expand the scope of this PR too significantly, as arguably it's a blind spot in the current report and confirmations can take some time on mainnet.

Open question is how to attribute the on-chain sweeper fee to the various inputs.

Yeah we also have this issue where we want to account for the full amount recovered when we need to claim HTLCs on chain. I think this can be deferred from this PR and either picked up in #2075, or a sort of "fee report" message that can be requested from the sweeper (new PR all together).

Currently the channel close summary is already written before the contract resolvers finish their work. This order would need to change.

We can still write the close summary as soon as we detect a channel close, but then update the fields as we go.

At that point, the information on recovered/lost/fee balances is lost. To be able to keep reporting this, it should be persisted somewhere else (briefcase?).

Yeah so we'd basically progressively move over this state from the nursery over to the main channel arb.

@Roasbeef
Copy link
Member

Roasbeef commented Jan 8, 2019

How to make a small first step?

I think the first step would be to ensure we report (via the resolver) the state of the HTLCs that haven't yet timed out. Once they timeout, they get handed off to the nursery and can be reported externally as part of the regular nursery report. Next would be to report the state of the non-delay output while it hasn't yet been fully swept. After that we'd start to store within the channel arb's log the finalized state of all resolved contracts. The final step would then be moving all accounting from the NurseryReport method into the channel arb itself.

@joostjager joostjager force-pushed the resolverstate branch 3 times, most recently from fd9d073 to dfadff1 Compare January 16, 2019 20:00
@joostjager joostjager force-pushed the resolverstate branch 9 times, most recently from cdc1e98 to baf480f Compare January 17, 2019 13:19
@joostjager joostjager changed the title rpc+contractcourt: merge the contractResolver state into the pendingchannels RPC response [DO NOT REVIEW] rpc+contractcourt: merge the contractResolver state into the pendingchannels RPC response Jan 17, 2019
@joostjager
Copy link
Collaborator Author

@Roasbeef It is ready for review

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 22, 2019
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.

Nice work! I like the little fix ups and refactoring along the way. No major comments, mostly style related nits. Happy to have this hole patched before we continue to overhaul the resolvers to communicate directly with the sweeper.

contractcourt/channel_arbitrator.go Show resolved Hide resolved
utxonursery.go Show resolved Hide resolved
rpcserver.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/contract_resolvers.go Show resolved Hide resolved
contractcourt/htlc_incoming_contest_resolver.go Outdated Show resolved Hide resolved
High Priority automation moved this from Blocked to Needs review Jan 23, 2019
@joostjager
Copy link
Collaborator Author

@Roasbeef PTAL

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.

Looks pretty good!

rpcserver.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Outdated Show resolved Hide resolved
contractcourt/htlc_incoming_contest_resolver.go Outdated Show resolved Hide resolved
contractcourt/htlc_outgoing_contest_resolver.go Outdated Show resolved Hide resolved
err := r.nurseryPopulateForceCloseResp(
&chanPoint, currentHeight, forceClose,
)
if err != nil {
return nil, err
}

err = r.arbitratorPopulateForceCloseResp(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment above applies to this statement as well

contractcourt/channel_arbitrator.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
contractcourt/chain_arbitrator.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
@joostjager
Copy link
Collaborator Author

ptal @halseth

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.

LGTM 🎿

One last comments, then g2g from my PoV.

// new resolver.
c.activeResolversLock.Lock()
for i, r := range c.activeResolvers {
if r == currentContract {
Copy link
Member

Choose a reason for hiding this comment

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

Comparing pointers makes me nervous at times, instead this can use the ResolverKey() method directly.

Copy link
Collaborator Author

@joostjager joostjager Feb 1, 2019

Choose a reason for hiding this comment

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

Yes valid point. Fixed and replace operation extracted to method.

Previously the arbitrator wasn't advanced to the final stage after
the last contract resolved.

Also channel arbitrator now does not ignore a log error anymore
unresolved contracts cannot be retrieved.
In this commit we fix a reporting gap that previously existed for htlcs
that were still contested.
This commit closes a reporting gap for htlc outputs on the remote
commitment tx. Those are waited out by nursery, but were not reported
before.
@joostjager
Copy link
Collaborator Author

ptal @Roasbeef

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.

LGTM 🔱

@Roasbeef Roasbeef merged commit 6032e47 into lightningnetwork:master Feb 2, 2019
High Priority automation moved this from Needs review to Done Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix needs review PR needs review by regular contributors P2 should be fixed if one has time rpc Related to the RPC interface
Projects
No open projects
High Priority
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants