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.

Sign up for GitHub

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

Jump to bottom

bolt1: init tlvs set remote address #4864

Merged
merged 5 commits into from
Feb 21, 2022
Merged

bolt1: init tlvs set remote address #4864

merged 5 commits into from
Feb 21, 2022

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Oct 13, 2021 •
edited
Loading

This implements RFC lightning/bolts#917 that
will add the IP remote_addr to init tlvs on incoming IP connections.
The other node may decide to use that for public IP detection behind a NAT.
Currently it only logs this information.

It has already been cross-tested with ACINQ/eclair#1973

I think we should decouple this from the actual PR that does use that new information,
as several options are thinkable, and also because we can speed up cross testing this
way to get the RFC done quicker.

Next Steps: What do we do when we receive a remote_addr?

  • Have n multiple peers or a minimal percentage of peers report the same remote_addr to consider it valid.
  • Or verify the remote_addr by making a test connection to ourself (can work, must not).
  • Or making a test conenction to a random node known from gossip to check if he also reports that address.
  • If a remote_addr considered valid and not yet known, insert this into an node_annoucment update, if:
    • No other remote_address of the same type (IPv4/IPv6) has been used that way.
    • If same type already exist, drop the one that is reported by fewer of out peers.

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch 3 times, most recently from 16c3f6b to aacd2d5 Compare October 17, 2021 11:24
@vincenzopalazzo vincenzopalazzo mentioned this pull request Oct 17, 2021
Merged
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch 3 times, most recently from 5c886f3 to c87890e Compare October 19, 2021 09:45
@m-schmoock m-schmoock mentioned this pull request Oct 21, 2021
Merged
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch 3 times, most recently from dd984dc to 13d02a8 Compare October 26, 2021 07:52
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from 13d02a8 to 4df5fd9 Compare October 30, 2021 17:24
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from 4df5fd9 to 3cc2d85 Compare December 9, 2021 14:21
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch 9 times, most recently from ec18362 to fe2d9d2 Compare January 3, 2022 10:15
@m-schmoock m-schmoock mentioned this pull request Jan 4, 2022
Merged
rustyrussell
rustyrussell previously requested changes Jan 6, 2022
View reviewed changes
connectd/peer_exchange_initmsg.c Outdated Show resolved Hide resolved
tests/test_connection.py Outdated Show resolved Hide resolved
connectd/peer_exchange_initmsg.c Outdated Show resolved Hide resolved
connectd/connectd_wire.csv Outdated Show resolved Hide resolved
connectd/peer_exchange_initmsg.c Show resolved Hide resolved
vincenzopalazzo
vincenzopalazzo reviewed Jan 7, 2022
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch 3 times, most recently from 15a51ac to 1381544 Compare January 13, 2022 09:08
@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from 1381544 to 6278b85 Compare January 13, 2022 09:24
@m-schmoock m-schmoock marked this pull request as ready for review January 13, 2022 09:35
@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Jan 13, 2022 •
edited
Loading

@rustyrussell I now use wireaddr.h for marshalling, as the RFC will use the same data structure for this field. This leads to a lot of mock/include changes. If you want we could split up wireaddr.h into wireaddr.h (actual structure and wire parsing) and wireaddr_internal.h (just the _internal struct) and maybe wireaddr_utils.h (stuff like is_toraddr ...). Maybe that we we can get at least rid of the common/base32.h include that currently needs to be pulled in a lot of places.

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from 6278b85 to c573c15 Compare January 13, 2022 10:10
@m-schmoock
Copy link
Collaborator Author

Just a testflake in tests/test_plugin.py::test_coin_movement_notices

@m-schmoock
Copy link
Collaborator Author

Note: Bolt lightning/bolts#917 has been merged yesterday.
Cross testing has been made with this branch, not sure though if that's the perfectly correct order.

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from c573c15 to ed20b5a Compare January 18, 2022 09:18
@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Jan 18, 2022 •
edited
Loading

squashed some commits and removed unused struct.

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from ed20b5a to 0196fa5 Compare January 21, 2022 11:26
@m-schmoock
Copy link
Collaborator Author

Resolved conflicts and rebased on current master

@m-schmoock m-schmoock force-pushed the bolt1/init_tlvs_remote_address branch from 0196fa5 to bcce20f Compare February 8, 2022 09:02
@m-schmoock
wiregen: adds note about what <count> can be
89cd60b
@m-schmoock
peer_wire: add remote_addr to init_tlv
f5a450e 
Unfortunately we can't do any smart parsing here since
wiregen does not support switch/type cases for different
substructure unions yet. So just give us a pointer we can use.
cdecker
cdecker approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

ACK bcce20f

connectd/connectd.c Outdated Show resolved Hide resolved
lightningd/peer_control.c Outdated Show resolved Hide resolved
@cdecker cdecker dismissed rustyrussell’s stale review February 16, 2022 11:16

Comments appear to have been addressed.

@m-schmoock
Copy link
Collaborator Author

I just force pushed @cdecker 's changes/remarks

@rustyrussell
Copy link
Contributor

Ack 7baae67

@rustyrussell rustyrussell merged commit df9a34b into ElementsProject:master Feb 21, 2022
@m-schmoock m-schmoock deleted the bolt1/init_tlvs_remote_address branch February 22, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo left review comments

@cdecker cdecker cdecker approved these changes

@rustyrussell rustyrussell Awaiting requested review from rustyrussell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m-schmoock @rustyrussell @cdecker @vincenzopalazzo