p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater #14033

pull Empact wants to merge 1 commits into bitcoin:master from Empact:nVersion-checks changing 3 files +3 −15
  1. Empact commented at 5:10 pm on August 23, 2018: member
    As per @jnewbery’s comment below, “After the version handshake, the version must be >= MIN_PEER_PROTO_VERSION, since we immediately disconnect any peer that advertises a lower version than that in its version message” #14033 (comment)
  2. laanwj commented at 6:06 pm on August 23, 2018: member
    I’m trying to remember what the plan was for removing the hardcoded alert I think now that the key has been revealed it’s ok to remove that for 0.18 as well
  3. laanwj added the label P2P on Aug 23, 2018
  4. DrahtBot commented at 6:27 pm on August 23, 2018: 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:

    • #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
    • #17194 (p2p: Avoid forwarding ADDR messages to SPV nodes by naumenkogs)

    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.

  5. MarcoFalke commented at 6:32 pm on August 23, 2018: member
    I couldn’t find anything in https://btcinformation.org/en/alert/2016-11-01-alert-retirement, but yeah, might as well remove it here.
  6. Empact force-pushed on Aug 23, 2018
  7. Empact commented at 6:49 pm on August 23, 2018: member

    Dropped the announcement in #14034. Restored the CAddress serialization check because absent it, tests were failing around verack.

    https://github.com/bitcoin/bitcoin/blob/540bf8aacc50aae0ea5beb76511905a7d2a3e15f/src/protocol.h#L346-L348

  8. kanzure commented at 6:49 pm on August 23, 2018: contributor

    I’m trying to remember what the plan was for removing the hardcoded alert. I think now that the key has been revealed it’s ok to remove that for 0.18 as well.

    The threat model seems to be: someone has been offline for quite a while (possibly using old software), reconnects to the network, and nobody has the final alert message. Instead they get connected to someone’s weird node that spams them with a valid signed message that isn’t the original final alert message. Is this particularly likely to happen and/or bad or something we’d want to prevent?

  9. gmaxwell commented at 6:56 pm on August 23, 2018: contributor
    Unless there is a reason to do otherwise, I think we should probably leave it in so long as we still connect to nodes that had the alert system enabled. @kanzure Because the alert key is published if someone starts older software that still has it, they could get a message like “Your wallet is outdated and could lose funds! go to leaksyourprivatekeys.com to get the new official bitcoin software”. The hardcoded final alert mitigates this by blocking the message entirely and/or at least causing a message that indicates that these notices might not be safe to heed.
  10. kanzure commented at 10:30 pm on August 23, 2018: contributor

    The hardcoded final alert mitigates this

    Right, they were asking about removing the hardcoded alert. Anyway, I agree with leaving it in- unless it’s causing issue or problems or maintenance headache.

  11. Empact commented at 10:35 pm on August 23, 2018: member
    Fair enough, @gmaxwell and @kanzure, I closed #14034
  12. laanwj commented at 9:33 am on August 27, 2018: member

    I think we should probably leave it in so long as we still connect to nodes that had the alert system enabled.

    I think this is a good policy, there is little overhead in keeping it but I also think it would make sense to add a comment saying this, so that it’s clear for future maintainers.

    (out of scope for this PR though as it no longer touches anything alert-system related)

  13. laanwj commented at 9:36 am on August 27, 2018: member

    Is it intentional that you’re keeping the static const int CADDR_TIME_VERSION = 31402; around in src/version.h?

    FWIW, there seem to be no other constants for versions before MIN_PEER_PROTO_VERSION.

    If so, please do add a comment saying this, so it won’t be ‘accidentally’ removed in a follow-up commit when someone notices that it’s no longer used.

  14. MarcoFalke renamed this:
    Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater
    p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater
    on Aug 27, 2018
  15. MarcoFalke commented at 1:03 pm on August 27, 2018: member
    utACK 66bbc8ce26547348906d4d648e7a22c6f9e3fc7a
  16. MarcoFalke commented at 1:22 pm on August 27, 2018: member

    I’d guess the remaining check can be replaced by nVersion != INIT_PROTO_VERSION, but no strong opinion.

     0diff --git a/src/protocol.h b/src/protocol.h
     1index 50d197415b..4778dd9520 100644
     2--- a/src/protocol.h
     3+++ b/src/protocol.h
     4@@ -344,7 +344,7 @@ public:
     5         if (s.GetType() & SER_DISK)
     6             READWRITE(nVersion);
     7         if ((s.GetType() & SER_DISK) ||
     8-            (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH)))
     9+            (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH)))
    10             READWRITE(nTime);
    11         uint64_t nServicesInt = nServices;
    12         READWRITE(nServicesInt);
    13diff --git a/src/version.h b/src/version.h
    14index d932b512d4..d2a747fab3 100644
    15--- a/src/version.h
    16+++ b/src/version.h
    17@@ -20,10 +20,6 @@ static const int GETHEADERS_VERSION = 31800;
    18 //! disconnect from peers older than this proto version
    19 static const int MIN_PEER_PROTO_VERSION = GETHEADERS_VERSION;
    20 
    21-//! nTime field added to CAddress, starting with this version;
    22-//! if possible, avoid requesting addresses nodes older than this
    23-static const int CADDR_TIME_VERSION = 31402;
    24-
    25 //! BIP 0031, pong message, is enabled for all versions AFTER this one
    26 static const int BIP0031_VERSION = 60000;
    27 
    28diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
    29index 0a3907cba4..8c6a3ed88d 100755
    30--- a/test/functional/test_framework/messages.py
    31+++ b/test/functional/test_framework/messages.py
    32@@ -193,7 +193,7 @@ class CAddress():
    33         self.ip = "0.0.0.0"
    34         self.port = 0
    35 
    36-    def deserialize(self, f, with_time=True):
    37+    def deserialize(self, f, *, with_time=True):
    38         if with_time:
    39             self.time = struct.unpack("<i", f.read(4))[0]
    40         self.nServices = struct.unpack("<Q", f.read(8))[0]
    41@@ -201,7 +201,7 @@ class CAddress():
    42         self.ip = socket.inet_ntoa(f.read(4))
    43         self.port = struct.unpack(">H", f.read(2))[0]
    44 
    45-    def serialize(self, with_time=True):
    46+    def serialize(self, *, with_time=True):
    47         r = b""
    48         if with_time:
    49             r += struct.pack("<i", self.time)
    50@@ -907,10 +907,10 @@ class msg_version():
    51         self.nServices = struct.unpack("<Q", f.read(8))[0]
    52         self.nTime = struct.unpack("<q", f.read(8))[0]
    53         self.addrTo = CAddress()
    54-        self.addrTo.deserialize(f, False)
    55+        self.addrTo.deserialize(f, with_time=False)
    56 
    57         self.addrFrom = CAddress()
    58-        self.addrFrom.deserialize(f, False)
    59+        self.addrFrom.deserialize(f, with_time=False)
    60         self.nNonce = struct.unpack("<Q", f.read(8))[0]
    61         self.strSubVer = deser_string(f)
    62 
    63@@ -930,8 +930,8 @@ class msg_version():
    64         r += struct.pack("<i", self.nVersion)
    65         r += struct.pack("<Q", self.nServices)
    66         r += struct.pack("<q", self.nTime)
    67-        r += self.addrTo.serialize(False)
    68-        r += self.addrFrom.serialize(False)
    69+        r += self.addrTo.serialize(with_time=False)
    70+        r += self.addrFrom.serialize(with_time=False)
    71         r += struct.pack("<Q", self.nNonce)
    72         r += ser_string(self.strSubVer)
    73         r += struct.pack("<i", self.nStartingHeight)
    
  17. in src/net_processing.cpp:1759 in 66bbc8ce26 outdated
    1755@@ -1758,7 +1756,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
    1756             }
    1757 
    1758             // Get recent addresses
    1759-            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < 1000)
    1760+            if (pfrom->fOneShot || connman->GetAddressCount() < 1000)
    


    luke-jr commented at 12:18 pm on August 30, 2018:
    This looks wrong? Shouldn’t it be unconditional now, since the condition was ||?

    Empact commented at 6:59 am on September 1, 2018:
    Yep, def wrong. Thanks for catching that.
  18. Empact force-pushed on Sep 1, 2018
  19. Empact commented at 7:00 am on September 1, 2018: member
  20. MarcoFalke commented at 3:36 pm on September 4, 2018: member
    re-utACK 7fb962c4c1946caf0b4546bcfb6901b9d41d1443
  21. in src/net_processing.cpp:2035 in 7fb962c4c1 outdated
    1755@@ -1758,11 +1756,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
    1756             }
    1757 
    1758             // Get recent addresses
    1759-            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < 1000)
    


    practicalswift commented at 7:17 am on September 5, 2018:
    GetAddressCount is no longer used and can be removed, no? :-)

    Empact commented at 6:31 pm on September 5, 2018:
    Indeed! Dropped GetAddressCount as well.
  22. Empact force-pushed on Sep 5, 2018
  23. DrahtBot commented at 5:06 pm on May 7, 2019: member
  24. DrahtBot closed this on May 7, 2019

  25. DrahtBot reopened this on May 7, 2019

  26. DrahtBot added the label Needs rebase on Sep 7, 2019
  27. Empact force-pushed on Jan 17, 2020
  28. DrahtBot removed the label Needs rebase on Jan 17, 2020
  29. vasild commented at 10:11 am on April 13, 2020: member
    utACK 60c1519
  30. DrahtBot added the label Needs rebase on Jun 4, 2020
  31. Empact force-pushed on Jun 22, 2020
  32. Empact commented at 8:18 pm on June 22, 2020: member
    Rebased, I think the x86_64 Linux build failure is spurious but I’m not able to rerun it.
  33. Empact force-pushed on Jun 22, 2020
  34. DrahtBot removed the label Needs rebase on Jun 22, 2020
  35. vasild commented at 7:26 am on June 23, 2020: member

    This looks good - it only changes e.g. A && B to B if we know A will always be true.

    What happened with the removal of CConnman::GetAddressCount()? That method is now unused.

  36. Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater 57b0c0a93a
  37. Empact force-pushed on Jun 23, 2020
  38. vasild approved
  39. vasild commented at 8:10 am on June 23, 2020: member
    ACK 57b0c0a9
  40. jnewbery commented at 1:55 am on July 1, 2020: member
    I think the change here: #14033 (comment) should be applied to remove all uses of CADDR_TIME_VERSION. I’d also suggest adding comments to the CAddress serializer/deserializer to say that the time is only omitted for CAddress objects in the initial VERSION message.
  41. vasild commented at 6:24 am on July 1, 2020: member

    In this patch so far we assume nVersion >= CADDR_TIME_VERSION is always going to be true and nVersion < CADDR_TIME_VERSION is always going to be false and we remove those expressions accordingly to not change the behavior.

    Why the difference in #14033 (comment) where nVersion >= CADDR_TIME_VERSION (31402) is replaced by nVersion != INIT_PROTO_VERSION (209)? If nVersion is in the range [210, 31401] then that would be a change in behavior and I guess this PR aims to be a non-behavioral change.

  42. jnewbery commented at 2:35 pm on July 1, 2020: member

    Why the difference in #14033 (comment) where nVersion >= CADDR_TIME_VERSION (31402) is replaced by nVersion != INIT_PROTO_VERSION (209)? If nVersion is in the range [210, 31401] then that would be a change in behavior and I guess this PR aims to be a non-behavioral change.

    Good question! I should have been a bit more explicit with my comment “I’d also suggest adding comments to the CAddress serializer/deserializer to say that the time is only omitted for CAddress objects in the initial VERSION message.” To expand:

    • CAddress objects appear in two different P2P message types: VERSION messages in the initial version-verack handshake, and ADDR messages, sent either in response to a GETADDR request, or unsolicited on a timer.
    • The version-verack handshake is responsible for P2P version negotiation. Prior to the handshake, we use INIT_PROTO_VERSION for serializing messages to the peer:

    https://github.com/bitcoin/bitcoin/blob/ffa70801dab7fa85c24fd5d19ca998e0910238d5/src/net_processing.cpp#L468

    https://github.com/bitcoin/bitcoin/blob/ffa70801dab7fa85c24fd5d19ca998e0910238d5/src/net_processing.cpp#L2298

    • After the version handshake, the version must be >= MIN_PEER_PROTO_VERSION, since we immediately disconnect any peer that advertises a lower version than that in its version message:

    https://github.com/bitcoin/bitcoin/blob/ffa70801dab7fa85c24fd5d19ca998e0910238d5/src/net_processing.cpp#L2262-L2267

    • There are therefore two ways to serialize a CAddress object for transmission on the network: INIT_PROTO_VERSION for the CAddress objects in the VERSION message, and >= MIN_PEER_PROTO_VERSION for CAddress objects in an ADDR message. This is why changing nVersion >= CADDR_TIME_VERSION (31402) to nVersion != INIT_PROTO_VERSION (209) is equivalent.
  43. vasild commented at 12:11 pm on July 2, 2020: member

    @jnewbery Thanks for the elaborate explanation! So nVersion could be less than CADDR_TIME_VERSION during CAddress serialization - it could be INIT_PROTO_VERSION and we want to keep the expression false in that case.

    I think the patch is good as it is and it would also be good if extended as per #14033 (comment).

  44. MarcoFalke commented at 1:00 pm on July 2, 2020: member
    Also, pull request description needs to be updated
  45. jnewbery commented at 3:56 pm on July 10, 2020: member

    @Empact are you still maintaining this PR? I’m doing some work in net_processing and would like to get rid of this cruft, so I’m happy to take this over if you’re no longer interested.

    I have a branch at https://github.com/jnewbery/bitcoin/tree/pr14033.1 that fully removes the CADDR_TIME_VERSION and GETHEADERS_VERSION constants, and adds comments. Feel free to grab those commits, or let me know if you’d prefer that I open a PR.

  46. sipa commented at 4:55 pm on July 10, 2020: member

    Code reivew ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

    PR description indeed needs updating.

  47. jnewbery commented at 5:04 pm on July 10, 2020: member

    Code review ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

    Can we change the PR description and get this merged? Other changes can be done in a follow-up.

  48. MarcoFalke merged this on Jul 10, 2020
  49. MarcoFalke closed this on Jul 10, 2020

  50. Empact deleted the branch on Jul 10, 2020
  51. Empact commented at 8:15 pm on July 10, 2020: member
    For those who wanted a PR description update, does the current description meet your expectations? @jnewbery thanks for the full explanation above, feel free to go ahead with the follow up, I don’t have any immediate aspirations.
  52. jonatack commented at 1:10 pm on July 11, 2020: member
    post-merge ACK 57b0c0a93a243769beb306c89560d1eda61f54bd
  53. sidhujag referenced this in commit 4e1f19c06c on Jul 11, 2020
  54. deadalnix referenced this in commit b3acb54e6c on Nov 24, 2020
  55. 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-12-11 06:12 UTC

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