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

Limit number of HTLCs in custom channel #1132

Merged
merged 6 commits into from
Nov 20, 2024
Merged

Limit number of HTLCs in custom channel #1132

merged 6 commits into from
Nov 20, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Sep 20, 2024

Fixes #1149

Depends on #1130.

Addresses comment in lightningnetwork/lnd#9072 (comment).

@guggero
Copy link
Member Author

guggero commented Sep 20, 2024

Calculated max values from TestMaxCommitSigMsgSize:

    aux_leaf_signer_test.go:111: Last valid commit sig msg size with: numAssetIDs=0, numHTLCs=966
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=1, numHTLCs=373
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=2, numHTLCs=231
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=3, numHTLCs=166
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=4, numHTLCs=130
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=5, numHTLCs=107
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=6, numHTLCs=91
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=7, numHTLCs=79
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=8, numHTLCs=70
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=9, numHTLCs=63
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=10, numHTLCs=57

@guggero guggero force-pushed the limit-asset-id-htlc branch from 39c71b1 to cfe3016 Compare September 20, 2024 12:16
@coveralls
Copy link

coveralls commented Sep 20, 2024

Pull Request Test Coverage Report for Build 11935690852

Details

  • 0 of 84 (0.0%) changed or added relevant lines in 4 files are covered.
  • 18 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.04%) to 41.263%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapcfg/server.go 0 1 0.0%
server.go 0 2 0.0%
psbt_channel_funder.go 0 21 0.0%
tapchannel/aux_funding_controller.go 0 60 0.0%
Files with Coverage Reduction New Missed Lines %
tapchannel/aux_funding_controller.go 1 0.0%
asset/mock.go 1 92.51%
commitment/tap.go 2 84.43%
asset/asset.go 2 81.13%
tapchannel/aux_leaf_signer.go 3 36.33%
tapdb/universe.go 4 80.91%
universe/interface.go 5 50.65%
Totals Coverage Status
Change from base Build 11920687788: -0.04%
Covered Lines: 25469
Relevant Lines: 61723

💛 - Coveralls

Copy link
Collaborator

@dstadulis dstadulis left a comment

Choose a reason for hiding this comment

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

Added a few design-option questions

tapchannel/aux_funding_controller.go Show resolved Hide resolved
tapchannel/aux_funding_controller.go Show resolved Hide resolved
tapchannel/aux_funding_controller.go Show resolved Hide resolved
tapchannel/aux_leaf_signer_test.go Show resolved Hide resolved
@dstadulis dstadulis added this to the v0.5 milestone Sep 23, 2024
@Roasbeef
Copy link
Member

Roasbeef commented Oct 8, 2024

Ok, discussed offline a bit, and I understand now the need to limit on the asset ID basis. Rather than asset ID, it's more accurate to say that we need to limit the total number of UTXOs that can exist within a funding output. Each time we create an HTLC, we may need to reference one or more asset UTXOs from the funding output. Each of those means an input referenced, which itself needs a signature. As a result, a single HTLC may actually need several HTLCs to be transmitted over the wire.

Without moving to a more efficient encoding (not much room can be gained), or to a new way of transmitting the signatures, then we need to limit the total amount of asset UTXOs that can reside within a funding output, or we run into message size limits on the protocol level.

One alternative path is to add the equivalent of SIGHASH_NOINPUT on the protocol layer. If we add this, then when we send HTLCs, we can actually send the second-level signatrues alongside them, as we just need the pkScript to be stable, which is the case for the next commitment. I think this breaks down though, as for the commitment after that, a different per-commitment-secret is used (new revocation secret), so that original signature can no longer be used.

@guggero guggero force-pushed the update-to-lnd-18-4 branch 8 times, most recently from 1bbe907 to d74dd65 Compare October 22, 2024 10:44
Base automatically changed from update-to-lnd-18-4 to main October 22, 2024 14:36
@guggero guggero force-pushed the limit-asset-id-htlc branch 2 times, most recently from fac3956 to 37efc27 Compare October 29, 2024 19:00
@guggero guggero force-pushed the limit-asset-id-htlc branch from 37efc27 to 75e27cf Compare November 6, 2024 14:39
@guggero
Copy link
Member Author

