[custom channels]: Fix bandwidth manager and other bugs #1236
Conversation
Pull Request Test Coverage Report for Build 12233514979Details
💛 - Coveralls |
tapchannel/commitment.go
Outdated
| feePerKw = view.FeePerKw | ||
| // When determining whether we need an anchor output, we need to | ||
| // count the non-asset HTLCs as well. | ||
| numHTLCs = uint64(len(nonAssetView.OurUpdates) + |
There was a problem hiding this comment.
would this lead to stuck force-closes? (covered in the LitD itest?)
trying to understand if we ever had coverage for this
There was a problem hiding this comment.
No, the channel will not force close (at least from my observations), but the HTLC will be rejected. This is covered in the new litd itest, where we pay sats over an asset channel.
There was a problem hiding this comment.
Won't this result in us over counting? The non asset view will still have an manifest HTLC for a given asset HTLC. I think it's enough to just have this combined sum here, and drop the numHTLCs increment in the inner loop.
There was a problem hiding this comment.
No, the non-asset view and the asset view are fully distinct. And even if we were over counting, we only use numHTLCs > 0 as a condition for determining whether we need a commitment anchor output.
There was a problem hiding this comment.
I thought about this again and my initial approach wasn't 100% correct. It is correct that the assetView and
nonAssetView are non-overlapping. But we need to replicate this behavior:
https://github.com/lightningnetwork/lnd/blob/ab7aae07085ec244f7888e990e8d8cc6ae6dfa83/lnwallet/commitment.go#L706
So we also need to check for dust, which I now added.
|
when we do "breaking version bumps" I wonder how can we get the Could maybe pin the LitD commit that we want to use to run the itests, then CI would use that |
Yeah, it's not optimal. We could point the litd itest to an unmerged commit, but IMO that's also not great, since we need to merge So I think the best strategy is just to always follow up a |
tapchannel/commitment.go
Outdated
| feePerKw = view.FeePerKw | ||
| // When determining whether we need an anchor output, we need to | ||
| // count the non-asset HTLCs as well. | ||
| numHTLCs = uint64(len(nonAssetView.OurUpdates) + |
There was a problem hiding this comment.
Won't this result in us over counting? The non asset view will still have an manifest HTLC for a given asset HTLC. I think it's enough to just have this combined sum here, and drop the numHTLCs increment in the inner loop.
guggero
force-pushed
the
bandwidth-manager-switch
branch
from
1039ab6 to
6eed85e
Compare
guggero
force-pushed
the
bandwidth-manager-switch
branch
from
6eed85e to
3c019a4
Compare
This commit fixes an issue where we wouldn't count non-asset HTLCs when deciding whether the commitment transaction would require a commitment anchor output for the remote party, so the number of allocations would be out of sync with the number of outputs on the commitment transaction.
Because the traffic shaper is now also used for checking the bandwidth when forwarding HTLCs to the final recipient, we also need to check our local buy quotes for a quote with the given RFQ ID.
Fixes #1234. Simplifies and unifies the marshaling of sell quotes and adds the asset amount by converting the request's max amount using the quote.
guggero
force-pushed
the
bandwidth-manager-switch
branch
from
3c019a4 to
2ed2772
Compare
Depends on lightningnetwork/lnd#9333.
Fixes the issue that would lead to a force close if the channel bandwidth of a target channel during a forward event isn't sufficient.
Also fixes #1234 (drive-by fix) and another edge case found during integration tests.