[WIP] Add support for addrv2 (BIP155) #16748

dongcarl commented at 9:32 PM on August 28, 2019: member

Original BIP: https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki Followup discussions after the BIP was merged: https://github.com/bitcoin/bips/pull/766

This rough WIP PR aims to allow propagating addresses with addrv2 messages, and signaling support for addrv2 messages with sendaddrv2 message.

The commits are structured such that every single commit should compile and pass tests.

TODO:

Wayyyyyyy more tests.

Things I'd like feedback on:

What would be the best way to split up https://github.com/bitcoin/bitcoin/commit/e6009922077ce330bc76b82b9c3aa1e813e40356? I've tried to split it up in multiple ways but every time I attempt to do it, it just increases the total diff.
In https://github.com/bitcoin/bitcoin/commit/fb78590939ff2d8c008cfd68f2f7df816389ac01, we introduce CJDNS as a new supported network. CJDNS addresses are simple IPv6 addresses under a certain prefix,
In https://github.com/bitcoin/bitcoin/commit/e10f8bfcaec44cf1e7b73c390225b80644e8367f, I'm wondering if there are better ways of structuring the code for ignoring single addr entries, and the entire message.

Things that surprised me:

addrMe was deprecated and no one seemed to remember: https://github.com/bitcoin/bips/pull/766#issuecomment-524444473

refactor: Tidy up CNetAddr construction fcc2a07f50

refactor: Serialize CService as CNetAddr 0bbe885f6c

Refactor NetAddr in an API compatible way in preparation for addrv2 e600992207

Add support for v2-only Networks fb78590939

Introduce addrv2 serialization e10f8bfcae

feat: Adapt addrman to addrv2, write tests 02dfac7799

feat: net_processing code for addrv2 c0414a6a17

feat: Make functional tests not freak out on sendaddrv2 e54fb70b3d

fanquake added the label P2P on Aug 28, 2019

DrahtBot commented at 10:30 PM on August 28, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17812 (config, test: asmap functional tests and feature refinements by jonatack)
#16702 (p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 9:31 AM on August 29, 2019: member

Concept ACK, thanks for working on this

naumenkogs commented at 4:39 PM on September 3, 2019: member

Concept ACK

I reviewed std::vector<unsigned char> CNetAddr::GetGroup() const part and it seems correct to me, although not sure mixing GetByte() and direct reading from the ip array is a good idea.

practicalswift commented at 3:13 PM on September 29, 2019: contributor

Concept ACK

Regarding the tests: could add a test for IsStandardV2Deserialization which seems both unused and untested currently? :)

laanwj added the label Feature on Oct 2, 2019

in src/netaddress.cpp:753 in e600992207 outdated

 749 | +    valid = (addr.m_network_id == NetworkID::IPV4 || addr.m_network_id == NetworkID::IPV6);
 750 |      network = addr;
 751 |      // Default to /32 (IPv4) or /128 (IPv6), i.e. match single address
 752 |      memset(netmask, 255, sizeof(netmask));
 753 |  
 754 |      // IPv4 addresses start at offset 12, and first 12 bytes must match, so just offset n

vasild commented at 5:23 PM on April 17, 2020:

nit: this comment is outdated now, needs to be adjusted (or deleted).

in src/netaddress.cpp:778 in e600992207 outdated

 781 | +    valid = (addr.m_network_id == NetworkID::IPV4 || addr.m_network_id == NetworkID::IPV6);
 782 |      network = addr;
 783 |      // Default to /32 (IPv4) or /128 (IPv6), i.e. match single address
 784 |      memset(netmask, 255, sizeof(netmask));
 785 |  
 786 |      // IPv4 addresses start at offset 12, and first 12 bytes must match, so just offset n

vasild commented at 5:25 PM on April 17, 2020:

nit: this comment is outdated now, needs to be adjusted (or deleted).

in src/netaddress.cpp:84 in fb78590939 outdated

  80 | @@ -81,6 +81,17 @@ bool CNetAddr::SetSpecial(const std::string &strName)
  81 |              m_network_id = NetworkID::TORV2;
  82 |              ip.assign(vchAddr.begin(), vchAddr.end());
  83 |              return true;
  84 | +        } else if (vchAddr.size() == 35) {

vasild commented at 7:20 PM on April 17, 2020:

nit: this function's comment is now outdated

in src/addrman.h:489 in 02dfac7799 outdated

 485 | @@ -477,7 +486,8 @@ class CAddrMan
 486 |          mapAddr.clear();
 487 |      }
 488 |  
 489 | -    CAddrMan()
 490 | +    CAddrMan(unsigned char serialization_version = ADDRMAN_HIGHEST_KNOWN_SERIALIZATION_VERSION)

