net: save the network type explicitly in CNetAddr

[vasild]

(chopped off from #19031 to ease review)

Before this change, we would analyze the contents of CNetAddr::ip[16]
in order to tell which type is an address. Change this by introducing a
new member CNetAddr::m_net that explicitly tells the type of the
address.

This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).

As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0 we can use
m_net == NET_IPV4.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

[vasild]

Rebased to wake up Travis

[troygiorshev]

ACK eb22eed

If you make any changes, consider running clang-format. No worries if you don't, we can always do it later in the roadmap. Might be nice to clean up the formatting of some of the other methods here as well, now that we're touching so much of this.

[Saibato]

So if i read this correct, if this is merged we narrow before even having benefit and v3 , and say an address is not routable if one flag in CNetAddr m_net say's something else than m_net == NET_IPV6 and skip that the Bitcoin imperative 'data decides'

So you want to change even the old ADDRv1 logic while BIP155 say's don't. hmm...
The way bitcoin is used in the wild is often strange and has hidden paths, i can only urge you to not touch in any way the old ADDR logic and the way that net works.

And btw if ip is filled with << instead of memcpy and data don't decide, how sophisticated is the checksum plan for bit or byte errors in the new ADDRv1/v2 stream?

btw Since the BIP155 ADDv2 is to send ip2 and v3 just as 32 byte without checksum, i would also recommend to concat least sig byte of hash(m net | 32 byte ) before send, but that;s prob a PR for BIP155.
edit@saibato: Not that to provide piggy pack support for tor onions in ipv6 packets when ipv6 in #1021 was merged is a holy move, but time passed by and the value of bitcoin is not only the timestamp consensus, but AFAICS also the network itself.

[michaelfolkson]

Concept ACK, Approach ACK.

The BIP refers to I2P and CJDNS overlay networks and gives them their own network IDs. An I2P address is a Tor v3 address and a CJDNS address is a IPV6 address (or at least they are the same size) right? I'm just trying to understand why they warrant their own reserved network IDs in the BIP but don't seem to be implemented here?

[jonatack]

Light ACK eb22eed though viewed in isolation this change seems to be making the code more complex, not simpler.

[jonatack]

Is this code still needed in GetNetwork()?

    if (IsInternal())
        return NET_INTERNAL;

[vasild]

Good question - no. But it was not necessary before this PR either. Some good simplification can be done here even on master. I will see whether it is better to add a new commit to this PR or file it as a separate PR.

[vasild]

Correction - it is needed, otherwise this test would fail:

https://github.com/bitcoin/bitcoin/blob/476436b/src/test/netbase_tests.cpp#L45

Anyway, I made some simplifications on top of this PR that remove the "extending" of enum Network with NET_UNKNOWN and NET_TEREDO and also removed GetExtNetwork(). To avoid distractions, I will not append those commits to this PR but would rather open a new PR against master, once this PR gets merged.

[jonatack]

Thanks @vasild, looking forward to reviewing the simplifications.

[Saibato]

Suggested change:

/// A set of special crafted FC00/7 IPv6 addresses we use to map a string or FQDN to an IPv6 address.
/// We uses these important fake addresses also in CAddrMan to keep track of which DNS seeds were used 
/// and how we manage buckets and peers.

With that in mind when dev we can take special care in transit to ADDRv2 to not mess with this address realm,
because we need them later to get our new long addresses in and out from peers,dat ?

[vasild]

I extended the comment, thanks!

In #19031 we drop the 6-byte fd6b:88c0:8724 prefix from these addresses and store them in memory as m_net=NET_INTERNAL, m_addr=...10 bytes.... Serializing that to addrv1 remains unchanged (we add the fd6b:88c0:8724 prefix) and they are never serialized as addrv2.

[Saibato]

Not that related here and just to your info, the follow up construction on this run in an assert failure when i made the v3 patch,

The structures need also some failure tolerance and flexibility.
Don;t mind if i am harsh and sometimes sarcastic, but you do here heart surgery and better not look good but beat the drum

