diff --git a/docs/release-notes/release-notes-0.21.0.md b/docs/release-notes/release-notes-0.21.0.md index 9749b580763..14e41b4bcc8 100644 --- a/docs/release-notes/release-notes-0.21.0.md +++ b/docs/release-notes/release-notes-0.21.0.md @@ -24,6 +24,12 @@ # New Features ## Functional Enhancements +- The second chance mechanism of the pathfinder was adjusted to apply a +significant penalty to the route instead of immediately retrying it. This means +that if a failure carries a fee policy update we'll still take it into account +but will retry that route with a lower priority. See more in the +[PR](https://github.com/lightningnetwork/lnd/pull/10278). + ## RPC Additions ## lncli Additions diff --git a/routing/missioncontrol.go b/routing/missioncontrol.go index b03724a2e62..9fba76ab2e8 100644 --- a/routing/missioncontrol.go +++ b/routing/missioncontrol.go @@ -672,12 +672,15 @@ func (m *MissionControl) applyPaymentResult( // Interpret result. i := interpretResult(&result.route.Val, result.failure.ValOpt()) + // This flag will be true if we grant a second chance to this route. + secondChance := false + if i.policyFailure != nil { if m.state.requestSecondChance( time.Unix(0, int64(result.timeReply.Val)), i.policyFailure.From, i.policyFailure.To, ) { - return nil + secondChance = true } } @@ -698,7 +701,10 @@ func (m *MissionControl) applyPaymentResult( // difference. The largest difference occurs when aprioriWeight is 1. In // that case, a node-level failure would not be applied to untried // channels. - if i.nodeFailure != nil { + // + // If a second chance is being given, skip the node-level failure + // recording. + if i.nodeFailure != nil && !secondChance { m.log.Debugf("Reporting node failure to Mission Control: "+ "node=%v", *i.nodeFailure) @@ -721,10 +727,24 @@ func (m *MissionControl) applyPaymentResult( pair, pairResult.amt) } + ts := time.Unix(0, int64(result.timeReply.Val)) + + // If we want to grant a second chance, we'll apply half of the + // full penalty in order to not totally exclude this route from + // future attempts, but also encourage alternative route + // selection. + if secondChance { + // To create this effect of applying half of the penalty + // we'll record this failure as if it happened a bit + // further in the past. The distance between current and + // recorded time is equal to the penalty half life, + // which is the time duration it takes for a penalty + // to decay to its half value. + ts = ts.Add(-1 * DefaultPenaltyHalfLife) + } + m.state.setLastPairResult( - pair.From, pair.To, - time.Unix(0, int64(result.timeReply.Val)), &pairResult, - false, + pair.From, pair.To, ts, &pairResult, false, ) } diff --git a/routing/missioncontrol_test.go b/routing/missioncontrol_test.go index aa13a064a61..b95c8db9296 100644 --- a/routing/missioncontrol_test.go +++ b/routing/missioncontrol_test.go @@ -228,7 +228,8 @@ func TestMissionControl(t *testing.T) { } // TestMissionControlChannelUpdate tests that the first channel update is not -// penalizing the channel yet. +// fully penalizing the channel. Instead we should see a half penalty being +// applied to the probability. func TestMissionControlChannelUpdate(t *testing.T) { ctx := createMcTestContext(t) @@ -237,7 +238,9 @@ func TestMissionControlChannelUpdate(t *testing.T) { ctx.reportFailure( 0, lnwire.NewFeeInsufficient(0, lnwire.ChannelUpdate1{}), ) - ctx.expectP(100, testAprioriHopProbability) + + // Apply the second chance half-penalty. + ctx.expectP(100, testAprioriHopProbability*0.5) // Report another failure for the same channel. We expect it to be // pruned. diff --git a/routing/router_test.go b/routing/router_test.go index b811793d258..f7345d9b137 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -781,6 +781,129 @@ func TestSendPaymentErrorFeeInsufficientPrivateEdge(t *testing.T) { ) } +// TestSendPaymentErrorFeeInsufficientSecondChance tests that the half-penalty +// behavior of the second chance logic is properly applied when receiving +// errors that contain channel updates. We expose the path where the second +// chance penalty along with the fee update are sufficient for a different route +// to be picked. +func TestSendPaymentErrorFeeInsufficientSecondChance(t *testing.T) { + t.Parallel() + + const startingBlockHeight = 101 + ctx := createTestCtxFromFile(t, startingBlockHeight, basicGraphFilePath) + + // Get the channel ID for the first hop of the route that will retrieve + // the second chance. + roasbeefSongoku := lnwire.NewShortChanIDFromInt( + ctx.getChannelIDFromAlias(t, "roasbeef", "songoku"), + ) + + var ( + preImage [32]byte + amt = lnwire.NewMSatFromSatoshis(1000) + expiryDelta = uint16(32) + ) + + // Craft a LightningPayment struct that'll send a payment from roasbeef + // to elst. Initially a path via songoku will be picked as it's the one + // with significantly lower fees. + payment := createDummyLightningPayment( + t, ctx.aliases["elst"], amt, + ) + + // Grab the chan ID that we'll apply the channel update on. + chanID := ctx.getChannelIDFromAlias(t, "songoku", "sophon") + + // Grab the chan ID of the expensive channel. We'll use this channel's + // policy as a reference to what will be provided in the other channel's + // update. + expChanID := ctx.getChannelIDFromAlias(t, "roasbeef", "phamnuwen") + _, expPolicy, _, err := ctx.graph.FetchChannelEdgesByID(expChanID) + require.NoError(t, err) + + // Prepare an error update for the channel where the fees are updated to + // be equal to the alternative (more expensive) route. We also subtract + // some minimal fee delta to make it look as if this route is still more + // favorable (by a small margin). Although, because of the second-chance + // penalty, we'll end up using the alternative route. + feeDelta := uint32(1000) + newFeeBase := uint32(expPolicy.FeeBaseMSat) - feeDelta + newFeePpm := uint32(expPolicy.FeeProportionalMillionths) - feeDelta + + errChanUpdate := lnwire.ChannelUpdate1{ + ShortChannelID: lnwire.NewShortChanIDFromInt(chanID), + Timestamp: uint32(testTime.Add(time.Minute).Unix()), + BaseFee: newFeeBase, + FeeRate: newFeePpm, + TimeLockDelta: expiryDelta, + } + signErrChanUpdate(t, ctx.privKeys["songoku"], &errChanUpdate) + + // We'll now modify the SendHTLC method to return an error for the + // songoku->sophon channel. + errorReturned := false + copy(preImage[:], bytes.Repeat([]byte{9}, 32)) + + dispatch, ok := ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld) + require.True(t, ok) + + dispatch.setPaymentResult( + func(firstHop lnwire.ShortChannelID) ([32]byte, error) { + if firstHop != roasbeefSongoku || errorReturned { + return preImage, nil + } + + errorReturned = true + + return [32]byte{}, htlcswitch.NewForwardingError( + // Within our error, we'll add a + // channel update which is meant to + // reflect the new fee schedule for the + // node/channel. + &lnwire.FailFeeInsufficient{ + Update: errChanUpdate, + }, 1, + ) + }, + ) + + // Send off the payment request to the router, what's expected to happen + // here is: + // * attempt 1: roasbeef -> songoku -> sophon -> elst, with an error + // that contains a channel update for songoku->sophon. + // * attempt 2: roasbeef -> phamnuwen -> sophon -> elst, with success. + paymentPreImage, route, err := ctx.router.SendPayment(payment) + require.NoErrorf(t, err, "unable to send payment: %v", + payment.paymentHash) + + require.True(t, errorReturned, + "failed to simulate error in the first payment attempt", + ) + + // The route selected should have three hops. Make sure that, + // path: roasbeef -> pham nuwen -> sophon -> elst + // is selected. + require.Equal(t, 3, len(route.Hops), "incorrect route length") + + // The preimage should match up with the one created above. + require.Equal(t, + paymentPreImage[:], preImage[:], "incorrect preimage used", + ) + + // The route should have pham nuwen as the first hop. + require.Equal(t, route.Hops[0].PubKeyBytes, ctx.aliases["phamnuwen"], + "route should go through pham nuwen as first hop", + ) + + expectedFinalHop := ctx.getChannelIDFromAlias(t, "sophon", "elst") + + // The route should pass via the expected final hop. + require.Equal(t, + expectedFinalHop, route.FinalHop().ChannelID, + "route did not pass through expected final hop", + ) +} + // TestSendPaymentPrivateEdgeUpdateFeeExceedsLimit tests that upon receiving a // ChannelUpdate in a fee related error from the private channel, we won't // choose the route in our second attempt if the updated fee exceeds our fee