Complete the BIP155 implementation and upgrade to TORv3

[vasild]

This PR contains the two remaining commits from #19031 to complete the BIP155 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

[vasild]

This builds on top of #19031, adding one more commit to complete the TORv3 puzzle. The dependency to #19031 is because if we create a TORv3 hidden service before #19031 is merged we will not be able to advertise it by gossiping. So, this PR is draft until all of #19031 is merged. Available for early review and testing.

[jonatack]

Concept ACK, am running this right now.

Debug build is clean, all tests are passing for me locally, and above all, for the first time I am operating a torv3 bitcoind service.

[jonatack]

Initial (light) pass through the code looks good apart from the minor comments that follow. For the three comments with the word "Sneak", suggest "Bitwise OR the" instead.

[ghost]

Can we also add the option to delete onion_private_key and onion_private_key_obsolete_torv2automatically every time bitcoind is restarted? So that it gets new address each time.

Or is it something out of scope for this PR and will need a new issue or PR to discuss?

[sipa]

@prayank23 It can take a (very) long time for a node's rumoured address to be picked up, especially Tor ones. Getting a new one every time you restart your node is probably a good way to never get any incoming connections at all.

It may be useful as an optional feature, but seems out of scope here.

[Saibato]

@prayank23

Can we also add the option to delete onion_private_key and onion_private_key_obsolete_torv2automatically every time bitcoind is restarted? So that it gets new address each time.

tyi; #19635 we probably will need that in some form for v3 anyway.

[laanwj]

@prayank23

Can we also add the option to delete onion_private_key and onion_private_key_obsolete_torv2automatically every time bitcoind is restarted? So that it gets new address each time.

Yes, but out of scope here. There's an issue for this already: #17491.

[Saibato]

@laanwj

I'd slightly prefer creating a new file for v3 instead

@vasild
Feel free to grab there, then i can close that PR

#19485

[tryphe]

utACK 9c9ff0a. Light code review, code looks great, apart from several uses of char name[1025] which seem a bit clunky (oops, just noticed this nit was a refactor and not an addition, am okay with no change). Will need to fire up a test build. Nice work!

[Sjors]

Concept ACK, I share Wladimir's preference for "creating a new file for v3 instead of moving the old one". Plus it'll make this change even smaller.

Perhaps begin with v3 as opt-in, then in the following release look at making it the default.

Given that v2 is deprecated I don't think v3 needs to be opt-in, at least not for new users.

Existing users might rely on a static v2 address, e.g. because they have another node connected to it, or they paired it with some remote control software. Afaik this only applies to bitcoind users, not QT (unless @luke-jr implemented pairing in Knots?). So this change needs some emphasis in the release notes.

We could add an (instantly deprecated) option -listenonion_v2.

[vasild]

Rebased on latest #19031 and addressed suggestions.

Indeed creating a new file for the new TORv3 private key seems like a better idea. It also opens up a possibility for the users to continue using the old TORv2 service if needed by copying the contents of the old file to the new file.

I think Bitcoin Core automatically creating just one Tor service is enough (that will be v3). If more complicated setup is needed, then the users can create static services in the Tor daemon - e.g. it is possible to create 5 TORv2 services and 7 TORv3 services all pointing to the same Bitcoin Core.

[jonatack]

Practical testing of this PR with various Tor configuration options by and between @vasild and I today. Will review this weekend.

[DrahtBot]

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: Add consistent formatting for messages.py repr methods #19889 (test: Add consistent formatting for messages.py repr methods by adaminsky)
• [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)
• tests: Update more tests to work with descriptor wallets #18788 (tests: Update more tests to work with descriptor wallets by achow101)

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.

[Sjors]

tACK 1550aa8 just the last commit

I was able launch a hidden v3 service on Ubuntu and connect to it.

[laanwj]

I think Bitcoin Core automatically creating just one Tor service is enough (that will be v3).

The only worry here would be that old nodes will run out of Tor nodes to connect to. But I don't know. I'm okay with this, it seems there's enough hurry to transition here that backwards compatibility concerns like that are mostly moot.

Edit: a related concern is that we should start hardcoding TorV3 peers. No need to do so in this PR though. Made a note about this in #17020.

[jonatack]

Tested ACK 1550aa8 (what an appropriate commit hash for BIP155). Reviewed the changes, and over the past 10 days have done multiple debug builds, test runs, and have been running and re-running bitcoind on and off with this branch as a Tor v3 service connected to several peers that are also test-deploying this PR live on mainnet.

Thanks for adding the unit tests. A few non-blocking comments follow.

[jonatack]

The only worry here would be that old nodes will run out of Tor nodes to connect to. But I don't know. I'm okay with this, it seems there's enough hurry to transition here that backwards compatibility concerns like that are mostly moot.

Maybe it would be good for the release notes to describe the situation with the planned full removal of v2 by the Tor Project and warn users running older nodes with an onion service that they will have to upgrade within less than a year after the release to v0.21+. It might not need to be in this PR though.

[maflcko]

+ make -C src --jobs=1 check-security V=1
make: Entering directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
Checking binary security...
READELF=/gnu/store/pzq9s5ldl90h72p26jkdakyccdg9ib3k-profile/bin/x86_64-linux-gnu-readelf OBJDUMP=x86_64-linux-gnu-objdump OTOOL= /gnu/store/pzq9s5ldl90h72p26jkdakyccdg9ib3k-profile/bin/python3.7 ../contrib/devtools/security-check.py bitcoind  bitcoin-cli bitcoin-tx bitcoin-wallet test/test_bitcoin bench/bench_bitcoin qt/bitcoin-qt  
make: Leaving directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
+ case "$HOST" in
+ make -C src --jobs=1 check-symbols V=1
make: Entering directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
Checking glibc back compat...
READELF=/gnu/store/pzq9s5ldl90h72p26jkdakyccdg9ib3k-profile/bin/x86_64-linux-gnu-readelf CPPFILT=/gnu/store/pzq9s5ldl90h72p26jkdakyccdg9ib3k-profile/bin/x86_64-linux-gnu-c++filt /gnu/store/pzq9s5ldl90h72p26jkdakyccdg9ib3k-profile/bin/python3.7 ../contrib/devtools/symbol-check.py bitcoind  bitcoin-cli bitcoin-tx bitcoin-wallet test/test_bitcoin bench/bench_bitcoin qt/bitcoin-qt  
test/test_bitcoin: export of symbol in6addr_loopback not allowed
test/test_bitcoin: failed EXPORTED_SYMBOLS
make: *** [Makefile:20952: check-symbols] Error 1
make: Leaving directory '/bitcoin/distsrc-x86_64-linux-gnu/src'

[maflcko]

This breaks the guix/gitian build. cc @fanquake

[vasild]

The patch below may fix it, but I can't confirm because on my machine contrib/devtools/symbol-check.py is happy with src/test/test_bitcoin which contains:

nm src/test/test_bitcoin |grep in6addr_loopback
                 U in6addr_loopback

possible workaround symbol-check.py in6addr_loopback

diff --git i/src/test/netbase_tests.cpp w/src/test/netbase_tests.cpp
index eb0d95a37..f473c092a 100644
--- i/src/test/netbase_tests.cpp
+++ w/src/test/netbase_tests.cpp
@@ -447,25 +447,27 @@ BOOST_AUTO_TEST_CASE(netbase_dont_resolve_strings_with_embedded_nul_characters)
     BOOST_CHECK(!LookupSubNet(std::string("5wyqrzbvrdsumnok.onion\0example.com\0", 35), ret));
 }
 
 // Since CNetAddr (un)ser is tested separately in net_tests.cpp here we only
 // try a few edge cases for port, service flags and time.
 
+static const struct in6_addr in6addr_loopback_ = IN6ADDR_LOOPBACK_INIT;
+
 static const std::vector<CAddress> fixture_addresses({
     CAddress(
-        CService(CNetAddr(in6addr_loopback), 0 /* port */),
+        CService(CNetAddr(in6addr_loopback_), 0 /* port */),
         NODE_NONE,
         0x4966bc61U /* Fri Jan  9 02:54:25 UTC 2009 */
     ),
     CAddress(
-        CService(CNetAddr(in6addr_loopback), 0x00f1 /* port */),
+        CService(CNetAddr(in6addr_loopback_), 0x00f1 /* port */),
         NODE_NETWORK,
         0x83766279U /* Tue Nov 22 11:22:33 UTC 2039 */
     ),
     CAddress(
-        CService(CNetAddr(in6addr_loopback), 0xf1f2 /* port */),
+        CService(CNetAddr(in6addr_loopback_), 0xf1f2 /* port */),
         static_cast<ServiceFlags>(NODE_WITNESS | NODE_COMPACT_FILTERS | NODE_NETWORK_LIMITED),
         0xffffffffU /* Sun Feb  7 06:28:15 UTC 2106 */
     )
 });
 
 // fixture_addresses should equal to this when serialized in V1 format.

[sipa]

See #20127.

[vasild]

Congratulations to everybody involved 🎉

Notice - I did not write the BIP, did not write the code initially, did not do the reviews nor did I test it much or nag people to prioritize it. Yet, this got a clean specification, solid code, thorough reviews and testing and was merged in time for 0.21. A true team effort!

Just Epic!

[jnewbery]

"if the file is modified by 0.21.0 or newer then older versions will not be able to read it. Those old versions, in the event of a downgrade, will log an error message that deserialization has failed and will continue normal operation"

I don't see where this is enforced in 0.20 or earlier versions. If I'm understanding the v0.20 code correctly, then it'll try to unserialize the file, read garbage for the CAddress objects because the format has changed, but then not necessarily fail.

[vasild]

You are right!

A dirty hack like the following would trick <=0.20.1 to always fail deserialization of files in format >=Format::V3_BIP155 with Incorrect keysize in addrman deserialization

diff --git i/src/addrman.h w/src/addrman.h
index b4089dc89..9f6614cd1 100644
--- i/src/addrman.h
+++ w/src/addrman.h
@@ -326,19 +326,19 @@ public:
     void Serialize(Stream& s_) const
     {
         LOCK(cs);
 
         // Always serialize in the latest version (currently Format::V3_BIP155).
 
         OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
 
         s << static_cast<uint8_t>(Format::V3_BIP155);
-        s << ((unsigned char)32);
+        s << ((unsigned char)33);
         s << nKey;
         s << nNew;
         s << nTried;
 
         int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
         s << nUBuckets;
         std::map<int, int> mapUnkIds;
         int nIds = 0;
         for (const auto& entry : mapInfo) {
@@ -406,18 +406,21 @@ public:
             // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
             // unserialize methods know that an address in addrv2 format is coming.
             stream_version |= ADDRV2_FORMAT;
         }
 
         OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
 
         unsigned char nKeySize;
         s >> nKeySize;
+        if (format >= Format::V3_BIP155) {
+            --nKeySize;
+        }
         if (nKeySize != 32) throw std::ios_base::failure("Incorrect keysize in addrman deserialization");
         s >> nKey;
         s >> nNew;
         s >> nTried;
         int nUBuckets = 0;
         s >> nUBuckets;
         if (format >= Format::V1_DETERMINISTIC) {
             nUBuckets ^= (1 << 30);
         }

[jnewbery]

Yes, that would work. It's essentially repurposing the keysize field in the peers.dat file as a "non forward-compatible version number" (i.e. if a newer version of the software writes a higher number here, then an older version of the software wouldn't be able to read that file).

I believe that's what you wanted to achieve with the code here: https://github.com/bitcoin/bitcoin/pull/19954/files#diff-164bd9e2e30f54d0a79eb7cc372309e2f2155edc6c3f051290ab078f03f6a771R396, but of course that only applies for v0.21 onwards, for file versions higher than Format::V3_BIP155. That code means that we now have no way to make a downgrade-safe change to the peers.dat file version. Any bump to the format number means that older software versions can't read the file. Note that we have made downgrade-safe version changes before (I think both v0-v1 and v1-v2 were both downgrade-safe), and it would be nice to keep this as a possibility.

Repurposing the keysize field and reverting the change to the format field seems ok and would almost be turning them into major (non-forward-compatible) and minor (forward-compatible) version numbers. It should go without saying that we always want to have backward-compatibility, so peers.dat files written by old versions are readable by new versions.

Obviously we should get any changes made before the 0.21 release.

[vasild]

See if this will make sense to you: #20284