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

Fixes related to address inside conf. #234

Closed
wants to merge 2 commits into from

Conversation

Harshit933
Copy link
Collaborator

@Harshit933 Harshit933 commented May 26, 2024

There are two things that are addressed here :

  1. Throw a warning that the node is not reachable if the announce-addr inside conf is not present.
  2. Stop the node in case the announce-addr mentioned inside the conf is wrong or could not be binded.
    Fixes Remove the default address as 127.0.0.1  #231

This commit removes the `127.0.0.1` address as the default address
for the node to listen for incoming connections. Also, we do not stop
the node, we just give the warning that the node is not reachable.
@@ -203,6 +204,7 @@ impl LampoPeerManager {
}
result
});
result.join().unwrap()?;
Copy link
Owner

Choose a reason for hiding this comment

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

Why this change? you are blocking lampo so from this join the code will never return, Have you tested the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We simply return from the thread when we are not able to bind the address.
https://github.com/vincenzopalazzo/lampo.rs/blob/a973f1548e0f076cf7016783f6eb021d1407e928/lampod/src/ln/peer_manager.rs#L147C67-L147C74
Then we just join the child thread with the main thread and see if there are any errors that we get from there.
I have tested this.

Copy link
Owner

Choose a reason for hiding this comment

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

are you sure that the join will not block the execution of the lampo::listen? can you add an extra log::info and see if you see it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still need to see this, Will do in this week!

Copy link
Owner

Choose a reason for hiding this comment

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

Take your time, this will be in the august release

When the address specified inside the `conf` is invalid or could
not be binded we should ideally stop the node. This case was not
handled earlier.
@vincenzopalazzo vincenzopalazzo added this to the v24.06 milestone Jun 17, 2024
@vincenzopalazzo vincenzopalazzo added the 👀 - seeking reviews This PR is seeking reviews label Jun 17, 2024
@vincenzopalazzo vincenzopalazzo modified the milestones: v24.06, v24.08 Jun 29, 2024
@vincenzopalazzo
Copy link
Owner

Closing because inactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 - seeking reviews This PR is seeking reviews
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove the default address as 127.0.0.1
2 participants