Complete the BIP155 implementation and upgrade to TORv3 #19954

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

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.

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?

@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.

@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.

@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.

@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

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.

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.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19889 (test: Add consistent formatting for messages.py repr methods by adaminsky)
• #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
• #19238 (refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto)
• #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.

tACK 1550aa82659506ef89db67e482d01f014f6a1977 just the last commit

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

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.

Tested ACK 1550aa82659506ef89db67e482d01f014f6a1977 (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.

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.

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

… old nodes will run out of Tor nodes to connect to …

If there are N old nodes each one of them will be able to connect to the other N-1. Only the very last one (when N=1) will have nobody to connect to. Also, statically configured Tor v2 services (via torrc) will keep operating as long as the Tor network supports them.

According to https://bitnodes.io/nodes/ 37% are running the latest version (0.20.0 or 0.20.1), now, about 2 months before the release of 0.21.

Approach ACK ff76c3444c574b14a2c62999b82cba812816884b.

nit: It seems more readable if all comparisons with m_format will use the same sign, say >=, instead of mixing >= and >.

Addresses with length > 512 or addresses from one of the BIP155 founding networks (1..6) with wrong length (e.g. IPv4 address with length 5):

We throw an exception during unserialize which means that subsequent addresses in the same addrv2 message will not be read and the peer is Misbehaving()ed.

Addresses from unknown network id (0 or >= 7), with address length <= 512:

Those are possibly legit addresses from a future extension of BIP155. We consume the address from the stream and keep parsing subsequent addresses in the same addrv2 message. The unknown address will however not be relayed or added to addrman (it is unserialized as all-0s IPv6 address which is !IsValid()).

I2P and CJDNS addresses (BIP155 network ids 5 and 6):

Relevant IRC discussion on how to handle those. There are concerns whether we should relay or add them to addrman because we can’t further verify their validity or check if a node actually exists at that address.

The current code actually would not add such addresses to addrman because they are !IsReachable(), so we would not get “garbage” in addrman.

The current code would however relay such addresses (to 1.5 peers like other !IsReachable() addresses).

• This has the downside of garbage gossip traffic. Is it any worse than somebody gossiping garbage IPv6 addresses? Honest nodes don’t have a way to verify if there is a node listening on a I2P address (yet). But even now we don’t verify if there is a node listening on an address before relaying it.
• The benefit is that once I2P support is added in version 0.2X then older nodes, e.g. 0.21 will relay I2P addresses and help with propagation.

 0--- i/src/net_processing.cpp
 1+++ w/src/net_processing.cpp
 2@@ -2643,14 +2643,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 3             pfrom.AddAddressKnown(addr);
 4             if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) {
 5                 // Do not process banned/discouraged addresses beyond remembering we received them
 6                 continue;
 7             }
 8             bool fReachable = IsReachable(addr);
 9-            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
10-            {
11+            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 &&
12+                addr.IsRoutable() && !addr.IsI2P() && !addr.IsCJDNS()) {
13                 // Relay to a limited number of other nodes
14                 RelayAddress(addr, fReachable, m_connman);
15             }
16             // Do not store addresses outside our network
17             if (fReachable)
18                 vAddrOk.push_back(addr);

What are your thoughts?

@vasild

Addresses with length > 512 or addresses from one of the BIP155 founding networks (1..6) with wrong length (e.g. IPv4 address with length 5): We throw an exception during unserialize which means that subsequent addresses in the same addrv2 message will not be read and the peer is Misbehaving()ed.

Sounds like correct behavior to me. Larger addresses are explicitly forbidden, and can be seen as an indication that the entire message is malformed.

Addresses from unknown network id (0 or >= 7), with address length <= 512: We consume the address from the stream and keep parsing subsequent addresses in the same addrv2 message. The unknown address will however not be relayed or added to addrman

Seems good to me too.

The current code would however relay such addresses (to 1.5 peers like other !IsReachable() addresses).

I’m not sure about this. I’m not sure we should do anything (store, relay) with addresses that we cannot connect to ourselves. But as you say this is a broader scope decision. Staying (at least in this PR) with the current behaviour for valid but unreachable addresses doesn’t sound unreasonable to me.

Tested re-ACK bb01c77865a565764e4e2f86265afef45506d7d7

Reviewed the diff since my last review per git range-diff 301959f 1550aa8 bb01c77, re-reviewed the changeset, rebuilt, deleted the onion_private_key_torv3 file from previous testing and relaunched; node seems to be running nominally. The additional tests are great. A few comments above to take or leave; none are blockers.

Tested bb01c77865a565764e4e2f86265afef45506d7d7 on Linux Mint 20 (x86_64). I’ve verified P2P messages in the testnet with Wireshark 3.2.3.

• “master-…” means bitcoind compiled against master
• “pr-…” means bitcoind compiled against this PR
• a peer before connecting has no peers.dat

