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

offers: don't include unadvertised nodes as introduction node candidates #154

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Aug 27, 2024

When requesting an invoice from an offer, we need to create a reply path for the offer node to reply to. When creating this path we currently don't check whether the introduction node we selected is advertised (with public channels) or not:

/// create_reply_path creates a blinded path to provide to the offer maker when requesting an

This PR makes sure we pick an advertised node, which the offer node will actually be able to reach, potentially improving payment reliability.

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (259de8b) to head (5acb051).
Report is 8 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #154   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines         105     126   +21     
======================================
- Misses        105     126   +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@orbitalturtle orbitalturtle force-pushed the intro-announced-peers branch 2 times, most recently from dd43ae2 to 0ecc4ea Compare August 27, 2024 06:03
@orbitalturtle orbitalturtle requested a review from dunxen August 28, 2024 04:34
Copy link
Collaborator

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Feel free to fix the nit in a followup if you'd like.

src/lndk_offers.rs Outdated Show resolved Hide resolved
// advertised node with at least one public channel.
match connector.get_node_info(peer.pub_key, true).await {
Ok(node) => {
if node.channels.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've confirmed that the corresponding LND call only gives announced channels here, so looks good 👍

@orbitalturtle orbitalturtle force-pushed the intro-announced-peers branch from 0ecc4ea to 5acb051 Compare August 28, 2024 17:55
@orbitalturtle orbitalturtle merged commit 83e1c90 into lndk-org:master Aug 28, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants