Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p2p: remove tor v2 support #22050

Merged
merged 13 commits into from
Jun 3, 2021
Merged

Conversation

jonatack
Copy link
Contributor

@jonatack jonatack commented May 24, 2021

image

This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.

  • no longer serialize/deserialize and relay Tor v2 addresses
  • ignore incoming Tor v2 addresses
  • remove Tor v2 addresses from the addrman and peers.dat on node launch
  • update generate-seeds.py to ignore Tor v2 addresses
  • remove Tor v2 hard-coded seeds

Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in #21351

<bitcoind debug log>
Socks5() connect to […].onion:8333 failed: general failure

<tor log>
Invalid hostname [scrubbed]; rejecting

and the addrman no longer has Tor v2 addresses on launching bitcoind.

$ ./src/bitcoin-cli -addrinfo
{
  "addresses_known": {
    "ipv4": 44483,
    "ipv6": 8467,
    "torv2": 0,
    "torv3": 2296,
    "i2p": 6,
    "total": 55252
  }
}

After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.

Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.

Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers: rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion

Screenshot from 2021-05-28 00-26-17

Tested using addnode, getaddednodeinfo,addpeeraddress, disconnectnode and -addrinfo that a currently valid, connectable Tor v2 peer can no longer be added:

Screenshot from 2021-05-30 11-32-05

Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.

@jonatack
Copy link
Contributor Author

A recent IRC discussion for more context. http://www.erisian.com.au/bitcoin-core-dev/log-2021-05-20.html#l-401

@theStack
Copy link
Contributor

Concept ACK

@Rspigler
Copy link
Contributor

Concept ACK

@jonatack jonatack marked this pull request as draft May 25, 2021 06:25
@dunxen
Copy link
Contributor

dunxen commented May 25, 2021

Concept ACK. (Thank you for your service, tor v2)

@0xB10C
Copy link
Contributor

0xB10C commented May 25, 2021

Concept ACK

@laanwj laanwj added this to the 22.0 milestone May 25, 2021
@laanwj
Copy link
Member

laanwj commented May 25, 2021

Concept ACK, adding 22.0 label
Going to test this.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 25, 2021

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

Conflicts

No conflicts as of last run.

@jonatack
Copy link
Contributor Author

This seems to be ready. Updated the PR description. Will propose release documentation in a follow-up.

@jonatack jonatack marked this pull request as ready for review May 28, 2021 08:18
@willcl-ark
Copy link
Member

Light tested ACK, rebased on top of master @ 123b401

Used Tor version 0.4.5.7. with manually configured hidden service and a bitcoin.conf with

listenonion=1
onlynet=onion
proxy=127.0.0.1:9050
  • Node has not picked up any Tor V2 addresses since startup and has a healthy number of V3 addresses in a short period of time:

Screenshot 2021-05-28 at 20 16 13

  • Tested bootstrapping without peers.dat and that picked up over 1000 V3 peers in just a few minutes.

  • Adding a V2 address check back into src/test/net_test.cpp resulted in expected test failure:

test/net_tests.cpp(614): error: in "net_tests/cnetaddr_unserialize_v2": check addr.IsValid() has failed

@jonatack
Copy link
Contributor Author

jonatack commented May 29, 2021

