Conversation
A previous bug fix was wrong... In the case where an HTLC sends all the balance from the initiator to the recipient, then the initiator has zero balance _and_ there is no HTLC going to the initiator. So the root split still becomes nil and we panic. So we re-declare the rule for the case where the initiator has zero balance: If the initiator doesn't have any balance to carry the split root, then the very first HTLC will carry the split root. If there is no HTLC, it means all the balance is on the recipient's side and there is no split (hence no split root required).
This was referenced Jul 17, 2024
guggero
force-pushed
the
channel-bug-fixes
branch
from
19d9072 to
a4e894a
Compare
Roasbeef
reviewed
Jul 17, 2024
If there is no root locator defined when creating a split commitment, we return an error instead of running into a panic later on.
If we fail to fund the channel (for example because we don't have enough BTC or asset funds), we inform the remote about it, so they can remove the pending channel from their in-memory state and allow us to try again.
guggero
force-pushed
the
channel-bug-fixes
branch
from
a4e894a to
d56be62
Compare
By default, we only want to show assets in the asset list that are ready to be spent. So we don't want to include not yet confirmed freshly minted assets (those are the only assets that are materialized in the asset table while not being confirmed yet). We add a new RPC and CLI flag that allows the user to show the assets on explicit request.
To avoid attempting to spend freshly minted assets that haven't been confirmed yet, we set the minimum anchor height to 1 to avoid selecting them when funding any transaction.
guggero
force-pushed
the
channel-bug-fixes
branch
from
d56be62 to
0fcb312
Compare
gijswijs
reviewed
Jul 18, 2024
Contributor
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Codewise, lgtm.
I do have a few questions so that I can confirm my understanding.
This PR specifically fixes the issue where the split root becomes nil with commit cb19dd6.
This issue came to light with this report #1019
It also fixes the issue of poor UX that resulted in selecting unconfirmed minting transactions mentioned here #1026
The are 2 todos mentioned in that thread
As a funder, release locked/leased UTXOs if funding fails
As a recipient, do an asset specific universe sync on incoming funding request, to make sure we know at least the meta info and other issuance proof
Those 2 todos are not fixed in this PR but in PR #1029, Correct?
I want to be sure that we can close that issue.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1019.
Fixes #1026.
Fixes another bug found by increasing the test coverage in lightninglabs/lightning-terminal#797: When there is no balance and no HTLC going toward the initiator of a channel, we still need to create a split root output.
We then also fix an issue that we didn't send a funding error to the remote party when we couldn't fund the channel. So they had it pending until it timed out after 10 minutes (or restart) and we couldn't try again ("number of pending channel exceeds maximum").
And finally, we make sure we don't allow not yet confirmed freshly minted assets to be used for funding any asset transaction, including channel openings. To make the user experience around those unconfirmed assets we also filter them by default and add a new count and flag to the RPC interface.