-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
Calculated max values from
|
39c71b1
to
cfe3016
Compare
Pull Request Test Coverage Report for Build 11935690852Details
💛 - Coveralls |
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 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
@@ -110,6 +110,7 @@ func (l *LndPbstChannelFunder) OpenChannel(ctx context.Context, | |||
}, | |||
}), | |||
lndclient.WithRemoteReserve(CustomChannelRemoteReserve), | |||
lndclient.WithRemoteMaxHtlc(req.RemoteMaxHtlc), |
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.
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.
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.
// 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.
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?
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.
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
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.
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
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 🔋
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).