Changed generate-seeds.py to ignore torv2 addresses rather than raise. (Edit: I don't know if using None or an optional type would be preferred to -1 or more pythonic, happy to update if so.)

git diff 5715134 ef34bda

diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py
index aaccefcf6f..ae97240203 100755
--- a/contrib/seeds/generate-seeds.py
+++ b/contrib/seeds/generate-seeds.py
@@ -47,8 +47,10 @@ def name_to_bip155(addr):
     if addr.endswith('.onion'):
         vchAddr = b32decode(addr[0:-6], True)
         if len(vchAddr) == 35:
-            assert(vchAddr[34] == 3)
+            assert vchAddr[34] == 3
             return (BIP155Network.TORV3, vchAddr[:32])
+        elif len(vchAddr) == 10:
+            return (BIP155Network.TORV2, vchAddr)
         else:
             raise ValueError('Invalid onion %s' % vchAddr)
     elif addr.endswith('.b32.i2p'):
@@ -98,7 +100,10 @@ def parse_spec(s):
 
     host = name_to_bip155(host)
 
-    return host + (port, )
+    if host[0] == BIP155Network.TORV2:
+        return -1  # TORV2 is no longer supported, so we ignore it
+    else:
+        return host + (port, )
 
 def ser_compact_size(l):
     r = b""
@@ -134,6 +139,8 @@ def process_nodes(g, f, structname):
             continue
 
         spec = parse_spec(line)
+        if spec == -1:  # ignore this entry (e.g. no longer supported addresses like TORV2)
+            continue
         blob = bip155_serialize(spec)
         hoststr = ','.join(('0x%02x' % b) for b in blob)
         g.write(f'    {hoststr},\n')

@benthecarman
Copy link
Contributor

utACK ef34bda

@maflcko maflcko requested a review from laanwj June 2, 2021 05:01
@maflcko
Copy link
Member

maflcko commented Jun 2, 2021

cc @laanwj from #22050 (comment)

contrib/seeds/generate-seeds.py Outdated Show resolved Hide resolved
src/netaddress.cpp Show resolved Hide resolved
@jonatack
Copy link
Contributor Author

jonatack commented Jun 3, 2021

Changed the torv2-in-ipv6 case to deserialize explicitly to an invalid address which will be ignored, changed the ignore value in generate-seeds.py from -1 to None, extracted the onion-to-string code to a helper, and updated a Doxygen comment.

git diff ef34bda 5d82a57

diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py
index ae97240203..0ce868bfcc 100755
--- a/contrib/seeds/generate-seeds.py
+++ b/contrib/seeds/generate-seeds.py
@@ -101,7 +101,7 @@ def parse_spec(s):
     host = name_to_bip155(host)
 
     if host[0] == BIP155Network.TORV2:
-        return -1  # TORV2 is no longer supported, so we ignore it
+        return None  # TORV2 is no longer supported, so we ignore it
     else:
         return host + (port, )
 
@@ -139,7 +139,7 @@ def process_nodes(g, f, structname):
             continue
 
         spec = parse_spec(line)
-        if spec == -1:  # ignore this entry (e.g. no longer supported addresses like TORV2)
+        if spec is None:  # ignore this entry (e.g. no longer supported addresses like TORV2)
             continue
         blob = bip155_serialize(spec)
         hoststr = ','.join(('0x%02x' % b) for b in blob)
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index e46f9c2568..0ae8dd0698 100644
--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -145,6 +145,13 @@ void CNetAddr::SetLegacyIPv6(Span<const uint8_t> ipv6)
         // IPv4-in-IPv6
         m_net = NET_IPV4;
         skip = sizeof(IPV4_IN_IPV6_PREFIX);
+    } else if (HasPrefix(ipv6, TORV2_IN_IPV6_PREFIX)) {
+        // TORv2-in-IPv6 (unsupported). Unserialize as !IsValid(), thus ignoring them.
+        // Mimic a default-constructed CNetAddr object which is !IsValid() and thus
+        // will not be gossiped, but continue reading next addresses from the stream.
+        m_net = NET_IPV6;
+        m_addr.assign(ADDR_IPV6_SIZE, 0x0);
+        return;
     } else if (HasPrefix(ipv6, INTERNAL_IN_IPV6_PREFIX)) {
         // Internal-in-IPv6
         m_net = NET_INTERNAL;
@@ -587,31 +594,26 @@ static std::string IPv6ToString(Span<const uint8_t> a, uint32_t scope_id)
     return r;
 }
 
