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.

Sign up for GitHub

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

Jump to bottom

htlcswitch: fix flake in TestChannelLinkCancelFullCommitment #9221

Merged
merged 3 commits into from
Nov 1, 2024
Merged

htlcswitch: fix flake in TestChannelLinkCancelFullCommitment #9221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8311bc5
htlcswitch: fix flake in `TestChannelLinkCancelFullCommitment`
yyforyongyu Oct 25, 2024
cfa073e
htlcswitch: increase logs
ziggie1984 Oct 31, 2024
5fe9bb2
htlcswitch: adopt test for dust threshold
ziggie1984 Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion htlcswitch/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3793,7 +3793,8 @@ func (l *channelLink) processExitHop(add lnwire.UpdateAddHTLC,
// ADD, nor will we settle the corresponding invoice or respond with the
// preimage.
if l.cfg.HodlMask.Active(hodl.ExitSettle) {
l.log.Warnf(hodl.ExitSettle.Warning())
l.log.Warnf("%s for htlc(rhash=%x,htlcIndex=%v)",
hodl.ExitSettle.Warning(), add.PaymentHash, add.ID)

return nil
}
Expand Down
28 changes: 14 additions & 14 deletions htlcswitch/link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/lightningnetwork/lnd/channeldb"
"github.com/lightningnetwork/lnd/channeldb/models"
"github.com/lightningnetwork/lnd/contractcourt"
"github.com/lightningnetwork/lnd/fn"
"github.com/lightningnetwork/lnd/htlcswitch/hodl"
"github.com/lightningnetwork/lnd/htlcswitch/hop"
"github.com/lightningnetwork/lnd/input"
Expand Down Expand Up @@ -864,7 +865,7 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) {
)

// Fill up the commitment from Alice's side with 20 sat payments.
count := (input.MaxHTLCNumber / 2)
count := int(maxInflightHtlcs)
amt := lnwire.NewMSatFromSatoshis(20000)

