Remove unused constants CADDR_TIME_VERSION and GETHEADERS_VERSION

[jnewbery]

These constants are no longer required and can be removed.

Additional code comments are added to explain CAddress serialization.

[Empact]

Code review ACK 158f28d

See also #14033.

[maflcko]

Concept ACK

[maflcko]

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]

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]

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

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Add parameter feature to serialization and use it for CAddress #19503 (Add parameter feature to serialization and use it for CAddress by sipa)
• Implement ADDRv2 support (part of BIP155) #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.

[maflcko]

ACK 7bb6f9b already an improvement, but maybe getting rid of INIT_PROTO_VERSION here would be an even stronger improvement (can be done later)

[jnewbery]

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)

[jonatack]

ACK 7bb6f9b

[practicalswift]

Concept ACK: less cruft is good

[vasild]

ACK 7bb6f9b

[jnewbery]

I think this is ready for merge. Three ACKs on the current HEAD:

vasild (#19486 (review))
jonatack (#19486 (review))
MarcoFalke (#19486 (comment))

plus one ACK on a previous HEAD:

empact (#19486 (comment))