Splice: Better balance checking #6748
Conversation
|
@rustyrussell One question I was unsure of here -> if there is an HTLC in the channel struct should that be counted as a pending HTLC or should it only be considered if it is in certain specific states? |
ddustin
force-pushed
the
ddustin/splice_stuck_htlc
branch
from
bacb181
to
90b0245
Compare
| if (!amount_msat_add(&funding_amount, | ||
| funding_amount, | ||
| pending_htlcs[TX_INITIATOR])) | ||
| peer_failed_warn(peer->pps, &peer->channel_id, | ||
| "Unable to calculate starting channel amount"); | ||
| if (!amount_msat_add(&funding_amount, | ||
| funding_amount, | ||
| pending_htlcs[TX_ACCEPTER])) | ||
| peer_failed_warn(peer->pps, &peer->channel_id, | ||
| "Unable to calculate starting channel amount"); |
There was a problem hiding this comment.
I cried in the last couple of days to understand this logic here, and I am still not sure 100% for what this code does
Can you clarify this operation? and why do we need to look at the pending htlc? we can not just look at the peer->channel->view? to get the balance of the channel. it should contains the on-flight htlc too, right?
Please add a couple of comments in the code to make the review easy to look at, thanks
There was a problem hiding this comment.
It seems pending HTLC amounts are not included in the channel->view amount as my initial splicing code assumed. I've asked rusty for some clarification here: #6748 (comment)
There was a problem hiding this comment.
I am still confused by this change here.
From the following comment https://github.com/ElementsProject/lightning/blob/master/common/initial_channel.h#L19 the peer->channel->view contains all the pending change, this mean that it contains also the pending_htlcs?
Is the comment outdated?
Can you clarify these operations for readers, please?
There was a problem hiding this comment.
as far as I understand, peer->channel->view only contains the main outputs.
I think there's a semantic confusion here, because the doc on the view field talks about "pending" in the sense of whether protocol changes (such as add_htlc) are committed with a signature, but the new code here is using "pending" in the sense of "live" or "in-flight" HTLCs. should probably drop the use of the world pending.
There was a problem hiding this comment.
Yes, pending means "uncommitted" in that comment. Should be fixed!
ddustin
force-pushed
the
ddustin/splice_stuck_htlc
branch
from
90b0245
to
9ef52ad
Compare
rustyrussell
force-pushed
the
ddustin/splice_stuck_htlc
branch
from
9ef52ad
to
8d0d7b7
Compare
|
Fixed up Python whitespace which was annoying flake8, and rebased onto #6802 for test flake fixes. |
rustyrussell
force-pushed
the
ddustin/splice_stuck_htlc
branch
from
8d0d7b7
to
c0dc177
Compare
* Regression test added for Issue ElementsProject#6572 (issuecomment-1730808863) w/stuck HTLC * `check_balance` adjusted to calculate pending HTLCs explicitly * Test confirmed to fail prior to PR ElementsProject#6713 ChangeLog-Fixed: Issue splicing with pending / stuck HTLCs fixed.
rustyrussell
force-pushed
the
ddustin/splice_stuck_htlc
branch
from
c0dc177
to
864c363
Compare
|
Rebased on master. |
| if (!amount_msat_add(&funding_amount, | ||
| funding_amount, | ||
| pending_htlcs[TX_INITIATOR])) | ||
| peer_failed_warn(peer->pps, &peer->channel_id, | ||
| "Unable to calculate starting channel amount"); | ||
| if (!amount_msat_add(&funding_amount, | ||
| funding_amount, | ||
| pending_htlcs[TX_ACCEPTER])) | ||
| peer_failed_warn(peer->pps, &peer->channel_id, | ||
| "Unable to calculate starting channel amount"); |
There was a problem hiding this comment.
I am still confused by this change here.
From the following comment https://github.com/ElementsProject/lightning/blob/master/common/initial_channel.h#L19 the peer->channel->view contains all the pending change, this mean that it contains also the pending_htlcs?
Is the comment outdated?
Can you clarify these operations for readers, please?
|
Yes, note that htlcs need to be in a committed state for splicing, so this code (which omits the committed check for htlcs) is actually correct here (though not general). I don't think you need to recalc funding_amount though. channel->funding_sats is this? |
check_balanceadjusted to calculate pending HTLCs explicitlyChangeLog-Fixed: Issue splicing with pending / stuck HTLCs fixed.