Added option to send remote IP to peers by Psycho-Pirate · Pull Request #1326 · lightningdevkit/rust-lightning

Psycho-Pirate · 2022-02-21T21:26:35Z

Psycho-Pirate · 2022-02-21T21:29:10Z

I am new to this community and using rust in general.
Please have a look and suggest any corrections. @TheBlueMatt
How should I add the address to the init message?

TheBlueMatt

For adding it to the message, you'll need to edit InitMessage and add a new field, then add it to the TLV encoding in msgs.rs as defined for InitMessage.

Psycho-Pirate · 2022-02-22T15:43:09Z

For adding it to the message, you'll need to edit InitMessage and add a new field, then add it to the TLV encoding in msgs.rs as defined for InitMessage.

I am not able to find where InitMessage is defined in msg.rs. Where should I add the field exactly?

Also in peer_handler.rs, don't I have to use the newly stored NetAddress somewhere? probably in handle_message function?

TheBlueMatt · 2022-02-22T16:35:34Z

Oops sorry, its Init, not InitMessage.

Psycho-Pirate · 2022-02-22T18:02:02Z

I was able to find this in msgs.rs

pub struct Init {
	/// The relevant features which the sender supports
	pub features: InitFeatures,
}

Do I have to add the field here or where InitFeatures is implemented?

Psycho-Pirate · 2022-02-22T18:05:36Z

Also is there any documentation I can refer to? I did go through the ldk website but there wasn't much info present there on init messages.

TheBlueMatt · 2022-02-22T20:33:22Z

You should add it as a field in the Init struct. If you're not sure where to go, I recommend starting with the Rust book as an introduction to types and structs in Rust, see https://doc.rust-lang.org/stable/book/

Psycho-Pirate · 2022-02-23T15:33:40Z

You should add it as a field in the Init struct. If you're not sure where to go, I recommend starting with the Rust book as an introduction to types and structs in Rust, see https://doc.rust-lang.org/stable/book/

Thanks, I have added the net_address as a field and resolved some of the errors. Please tell me what else needs to be done.

Psycho-Pirate · 2022-02-25T19:26:04Z

I ran the following code in encoding_init and I am getting assertion error:

		assert_eq!(msgs::Init { features: InitFeatures::empty(),
			net_address: Some(msgs::NetAddress::IPv4 {
				addr: [127, 0, 0, 1],
				port: 1000,
			}),
		}.encode(), hex::decode("0307017f000103e8").unwrap());

Here is the assertion error

thread 'ln::msgs::tests::encoding_init' panicked at 'assertion failed: `(left == right)`
  left: `[3, 7, 1, 127, 0, 0, 1, 3, 232, 0, 0, 0, 0]`,
 right: `[3, 7, 1, 127, 0, 1, 3, 232]`'

How should I fix this?
@TheBlueMatt

TheBlueMatt · 2022-02-25T23:59:29Z

See the check_commits and benchmark CI passes - there are other parts of the code that still need updating.

codecov-commenter · 2022-02-26T00:05:05Z

Codecov Report

Merging #1326 (f64383f) into main (74b9c1a) will increase coverage by 0.02%.
The diff coverage is 96.62%.

❗ Current head f64383f differs from pull request most recent head fc2f793. Consider uploading reports for the commit fc2f793 to get more accurate results

@@            Coverage Diff             @@
##             main    #1326      +/-   ##
==========================================
+ Coverage   90.75%   90.78%   +0.02%     
==========================================
  Files          73       73              
  Lines       41062    41002      -60     
  Branches    41062    41002      -60     
==========================================
- Hits        37266    37223      -43     
+ Misses       3796     3779      -17

Impacted Files	Coverage Δ
lightning-net-tokio/src/lib.rs	`75.88% <66.66%> (-0.81%)`	⬇️
lightning-background-processor/src/lib.rs	`93.10% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.72% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.79% <100.00%> (-0.06%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.54% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.07% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/msgs.rs	`86.37% <100.00%> (+0.53%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.15% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/peer_handler.rs	`52.71% <100.00%> (+4.40%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`96.84% <100.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74b9c1a...fc2f793. Read the comment docs.

Psycho-Pirate · 2022-02-26T00:14:07Z

See the check_commits and benchmark CI passes - there are other parts of the code that still need updating.

How can I fix the check_commits test? It is failing because of some previous commit.

TheBlueMatt · 2022-02-26T01:31:58Z

You can rebase and squash intermediate commits to rewrite the history to make the code pass at each commit.

Psycho-Pirate · 2022-02-26T01:36:30Z

Please have a look at the fuzz test as well

Psycho-Pirate · 2022-02-26T18:43:54Z

This is the error I am getting

error: failed to run LLVM passes: unknown pass name 'sancov'

I am not sure how to fix this.

TheBlueMatt · 2022-02-27T00:03:18Z

The fuzz failure is an upstream regression, see #1335, you can ignore it here.

Psycho-Pirate · 2022-02-28T21:23:47Z

I have fixed the warnings, is there anything else that needs to be done?
@TheBlueMatt

valentinewallace

should we open an issue to support this in net-tokio?
do we care to expose our remote IP in the future, from the Init messages we receive?

I'm pretty fine to address all feedback in follow-up, since it's an external contributor PR and the code looks fine :)

