implement fromstr trait to netaddress #2134

jbesraa · 2023-03-28T11:24:20Z

resolves #2056

lightning/src/ln/msgs.rs

jbesraa · 2023-03-29T16:45:02Z

@TheBlueMatt @tnull any recommendation on how to decode the onionv3 address? I could potentially use https://crates.io/crates/base32 but im not sure about adding a new package just for that?

TheBlueMatt · 2023-03-30T00:38:54Z

Hmm, yea, that's a good question - I took a brief glance at the base32 code, it looks fine, though could use one or two trivial changes (and a fuzzer, it has an obvious panic in the decode!). Luckily its the same license so we'd have to add a notice that it includes code which is "Copyright (c) 2015 The base32 Developers" but that's nbd. We should pull it in by coping their lib.rs, though, not taking the dependency, there's no need for a dep here.

jbesraa · 2023-04-04T17:03:58Z

made some changes(main functionality is implemented and tests are passing) to the code and updated the top comment accordingly

lightning/src/ln/msgs.rs

lightning/Cargo.toml

lightning/src/ln/msgs.rs

lightning/src/util/base32.rs

lightning/src/ln/msgs.rs

lightning/src/util/base32.rs

jbesraa · 2023-04-24T13:12:41Z

i can see the fuzzer failing locally as well, but not sure what to make from the error message

lightning/src/ln/msgs.rs

TheBlueMatt · 2023-05-01T18:22:30Z

lightning/src/util/base32.rs

+use crate::prelude::*;
+
+/// Alphabet used for encoding and decoding.
+#[derive(Copy, Clone)]


(a) we can drop the crockford encoding, we don't use it. (b) we should move the zbase32 stuff we already have to here - its the same code anyway, but just a different alphabet.

lightning/src/ln/msgs.rs

TheBlueMatt · 2023-05-11T19:35:18Z

Let me know when you're ready for another look here, looks like you're in the midst of merging the two base32s.

codecov-commenter · 2023-05-12T12:10:50Z

Codecov Report

Patch coverage: 93.17% and project coverage change: +0.01% 🎉

Comparison is base (e9d9711) 90.58% compared to head (529a09f) 90.60%.

❗ Current head 529a09f differs from pull request most recent head 9a8b7e3. Consider uploading reports for the commit 9a8b7e3 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
+ Coverage   90.58%   90.60%   +0.01%     
==========================================
  Files         110      110              
  Lines       57526    57555      +29     
  Branches    57526    57555      +29     
==========================================
+ Hits        52112    52149      +37     
+ Misses       5414     5406       -8

Files Changed	Coverage Δ
lightning/src/ln/msgs.rs	`86.20% <89.77%> (+0.38%)`	⬆️
lightning/src/util/message_signing.rs	`92.30% <90.47%> (ø)`
lightning/src/util/base32.rs	`96.63% <96.63%> (ø)`

... and 19 files with indirect coverage changes

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

jbesraa · 2023-05-15T12:17:21Z

@TheBlueMatt can u please give this another round of review? also, any tips on how to debug the fuzzer locally would be much appreciated

lightning/src/util/base32.rs

lightning/src/ln/msgs.rs

TheBlueMatt · 2023-05-21T01:45:10Z

Re: fuzzer reproduction, the fuzz/README.md file has a section on ## A fuzz test failed on Travis, what do I do?

jbesraa · 2023-06-28T22:06:52Z

