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.
482 | + // TORv3 (ED25519-V3) one. 483 | + static const char torv2_key_prefix[] = "RSA1024:"; 484 | + if (private_key.substr(0, strlen(torv2_key_prefix)) == torv2_key_prefix) { 485 | + auto obsolete_key_file = GetPrivateKeyFile().string() + "_obsolete_torv2"; 486 | + LogPrint(BCLog::TOR, "tor: Detected an obsolete TORv2 private key, will move it " 487 | + "to %s and will create new TORv3 one\n", obsolete_key_file);
Linter fails on missing newline here.
$ test/lint/lint-logs.sh
All calls to LogPrintf() and LogPrint() should be terminated with \n
Deleted this code altogether.
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.
75 | @@ -76,6 +76,19 @@ extern const char* VERACK; 76 | * network. 77 | */ 78 | extern const char* ADDR; 79 | +/** 80 | + * The addrv2 message relays connection information for peers on the network just 81 | + * like the addr message, but is extended to allow gossipping of longer node
nit: gossiping
Fixed.
84 | +extern const char *ADDRv2; 85 | +/** 86 | + * The sendaddrv2 message signals support for receiving ADDRv2 messages (BIP155). 87 | + * It also implies that its sender can encode as ADDRv2 and would send ADDRv2 88 | + * instead of ADDR to a peer who has indicated ADDRv2 support (has sent 89 | + * SENDADDRv2 himself).
s/who has indicated ADDRv2 support (has sent SENDADDRV2 himself)/that has signaled ADDRv2 support by sending SENDADDRv2./
Changed.
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.
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.
Can we also add the option to delete
onion_private_keyandonion_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.
56 | @@ -57,6 +57,7 @@ Subdirectory | File(s) | Description 57 | `./` | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used 58 | `./` | `mempool.dat` | Dump of the mempool's transactions 59 | `./` | `onion_private_key` | Cached Tor onion service private key for `-listenonion` option 60 | +`./` | `onion_private_key_obsolete_torv2` | Obsolete Tor onion service private key used to create TORv2 hidden services. This file is automatically created as a backup when new TORv3 private key is generated and saved into `onion_private_key`. Bitcoin Core does not need it and will never attempt to read it. Remove if you do not need the old TORv2 hidden service.
I'd slightly prefer creating a new file for v3 instead of moving the old one. This makes it easier to go back and forth to test this (as well as downgrade, when this becomes part of a release).
It also makes for amore straightforward path to both v2 and v3 hidden services (as @jonatack suggests).
Indeed. Done!
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.
utACK 9c9ff0a8c40421b43922a6b211f99b6aaf94a798. 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!
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
532 | @@ -533,7 +533,7 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
533 |
534 | // Finally - now create the service
535 | if (private_key.empty()) // No private key, generate one
nit: add {
Done
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.
55 | @@ -56,7 +56,8 @@ Subdirectory | File(s) | Description 56 | `./` | `fee_estimates.dat` | Stores statistics used to estimate minimum transaction fees and priorities required for confirmation 57 | `./` | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used 58 | `./` | `mempool.dat` | Dump of the mempool's transactions 59 | -`./` | `onion_private_key` | Cached Tor onion service private key for `-listenonion` option 60 | +`./` | `onion_private_key` | Obsolete: cached Tor onion service private key for `-listenonion` option. Was used for TORv2 services. Bitcoin Core does not need this file and ignores it starting from version 0.21.
Maybe move this line to ## Legacy subdirectories and files, if we're no longer going to spin up a v2 service.
1550aa8 nits, s/TORv2/Tor v2/ and s/does not need/no longer needs/ (though if you move it to the legacy section, use the "replaced by" format instead)
Done.
Done - "replaced by ..."
52 | @@ -53,6 +53,13 @@ void static inline WriteLE64(unsigned char* ptr, uint64_t x) 53 | memcpy(ptr, (char*)&v, 8); 54 | } 55 | 56 | +uint16_t static inline ReadBE16(const unsigned char* ptr) 57 | +{ 58 | + uint16_t x; 59 | + memcpy((char*)&x, ptr, 2); 60 | + return be16toh(x);
Good, looks ilke we already have an implementation of be16toh in src/compat/endian.h, so it doesn't need to be added.
78 | + now created as a TORv3 service instead of TORv2. The private key that was used 79 | + for TORv2 (if any) will be left untouched in `onion_private_key` and can be 80 | + removed if not needed. Bitcoin Core will not attempt to read it anymore. The 81 | + private key for the TORv3 service will be saved in `onion_private_key_torv3`. 82 | + If using the old TORv2 service is desirable (not recommended), then 83 | + `onion_private_key` can be copied over `onion_private_key_torv3`.
1550aa8 suggestions
s/TOR/Tor / throughout, e.g. TORv2 -> Tor v2 -- the Tor project even tweeted recently to the community to please write "Tor" rather than "TOR"
s/will be now/will now be/
s/Bitcoin Core will not attempt to read it anymore/Bitcoin Core will no longer attempt to read it/ (or "will henceforth ignore it")
s/If using the old TORv2 service is desirable/To use the deprecated Tor v2 service/ (prefer active voice to passive voice)
maybe mention that these cookie auth files are located in the .bitcoin directory
"can be copied over" is unclear (to me)
Done.
cp -f onion_private_key onion_private_key_torv3 is clearer? :)
LGTM
328 | @@ -318,8 +329,15 @@ friend class CAddrManTest; 329 | { 330 | LOCK(cs); 331 | 332 | - unsigned char nVersion = 2; 333 | - s << nVersion; 334 | + const auto stream_version_orig = s.GetVersion(); 335 | + 336 | + if (m_format >= Format::V3) {
896a5af style nit, Format::V3 having the meaning of "want addrv2" is somewhat confusing, maybe create a boolean helper function IsAddrV2Format() to use both here and line 405 below for the code to be self-explanatory, or use more descriptive enum Format names, not sure here.
Idem for m_format > Format::V0, m_format > Format::V1,and m_format >= Format::V2
AddrMan already had versions on its on-disk database with numbers. So I left the convention as is and just added new version "3" since the last one used was "2". This has nothing to do with "addrv2" or "torv3" :)
IsAddrV2Format() would spoil the strict ordering of the AddrMan on-disk versions.
Yes, I understand. Helper functions that replace the various m_format >= Format::V<n> may possibly be nice, but opinions can vary and it isn't a blocker.
2605 | + 2606 | + LOCK(cs_main); 2607 | + Misbehaving(pfrom.GetId(), 20, 2608 | + strprintf( 2609 | + "Supplied us with a message that contains an unparsable address: %s", 2610 | + e.what()));
99047b077 is there test coverage for this new try/catch/Misbehaving/logging unparseable address code? has there been discussion on how much to add to the misbehavior score for this?
Yes! (since the last push with updated tests)
has there been discussion on how much to add to the misbehavior score for this?
I don't think so. It is currently punishing the peer as much as if he sent us an addr message with >1000 addresses. I do not insist on that.
Nice work on the tests!
2634 | @@ -2612,6 +2635,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat 2635 | if (!MayHaveUsefulAddressDB(addr.nServices) && !HasAllDesirableServiceFlags(addr.nServices)) 2636 | continue; 2637 | 2638 | + // Skip invalid addresses, maybe a new BIP155 network id from the future. 2639 | + if (!addr.IsValid()) { 2640 | + continue;
99047b0 covered by a test?
Yes! (since the last push with updated tests)
75 | @@ -76,6 +76,18 @@ extern const char* VERACK; 76 | * network. 77 | */ 78 | extern const char* ADDR; 79 | +/** 80 | + * The addrv2 message relays connection information for peers on the network just 81 | + * like the addr message, but is extended to allow gossiping of longer node
99047b07 nit, maybe "gossiping of longer node addresses up to 512 bytes"
I deliberately leave numbers outside of comments - as too much unnecessary details and also could go out of sync if the code is updated and the comment not.
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.
2664 | @@ -2637,6 +2665,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat 2665 | return; 2666 | } 2667 | 2668 | + if (msg_type == NetMsgType::SENDADDRv2) { 2669 | + pfrom.m_wants_addrv2 = true;
Had a look at if this needs a lock; it doesn't seem so.
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.
Address review suggestions.
Address review suggestion.
Ready to move this PR out of Draft status? 😀 Edit: snap!
Concept ACK.
337 | + // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress 338 | + // serialize methods produce an address in v2 format. 339 | + s.SetVersion(stream_version_orig | ADDRV2_FORMAT); 340 | + } 341 | + 342 | + s << (uint8_t)m_format;
e025121c1ca4b86296c6b36386786522bd79a6e1
s << static_cast<uint8_t>(m_format);
Done
Per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast and C++ Programming Language 4th Edition, section 11.5, it might be good to update the next line as well if you retouch.
s << static_cast<uint8_t>(m_format);
- s << ((unsigned char)32);
+ s << static_cast<unsigned char>(32);
That line is unmodified by the patch. I try to avoid sneaking unrelated improvements to surrounding code in order to ease reviewers and future history observers.
That's fair.
401 | + 402 | + const auto stream_version_orig = s.GetVersion(); 403 | + 404 | + uint8_t format_tmp; 405 | + s >> format_tmp; 406 | + m_format = (Format)format_tmp;
e025121c1ca4b86296c6b36386786522bd79a6e1
m_format = static_cast<Format>(format_tmp);
Done
Approach ACK ff76c3444c574b14a2c62999b82cba812816884b.
nit: It seems more readable if all comparisons with m_format will use the same sign, say >=, instead of mixing >= and >.
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.
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()).
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).
<details> <summary>The change to not relay I2P and CJDNS addresses is easy, click here.</summary>
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -2643,14 +2643,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
pfrom.AddAddressKnown(addr);
if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) {
// Do not process banned/discouraged addresses beyond remembering we received them
continue;
}
bool fReachable = IsReachable(addr);
- if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
- {
+ if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 &&
+ addr.IsRoutable() && !addr.IsI2P() && !addr.IsCJDNS()) {
// Relay to a limited number of other nodes
RelayAddress(addr, fReachable, m_connman);
}
// Do not store addresses outside our network
if (fReachable)
vAddrOk.push_back(addr);
</details>
What are your thoughts?
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.
97 | @@ -98,6 +98,7 @@ Path | Description | Repository notes 98 | `blkindex.dat` | Blockchain index BDB database; replaced by {`chainstate/`, `blocks/index/`, `blocks/revNNNNN.dat`<sup>[\[2\]](#note2)</sup>} in 0.8.0 | [PR #1677](https://github.com/bitcoin/bitcoin/pull/1677) 99 | `blk000?.dat` | Block data (custom format, 2 GiB per file); replaced by `blocks/blkNNNNN.dat`<sup>[\[2\]](#note2)</sup> in 0.8.0 | [PR #1677](https://github.com/bitcoin/bitcoin/pull/1677) 100 | `addr.dat` | Peer IP address BDB database; replaced by `peers.dat` in [0.7.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.7.0.md) | [PR #1198](https://github.com/bitcoin/bitcoin/pull/1198), [`928d3a01`](https://github.com/bitcoin/bitcoin/commit/928d3a011cc66c7f907c4d053f674ea77dc611cc) 101 | +`onion_private_key` | Cached Tor onion service private key for `-listenonion` option. Was used for Tor v2 services; replaced by `onion_private_key_torv3` in [0.21.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md) [PR #19954](https://github.com/bitcoin/bitcoin/pull/19954)
Missing column separator |?
`onion_private_key` | Cached Tor onion service private key for `-listenonion` option. Was used for Tor v2 services; replaced by `onion_private_key_torv3` in [0.21.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md) | [PR [#19954](/bitcoin-bitcoin/19954/)](https://github.com/bitcoin/bitcoin/pull/19954)
Fixed
79 | + for Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` 80 | + and can be removed if not needed. Bitcoin Core will no longer attempt to read it. 81 | + The private key for the Tor v3 service will be saved in `onion_private_key_torv3`. 82 | + To use the deprecated Tor v2 service (not recommended), then `onion_private_key` 83 | + can be copied over `onion_private_key_torv3` 84 | + (`cp -f onion_private_key onion_private_key_torv3`).
(code) markup inside parentheses; maybe easier to read as:
, e.g. `cp -f onion_private_key onion_private_key_torv3`.
Done
9 | @@ -10,7 +10,7 @@ 10 | CAddress, 11 | NODE_NETWORK, 12 | NODE_WITNESS, 13 | - msg_addr, 14 | + msg_addrv2,
Should we test for both msg_addr and msg_addrv2?
Right, p2p_addr_relay.py is the only test that exercises msg_addr and if we flip it to test msg_addrv2 then msg_addr will be untested.
I tried to combine both in one test, but it became too convoluted and so I copied the new test to p2p_addrv2_relay.py and restored the original p2p_addr_relay.py.
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.
bitcoind compiled against masterbitcoind compiled against this PRpeers.datThe 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).
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.
2413 | @@ -2414,6 +2414,9 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat 2414 | 2415 | m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::VERACK)); 2416 | 2417 | + // Signal ADDRv2 support (BIP155). 2418 | + m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::SENDADDRv2));
76afce803bb5309a69517df5eac68529b7af2d29
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::SENDADDRv2));
Done
531 | @@ -532,8 +532,9 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply& 532 | } 533 | 534 | // Finally - now create the service 535 | - if (private_key.empty()) // No private key, generate one 536 | - private_key = "NEW:RSA1024"; // Explicitly request RSA1024 - see issue #9214 537 | + if (private_key.empty()) { // No private key, generate one 538 | + private_key = "NEW:ED25519-V3"; // Explicitly request key type - see issue #9214 539 | + }
Correct.
73 | @@ -74,6 +74,15 @@ P2P and network changes 74 | node using P2P relay. This version reduces the initial broadcast guarantees 75 | for wallet transactions submitted via P2P to a node running the wallet. (#18038) 76 | 77 | +- The Tor hidden service that is automatically created (`-listenonion`) will now
Changed
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.
@vasild in the latest push, the p2p_addr_relay.py file perms were changed from 755 to 644; could you please revert that?
<details><summary>a few suggestions for <code>p2p_addrv2_relay.py</code></summary><p>
diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py
index 19cfd0eadb..ca593148cc 100755
--- a/test/functional/p2p_addrv2_relay.py
+++ b/test/functional/p2p_addrv2_relay.py
@@ -6,18 +6,17 @@
@@ -6,18 +6,17 @@
Test addrv2 relay
"""
+import time
+
from test_framework.messages import (
CAddress,
+ msg_addrv2,
NODE_NETWORK,
NODE_WITNESS,
- msg_addrv2,
)
from test_framework.p2p import P2PInterface
from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import (
- assert_equal,
-)
-import time
+from test_framework.util import assert_equal
ADDRS = []
for i in range(10):
@@ -48,11 +47,11 @@ class AddrReceiver(P2PInterface):
class AddrTest(BitcoinTestFramework):
def set_test_params(self):
- self.setup_clean_chain = False
+ self.setup_clean_chain = True
self.num_nodes = 1
def run_test(self):
- self.log.info('Create connection that sends addr messages')
+ self.log.info('Create connection that sends addrv2 messages')
addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
msg = msg_addrv2()
@@ -61,7 +60,7 @@ class AddrTest(BitcoinTestFramework):
with self.nodes[0].assert_debug_log(['addr message size = 1010']):
addr_source.send_and_ping(msg)
- self.log.info('Check that addr message content is relayed and added to addrman')
+ self.log.info('Check that addrv2 message content is relayed and added to addrman')
addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
msg.addrs = ADDRS
with self.nodes[0].assert_debug_log([
</p></details>
718 | @@ -718,7 +719,7 @@ void TorController::Reconnect() 719 | 720 | fs::path TorController::GetPrivateKeyFile() 721 | { 722 | - return GetDataDir() / "onion_private_key"; 723 | + return GetDataDir() / "onion_private_key_v3";
return GetDataDir() / "onion_v3_private_key";
Done
26 | +from test_framework.util import ( 27 | + assert_equal, 28 | + hex_str_to_bytes, 29 | +) 30 | +import struct 31 | +import time
nit: IIRC, per PEP8, OS-level and stdlib imports go at the top.
Done
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.
2634 | @@ -2610,6 +2635,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat 2635 | if (!MayHaveUsefulAddressDB(addr.nServices) && !HasAllDesirableServiceFlags(addr.nServices)) 2636 | continue; 2637 | 2638 | + // Skip invalid addresses, maybe a new BIP155 network id from the future. 2639 | + if (!addr.IsValid()) { 2640 | + continue;
Should skipping invalid BIP155 addresses be applied only for addrv2 messages?
if (!addr.IsValid() && msg_type == NetMsgType::ADDRv2) {
continue;
}
I realized that this code is not necessary so I dropped it altogether - an invalid address is not routable and is not reachable, so we are not going to do anything with it.
263 | @@ -264,6 +264,17 @@ friend class CAddrManTest; 264 | void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); 265 | 266 | public: 267 | + //! Serialization versions. 268 | + enum class Format : uint8_t { 269 | + V0 = 0, //!< historic format, before commit e6b343d88
How about giving these a bit more descriptive names, like V0_HISTORICAL, V1_DETERMINISTIC, V2_ASMAP, V3_BIP155?
Renamed
334 | + const auto stream_version_orig = s.GetVersion(); 335 | + 336 | + if (m_format >= Format::V3) { 337 | + // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress 338 | + // serialize methods produce an address in v2 format. 339 | + s.SetVersion(stream_version_orig | ADDRV2_FORMAT);
A cleaner approach is using OverrideStream here (and elsewhere), which removes the need to undo the version change, or having a mutable version in CHashWriter.
Addressed review suggestions.
389 | @@ -371,13 +390,25 @@ friend class CAddrManTest;
390 | }
391 |
392 | template<typename Stream>
pico-nit:
template <typename Stream>
Done
263 | @@ -264,6 +264,17 @@ friend class CAddrManTest; 264 | void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); 265 | 266 | public: 267 | + //! Serialization versions. 268 | + enum class Format : uint8_t { 269 | + V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 270 | + V1_DETERMINISTIC = 1, //!< for pre-asmap files 271 | + V2_ASMAP = 2, //!< for files including asmap version 272 | + V3_BIP155 = 3, //!< same as V2 plus addresses are in BIP155 format
pico-nit, as clang-format suggests, and s/V2/V2_ASMAP/ in comment:
V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88
V1_DETERMINISTIC = 1, //!< for pre-asmap files
V2_ASMAP = 2, //!< for files including asmap version
V3_BIP155 = 3, //!< same as V2_ASMAP plus addresses are in BIP155 format
Done
101 | @@ -102,13 +102,14 @@ class CHashWriter 102 | CSHA256 ctx; 103 | 104 | const int nType; 105 | - const int nVersion; 106 | + int nVersion; 107 | public:
pico-nit:
int nVersion;
public:
Done
re-ACK bbbc8e4c657e8ea81edb6aca2b1870cd3b7459ad.
I'd like that s and s_ objects of the Stream class were named more descriptive, but I have no particular suggestion.
Addressed review suggestions.
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 !
109 |
110 | CHashWriter(int nTypeIn, int nVersionIn) : nType(nTypeIn), nVersion(nVersionIn) {}
111 |
112 | int GetType() const { return nType; }
113 | int GetVersion() const { return nVersion; }
114 | + void SetVersion(int n) { nVersion = n; }
03572e9 After git diff c212c7e 703db15 using OverrideStream, it looks like we can now revert these changes in hash.h and streams.h.
+++ b/src/hash.h
@@ -102,7 +102,7 @@ private:
const int nType;
- int nVersion;
+ const int nVersion;
@@ -110,7 +110,6 @@ public:
int GetVersion() const { return nVersion; }
- void SetVersion(int n) { nVersion = n; }
+++ b/src/streams.h
@@ -603,7 +603,7 @@ class CAutoFile
const int nType;
- int nVersion;
+ const int nVersion;
@@ -651,7 +651,6 @@ public:
int GetVersion() const { return nVersion; }
- void SetVersion(int n) { nVersion = n; }
Good catch! Removed.
398 | 399 | Clear(); 400 | - unsigned char nVersion; 401 | - s >> nVersion; 402 | + 403 | + uint8_t format_tmp;
You could use s_ >> Using<CustomUintFormatter<1>>(m_format) here (and possibly also in Serialize, for consistency, though it isn't really a gain there).
Used CustomUintFormatter here, but in Serialize it did not compile:
src/serialize.h:504:15: error: invalid operands to binary expression ('CAddrMan::Format' and 'int')
if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
~ ^ ~
so I left the static_cast there.
650 | @@ -650,6 +651,7 @@ class CAutoFile 651 | // 652 | int GetType() const { return nType; } 653 | int GetVersion() const { return nVersion; } 654 | + void SetVersion(int n) { nVersion = n; }
This should be unnecessary as well.
Removed
271 | + V2_ASMAP = 2, //!< for files including asmap version 272 | + V3_BIP155 = 3, //!< same as V2_ASMAP plus addresses are in BIP155 format 273 | + }; 274 | + 275 | + //! Serialization / unserialization format. 276 | + Format m_format = Format::V3_BIP155;
It's a bit ugly to make the choice of how addrman is serialized on disk dependent on external code properly setting this m_format variable.
I think it's fine to just always serialize in the latest format. In that case the format variable can be just be local to the deserialization code.
Alternatively, if you want to focus on retaining backward compatibility as long as no BIP155 has been used, you could make the format decision at serialization depend on whether or not IsAddrV1Compatible() is true for all addresses in it.
I agree "it's fine to just always serialize in the latest format", changed the code accordingly - now the format variable is a local one in Unserialize() while Serialize() just serializes the constant Format::V3_BIP155.
This is also closer to the behavior before this PR (it always serialized in version 2, the latest at the time).
Does this mean that peers.dat becomes unreadable if a user choose to downgrade his node at some point in the future?
Testing this branch:
One or two restarts (always the first, sometimes a second one as well) after cp onion_private_key onion_v3_private_key to run a v2 service
ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error
Invalid or missing peers.dat; recreating
One, and sometimes two, restarts after rm onion_v3_private_key to return to v3
ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error
Invalid or missing peers.dat; recreating
Edit: verified that these messages are new in the debug log and were absent from testing of previous versions.
Does this mean that peers.dat becomes unreadable if a user choose to downgrade his node at some point in the future?
Yes. We can't have bitcoind-before-this-PR to read e.g. torv3 addresses from peers.dat.
Does this require mention in release notes?
4136 | @@ -4104,14 +4137,14 @@ bool PeerManager::SendMessages(CNode* pto) 4137 | // receiver rejects addr messages larger than MAX_ADDR_TO_SEND 4138 | if (vAddr.size() >= MAX_ADDR_TO_SEND) 4139 | { 4140 | - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr)); 4141 | + m_connman.PushMessage(pto, msgMaker.Make(msg_type, vAddr));
If you use the CNetMsgMaker:: Make(int nFlags, std::string msg_type, Args&&... args) variant here, you can control the serialization flags, without needing the special casing for ADDRV2 inside Make.
A good interface is hard or impossible to misuse. If we add ADDRV2_FORMAT to nFlags inside Make() that makes it impossible for the caller to misuse it like:
msgMaker.Make(0, NetMsgType::ADDRv2, vAddr); // huh!?
I think it is not good to impose on the caller: "whenever you pass NetMsgType::ADDRv2 for msg_type you must also pass ADDRV2_FORMAT in nFlags".
@vasild I agree with the notion, but this is just the wrong layer to deal with that. The serialization framework (which I'd consider netmessagemaker to be a thin wrapper around) shouldn't have knowledge about the data being serialized, or even about which message types exist.
I think it is not good to impose on the caller: "whenever you pass
NetMsgType::ADDRv2formsg_typeyou must also passADDRV2_FORMATinnFlags".
I think that's the wrong way to think about it. CNetMsgMaker should do as it's told: serializing the object passed with the specified flags, and nothing else. Knowledge about what message types exist, and how data is serialized in them belongs in net_processing.
That is also a valid point. Changed.
Addressed review suggestions and extended some tests.
Filtered code coverage report (files not modified by this PR are omitted and not modified lines in files that are otherwise modified are dimmed).
2601 | std::vector<CAddress> vAddr; 2602 | - vRecv >> vAddr; 2603 | + 2604 | + try { 2605 | + s >> vAddr; 2606 | + } catch (const std::exception& e) {
This block shouldn't be needed, PeerManager::ProcessMessages will catch exceptions thrown during message processing, and skip to the next message. That doesn't trigger any Misbehaving, but whether deserialization failure should have a misbehavior score (and the future of misbehavior scores at all...) seems like an orthogonal issue that needn't be addressed here.
I agree, removed the try/catch.
2618 | if (vAddr.size() > MAX_ADDR_TO_SEND)
2619 | {
2620 | - Misbehaving(pfrom.GetId(), 20, strprintf("addr message size = %u", vAddr.size()));
2621 | + Misbehaving(pfrom.GetId(), 20,
2622 | + strprintf("%s message size = %u",
2623 | + msg_type == NetMsgType::ADDR ? "addr" : "addrv2",
This can instead be:
Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
As msg_type is just const char*.
Changed. Btw msg_type is std::string (not const char*). strprintf() takes it either way.
75 | @@ -76,6 +76,18 @@ extern const char* VERACK; 76 | * network. 77 | */ 78 | extern const char* ADDR; 79 | +/** 80 | + * The addrv2 message relays connection information for peers on the network just 81 | + * like the addr message, but is extended to allow gossiping of longer node 82 | + * addresses (see BIP155). 83 | + */ 84 | +extern const char *ADDRv2;
Style nit: constants are all caps, so ADDRV2
Renamed
85 | +/** 86 | + * The sendaddrv2 message signals support for receiving ADDRv2 messages (BIP155). 87 | + * It also implies that its sender can encode as ADDRv2 and would send ADDRv2 88 | + * instead of ADDR to a peer that has signaled ADDRv2 support by sending SENDADDRv2. 89 | + */ 90 | +extern const char *SENDADDRv2;
Style nit: constants are all caps, so SENDADDRV2.
Renamed
Code review ACK modulo nits.
I tested:
externalip= with torv3 address, and saw it reported in logsaddrv2 reported in getpeerinfo output
Addressed review suggestions.
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.
Re-reviewing and testing. I wonder if we need to warn or explain to users about the "ERROR: DeserializeDB: Deserialize or I/O error" and "Invalid or missing peers.dat" messages in the debug log if they go back to a v2 service.
59 | @@ -60,6 +60,7 @@ class OverrideStream 60 | int GetVersion() const { return nVersion; } 61 | int GetType() const { return nType; } 62 | size_t size() const { return stream->size(); } 63 | + void ignore(size_t size) { return stream->ignore(size); }
0e05e14
void ignore(size_t size) const { return stream->ignore(size); }
ignore() modifies the state of the stream. It is not const in CDataStream and CAutoFile so this would not compile.
It does compile (I test if my suggestions compile before making them), though arguably const on a void function may not make sense. Consider this suggestion a question.
"void f() const makes the function itself const. This only really has meaning for member functions. Making a member function const means that it cannot call any non-const member functions, nor can it change any member variables. It also means that the function can be called via a const object of the class." http://www.cplusplus.com/forum/general/12087/ (reply in 2009)
I think it should not be const because it modifies the state of the stream. I guess it is not const for the same reason in the other stream classes.
79 | + for Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` 80 | + and can be removed if not needed. Bitcoin Core will no longer attempt to read it. 81 | + The private key for the Tor v3 service will be saved in `onion_v3_private_key`. 82 | + To use the deprecated Tor v2 service (not recommended), then `onion_private_key` 83 | + can be copied over `onion_v3_private_key`, e.g. 84 | + `cp -f onion_private_key onion_v3_private_key`.
This seems like a possible source of user error, but I'm not sure how best to resolve it...
Perhaps we could allow for -listenonion=v2 and -listenonion=v3? Somehow that doesn't seem the most satisfying to me either...
1f06a2901d1ee924b95f482e8693842ee07863de Perhaps inform users of the debug log messages, e.g. ERROR: DeserializeDB: Deserialize or I/O error and Invalid or missing peers.dat, that they may see if they use tor v2 henceforth, at least per my testing.
Personally, I think it would be ok to not support v2 addresses at all anymore. Given the short time they have left. But that there's a workaround (even if clunky) might help for some people.
Added a release notes mention.
490 | + case NET_MAX: // m_net is never and should not be set to NET_MAX 491 | + assert(false); 492 | + } 493 | + 494 | + return false; 495 | +}
93c8f0ea suggestion (like GetBIP155Network)
- }
-
- return false;
+ } // no default case, so the compiler can warn about missing cases
+ assert(false);
}
Done
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.
Code review ACK 1f06a2901d1ee924b95f482e8693842ee07863de
Tested re-ACK 1f06a2901d1ee924b95f482e8693842ee07863de modulo comments below. Started new v3 service by deleting onion_v3_private_key and after running previously on master, no errors, local addr new tor v3 address. Restarted after cp onion_private_key onion_v3_private_key, saw ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error and Invalid or missing peers.dat; recreating, local addr usual tor v2 address. Restarted after deleting onion_v3_private_key, no errors, local addr new tor v3 address. I see "addrv2" and "sendaddrv2" messages in getpeerinfo in both the "bytesrecv_per_msg" and "bytessent_per_msg" objects. Would be good to nail down those errors.
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:
13:39:37 < vasild> CAddress::unser: time=1601829922, services=134218813, addr=62.107.200.30:8333
13:39:56 < vasild> CAddress::unser: time=1600339981, services=134217741, addr=akinbo7tlegsnsxn.onion:8333
13:40:31 < vasild> jonatack: wumpus: so...
13:41:05 < vasild> in the network there are some peers with services=unusually high value
13:42:07 < sipa> that's 0x0800000d
13:42:14 < vasild> they get in via p2p gossip into addrman without causing problems
13:42:16 < vasild> sipa: right
13:43:18 < vasild> using addrv1
13:44:03 < vasild> then addrman saves it on disk just fine in V3_BIP155 format using READWRITE(COMPACTSIZE(services_tmp))
13:44:44 < vasild> however, on read back from disk it bricks because ReadCompactSize() contains this at the end:
13:44:53 < vasild> if (nSizeRet > (uint64_t)MAX_SIZE)
13:44:54 < vasild> throw std::ios_base::failure("ReadCompactSize(): size too large");
13:45:18 < vasild> and MAX_SIZE is quite low value 0x02000000
13:45:48 < vasild> I wondered before wtf is that limit but decided it is harmless
13:46:14 < sipa> vasild: ehhhh
13:46:27 < sipa> this means we need to bypass that limit when deserializing addrv2
13: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.datformat change and consequences in the release notes)
:+1:
This is needed when we want to encode an arbitrary number as CompactSize
like node service flags, which is a bitmask and could be bigger than the
usual size of an object.
Addressed review suggestions plus
peers.dat format changeaddrv2 encoding (the old addr encoding uses fixed 8-byte representation)peers.dat23 | @@ -24,7 +24,11 @@ 24 | #include <prevector.h> 25 | #include <span.h> 26 | 27 | -static const unsigned int MAX_SIZE = 0x02000000; 28 | +/** 29 | + * The maximum size of a serialized object in bytes or number of elements 30 | + * (for eg vectors) when the size is encoded as CompactSize.
1d3ec2a nit
* (e.g. for vectors) when the size is encoded as CompactSize.
The dot terminates the doxygen title.
TIL, thanks. Apparently, per https://www.doxygen.nl/manual/docblocks.html#docexamples: "To enable this behavior you should set JAVADOC_AUTOBRIEF to YES in the configuration file. If you enable this option and want to put a dot in the middle of a sentence without ending it, you should put a backslash and a space after it. Here is an example: /** Brief description (e.g.\ using only a few words). Details follow. */"
Indeed, doesn't seem worth it.
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.
Migration from deprecated Tor v2 to Tor v3
-----------------------
The Tor onion service that is automatically created (`-listenonion`) will now be
created as a Tor v3 service instead of Tor v2. The private key that was used for
Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` and
can be removed if not needed. Bitcoin Core will no longer attempt to read it.
The private key for the Tor v3 service will be saved in `onion_v3_private_key`.
To use the deprecated Tor v2 service (not recommended), the `onion_private_key` file
can be copied over `onion_v3_private_key`, e.g. `cp -f onion_private_key
onion_v3_private_key`. (#19954)
Peers.dat file format change
-----------------------
The node's known peers are persisted to disk in a file called `peers.dat`. Starting
with this v0.21.0 release, the `peers.dat` file format is changed in a backwards-incompatible
way to accommodate the storage of large BIP155 addresses, such as Tor v3 addresses.
Pre-0.21 Bitcoin Core software will not be able to read this new `peers.dat` format. In the
event of downgrading to pre-0.21 after running 0.21, the debug log may print an error that
deserialization has failed and a message that peers.dat is invalid or missing and is being
recreated, and bitcoind will continue normal operation with a new `peers.dat` file. (#19954)
398 | + 399 | + static constexpr Format maximum_supported_format = Format::V3_BIP155; 400 | + if (format > maximum_supported_format) { 401 | + throw std::ios_base::failure(strprintf( 402 | + "Unsupported format of addrman database: %u. Maximum supported is %u. " 403 | + "Continuing operation without using the saved list of peers.",
6fce3ce maybe
"Continuing operation by creating a new peers.dat file in the maximum supported format",
currently (for me on debian) it would print
ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 4. Maximum supported is 3. Continuing operation without using the saved list of peers.: iostream error
"Continuing operation by creating a new peers.dat" is, strictly speaking, not true because the new peers.dat will not be created at the time this message is printed. It will be created later during shutdown or during one of the periodic dumps to disk.
I left it as is.
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.
-The Tor onion service that is automatically created (`-listenonion`) will now be
+The Tor onion service that is automatically created by setting the `-listenonion` configuration parameter will now be
created as a Tor v3 service instead of Tor v2. The private key that was used for
-Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` and
+Tor v2 (if any) will be left untouched in the `onion_private_key` file in the data directory (see `-datadir`) and
can be removed if not needed. Bitcoin Core will no longer attempt to read it.
-The private key for the Tor v3 service will be saved in `onion_v3_private_key`.
+The private key for the Tor v3 service will be saved in a file named `onion_v3_private_key`.
To use the deprecated Tor v2 service (not recommended), then `onion_private_key`
can be copied over `onion_v3_private_key`, e.g. `cp -f onion_private_key
onion_v3_private_key`. (#19954)
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>
TORv2 is deprecated [1], thus whenever we create the hidden service
ourselves create a TORv3 one instead.
[1] https://blog.torproject.org/v2-deprecation-timeline
Did some rewording of the release notes, mostly as suggested.
Tested that spinning up a v3 onion and connecting to it still works as of dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5. The PR title should probably mention that we start using sendaddrv2 here, with TorV3 just being the cherry on top.
404 | + static_cast<uint8_t>(format), 405 | + static_cast<uint8_t>(maximum_supported_format))); 406 | + } 407 | + 408 | + int stream_version = s_.GetVersion(); 409 | + if (format >= Format::V3_BIP155) {
As maximum_supported_format is currently V3_BIP155, the format should never be superior otherwise it would have throw L397. Did this is an attempt to future-proof bump of the serialization format ? A future serialization format may override ADDRV2_FORMAT.
Yes, this code will now only be reached if format == V3_BIP155. In the future format may be higher. By using >= we assume that a future format V4_FOO will also use addrv2 encoding in which case this code will not have to be changed. If a future V4_FOO does not use addrv2, then this line will have to be changed to ==.
Notice however that if we use == now and a future V4_FOO still uses addrv2, then this line will have to be changed from == to >=.
867 | @@ -868,6 +868,11 @@ class CNode 868 | bool m_legacyWhitelisted{false}; 869 | bool fClient{false}; // set by version message 870 | bool m_limited_node{false}; //after BIP159, set by version message 871 | + /** 872 | + * Whether the peer has signaled support for receiving ADDRv2 (BIP155) 873 | + * messages, implying a preference to receive ADDRv2 instead of ADDR ones.
I think the BIP could be clearer on addr/addrv2 processing responsibility once a sendaddrv2 is received.
Once a sendaddrv2 is received from a peer, a node :
addrv2 messages to gossip addressesaddrv2messagesaddr messagesI guess it's better to be liberal with acceptance of legacy addr messages from addrv2-signaling peers (AFAICT what this PR does) to avoid loosing a potentially good block/tx-relay peer, even if it's a clear hint of a buggy BIP155 implementation.
Do you mean this?
Once
Breceivessendaddrv2fromA:
BMUST useaddrv2messages to gossip addresses toABMUST acceptaddrv2messages fromABMAY acceptaddrmessages fromA
The current PR accepts both addr and addrv2 no matter whether sendaddrv2 has been sent or received.
Do you mean this?
Yes, I think the BIP should state this clearly to hint other implementations. The current PR code is correct.
1119 | @@ -1115,11 +1120,16 @@ class CNode 1120 | 1121 | void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand) 1122 | { 1123 | + // Whether the peer supports the address in `_addr`. For example, 1124 | + // nodes that do not implement BIP155 cannot receive Tor v3 addresses 1125 | + // because they require ADDRv2 (BIP155) encoding. 1126 | + const bool addr_format_supported = m_wants_addrv2 || _addr.IsAddrV1Compatible();
I think this check could be part of the net_processing layer, thus saving a function call in case of not-supported address as nothing is done otherwise. Further, it avoids having m_wants_addrv2 in CNode which may contradict @jnewbery 's #19398, especially this PoC cleaning branch : https://github.com/jnewbery/bitcoin/commit/6fb753520787e13b5fdcbf835be781506537841b
That said, gathering the check here likely avoids duplicating checks in net_processing, and it can be clean-up later if it considered as worthy.
PushAddress() is called from 5 places. We don't want to duplicate the same code in 5 places.
Well another alternative is a new method IsAddrUnderstood on CNode making the call to PushAddress() conditional on it results. It can be done later, not a real concern.
PushAddress() and all of the addr relay data will eventually move into net processing. See the commits in https://github.com/jnewbery/bitcoin/commits/2020-06-cs-main-split. That work obviously shouldn't hold up new features like BIP 155 and I'll continue to rebase those commits on master as things change. #19829 and #19910 are the next PRs to review if you'd like to help.
2592 | + if (msg_type == NetMsgType::ADDR || msg_type == NetMsgType::ADDRV2) { 2593 | + int stream_version = vRecv.GetVersion(); 2594 | + if (msg_type == NetMsgType::ADDRV2) { 2595 | + // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress 2596 | + // unserialize methods know that an address in v2 format is coming. 2597 | + stream_version |= ADDRV2_FORMAT;
It might be interesting to underscore in comment that an address discovered from a BIP155 upgraded peer will be relayed further (RelayAddress) to non-upgraded ones through legacy addr message if the contained address is understood.
I think it should even be part of the BIP to make a best-effort of BIP155 implementations filtering and relaying forward to non-upgraded peers addresses initially discovered through addrv2. What you want to avoid is a node failing to relay a whole ADDRV2 blob to non-upgraded peers, without extracting the peer-understood addresses.
addr and addrv2 are just a transport encodings. Once received, the address is not in any particular encoding, it is a CAddress object in the memory of the node, not in addr nor in addrv2 encoding.
So, when we further relay a CAddress object we don't care in what encoding we received it.
So, when we further relay a CAddress object we don't care in what encoding we received it.
Right, it's a just a matter of stating this in the BIP to avoid other implementations not propagating addresses due to receiving transport encoding A but forgetting to relay the decoded addr stream to peers speaking transport encoding B.
I don't think we should aim to specify relay behavior in BIPs. There is no requirement that all software behaves the same, and best practices around it are constant changing in response to attacks and observed network conditions.
The BIP specifies what means of communicating addresses are available. What that gets used for is local node policy.
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.
re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed.
ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5
Two small follow-ups (not blockers here):
doc/bips.mdRelayAddress (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.
"bytessent_per_msg": {
"addrv2": 132,
"feefilter": 32,
"getaddr": 24,
"getdata": 3918,
"getheaders": 1053,
"headers": 25,
"inv": 30017,
"ping": 96,
"pong": 96,
"sendaddrv2": 24,
"sendcmpct": 66,
"sendheaders": 24,
"verack": 24,
"version": 127,
"wtxidrelay": 24
},
"bytesrecv_per_msg": {
"addrv2": 17444,
"feefilter": 32,
"getheaders": 1053,
"headers": 106,
"inv": 16996,
"notfound": 61,
"ping": 96,
"pong": 96,
"sendaddrv2": 24,
"sendcmpct": 66,
"sendheaders": 24,
"tx": 39740,
"verack": 24,
"version": 127,
"wtxidrelay": 24
},
Code Review ACK dcf0cb4
460 | + CService(CNetAddr(in6addr_loopback), 0x00f1 /* port */), 461 | + NODE_NETWORK, 462 | + 0x83766279U /* Tue Nov 22 11:22:33 UTC 2039 */ 463 | + ), 464 | + CAddress( 465 | + CService(CNetAddr(in6addr_loopback), 0xf1f2 /* port */),
+ 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'
This breaks the guix/gitian build. cc @fanquake
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
<details> <summary>possible workaround symbol-check.py in6addr_loopback</summary>
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.
Congratulations to everybody involved :tada:
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!
64 | +format of this file has been changed in a backwards-incompatible way in order to 65 | +accommodate the storage of Tor v3 and other BIP155 addresses. This means that if 66 | +the file is modified by 0.21.0 or newer then older versions will not be able to 67 | +read it. Those old versions, in the event of a downgrade, will log an error 68 | +message that deserialization has failed and will continue normal operation 69 | +as if the file was missing, creating a new empty one. (#19954)
"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.
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);
}
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.
See if this will make sense to you: https://github.com/bitcoin/bitcoin/pull/20284
Milestone
0.21.0