Remove unused constants CADDR_TIME_VERSION and GETHEADERS_VERSION #19486

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:pr14033.1 changing 3 files +16 −15
  1. jnewbery commented at 7:39 pm on July 10, 2020: member

    These constants are no longer required and can be removed.

    Additional code comments are added to explain CAddress serialization.

  2. DrahtBot added the label P2P on Jul 10, 2020
  3. DrahtBot added the label Tests on Jul 10, 2020
  4. MarcoFalke removed the label Tests on Jul 10, 2020
  5. MarcoFalke added the label Refactoring on Jul 10, 2020
  6. in test/functional/test_framework/messages.py:219 in 158f28d9fa outdated
    218@@ -218,6 +219,7 @@ def deserialize(self, f, with_time=True):
    219     def serialize(self, with_time=True):
    


    MarcoFalke commented at 8:12 pm on July 10, 2020:

    style-nit: Passing pod literals without a name makes code harder to read. My preference would be to rule out the possibility to pass without name.

    0    def serialize(self, *, with_time=True):
    

    Same for unserialize, see #14033 (comment)


    MarcoFalke commented at 8:31 pm on July 10, 2020:
    Also makes review easier because running the tests proves that no other place than msg_version sets it to false

    jnewbery commented at 8:34 pm on July 10, 2020:
    What’s a pod literal?

    MarcoFalke commented at 8:36 pm on July 10, 2020:
    An integral value like True or 1. Not sure what the right term for that is

    jnewbery commented at 9:18 pm on July 10, 2020:
    I’ve taken your suggested change
  7. Empact commented at 8:24 pm on July 10, 2020: member
  8. MarcoFalke commented at 8:30 pm on July 10, 2020: member
    Concept ACK
  9. [protocol] Remove unused CADDR_TIME_VERSION
    Add comments to CAddress serialization code explaining why
    it's no longer needed.
    37a934e6b3
  10. [protocol] Remove unused GETHEADERS_VERSION
    GETHEADERS_VERSION is unused. No need to keep it in the codebase
    for just its historic comment.
    7bb6f9bfdb
  11. in src/protocol.h:374 in 158f28d9fa outdated
    370@@ -371,7 +371,13 @@ class CAddress : public CService
    371             READWRITE(nVersion);
    372         }
    373         if ((s.GetType() & SER_DISK) ||
    374-            (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
    375+            (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) {
    


    MarcoFalke commented at 8:50 pm on July 10, 2020:
    style-nit: the python code reads much nicer, so I’d slightly prefer if the C++ code was also easy to read. Not blocking of course, but for example passing down an option with_time per #19477 (comment) would be nice.

    sipa commented at 8:58 pm on July 10, 2020:

    I’m working on that. In a branch I’m working on I have this patch that make another (vaguely related) readability improvement:

     0diff --git a/src/protocol.h b/src/protocol.h
     1index 985f44640b..b74a24e007 100644
     2--- a/src/protocol.h
     3+++ b/src/protocol.h
     4@@ -359,6 +359,11 @@ class CAddress : public CService
     5 {
     6     static constexpr uint32_t TIME_INIT{100000000};
     7 
     8+    /** The disk serialization for CAddress includes a version number.
     9+     *  Traditionally the number CLIENT_VERSION was used, but has since
    10+     *  been disentangled from it. */
    11+    static constexpr int DISK_VERSION{201000};
    12+
    13 public:
    14     CAddress() : CService{} {};
    15     explicit CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
    16@@ -366,13 +371,14 @@ public:
    17     SERIALIZE_METHODS(CAddress, obj)
    18     {
    19         SER_READ(obj, obj.nTime = TIME_INIT);
    20-        int nVersion = s.GetVersion();
    21         if (s.GetType() & SER_DISK) {
    22-            READWRITE(nVersion);
    23-        }
    24-        if ((s.GetType() & SER_DISK) ||
    25-            (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
    26+            int disk_version = DISK_VERSION;
    27+            READWRITE(disk_version);
    28             READWRITE(obj.nTime);
    29+        } else if (s.GetType() & SER_NETWORK) {
    30+            if (s.GetVersion() >= CADDR_TIME_VERSION) {
    31+                READWRITE(obj.nTime);
    32+            }
    33         }
    34         READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
    35         READWRITEAS(CService, obj);
    

    Which makes it clear that the version constant being compared to has nothing to do with the client version (and never has).

  12. jnewbery force-pushed on Jul 10, 2020
  13. jnewbery commented at 9:20 pm on July 10, 2020: member
    I think this can be taken separately from @sipa’s suggested change.
  14. DrahtBot commented at 0:18 am on July 11, 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:

    • #19503 (Add parameter feature to serialization and use it for CAddress by sipa)
    • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)

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

  15. MarcoFalke commented at 10:07 am on July 11, 2020: member
    ACK 7bb6f9bfdbd3b0b5594febc9a0ae2f84e6d7add1 already an improvement, but maybe getting rid of INIT_PROTO_VERSION here would be an even stronger improvement (can be done later)
  16. jnewbery commented at 11:43 am on July 11, 2020: member

    getting rid of INIT_PROTO_VERSION here would be an even stronger improvement

    I agree that’d be good. Let’s save it until after the various version fields in CNode are unified (#17785)

  17. jonatack commented at 1:37 pm on July 11, 2020: member
    ACK 7bb6f9bfdbd3b0b5594febc9a0ae2f84e6d7add1
  18. practicalswift commented at 5:39 pm on July 11, 2020: contributor
    Concept ACK: less cruft is good
  19. vasild approved
  20. vasild commented at 6:55 am on July 13, 2020: member
    ACK 7bb6f9bf
  21. MarcoFalke merged this on Jul 13, 2020
  22. MarcoFalke closed this on Jul 13, 2020

  23. jnewbery deleted the branch on Jul 13, 2020
  24. sidhujag referenced this in commit 470b847c20 on Jul 14, 2020
  25. jasonbcox referenced this in commit 678e25812f on Nov 24, 2020
  26. 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 03:12 UTC

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