vasild commented at 7:58 PM on April 17, 2020:

nit: deserves explicit

vasild commented at 2:49 PM on May 6, 2020:

No, it does not because the argument is unsigned char.

in src/test/net_tests.cpp:169 in 02dfac7799 outdated

 164 | +        unsigned char pchMsgTmp[4];
 165 | +        ssPeers1 >> pchMsgTmp;
 166 | +        ssPeers1 >> addrman1;
 167 | +    } catch (const std::exception&) {
 168 | +        exceptionThrown = true;
 169 | +    }

vasild commented at 8:09 PM on April 17, 2020:

nit: if we just let the exception uncaught, this will suffice to fail the test?

in src/net_processing.cpp:2143 in c0414a6a17 outdated

2140 | -        vRecv >> vAddr;
2141 | +        try {
2142 | +            vRecv >> vAddr;
2143 | +        } catch (...) {
2144 | +            LOCK(cs_main);
2145 | +            Misbehaving(pfrom->GetId(), 20, "absurdly large addrv2 entry");

vasild commented at 2:30 PM on April 21, 2020:

This message implies that the only reason vRecv >> vAddr could throw is a too large addrv2 entry. While this is the case with the current code, it is better to avoid such an assumption to make it less fragile wrt future changes (where we start throwing for more reasons).

Would it be possible to catch the exception object and print something like "can't parse address message: %s", e.what()?

in src/net_processing.cpp:2200 in c0414a6a17 outdated

2196 | @@ -2181,6 +2197,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2197 |          return true;
2198 |      }
2199 |  
2200 | +    if (strCommand == NetMsgType::SENDADDRv2) {

vasild commented at 2:54 PM on April 21, 2020:

This would accept the SENDADDRv2 message at any time during the connection lifetime. @laanwj commented:

I think for simplicy of implementation we'd want to mandate that this should be sent after version but before any other (non-BIP-signalling) messages, so a connection won't switch to v2 suddenly halfway other things.

It looks ok to me to switch to v2 in the middle of the connection lifetime, or do I miss something?

in src/net_processing.cpp:3616 in c0414a6a17 outdated

3611 | @@ -3590,14 +3612,24 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3612 |                      // receiver rejects addr messages larger than 1000
3613 |                      if (vAddr.size() >= 1000)
3614 |                      {
3615 | -                        connman->PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr));
3616 | +                        if (state.m_accepts_addrv2) {
3617 | +                            connman->PushMessage(pto, msgMaker.Make(SERIALIZE_ADDR_AS_V2, NetMsgType::ADDRv2, vAddr));

vasild commented at 3:22 PM on April 21, 2020:

The arguments seem redundant: NetMsgType::ADDRv2 already implies v2, so SERIALIZE_ADDR_AS_V2 shouldn't be necessary?

This also opens up the possibility for an unintentional misuses like Make(SERIALIZE_ADDR_AS_V2, NetMsgType::ADDR, vAddr) or Make(NetMsgType::ADDRv2, vAddr). Would it be more appropriate to have something like the following inside CNetMsgMaker::Make()?

if (sCommand == NetMsgType::ADDRv2) {
    nFlags |= SERIALIZE_ADDR_AS_V2;
}

and call it like Make(NetMsgType::ADDRv2, vAddr)?

in src/net_processing.cpp:2136 in c0414a6a17 outdated

2130 | @@ -2124,9 +2131,18 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2131 |          return false;
2132 |      }
2133 |  
2134 | -    if (strCommand == NetMsgType::ADDR) {
2135 | +    if (strCommand == NetMsgType::ADDR || strCommand == NetMsgType::ADDRv2) {
2136 | +        if (strCommand == NetMsgType::ADDRv2) {
2137 | +            vRecv.SetVersion(vRecv.GetVersion() | SERIALIZE_ADDR_AS_V2);

vasild commented at 4:32 PM on April 21, 2020:

Here SERIALIZE_ADDR_AS_V2 is being used as a deserialize instruction ("deserialize this addr message which is in v2 format") which may be confusing. What about using a separate constant like DESERIALIZE_ADDR_FROM_V2, or just one constant but with a generic name like ADDR_V2_FORMAT?

vasild commented at 7:18 PM on May 6, 2020:

This will flip vRecv to always deserialize addr messages as addrv2. If we subsequently receive addr(v1) then it will fail to parse it as addrv2. It would be unusual for a peer to send addrv2 followed by addrv1, but still that's not forbidden and we should handle it.

vasild commented at 7:12 PM on April 21, 2020: member

I reviewed the code, overall it looks good to me.

With this implementation it is not be possible for a node to choose to connect to peers that support addv2 (in the hopes of learning more torv3/i2p/cjdns addresses) because the support is signaled inside each p2p connection. BIP155 says a protocol version bump should be used for signaling, but I gather this topic is not settled yet.

I will try to write some tests for this PR.

Some questions and nits below, feel free to ignore.

vasild commented at 8:27 AM on April 22, 2020: member

What would be the best way to split up e600992? I've tried to split it up in multiple ways but every time I attempt to do it, it just increases the total diff.

It looks ok to me, no screaming need to split it.

In e10f8bf, I'm wondering if there are better ways of structuring the code for ignoring single addr entries, and the entire message.

You mean that CNetAddr::Unserialize() was changed to throw an exception and the parsing code is now in try/catch? I think the current approach is sound, given that 1) some addresses might already have been deserialized into the vector before we stumble on a problematic one, 2) the Unserialize() returns void and 3) we should stop parsing and reject the entire message, not just a particular address from it.

