Add flags for if a channel is pub and funding txo in ChannelDetails #912
Conversation
TheBlueMatt
force-pushed
the
2021-05-more-chan-info
branch
from
77bf352 to
1a3f65f
Compare
|
Amended, only diff is the new changes in |
The problem is in the normal router tests, too. |
Codecov Report
@@ Coverage Diff @@
## main #912 +/- ##
==========================================
+ Coverage 90.57% 90.86% +0.28%
==========================================
Files 59 59
Lines 29738 30808 +1070
==========================================
+ Hits 26936 27993 +1057
- Misses 2802 2815 +13
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-05-more-chan-info
branch
from
3e284f5 to
43f306d
Compare
|
Squashed without change. |
lightning/src/ln/channelmanager.rs
Outdated
| /// channel is not currently being shut down. `funding_locked` message exchange implies the | ||
| /// required confirmation count has been reached (and we were connected to the peer at some | ||
| /// point after the funding transaction received enough confirmations). | ||
| pub is_confirmed: bool, |
There was a problem hiding this comment.
Maybe call it is_usable if it matches exactly Channel method. "Confirmed" is a bit ambiguous for a) ongoing shutdown implies the channel has been confirmed and b) zero-conf channel don't expect any confirmation and are ready-to-go at creation ?
There was a problem hiding this comment.
is_usable is overloaded in a few places in our code already, and is a bit vague. We could add verbosity (like is_blockchain_confirmed or reached_target_blockchain_confirmations) or we could maybe refer to "locked" eg is_blockchain_locked. What do you think?
There was a problem hiding this comment.
Maybe is_funding_locked would be sufficient as it covers having enough confirmations and having exchanged the funding_locked messages.
That said, I don't understand the argument against is_usable. It is only used in Channel as far as I can tell and is defined in the same manner as here. Where else is it used?
There was a problem hiding this comment.
Ah, I see there is ChannelManager::list_usable_channels which is checking against is_live. Perhaps that needs to be renamed instead. Or maybe is_live should really be is_usable? Then we'd need a new name for Channel::is_usable to something like is_funding_locked or maybe simply is_funded.
There was a problem hiding this comment.
Maybe we rename list_usable_channels to list_live_channels or so? I like live/usable including "we're connected to the peer, in addition to is_funding_locked (which I'll rename this to).
There was a problem hiding this comment.
Hmm, I actually feel like "live" is more descriptive? Live, to me, is all about us being actively connected to the peer, ie the peer (and thus channel) are "live". What about renaming list_usable_channels to list_live_channels and leaving this as is_funding_locked (which is really just about the state of the funding transaction).
There was a problem hiding this comment.
More descriptive in terms of functionality. Is something "live" also "usable"? Only if "live" includes "funding locked" which I don't think is obvious from the word "live" alone -- you need to read the docs to figure that out.
Regardless of which, my other point regarding Channel::is_usable still stands if something that is live is suppose to also be usable.
There was a problem hiding this comment.
Hmm, I guess I felt like "live" did capture that. I'm happy to go with "usable" if you feel strongly. Note that Channel isn't a public API, so we can rename it at will whenever :).
There was a problem hiding this comment.
You can be "actively connected to the peer" (i.e., "live") but the channel may not usable though, right? i.e., if the funding isn't locked.
Currently, the problem that I see is "live" is defined as a superset of "usable", but it seems like the opposite relation is true in actuality (i.e., when funding isn't locked). But really "live" has to do with the connection and "usable" has to do with the channel. So, maybe it should be is_live && is_funding_locked == is_usable?
Anyhow, it's not that I feel strongly on naming it list_live_channels, I'm just pointing out that there is inconsistency and ambiguity in the terminology that we are using. Some of the ambiguity is cleared up by the documentation (i.e. defining the meaning of "live") but is also contradictory (i.e., something "live" is usable but something just "usable" is not).
There was a problem hiding this comment.
I don't think we currently have any code to indicate "connected to peer but channel is not yet confirmed". We could add that too, I suppose, but for now I agree we should pick love or usable and stick to it.
|
Pushed a commit to rename ChannelDetails::is_live to is_usable. Hopefully this is ready to go now. |
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
LGTM. Internal renaming for consistency can come later.
TheBlueMatt
force-pushed
the
2021-05-more-chan-info
branch
from
49ff0b2 to
1f3b990
Compare
This matches is_usable_channels and slightly better captures the meaning.
TheBlueMatt
force-pushed
the
2021-05-more-chan-info
branch
from
1f3b990 to
62f466a
Compare
|
Squashed without changes |
|
Code Review ACK 62f466a Agree with Jeff reasoning on terminology point live-vs-usable meaning. |
3 participants
No description provided.