Complete the BIP155 implementation and upgrade to TORv3 #19954

pull vasild wants to merge 4 commits into bitcoin:master from vasild:torv3 changing 19 files +596 −48
  1. vasild commented at 1:46 pm on September 14, 2020: member

    This PR contains the two remaining commits from #19031 to complete the BIP155 implementation:

    net: CAddress & CAddrMan: (un)serialize as ADDRv2 net: advertise support for ADDRv2 via new message

    plus one more commit:

    tor: make a TORv3 hidden service instead of TORv2

  2. vasild commented at 1:46 pm on September 14, 2020: member
    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.
  3. fanquake added the label P2P on Sep 14, 2020
  4. vasild force-pushed on Sep 14, 2020
  5. in src/torcontrol.cpp:483 in 9c9ff0a8c4 outdated
    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);
    


    jonatack commented at 3:33 pm on September 14, 2020:

    Linter fails on missing newline here.

    0$ test/lint/lint-logs.sh
    1All calls to LogPrintf() and LogPrint() should be terminated with \n
    

    vasild commented at 12:45 pm on September 18, 2020:
    Deleted this code altogether.
  6. jonatack commented at 3:35 pm on September 14, 2020: member

    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.

  7. in src/protocol.h:81 in a1b067e089 outdated
    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
    


    jonatack commented at 4:13 pm on September 14, 2020:
    nit: gossiping

    vasild commented at 12:44 pm on September 18, 2020:
    Fixed.
  8. in src/protocol.h:89 in a1b067e089 outdated
    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).
    


    jonatack commented at 4:15 pm on September 14, 2020:
    s/who has indicated ADDRv2 support (has sent SENDADDRV2 himself)/that has signaled ADDRv2 support by sending SENDADDRv2./

    vasild commented at 12:44 pm on September 18, 2020:
    Changed.
  9. jonatack commented at 4:18 pm on September 14, 2020: member
    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.
  10. ghost commented at 5:45 pm on September 14, 2020: none

    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?

  11. sipa commented at 5:47 pm on September 14, 2020: member

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

  12. Saibato commented at 6:32 pm on September 14, 2020: contributor

    @prayank23

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

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

  13. in doc/files.md:60 in 9c9ff0a8c4 outdated
    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.
    


    laanwj commented at 6:16 pm on September 15, 2020:

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


    vasild commented at 12:45 pm on September 18, 2020:
    Indeed. Done!
  14. laanwj commented at 6:18 pm on September 15, 2020: member

    @prayank23

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

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

  15. Saibato commented at 7:29 pm on September 15, 2020: contributor

    @laanwj

    I’d slightly prefer creating a new file for v3 instead @vasild Feel free to grab there, then i can close that PR

    #19485

  16. tryphe commented at 7:05 am on September 16, 2020: contributor
    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!
  17. Sjors commented at 11:52 am on September 17, 2020: member

    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.

  18. vasild force-pushed on Sep 18, 2020
  19. vasild force-pushed on Sep 18, 2020
  20. vasild commented at 12:59 pm on September 18, 2020: member

    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.

  21. jonatack commented at 6:15 pm on September 18, 2020: member
    Practical testing of this PR with various Tor configuration options by and between @vasild and I today. Will review this weekend.
  22. DrahtBot commented at 1:33 pm on September 19, 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:

    • #19889 (test: Add consistent formatting for messages.py repr methods by adaminsky)
    • #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
    • #19238 (refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto)
    • #18788 (tests: Update more tests to work with descriptor wallets by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  23. in src/torcontrol.cpp:535 in 1550aa8265 outdated
    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
    


    Sjors commented at 2:49 pm on September 21, 2020:
    nit: add {

    vasild commented at 3:12 pm on September 28, 2020:
    Done
  24. Sjors commented at 3:19 pm on September 21, 2020: member

    tACK 1550aa82659506ef89db67e482d01f014f6a1977 just the last commit

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

  25. laanwj commented at 12:11 pm on September 23, 2020: member

    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.

  26. in doc/files.md:59 in 1550aa8265 outdated
    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.
    


    laanwj commented at 12:19 pm on September 23, 2020:
    Maybe move this line to ## Legacy subdirectories and files, if we’re no longer going to spin up a v2 service.

    jonatack commented at 1:14 pm on September 25, 2020:
    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)

    vasild commented at 3:13 pm on September 28, 2020:
    Done.

    vasild commented at 3:21 pm on September 28, 2020:
    Done - “replaced by …”
  27. in src/crypto/common.h:60 in 1550aa8265 outdated
    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);
    


    laanwj commented at 12:34 pm on September 23, 2020:
    Good, looks ilke we already have an implementation of be16toh in src/compat/endian.h, so it doesn’t need to be added.
  28. in doc/release-notes.md:83 in 1550aa8265 outdated
    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`.
    


    jonatack commented at 11:44 am on September 25, 2020:

    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)


    vasild commented at 3:15 pm on September 28, 2020:

    Done.

    cp -f onion_private_key onion_private_key_torv3 is clearer? :)


    jonatack commented at 2:50 pm on September 30, 2020:
    LGTM
  29. in src/addrman.h:334 in 1550aa8265 outdated
    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) {
    


    jonatack commented at 11:50 am on September 25, 2020:

    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


    vasild commented at 3:18 pm on September 28, 2020:

    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.


    jonatack commented at 2:51 pm on September 30, 2020:
    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.
  30. in src/net_processing.cpp:2609 in 1550aa8265 outdated
    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()));
    


    jonatack commented at 12:49 pm on September 25, 2020:
    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?

    vasild commented at 3:20 pm on September 28, 2020:

    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.


    jonatack commented at 3:10 pm on September 30, 2020:
    Nice work on the tests!
  31. in src/net_processing.cpp:2640 in 1550aa8265 outdated
    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;
    


    jonatack commented at 12:52 pm on September 25, 2020:
    99047b0 covered by a test?

    vasild commented at 3:20 pm on September 28, 2020:
    Yes! (since the last push with updated tests)
  32. in src/protocol.h:81 in 1550aa8265 outdated
    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
    


    jonatack commented at 1:10 pm on September 25, 2020:
    99047b07 nit, maybe “gossiping of longer node addresses up to 512 bytes”

    vasild commented at 3:21 pm on September 28, 2020:
    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.
  33. jonatack commented at 1:26 pm on September 25, 2020: member

    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.

  34. in src/net_processing.cpp:2654 in 1550aa8265 outdated
    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;
    


    jonatack commented at 4:55 pm on September 25, 2020:
    Had a look at if this needs a lock; it doesn’t seem so.
  35. jonatack commented at 5:08 pm on September 25, 2020: member

    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.

  36. vasild commented at 8:07 am on September 28, 2020: member

    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.

  37. vasild force-pushed on Sep 28, 2020
  38. vasild commented at 3:09 pm on September 28, 2020: member
    Rebase on latest #19031
  39. vasild force-pushed on Sep 28, 2020
  40. vasild commented at 3:10 pm on September 28, 2020: member
    Address review suggestions.
  41. vasild force-pushed on Sep 28, 2020
  42. vasild commented at 3:13 pm on September 28, 2020: member
    Address review suggestion.
  43. vasild force-pushed on Sep 29, 2020
  44. vasild marked this as ready for review on Sep 29, 2020
  45. jonatack commented at 8:56 am on September 29, 2020: member
    Ready to move this PR out of Draft status? 😀 Edit: snap!
  46. vasild commented at 8:58 am on September 29, 2020: member
    Opened for review. This PR supersedes #19031 and completes the TORv3 support.
  47. hebasto commented at 9:00 am on September 29, 2020: member
    Concept ACK.
  48. vasild renamed this:
    tor: make a TORv3 hidden service instead of TORv2
    tor: complete the TORv3 implementation
    on Sep 29, 2020
  49. in src/addrman.h:340 in e025121c1c outdated
    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;
    


    hebasto commented at 9:31 am on September 29, 2020:

    e025121c1ca4b86296c6b36386786522bd79a6e1

    0        s << static_cast<uint8_t>(m_format);
    

    vasild commented at 7:49 am on September 30, 2020:
    Done

    jonatack commented at 2:52 pm on September 30, 2020:

    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.

    0         s << static_cast<uint8_t>(m_format);
    1-        s << ((unsigned char)32);
    2+        s << static_cast<unsigned char>(32);
    

    vasild commented at 1:03 pm on October 2, 2020:
    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.

    jonatack commented at 1:13 pm on October 2, 2020:
    That’s fair.
  50. in src/addrman.h:404 in e025121c1c outdated
    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;
    


    hebasto commented at 9:36 am on September 29, 2020:

    e025121c1ca4b86296c6b36386786522bd79a6e1

    0        m_format = static_cast<Format>(format_tmp);
    

    vasild commented at 7:49 am on September 30, 2020:
    Done
  51. hebasto changes_requested
  52. hebasto commented at 9:48 am on September 29, 2020: member

    Approach ACK ff76c3444c574b14a2c62999b82cba812816884b.

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

  53. vasild force-pushed on Sep 30, 2020
  54. vasild commented at 7:49 am on September 30, 2020: member
    Addressed suggestions by @hebasto - always compare m_format with >= and use static_cast instead of C-style cast.
  55. vasild commented at 10:34 am on September 30, 2020: member

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

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

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

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

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

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

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

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

    • This has the downside of garbage gossip traffic. Is it any worse than somebody gossiping garbage IPv6 addresses? Honest nodes don’t have a way to verify if there is a node listening on a I2P address (yet). But even now we don’t verify if there is a node listening on an address before relaying it.
    • The benefit is that once I2P support is added in version 0.2X then older nodes, e.g. 0.21 will relay I2P addresses and help with propagation.
     0--- i/src/net_processing.cpp
     1+++ w/src/net_processing.cpp
     2@@ -2643,14 +2643,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
     3             pfrom.AddAddressKnown(addr);
     4             if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) {
     5                 // Do not process banned/discouraged addresses beyond remembering we received them
     6                 continue;
     7             }
     8             bool fReachable = IsReachable(addr);
     9-            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
    10-            {
    11+            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 &&
    12+                addr.IsRoutable() && !addr.IsI2P() && !addr.IsCJDNS()) {
    13                 // Relay to a limited number of other nodes
    14                 RelayAddress(addr, fReachable, m_connman);
    15             }
    16             // Do not store addresses outside our network
    17             if (fReachable)
    18                 vAddrOk.push_back(addr);
    

    What are your thoughts?

  56. laanwj commented at 11:40 am on September 30, 2020: member

    @vasild

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

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

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

    Seems good to me too.

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

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

  57. in doc/files.md:101 in bb01c77865 outdated
     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)
    


    jonatack commented at 2:57 pm on September 30, 2020:

    Missing column separator |?

    0`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)
    

    vasild commented at 12:48 pm on October 2, 2020:
    Fixed
  58. in doc/release-notes.md:84 in bb01c77865 outdated
    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`).
    


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

    (code) markup inside parentheses; maybe easier to read as:

    0  , e.g. `cp -f onion_private_key onion_private_key_torv3`.
    

    vasild commented at 12:48 pm on October 2, 2020:
    Done
  59. in test/functional/p2p_addr_relay.py:13 in bb01c77865 outdated
     9@@ -10,7 +10,7 @@
    10     CAddress,
    11     NODE_NETWORK,
    12     NODE_WITNESS,
    13-    msg_addr,
    14+    msg_addrv2,
    


    jonatack commented at 3:03 pm on September 30, 2020:
    Should we test for both msg_addr and msg_addrv2?

    vasild commented at 12:51 pm on October 2, 2020:

    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.

  60. jonatack commented at 3:16 pm on September 30, 2020: member

    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.

  61. hebasto commented at 11:09 am on October 1, 2020: member

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

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

    Setup A – master-peer connects to pr-node

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

    Setup B – pr-peer connects to pr-node

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

  62. hebasto commented at 1:20 pm on October 1, 2020: member

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

  63. in src/net_processing.cpp:2418 in 76afce803b outdated
    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));
    


    hebasto commented at 3:20 pm on October 1, 2020:

    76afce803bb5309a69517df5eac68529b7af2d29

    0        m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::SENDADDRv2));
    

    vasild commented at 12:51 pm on October 2, 2020:
    Done
  64. in src/torcontrol.cpp:537 in bb01c77865 outdated
    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+        }
    


    hebasto commented at 3:32 pm on October 1, 2020:

    bb01c77865a565764e4e2f86265afef45506d7d7

    The comment about #9214 seems outdated now:

    0        if (private_key.empty()) {
    1            // No private key, generate one.
    2            private_key = "NEW:ED25519-V3";
    3        }
    

    vasild commented at 12:55 pm on October 2, 2020:

    The comment is still relevant - we want to explicitly request the key type in order to avoid problems like #9214 in the future.

    In other words - we don’t want somebody to change this to NEW:BEST because if/when Tor upgrades to v4 then we may have #9214 again.


    hebasto commented at 1:36 pm on October 2, 2020:
    Correct.
  65. in doc/release-notes.md:77 in bb01c77865 outdated
    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
    


    hebasto commented at 3:35 pm on October 1, 2020:

    bb01c77865a565764e4e2f86265afef45506d7d7

    See #19638:

    0- The Tor onion service that is automatically created (`-listenonion`) will now
    

    vasild commented at 12:51 pm on October 2, 2020:
    Changed
  66. hebasto approved
  67. hebasto commented at 3:38 pm on October 1, 2020: member

    ACK bb01c77865a565764e4e2f86265afef45506d7d7

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

  68. laanwj commented at 11:47 am on October 2, 2020: member

    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.

  69. vasild force-pushed on Oct 2, 2020
  70. vasild force-pushed on Oct 2, 2020
  71. vasild commented at 1:00 pm on October 2, 2020: member
    Addressed review suggestions. Notice - I restored the original version of p2p_addr_relay.py and the test for addrv2 is now in a new test p2p_addrv2_relay.py.
  72. jonatack commented at 1:15 pm on October 2, 2020: member

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

    Nice!

  73. hebasto approved
  74. hebasto commented at 1:34 pm on October 2, 2020: member
    re-ACK c212c7eea6c0f67d7c65467c6c93f188aaeb02f3, only suggested changes since my previous review.
  75. jonatack commented at 11:07 am on October 3, 2020: member

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

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


    jonatack commented at 11:16 am on October 3, 2020:
    0    return GetDataDir() / "onion_v3_private_key";
    

    vasild commented at 1:12 pm on October 5, 2020:
    Done
  77. in test/functional/p2p_invalid_messages.py:30 in c212c7eea6 outdated
    26+from test_framework.util import (
    27+    assert_equal,
    28+    hex_str_to_bytes,
    29+)
    30+import struct
    31+import time
    


    jonatack commented at 11:18 am on October 3, 2020:
    nit: IIRC, per PEP8, OS-level and stdlib imports go at the top.

    vasild commented at 1:12 pm on October 5, 2020:
    Done
  78. jonatack commented at 11:34 am on October 3, 2020: member

    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.

  79. in src/net_processing.cpp:2640 in c212c7eea6 outdated
    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;
    


    jonatack commented at 11:35 am on October 3, 2020:

    Should skipping invalid BIP155 addresses be applied only for addrv2 messages?

    0if (!addr.IsValid() && msg_type == NetMsgType::ADDRv2) {
    1    continue;
    2}
    

    vasild commented at 1:15 pm on October 5, 2020:
    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.
  80. in src/addrman.h:269 in 31876b3389 outdated
    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
    


    sipa commented at 10:29 pm on October 4, 2020:
    How about giving these a bit more descriptive names, like V0_HISTORICAL, V1_DETERMINISTIC, V2_ASMAP, V3_BIP155?

    vasild commented at 1:16 pm on October 5, 2020:
    Renamed
  81. in src/addrman.h:337 in 31876b3389 outdated
    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);
    


    sipa commented at 10:31 pm on October 4, 2020:
    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.

    vasild commented at 1:17 pm on October 5, 2020:

    Right, changed to OverrideStream.

    #19503 is also an alternative if/when it gets merged.

  82. vasild force-pushed on Oct 5, 2020
  83. vasild commented at 1:20 pm on October 5, 2020: member
    Addressed review suggestions.
  84. in src/addrman.h:392 in bbbc8e4c65 outdated
    389@@ -371,13 +390,25 @@ friend class CAddrManTest;
    390     }
    391 
    392     template<typename Stream>
    


    hebasto commented at 2:04 pm on October 5, 2020:

    pico-nit:

    0    template <typename Stream>
    

    vasild commented at 2:41 pm on October 5, 2020:
    Done
  85. in src/addrman.h:272 in bbbc8e4c65 outdated
    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
    


    hebasto commented at 2:07 pm on October 5, 2020:

    pico-nit, as clang-format suggests, and s/V2/V2_ASMAP/ in comment:

    0        V0_HISTORICAL = 0,    //!< historic format, before commit e6b343d88
    1        V1_DETERMINISTIC = 1, //!< for pre-asmap files
    2        V2_ASMAP = 2,         //!< for files including asmap version
    3        V3_BIP155 = 3,        //!< same as V2_ASMAP plus addresses are in BIP155 format
    

    vasild commented at 2:41 pm on October 5, 2020:
    Done
  86. in src/hash.h:106 in bbbc8e4c65 outdated
    101@@ -102,13 +102,14 @@ class CHashWriter
    102     CSHA256 ctx;
    103 
    104     const int nType;
    105-    const int nVersion;
    106+    int nVersion;
    107 public:
    


    hebasto commented at 2:08 pm on October 5, 2020:

    pico-nit:

    0    int nVersion;
    1
    2public:
    

    vasild commented at 2:41 pm on October 5, 2020:
    Done
  87. hebasto approved
  88. hebasto commented at 2:09 pm on October 5, 2020: member
    re-ACK bbbc8e4c657e8ea81edb6aca2b1870cd3b7459ad.
  89. hebasto commented at 2:17 pm on October 5, 2020: member
    I’d like that s and s_ objects of the Stream class were named more descriptive, but I have no particular suggestion.
  90. vasild force-pushed on Oct 5, 2020
  91. vasild commented at 2:41 pm on October 5, 2020: member
    Addressed review suggestions.
  92. hebasto approved
  93. hebasto commented at 3:10 pm on October 5, 2020: member

    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 !

  94. in src/hash.h:113 in 703db15ae1 outdated
    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; }
    


    jonatack commented at 4:11 pm on October 5, 2020:

    03572e9 After git diff c212c7e 703db15 using OverrideStream, it looks like we can now revert these changes in hash.h and streams.h.

     0+++ b/src/hash.h
     1@@ -102,7 +102,7 @@ private:
     2     const int nType;
     3-    int nVersion;
     4+    const int nVersion;
     5@@ -110,7 +110,6 @@ public:
     6     int GetVersion() const { return nVersion; }
     7-    void SetVersion(int n) { nVersion = n; }
     8
     9+++ b/src/streams.h
    10@@ -603,7 +603,7 @@ class CAutoFile
    11     const int nType;
    12-    int nVersion;
    13+    const int nVersion; 
    14@@ -651,7 +651,6 @@ public:
    15     int GetVersion() const       { return nVersion; }
    16-    void SetVersion(int n)       { nVersion = n; }
    

    vasild commented at 9:28 am on October 6, 2020:
    Good catch! Removed.
  95. in src/addrman.h:399 in 03572e94cb outdated
    398 
    399         Clear();
    400-        unsigned char nVersion;
    401-        s >> nVersion;
    402+
    403+        uint8_t format_tmp;
    


    sipa commented at 11:32 pm on October 5, 2020:
    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).

    vasild commented at 9:29 am on October 6, 2020:

    Used CustomUintFormatter here, but in Serialize it did not compile:

    0src/serialize.h:504:15: error: invalid operands to binary expression ('CAddrMan::Format' and 'int')
    1        if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
    2            ~ ^ ~
    

    so I left the static_cast there.

  96. in src/streams.h:654 in 03572e94cb outdated
    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; }
    


    sipa commented at 11:43 pm on October 5, 2020:
    This should be unnecessary as well.

    vasild commented at 9:29 am on October 6, 2020:
    Removed
  97. in src/addrman.h:276 in 03572e94cb outdated
    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;
    


    sipa commented at 11:50 pm on October 5, 2020:

    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.


    vasild commented at 9:32 am on October 6, 2020:

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


    hebasto commented at 9:33 am on October 6, 2020:
    Does this mean that peers.dat becomes unreadable if a user choose to downgrade his node at some point in the future?

    jonatack commented at 5:24 pm on October 6, 2020:

    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

    0ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error 
    1Invalid or missing peers.dat; recreating
    

    One, and sometimes two, restarts after rm onion_v3_private_key to return to v3

    0ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error 
    1Invalid or missing peers.dat; recreating
    

    Edit: verified that these messages are new in the debug log and were absent from testing of previous versions.


    vasild commented at 9:21 am on October 8, 2020:

    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.


    hebasto commented at 3:28 pm on October 8, 2020:
    Does this require mention in release notes?
  98. in src/net_processing.cpp:4140 in 44364147d7 outdated
    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));
    


    sipa commented at 1:11 am on October 6, 2020:
    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.

    vasild commented at 9:45 am on October 6, 2020:

    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:

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


    sipa commented at 5:15 pm on October 6, 2020:

    @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::ADDRv2 for msg_type you must also pass ADDRV2_FORMAT in nFlags”.

    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.


    vasild commented at 9:18 am on October 7, 2020:
    That is also a valid point. Changed.
  99. vasild force-pushed on Oct 6, 2020
  100. vasild commented at 9:47 am on October 6, 2020: member
    Addressed review suggestions and extended some tests.
  101. vasild commented at 9:53 am on October 6, 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.

  102. in src/net_processing.cpp:2604 in 9502ad1002 outdated
    2601         std::vector<CAddress> vAddr;
    2602-        vRecv >> vAddr;
    2603+
    2604+        try {
    2605+            s >> vAddr;
    2606+        } catch (const std::exception& e) {
    


    sipa commented at 10:57 pm on October 6, 2020:
    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.

    vasild commented at 9:19 am on October 7, 2020:
    I agree, removed the try/catch.
  103. in src/net_processing.cpp:2620 in 9502ad1002 outdated
    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",
    


    sipa commented at 11:02 pm on October 6, 2020:

    This can instead be:

    0Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
    

    As msg_type is just const char*.


    vasild commented at 9:21 am on October 7, 2020:
    Changed. Btw msg_type is std::string (not const char*). strprintf() takes it either way.
  104. in src/protocol.h:84 in 9502ad1002 outdated
    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;
    


    sipa commented at 11:03 pm on October 6, 2020:
    Style nit: constants are all caps, so ADDRV2

    vasild commented at 9:21 am on October 7, 2020:
    Renamed
  105. in src/protocol.h:90 in 9502ad1002 outdated
    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;
    


    sipa commented at 11:04 pm on October 6, 2020:
    Style nit: constants are all caps, so SENDADDRV2.

    vasild commented at 9:21 am on October 7, 2020:
    Renamed
  106. sipa commented at 1:36 am on October 7, 2020: member

    Code review ACK modulo nits.

    I tested:

    • externalip= with torv3 address, and saw it reported in logs
    • upgrading a automatically generated torv2 address to a v3 one
    • automatically creating a torv3 address
    • connecting two nodes with this patch to each other, and seeing addrv2 reported in getpeerinfo output
  107. vasild force-pushed on Oct 7, 2020
  108. vasild commented at 9:22 am on October 7, 2020: member
    Addressed review suggestions.
  109. laanwj added this to the milestone 0.21.0 on Oct 8, 2020
  110. laanwj commented at 11:22 am on October 8, 2020: member

    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.

  111. jonatack commented at 3:00 pm on October 8, 2020: member
    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.
  112. in src/streams.h:63 in 1f06a2901d outdated
    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); }
    


    jonatack commented at 3:48 pm on October 8, 2020:

    0e05e14

    0    void ignore(size_t size) const { return stream->ignore(size); }
    

    vasild commented at 8:12 am on October 9, 2020:
    ignore() modifies the state of the stream. It is not const in CDataStream and CAutoFile so this would not compile.

    jonatack commented at 8:45 am on October 9, 2020:
    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.

    jonatack commented at 9:07 am on October 9, 2020:

    “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)

    also: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#con2-by-default-make-member-functions-const


    vasild commented at 9:14 am on October 9, 2020:
    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.
  113. in doc/release-notes.md:92 in 1f06a2901d outdated
    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`.
    


    dongcarl commented at 3:50 pm on October 8, 2020:
    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…

    jonatack commented at 4:07 pm on October 8, 2020:
    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.

    laanwj commented at 7:04 am on October 9, 2020:
    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.

    vasild commented at 8:06 am on October 9, 2020:
    @laanwj, I agree, given torv2 addresses will cease in less than a year - no need to add new config options (or extend existent ones) only to deprecate them immediately.

    vasild commented at 9:12 am on October 9, 2020:
    Added a release notes mention.
  114. in src/netaddress.cpp:495 in 1f06a2901d outdated
    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+}
    


    jonatack commented at 3:59 pm on October 8, 2020:

    93c8f0ea suggestion (like GetBIP155Network)

    0-    }
    1-
    2-    return false;
    3+    } // no default case, so the compiler can warn about missing cases
    4+    assert(false);
    5 }
    

    vasild commented at 9:12 am on October 9, 2020:
    Done
  115. vasild commented at 4:05 pm on October 8, 2020: member

    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.

  116. dongcarl commented at 4:08 pm on October 8, 2020: member
    Code review ACK 1f06a2901d1ee924b95f482e8693842ee07863de
  117. jonatack commented at 4:22 pm on October 8, 2020: member
    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.
  118. laanwj commented at 7:06 am on October 9, 2020: member

    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)

  119. sipa commented at 7:08 am on October 9, 2020: member

    Earlier today on IRC:

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

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

  120. laanwj commented at 7:22 am on October 9, 2020: member

    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.

  121. sipa commented at 7:26 am on October 9, 2020: member

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

  122. jonatack commented at 7:33 am on October 9, 2020: member

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

    Yes, that’s what I’m seeing.

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

    :+1:

  123. Support bypassing range check in ReadCompactSize
    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.
    1d3ec2a1fd
  124. vasild force-pushed on Oct 9, 2020
  125. vasild commented at 9:20 am on October 9, 2020: member

    Addressed review suggestions plus

    • Added release notes on the peers.dat format change
    • Made it possible to read big compact-size numbers (thanks to @sipa for the patch) this will enable addrman to read arbitrary big service flags from its database and also for the p2p layer to receive them when in addrv2 encoding (the old addr encoding uses fixed 8-byte representation)
    • Added a human-readable error message if addrman detects a future format of peers.dat
  126. in src/serialize.h:29 in 9b56a68a85 outdated
    23@@ -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.
    


    jonatack commented at 9:23 am on October 9, 2020:

    1d3ec2a nit

    0 * (e.g. for vectors) when the size is encoded as CompactSize.
    

    vasild commented at 11:00 am on October 9, 2020:
    The dot terminates the doxygen title. dox1 dox2

    jonatack commented at 11:10 am on October 9, 2020:

    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.

  127. jonatack commented at 9:55 am on October 9, 2020: member

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

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


    jonatack commented at 10:22 am on October 9, 2020:

    6fce3ce maybe

    0                "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


    vasild commented at 2:49 pm on October 9, 2020:

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

  129. jonatack commented at 10:30 am on October 9, 2020: member

    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.

  130. harding commented at 12:27 pm on October 9, 2020: contributor

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

     0-The Tor onion service that is automatically created (`-listenonion`) will now be
     1+The Tor onion service that is automatically created by setting the `-listenonion` configuration parameter will now be
     2 created as a Tor v3 service instead of Tor v2. The private key that was used for
     3-Tor v2 (if any) will be left untouched in `onion_private_key` in `-datadir` and
     4+Tor v2 (if any) will be left untouched in the `onion_private_key` file in the data directory (see `-datadir`)  and
     5 can be removed if not needed. Bitcoin Core will no longer attempt to read it.
     6-The private key for the Tor v3 service will be saved in `onion_v3_private_key`.
     7+The private key for the Tor v3 service will be saved in a file named `onion_v3_private_key`.
     8 To use the deprecated Tor v2 service (not recommended), then `onion_private_key`
     9 can be copied over `onion_v3_private_key`, e.g. `cp -f onion_private_key
    10 onion_v3_private_key`. (#19954)
    
  131. jonatack commented at 12:32 pm on October 9, 2020: member
    Thanks @harding!
  132. net: CAddress & CAddrMan: (un)serialize as ADDRv2
    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>
    201a4596d9
  133. net: advertise support for ADDRv2 via new message
    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>
    353a3fdaad
  134. tor: make a TORv3 hidden service instead of TORv2
    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
    dcf0cb4776
  135. vasild force-pushed on Oct 9, 2020
  136. vasild commented at 2:46 pm on October 9, 2020: member
    Did some rewording of the release notes, mostly as suggested.
  137. Sjors commented at 5:43 pm on October 9, 2020: member
    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.
  138. vasild renamed this:
    tor: complete the TORv3 implementation
    Complete the BIP155 implementation and upgrade to TORv3
    on Oct 9, 2020
  139. in src/addrman.h:405 in dcf0cb4776
    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) {
    


    ariard commented at 11:54 pm on October 9, 2020:
    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.

    vasild commented at 12:07 pm on October 10, 2020:

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

  140. in src/net.h:873 in dcf0cb4776
    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.
    


    ariard commented at 0:17 am on October 10, 2020:

    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 :

    • MUST use addrv2 messages to gossip addresses
    • MUST accept addrv2messages
    • MAY accept addr messages

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


    vasild commented at 12:23 pm on October 10, 2020:

    Do you mean this?

    Once B receives sendaddrv2 from A:

    • B MUST use addrv2 messages to gossip addresses to A
    • B MUST accept addrv2 messages from A
    • B MAY accept addr messages from A

    The current PR accepts both addr and addrv2 no matter whether sendaddrv2 has been sent or received.


    ariard commented at 2:39 pm on October 10, 2020:

    Do you mean this?

    Yes, I think the BIP should state this clearly to hint other implementations. The current PR code is correct.

  141. in src/net.h:1126 in dcf0cb4776
    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();
    


    ariard commented at 0:40 am on October 10, 2020:

    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.


    vasild commented at 12:52 pm on October 10, 2020:
    PushAddress() is called from 5 places. We don’t want to duplicate the same code in 5 places.

    ariard commented at 2:38 pm on October 10, 2020:
    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.

    jnewbery commented at 11:48 am on October 12, 2020:
    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.
  142. in src/net_processing.cpp:2596 in dcf0cb4776
    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;
    


    ariard commented at 0:58 am on October 10, 2020:

    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.


    vasild commented at 12:48 pm on October 10, 2020:

    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.


    ariard commented at 2:43 pm on October 10, 2020:

    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.


    sipa commented at 4:53 pm on October 10, 2020:

    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.

  143. ariard commented at 1:10 am on October 10, 2020: member

    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.

  144. hebasto approved
  145. hebasto commented at 1:21 pm on October 10, 2020: member
    re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed.
  146. sipa commented at 5:36 pm on October 10, 2020: member

    ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5

    Two small follow-ups (not blockers here):

    • Mention BIP155 support in doc/bips.md
    • Don’t relay I2P/CJDNS in RelayAddress (until there is a sizable network that actually supports it)
  147. jonatack commented at 6:14 pm on October 10, 2020: member

    re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 per git diff 9b56a68 dcf0cb4 only change since last review is an update to the release notes which partially picked up the suggested text. Running a node on this branch and addnode-ing to 6 other Tor v3 nodes, I see “addrv2” and “sendaddrv2” messages in getpeerinfo in both the “bytesrecv_per_msg” and “bytessent_per_msg” JSON objects.

     0    "bytessent_per_msg": {
     1      "addrv2": 132,
     2      "feefilter": 32,
     3      "getaddr": 24,
     4      "getdata": 3918,
     5      "getheaders": 1053,
     6      "headers": 25,
     7      "inv": 30017,
     8      "ping": 96,
     9      "pong": 96,
    10      "sendaddrv2": 24,
    11      "sendcmpct": 66,
    12      "sendheaders": 24,
    13      "verack": 24,
    14      "version": 127,
    15      "wtxidrelay": 24
    16    },
    17    "bytesrecv_per_msg": {
    18      "addrv2": 17444,
    19      "feefilter": 32,
    20      "getheaders": 1053,
    21      "headers": 106,
    22      "inv": 16996,
    23      "notfound": 61,
    24      "ping": 96,
    25      "pong": 96,
    26      "sendaddrv2": 24,
    27      "sendcmpct": 66,
    28      "sendheaders": 24,
    29      "tx": 39740,
    30      "verack": 24,
    31      "version": 127,
    32      "wtxidrelay": 24
    33    },
    
  148. ariard commented at 0:04 am on October 11, 2020: member
    Code Review ACK dcf0cb4
  149. fanquake merged this on Oct 11, 2020
  150. fanquake closed this on Oct 11, 2020

  151. vasild deleted the branch on Oct 11, 2020
  152. in src/test/netbase_tests.cpp:465 in dcf0cb4776
    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 */),
    


    MarcoFalke commented at 6:56 am on October 12, 2020:
     0+ make -C src --jobs=1 check-security V=1
     1make: Entering directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
     2Checking binary security...
     3READELF=/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  
     4make: Leaving directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
     5+ case "$HOST" in
     6+ make -C src --jobs=1 check-symbols V=1
     7make: Entering directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
     8Checking glibc back compat...
     9READELF=/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  
    10test/test_bitcoin: export of symbol in6addr_loopback not allowed
    11test/test_bitcoin: failed EXPORTED_SYMBOLS
    12make: *** [Makefile:20952: check-symbols] Error 1
    13make: Leaving directory '/bitcoin/distsrc-x86_64-linux-gnu/src'
    

    MarcoFalke commented at 6:57 am on October 12, 2020:
    This breaks the guix/gitian build. cc @fanquake

    vasild commented at 7:35 am on October 12, 2020:

    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:

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

    sipa commented at 7:37 am on October 12, 2020:
    See #20127.
  153. vasild commented at 12:04 pm on October 12, 2020: member

    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!

  154. in doc/release-notes.md:69 in dcf0cb4776
    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)
    


    jnewbery commented at 11:06 am on November 1, 2020:

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

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


    vasild commented at 2:42 pm on November 1, 2020:

    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

     0diff --git i/src/addrman.h w/src/addrman.h
     1index b4089dc89..9f6614cd1 100644
     2--- i/src/addrman.h
     3+++ w/src/addrman.h
     4@@ -326,19 +326,19 @@ public:
     5     void Serialize(Stream& s_) const
     6     {
     7         LOCK(cs);
     8 
     9         // Always serialize in the latest version (currently Format::V3_BIP155).
    10 
    11         OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
    12 
    13         s << static_cast<uint8_t>(Format::V3_BIP155);
    14-        s << ((unsigned char)32);
    15+        s << ((unsigned char)33);
    16         s << nKey;
    17         s << nNew;
    18         s << nTried;
    19 
    20         int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
    21         s << nUBuckets;
    22         std::map<int, int> mapUnkIds;
    23         int nIds = 0;
    24         for (const auto& entry : mapInfo) {
    25@@ -406,18 +406,21 @@ public:
    26             // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
    27             // unserialize methods know that an address in addrv2 format is coming.
    28             stream_version |= ADDRV2_FORMAT;
    29         }
    30 
    31         OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
    32 
    33         unsigned char nKeySize;
    34         s >> nKeySize;
    35+        if (format >= Format::V3_BIP155) {
    36+            --nKeySize;
    37+        }
    38         if (nKeySize != 32) throw std::ios_base::failure("Incorrect keysize in addrman deserialization");
    39         s >> nKey;
    40         s >> nNew;
    41         s >> nTried;
    42         int nUBuckets = 0;
    43         s >> nUBuckets;
    44         if (format >= Format::V1_DETERMINISTIC) {
    45             nUBuckets ^= (1 << 30);
    46         }
    

    jnewbery commented at 3:31 pm on November 1, 2020:

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

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

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

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


    vasild commented at 11:09 am on November 2, 2020:
    See if this will make sense to you: https://github.com/bitcoin/bitcoin/pull/20284
  155. Fabcien referenced this in commit 9f2f484d69 on Feb 11, 2021
  156. Fabcien referenced this in commit e9b2955f86 on Feb 11, 2021
  157. Fabcien referenced this in commit 15f4d6097a on Feb 11, 2021
  158. Fabcien referenced this in commit 6ade7a6442 on Feb 11, 2021
  159. kittywhiskers referenced this in commit c0b91501df on May 25, 2021
  160. kittywhiskers referenced this in commit 81741adc92 on May 25, 2021
  161. kittywhiskers referenced this in commit 33fa22b85c on May 25, 2021
  162. kittywhiskers referenced this in commit 4df4517bca on May 25, 2021
  163. UdjinM6 referenced this in commit f5b4b28afe on May 28, 2021
  164. UdjinM6 referenced this in commit fff5fcfd95 on May 28, 2021
  165. UdjinM6 referenced this in commit 680067ce7a on May 29, 2021
  166. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  167. 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 19:12 UTC

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