Limit number of HTLCs in custom channel#1132
Conversation
|
Calculated max values from |
39c71b1 to
cfe3016
Compare
Pull Request Test Coverage Report for Build 11935690852Details
💛 - Coveralls |
dstadulis
left a comment
There was a problem hiding this comment.
Added a few design-option questions
|
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 |
1bbe907 to
d74dd65
Compare
fac3956 to
37efc27
Compare
37efc27 to
75e27cf
Compare
|
Rebased to make sure CI runs against new |
psbt_channel_funder.go
Outdated
| }, | ||
| }), | ||
| lndclient.WithRemoteReserve(CustomChannelRemoteReserve), | ||
| lndclient.WithRemoteMaxHtlc(req.RemoteMaxHtlc), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess that's what would happen, yes. But just to make sure I added a condition.
| // 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 |
There was a problem hiding this comment.
Is this TODO still applicable?
There was a problem hiding this comment.
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.
75e27cf to
fffac93
Compare
ffranr
left a comment
There was a problem hiding this comment.
Clean commits! Thanks.
Why are we limiting HTLCs based on the TAP level content? Basically, what's the resolution here: f807b5f#r1776473336
dcee975 to
ae05c63
Compare
ae05c63 to
4bf7c8d
Compare
|
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. |
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.
d8631de to
9a7a4c1
Compare
Fixes #1149
Depends on #1130.
Addresses comment in lightningnetwork/lnd#9072 (comment).