Remove unused constants `CADDR_TIME_VERSION` and `GETHEADERS

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.

DrahtBot added the label P2P on Jul 10, 2020

DrahtBot added the label Tests on Jul 10, 2020

MarcoFalke removed the label Tests on Jul 10, 2020

MarcoFalke added the label Refactoring on Jul 10, 2020

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.

    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

Empact commented at 8:24 PM on July 10, 2020: member

Code review ACK https://github.com/bitcoin/bitcoin/pull/19486/commits/158f28d9faf8105b6ed2e3440f21691e5e9ff75b

See also #14033.

MarcoFalke commented at 8:30 PM on July 10, 2020: member

Concept ACK

[protocol] Remove unused CADDR_TIME_VERSION

Add comments to CAddress serialization code explaining why
it's no longer needed.

37a934e6b3

[protocol] Remove unused GETHEADERS_VERSION

GETHEADERS_VERSION is unused. No need to keep it in the codebase
for just its historic comment.

7bb6f9bfdb

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:

diff --git a/src/protocol.h b/src/protocol.h
index 985f44640b..b74a24e007 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -359,6 +359,11 @@ class CAddress : public CService
 {
     static constexpr uint32_t TIME_INIT{100000000};
 
+    /** The disk serialization for CAddress includes a version number.
+     *  Traditionally the number CLIENT_VERSION was used, but has since
+     *  been disentangled from it. */
+    static constexpr int DISK_VERSION{201000};
+
 public:
     CAddress() : CService{} {};
     explicit CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
@@ -366,13 +371,14 @@ public:
     SERIALIZE_METHODS(CAddress, obj)
     {
         SER_READ(obj, obj.nTime = TIME_INIT);
-        int nVersion = s.GetVersion();
         if (s.GetType() & SER_DISK) {
-            READWRITE(nVersion);
-        }
-        if ((s.GetType() & SER_DISK) ||
-            (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
+            int disk_version = DISK_VERSION;
+            READWRITE(disk_version);
             READWRITE(obj.nTime);
+        } else if (s.GetType() & SER_NETWORK) {
+            if (s.GetVersion() >= CADDR_TIME_VERSION) {
+                READWRITE(obj.nTime);
+            }
         }
         READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
         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).

jnewbery force-pushed on Jul 10, 2020

jnewbery commented at 9:20 PM on July 10, 2020: member

I think this can be taken separately from @sipa's suggested change.

DrahtBot commented at 12: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.

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)