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

Send remote address in init #1973

Merged
merged 2 commits into from
Jan 21, 2022
Merged

Send remote address in init #1973

merged 2 commits into from
Jan 21, 2022

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Sep 28, 2021

This adds the option to report a remote IP address back to a connecting peer using the init message.
A node can decide to use that information to discover a potential update to its public IP address (NAT) and use that for a node_announcement update message containing the new address.

See lightning/bolts#917

I have successfully tested cross-compatibility with c-lightning ElementsProject/lightning#4864

@m-schmoock
Copy link

Hey @t-bast ,
clightning is tracking this in ElementsProject/lightning#4864
It can send and receive the remote_addr.
I'm currently developing the part where the daemon decides what to do when we receive a remote_addr.

We can do cross compat tests when you are ready.

@t-bast
Copy link
Member Author

t-bast commented Oct 21, 2021

@m-schmoock cool, we're mostly at the same point then. I think it's worth shipping the part where we just send/receive that field whenever possible, and we can decide later how to act on it based on what we see in the wild.

This branch is ready for cross compat tests if you want. You can build it, run it locally and test it against your c-lightning branch. I have a lot of sample scripts to do cross-compat tests here if that can help.

I'll do the same on my side, I'll compile your branch and test it with mine.

@m-schmoock
Copy link

Nice, just remember to compile it with EXPERIMENTAL_FEATURES=1 as the feature will be hidden behind precompiler directives otherwise.

@t-bast
Copy link
Member Author

t-bast commented Oct 22, 2021

@m-schmoock I tested this between eclair and c-lightning, we correctly parse the tlvs 👍

However your implementation currently sends back private addresses (e.g. 127.0.0.1), I think you should filter these and not include them in the tlv stream, right? It is filtered out on the eclair side, but it would be better if it wasn't sent in the first place?

@m-schmoock
Copy link

@t-bast
Yep, I still need to do that logic.

@m-schmoock
Copy link

Hi @t-bast
I have not forgotten that PR on our side, looking forward doing it this week.
We merged in experimental DNS just yet ( ElementsProject/lightning#4829 ) So if you want to pick up on that topic as well (since they are related) we can proceed on the RFCs...

@t-bast
Copy link
Member Author

t-bast commented Nov 30, 2021

I have not forgotten that PR on our side, looking forward doing it this week.

No worries, there's no rush here, whenever you're ready.

We merged in experimental DNS just yet ( ElementsProject/lightning#4829 ) So if you want to pick up on that topic as well (since they are related) we can proceed on the RFCs...

To be honest I'm not that interested in the DNS one for now, that's not something our users have been asking for at all and we don't have a good use for it in the short term, so it's lower on my priority list.

@t-bast
Copy link
Member Author

t-bast commented Jan 5, 2022

@m-schmoock cross-compatibility with ElementsProject/lightning#4864 is working like a charm, I tested in both directions, the reported IP address is correctly logged.

@t-bast t-bast marked this pull request as ready for review January 5, 2022 15:43
@t-bast t-bast requested a review from pm47 January 5, 2022 15:43
@codecov-commenter
Copy link

Codecov Report

Merging #1973 (ecb36b1) into master (27579a5) will decrease coverage by 0.07%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1973      +/-   ##
==========================================
- Coverage   87.62%   87.55%   -0.08%     
==========================================
  Files         166      166              
  Lines       12886    12900      +14     
  Branches      543      543              
==========================================
+ Hits        11292    11295       +3     
- Misses       1594     1605      +11     
Impacted Files Coverage Δ
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.21% <ø> (ø)
...q/eclair/wire/protocol/LightningMessageTypes.scala 92.50% <83.33%> (-4.28%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 86.74% <100.00%> (+0.91%) ⬆️
...cinq/eclair/wire/protocol/SetupAndControlTlv.scala 100.00% <100.00%> (ø)
...n/scala/fr/acinq/eclair/channel/DustExposure.scala 96.96% <0.00%> (-3.04%) ⬇️
...main/scala/fr/acinq/eclair/router/Validation.scala 90.76% <0.00%> (-2.70%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 84.07% <0.00%> (-0.89%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 87.80% <0.00%> (-0.08%) ⬇️

This adds the option to report a remote IP address back to a connecting
peer using the `init` message. A node can decide to use that information
to discover a potential update to its public IP address (NAT) and use
that for a `node_announcement` update message containing the new address.

See lightning/bolts#917
@t-bast t-bast merged commit f8d507b into master Jan 21, 2022
@t-bast t-bast deleted the init-remote-addr branch January 21, 2022 11:40
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.

4 participants