Setup A – master-peer connects to pr-node

The pr-node sends the “sendaddrv2” message to the master-peer just after sending the “verack” message. In response to master-peer’s “getaddr” message the pr-node sends the “addr” message with 1000 addresses and payload size 30003 bytes (1000*30+3).

Setup B – pr-peer connects to pr-node

Both the pr-node and the pr-peer send each other the “sendaddrv2” message just after sending the “verack” message. In response to pr-peer’s “getaddr” message the pr-node sends the “addrv2” message with 1000 addresses and payload size 16463 bytes. Some addresses I observed are 13 bytes long.

Does addrv2 code follows https://github.com/bitcoin/bips/pull/967 ?

UPDATE: I’v verified that this PR follows BIP 155 with (yet unmerged) https://github.com/bitcoin/bips/pull/967

UPDATE-2: https://github.com/bitcoin/bips/pull/967 just merged.

ACK bb01c77865a565764e4e2f86265afef45506d7d7

May I suggest to replace s/“onion_private_key_torv3”/“onion_private_key_v3”/ as “tor” substring seems redundant here?

May I suggest to replace s/“onion_private_key_torv3”/“onion_private_key_v3”/ as “tor” substring seems redundant here?

Kind of agree, the same came to mind for me but didn’t bother to comment it initially. But a shorter filename that still contains the same information is good I suppose.

I restored the original version of p2p_addr_relay.py and the test for addrv2 is now in a new test p2p_addrv2_relay.py.

Nice!

