Implement ADDRv2 support (part of BIP155) #19031

vasild · 2020-05-20T16:16:33Z

An implementation of BIP155 addrv2 messages. To ease review it is split in a few logical changes and submitted as separate, smaller, PRs.

The current one for review is: #19954

Preparation changes

Commits:

test: add an edge case test for CSubNet (merged via test: add two edge case tests for CSubNet #19351)
net: improve encapsulation of CNetAddr (merged via net: improve encapsulation of CNetAddr #19360)
net: document enum Network (merged via net: save the network type explicitly in CNetAddr #19534)
net: save the network type explicitly in CNetAddr (merged via net: save the network type explicitly in CNetAddr #19534)

Change `CNetAddr::ip` to have flexible size

Commits:

net: change CNetAddr::ip to have flexible size (merged via net: change CNetAddr::ip to have flexible size #19628)

Implement `addrv2` (un)serializing

Add support to serialize CNetAddr and CAddress in ADDRv2 format. Invoke that from CAddrMan serialization methods. Commits:

net: move HasPrefix() so it can be reused (merged via net: CNetAddr: add support to (un)serialize as ADDRv2 #19845)
test: move HasReason() so it can be reused (merged via net: CNetAddr: add support to (un)serialize as ADDRv2 #19845)
net: CNetAddr: add support to (un)serialize as ADDRv2 (merged via net: CNetAddr: add support to (un)serialize as ADDRv2 #19845)
net: recognize TORv3/I2P/CJDNS networks (merged via net: CNetAddr: add support to (un)serialize as ADDRv2 #19845)
net: CAddress & CAddrMan: (un)serialize as ADDRv2 (merged via Complete the BIP155 implementation and upgrade to TORv3 #19954)

Advertise support

Advertise ADDRv2 support to peers, handle incoming ADDRv2 messages and send to peers in that format if they have advertised support. Commits:

net: advertise support for ADDRv2 via new message (merged via Complete the BIP155 implementation and upgrade to TORv3 #19954)

All the steps to get Tor v3 support are outlined in issue#18884 Tor v3 support, this PR is one of them.

src/protocol.h

dongcarl · 2020-05-20T16:55:57Z

Concept ACK 😄

Am I correct in saying that you've replaced NetworkID in my implementation with Bip155NetworkId? (totally fine btw, just wanted to make sure)

laanwj · 2020-05-20T17:03:29Z

Concept ACK!

vasild · 2020-05-20T18:17:27Z

Am I correct in saying that you've replaced NetworkID in my implementation with Bip155NetworkId? (totally fine btw, just wanted to make sure)

Yes, thanks for asking!

Ideally we would have just one enum for network types, not two. That would contain the network ids from BIP155 + the other ones we have now - unroutable, internal/name, etc. But the problem is that lots of code relies on the values being sequential - there are loops from 0 to NET_MAX. I did not wanted to plant e.g. "unroutable" as 0x07 just after CJDNS or to change those loops and assumptions because it would bloat this PR too much.

So I introduced a second enum but made it private inside CNetAddr and named it BIP155... to denote that it contains just the networks mentioned in BIP155 and nothing else.

DrahtBot · 2020-05-20T18:57:20Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Check that invalid peer traffic is accounted for #20028 (test: Check that invalid peer traffic is accounted for by MarcoFalke)
test: Add consistent formatting for messages.py repr methods #19889 (test: Add consistent formatting for messages.py repr methods by adaminsky)
net: don't try to relay to the address' originator #19763 (net: don't try to relay to the address' originator by vasild)
[tests] Allow outbound & block-relay-only connections in functional tests. #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
refactor: Make CAddrMan::cs non-recursive #19238 (refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto)
Use std::chrono for the time to rotate destination of addr messages + tests #18642 (Use std::chrono for the time to rotate destination of addr messages + tests 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.

naumenkogs · 2020-05-20T22:13:21Z

Concept ACK. WIll do a good review later.

…DDRv2 7be6ff6 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov) e0d7357 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov) fe42411 test: move HasReason so it can be reused (Vasil Dimov) d2bb681 util: move HasPrefix() so it can be reused (Vasil Dimov) Pull request description: (chopped off from bitcoin#19031 to ease review) Add an optional support to serialize/unserialize `CNetAddr` in ADDRv2 format (BIP155). The new serialization is engaged by ORing a flag into the stream version. So far this is only used in tests to ensure the new code works as expected. ACKs for top commit: Sjors: re-tACK 7be6ff6 sipa: re-utACK 7be6ff6 eriknylund: ACK 7be6ff6 I built the PR on macOS Catalina 10.15.6, ran both tests and functional tests. I've reviewed the code and think the changes look good and according to BIP155. I verified that the added Base32 encoding test looks as proposed and working. I've run a node for a week only with Onion addresses `-onlynet=onion` without issues and I can connect to other peer reviewers running TorV3 on their nodes and I can connect both of my test nodes to each other. jonatack: re-ACK 7be6ff6 per `git diff b9c46e0 7be6ff6`, debug build, ran/running bitcoind with this change and observed the log and `-netinfo` peer connections while connected as a tor v2 service to both tor v2 peers and also five tor v3 peers. hebasto: ACK 7be6ff6, tested on Linux Mint 20 (x86_64): on top of this pull and bitcoin#19031 I'm able to connect to onion v3 addresses, and jonatack is able to connect to my created onion v3 address. Tree-SHA512: dc621411ac4393993aa3ccad10991717ec5f9f2643cae46a24a89802df0a33d6042994fc8ff2f0f397a3dbcd1c0e58fe4724305a2f9eb64d9342c3bdf784d9be

hebasto · 2020-09-29T07:02:37Z

Mind rebasing?

DrahtBot · 2020-09-29T07:59:15Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Change the serialization of `CAddrMan` to serialize its addresses in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format version (3). Add support for ADDRv2 format in `CAddress` (un)serialization. Co-authored-by: Carl Dong <contact@carldong.me>

Introduce a new message `sendaddrv2` to signal support for ADDRv2. Send the new message immediately after sending the `VERACK` message. Add support for receiving and parsing ADDRv2 messages. Send ADDRv2 messages (instead of ADDR) to a peer if he has advertised support for it. Co-authored-by: Carl Dong <contact@carldong.me>

vasild · 2020-09-29T08:49:58Z

Rebased now that #19845 is merged.

vasild · 2020-09-29T08:53:28Z

Two more commits remain from this PR to be merged:

net: CAddress & CAddrMan: (un)serialize as ADDRv2
net: advertise support for ADDRv2 via new message

they are included in #19954.

Closing this as superseded by #19954.

dcf0cb4 tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov) 353a3fd net: advertise support for ADDRv2 via new message (Vasil Dimov) 201a459 net: CAddress & CAddrMan: (un)serialize as ADDRv2 (Vasil Dimov) 1d3ec2a Support bypassing range check in ReadCompactSize (Pieter Wuille) Pull request description: This PR contains the two remaining commits from #19031 to complete the [BIP155](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) implementation: `net: CAddress & CAddrMan: (un)serialize as ADDRv2` `net: advertise support for ADDRv2 via new message` plus one more commit: `tor: make a TORv3 hidden service instead of TORv2` ACKs for top commit: jonatack: re-ACK dcf0cb4 per `git diff 9b56a68 dcf0cb4` only change since last review is an update to the release notes which partially picked up the suggested text. Running a node on this branch and addnode-ing to 6 other Tor v3 nodes, I see "addrv2" and "sendaddrv2" messages in getpeerinfo in both the "bytesrecv_per_msg" and "bytessent_per_msg" JSON objects. sipa: ACK dcf0cb4 hebasto: re-ACK dcf0cb4, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed. laanwj: Edit: I have to retract this ACK for now, I'm having some problems with this PR on a FreeBSD node. It drops all outgoing connections with this dcf0cb4 merged on master (12a1c3a). ariard: Code Review ACK dcf0cb4 Tree-SHA512: 28d4d0d817b8664d2f4b18c0e0f31579b2f0f2d23310ed213f1f436a4242afea14dfbf99e07e15889bc5c5c71ad50056797e9307ff8a90e96704f588a6171308

bc74a40 net: improve encapsulation of CNetAddr (Vasil Dimov) Pull request description: Do not access `CNetAddr::ip` directly from `CService` methods. This improvement will help later when we change the type of `CNetAddr::ip` (in the BIP155 implementation). (chopped off from bitcoin#19031 to ease review) ACKs for top commit: dongcarl: ACK bc74a40 naumenkogs: ACK bc74a40 fjahr: Code review ACK bc74a40 laanwj: code review ACK bc74a40 jonatack: ACK bc74a40 jnewbery: ACK bc74a40 Tree-SHA512: 29a203905538e8311e3249b78565abe69ce36dc4ec239bec85c726c30e1a7b55b0aaf5c6659b676935008e068cfa53d716f7a598469064108daf130f94329a5d

ccef5d7 test: add two edge case tests for CSubNet (Vasil Dimov) Pull request description: This is chopped off from bitcoin#19031. It is needed because later 19031 modifies the related code and the tests ensure that no surprising changes in behavior sneak in. ACKs for top commit: practicalswift: ACK ccef5d7 -- more test coverage is better than less test coverage :) laanwj: ACK ccef5d7 hebasto: ACK ccef5d7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 6d386672b6598aeddd33dabe3512e816cf548d5c1af56c4c9e6f897d513b62ba4659cde73405811a0df286ffee3a3f084ab7caf8e3a2086fa9ddecd1bdcb3c67

bcfebb6 net: save the network type explicitly in CNetAddr (Vasil Dimov) 100c64a net: document `enum Network` (Vasil Dimov) Pull request description: (chopped off from bitcoin#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`. ACKs for top commit: troygiorshev: reACK bcfebb6 via `git range-diff master 64897c5 bcfebb6` jonatack: re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind. laanwj: Code review ACK bcfebb6 Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b Signed-off-by: pasta <pasta@dashboost.org> # Conflicts: # src/netaddress.cpp # src/netaddress.h

bc74a40 net: improve encapsulation of CNetAddr (Vasil Dimov) Pull request description: Do not access `CNetAddr::ip` directly from `CService` methods. This improvement will help later when we change the type of `CNetAddr::ip` (in the BIP155 implementation). (chopped off from bitcoin#19031 to ease review) ACKs for top commit: dongcarl: ACK bc74a40 naumenkogs: ACK bc74a40 fjahr: Code review ACK bc74a40 laanwj: code review ACK bc74a40 jonatack: ACK bc74a40 jnewbery: ACK bc74a40 Tree-SHA512: 29a203905538e8311e3249b78565abe69ce36dc4ec239bec85c726c30e1a7b55b0aaf5c6659b676935008e068cfa53d716f7a598469064108daf130f94329a5d

ccef5d7 test: add two edge case tests for CSubNet (Vasil Dimov) Pull request description: This is chopped off from bitcoin#19031. It is needed because later 19031 modifies the related code and the tests ensure that no surprising changes in behavior sneak in. ACKs for top commit: practicalswift: ACK ccef5d7 -- more test coverage is better than less test coverage :) laanwj: ACK ccef5d7 hebasto: ACK ccef5d7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 6d386672b6598aeddd33dabe3512e816cf548d5c1af56c4c9e6f897d513b62ba4659cde73405811a0df286ffee3a3f084ab7caf8e3a2086fa9ddecd1bdcb3c67

bcfebb6 net: save the network type explicitly in CNetAddr (Vasil Dimov) 100c64a net: document `enum Network` (Vasil Dimov) Pull request description: (chopped off from bitcoin#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`. ACKs for top commit: troygiorshev: reACK bcfebb6 via `git range-diff master 64897c5 bcfebb6` jonatack: re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind. laanwj: Code review ACK bcfebb6 Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b Signed-off-by: pasta <pasta@dashboost.org> # Conflicts: # src/netaddress.cpp # src/netaddress.h

ccef5d7 test: add two edge case tests for CSubNet (Vasil Dimov) Pull request description: This is chopped off from bitcoin#19031. It is needed because later 19031 modifies the related code and the tests ensure that no surprising changes in behavior sneak in. ACKs for top commit: practicalswift: ACK ccef5d7 -- more test coverage is better than less test coverage :) laanwj: ACK ccef5d7 hebasto: ACK ccef5d7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 6d386672b6598aeddd33dabe3512e816cf548d5c1af56c4c9e6f897d513b62ba4659cde73405811a0df286ffee3a3f084ab7caf8e3a2086fa9ddecd1bdcb3c67

bcfebb6 net: save the network type explicitly in CNetAddr (Vasil Dimov) 100c64a net: document `enum Network` (Vasil Dimov) Pull request description: (chopped off from bitcoin#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`. ACKs for top commit: troygiorshev: reACK bcfebb6 via `git range-diff master 64897c5 bcfebb6` jonatack: re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind. laanwj: Code review ACK bcfebb6 Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b Signed-off-by: pasta <pasta@dashboost.org> # Conflicts: # src/netaddress.cpp # src/netaddress.h

bc74a40a56128f81f11151d5966f53b82f19038c net: improve encapsulation of CNetAddr (Vasil Dimov) Pull request description: Do not access `CNetAddr::ip` directly from `CService` methods. This improvement will help later when we change the type of `CNetAddr::ip` (in the BIP155 implementation). (chopped off from bitcoin/bitcoin#19031 to ease review) ACKs for top commit: dongcarl: ACK bc74a40a56128f81f11151d5966f53b82f19038c naumenkogs: ACK bc74a40 fjahr: Code review ACK bc74a40 laanwj: code review ACK bc74a40a56128f81f11151d5966f53b82f19038c jonatack: ACK bc74a40a56128f81f11151d5966f53b82f19038c jnewbery: ACK bc74a40a5 Tree-SHA512: 29a203905538e8311e3249b78565abe69ce36dc4ec239bec85c726c30e1a7b55b0aaf5c6659b676935008e068cfa53d716f7a598469064108daf130f94329a5d

ccef5d7bf0af8377c6c779295f7b41d5af435c47 test: add two edge case tests for CSubNet (Vasil Dimov) Pull request description: This is chopped off from bitcoin/bitcoin#19031. It is needed because later 19031 modifies the related code and the tests ensure that no surprising changes in behavior sneak in. ACKs for top commit: practicalswift: ACK ccef5d7bf0af8377c6c779295f7b41d5af435c47 -- more test coverage is better than less test coverage :) laanwj: ACK ccef5d7bf0af8377c6c779295f7b41d5af435c47 hebasto: ACK ccef5d7bf0af8377c6c779295f7b41d5af435c47, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 6d386672b6598aeddd33dabe3512e816cf548d5c1af56c4c9e6f897d513b62ba4659cde73405811a0df286ffee3a3f084ab7caf8e3a2086fa9ddecd1bdcb3c67

bcfebb6d5511ad4c156868bc799831ace628a225 net: save the network type explicitly in CNetAddr (Vasil Dimov) 100c64a95b518a6a19241aec4058b866a8872d9b net: document `enum Network` (Vasil Dimov) Pull request description: (chopped off from bitcoin/bitcoin#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`. ACKs for top commit: troygiorshev: reACK bcfebb6d5511ad4c156868bc799831ace628a225 via `git range-diff master 64897c5 bcfebb6` jonatack: re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind. laanwj: Code review ACK bcfebb6d5511ad4c156868bc799831ace628a225 Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b Signed-off-by: pasta <pasta@dashboost.org>

vasild commented May 20, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

This was referenced May 20, 2020

Tor v3 support #18884

Closed

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

Closed

DrahtBot added GUI P2P RPC/REST/ZMQ Tests labels May 20, 2020

DrahtBot added the Needs rebase label May 20, 2020

vasild force-pushed the addrv2 branch 2 times, most recently from 1d8592c to 43c59ed Compare May 20, 2020 20:00

This was referenced May 20, 2020

refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg #18937

Merged

The Zero Allocations project #18849

Closed

DrahtBot removed the Needs rebase label May 20, 2020

This was referenced May 20, 2020

refactor: test: replace inv type magic numbers by constants #18764

Merged

addrman: improve performance by using more suitable containers #18722

Merged

vasild force-pushed the addrv2 branch 5 times, most recently from be45d10 to 5b5ee14 Compare May 22, 2020 19:55

maflcko removed GUI RPC/REST/ZMQ labels May 27, 2020

This was referenced Sep 29, 2020

net: don't try to relay to the address' originator #19763

Merged

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

DrahtBot mentioned this pull request Sep 29, 2020

Use std::chrono for the time to rotate destination of addr messages + tests #18642

Merged

DrahtBot added the Needs rebase label Sep 29, 2020

vasild and others added 2 commits September 29, 2020 10:30

vasild force-pushed the addrv2 branch from e1da409 to fb90d27 Compare September 29, 2020 08:41

vasild closed this Sep 29, 2020

PastaPastaPasta mentioned this pull request Jan 18, 2021

Backport partial addrv2 support dashpay/dash#3939

Merged

kwvg mentioned this pull request Mar 8, 2021

partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 dashpay/dash#4025

Closed

fdoving mentioned this pull request Jun 11, 2021

Add support for TOR V3 RavenProject/Ravencoin#1062

Open

zancas mentioned this pull request Sep 20, 2021

WIP: A draft PR for collaborating on ZIP155 zcash/zcash#5276

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ADDRv2 support (part of BIP155) #19031

Implement ADDRv2 support (part of BIP155) #19031

vasild commented May 20, 2020 •

edited

Loading

dongcarl commented May 20, 2020

laanwj commented May 20, 2020

vasild commented May 20, 2020

DrahtBot commented May 20, 2020 •

edited

Loading

naumenkogs commented May 20, 2020

hebasto commented Sep 29, 2020

DrahtBot commented Sep 29, 2020

vasild commented Sep 29, 2020

vasild commented Sep 29, 2020

Implement ADDRv2 support (part of BIP155) #19031

Implement ADDRv2 support (part of BIP155) #19031

Conversation

vasild commented May 20, 2020 • edited Loading

Preparation changes

Change CNetAddr::ip to have flexible size

Implement addrv2 (un)serializing

Advertise support

dongcarl commented May 20, 2020

laanwj commented May 20, 2020

vasild commented May 20, 2020

DrahtBot commented May 20, 2020 • edited Loading

Conflicts

naumenkogs commented May 20, 2020

hebasto commented Sep 29, 2020

DrahtBot commented Sep 29, 2020

vasild commented Sep 29, 2020

vasild commented Sep 29, 2020

vasild commented May 20, 2020 •

edited

Loading

Change `CNetAddr::ip` to have flexible size

Implement `addrv2` (un)serializing

DrahtBot commented May 20, 2020 •

edited

Loading