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

lnrpc: expose commitment type on pending open and waiting close channels #4129

Merged
merged 2 commits into from
Mar 31, 2020

Conversation

joostjager
Copy link
Contributor

Small addition so that users with waiting close channels can find out whether it makes sense to try to bump the commitment fee.

Added commit type to pending open channels as well as it was just a single line.

Pending close channels take their info from the close summary, which unfortunately doesn't provide easy access to the commit type.

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 after rebase

Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Nice!

@joostjager
Copy link
Contributor Author

Realizing now that for the pending close channels, the default commitment type will be displayed always. Need to fix that in some way.

@joostjager
Copy link
Contributor Author

The problem is that the PendingChannel proto message is used in every pending channel category. Options:

  1. Add an 'unavailable' commit type and return that for the pending close channels
  2. Lift commit type out of the common PendingChannel message into only pending open and waiting close. Works, but it can be argued that the commit type shouldn't really reside at that level.
  3. Make commit type available for pending close channels. A lot of work because the force close summary doesn't contain this info now. And still for existing channels, we wouldn't know the value of that field.

@carlaKC @cfromknecht prefs?

@carlaKC
Copy link
Collaborator

carlaKC commented Mar 31, 2020

  1. Add an 'unavailable' commit type and return that for the pending close channels

I think this would be my preference, moving the field (2) doesn't make sense, and (3) is more work than it's worth for a rpc change.

#4111 added some information to pending channels using the historical channel bucket, I think it's ok to do the same here:

  • pending_open_channels + waiting_close_channels: ok, have db channel
  • pending_closing_channels + pending_force_closing_channels: can get from historical channel, if present, otherwise leave as unknown

@cfromknecht
Copy link
Contributor

Yes I agree with @carlaKC, i think leaving as unknown is likely the best option at this point. Several of our recently added rpc fields take this approach when we can't backfill information.

@joostjager
Copy link
Contributor Author

unknown or is unavailable better?

@joostjager
Copy link
Contributor Author

Stuck with UNKNOWN. Unfortunately it became UNKNOWN_COMMITMENT_TYPE because proto requires global enum values to be unique. Because this enum is shared between messages it needs to be global.

@joostjager
Copy link
Contributor Author

Nice to see the historical bucket being used now. The originally intent was for anchors, but in the end we didn't need it.

@joostjager
Copy link
Contributor Author

@cfromknecht final check whether there is a better solution for the proto enum

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!

@joostjager joostjager merged commit 7ba5bf6 into lightningnetwork:master Mar 31, 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