@vasild in the latest push, the p2p_addr_relay.py file perms were changed from 755 to 644; could you please revert that?

 0diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py
 1index 19cfd0eadb..ca593148cc 100755
 2--- a/test/functional/p2p_addrv2_relay.py
 3+++ b/test/functional/p2p_addrv2_relay.py
 4@@ -6,18 +6,17 @@
 5@@ -6,18 +6,17 @@
 6 Test addrv2 relay
 7 """
 8 
 9+import time
10+
11 from test_framework.messages import (
12     CAddress,
13+    msg_addrv2,
14     NODE_NETWORK,
15     NODE_WITNESS,
16-    msg_addrv2,
17 )
18 from test_framework.p2p import P2PInterface
19 from test_framework.test_framework import BitcoinTestFramework
20-from test_framework.util import (
21-    assert_equal,
22-)
23-import time
24+from test_framework.util import assert_equal
25 
26 ADDRS = []
27 for i in range(10):
28@@ -48,11 +47,11 @@ class AddrReceiver(P2PInterface):
29 
30 class AddrTest(BitcoinTestFramework):
31     def set_test_params(self):
32-        self.setup_clean_chain = False
33+        self.setup_clean_chain = True
34         self.num_nodes = 1
35 
36     def run_test(self):
37-        self.log.info('Create connection that sends addr messages')
38+        self.log.info('Create connection that sends addrv2 messages')
39         addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
40         msg = msg_addrv2()
41 
42@@ -61,7 +60,7 @@ class AddrTest(BitcoinTestFramework):
43         with self.nodes[0].assert_debug_log(['addr message size = 1010']):
44             addr_source.send_and_ping(msg)
45 
46-        self.log.info('Check that addr message content is relayed and added to addrman')
47+        self.log.info('Check that addrv2 message content is relayed and added to addrman')
48         addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
49         msg.addrs = ADDRS
50         with self.nodes[0].assert_debug_log([

Tested that:

• restarting a node after running cp -f onion_private_key onion_private_key_v3 does indeed launch a Tor v2 service

• following which, restarting again after rm onion_private_key_v3 creates a new Tor v3 service

I’ve also been testing this branch rebased over #20002, which is based on #19998, and is a clean rebase.

A few more comments below. This PR seems close.

re-ACK 703db15ae197c6b2e9489c14af441c5d796ff082, tested on Linux Mint 20 (x86_64).

Besides checking network traffic, I’ve verified that my node is able to connect to peers with v2 and v3 onion addresses, and is reachable via v3 onion service. Created file with key in my datadir is onion_v3_private_key.

Thanks @vasild !

Filtered code coverage report (files not modified by this PR are omitted and not modified lines in files that are otherwise modified are dimmed).

List of modified and not covered lines.

Code review ACK modulo nits.

I tested:

• externalip= with torv3 address, and saw it reported in logs
• upgrading a automatically generated torv2 address to a v3 one
• automatically creating a torv3 address
• connecting two nodes with this patch to each other, and seeing addrv2 reported in getpeerinfo output

re-ACK 1f06a2901d1ee924b95f482e8693842ee07863de Updated my nodes with the old version of this patch to the current one (and renamed the key file, the new name is good), it still works as expected.

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 dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 merged on master (12a1c3ad1a43634d2a98717e49e3f02c4acea2fe).

Edit.2: I cannot reproduce this issue in any way. I have a binary that reproduces the problem, but building the same commit doesn’t reproduce it. It seems to have been a miscompile?!? In any case I doubt this is related to your specific code.

Edit.3: Okay, I dived deeper in my problem here and it 100% sure was a local miscompile issue. While debugging the broken binary using gdb and putting a watch on a node’s fDisconnect field, I noticed something really strange: the address of the field was changing between files. The line pfrom.fSuccessfullyConnected = true; was setting the fDisconnect to true, an off-by-one error. So some incompatible/stale files were being linked when switching between branches. These had a different conception of the CNode structure. The root cause is likely the --disable-dependency-tracking configure flag which is (or used to be) necessary to make configure pass on FreeBSD..

Sorry for the distraction. Will re-ACK.

Just to clarify (assuming this PR goes in 0.21): the format of peers.dat is changed in a backwards-incompatible way. Meaning that 0.20 and older will be unable to read peers.dat written by 0.21 or newer. This boils down to the fact that 0.20 and older cannot read e.g. torv3 addresses encoded in BIP155-addrv2 format.

This is not related in any way to the onion_private_key and onion_v3_private_key files and whether the node creates an onion service or not.

Just to clarify (assuming this PR goes in 0.21): the format of peers.dat is changed in a backwards-incompatible way. Meaning that 0.20 and older will be unable to read peers.dat written by 0.21 or newer. This boils down to the fact that 0.20 and older cannot read e.g. torv3 addresses encoded in BIP155-addrv2 format.

This only means starting with (effectively) an empty peers.dat if you downgrade to a pre-0.21 version, right? That’s okay. Loss of peers.dat is not a big deal to most users. As long as it doesn’t result in the program not starting at all. That would be kind of unfriendly.

(Nevertheless, it does make sense to mention the peers.dat format change and consequences in the release notes)

Earlier today on IRC:

 013:39:37 < vasild> CAddress::unser: time=1601829922, services=134218813, addr=62.107.200.30:8333
 113:39:56 < vasild> CAddress::unser: time=1600339981, services=134217741, addr=akinbo7tlegsnsxn.onion:8333
 213:40:31 < vasild> jonatack: wumpus: so...
 313:41:05 < vasild> in the network there are some peers with services=unusually high value
 413:42:07 < sipa> that's 0x0800000d
 513:42:14 < vasild> they get in via p2p gossip into addrman without causing problems
 613:42:16 < vasild> sipa: right
 713:43:18 < vasild> using addrv1
 813:44:03 < vasild> then addrman saves it on disk just fine in V3_BIP155 format using READWRITE(COMPACTSIZE(services_tmp))
 913:44:44 < vasild> however, on read back from disk it bricks because ReadCompactSize() contains this at the end:
1013:44:53 < vasild>     if (nSizeRet > (uint64_t)MAX_SIZE)
1113:44:54 < vasild>         throw std::ios_base::failure("ReadCompactSize(): size too large");
1213:45:18 < vasild> and MAX_SIZE is quite low value 0x02000000
1313:45:48 < vasild> I wondered before wtf is that limit but decided it is harmless
1413:46:14 < sipa> vasild: ehhhh
1513:46:27 < sipa> this means we need to bypass that limit when deserializing addrv2
1613:46:46 < vasild> so WriteCompactSize() can write >0x02000000 but ReadCompactSize() cannot read it back :-X

I wrote a small patch that allows bypassing the range check in ReadCompactSize: https://github.com/sipa/bitcoin/commits/202010_compactsize_bound. Feel free to cherry-pick or squash. It allows using without range check using Using<CompactSizeFormatter<false>>(obj) instead of COMPACTSIZE(obj).

13:46:46 < vasild> so WriteCompactSize() can write >0x02000000 but ReadCompactSize() cannot read it back :-X

Ouch, good catch! This makes me doubt the choice to use a CompactSize for anything else than a size a little. It seems like misuse of a typed value. @sipa’s patch to allow to bypass the check would solve the immediate issue, I just hope it won’t cause problems for maintainers in the future again if other sanity checks are added, with people thinking they’ll be size specific.

@laanwj Well, they’re already used as non-size values in BIP152, though luckily they’re restricted to 16-bit values there, so the range limit was not an issue.

I think it’s fine to use it in BIP155 as well. The code is a bit confusing but perhaps we should just rename CompactSize to something else at some point. It’s already known under other names outside Bitcoin Core anyway.

This only means starting with (effectively) an empty peers.dat if you downgrade to a pre-0.21 version, right?

Yes, that’s what I’m seeing.

(Nevertheless, it does make sense to mention the peers.dat format change and consequences in the release notes)

:+1:

Addressed review suggestions plus

• Added release notes on the peers.dat format change
• Made it possible to read big compact-size numbers (thanks to @sipa for the patch) this will enable addrman to read arbitrary big service flags from its database and also for the p2p layer to receive them when in addrv2 encoding (the old addr encoding uses fixed 8-byte representation)
• Added a human-readable error message if addrman detects a future format of peers.dat

Initial updated suggestion for the release notes, though I am sure it can be improved further (pinging @harding). It may need to be edited by hand in the wiki if the notes have already been merged and branched off.

 0Migration from deprecated Tor v2 to Tor v3
 1-----------------------
 2The Tor onion service that is automatically created (`-listenonion`) will now be
 3created as a Tor v3 service instead of Tor v2. The private key that was used for
 4Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` and
 5can be removed if not needed. Bitcoin Core will no longer attempt to read it.
 6The private key for the Tor v3 service will be saved in `onion_v3_private_key`.
 7To use the deprecated Tor v2 service (not recommended), the `onion_private_key` file
 8can be copied over `onion_v3_private_key`, e.g. `cp -f onion_private_key
 9onion_v3_private_key`. (#19954)
10
11Peers.dat file format change
12-----------------------
13The node's known peers are persisted to disk in a file called `peers.dat`. Starting
14with this v0.21.0 release, the `peers.dat` file format is changed in a backwards-incompatible
15way to accommodate the storage of large BIP155 addresses, such as Tor v3 addresses.
16Pre-0.21 Bitcoin Core software will not be able to read this new `peers.dat` format. In the
17event of downgrading to pre-0.21 after running 0.21, the debug log may print an error that
18deserialization has failed and a message that peers.dat is invalid or missing and is being
19recreated, and bitcoind will continue normal operation with a new `peers.dat` file. (#19954)

ACK 9b56a68a85ce32b13d3db605ac56bb9743437d2f per git diff 1f06a29 9b56a68, tested with v3/v2/v3 onion private keys, tested on v0.18.1, and tested the new error message.

In my testing, the new patch has fixed the DB deserialize errors and peers.dat messages on this branch after cp onion_private_key onion_v3_private_key.

Restarting on v0.18.1, as expected I see ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file: iostream error and Invalid or missing peers.dat; recreating.

@jonatack The peers.dat text looks fine to me. Here’s a few suggestions for the v3 onion service text, but I also think that was fine.

 0-The Tor onion service that is automatically created (`-listenonion`) will now be
 1+The Tor onion service that is automatically created by setting the `-listenonion` configuration parameter will now be
 2 created as a Tor v3 service instead of Tor v2. The private key that was used for
 3-Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` and
 4+Tor v2 (if any) will be left untouched in the `onion_private_key` file in the data directory (see `-datadir`)  and
 5 can be removed if not needed. Bitcoin Core will no longer attempt to read it.
 6-The private key for the Tor v3 service will be saved in `onion_v3_private_key`.
 7+The private key for the Tor v3 service will be saved in a file named `onion_v3_private_key`.
 8 To use the deprecated Tor v2 service (not recommended), then `onion_private_key`
 9 can be copied over `onion_v3_private_key`, e.g. `cp -f onion_private_key
10 onion_v3_private_key`. (#19954)

Ongoing review, minor points so far.

I do have a concern with a BIP about future extensions of the network IDs table.

How a BIP-155-with-extended-table node will learn the set of supported network IDs by a BIP-155-with-original-table peer and thus avoid to waste bandwidth on non-understood by receiver addresses ?

I think this is independent of the question if a connection is established or not to the known network. Maybe sendaddrv2 should have a version bit, this would avoid introducing sendaddrvX at each table update, as far as yet-to-added network address are under maximum v2 length.

ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5

Two small follow-ups (not blockers here):

• Mention BIP155 support in doc/bips.md
• Don’t relay I2P/CJDNS in RelayAddress (until there is a sizable network that actually supports it)

re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 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.

 0    "bytessent_per_msg": {
 1      "addrv2": 132,
 2      "feefilter": 32,
 3      "getaddr": 24,
 4      "getdata": 3918,
 5      "getheaders": 1053,
 6      "headers": 25,
 7      "inv": 30017,
 8      "ping": 96,
 9      "pong": 96,
10      "sendaddrv2": 24,
11      "sendcmpct": 66,
12      "sendheaders": 24,
13      "verack": 24,
14      "version": 127,
15      "wtxidrelay": 24
16    },
17    "bytesrecv_per_msg": {
18      "addrv2": 17444,
19      "feefilter": 32,
20      "getheaders": 1053,
21      "headers": 106,
22      "inv": 16996,
23      "notfound": 61,
24      "ping": 96,
25      "pong": 96,
26      "sendaddrv2": 24,
27      "sendcmpct": 66,
28      "sendheaders": 24,
29      "tx": 39740,
30      "verack": 24,
31      "version": 127,
32      "wtxidrelay": 24
33    },