Imputed costs v2 #3
Conversation
Pull reviewers statsStats of the last 30 days for lnd:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @feelancer21, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant new feature that allows users to influence Lightning Network routing decisions by defining and applying 'imputed costs'. These are virtual costs or penalties assigned to specific node pairs, which are then factored into the pathfinding algorithm. The primary purpose is to provide a flexible mechanism for users to guide route selection based on their own criteria, beyond just standard routing fees. This is a proof-of-concept (v2) that lays the groundwork for more sophisticated routing strategies, enabling users to prioritize certain paths or avoid others based on custom cost models. The imputed costs are currently in-memory only and not persisted across restarts.
Highlights
- New Imputed Cost Restriction for Routing and Payments: I've introduced a new
ImputedCostRestrictionmessage and integrated it intoQueryRoutesRequestandSendPaymentRequest. This allows users to specify virtual cost criteria, including a namespace and a total cost limit, when querying for routes or sending payments. - Experimental RPCs for Imputed Cost Management: I've added three new experimental RPCs to the
routerrpc.Routerservice:XImportImputedCosts,XQueryImputedCosts, andXDeleteImputedCosts. These APIs enable users to programmatically manage in-memory imputed cost configurations, influencing routing decisions without affecting actual network fees. - Centralized Imputed Cost Management System: A new
ImputedCostManagercomponent has been implemented to handle the storage and retrieval of imputed cost namespaces and their associated parameters. This manager provides the core logic for applying virtual costs during pathfinding and is integrated across various routing components. - Integration of Imputed Costs into Pathfinding Algorithm: The pathfinding algorithm (
findPath) has been updated to incorporateimputedDistInfoand utilize anImputedCostControl. This allows the algorithm to consider and apply these virtual costs, dynamically adjusting path selection based on user-defined preferences and ensuring cost limits are respected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an imputed costs feature for routing decisions in the Lightning Network. Imputed costs allow users to apply additional virtual costs or penalties to specific node pairs to influence route selection without modifying actual network fees.
- Adds a new
ImputedCostManagerwith namespace-based cost configurations - Integrates imputed cost calculations into the pathfinding algorithm
- Provides new gRPC APIs for importing, querying, and deleting imputed cost configurations
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server.go | Initializes ImputedCostManager and integrates it into server routing components |
| rpcserver.go | Adds ImputedCostManager to RPC server dependencies |
| routing/router.go | Adds ImputedCostRestriction field to LightningPayment struct |
| routing/payment_session.go | Integrates imputed cost control into payment session route requests |
| routing/pathfind.go | Implements imputed cost calculations in the pathfinding algorithm |
| routing/imputed_cost.go | Core implementation of imputed cost manager and related types |
| routing/imputed_cost_test.go | Comprehensive test suite for imputed cost functionality |
| routing/heap.go | Adds imputedDistInfo field to nodeWithDist struct |
| lnrpc/routerrpc/* | Generated gRPC code for new imputed cost APIs |
| lnrpc/lightning.proto | Adds ImputedCostRestriction message to QueryRoutesRequest |
| return res, nil | ||
| } | ||
|
|
||
| // ImputedCostParameters defines the cost parameters for a node pair, mirroring |
There was a problem hiding this comment.
The comment contains a capitalization inconsistency. 'ImputedCostParameters' should be 'imputedCostParameters' to match the actual struct name on line 123.
| // ImputedCostParameters defines the cost parameters for a node pair, mirroring | |
| // imputedCostParameters defines the cost parameters for a node pair, mirroring |
| // will happen and this value remains unused. | ||
| minShardAmt lnwire.MilliSatoshi | ||
|
|
||
| // replace with Interface? |
There was a problem hiding this comment.
This TODO comment should be removed or expanded into a proper TODO with context about what interface should be used and why.
| // replace with Interface? | |
| // TODO: Consider replacing ImputedCostManager with an interface to improve | |
| // modularity and testability. This would allow different implementations | |
| // of cost management to be injected, making the code more flexible and | |
| // easier to test. Define an interface that includes the required methods | |
| // from ImputedCostManager and update the code accordingly if needed. |
| ShouldSetExpEndorsement func() bool | ||
|
|
||
| // Provides access to the ImputedCostNamespaces. | ||
| // Question: Replace with interface? |
There was a problem hiding this comment.
This question comment should be removed or converted to a proper TODO with specific details about what interface should be considered.
| // Question: Replace with interface? | |
| // TODO: Evaluate whether ImputedCostManager should be replaced with an interface |
| fromNode := m.selfNode | ||
|
|
||
| for _, hop := range attempt.Route.Hops { | ||
| // TODO(feelancer21): Think about the correct amount!!! |
There was a problem hiding this comment.
This TODO comment is unclear and uses excessive exclamation marks. It should specify what needs to be considered about the amount calculation.
| // TODO(feelancer21): Think about the correct amount!!! | |
| // TODO(feelancer21): Determine the correct amount to use for cost calculation. | |
| // Consider how hop.AmtToForward interacts with the cost model and ensure | |
| // that the calculation aligns with the expected behavior of the payment routing. |
| p.payment.ImputedCostRestriction, | ||
| ) | ||
| if err != nil { | ||
| return nil, errNoPathFound |
There was a problem hiding this comment.
When imputed cost manager fails to get payment control, returning 'errNoPathFound' is misleading. This should return the actual error from GetPaymentControl to provide better debugging information.
| return nil, errNoPathFound | |
| return nil, err |
| paymentControl := channeldb.NewPaymentControl(dbs.ChanStateDB) | ||
|
|
||
| s.controlTower = routing.NewControlTower(paymentControl) | ||
|
|
There was a problem hiding this comment.
The paymentControl and controlTower initialization has been moved up, but the original location is still shown with removal markers. This creates duplicate initialization of these components, which could lead to bugs.
| paymentControl := channeldb.NewPaymentControl(dbs.ChanStateDB) | |
| s.controlTower = routing.NewControlTower(paymentControl) | |
| // Removed duplicate initialization of paymentControl and controlTower. |
There was a problem hiding this comment.
Code Review
This pull request introduces the concept of "imputed costs" to influence route selection in the pathfinding algorithm. It adds new RPCs for managing imputed cost data and integrates this data into the payment and routing logic. The changes are well-structured and include new logic for cost calculation, management, and corresponding tests. My review focuses on adherence to the provided style guide, potential improvements for code clarity and testability, and some design considerations that might be worth discussing.
| fromNode := m.selfNode | ||
|
|
||
| for _, hop := range attempt.Route.Hops { | ||
| // TODO(feelancer21): Think about the correct amount!!! |
There was a problem hiding this comment.
This TODO comment indicates uncertainty about the correctness of the amount being used. This should be resolved before merging, as it could affect the accuracy of cost calculations.
| XQueryImputedCosts is an experimental API that retrieves the currently | ||
| active imputed cost configurations for the specified namespaces. If no | ||
| namespaces are specified, all configured namespaces will be returned. | ||
| */ | ||
| rpc XQueryImputedCosts (QueryImputedCostsRequest) | ||
| returns (QueryImputedCostsResponse); | ||
|
|
||
| /* | ||
| XDeleteImputedCosts is an experimental API that permanently removes the | ||
| specified namespaces and all their associated imputed cost configurations. | ||
| */ |
|
|
||
| message ImputedCostParameters { | ||
| // Success-based imputed cost rate in parts per million (ppm) of the amount | ||
| // sent. These virtual costs are only applied when the payment succeeds, | ||
| // and due to pathfinding algorithm constraints, only non-negative values | ||
| // are currently supported. | ||
| int32 cost_rate_ppm = 1; | ||
|
|
||
| // Success-based base imputed cost in millisatoshis. This fixed virtual cost | ||
| // is only applied when the payment succeeds, regardless of the payment | ||
| // amount. Due to pathfinding algorithm constraints, only non-negative | ||
| // values are currently supported. | ||
| int32 cost_base_msat = 2; | ||
|
|
||
| // Attempt-based imputed cost rate in parts per million (ppm) of the amount | ||
| // sent. These virtual costs are applied for every routing attempt through | ||
| // this node pair, regardless of whether the payment ultimately succeeds or | ||
| // fails. Due to pathfinding algorithm constraints, only non-negative values | ||
| // are currently supported. | ||
| int32 attempt_cost_rate_ppm = 3; | ||
|
|
||
| // Attempt-based base imputed cost in millisatoshis. This fixed virtual cost | ||
| // is applied for every routing attempt through this node pair, regardless | ||
| // of the payment amount or ultimate success/failure. Due to pathfinding | ||
| // algorithm constraints, only non-negative values are currently supported. | ||
| int32 attempt_cost_base_msat = 4; |
There was a problem hiding this comment.
The comments for these fields state that only non-negative values are currently supported. Using int32 allows negative values, which could be misleading. Consider using uint32 to enforce this constraint at the protobuf level, or add validation logic in the application layer. If negative values are planned for future use, this can be left as is, but it's worth a discussion.
|
|
||
| // Provides access to the ImputedCostNamespaces. | ||
| // Question: Replace with interface? | ||
| ImputedCostManager *routing.ImputedCostManager |
There was a problem hiding this comment.
The comment here raises a valid point. Using an interface for ImputedCostManager instead of a concrete type would improve testability and decoupling. It's a good practice to depend on abstractions rather than concretions. I recommend making this change.
lnrpc/routerrpc/router_backend.go
Outdated
| return nil, fmt.Errorf("total cost limit must be non-negative:"+ | ||
| " %d", restr.TotalCostLimitMsat) |
There was a problem hiding this comment.
This error message formatting violates the style guide for long error messages.1 It should be on a single line with string concatenation.
Style Guide References
| return nil, fmt.Errorf("total cost limit must be non-negative:"+ | |
| " %d", restr.TotalCostLimitMsat) | |
| return nil, fmt.Errorf("total cost limit must be non-negative: %d", | |
| restr.TotalCostLimitMsat) |
Footnotes
| func (m *ImputedCostManager) calcShardLimit(paymentHash lntypes.Hash, shardAmt, | ||
| costLimit lnwire.MilliSatoshi, model imputedCostModel) ( | ||
| lnwire.MilliSatoshi, error) { |
There was a problem hiding this comment.
This function signature exceeds the 80-character line limit, violating the style guide.1 Please wrap the arguments to new lines.
Style Guide References
| func (m *ImputedCostManager) calcShardLimit(paymentHash lntypes.Hash, shardAmt, | |
| costLimit lnwire.MilliSatoshi, model imputedCostModel) ( | |
| lnwire.MilliSatoshi, error) { | |
| func (m *ImputedCostManager) calcShardLimit(paymentHash lntypes.Hash, | |
| shardAmt, costLimit lnwire.MilliSatoshi, model imputedCostModel) ( | |
| lnwire.MilliSatoshi, error) { |
Footnotes
feelancer21
force-pushed
the
imputed-costs-v2
branch
7 times, most recently
from
61db23d to
b8489a1
Compare
- Add experimental imputed costs API to router service with three new RPCs: XImportImputedCosts, XQueryImputedCosts, XDeleteImputedCosts. - Introduces messages for ImputedCostNamespace, ImputedCostPair and ImputedCostParameters with support for success-based and attempt-based cost calculations. - Adds imputed_cost_restrictions field to SendPaymentRequest and QueryRoutesRequest for total routing cost limits. - Compiling of the protos - Update of router.yaml
We added the ImputedCostManager to SessionSource and paymentSession and fixed the existing utests.
`queryicm` queries the internal state of the imputed cost manager.
feelancer21
force-pushed
the
imputed-costs-v2
branch
from
76cfd40 to
f36dc87
Compare
v2 of PoC PR
Chnge Description
Description of change / link to associated issue.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.