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.

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

Use all valid routes during blinded path construction #9334

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
8 changes: 8 additions & 0 deletions docs/release-notes/release-notes-0.20.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@

# Bug Fixes

* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9334) in
constructing blinded payment paths for invoices. The restriction on the maximum
number of paths is now applied later during pathfinding, ensuring that all
potential paths are considered before filtering out ineligible ones. This allows
for better utilization of the available paths for payments.

# New Features

## Functional Enhancements
Expand Down Expand Up @@ -72,3 +78,5 @@
## Tooling and Documentation

# Contributors (Alphabetical Order)

* Pins
4 changes: 4 additions & 0 deletions lnrpc/invoicesrpc/addinvoice.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ type BlindedPathConfig struct {
// less than this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Squash this commit with the previous.

MinNumPathHops uint8

// MaxNumPaths is the maximum number of blinded paths to select.
MaxNumPaths uint8

// DefaultDummyHopPolicy holds the default policy values to use for
// dummy hops in a blinded path in the case where they cant be derived
// through other means.
Expand Down Expand Up @@ -542,6 +545,7 @@ func AddInvoice(ctx context.Context, cfg *AddInvoiceConfig,
},
MinNumHops: blindCfg.MinNumPathHops,
DefaultDummyHopPolicy: blindCfg.DefaultDummyHopPolicy,
MaxNumPaths: blindCfg.MaxNumPaths,
},
)
if err != nil {
Expand Down
11 changes: 9 additions & 2 deletions routing/blindedpath/blinded_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ type BuildBlindedPathCfg struct {
// route.
MinNumHops uint8

// MaxNumPaths is the maximum number of blinded paths to select.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The MaxNumPath can now be removed from BlindedPathRestrictions we are not using it anymore there. Super confusing to have all this MaxPaths variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right, thanks!

MaxNumPaths uint8

// DefaultDummyHopPolicy holds the policy values that should be used for
// dummy hops in the cases where it cannot be derived via other means
// such as averaging the policy values of other hops on the path. This
Expand Down Expand Up @@ -129,11 +132,15 @@ func BuildBlindedPaymentPaths(cfg *BuildBlindedPathCfg) (
// Not every route returned will necessarily result in a usable blinded
// path and so the number of paths returned might be less than the
// number of real routes returned by FindRoutes above.
paths := make([]*zpay32.BlindedPaymentPath, 0, len(routes))
paths := make([]*zpay32.BlindedPaymentPath, 0, min(len(routes),
int(cfg.MaxNumPaths)))

// For each route returned, we will construct the associated blinded
// payment path.
// payment path, until the maximum number of allowed paths is reached.
for _, route := range routes {
if len(paths) >= int(cfg.MaxNumPaths) {
break
}
// Extract the information we need from the route.
candidatePath := extractCandidatePath(route)

Expand Down
69 changes: 57 additions & 12 deletions routing/blindedpath/blinded_path_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -547,35 +547,58 @@ func genBlindedRouteData(rand *rand.Rand) *record.BlindedRouteData {
// an example mentioned in this spec document:
// https://github.com/lightning/bolts/blob/master/proposals/route-blinding.md
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try keeping the commit msg below 50 chars, so its not overflowing like you see here.

// This example does not use any dummy hops.
// Added Dave to test MaxNumPaths restriction.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a isolated test which does nothing more than testing the maxPaths. Now the test feels a bit confusing, we can just add a new test, make sure the paths are returned but not check all the values which we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can make it clear with better comments.

func TestBuildBlindedPath(t *testing.T) {
// Alice chooses the following path to herself for blinded path
// construction:
// Carol -> Bob -> Alice.
// Carol
// |
// -> Bob -> Alice.
// |
// Dave
// Let's construct the corresponding route.Route for this which will be
// returned from the `FindRoutes` config callback.
var (
privC, pkC = btcec.PrivKeyFromBytes([]byte{1})
privB, pkB = btcec.PrivKeyFromBytes([]byte{2})
privA, pkA = btcec.PrivKeyFromBytes([]byte{3})
_, pkD = btcec.PrivKeyFromBytes([]byte{4})

carol = route.NewVertex(pkC)
bob = route.NewVertex(pkB)
alice = route.NewVertex(pkA)
dave = route.NewVertex(pkD)

chanCB = uint64(1)
chanBA = uint64(2)
chanDB = uint64(3)
)

realRoute := &route.Route{
SourcePubKey: carol,
Hops: []*route.Hop{
{
PubKeyBytes: bob,
ChannelID: chanCB,
realRoute := []*route.Route{
{
SourcePubKey: carol,
Hops: []*route.Hop{
{
PubKeyBytes: bob,
ChannelID: chanCB,
},
{
PubKeyBytes: alice,
ChannelID: chanBA,
},
},
{
PubKeyBytes: alice,
ChannelID: chanBA,
},
{
SourcePubKey: dave,
Hops: []*route.Hop{
{
PubKeyBytes: bob,
ChannelID: chanDB,
},
{
PubKeyBytes: alice,
ChannelID: chanBA,
},
},
},
}
Expand All @@ -589,13 +612,17 @@ func TestBuildBlindedPath(t *testing.T) {
ChannelID: chanBA,
ToNode: alice,
},
chanDB: {
ChannelID: chanDB,
ToNode: bob,
},
}

paths, err := BuildBlindedPaymentPaths(&BuildBlindedPathCfg{
blindedPathCfg := (&BuildBlindedPathCfg{
FindRoutes: func(_ lnwire.MilliSatoshi) ([]*route.Route,
error) {

return []*route.Route{realRoute}, nil
return realRoute, nil
},
FetchChannelEdgesByID: func(chanID uint64) (
*models.ChannelEdgeInfo, *models.ChannelEdgePolicy,
Expand Down Expand Up @@ -623,10 +650,25 @@ func TestBuildBlindedPath(t *testing.T) {
ValueMsat: 1000,
MinFinalCLTVExpiryDelta: 12,
BlocksUntilExpiry: 200,
MaxNumPaths: 2,
})

// Two blinded payment paths are expected.
// Carol -> Bob and Dave -> Bob
paths, err := BuildBlindedPaymentPaths(blindedPathCfg)
require.NoError(t, err)
require.Len(t, paths, 2)

// Now lets test the MaxNumPaths restriction.
blindedPathCfg.MaxNumPaths = 1

// Just one blinded payment path is expected, restricted above by
// MaxNumPaths.
paths, err = BuildBlindedPaymentPaths(blindedPathCfg)
require.NoError(t, err)
require.Len(t, paths, 1)

// Continue testing all other expected values.
path := paths[0]

// Check that all the accumulated policy values are correct.
Expand Down Expand Up @@ -809,6 +851,7 @@ func TestBuildBlindedPathWithDummyHops(t *testing.T) {
MinHTLCMsat: 1000,
MaxHTLCMsat: lnwire.MaxMilliSatoshi,
},
MaxNumPaths: 1,
})
require.NoError(t, err)
require.Len(t, paths, 1)
Expand Down Expand Up @@ -988,6 +1031,7 @@ func TestBuildBlindedPathWithDummyHops(t *testing.T) {
MinHTLCMsat: 1000,
MaxHTLCMsat: lnwire.MaxMilliSatoshi,
},
MaxNumPaths: 1,
})
require.NoError(t, err)
require.Len(t, paths, 1)
Expand Down Expand Up @@ -1022,6 +1066,7 @@ func TestSingleHopBlindedPath(t *testing.T) {
ValueMsat: 1000,
MinFinalCLTVExpiryDelta: 12,
BlocksUntilExpiry: 200,
MaxNumPaths: 1,
})
require.NoError(t, err)
require.Len(t, paths, 1)
Expand Down
19 changes: 7 additions & 12 deletions routing/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -604,9 +604,6 @@ type BlindedPathRestrictions struct {
// our node along with an introduction node hop.
NumHops uint8

// MaxNumPaths is the maximum number of blinded paths to select.
Copy link
Collaborator

Choose a reason for hiding this comment

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

squash as well

MaxNumPaths uint8

// NodeOmissionSet is a set of nodes that should not be used within any
// of the blinded paths that we generate.
NodeOmissionSet fn.Set[route.Vertex]
Expand Down Expand Up @@ -696,18 +693,16 @@ func (r *ChannelRouter) FindBlindedPaths(destination route.Vertex,
return routes[i].probability > routes[j].probability
})

// Now just choose the best paths up until the maximum number of allowed
// paths.
bestRoutes := make([]*route.Route, 0, restrictions.MaxNumPaths)
for _, route := range routes {
if len(bestRoutes) >= int(restrictions.MaxNumPaths) {
break
}
log.Debugf("Found %v routes, discarded %v low probability routes",
len(paths), len(paths)-len(routes))

bestRoutes = append(bestRoutes, route.route)
// Return all routes.
allRoutes := make([]*route.Route, 0, len(routes))
for _, route := range routes {
allRoutes = append(allRoutes, route.route)
}

return bestRoutes, nil
return allRoutes, nil
}

// generateNewSessionKey generates a new ephemeral private key to be used for a
Expand Down
25 changes: 2 additions & 23 deletions routing/router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3153,12 +3153,11 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
}

// All the probabilities are set to 1. So if we restrict the path length
// to 2 and allow a max of 3 routes, then we expect three paths here.
// to 2, then we expect three paths here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

also squash this one which is basically a consequence of the change before.

routes, err := ctx.router.FindBlindedPaths(
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 3,
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
},
)
require.NoError(t, err)
Expand Down Expand Up @@ -3192,8 +3191,7 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
}

// Now, let's lower the MC probability of the B-D to 0.5 and F-D link to
// 0.25. We will leave the MaxNumPaths as 3 and so all paths should
// still be returned but the order should be:
// 0.25:
// 1) A -> C -> D
// 2) A -> B -> D
// 3) A -> F -> D
Expand All @@ -3203,7 +3201,6 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 3,
},
)
require.NoError(t, err)
Expand All @@ -3220,7 +3217,6 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 3,
},
)
require.NoError(t, err)
Expand All @@ -3230,27 +3226,12 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
"alice,charlie,dave",
})