in src/addrman.h:214 in e54fb70b3d

 210 | @@ -209,6 +211,7 @@ class CAddrMan
 211 |      //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
 212 |      std::set<int> m_tried_collisions;
 213 |  
 214 | +    unsigned char m_serialization_version;

vasild commented at 12:24 PM on April 22, 2020:

This member variable is unused.

in src/netaddress.cpp:829 in e54fb70b3d

 838 |  {
 839 | -    memset(netmask, 255, sizeof(netmask));
 840 | +    valid &= (addr.m_network_id == NetworkID::IPV4 || addr.m_network_id == NetworkID::IPV6);
 841 | +
 842 | +    memset(netmask, 0, sizeof(netmask));
 843 | +    memset(netmask, 255, addr.ip.size());

vasild commented at 3:26 PM on April 24, 2020:

netmask is still defined as uint8_t netmask[16]; and if this is one of the new addresses that has addr.ip.size() equal to 32, then this memset() will overflow the buffer. Yes, valid will be false due to the above valid &= ..., but the overflow would still happen.

vasild commented at 9:05 AM on May 1, 2020: member

I rebased this and fixed some unit tests and a compilation warning at: https://github.com/vasild/bitcoin/tree/2019-07-addrv2v4 Next I will address the above suggestions.

dongcarl commented at 4:19 PM on May 1, 2020: member

@vasild Great! I'll close this one as soon as you open your PR :-)

in src/netaddress.h:136 in e54fb70b3d

 133 | +            if (s.GetVersion() & SERIALIZE_ADDR_AS_V2) {
 134 | +                s << static_cast<uint8_t>(m_network_id);
 135 | +                s << ip;
 136 | +            } else {
 137 | +                unsigned char ip_temp[16];
 138 | +                GetV1Serialization(ip_temp);

vasild commented at 7:15 PM on May 6, 2020:

We must make sure that we never end up here with this being one of the new address types, e.g. Tor v3. If that happens GetV1Serialization() will return false (which is currently ignored).

We should not attempt to send e.g. Tor v3 address to a peer that did not signal ADDRv2 support. Maybe CNode::PushAddress() is a good place to plant a check for that and skip adding new type addresses to the send queue of an old peer.

vasild commented at 7:36 PM on May 6, 2020: member

Just to note - I am addressing the above suggestions and will then open a PR, which I deem would be ready for wider review.

vasild commented at 4:24 PM on May 20, 2020: member

Just to note - I am addressing the above suggestions and will then open a PR, which I deem would be ready for wider review.

Done: https://github.com/bitcoin/bitcoin/pull/19031

dongcarl commented at 4:49 PM on May 20, 2020: member

Closing in favor of #19031, will review there.

dongcarl closed this on May 20, 2020

jonatack commented at 6:49 PM on September 14, 2020: member

Big, big, big kudos for your work on this @dongcarl.

DrahtBot locked this on Feb 15, 2022