apologies for the slow execution, life got in the middle (:
did some improvements to from_str, will handle the base32 code and the fuzzer in the next days hopefully!

...and ill cleanup commits

TheBlueMatt

Okay, really just one nit left!

lightning/src/util/base32.rs

lightning/src/ln/msgs.rs

TheBlueMatt

A few minor nits, but nothing blocking.

TheBlueMatt · 2023-08-27T16:41:09Z

lightning/src/util/base32.rs

+	];
+
+	const RFC4648_TEST_VECTORS: &[(&[u8], &[u8])] = &[
+		(b"", b""),


data type for the second element in the tuple should be a str, which you're currently doing in the tests with std::str::from_utf8...unwrap.

TheBlueMatt · 2023-08-27T16:44:02Z

lightning/src/util/base32.rs

+
+		for (_, encoded) in RFC4648_TEST_VECTORS {
+			let decoded = &Alphabet::RFC4648 { padding: true }.decode(std::str::from_utf8(encoded).unwrap()).unwrap();
+			assert_eq!(&Alphabet::RFC4648 { padding: true }.encode(decoded).as_bytes(), encoded);


This test is redundant - we already test that if we decode we get the expected value and if we encode we get the expected value, there's no need for an additional round-trip test.

TheBlueMatt · 2023-08-27T16:50:36Z

lightning/src/ln/msgs.rs

@@ -3296,10 +3403,10 @@ mod tests {
 		let shutdown = msgs::Shutdown {
 			channel_id: [2; 32],
 			scriptpubkey:
-				     if script_type == 1 { Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey() }
+				if script_type == 1 { Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey() }


Why did you remove the alignment here and below?

hmm this was a space, now it looks like this:

Indeed, it was a space to align the opening { nicely no matter the users' configured tabstop.

TheBlueMatt · 2023-08-27T16:53:01Z

lightning/src/ln/msgs.rs

+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		match self {
+			NetAddressParseError::SocketAddrParse => write!(f, "Socket address (IPv4/IPv6) parsing error"),
+			NetAddressParseError::InvalidInput => write!(f, "{}", "Invalid input format. \


You can drop the , "{}" part.

TheBlueMatt · 2023-08-27T16:57:12Z

Oh, if you push again, please fix the commit title on the last commit - commit description text must include a blank line between the title and the first non-title line (otherwise git treats it as a really long title, which breaks lots of things).

TheBlueMatt · 2023-08-27T16:58:26Z

Also, the description in the first commit doesn't tell me anything " restructure encode function restructure decode function" doesn't tell me what the commit is doing or why its doing that, though the title itself is pretty clear. Commit descriptions shouldn't include the history of that commit, by the time we merge it we don't care too much about the history of the PR's review.

tnull

Mostly looks good from my side, but have yet to go over the base32 logic stuff one last time.

lightning/src/util/base32.rs

lightning/src/util/message_signing.rs

lightning/src/ln/msgs.rs

tnull · 2023-08-29T11:34:25Z

Also needs a minor rebase now it seems.

TheBlueMatt · 2023-09-01T19:56:09Z

Any update here, would really love to land this for 0.0.117 but its gonna get over the line in the next week or two.

jbesraa · 2023-09-04T15:30:31Z

@tnull split the indentation to a separate commit and added short docs for the numbers

tnull

Generally looks good, mod some nits/minor comments.

tnull · 2023-09-05T11:53:31Z

lightning/src/util/mod.rs

@@ -58,3 +58,4 @@ pub mod test_utils;
 #[cfg(any(test, feature = "_test_utils"))]
 pub mod test_channel_signer;

+pub mod enforcing_trait_impls;


This file was removed on main and we shouldn't re-add the module here.

tnull · 2023-09-05T12:07:28Z

lightning/src/util/message_signing.rs

@@ -143,4 +143,4 @@ mod test {
            assert!(verify(c[1].as_bytes(), c[2], &PublicKey::from_str(c[3]).unwrap()))
        }
    }
-}
+}


Don't remove the trailing newline.

tnull · 2023-09-05T12:12:50Z

lightning/src/util/base32.rs

+impl Alphabet {
+	/// Encode bytes into a base32 string.
+	pub fn encode(&self, data: &[u8]) -> String {
+		// This line calculates the length of the output string, which is equal to the original data length * 8 / 5, rounded up. The + 4 at the end is to account for the extra padding characters that may be added.


nit: might want to wrap the comment lines also.

tnull · 2023-09-05T12:20:53Z

lightning/src/util/base32.rs

+				if *padding {
+					let len = ret.len();
+					let num_extra = len - output_length;
+					for i in 1..num_extra + 1 {


Isn't this just the same as the following, which is a lot less confusing?:

for i in output_length..len { ret[i] = b'='; }

tnull · 2023-09-05T12:29:00Z

lightning/src/util/base32.rs

+		// If the string has more characters than are required to alphabet_encode the number of bytes
+		// decodable, treat the string as invalid.
+		match data.len() % 8 { 1|3|6 => return Err(()), _ => {} }
+		let mut ret = Self::decode_data(data, alphabet)?;


nit: Doesn't need to be mut.

tnull · 2023-09-05T12:30:54Z

lightning/src/util/base32.rs

+		let cap = (data.len() + 4) / 5 * 8;
+		let mut ret = Vec::with_capacity(cap);
+		for chunk in data.chunks(5) {
+			let buf = {


I don't think we need this additional let binding? Why not just use the mut buf below?

tnull · 2023-09-05T12:32:36Z

lightning/src/util/base32.rs

+		let cap = (data.len() + 7) / 8 * 5;
+		let mut ret = Vec::with_capacity(cap);
+		for chunk in data.chunks(8) {
+			let buf = {


Same here, why do we need this additional let binding?

jbesraa · 2023-09-05T13:25:30Z

Fixed Up:

Wrapped comments into multiple lines
Removed unnecessary mut and let bindings
Improved readability of if
Reverted EOL removal

TheBlueMatt · 2023-09-05T22:56:46Z

lightning/src/util/base32.rs

+	pub fn encode(&self, data: &[u8]) -> String {
+		// This line calculates the length of the output string, 
+		// which is equal to the original data length * 8 / 5, rounded up.
+		// The + 4 at the end is to account for the extra padding characters that may be added.


No, the +4 is the "rounded up" part - rounding up with integer division is generally done by doing (numerator + divisor - 1) / divisor

wdyt about this?

1 pub fn encode(&self, data: &[u8]) -> String { | 45 // output_length is calculated as follows: | 1 // / 5 divides the data length by the number of bits per chunk (5), | 2 // * 8 multiplies the result by the number of characters per chunk (8). | 3 // + 4 rounds up to the nearest character. 4 let output_length = (data.len() * 8 + 4) / 5; ```

TheBlueMatt · 2023-09-05T22:57:42Z

lightning/src/util/base32.rs

+
+	/// Encode a byte slice into a base32 string.
+	fn encode_data(data: &[u8], alphabet: &'static [u8]) -> Vec<u8> {
+		// The + 4 at the end is to account for the padding characters that may be needed,


No, the +5 is to round up.

wdyt about this?

102 fn encode_data(data: &[u8], alphabet: &'static [u8]) -> Vec<u8> { | 1 // cap is calculated as follows: | 2 // / 5 divides the data length by the number of bits per chunk (5), | 3 // * 8 multiplies the result by the number of characters per chunk (8). | 4 // + 4 rounds up to the nearest character.

TheBlueMatt · 2023-09-05T22:57:48Z

lightning/src/util/base32.rs

+
+
+	fn decode_data(data: &[u8], alphabet: [i8; 43]) -> Result<Vec<u8>, ()> {
+		// The + 7 at the end is to account for the padding characters that may be needed, 


No, the +7 is to round up.

wdyt about this?

129 fn decode_data(data: &[u8], alphabet: [i8; 43]) -> Result<Vec<u8>, ()> { | 1 // cap is calculated as follows: | 2 // / 8 divides the data length by the number of characters per chunk (8), | 3 // * 5 multiplies the result by the number of bits per chunk (5), | 4 // + 7 rounds up to the nearest byte.

TheBlueMatt · 2023-09-05T22:58:19Z

lightning/src/util/base32.rs

+
+	fn decode_data(data: &[u8], alphabet: [i8; 43]) -> Result<Vec<u8>, ()> {
+		// The + 7 at the end is to account for the padding characters that may be needed, 
+		// The / 8 divides the Base64 string length by the number of characters per chunk (8), 


nit: here, and a few places, you have excess whitespace at the end of your line. A local git show should highlight these depending on your terminal settings.

TheBlueMatt · 2023-09-05T23:00:04Z

lightning/src/ln/msgs.rs

+			match {
+				addr
+			} {


Suggested change

match {

addr

} {

match addr {

- Add fuzz test for `NetAddress` `from_str` function

tnull self-requested a review March 28, 2023 11:24

TheBlueMatt reviewed Mar 28, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 4, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

tnull reviewed Apr 5, 2023

View reviewed changes

lightning/Cargo.toml Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 7, 2023

View reviewed changes

jbesraa force-pushed the add_fromstr_to_netaddress branch 4 times, most recently from 362a9b4 to d807a53 Compare April 8, 2023 17:02

TheBlueMatt reviewed Apr 12, 2023

View reviewed changes

lightning/src/util/base32.rs Outdated Show resolved Hide resolved

TheBlueMatt mentioned this pull request Apr 13, 2023

impl fmt::Display for NetAddress #2183

Closed

jbesraa force-pushed the add_fromstr_to_netaddress branch from 25250ca to 3e46d4c Compare April 23, 2023 16:21

TheBlueMatt reviewed Apr 24, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the add_fromstr_to_netaddress branch from b6f5ae6 to 471c5d7 Compare May 1, 2023 09:55

TheBlueMatt reviewed May 1, 2023

View reviewed changes

jbesraa force-pushed the add_fromstr_to_netaddress branch 2 times, most recently from a092195 to cf72e89 Compare May 11, 2023 08:16

jbesraa force-pushed the add_fromstr_to_netaddress branch from cf72e89 to ea1b296 Compare May 12, 2023 12:09

TheBlueMatt reviewed May 21, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the add_fromstr_to_netaddress branch from 359e099 to d74c87f Compare June 28, 2023 22:04

TheBlueMatt reviewed Aug 26, 2023

View reviewed changes

lightning/src/util/base32.rs Outdated Show resolved Hide resolved

lightning/src/util/base32.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Show resolved Hide resolved

jbesraa force-pushed the add_fromstr_to_netaddress branch from 56b168a to dbded6e Compare August 26, 2023 11:44

TheBlueMatt previously approved these changes Aug 27, 2023

View reviewed changes

jbesraa dismissed TheBlueMatt’s stale review via 529a09f August 27, 2023 18:36

jbesraa force-pushed the add_fromstr_to_netaddress branch from dbded6e to 529a09f Compare August 27, 2023 18:36

tnull mentioned this pull request Aug 28, 2023

impl ToSocketAddrs for Hostname (and possibly other NetAddress variants) #2535

Closed

tnull reviewed Aug 28, 2023

View reviewed changes

lightning/src/util/base32.rs Show resolved Hide resolved

lightning/src/util/message_signing.rs Show resolved Hide resolved

lightning/src/ln/msgs.rs Show resolved Hide resolved

jbesraa force-pushed the add_fromstr_to_netaddress branch 4 times, most recently from c9750ce to 03e4ac8 Compare September 4, 2023 15:29

tnull reviewed Sep 5, 2023

View reviewed changes

jbesraa force-pushed the add_fromstr_to_netaddress branch from 03e4ac8 to 53db4ca Compare September 5, 2023 13:23

jbesraa force-pushed the add_fromstr_to_netaddress branch from 53db4ca to 5f354fa Compare September 5, 2023 14:16

TheBlueMatt reviewed Sep 5, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Sep 5, 2023

Rename SocketAddress from NetAddress #2549

Merged

jbesraa added 4 commits September 6, 2023 15:07

Add RFC4648 base32 encode and decode functions

d736ca8

Move zbase32 implementation to base32 file

b2d3b94

Fix indent in message_signing.rs

32a0113

Implement from_str trait for NetAddress

4f45cdc

- Add fuzz test for `NetAddress` `from_str` function

jbesraa force-pushed the add_fromstr_to_netaddress branch from 5f354fa to 4f45cdc Compare September 6, 2023 16:28

TheBlueMatt approved these changes Sep 6, 2023

View reviewed changes

tnull approved these changes Sep 7, 2023

View reviewed changes

tnull merged commit b5e9594 into lightningdevkit:main Sep 7, 2023



		fn decode_data(data: &[u8], alphabet: [i8; 43]) -> Result<Vec<u8>, ()> {
		// The + 7 at the end is to account for the padding characters that may be needed,

-			match {
-				addr
-			} {
+			match addr {

implement fromstr trait to netaddress #2134

implement fromstr trait to netaddress #2134

Conversation

jbesraa commented Mar 28, 2023 • edited Loading

jbesraa commented Mar 29, 2023

TheBlueMatt commented Mar 30, 2023

jbesraa commented Apr 4, 2023

jbesraa commented Apr 24, 2023

Choose a reason for hiding this comment

TheBlueMatt commented May 11, 2023 • edited Loading

codecov-commenter commented May 12, 2023 • edited Loading

Codecov Report

jbesraa commented May 15, 2023

TheBlueMatt commented May 21, 2023

jbesraa commented Jun 28, 2023 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 27, 2023

TheBlueMatt commented Aug 27, 2023

tnull left a comment

Choose a reason for hiding this comment

tnull commented Aug 29, 2023 • edited Loading

TheBlueMatt commented Sep 1, 2023

jbesraa commented Sep 4, 2023

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 28, 2023 •

edited

Loading

TheBlueMatt commented May 11, 2023 •

edited

Loading

codecov-commenter commented May 12, 2023 •

edited

Loading

jbesraa commented Jun 28, 2023 •

edited

Loading

tnull commented Aug 29, 2023 •

edited

Loading

jbesraa commented Sep 5, 2023 •

edited

Loading