+static std::string OnionToString(const Span<const uint8_t>& addr)
+{
+    uint8_t checksum[torv3::CHECKSUM_LEN];
+    torv3::Checksum(addr, checksum);
+    // TORv3 onion_address = base32(PUBKEY | CHECKSUM | VERSION) + ".onion"
+    prevector<torv3::TOTAL_LEN, uint8_t> address{addr.begin(), addr.end()};
+    address.insert(address.end(), checksum, checksum + torv3::CHECKSUM_LEN);
+    address.insert(address.end(), torv3::VERSION, torv3::VERSION + sizeof(torv3::VERSION));
+    return EncodeBase32(address) + ".onion";
+}
+
 std::string CNetAddr::ToStringIP() const
 {
     switch (m_net) {
     case NET_IPV4:
         return IPv4ToString(m_addr);
-    case NET_IPV6: {
+    case NET_IPV6:
         return IPv6ToString(m_addr, m_scope_id);
-    }
     case NET_ONION:
-        switch (m_addr.size()) {
-        case ADDR_TORV3_SIZE: {
-
-            uint8_t checksum[torv3::CHECKSUM_LEN];
-            torv3::Checksum(m_addr, checksum);
-
-            // TORv3 onion_address = base32(PUBKEY | CHECKSUM | VERSION) + ".onion"
-            prevector<torv3::TOTAL_LEN, uint8_t> address{m_addr.begin(), m_addr.end()};
-            address.insert(address.end(), checksum, checksum + torv3::CHECKSUM_LEN);
-            address.insert(address.end(), torv3::VERSION, torv3::VERSION + sizeof(torv3::VERSION));
-
-            return EncodeBase32(address) + ".onion";
-        }
-        default:
-            assert(false);
-        }
+        return OnionToString(m_addr);
     case NET_I2P:
         return EncodeBase32(m_addr, false /* don't pad with = */) + ".b32.i2p";
     case NET_CJDNS:
diff --git a/src/netaddress.h b/src/netaddress.h
index d539bd51ff..0d04ab88fb 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -299,7 +299,7 @@ class CNetAddr
         /**
          * Get the BIP155 network id of this address.
          * Must not be called for IsInternal() objects.
-         * @returns BIP155 network id
+         * @returns BIP155 network id, except TORV2 which is no longer supported.
          */
         BIP155Network GetBIP155Network() const;

@laanwj
Copy link
Member

laanwj commented Jun 3, 2021

Changes look good to me now.
Code review ACK 5d82a57
Tested ACK of an earlier commit, which I tested for a while on one of my nodes. I noticed no difference in behavior except for no longer connecting to Tor v2 nodes.

@laanwj laanwj merged commit 07ededa into bitcoin:master Jun 3, 2021
@jonatack jonatack deleted the torv2-remove-support branch June 3, 2021 17:10
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 3, 2021
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5d82a57

Some followups in #22179.

Comment on lines -75 to -82
case BIP155Network::TORV2:
if (address_size == ADDR_TORV2_SIZE) {
m_net = NET_ONION;
return true;
}
throw std::ios_base::failure(
strprintf("BIP155 TORv2 address with length %u (should be %u)", address_size,
ADDR_TORV2_SIZE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(can be ignored)

This change in CNetAddr::SetNetFromBIP155Network() will now accept and ignore a Tor v2 addresses with bogus length, e.g. 13. whereas we know it should be exactly 10. The comment near the bottom of this function: // Don't throw on addresses with unknown network ids (maybe from the future). is somewhat incorrect now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll have a look.

laanwj added a commit that referenced this pull request Jun 12, 2021
…2 support

2ad034a doc: update release notes with removal of tor v2 support (Jon Atack)
49938ee doc: update tor.md with removal of tor v2 support (Jon Atack)

Pull request description:

  Follow-up documentation to #22050 that removed support for Tor version 2 hidden services from Bitcoin Core.

ACKs for top commit:
  laanwj:
    ACK 2ad034a

Tree-SHA512: 0f13f9d1db7e11f1e3d9967b6d17b8dc3144b3ab3a258c706464c5e6ac5cbcf2ce2db4ea54be9939f05a82ebd1e7f325f50b435f9822c08b4f21ed4ac58de0af
@jonatack jonatack mentioned this pull request Sep 3, 2021
laanwj added a commit that referenced this pull request Sep 16, 2021
…etworkName()

49d503a doc: update -addrinfo in release-notes.md and tor.md (Jon Atack)
75ea9ec cli -addrinfo: drop torv2, torv3 becomes onion per GetNetworkName() (Jon Atack)

Pull request description:

  #22050 removed torv2 support from 22.0. For 23.0 and subsequent releases, we can probably remove torv2 from -addrinfo.

  before
  ```
    "addresses_known": {
      "ipv4": 58305,
      "ipv6": 5138,
      "torv2": 0,
      "torv3": 5441,
      "i2p": 14,
      "total": 68898
    }
  ```
  after
  ```
    "addresses_known": {
      "ipv4": 58305,
      "ipv6": 5138,
      "onion": 5441,
      "i2p": 14,
      "total": 68898
    }
  ```
  Per the naming of `netbase.{h, cpp}::GetNetworkName()`, torv3 becomes onion, which is what is printed in the output of getpeerinfo, getnetworkinfo and getnodeaddresses.

ACKs for top commit:
  practicalswift:
    cr ACK 49d503a
  Zero-1729:
    tACK 49d503a 🧉
  klementtan:
    Code review and tested ACK 49d503a

Tree-SHA512: bca52520d8b12c26f1c329d661b9e22c567954ed2af7d2a16d7669eae1a221eada20944f8b2f4e78e31a7190d5f3d3fbfd37509e5edf2d9a3747a0a8f4e375bb
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet