-
Notifications
You must be signed in to change notification settings - Fork 113
Adapt channel balance reporting to use confirmed candidate #634
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
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mod two nits.
We could also revisit whether to expose the balance candidates when exposing splicing, or whether we'd want to leave as-is (cc @jkczyz).
| } => { | ||
| let balance = balance_candidates.get(confirmed_balance_candidate_index).unwrap(); | ||
|
|
||
| Self::ClaimableOnChannelClose { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind updating the docs on our ClaimableOnChannelClose to note that we're just exposing this balance in a splicing scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added that values are based on the confirmed channel state, ignoring pending splices. I don't think it is what you suggest, that we are just exposing in a splicing scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted above that this is not the case. Confirmed but not locked splices are taken into account.
Also, LdkBalance::claimable_amount_satoshis uses a different way of calculating this when the index is 0. But maybe that is just to avoid the unwrap? (cc:: @wpaulino)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah when the index is 0, either we don't have a pending splice, or we do but none of the splice transactions has confirmed. In the latter case, we still want to report some indication of the unconfirmed splice balance.
With splicing now implemented, a channel may have multiple holder commitment transactions and corresponding balance candidates. ldk-node now reports the confirmed balance candidate rather than a single static balance, ensuring the exposed value matches the channel's onchain state. Other candidate balances remain internal for now.
7729e0c to
006a06e
Compare
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| /// force-closed now. Values do not take into account any pending splices and are only based | ||
| /// on the confirmed state of the channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is the case. It will take into account a pending splice if it is confirmed but not locked (i.e., doesn't have sufficient number of confirmations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, let us know if there is a way to update the docs to make this clear.
| } => { | ||
| let balance = balance_candidates.get(confirmed_balance_candidate_index).unwrap(); | ||
|
|
||
| Self::ClaimableOnChannelClose { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted above that this is not the case. Confirmed but not locked splices are taken into account.
Also, LdkBalance::claimable_amount_satoshis uses a different way of calculating this when the index is 0. But maybe that is just to avoid the unwrap? (cc:: @wpaulino)
With splicing now implemented, a channel may have multiple holder commitment transactions and corresponding balance candidates. ldk-node now reports the confirmed balance candidate rather than a single static balance, ensuring the exposed value matches the channel's onchain state. Other candidate balances remain internal for now.