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

routerrpc: fix estimateroutefee for public route hints #9433

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+45 −6
Open

routerrpc: fix estimateroutefee for public route hints #9433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15dea83
routerrpc: fix estimateroutefee for public route hints
hieblmi Jan 22, 2025
eb1d56a
docs: update release notes
hieblmi Jan 22, 2025
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
6 changes: 5 additions & 1 deletion docs/release-notes/release-notes-0.19.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@
https://github.com/lightningnetwork/lnd/pull/9253)

* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9322) that caused
estimateroutefee to ignore the default payment timeout.
estimateroutefee to ignore the default payment timeout.

* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9433) that caused
`estimateroutefee` to assume probing an LSP when given a route hint containing
a public channel.

# New Features

Expand Down
6 changes: 6 additions & 0 deletions lnrpc/routerrpc/router_backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/lightningnetwork/lnd/channeldb"
"github.com/lightningnetwork/lnd/feature"
"github.com/lightningnetwork/lnd/fn/v2"
"github.com/lightningnetwork/lnd/graph/db/models"
"github.com/lightningnetwork/lnd/htlcswitch"
"github.com/lightningnetwork/lnd/lnrpc"
"github.com/lightningnetwork/lnd/lntypes"
Expand Down Expand Up @@ -44,6 +45,11 @@ type RouterBackend struct {
// SelfNode is the vertex of the node sending the payment.
SelfNode route.Vertex

// FetchChannelInfo is a closure that we'll use the fetch the latest
// routing state for a particular channel identified by its channel ID.
Copy link
Collaborator

Choose a reason for hiding this comment

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

routing state? maybe just write ChannelPolicy ?

FetchChannelInfo func(chanID uint64) (*models.ChannelEdgeInfo,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although we are currently only interested in a bool return value, we might still return the full chaninfo as a prevision for future use case, hmm I am not sure maybe we just simplify the function signature for now and only return a bool (also then maybe changing the name of the function)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also repurpose some of the other endpoints exposed here already, like FetchChannelCapacity or FetchChannelEndpoints, which would return EdgeNotFound

*models.ChannelEdgePolicy, *models.ChannelEdgePolicy, error)

// FetchChannelCapacity is a closure that we'll use the fetch the total
// capacity of a channel to populate in responses.
FetchChannelCapacity func(chanID uint64) (btcutil.Amount, error)
Expand Down
19 changes: 15 additions & 4 deletions lnrpc/routerrpc/router_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/lightningnetwork/lnd/aliasmgr"
"github.com/lightningnetwork/lnd/channeldb"
"github.com/lightningnetwork/lnd/fn/v2"
graphdb "github.com/lightningnetwork/lnd/graph/db"
"github.com/lightningnetwork/lnd/lnrpc"
"github.com/lightningnetwork/lnd/lnrpc/invoicesrpc"
"github.com/lightningnetwork/lnd/lntypes"
Expand Down Expand Up @@ -552,7 +553,7 @@ func (s *Server) probePaymentRequest(ctx context.Context, paymentRequest string,
// If the hints don't indicate an LSP then chances are that our probe
// payment won't be blocked along the route to the destination. We send
// a probe payment with unmodified route hints.
if !isLSP(hints) {
if !isLSP(hints, s.cfg.RouterBackend) {
probeRequest.RouteHints = invoicesrpc.CreateRPCRouteHints(hints)
return s.sendProbePayment(ctx, probeRequest)
}
Expand Down Expand Up @@ -627,12 +628,13 @@ func (s *Server) probePaymentRequest(ctx context.Context, paymentRequest string,
// isLSP checks if the route hints indicate an LSP. An LSP is indicated with
// true if the last node in each route hint has the same node id, false
// otherwise.
func isLSP(routeHints [][]zpay32.HopHint) bool {
func isLSP(routeHints [][]zpay32.HopHint, routerBackend *RouterBackend) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not make isLSP part of ther routerbackend func (s *Server) isLSP makes it way cleaner ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think passing in a function in instead is good enough, also makes it easier to mock in a unit test

if len(routeHints) == 0 || len(routeHints[0]) == 0 {
return false
}

refNodeID := routeHints[0][len(routeHints[0])-1].NodeID
refHint := routeHints[0][len(routeHints[0])-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we rename the ref prefixes to something that's more explanatory?

refNodeID := refHint.NodeID
for i := 1; i < len(routeHints); i++ {
// Skip empty route hints.
if len(routeHints[i]) == 0 {
Expand All @@ -649,7 +651,16 @@ func isLSP(routeHints [][]zpay32.HopHint) bool {
}
}

return true
// If the ref node hint contains a public channel we can send a probe to
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's move it at the beginning so we can do the check first something like this:
We just check, does the refChanID exist, if it does we exit early.

_, _, _, err := routerBackend.FetchChannelInfo(refHint.ChannelID)
	if err == nil {
		return false
	}

// it directly, hence don't signal an LSP.
_, _, _, err := routerBackend.FetchChannelInfo(refHint.ChannelID)
if errors.Is(err, graphdb.ErrEdgeNotFound) {
return true
}

// The ref node hint contains a public channel, so we don't signal an
// LSP.
return false
}

// prepareLspRouteHints assumes that the isLsp heuristic returned true for the
Expand Down
9 changes: 8 additions & 1 deletion lnrpc/routerrpc/router_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package routerrpc

import (
"context"
"github.com/lightningnetwork/lnd/graph/db/models"
"testing"
"time"

Expand Down Expand Up @@ -387,7 +388,13 @@ func TestIsLsp(t *testing.T) {

for _, tc := range lspTestCases {
t.Run(tc.name, func(t *testing.T) {
require.Equal(t, tc.isLsp, isLSP(tc.routeHints))
require.Equal(t, tc.isLsp, isLSP(tc.routeHints, &RouterBackend{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to add a unit test for this behaviour.

FetchChannelInfo: func(chanID uint64) (*models.ChannelEdgeInfo,
*models.ChannelEdgePolicy, *models.ChannelEdgePolicy, error) {

return nil, nil, nil, nil
},
}))
if !tc.isLsp {
return
}
Expand Down
11 changes: 11 additions & 0 deletions rpcserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,17 @@ func (r *rpcServer) addDeps(s *server, macService *macaroons.Service,

routerBackend := &routerrpc.RouterBackend{
SelfNode: selfNode.PubKeyBytes,
FetchChannelInfo: func(chanID uint64) (*models.ChannelEdgeInfo,
*models.ChannelEdgePolicy, *models.ChannelEdgePolicy,
error) {

info, policy1, policy2, err := graph.FetchChannelEdgesByID(chanID) //nolint:ll
Copy link
Collaborator

Choose a reason for hiding this comment

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

put nolint in a line before.

if err != nil {
return nil, nil, nil, err
}

return info, policy1, policy2, nil
},
FetchChannelCapacity: func(chanID uint64) (btcutil.Amount,
error) {

Expand Down
Loading