net: CNetAddr: add support to (un)serialize as ADDRv2 #19845

pull vasild wants to merge 4 commits into bitcoin:master from vasild:ser_addrv2_cnetaddr changing 12 files +783 −77
  1. vasild commented at 1:15 pm on August 31, 2020: member

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

  2. DrahtBot added the label Build system on Aug 31, 2020
  3. DrahtBot added the label P2P on Aug 31, 2020
  4. DrahtBot added the label Utils/log/libs on Aug 31, 2020
  5. DrahtBot commented at 2:47 pm on August 31, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19954 (tor: make a TORv3 hidden service instead of TORv2 by vasild)
    • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)

    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.

  6. MarcoFalke removed the label Build system on Sep 1, 2020
  7. MarcoFalke removed the label Utils/log/libs on Sep 1, 2020
  8. vasild force-pushed on Sep 1, 2020
  9. vasild commented at 10:25 am on September 1, 2020: member
    If #19503 gets merged then it could be used to configure the stream to (un)ser in V2 format instead of the currently used ADDRV2_FORMAT flag that is ORed into the stream version.
  10. jonatack commented at 1:46 pm on September 1, 2020: member

    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.

  11. vasild commented at 8:03 am on September 2, 2020: member
    This PR now contains #19841, needed for the SHA3_256 implementation.
  12. MarcoFalke commented at 1:03 pm on September 5, 2020: member
    Would it make sense to benchmark how fast addr2 messages are deserialized? Maybe we should even benchmark every message type?
  13. vasild commented at 1:38 pm on September 7, 2020: member

    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.

  14. DrahtBot added the label Needs rebase on Sep 10, 2020
  15. vasild force-pushed on Sep 10, 2020
  16. vasild commented at 6:45 pm on September 10, 2020: member
    Rebased now that the SHA3_256 implementation is in master.
  17. DrahtBot removed the label Needs rebase on Sep 10, 2020
  18. laanwj added this to the "Blockers" column in a project

  19. in src/netaddress.h:198 in e57bedde31 outdated
    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>
    


    sipa commented at 10:32 pm on September 10, 2020:

    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.


    vasild commented at 11:43 am on September 11, 2020:

    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.

  20. in src/netaddress.h:201 in c73ea45e12 outdated
    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
    


    sipa commented at 10:35 pm on September 10, 2020:

    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.


    vasild commented at 12:00 pm on September 11, 2020:
    Removed.
  21. in src/netaddress.h:231 in c73ea45e12 outdated
    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 {
    


    sipa commented at 10:40 pm on September 10, 2020:
    Nit: you’re using both “Bip155” and “BIP155” is this commit. I prefer “BIP155”.

    vasild commented at 11:48 am on September 11, 2020:
    Changed to “BIP155”, that way its also more consistent with the rest of the code.
  22. in src/test/net_tests.cpp:438 in 9eb5b089c5 outdated
    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)
    


    sipa commented at 10:44 pm on September 10, 2020:
    Comment is wrong here, I think (I2P, not TORv2).

    vasild commented at 11:51 am on September 11, 2020:
    Fixed.
  23. sipa commented at 11:02 pm on September 10, 2020: member

    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.

  24. in src/test/net_tests.cpp:448 in 9eb5b089c5 outdated
    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)
    


    jonatack commented at 10:19 am on September 11, 2020:
    9eb5b089 s/I2P/CJDNS/

    vasild commented at 11:51 am on September 11, 2020:
    Fixed.
  25. in src/test/net_tests.cpp:458 in 9eb5b089c5 outdated
    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)
    


    jonatack commented at 10:19 am on September 11, 2020:
    9eb5b089 s/TORv2/CJDNS/

    vasild commented at 11:52 am on September 11, 2020:
    Fixed.
  26. in src/netaddress.h:55 in 9eb5b089c5 outdated
    49@@ -39,9 +50,15 @@ enum Network
    50     /// IPv6
    51     NET_IPV6,
    52 
    53-    /// TORv2
    54+    /// TOR (v2 or v3)
    55     NET_ONION,
    


    jonatack commented at 10:39 am on September 11, 2020:

    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.


    jonatack commented at 11:06 am on September 11, 2020:
    Semi-related, at some point it might be helpful to be able to expose network types in getpeerinfo, rather than clients trying to guess like we currently have to do in #19643.

    vasild commented at 1:43 pm on September 11, 2020:

    I did consider this but did not make them separate because:

    • Intrinsically, TORv2 and TORv3 addresses belong to the same network - the TOR network. It is more like odd and even IPv4 addresses, not so much as IPv4 and IPv6 addresses.
    • Given that TORv2 is going to be dropped soon, we would have to clean up our code and remove any remains from it which would be easier if this is kept just as NET_ONION.

    sipa commented at 5:59 pm on September 11, 2020:
    I considered suggesting this too, but outside of netaddress.*, the distinction probably doesn’t matter (e.g. the decision whether to use a proxy and which one doesn’t need to distinguish between TorV2 and TorV3), so this approach allows keeping the changes more local.
  27. jonatack commented at 10:59 am on September 11, 2020: member
    Concept and approach ACK on first pass through the changeset. The unit tests are very helpful. I’m looking forward to this functionality.
  28. util: move HasPrefix() so it can be reused
    Move the function `HasPrefix()` from `netaddress.cpp` to `util/string.h`
    so it can be reused by `CNetAddr` methods (and possibly others).
    d2bb681f96
  29. vasild force-pushed on Sep 11, 2020
  30. vasild commented at 1:44 pm on September 11, 2020: member
    Addressed review suggestions and added TORv3 address parsing with some tests.
  31. sipa commented at 6:01 pm on September 11, 2020: member

    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",
    
  32. in src/netaddress.h:367 in 3472f64442 outdated
    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)) {
    


    sipa commented at 6:32 pm on September 11, 2020:
    Why do this for NET_INTERNAL, but not for IPv4 or TORv2?

    vasild commented at 7:26 pm on September 11, 2020:

    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.


    vasild commented at 8:38 pm on September 11, 2020:
    Following an IRC discussion - I changed it to recognize and ignore (unserialize as invalid) embedded IPv4 and TORv2 in IPv6 (when in addrv2 message).
  33. jonatack commented at 6:48 pm on September 11, 2020: member

    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",
    
  34. sipa commented at 8:33 pm on September 11, 2020: member

    from my tor v2 address.

    Tor hidden service connections don’t have a “from” address ;)

  35. vasild force-pushed on Sep 11, 2020
  36. vasild force-pushed on Sep 12, 2020
  37. vasild commented at 8:00 am on September 12, 2020: member
    Ignored embedded IPv4 and TORv2 in IPv6 when unserializing in V2 format, as per BIP155 + tests to confirm they render as invalid.
  38. in src/netaddress.cpp:55 in 1046e17c13 outdated
    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)
    


    jonatack commented at 12:24 pm on September 14, 2020:
    8036a267 Perhaps call this function SetBIP155Network as it sets m_net whereas GetBIP155Network reads it.

    vasild commented at 2:10 pm on September 15, 2020:

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


    jonatack commented at 3:59 pm on September 15, 2020:
    SGTM.
  39. in src/netaddress.h:265 in 1046e17c13 outdated
    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`.
    


    jonatack commented at 12:27 pm on September 14, 2020:
    8036a267 These two “Deduce” words make the two functions seem similar, whereas one function reads m_net and the other sets m_net.

    vasild commented at 2:10 pm on September 15, 2020:
    Reworded.

    Sjors commented at 1:17 pm on September 16, 2020:

    The word “deduce” in the doc here is still confusing. Suggestion:

    Set m_net after validating the serialised address. This currently checks if the address length matches what is expected for the serialised BIP155 id.

  40. in src/netaddress.h:425 in 1046e17c13 outdated
    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
    


    jonatack commented at 12:41 pm on September 14, 2020:
    8036a267 nit, perhaps s/object/IPV6 object/

    vasild commented at 2:11 pm on September 15, 2020:
    Changed to “default-constructed CNetAddr object”.
  41. in src/test/net_tests.cpp:315 in 1046e17c13 outdated
    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
    


    jonatack commented at 12:53 pm on September 14, 2020:
    8036a267 suggest s/Sneak/Bitwise-OR the/ (here and line 348) as this operation isn’t covert or sneaky :)

    vasild commented at 2:12 pm on September 15, 2020:
    Changed to just “Add ADDRV2_FORMAT …”
  42. in src/netaddress.cpp:234 in 1046e17c13 outdated
    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;
    


    jonatack commented at 1:46 pm on September 14, 2020:

    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()};
    

    vasild commented at 2:12 pm on September 15, 2020:
    Changed.

    jonatack commented at 4:01 pm on September 15, 2020:
    My comment wasn’t about the syntax, but rather why not use std::string and size().

    vasild commented at 11:45 am on September 16, 2020:
    Ah, now I see std::string instead of char*. There is no need to create a std::string object.
  43. in src/netaddress.cpp:242 in 1046e17c13 outdated
    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());
    


    jonatack commented at 1:57 pm on September 14, 2020:

    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.

        • Rationale: Although this is guaranteed to be safe starting with C++11, .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).


    vasild commented at 2:16 pm on September 15, 2020:

    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.

  44. in src/netaddress.cpp:489 in 1046e17c13 outdated
    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]);
    


    jonatack commented at 2:16 pm on September 14, 2020:

    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]);
    

    vasild commented at 2:20 pm on September 15, 2020:
    Changed to ReadBE16(), must be more readable now.
  45. in src/netaddress.cpp:520 in 1046e17c13 outdated
    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"
    


    jonatack commented at 2:17 pm on September 14, 2020:
    1046e17c nit suggestion if you retouch: s/onion_address/tor v3 onion address/

    vasild commented at 2:17 pm on September 15, 2020:
    I copied that verbatim from https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt#n2135, but anyway - I now prefixed it with “TORv3”.
  46. in src/netaddress.cpp:205 in 1046e17c13 outdated
    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]
    


    jonatack commented at 2:19 pm on September 14, 2020:
    1046e17c nit suggestion if you retouch: s/CHECKSUM/tor v3 checksum/

    vasild commented at 2:17 pm on September 15, 2020:
    I copied that verbatim from https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt#n2135, but anyway - I now prefixed it with “TORv3”.
  47. in src/netaddress.cpp:947 in 1046e17c13 outdated
    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()) {
    


    jonatack commented at 2:31 pm on September 14, 2020:
    1046e17c IsCJDNS()…should CJDNS IP strings be in IPv4 dotted-quad or IPv6 colon-hexadecimal notation?

    vasild commented at 2:18 pm on September 15, 2020:
    To my understanding CJDNS addresses have the same representation as IPv6, just different meaning.
  48. in src/util/strencodings.cpp:215 in 1046e17c13 outdated
    212-    while (str.size() % 8) str += '=';
    213+    if (pad) {
    214+        while (str.size() % 8) {
    215+            str += '=';
    216+        }
    217+    }
    


    jonatack commented at 2:37 pm on September 14, 2020:
    1046e17c would be good to add an explanation of why pad is needed, maybe in a doxygen doc for the function in the header file strencodings.h

    vasild commented at 2:19 pm on September 15, 2020:
    I don’t know why the padding is needed! :) But I added a comment on what the newly added parameter does.

    jonatack commented at 3:59 pm on September 15, 2020:
    Thanks.
  49. jonatack commented at 2:40 pm on September 14, 2020: member
    Almost-ACK, everything seems functional. A few comments below.
  50. vasild force-pushed on Sep 15, 2020
  51. in src/netaddress.cpp:86 in 8d2b60cbd7 outdated
    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;
    


    jonatack commented at 2:56 pm on September 15, 2020:
    80c77c31 nit, indenting is off by one for the NET_IPV4, NET_IPV6, and NET_ONION cases

    vasild commented at 7:41 pm on September 15, 2020:
    Fixed.
  52. in src/netaddress.cpp:232 in 8d2b60cbd7 outdated
    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)
    


    jonatack commented at 2:58 pm on September 15, 2020:

    8d2b60cb nit, while touching this line

    0bool CNetAddr::SetSpecial(const std::string& str)
    

    vasild commented at 7:41 pm on September 15, 2020:
    Done.
  53. in src/netaddress.cpp:485 in 8d2b60cbd7 outdated
    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) {
    


    jonatack commented at 3:04 pm on September 15, 2020:

    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+{
    

    vasild commented at 7:42 pm on September 15, 2020:
    Fixed.
  54. in src/netaddress.cpp:634 in 8d2b60cbd7 outdated
    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.
    


    jonatack commented at 3:07 pm on September 15, 2020:

    8d2b60cb nit, clang format while touching this

    0-uint32_t CNetAddr::GetNetClass() const {
    1+uint32_t CNetAddr::GetNetClass() const
    2+{
    

    vasild commented at 7:41 pm on September 15, 2020:
    Done.
  55. in src/netaddress.cpp:501 in 8d2b60cbd7 outdated
    492+                     ReadBE16(&a[6]),
    493+                     ReadBE16(&a[8]),
    494+                     ReadBE16(&a[10]),
    495+                     ReadBE16(&a[12]),
    496+                     ReadBE16(&a[14]));
    497+    // clang-format on
    


    jonatack commented at 3:12 pm on September 15, 2020:
    8d2b60cb With clang-format-12 these two clang-format declarations don’t seem needed for me; ignore me if they are needed for other versions.

    vasild commented at 7:38 pm on September 15, 2020:
    clang-format 12 puts everything in one line for me.
  56. in src/test/net_tests.cpp:356 in 8d2b60cbd7 outdated
    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"));
    


    jonatack commented at 3:22 pm on September 15, 2020:
    Heh, I recognize that address ;)
  57. in src/netaddress.h:337 in 8d2b60cbd7 outdated
    332+         * Serialize as ADDRv2 / BIP155.
    333+         */
    334+        template <typename Stream>
    335+        void SerializeV2Stream(Stream& s) const
    336+        {
    337+            if (m_net == NET_INTERNAL) {
    


    jonatack commented at 3:47 pm on September 15, 2020:

    80c77c31

    0            if (IsInternal()) {
    

    vasild commented at 7:42 pm on September 15, 2020:
    Done.
  58. jonatack commented at 3:58 pm on September 15, 2020: member

    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

  59. vasild force-pushed on Sep 15, 2020
  60. jonatack commented at 8:13 am on September 16, 2020: member
    Code review re-ACK per git diff 8d2b60c 3eef1e6
  61. in src/netaddress.h:245 in fdfa3c6d4b outdated
    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
    


    Sjors commented at 1:19 pm on September 16, 2020:

    Maybe add:

    returns false for unrecognised (future) network ids, which must be ignored (BIP155).


    vasild commented at 1:23 pm on September 17, 2020:
  62. in src/test/net_tests.cpp:409 in fdfa3c6d4b outdated
    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();
    


    Sjors commented at 2:27 pm on September 16, 2020:
    Maybe add a test where address is shorter than the announced address length (which itself is valid). Is this code covered by fuzzing?

    vasild commented at 1:30 pm on September 17, 2020:
  63. in src/test/net_tests.cpp:540 in fdfa3c6d4b outdated
    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.
    


    Sjors commented at 2:34 pm on September 16, 2020:
    Suggest adding a test for unknown with reasonable length.

    vasild commented at 1:32 pm on September 17, 2020:
  64. in src/test/net_tests.cpp:363 in fdfa3c6d4b outdated
    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");
    


    Sjors commented at 2:42 pm on September 16, 2020:

    I get that:

    1. 0210 indicates IPv6 (in which the internal is embedded)
    2. 0xFD + sha256("bitcoin")[0:5] is the prefix for internal, which explains fd6b88c08724

    But how is the rest (ca978112ca1bbdcafac2) generated? Might be worth a comment. Either way, it matches the pre-existing v1 test.


  65. in src/netaddress.cpp:41 in 3eef1e65c2 outdated
    37+        case ADDR_TORV2_SIZE:
    38+            return BIP155Network::TORV2;
    39+        case ADDR_TORV3_SIZE:
    40+            return BIP155Network::TORV3;
    41+        default:
    42+            assert(false);
    


    Sjors commented at 2:50 pm on September 16, 2020:

    This needs a strong warning:

    0// length should have been checked before calling this function
    

    vasild commented at 2:06 pm on September 17, 2020:
    The length cannot be something else than 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.

    sipa commented at 8:13 pm on September 18, 2020:

    // 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”.

  66. Sjors approved
  67. Sjors commented at 3:30 pm on September 16, 2020: member

    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.

  68. pinheadmz commented at 4:35 pm on September 16, 2020: member
    concept ACK, tested ACK: Built and tested unit and functional on OSX 10.14 Was able to addnode onionV3 addresses and connect after launching with -proxy=127.0.0.1:9050 -onlynet=onion. Reviewing code next…
  69. in src/test/util/setup_common.h:161 in 6ab0a741ab outdated
    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 {
    


    fjahr commented at 8:39 pm on September 16, 2020:
    nit: HasReason is a class but the commit message is talking about moving a function.

    vasild commented at 2:07 pm on September 17, 2020:
    Fixed!
  70. in src/netaddress.cpp:56 in fdfa3c6d4b outdated
    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)
    


    fjahr commented at 8:54 pm on September 16, 2020:

    nit

    0bool CNetAddr::SetNetFromBIP155Network(const uint8_t possible_bip155_net, const size_t address_size)
    

    jonatack commented at 9:54 pm on September 16, 2020:

    fjahr commented at 11:23 pm on September 16, 2020:
    Then I disagree with that guideline :) Using const in function arguments unlocks potential compiler optimizations and clarifies the code of that function. Not doing it just to ensure that people are not confused when they afterward encounter another function that does not follow that practice seems backward. We are also already using const in function args in many places of the codebase, so “rarely declared const” doesn’t apply.

    fjahr commented at 11:37 pm on September 16, 2020:
    Anyway, it’s just a nit so feel free to ignore obviously.

    jonatack commented at 5:04 am on September 17, 2020:
    Yes, fair points. Curious what people think.

    vasild commented at 2:24 pm on September 17, 2020:

    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.


    sipa commented at 5:24 pm on September 18, 2020:

    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.

  71. in src/util/strencodings.cpp:204 in 3eef1e65c2 outdated
    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)
    


    fjahr commented at 9:28 pm on September 16, 2020:

    nit: same with the string one below

    0std::string EncodeBase32(Span<const unsigned char> input, const bool pad)
    

    vasild commented at 2:25 pm on September 17, 2020:
    Lets have the conversation in just one place: #19845 (review)
  72. fjahr commented at 9:42 pm on September 16, 2020: member

    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.

  73. test: move HasReason so it can be reused
    Move the class `HasReason` from `miner_tests.cpp` to
    `setup_common.h` so that it can be reused by other tests.
    fe42411b4b
  74. vasild force-pushed on Sep 17, 2020
  75. vasild commented at 12:58 pm on September 17, 2020: member

    Note that the ADDRV2_FORMAT constant 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?

  76. vasild commented at 2:32 pm on September 17, 2020: member

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

    List of modified and not covered lines.

  77. in src/netaddress.h:267 in 65a8d3199d outdated
    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
    


    jonatack commented at 4:14 pm on September 17, 2020:
    “not unrecognised” -> “not recognised” or “unrecognised”

    vasild commented at 8:17 pm on September 17, 2020:
    Fixed.
  78. jonatack commented at 6:09 pm on September 17, 2020: member

    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.

  79. net: CNetAddr: add support to (un)serialize as ADDRv2
    Co-authored-by: Carl Dong <contact@carldong.me>
    e0d73573a3
  80. vasild force-pushed on Sep 17, 2020
  81. vasild commented at 8:49 am on September 18, 2020: member
    Should be all good for re-review.
  82. jonatack commented at 9:52 am on September 18, 2020: member
    re-ACK b9c46e0a935a14063913f0c999922f8e22bce61d per git diff 65a8d31 b9c46e0
  83. vasild force-pushed on Sep 18, 2020
  84. in src/netaddress.cpp:201 in b9c46e0a93 outdated
    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";
    


    sipa commented at 6:58 pm on September 18, 2020:

    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.


    vasild commented at 8:23 am on September 21, 2020:

    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.

  85. in src/netaddress.h:391 in b9c46e0a93 outdated
    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;
    


    eriknylund commented at 7:50 pm on September 18, 2020:
    I don’t see the 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?

    sipa commented at 8:00 pm on September 18, 2020:
    It’s a member variable of CNetAddr, so yes, it is used later.
  86. sipa commented at 8:22 pm on September 18, 2020: member

    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.

  87. in src/netaddress.cpp:197 in b9c46e0a93 outdated
    193@@ -101,24 +194,82 @@ bool CNetAddr::SetInternal(const std::string &name)
    194     return true;
    195 }
    196 
    197+namespace torv3 {
    


    eriknylund commented at 9:12 pm on September 18, 2020:
    To my (limited) understanding the use of namespaces are rare in the project, is there a specific reason it has to be used here?

    vasild commented at 8:36 am on September 21, 2020:

    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().


    eriknylund commented at 9:15 am on September 21, 2020:
    I completely agree, I think it makes things more contained, well structured and easier to comprehend. It seems to me a lot of new code would benefit from namespacing and perhaps worth considering when touching old code as well?
  88. in src/util/strencodings.cpp:211 in b9c46e0a93 outdated
    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) {
    


    eriknylund commented at 9:36 pm on September 18, 2020:
    I have improved the test for this method to cover both branches. Is that something worth having in this PR already? Here’s the diff and I’m happy to adjust if there’s a better way to test it https://gist.github.com/eriknylund/cd2bcaf9c3b4ada8a304001c2df0bfbd

    vasild commented at 8:16 am on September 21, 2020:
    Added the test, thanks!
  89. net: recognize TORv3/I2P/CJDNS networks
    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>
    7be6ff6187
  90. vasild force-pushed on Sep 21, 2020
  91. vasild commented at 8:19 am on September 21, 2020: member

    Added no-padding tests for EncodeBase32() by @eriknylund and a simplification around torv3::VERSION suggested by @sipa.

    Thanks!

  92. Sjors commented at 2:45 pm on September 21, 2020: member
    re-tACK 7be6ff61875a8d5d2335bff5d1f16ba40557adb0
  93. eriknylund approved
  94. eriknylund commented at 6:32 pm on September 22, 2020: contributor
    ACK 7be6ff61875a8d5d2335bff5d1f16ba40557adb0 I built the PR on macOS Catalina 10.15.6, ran both tests and functional tests. I’ve reviewed the code and think the changes look good and according to BIP155. I verified that the added Base32 encoding test looks as proposed and working. I’ve run a node for a week only with Onion addresses -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.
  95. in src/test/base32_tests.cpp:22 in 7be6ff6187
    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]);
    


    jonatack commented at 3:00 pm on September 24, 2020:

    Sanity-checked that the new test fails with

    0-        strEnc = EncodeBase32(vstrIn[i], false);
    1+        strEnc = EncodeBase32(vstrIn[i], true);
    
  96. jonatack commented at 3:15 pm on September 24, 2020: member
    re-ACK 7be6ff61875a8d5d2335bff5d1f16ba40557adb0 per 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.
  97. sipa commented at 2:47 am on September 26, 2020: member
    re-utACK 7be6ff61875a8d5d2335bff5d1f16ba40557adb0
  98. in src/test/util/setup_common.h:170 in fe42411b4b outdated
    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+};
    


    hebasto commented at 7:16 pm on September 26, 2020:

    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};
    

    vasild commented at 7:10 am on September 28, 2020:
    Right, I should deploy some automatic pre-push clang-format here. Not worth invalidating ACKs though.

    vasild commented at 10:17 am on September 29, 2020:
  99. in src/test/util/setup_common.h:164 in fe42411b4b outdated
    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>
    


    hebasto commented at 7:22 pm on September 26, 2020:

    fe42411b4b07b99c591855f5f00ad45dfeec8e30

    Why introduce a template in “move” commit? This pull compiles with the previous std::runtime_error parameter.


    vasild commented at 7:09 am on September 28, 2020:
    We are now passing 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.
  100. in src/netaddress.h:32 in e0d73573a3 outdated
    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;
    


    hebasto commented at 7:32 pm on September 26, 2020:

    e0d73573a37bf4b519f6f61e5678572d48a64517

    nit:

    0static constexpr int ADDRV2_FORMAT = 0x20000000;
    

    vasild commented at 7:12 am on September 28, 2020:
    Right! But I will leave it as is, because of the amount of ACKs it is buried under.

    vasild commented at 10:17 am on September 29, 2020:
  101. hebasto approved
  102. hebasto commented at 9:17 pm on September 26, 2020: member

    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.

  103. sipa merged this on Sep 28, 2020
  104. sipa closed this on Sep 28, 2020

  105. sipa commented at 7:28 pm on September 28, 2020: member
    @hebasto I edited your ACK comment above to avoid having the @jonatack highlight and your onion address in the merge commit.
  106. sidhujag referenced this in commit 06cbbae452 on Sep 29, 2020
  107. fanquake removed this from the "Blockers" column in a project

  108. vasild deleted the branch on Sep 29, 2020
  109. vasild referenced this in commit 7db9bf3225 on Sep 29, 2020
  110. vasild referenced this in commit b76317461e on Oct 31, 2020
  111. vasild referenced this in commit 89836a82ee on Oct 31, 2020
  112. laanwj referenced this in commit 79a3b59cc7 on Nov 9, 2020
  113. sidhujag referenced this in commit 55fb188668 on Nov 9, 2020
  114. MarcoFalke referenced this in commit 2fa085a5d7 on Nov 16, 2020
  115. sidhujag referenced this in commit 806ac5fe66 on Nov 16, 2020
  116. janus referenced this in commit 5e6fe52c90 on Dec 13, 2020
  117. deadalnix referenced this in commit 7af7f84992 on Feb 9, 2021
  118. deadalnix referenced this in commit c78ecba784 on Feb 9, 2021
  119. deadalnix referenced this in commit 3705100b63 on Feb 9, 2021
  120. kittywhiskers referenced this in commit b5f18cefdd on May 25, 2021
  121. kittywhiskers referenced this in commit 42872e53b8 on May 25, 2021
  122. kittywhiskers referenced this in commit 41ed5a841a on May 25, 2021
  123. kittywhiskers referenced this in commit 7770341841 on May 25, 2021
  124. UdjinM6 referenced this in commit beac6df320 on May 28, 2021
  125. UdjinM6 referenced this in commit 9d4f908e8b on May 28, 2021
  126. UdjinM6 referenced this in commit 20bd3017b9 on May 29, 2021
  127. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  128. Fabcien referenced this in commit c50d2ac071 on Dec 22, 2021
  129. Fabcien referenced this in commit df99eb6da0 on Dec 23, 2021
  130. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me