Check CJDNS address is valid #20616

lontivero commented at 5:33 PM on December 10, 2020: contributor

CJDNS addresses start with 0xFC and for that reason if a netaddr was unserialized with network type cjdns but its address prefix is not 0xFC then that netaddr should be considered invalid.

lontivero force-pushed on Dec 10, 2020

DrahtBot added the label P2P on Dec 10, 2020

laanwj commented at 12:00 PM on December 11, 2020: member

Concept ACK, CJDNS addresses do and will always start with 0xFC (we could even have used this to save a byte in the wire representation) this is a simple sanity check.

practicalswift commented at 3:24 PM on December 11, 2020: contributor

Concept ACK

Nice contribution. Thanks for adding a test too!

in src/netaddress.cpp:444 in 96b98ad92c outdated

 436 | @@ -437,6 +437,9 @@ bool CNetAddr::IsValid() const
 437 |          return false;
 438 |      }
 439 |  
 440 | +    if (IsCJDNS() && (m_addr[0] != 0xFC))
 441 | +        return false;
 442 | +

jonatack commented at 3:34 PM on December 11, 2020:

nit, same line or use braces for new code, per doc/developer-notes.md

If an `if` only has a single-statement `then`-clause, it can appear
on the same line as the `if`, without braces. In every other case,
braces are required, and the `then` and `else` clauses must appear
correctly indented on a new line.

lontivero commented at 7:03 PM on December 11, 2020:

I wanted to follow the same style that the rest of the if statements had in the method. Should I change it anyway?

sipa commented at 7:04 PM on December 11, 2020:

Yes, absolutely. The developer notes explicitly state you should not try to mimick surrounding style when writing new code.

lontivero commented at 3:27 PM on December 14, 2020:

Done

jonatack commented at 3:35 PM on December 11, 2020: member

Concept ACK

lontivero force-pushed on Dec 11, 2020

lontivero force-pushed on Dec 14, 2020

in src/netaddress.cpp:440 in fbd400db10 outdated

 436 | @@ -437,6 +437,11 @@ bool CNetAddr::IsValid() const
 437 |          return false;
 438 |      }
 439 |  
 440 | +    // cjdns addresses always start with 0xfc

jonatack commented at 4:31 PM on December 14, 2020:

suggestion, if you retouch:

    // CJDNS addresses always start with 0xfc

as everywhere else in the file and in net_tests.cpp refers to CJDNS

in src/test/net_tests.cpp:614 in fbd400db10 outdated

 608 | +    s << MakeSpan(ParseHex("06"                               // network type (CJDNS)
 609 | +                           "10"                               // address length
 610 | +                           "aa000001000200030004000500060007" // address
 611 | +                           ));
 612 | +    s >> addr;
 613 | +    BOOST_CHECK(!addr.IsValid());

jonatack commented at 4:38 PM on December 14, 2020:

perhaps assert IsCJDNS() before the invalidity check

     s >> addr;
+    BOOST_CHECK(addr.IsCJDNS());
     BOOST_CHECK(!addr.IsValid());

jonatack commented at 4:39 PM on December 14, 2020: member

ACK fbd400db1084d983ea676725fa912336e8612a87

A couple of minor suggestions below, happy to re-ACK if you update

Check if Cjdns address is valid f7264fff0a

lontivero force-pushed on Dec 14, 2020

jonatack commented at 7:04 PM on December 14, 2020: member

ACK f7264fff0a098f8b6354c7373b8790791c25dd07

practicalswift commented at 7:41 PM on December 14, 2020: contributor

cr ACK f7264fff0a098f8b6354c7373b8790791c25dd07: patch looks correct

theStack approved

theStack commented at 9:31 AM on December 15, 2020: member

ACK f7264fff0a098f8b6354c7373b8790791c25dd07 ✔️ Verified that Cjdns addresses always start with 0xfc by reading https://gathman.org/meshdoc/Cjdns%20-%20Wikipedia.html#Address_generation and https://www.systutorials.com/docs/linux/man/5-cjdroute.conf/

laanwj commented at 4:50 PM on December 15, 2020: member

@theStack correct, and fwiw the requirement also mentioned in the BIP https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki#appendix-d-cjdns-address-encoding

laanwj merged this on Dec 15, 2020

laanwj closed this on Dec 15, 2020

sipa commented at 5:43 PM on December 15, 2020: member

Posthumous utACK f7264fff0a098f8b6354c7373b8790791c25dd07

I think this check makes sense because it is so simple and easily observable, but we probably don't want to go much further with similar sanity checks (e.g. we could go check if relayed torv3 contain an actual ed25519 pubkey, which IMO would lead us too far).

sidhujag referenced this in commit 28852243b6 on Dec 15, 2020

PastaPastaPasta referenced this in commit 4561d8af49 on Jun 27, 2021

PastaPastaPasta referenced this in commit 80d712c5ad on Jun 28, 2021

PastaPastaPasta referenced this in commit 366c69839d on Jun 29, 2021

PastaPastaPasta referenced this in commit ba0e274bc6 on Jul 1, 2021

PastaPastaPasta referenced this in commit cc0dc3d7e2 on Jul 1, 2021

PastaPastaPasta referenced this in commit 43b948da43 on Jul 15, 2021

PastaPastaPasta referenced this in commit 5ff3dd1b34 on Jul 16, 2021

DrahtBot locked this on Feb 15, 2022