routerrpc: isolate LSP route fee probes#10771
routerrpc: isolate LSP route fee probes#10771yyforyongyu wants to merge 2 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello, 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 addresses a bug in the LSP route fee estimation process where multiple probes were incorrectly reusing payment-level state. By ensuring each probe utilizes a unique payment hash, the system now correctly isolates individual LSP probes. The changes also include architectural improvements to facilitate better test coverage for the probe dispatch mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
43b2eda to
499e23c
Compare
There was a problem hiding this comment.
Code Review
This pull request fixes an issue in EstimateRouteFee where multiple LSP probes were reusing the same payment hash and CLTV delta. The changes introduce a unique payment hash for each LSP probe and refactor probePaymentRequest to allow for better testability via a stubbed sender. A new unit test, TestProbePaymentRequestUsesUniqueHashPerLSP, has been added to verify this behavior. One piece of feedback was provided regarding an error message that incorrectly refers to a 'preimage' instead of a 'payment hash'.
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Low (1 file)
AnalysisThe highest-severity file is With only 2 non-test, non-generated files changed and 39 lines of non-test/generated additions+deletions, none of the bump thresholds (>20 files, >500 lines, multiple critical packages) are triggered. Severity remains HIGH. This PR touches the router RPC server, which handles payment routing API calls. A knowledgeable engineer familiar with the routerrpc layer and Lightning payment flows should review it. To override, add a |
499e23c to
8eab1ed
Compare
|
should be not needed if #10772 lands |
I don't think so - why do we use the same hash for different probes? |
|
its a probe where the hash is not settleable anyways so why not reuse the dummy hash ? |
But this doesn't mean the same hash is better than a separate hash? I also don't understand why we wanna use the same hash in the first place - do we wanna make it easy for other nodes to link the behaviors here? and it makes the logs/attempt tracking more difficult here if they just show the same hash? unless the caller intended retries of the exact same logical probe, i don't think we can benefit from using the same hash. |
Summary
EstimateRouteFeeLSP probe so later probes cannot reuse payment-level state from earlier probes.EstimateRouteFeefix.Fix #10677
Bug
CI flaked in PR #10689 on the
bitcoind-etcditest job:https://github.com/lightningnetwork/lnd/actions/runs/24841352445/job/72716012767?pr=10689
Failed test:
Failure:
The test creates three public LSP hints for a private destination:
100200120The returned fee was Eve's fee, but the returned timelock was calculated with Bob's CLTV delta.
Root Cause
probePaymentRequestgenerated one random payment hash for the overall invoice probe request, then reused that same hash for every per-LSP probe.The payment lifecycle keys payment state by payment hash. If Bob is probed first, the payment record is initialized with Bob's
FinalCltvDelta=100. When Eve is probed later with the same payment hash, the router treats it as another attempt for the same payment and can reuse payment-level state from the first probe. The route can then carry Eve's fee but Bob's CLTV delta.This is order-dependent because LSPs are iterated from a Go map. If Eve is probed first, the test passes. If Bob is probed first, Eve can inherit Bob's CLTV delta and the test fails.
Log Evidence
I downloaded the itest artifact
logs-itest-bitcoind-etcd.zipfrom the failed run. Relevant log file:Passing two-LSP case: Eve is probed first, uses expiry/cltv
864, and the final result returnstimelock: 944.Failing three-LSP case: Bob is probed first with payment hash
a539...andcltv 764. Eve is probed next with the same payment hash, logsResuming HTLC attempt, still usescltv 764, succeeds with Eve's fee, and returnstimelock: 844.The
100delta between expected944and actual844matches Bob's LSP CLTV delta being used where Eve's200should have been used.Fix
Generate a fresh random payment hash for each LSP probe iteration. This makes each LSP probe an independent payment from the payment lifecycle's perspective, while keeping the existing single-hash behavior for the non-LSP probe path.
Testing
GOWORK=off go test ./lnrpc/routerrpc