(chopped off from #19031 to ease review)
Add an optional support to serialize/unserialize CNetAddr in ADDRv2 format (BIP155). The new serialization is engaged by ORing a flag into the stream version.
So far this is only used in tests to ensure the new code works as expected.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Concept ACK. First read-through looks promising and happy to see the additional unit test coverage.
Edit: noting here for myself that local debug build is clean and unit tests are green.
Would it make sense to benchmark how fast addr2 messages are deserialized? Maybe we should even benchmark every message type?
0diff --git i/src/bench/addrman.cpp w/src/bench/addrman.cpp
1index ebdad5a4b..db6b1c9fa 100644
2--- i/src/bench/addrman.cpp
3+++ w/src/bench/addrman.cpp
4@@ -1,20 +1,24 @@
5 // Copyright (c) 2020-2020 The Bitcoin Core developers
6 // Distributed under the MIT software license, see the accompanying
7 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
8
9 #include <addrman.h>
10 #include <bench/bench.h>
11+#include <netaddress.h>
12 #include <random.h>
13+#include <serialize.h>
14+#include <streams.h>
15 #include <util/time.h>
16+#include <version.h>
17
18 #include <vector>
19
20 /* A "source" is a source address from which we have received a bunch of other addresses. */
21
22-static constexpr size_t NUM_SOURCES = 64;
23+static constexpr size_t NUM_SOURCES = 1000;
24 static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
25
26 static std::vector<CAddress> g_sources;
27 static std::vector<std::vector<CAddress>> g_addresses;
28
29 static void CreateAddresses()
30@@ -135,6 +139,36 @@ static void AddrManGood(benchmark::Bench& bench)
31 }
32
33 BENCHMARK(AddrManAdd);
34 BENCHMARK(AddrManSelect);
35 BENCHMARK(AddrManGetAddr);
36 BENCHMARK(AddrManGood);
37+
38+static void AddressUnser(benchmark::Bench& bench, int add_to_version)
39+{
40+ CreateAddresses();
41+
42+ CDataStream s_orig(SER_NETWORK, PROTOCOL_VERSION | add_to_version);
43+ s_orig << g_sources;
44+
45+ bench.run([&] {
46+ CDataStream s(s_orig);
47+
48+ std::vector<CAddress> unserialized;
49+ s >> unserialized;
50+
51+ assert(s.empty());
52+ });
53+}
54+
55+static void AddressUnserV1(benchmark::Bench& bench)
56+{
57+ AddressUnser(bench, 0);
58+}
59+
60+static void AddressUnserV2(benchmark::Bench& bench)
61+{
62+ AddressUnser(bench, ADDRV2_FORMAT);
63+}
64+
65+BENCHMARK(AddressUnserV1);
66+BENCHMARK(AddressUnserV2);
Results (one operation is “unserialize 1000 addresses”):
0$ ./src/bench/bench_bitcoin -filter="AddressUnser.*"
1
2| ns/op | op/s | err% | total | benchmark
3|--------------------:|--------------------:|--------:|----------:|:----------
4| 39,362.00 | 25,405.21 | 0.3% | 0.00 | `AddressUnserV1`
5| 49,457.00 | 20,219.58 | 0.0% | 0.00 | `AddressUnserV2`
Unserialize in V2 is expected to be somewhat slower and it is indeed. Looks ok to me.
master.
191@@ -192,6 +192,17 @@ class CNetAddr
192 friend class CSubNet;
193
194 private:
195+ /**
196+ * Check whether a container begins with the given prefix.
197+ */
198+ template <typename T1, size_t PREFIX_LEN>
Just a nit, feel free to leave as is.
No real reason why this needs to be in netaddress.h or restricted to CNetAddr. Maybe it can move to one of the util/*.h files or even a new one. It’s probably useful elsewhere.
Yes, I did consider making it more visible but refrained because of the wretched std::array argument (which was imposed by gcc 4.8) - I think it is not very friendly like this.
Moved now to util/string.h since you mention this. Once support for gcc 4.8 is dropped we can improve it to take some more generic argument.
197 }
198
199 /**
200 * Unserialize from a stream.
201+ * @throws std::ios_base::failure if the stream is in ADDRv2 format and
202+ * the data is invalid and cannot be unserialized; ADDRv1 format
That’s not technically true; EOF can be reached in ADDRv1 mode.
I don’t think this comment adds much; every deserializer can always fail.
224@@ -203,11 +225,41 @@ class CNetAddr
225 std::equal(std::begin(prefix), std::end(prefix), std::begin(obj));
226 }
227
228+ /**
229+ * BIP155 network ids recognized by this software.
230+ */
231+ enum Bip155Network : uint8_t {
433+ BOOST_CHECK_EQUAL(addr.ToString(),
434+ "ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p");
435+ BOOST_REQUIRE(s.empty());
436+
437+ // Invalid I2P, with bogus length.
438+ s << MakeSpan(ParseHex("05" // network type (TORv2)
Concept/approach ACK. Code looks good, with just a few nits.
I’m a bit surprised that string serialization of TORv3 is implemented (CNetAddr::ToStringIP), but parsing (CNetAddr::SetSpecial) is not. The latter would enable passing TORv3 addresses in -addnode etc. Not a requirement for this PR, but I wanted to point it out as I also don’t see it in a follow-up either.
444+ HasReason("BIP155 I2P address with length 3 (should be 32)"));
445+ BOOST_REQUIRE(!s.empty()); // The stream is not consumed on invalid input.
446+ s.clear();
447+
448+ // Valid CJDNS.
449+ s << MakeSpan(ParseHex("06" // network type (I2P)
454+ BOOST_CHECK(addr.IsValid());
455+ BOOST_CHECK_EQUAL(addr.ToString(), "fc00:1:2:3:4:5:6:7");
456+ BOOST_REQUIRE(s.empty());
457+
458+ // Invalid CJDNS, with bogus length.
459+ s << MakeSpan(ParseHex("06" // network type (TORv2)
49@@ -39,9 +50,15 @@ enum Network
50 /// IPv6
51 NET_IPV6,
52
53- /// TORv2
54+ /// TOR (v2 or v3)
55 NET_ONION,
Would it make sense to have NET_ONION_V2 and NET_ONION_V3, with associated IsOnionV2() and IsOnionV3() bool helpers as subsets of IsTor()/IsOnion()?
Doing so might also clean up GetBip155Network(), SetIP(), and ToStringIP() by removing the size checks to a better place.
I did consider this but did not make them separate because:
NET_ONION.
Move the function `HasPrefix()` from `netaddress.cpp` to `util/string.h`
so it can be reused by `CNetAddr` methods (and possibly others).
As a test I created a TORv3 bitcoin service, and connected to it with this branch:
0
1$ ./src/bitcoin-cli getpeerinfo
2[
3 {
4 "id": 1,
5 "addr": "kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion",
362+ if (RecognizeBIP155Network(bip155_net, address_size)) {
363+ m_addr.resize(address_size);
364+ s >> MakeSpan(m_addr);
365+
366+ // Recognize NET_INTERNAL embedded in IPv6.
367+ if (m_net == NET_IPV6 && HasPrefix(m_addr, INTERNAL_IN_IPV6_PREFIX)) {
Because IPv4 and TORv2 have their own BIP155 network ids and should not be disguised as IPv6.
NET_INTERNAL is not gossiped, but addrman may keep such addresses as “source” of other addresses and thus end up (un)serializing them when saving its database to disk in a V2 stream (not in this PR).
In this PR we may end up having some TORv3 addresses in addrman and try to serialize them in V1 stream. They will go as 16 zeroes.
Connected to @sipa from my tor v2 address.
02020-09-11T18:44:21Z New outbound peer connected: version: 70016, blocks=647799, peer=5, peeraddr=kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion:8333 (full-relay)
getpeerinfo:
0 {
1 "id": 5,
2 "addr": "kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion",
3 "addrbind": "127.0.0.1:36454",
4 "services": "0000000000000409",
from my tor v2 address.
Tor hidden service connections don’t have a “from” address ;)
51+ } // no default case, so the compiler can warn about missing cases
52+
53+ assert(false);
54+}
55+
56+bool CNetAddr::RecognizeBIP155Network(uint8_t possible_bip155_net, size_t address_size)
SetBIP155Network as it sets m_net whereas GetBIP155Network reads it.
SetBIP155Network would be incorrect because m_net, which is being set, is of type Network, not BIP155Network.
Renamed to this function to SetNetFromBIP155Network() to imply “set m_net from variable of type BIP155Network”.
260+ * @returns BIP155 network id
261+ */
262+ BIP155Network GetBIP155Network() const;
263+
264+ /**
265+ * Deduce the network type from BIP155 network id and size and set `m_net`.
m_net and the other sets m_net.
The word “deduce” in the doc here is still confusing. Suggestion:
Set
m_netafter validating the serialised address. This currently checks if the address length matches what is expected for the serialised BIP155 id.
420+ // If we receive an unknown BIP155 network id (from the future?) then
421+ // ignore the address - unserialize as !IsValid().
422+ s.ignore(address_size);
423+ }
424+
425+ // Mimic a default-constructed object which is !IsValid() and thus will not
311
312+BOOST_AUTO_TEST_CASE(cnetaddr_serialize_v2)
313+{
314+ CNetAddr addr;
315+ CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
316+ // Sneak ADDRV2_FORMAT into the version so that the CNetAddr
233 {
234- if (strName.size()>6 && strName.substr(strName.size() - 6, 6) == ".onion") {
235- std::vector<unsigned char> vchAddr = DecodeBase32(strName.substr(0, strName.size() - 6).c_str());
236- if (vchAddr.size() != ADDR_TORV2_SIZE) {
237+ static const char* suffix = ".onion";
238+ static constexpr size_t suffix_len = 6;
1046e17c perhaps
0- static const char* suffix = ".onion";
1- static constexpr size_t suffix_len = 6;
2+ static const std::string suffix{".onion"};
3+ static const size_t suffix_len{suffix.size()};
std::string instead of char*. There is no need to create a std::string object.
239+
240+ if (str.size() <= suffix_len || str.substr(str.size() - suffix_len) != suffix) {
241+ return false;
242+ }
243+
244+ const auto& input = DecodeBase32(str.substr(0, str.size() - suffix_len).c_str());
1046e17 the previous code used c_str(), but maybe use data() while retouching this
Developer notes on c_str() just in case (ignore if this is noise):
Use .c_str() sparingly. Its only valid use is to pass C++ strings to C functions that take NULL-terminated
strings.
Do not use it when passing a sized array (so along with .size()). Use .data() instead to get a pointer to the raw data.
.data() communicates the intent better.In cases where do you call .c_str(), you might want to additionally check that the string does not contain embedded ‘\0’ characters, because it will (necessarily) truncate the string. This might be used to hide parts of the string from logging or to circumvent checks. If a use of strings is sensitive to this, take care to check the string for embedded NULL characters first and reject it if there are any (see ParsePrechecks in strencodings.cpp for an example).
Hmm, both c_str() and data() return the same thing. I kept it to c_str() because DecodeBase32() expects a nul-terminated string and c_str() has always been returning such string, whereas data() does so only after C++11. I.e. c_str() is more widely known to return a nul-terminated string.
Anyway, I added a check whether the string contains an embedded '\0' plus a test for it.
484+ assert(a.size() == ADDR_IPV6_SIZE);
485+ return strprintf("%x:%x:%x:%x:%x:%x:%x:%x",
486+ a[0] << 8 | a[1], a[2] << 8 | a[3],
487+ a[4] << 8 | a[5], a[6] << 8 | a[7],
488+ a[8] << 8 | a[9], a[10] << 8 | a[11],
489+ a[12] << 8 | a[13], a[14] << 8 | a[15]);
1046e17c nit, this might be more readable with one element per line:
0- a[0] << 8 | a[1], a[2] << 8 | a[3],
1- a[4] << 8 | a[5], a[6] << 8 | a[7],
2- a[8] << 8 | a[9], a[10] << 8 | a[11],
3- a[12] << 8 | a[13], a[14] << 8 | a[15]);
4+ a[0] << 8 | a[1],
5+ a[2] << 8 | a[3],
6+ a[4] << 8 | a[5],
7+ a[6] << 8 | a[7],
8+ a[8] << 8 | a[9],
9+ a[10] << 8 | a[11],
10+ a[12] << 8 | a[13],
11+ a[14] << 8 | a[15]);
ReadBE16(), must be more readable now.
518+ case ADDR_TORV3_SIZE: {
519+
520+ uint8_t checksum[torv3::CHECKSUM_LEN];
521+ torv3::Checksum(m_addr, checksum);
522+
523+ // onion_address = base32(PUBKEY | CHECKSUM | VERSION) + ".onion"
200+static const unsigned char VERSION[] = "\x03";
201+static constexpr size_t TOTAL_LEN = ADDR_TORV3_SIZE + CHECKSUM_LEN + VERSION_LEN;
202+
203+static void Checksum(Span<const uint8_t> addr_pubkey, uint8_t (&checksum)[CHECKSUM_LEN])
204+{
205+ // CHECKSUM = H(".onion checksum" | PUBKEY | VERSION)[:2]
930@@ -742,7 +931,7 @@ std::string CService::ToStringPort() const
931
932 std::string CService::ToStringIPPort() const
933 {
934- if (IsIPv4() || IsTor() || IsInternal()) {
935+ if (IsIPv4() || IsTor() || IsI2P() || IsInternal()) {
IsCJDNS()…should CJDNS IP strings be in IPv4 dotted-quad or IPv6 colon-hexadecimal notation?
212- while (str.size() % 8) str += '=';
213+ if (pad) {
214+ while (str.size() % 8) {
215+ str += '=';
216+ }
217+ }
pad is needed, maybe in a doxygen doc for the function in the header file strencodings.h
82+ strprintf("BIP155 TORv2 address with length %u (should be %u)", address_size,
83+ ADDR_TORV2_SIZE));
84+ case BIP155Network::TORV3:
85+ if (address_size == ADDR_TORV3_SIZE) {
86+ m_net = NET_ONION;
87+ return true;
229 * @returns Whether or not the operation was successful.
230 *
231 * @see CNetAddr::IsTor()
232 */
233-bool CNetAddr::SetSpecial(const std::string &strName)
234+bool CNetAddr::SetSpecial(const std::string &str)
8d2b60cb nit, while touching this line
0bool CNetAddr::SetSpecial(const std::string& str)
481@@ -328,28 +482,71 @@ enum Network CNetAddr::GetNetwork() const
482 return m_net;
483 }
484
485+static std::string IPv6ToString(Span<const uint8_t> a) {
8d2b60cb nit, clang format
0-static std::string IPv6ToString(Span<const uint8_t> a) {
1+static std::string IPv6ToString(Span<const uint8_t> a)
2+{
625@@ -429,20 +626,20 @@ uint32_t CNetAddr::GetLinkedIPv4() const
626 }
627
628 uint32_t CNetAddr::GetNetClass() const {
629- uint32_t net_class = NET_IPV6;
630- if (IsLocal()) {
631- net_class = 255;
632- }
633+ // Make sure that if we return NET_IPV6, then IsIPv6() is true. The callers expect that.
8d2b60cb nit, clang format while touching this
0-uint32_t CNetAddr::GetNetClass() const {
1+uint32_t CNetAddr::GetNetClass() const
2+{
492+ ReadBE16(&a[6]),
493+ ReadBE16(&a[8]),
494+ ReadBE16(&a[10]),
495+ ReadBE16(&a[12]),
496+ ReadBE16(&a[14]));
497+ // clang-format on
clang-format declarations don’t seem needed for me; ignore me if they are needed for other versions.
347+ BOOST_REQUIRE(addr.SetSpecial("6hzph5hv6337r6p2.onion"));
348+ s << addr;
349+ BOOST_CHECK_EQUAL(HexStr(s), "030af1f2f3f4f5f6f7f8f9fa");
350+ s.clear();
351+
352+ BOOST_REQUIRE(addr.SetSpecial("kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion"));
332+ * Serialize as ADDRv2 / BIP155.
333+ */
334+ template <typename Stream>
335+ void SerializeV2Stream(Stream& s) const
336+ {
337+ if (m_net == NET_INTERNAL) {
80c77c31
0 if (IsInternal()) {
The changes and added tests per git diff 1046e17 8d2b60c look good and this seems close. I’ve been running a tor v3 service with this PR since a day now.
A few nits below (feel free to ignore but happy to re-review if you update), and one suggestion to use IsInternal() rather than m_net == NET_INTERNAL.
ACK 8d2b60cbd737f9c27a7b42cf3373264fff6acf42
git diff 8d2b60c 3eef1e6
241+ */
242+ BIP155Network GetBIP155Network() const;
243+
244+ /**
245+ * Deduce the network type from BIP155 network id and size and set `m_net`.
246+ * @returns true if the network was recognized and `m_net` was set
Maybe add:
returns false for unrecognised (future) network ids, which must be ignored (BIP155).
359+ "fd0102" // address length (513 as CompactSize)
360+ "01020304")); // address
361+ BOOST_CHECK_EXCEPTION(s >> addr, std::ios_base::failure,
362+ HasReason("Address too long: 513 > 512"));
363+ BOOST_REQUIRE(!s.empty()); // The stream is not consumed on invalid input.
364+ s.clear();
address is shorter than the announced address length (which itself is valid). Is this code covered by fuzzing?
Added a new test.
Yes, it is fuzzed: https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/deserialize.cpp#L185
424+ BOOST_CHECK_EXCEPTION(s >> addr, std::ios_base::failure,
425+ HasReason("BIP155 TORv2 address with length 7 (should be 10)"));
426+ BOOST_REQUIRE(!s.empty()); // The stream is not consumed on invalid input.
427+ s.clear();
428+
429+ // Unknown, with extreme length.
321+ BOOST_CHECK_EQUAL(HexStr(s), "030af1f2f3f4f5f6f7f8f9fa");
322+ s.clear();
323+
324+ BOOST_REQUIRE(addr.SetInternal("a"));
325+ s << addr;
326+ BOOST_CHECK_EQUAL(HexStr(s), "0210fd6b88c08724ca978112ca1bbdcafac2");
I get that:
0210 indicates IPv6 (in which the internal is embedded)0xFD + sha256("bitcoin")[0:5] is the prefix for internal, which explains fd6b88c08724But how is the rest (ca978112ca1bbdcafac2) generated? Might be worth a comment. Either way, it matches the pre-existing v1 test.
37+ case ADDR_TORV2_SIZE:
38+ return BIP155Network::TORV2;
39+ case ADDR_TORV3_SIZE:
40+ return BIP155Network::TORV3;
41+ default:
42+ assert(false);
This needs a strong warning:
0// length should have been checked before calling this function
ADDR_TORV2_SIZE or ADDR_TORV3_SIZE. There is no code path that would lead to such scenario and if it happens it means some memory corruption or some future bug. It is the same as m_net being equal to NET_MAX for example.
// length should have been checked before calling this function
I disagree; length is part of the CNetAddr objects, so this precondition is just restating an implicit “can only be called on a valid CNetAddr object”.
tACK 3eef1e65c2ac6cbbddd734f3b192861d9e73ecfd
I did not check NET_I2P and NET_CJDNS stuff.
Note that the ADDRV2_FORMAT constant may be replaced with another mechanism, but that can easily be done in a followup PR.
If necessary the last commit, which adds v3 parsing and (implicitly) connecting, could be its own PR.
I’m able to connect to sipa’s (alleged) demo node. The peers window is confused by this, but that can wait.
addnode onionV3 addresses and connect after launching with -proxy=127.0.0.1:9050 -onlynet=onion.
Reviewing code next…
152@@ -153,4 +153,20 @@ CBlock getBlock13b8a();
153 // define an implicit conversion here so that uint256 may be used directly in BOOST_CHECK_*
154 std::ostream& operator<<(std::ostream& os, const uint256& num);
155
156+/**
157+ * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.
158+ * Use as
159+ * BOOST_CHECK_EXCEPTION(code that throws, exception type, HasReason("foo"));
160+ */
161+class HasReason {
HasReason is a class but the commit message is talking about moving a function.
37+ } // no default case, so the compiler can warn about missing cases
38+
39+ assert(false);
40+}
41+
42+bool CNetAddr::SetNetFromBIP155Network(uint8_t possible_bip155_net, size_t address_size)
nit
0bool CNetAddr::SetNetFromBIP155Network(const uint8_t possible_bip155_net, const size_t address_size)
I think this and #19845 (review) below are following these two guidelines:
0void g(const int i); // pedantic
My take is the same as the isocpp guidelines - no const for function arguments that are passed by value.
Whenever I see void func(const int x); my reaction is “Huh!? Did the author realize that no matter what the function body is it cannot modify the caller’s variable given as an argument? Or did the author mean const int* x or const int& x?
Yes, the compiler may do some optimizations inside the function body, but that is out-weighted by the above. And I guess modern compilers may do the optimizations even without the presence of const.
I agree with @vasild; using const for by-value arguments looks strange.
I would be very surprised if it enables any optimization in common cases. In the SSA transformation every assignment is treated as a new constant value, regardless of the variables that hold it or their types. This works especially well for small values, which tend to be the ones that are passed around by value.
All const does in this case is prevent accidentally modifying the local copy of the parameter during execution of the function. It’s similar to just declaring a local variable defined in the body of the function const, but in a way that can be confused to look like it’s part of the function’s contract.
200@@ -201,20 +201,24 @@ std::string DecodeBase64(const std::string& str, bool* pf_invalid)
201 return std::string((const char*)vchRet.data(), vchRet.size());
202 }
203
204-std::string EncodeBase32(Span<const unsigned char> input)
205+std::string EncodeBase32(Span<const unsigned char> input, bool pad)
nit: same with the string one below
0std::string EncodeBase32(Span<const unsigned char> input, const bool pad)
Concept ACK, light code-review ACK
I didn’t see any issues with the code but need a bit more time to review and test before I am comfortable giving a full ACK.
Move the class `HasReason` from `miner_tests.cpp` to
`setup_common.h` so that it can be reused by other tests.
Note that the
ADDRV2_FORMATconstant may be replaced with another mechanism, but that can easily be done in a followup PR.
Do you mean this one: #19845 (comment) or something else?
Filtered code coverage report (files not modified by this PR are omitted and not modified lines in files that are otherwise modified are dimmed).
263+ BIP155Network GetBIP155Network() const;
264+
265+ /**
266+ * Set `m_net` from the provided BIP155 network id and size after validation.
267+ * @retval true the network was recognized, is valid and `m_net` was set
268+ * @retval false not unrecognised (from future?) and should be silently ignored
re-ACK 65a8d31 per git diff 3eef1e6 65a8d31 modulo doc fix
Changed all the bogus length tests to test the bounds values of 0, (address size - 1), and (address size + 1) to check behavior; it was the same.
Co-authored-by: Carl Dong <contact@carldong.me>
git diff 65a8d31 b9c46e0
193@@ -101,24 +194,82 @@ bool CNetAddr::SetInternal(const std::string &name)
194 return true;
195 }
196
197+namespace torv3 {
198+// https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt#n2135
199+static constexpr size_t CHECKSUM_LEN = 2;
200+static constexpr size_t VERSION_LEN = 1;
201+static const unsigned char VERSION[] = "\x03";
Nit: If you write this as = {3}; instead, the write below can become hasher.Write(VERSION);.
You could even do something similar with prefix, writing the initializer as = {'.', 'o', 'n', 'i', 'o', 'n', ' ', ..., but that looks kind of ugly.
Changed to {3}, this also made it possible to remove the VERSION_LEN variable.
Yeah, {'.', 'o', ... makes it too weird. Given that Write(MakeSpan(prefix).first(prefix_len)) (which could have been simplified to Write(prefix)) is used in just one place, I left it as is.
386+ if (address_size > MAX_ADDRV2_SIZE) {
387+ throw std::ios_base::failure(strprintf(
388+ "Address too long: %u > %u", address_size, MAX_ADDRV2_SIZE));
389+ }
390+
391+ scopeId = 0;
scopeId variable being used in this block, could it be removed? Or is it required to be set here for something else down the line?
CNetAddr, so yes, it is used later.
utACK b9c46e0a935a14063913f0c999922f8e22bce61d
Also did some mild testing with a previous version of the code, and have a (succesfully reachable) TorV3 hidden service node running with it.
193@@ -101,24 +194,82 @@ bool CNetAddr::SetInternal(const std::string &name)
194 return true;
195 }
196
197+namespace torv3 {
The primary reason to use a namespace - things belong together.
Every time I spot myself prefixing symbols with a common prefix I ask myself whether I am implementing a poor man’s namespace. In this case, without a namespace, I would have had to prefix all the symbols with a torv3_ prefix. E.g. TORV3_CHECKSUM_LEN, TORV3_VERSION, Torv3Checksum().
208
209 std::string str;
210 str.reserve(((input.size() + 4) / 5) * 8);
211 ConvertBits<8, 5, true>([&](int v) { str += pbase32[v]; }, input.begin(), input.end());
212- while (str.size() % 8) str += '=';
213+ if (pad) {
Recognizing addresses from those networks allows us to accept and gossip
them, even though we don't know how to connect to them (yet).
Co-authored-by: eriknylund <erik@daychanged.com>
Added no-padding tests for EncodeBase32() by @eriknylund and a simplification around torv3::VERSION suggested by @sipa.
Thanks!
-onlynet=onion without issues and I can connect to other peer reviewers running TorV3 on their nodes and I can connect both of my test nodes to each other.
17 for (unsigned int i=0; i<sizeof(vstrIn)/sizeof(vstrIn[0]); i++)
18 {
19 std::string strEnc = EncodeBase32(vstrIn[i]);
20 BOOST_CHECK_EQUAL(strEnc, vstrOut[i]);
21+ strEnc = EncodeBase32(vstrIn[i], false);
22+ BOOST_CHECK_EQUAL(strEnc, vstrOutNoPadding[i]);
Sanity-checked that the new test fails with
0- strEnc = EncodeBase32(vstrIn[i], false);
1+ strEnc = EncodeBase32(vstrIn[i], true);
git diff b9c46e0 7be6ff6, debug build, ran/running bitcoind with this change and observed the log and -netinfo peer connections while connected as a tor v2 service to both tor v2 peers and also five tor v3 peers.
165+ bool operator() (const E& e) const {
166+ return std::string(e.what()).find(m_reason) != std::string::npos;
167+ };
168+private:
169+ const std::string m_reason;
170+};
fe42411b4b07b99c591855f5f00ad45dfeec8e30
nanonit – clang-format suggests:
0class HasReason
1{
2public:
3 explicit HasReason(const std::string& reason) : m_reason(reason) {}
4 template <typename E>
5 bool operator()(const E& e) const
6 {
7 return std::string(e.what()).find(m_reason) != std::string::npos;
8 };
9
10private:
11 const std::string m_reason;
12};
159+ * BOOST_CHECK_EXCEPTION(code that throws, exception type, HasReason("foo"));
160+ */
161+class HasReason {
162+public:
163+ explicit HasReason(const std::string& reason) : m_reason(reason) {}
164+ template <typename E>
fe42411b4b07b99c591855f5f00ad45dfeec8e30
Why introduce a template in “move” commit?
This pull compiles with the previous std::runtime_error parameter.
std::ios_base::failure as well. Now that you say it compiles with std::runtime_error I realized that the former inherits the latter in C++11.
27+ * A flag that is ORed into the protocol version to designate that addresses
28+ * should be serialized in (unserialized from) v2 format (BIP155).
29+ * Make sure that this does not collide with any of the values in `version.h`
30+ * or with `SERIALIZE_TRANSACTION_NO_WITNESS`.
31+ */
32+static const int ADDRV2_FORMAT = 0x20000000;
e0d73573a37bf4b519f6f61e5678572d48a64517
nit:
0static constexpr int ADDRV2_FORMAT = 0x20000000;
ACK 7be6ff61875a8d5d2335bff5d1f16ba40557adb0, tested on Linux Mint 20 (x86_64): on top of this pull and #19031 I’m able to connect to onion v3 addresses, and jonatack is able to connect to my created onion v3 address.
Verified BIP155 requirements.