-
Notifications
You must be signed in to change notification settings - Fork 2.2k
routerrpc: add log line for probing the invoice request #9991
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
routerrpc: add log line for probing the invoice request #9991
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
8acbc4c to
d089cd3
Compare
|
@hieblmi we might think also of defaulting LSP to false if the LSP node is not known to the network. |
Please! |
If you are looking for context why I am in favor of dropping the strict "one routing hint = LSP" assumption, check this dirty workaround we had to do in order to not have all probes to us fail: BoltzExchange/boltz-backend#975 |
IIUC you have an invoice to be probed that has a single non-existent route hint? If that's the case, why is the route hint in the invoice if you want to probe the public node of the invoice? Route hints in an invoice will be interpreted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change LGTM pending guggero's comment.
@ziggie1984 would you mind adding another commit that renames idMatchesRefNode in isLsp with matchesDestNode? RefNode was used before but forgotten to change, so just for consistency.
Correct.
Because it's a special routing hint, see https://docs.boltz.exchange/api/magic-routing-hints. I think it makes a lot of sense instead of failing probes to non-existent routing hints to drop the assumption that this is an LSP. And instead probe the invoice's node. Just like Ziggie said. Let me add some background info of how we even got here: It started out with the issue that Binance Lightning withdrawals into Boltz swaps were failing. We then went on a very tedious journey to find out that they do probing, figured out which exact probing method in LND Binance was using ( Setting LSP to false if the routing hint is not a known public node solves the issue and hopefully saves this incredible debug timesink to anyone who does similar things in the future. Hope that helps! |
d089cd3 to
31df58e
Compare
31df58e to
43edfeb
Compare
Was testing some estimateRoute probing and could barely see whats happening in the background, so added those important logs.