Psycho-Pirate · 2022-03-04T19:18:54Z

Is there anything else that needs to be done here?
@TheBlueMatt
@valentinewallace
@jkczyz

jkczyz · 2022-03-04T19:35:46Z

You'll need to get your branch in order still as per: https://github.com/lightningdevkit/rust-lightning/pull/1326/files#r817983366

Psycho-Pirate · 2022-03-04T21:02:55Z

You'll need to get your branch in order still as per: https://github.com/lightningdevkit/rust-lightning/pull/1326/files#r817983366

I have rebased it now. Is this alright?

TheBlueMatt · 2022-03-04T22:17:44Z

You should rename the commits to describe the changes that are happening in each, instead of "squashed commits".

TheBlueMatt

One real comment and some style nits, thanks for sticking with it, this is looking great now!

TheBlueMatt · 2022-03-15T20:57:39Z

+fn filter_addresses(ip_address: Option<NetAddress>) -> Option<NetAddress> {
+	match ip_address{
+		// For IPv4 range 10.0.0.0 - 10.255.255.255 (10/8)
+		Some(NetAddress::IPv4{addr: [0xA, 0x00..=0xFF, _, _], port: _}) => None,


nit: this would be more readable if you replace all the 0x00..=0xff places with just _. Also, for IPv4, not using hex is more readable, since the traditional representation is decimal, my previous nit about hex was just for v6, where the traditional representation is hex.

Psycho-Pirate · 2022-03-16T00:16:57Z

One real comment and some style nits, thanks for sticking with it, this is looking great now!

Thank you so much! I couldn't have done this without your guidance!

TheBlueMatt · 2022-03-16T16:49:45Z

See #1326 (comment) please feel free to squash the last fixup commit down (I already checked with @jkczyz ).

TheBlueMatt · 2022-03-16T18:59:49Z

Looks like this is now randomly including one commit from upstream?

Psycho-Pirate · 2022-03-16T19:29:31Z

Looks like this is now randomly including one commit from upstream?

I'm not really sure how to remove that, i used git rebase -i HEAD~3 and then squashed the commits.

TheBlueMatt · 2022-03-16T19:34:26Z

Take a look at the Bitcoin Core contributors guide, it has a section on how to do git rebases.

Psycho-Pirate · 2022-03-21T18:12:09Z

Is there anything else that needs to be done?

TheBlueMatt

Oops sorry, github didn't tag the PR as unread when you pushed...A few minor nits, needs a second review.

TheBlueMatt · 2022-03-21T23:31:36Z

Can you squash down the two new commits into the first commit? Then LGTM.

Psycho-Pirate · 2022-03-21T23:33:15Z

Can you squash down the two new commits into the first commit? Then LGTM.

Sure

valentinewallace

Needs rebase. Some optional nits but I'm ACK!

@TheBlueMatt do we care to expose our remote IP in the future or to use this for our node_announcements, from the Init messages we receive?

TheBlueMatt · 2022-03-22T21:07:59Z

@TheBlueMatt do we care to expose our remote IP in the future or to use this for our node_announcements, from the Init messages we receive?

Yea, its something to consider using, totally, Its kinda nice cause we can completely automate it without our users having to care, I opened #1377 for the followup.

TheBlueMatt · 2022-03-22T21:08:22Z

Oops, this now needs rebase, sorry about that!

…and updated documentation

Psycho-Pirate · 2022-03-23T21:04:11Z

Thank you so much! This was a great learning experience for me! @TheBlueMatt

TheBlueMatt reviewed Feb 21, 2022

View reviewed changes

Comment thread lightning/src/ln/peer_handler.rs Outdated

Comment thread lightning/src/ln/peer_handler.rs Outdated

TheBlueMatt reviewed Feb 23, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

Comment thread lightning/src/ln/peer_handler.rs Outdated

Psycho-Pirate commented Feb 23, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

Psycho-Pirate commented Feb 23, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs

TheBlueMatt reviewed Feb 25, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

Comment thread lightning/src/ln/msgs.rs Outdated

TheBlueMatt reviewed Feb 27, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs

Psycho-Pirate commented Feb 27, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

valentinewallace reviewed Mar 1, 2022

View reviewed changes

Comment thread lightning/src/ln/peer_handler.rs

Psycho-Pirate commented Mar 1, 2022

View reviewed changes

Comment thread lightning/src/ln/peer_handler.rs

jkczyz reviewed Mar 2, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

Comment thread lightning/src/routing/router.rs

TheBlueMatt reviewed Mar 10, 2022

View reviewed changes

Comment thread lightning/src/ln/peer_handler.rs Outdated

Comment thread lightning/src/ln/peer_handler.rs Outdated

TheBlueMatt reviewed Mar 15, 2022

View reviewed changes

jkczyz reviewed Mar 16, 2022

View reviewed changes

Comment thread lightning-net-tokio/src/lib.rs Outdated

Comment thread lightning-net-tokio/src/lib.rs

Comment thread lightning-net-tokio/src/lib.rs

TheBlueMatt reviewed Mar 16, 2022

View reviewed changes

Comment thread lightning-invoice/src/utils.rs Outdated

TheBlueMatt previously approved these changes Mar 21, 2022

View reviewed changes

Comment thread lightning/src/ln/peer_handler.rs Outdated

Comment thread lightning/src/ln/peer_handler.rs

Comment thread lightning/src/ln/peer_handler.rs

Psycho-Pirate dismissed TheBlueMatt’s stale review via 2f84b87 March 21, 2022 23:23

TheBlueMatt added the Seeking Code Review label Mar 22, 2022

valentinewallace self-requested a review March 22, 2022 18:29

valentinewallace previously approved these changes Mar 22, 2022

View reviewed changes

Comment thread lightning/src/ln/msgs.rs Outdated

Comment thread lightning/src/ln/msgs.rs Outdated

Comment thread lightning/src/ln/peer_handler.rs Outdated

Comment thread lightning/src/ln/msgs.rs

TheBlueMatt mentioned this pull request Mar 22, 2022

Automate Inclusion of (potential) public IPs in announcements #1377

Open

valentinewallace reviewed Mar 22, 2022

View reviewed changes

Comment thread lightning-net-tokio/src/lib.rs Outdated

Psycho-Pirate added 2 commits March 23, 2022 04:44

added network address in methods, filter_address function with tests …

20a81e5

…and updated documentation

Argument added in lightning-net-tokio/src/lib.rs and comments updated

fc2f793

Psycho-Pirate dismissed valentinewallace’s stale review via fc2f793 March 22, 2022 23:14

TheBlueMatt approved these changes Mar 23, 2022

View reviewed changes

TheBlueMatt assigned valentinewallace Mar 23, 2022

TheBlueMatt removed the Seeking Code Review label Mar 23, 2022

valentinewallace approved these changes Mar 23, 2022

View reviewed changes

TheBlueMatt merged commit b010aeb into lightningdevkit:main Mar 23, 2022

Conversation

Psycho-Pirate commented Feb 21, 2022

Uh oh!

Psycho-Pirate commented Feb 21, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Psycho-Pirate commented Feb 22, 2022

Uh oh!

TheBlueMatt commented Feb 22, 2022

Uh oh!

Psycho-Pirate commented Feb 22, 2022

Uh oh!

Psycho-Pirate commented Feb 22, 2022

Uh oh!

TheBlueMatt commented Feb 22, 2022

Uh oh!

Psycho-Pirate commented Feb 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Psycho-Pirate commented Feb 25, 2022

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Feb 25, 2022

Uh oh!

codecov-commenter commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Psycho-Pirate commented Feb 26, 2022

Uh oh!

TheBlueMatt commented Feb 26, 2022

Uh oh!

Psycho-Pirate commented Feb 26, 2022

Uh oh!

Psycho-Pirate commented Feb 26, 2022

Uh oh!

TheBlueMatt commented Feb 27, 2022

Uh oh!

Uh oh!

Uh oh!

Psycho-Pirate commented Feb 28, 2022

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Psycho-Pirate commented Mar 4, 2022

Uh oh!

jkczyz commented Mar 4, 2022

Uh oh!

Psycho-Pirate commented Mar 4, 2022

Uh oh!

TheBlueMatt commented Mar 4, 2022

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Psycho-Pirate commented Mar 16, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Feb 26, 2022 •

edited

Loading