// Change the MaxNumPaths to 1 to assert that only the best route is
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
// returned.
routes, err = ctx.router.FindBlindedPaths(
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 1,
},
)
require.NoError(t, err)
assertPaths(routes, []string{
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
"alice,bob,dave",
})

// Test the edge case where Dave, the recipient, is also the
// introduction node.
routes, err = ctx.router.FindBlindedPaths(
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 0,
NumHops: 0,
MaxNumPaths: 1,
},
)
require.NoError(t, err)
Expand All @@ -3265,7 +3246,6 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 3,
},
)
require.NoError(t, err)
Expand All @@ -3280,7 +3260,6 @@ func TestFindBlindedPathsWithMC(t *testing.T) {
dave, 1000, probabilitySrc, &BlindedPathRestrictions{
MinDistanceFromIntroNode: 2,
NumHops: 2,
MaxNumPaths: 3,
NodeOmissionSet: fn.NewSet(frank),
},
)
Expand Down
6 changes: 1 addition & 5 deletions rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -6029,7 +6029,6 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
blindingRestrictions := &routing.BlindedPathRestrictions{
MinDistanceFromIntroNode: globalBlindCfg.MinNumRealHops,
NumHops: globalBlindCfg.NumHops,
MaxNumPaths: globalBlindCfg.MaxNumPaths,
Copy link
Collaborator

Choose a reason for hiding this comment

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

also squash this one, its way easier to review them in one single commit

NodeOmissionSet: fn.NewSet[route.Vertex](),
}

Expand All @@ -6046,10 +6045,6 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
if blindCfg.NumHops != nil {
blindingRestrictions.NumHops = uint8(*blindCfg.NumHops)
}
if blindCfg.MaxNumPaths != nil {
blindingRestrictions.MaxNumPaths =
uint8(*blindCfg.MaxNumPaths)
}

for _, nodeIDBytes := range blindCfg.NodeOmissionList {
vertex, err := route.NewVertexFromBytes(nodeIDBytes)
Expand Down Expand Up @@ -6150,6 +6145,7 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
MaxHTLCMsat: 0,
},
MinNumPathHops: blindingRestrictions.NumHops,
MaxNumPaths: globalBlindCfg.MaxNumPaths,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you still need to check for this:

if blindCfg.MaxNumPaths != nil {

}
}

Expand Down
Loading