Also to get in an out of peers and DNS seeds is somewhat unrelated to the data structure on the ADDR net.
We could easy use intern more data and flags on our address than we use on the wire.
Those two structures need not to be the same.
edit@saiboto what let me tend to ADDRv2 is 2038. tyi.

[vasild]

The BIP refers to I2P and CJDNS overlay networks and gives them their own network IDs. An I2P address is a Tor v3 address and a CJDNS address is a IPV6 address (or at least they are the same size) right? I'm just trying to understand why they warrant their own reserved network IDs in the BIP but don't seem to be implemented here?

@michaelfolkson, the sizes of I2P and Torv3 addresses are equal (32 bytes) and also IPv6 and CJDNS addresses are both 16 bytes, but their interpretation is different - ie given some chunk of 32 bytes we need to know whether that is an I2P address or Torv3 address. This is why they have a separate network ids. Also there could be an overlap - e.g. some 16 bytes could both be valid IPv6 address and a valid CJDNS address.

The code in this PR and its parent PR (#19031) aims to implement ADDRv2 address format and signaling with the networks that are currently supported. Once that is done, on top of it, we will implement new networks like Torv3, I2P and others.

[vasild]

Extended the NET_INTERNAL comment as per @Saibato's suggestion.

[Saibato]

In general some of this might be optimized out by the compiler,
And i would prefer for the transition period something like this that we can refactor out when all works fine
to an compiler and bytecode friendly version.

I would prefer constructions for all those ipv6 overrides in the rest of the code to look like this
suggested change:

if  (memcmp(ip, pchLocal, 16) == 0) && IsIPv6();

And some logging here and in the other overrides i.e.

LogPrint(BCLog::ADDV2, "old isXXXX() says so, ADDRv2 m_net did override with  value = xxx .....   

[jonatack]

re-ACK 662bb25 per git diff eb22eed 662bb25 only change is improved documentation.

[troygiorshev]

reACK 662bb25

Only change is in a comment.

[Saibato]

tyi, I will not ACK or NACK any of ADDRv2 roadmap, Since Tor v2 will be deprecated sooner or later. I will try to start a side project to morph some old legacy tunnel code code to a special general 'bitcoin-peer-address' tor or whatever proxy wrapper, for all old nodes to have full v3 and after 2038 support, without need to softfork This will from day one, mitigate any chainsplit or censorship.

[vasild]

Just to clarify - ADDRv2/BIP155 does not involve a softfork, chainsplit or censorship. Old nodes (e.g. 0.19.1) can't have full support for TORv3 because they don't contain the code for it.

[Saibato]

I full support ADDRv2 and i will comment and provide review.
Also imo ADDRv2 its inevitable for master.

My approach aims to let old tor nodes not behind and be forced to softfork when tor v2 fades out.

can't have full support for TORv3 because they don't contain the code for it

I love this cant's in software dev, because i can,
There is a great misconception in how MITM or say jmp esp works when its clear that you don;t have the space,bytes or source code on the wire, that always astound me; why not use a bit hacking voodoo for some useful things,when we could tunnel because we have solid keys

That wrapper is on track and orthogonal to this here and when ADDRv2 is there, it will be even more easy to support this.

Maybe it will make its path to contrib to assist all old node software in support of protocols aside from internet protocol.

edit@saibato
Btw:It also astound me in many defi or blockchain software projects how they think the real internet or tor works, there are multiple ways to do the wrapper, and one is of such nature. that there is a minor chance that you could hear some jaw bones touch the floor.

tl;tr

The status quo, so far is that even now every tor v2 proxy capable node, can even now connect over tor v3 9050 socks5 proxy to any node that provides a v3 inbound hidden service over torrc. What missing is a bit virgin seeding that could be done by a signed script file and banman and conman bitcoin logic and then address gossip about that and to store this, for node reboot, if you don;t want to run every time a long bash cli addnode <v3.onion> add .script or a long cmdline. All can be solved without touching the old software. And since we have a great FC00/7 space for grabs in all old nodes, that could i.e. bind tun or taps to them to proxy out to even stranger protocols.
What is the misconception, is that we need to know the full 256 address or greater pubkey space of other protocols, when in fact to work for bitcoin we only will ever have a short directory or index list we need to gossip, that fits easy in the enormous 80bit FC00/7 index space, we have in every node even now.
edit@saibato;
When this and the followup PR's gain track and we will have ADDRv2 nodes in the wild, users can run anyway stripped down master versions, they connect to local or remote with there old node to advertise there v3 address, if they wish so, but honestly the nature of hidden net, is that its hidden, so to advertise inbound is anyway not so sexy. I guess, if we would have ̶a̶ ̶b̶y̶ ̶s̶a̶t̶o̶s̶h̶i̶ ̶k̶e̶y̶s̶ signed v3/ip2 whatever seeds list, that will be enough to bootstrap and there would be almost none old node, to not use such a list?

[vasild]

Coverage report of modified code: https://people.freebsd.org/~vd/pr19534_662bb25ef_coverage/
(all modified code is covered by tests)

[Saibato]

Just to clarify - ADDRv2/BIP155 does not involve a softfork, chainsplit or censorship. Old nodes (e.g. 0.19.1) can't have full support for TORv3 because they don't contain the code for it.

wow, that was really funny took,me a while to figure out the zero. is a morse placeholder. This kind of humor makes really fun and that i like so much when programing senator brains. I almost tend to blind ACK

[vasild]

Made 2 changes as per review suggestions:

• Initialize CNetAddr::m_net in its definition instead of in the constructor
• Move the code that detects non-IPv6 addresses embedded in IPv6 to a separate method

[troygiorshev]

reACK 64897c5 via git range-diff master 662bb25 64897c5

Just one comment

[troygiorshev]

reACK bcfebb6 via git range-diff master 64897c5 bcfebb6

[vasild]

Updated a comment to reflect that we now call SetLegacyIPv6() instead of SetRaw(), as per suggestion.

[jonatack]

re-ACK bcfebb6 per git diff 662bb25 bcfebb6, code review, debug build/tests clean, ran bitcoind.

[promag]

Concept ACK.

[laanwj]

Code review ACK bcfebb6

[maflcko]

Any reason to pass around fixed-length data with a raw data pointer (of arbitrary length) when type safe alternatives like span or std::array (probably not applicable in this case) are available?

[laanwj]

The input has to be 16 bytes, in this case. Is there a way to enforce this in the type, for spans?
(sure, the [16] doesn't do anything here either except as a signal to the programmer)

[maflcko]

Not sure, but I presumed spans could be of non-dynamic extent as well. Does this not work?

const std::span<uint8_t, 16>& ipv6

[vasild]

Yes, in this case the [16] is just a "signal to the programmer". In SetRaw() we have a bare pointer const uint8_t* when calling this function.

I have removed SetRaw() in the subsequent commit and will revisit this code.

Some experiments:

int f1(const std::span<int, 3>& p)
{
    return p[5]; // no warning
}

int f2(const int (&p)[3])
{
    return p[5]; // warning: array index 5 is past the end of the array (which contains 3 elements) [-Warray-bounds]
}

int f3(const std::array<int, 3>& p)
{
    return p[5]; // no warning
}

int main(int, char**) {
    int a[3];
    int b[2];

    f1(a);
    f1(b); // error: no known conversion from 'int [2]' to 'const std::span<int, 3>'

    f2(a);
    f2(b); // error: no known conversion from 'int [2]' to 'const int [3]'

    f3(std::array<int, 3>{a[0], a[1], a[2]});

    return 0;
}

[vasild]

So, in #19628 I changed the argument to const uint8_t (&ipv6)[ADDR_IPv6_SIZE] mostly to make the interface clean. From 3 callers 2 need an abusive typecasts because they don't have an array (one has bare pointer (the fuzz tests) and the other has struct in6_addr).

Feel free to comment there: https://github.com/bitcoin/bitcoin/pull/19628/files#diff-b64569708508232923e5fe3059396334R28

[vasild]

Thanks to everybody involved!

I opened the next PR at #19628. It is the biggest one from the ADDRv2 roadmap (#19031) and is mostly (but not entirely) mechanical.

After it follow more "interesting" commits :)