htlcAmt, totalTimelock, hopsForwards := generateHops(amt,
Expand Down Expand Up @@ -902,17 +903,17 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) {

// Now make an additional payment from Alice to Bob, this should be
// canceled because the commitment in this direction is full.
err = <-makePayment(
resp := makePayment(
n.aliceServer, n.bobServer, firstHop, hopsForwards, amt,
htlcAmt, totalTimelock,
).err
if err == nil {
t.Fatalf("overflow payment should have failed")
}
lerr, ok := err.(*LinkError)
if !ok {
t.Fatalf("expected LinkError, got: %T", err)
}
)

paymentErr, timeoutErr := fn.RecvOrTimeout(resp.err, 30*time.Second)
require.NoError(t, timeoutErr, "timeout receiving payment resp")
require.Error(t, paymentErr, "overflow payment should have failed")
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved

var lerr *LinkError
require.ErrorAs(t, paymentErr, &lerr)

msg := lerr.WireMessage()
if _, ok := msg.(*lnwire.FailTemporaryChannelFailure); !ok {
Expand All @@ -938,10 +939,9 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) {

// Ensure that all of the payments sent by alice eventually succeed.
for errChan := range aliceErrChan {
err := <-errChan
if err != nil {
t.Fatalf("alice payment failed: %v", err)
}
receivedErr, err := fn.RecvOrTimeout(errChan, 30*time.Second)
require.NoError(t, err, "payment timeout")
require.NoError(t, receivedErr, "alice payment failed")
}
}

Expand Down
64 changes: 29 additions & 35 deletions htlcswitch/switch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/lightningnetwork/lnd/htlcswitch/hodl"
"github.com/lightningnetwork/lnd/htlcswitch/hop"
"github.com/lightningnetwork/lnd/lntest/mock"
"github.com/lightningnetwork/lnd/lntest/wait"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/lightningnetwork/lnd/lnwire"
Expand Down Expand Up @@ -4289,14 +4290,19 @@ func TestSwitchDustForwarding(t *testing.T) {

// We'll test that once the default threshold is exceeded on the
// Alice -> Bob channel, either side's calls to SendHTLC will fail.
//
// Alice will send 354 HTLC's of 700sats. Bob will also send 354 HTLC's
// of 700sats.
numHTLCs := 354
numHTLCs := maxInflightHtlcs
aliceAttemptID, bobAttemptID := numHTLCs, numHTLCs
amt := lnwire.NewMSatFromSatoshis(700)
aliceBobFirstHop := n.aliceChannelLink.ShortChanID()

// We decreased the max number of inflight HTLCs therefore we also need
// do decrease the max fee exposure.
maxFeeExposure := lnwire.NewMSatFromSatoshis(74500)
n.aliceChannelLink.cfg.MaxFeeExposure = maxFeeExposure
n.firstBobChannelLink.cfg.MaxFeeExposure = maxFeeExposure

// Alice will send 50 HTLC's of 700sats. Bob will also send 50 HTLC's
// of 700sats.
sendDustHtlcs(t, n, true, amt, aliceBobFirstHop, numHTLCs)
sendDustHtlcs(t, n, false, amt, aliceBobFirstHop, numHTLCs)

Expand All @@ -4318,22 +4324,13 @@ func TestSwitchDustForwarding(t *testing.T) {
OnionBlob: blob,
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried to understand this testcase but tbh, I don't really understand it. In the description it says that we are testing the treshold here however I think what we are really testing is the channel contraint of the max_inflight_htlcs we are allowed to put onto a channel. So my question is what do we really want to test here.

When sending dust (sendDustHtlcs) we are putting 50 htlcs onto the channel in each direction, the 51 one fails:

2024-10-31 10:28:31.426 [WRN] HSWC: ChannelLink(bcfe226d21c9936ee578df4afb65835abb9790392e45bbaf0c245394430838f8:0): Unable to handle downstream add HTLC: commitment transaction exceed max htlc number
2024-10-31 10:28:31.426 [DBG] HSWC: Storing result for attemptID=50
2024-10-31 10:28:31.439 [DBG] HSWC: Tearing down circuit with FAIL pkt, removing circuit=(Chan ID=0:0:0, HTLC ID=50) with keystone=(Chan ID=0:0:0, HTLC ID=0)

And all the followup tests just try to add another htlc to the link but its already blocked, so I think this testcase might be a relict of the former dust-treshold implementation, which now changed and is by default 500_000 sats ?

Copy link
Member Author

Choose a reason for hiding this comment

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

the main test I think is in checkAlmostDust or assertAlmostDust, so this PR focuses on removing the test flakes, but yeah agree we should start making the tests here better since it gives flakes quite often.

Copy link
Collaborator

Choose a reason for hiding this comment

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

decreased the fee-exposure and made sure we hit the error of being overexposed and not hitting the error that we hit the channel contraint.

checkAlmostDust := func(link *channelLink, mbox MailBox,
whoseCommit lntypes.ChannelParty) bool {

timeout := time.After(15 * time.Second)
pollInterval := 300 * time.Millisecond
expectedDust := 354 * 2 * amt
// This is the expected dust without taking the commitfee into account.
expectedDust := maxInflightHtlcs * 2 * amt

for {
<-time.After(pollInterval)

select {
case <-timeout:
return false
default:
}
assertAlmostDust := func(link *channelLink, mbox MailBox,
whoseCommit lntypes.ChannelParty) {

err := wait.NoError(func() error {
linkDust := link.getDustSum(
whoseCommit, fn.None[chainfee.SatPerKWeight](),
)
Expand All @@ -4347,26 +4344,29 @@ func TestSwitchDustForwarding(t *testing.T) {
}

if totalDust == expectedDust {
break
return nil
}
}

return true
return fmt.Errorf("got totalDust=%v, expectedDust=%v",
totalDust, expectedDust)
}, 15*time.Second)
require.NoError(t, err, "timeout checking dust")
}

// Wait until Bob is almost at the fee threshold.
bobMbox := n.bobServer.htlcSwitch.mailOrchestrator.GetOrCreateMailBox(
n.firstBobChannelLink.ChanID(),
n.firstBobChannelLink.ShortChanID(),
)
require.True(
t, checkAlmostDust(
n.firstBobChannelLink, bobMbox, lntypes.Local,
),
)
assertAlmostDust(n.firstBobChannelLink, bobMbox, lntypes.Local)

// Sending one more HTLC should fail. SendHTLC won't error, but the
// HTLC should be failed backwards.
// HTLC should be failed backwards. When sending we only check for the
// dust amount without the commitment fee. When the HTLC is added to the
// commitment state (link) we also take into account the commitment fee
// and with a fee of 6000 sat/kw and a commitment size of 724 (non
// anchor channel) we are overexposed in fees (maxFeeExposure) that's
// why the HTLC is failed back.
err = n.bobServer.htlcSwitch.SendHTLC(
aliceBobFirstHop, uint64(bobAttemptID), failingHtlc,
)
Expand Down Expand Up @@ -4412,9 +4412,7 @@ func TestSwitchDustForwarding(t *testing.T) {
aliceBobFirstHop, uint64(bobAttemptID), nondustHtlc,
)
require.NoError(t, err)
require.True(t, checkAlmostDust(
n.firstBobChannelLink, bobMbox, lntypes.Local,
))
assertAlmostDust(n.firstBobChannelLink, bobMbox, lntypes.Local)

// Check that the HTLC failed.
bobResultChan, err = n.bobServer.htlcSwitch.GetAttemptResult(
Expand Down Expand Up @@ -4492,11 +4490,7 @@ func TestSwitchDustForwarding(t *testing.T) {
aliceMbox := aliceOrch.GetOrCreateMailBox(
n.aliceChannelLink.ChanID(), n.aliceChannelLink.ShortChanID(),
)
require.True(
t, checkAlmostDust(
n.aliceChannelLink, aliceMbox, lntypes.Remote,
),
)
assertAlmostDust(n.aliceChannelLink, aliceMbox, lntypes.Remote)

err = n.aliceServer.htlcSwitch.SendHTLC(
n.aliceChannelLink.ShortChanID(), uint64(aliceAttemptID),
Expand Down
8 changes: 6 additions & 2 deletions htlcswitch/test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ import (
"github.com/stretchr/testify/require"
)

// maxInflightHtlcs specifies the max number of inflight HTLCs. This number is
// chosen to be smaller than the default 483 so the test can run faster.
const maxInflightHtlcs = 50

var (
alicePrivKey = []byte("alice priv key")
bobPrivKey = []byte("bob priv key")
Expand Down Expand Up @@ -145,7 +149,7 @@ func createTestChannel(t *testing.T, alicePrivKey, bobPrivKey []byte,
channelCapacity),
ChanReserve: aliceReserve,
MinHTLC: 0,
MaxAcceptedHtlcs: input.MaxHTLCNumber / 2,
MaxAcceptedHtlcs: maxInflightHtlcs,
}
aliceCommitParams := channeldb.CommitmentParams{
DustLimit: btcutil.Amount(200),
Expand All @@ -157,7 +161,7 @@ func createTestChannel(t *testing.T, alicePrivKey, bobPrivKey []byte,
channelCapacity),
ChanReserve: bobReserve,
MinHTLC: 0,
MaxAcceptedHtlcs: input.MaxHTLCNumber / 2,
MaxAcceptedHtlcs: maxInflightHtlcs,
}
bobCommitParams := channeldb.CommitmentParams{
DustLimit: btcutil.Amount(800),
Expand Down
Loading