guggero commented Nov 6, 2024

Rebased to make sure CI runs against new litd branch. Also taking out of draft state since the discussions showed we want this safety feature as a short-term stop-gap solution.

@guggero guggero marked this pull request as ready for review November 6, 2024 14:40
@guggero guggero requested review from Roasbeef and ffranr November 6, 2024 14:40
@lightninglabs-deploy
Copy link

@Roasbeef: review reminder
@guggero, remember to re-request review from reviewers when ready

tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved
@@ -110,6 +110,7 @@ func (l *LndPbstChannelFunder) OpenChannel(ctx context.Context,
},
}),
lndclient.WithRemoteReserve(CustomChannelRemoteReserve),
lndclient.WithRemoteMaxHtlc(req.RemoteMaxHtlc),
Copy link
Member

Choose a reason for hiding this comment

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

Should we only set it if the value is above zero?

Or I guess lnd will just pick that up as not set, and apply the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that's what would happen, yes. But just to make sure I added a condition.

tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
// have a decent number of HTLCs available. See Godoc of maxNumAssetIDs
// for more information.
//
// TODO(guggero): This following code is obviously wrong and needs to be
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO still applicable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need to actually count the number of different asset IDs (tranches) that are used for funding a channel, once we allow grouped assets to be used. So this will change in 0.6.

@guggero guggero force-pushed the limit-asset-id-htlc branch from 75e27cf to fffac93 Compare November 14, 2024 11:21
@guggero guggero requested a review from Roasbeef November 14, 2024 11:21
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Clean commits! Thanks.

Why are we limiting HTLCs based on the TAP level content? Basically, what's the resolution here: f807b5f#r1776473336

tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved
tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved
tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved
tapchannel/aux_funding_controller.go Show resolved Hide resolved
@guggero guggero force-pushed the limit-asset-id-htlc branch 2 times, most recently from dcee975 to ae05c63 Compare November 18, 2024 16:33
@guggero guggero requested a review from ffranr November 18, 2024 16:34
@guggero guggero force-pushed the limit-asset-id-htlc branch from ae05c63 to 4bf7c8d Compare November 19, 2024 19:17
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🔋

@guggero
Copy link
Member Author

guggero commented Nov 20, 2024

Looks like we forgot to add the commitment type to the channel acceptor. Didn't notice this litd itest failure before, since it ran against the wrong branch.
So we'll need to wait for lightningnetwork/lnd#9288 and then bump the lnd version.

This test attempts to find out what the maximum number of HTLC
signatures we can pack into a CommitSig message is. This number depends
on the number of different fungible asset pieces (asset IDs) we have, as
we need to provide a signature for each piece.

The resulting values are:

    aux_leaf_signer_test.go:111: Last valid commit sig msg size with: numAssetIDs=0, numHTLCs=966
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=1, numHTLCs=373
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=2, numHTLCs=231
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=3, numHTLCs=166
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=4, numHTLCs=130
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=5, numHTLCs=107
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=6, numHTLCs=91
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=7, numHTLCs=79
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=8, numHTLCs=70
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=9, numHTLCs=63
    aux_leaf_signer_test.go:103: Last valid commit sig msg size with: numAssetIDs=10, numHTLCs=57
We want to be able to dictate how many HTLCs the remote side can add to
the channel we create. We'll do the same for the responder side with a
channel acceptor in one of the following commits.
We add a placeholder implementation that currently does nothing but will
help us to think about this problem when we implement fungible asset
support in tapd.
This fixes a CI check that was missed in a previously merged PR.
@guggero guggero force-pushed the limit-asset-id-htlc branch from d8631de to 9a7a4c1 Compare November 20, 2024 14:43
@guggero guggero merged commit e3a0aa1 into main Nov 20, 2024
18 checks passed
@guggero guggero deleted the limit-asset-id-htlc branch November 20, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Ensure HTLC signature transport demands conform to BOLT wire-message size limitations
6 participants