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

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)

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

I couldn't find anything in https://btcinformation.org/en/alert/2016-11-01-alert-retirement, but yeah, might as well remove it here.

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

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?

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.

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.

Fair enough, @gmaxwell and @kanzure, I closed #14034

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)

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.

utACK 66bbc8ce26547348906d4d648e7a22c6f9e3fc7a

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

<details><summary><code>nVersion != INIT_PROTO_VERSION</code> diff</summary>

diff --git a/src/protocol.h b/src/protocol.h
index 50d197415b..4778dd9520 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -344,7 +344,7 @@ public:
         if (s.GetType() & SER_DISK)
             READWRITE(nVersion);
         if ((s.GetType() & SER_DISK) ||
-            (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH)))
+            (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH)))
             READWRITE(nTime);
         uint64_t nServicesInt = nServices;
         READWRITE(nServicesInt);
diff --git a/src/version.h b/src/version.h
index d932b512d4..d2a747fab3 100644
--- a/src/version.h
+++ b/src/version.h
@@ -20,10 +20,6 @@ static const int GETHEADERS_VERSION = 31800;
 //! disconnect from peers older than this proto version
 static const int MIN_PEER_PROTO_VERSION = GETHEADERS_VERSION;
 
-//! nTime field added to CAddress, starting with this version;
-//! if possible, avoid requesting addresses nodes older than this
-static const int CADDR_TIME_VERSION = 31402;
-
 //! BIP 0031, pong message, is enabled for all versions AFTER this one
 static const int BIP0031_VERSION = 60000;
 
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 0a3907cba4..8c6a3ed88d 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -193,7 +193,7 @@ class CAddress():
         self.ip = "0.0.0.0"
         self.port = 0
 
-    def deserialize(self, f, with_time=True):
+    def deserialize(self, f, *, with_time=True):
         if with_time:
             self.time = struct.unpack("<i", f.read(4))[0]
         self.nServices = struct.unpack("<Q", f.read(8))[0]
@@ -201,7 +201,7 @@ class CAddress():
         self.ip = socket.inet_ntoa(f.read(4))
         self.port = struct.unpack(">H", f.read(2))[0]
 
-    def serialize(self, with_time=True):
+    def serialize(self, *, with_time=True):
         r = b""
         if with_time:
             r += struct.pack("<i", self.time)
@@ -907,10 +907,10 @@ class msg_version():
         self.nServices = struct.unpack("<Q", f.read(8))[0]
         self.nTime = struct.unpack("<q", f.read(8))[0]
         self.addrTo = CAddress()
-        self.addrTo.deserialize(f, False)
+        self.addrTo.deserialize(f, with_time=False)
 
         self.addrFrom = CAddress()
-        self.addrFrom.deserialize(f, False)
+        self.addrFrom.deserialize(f, with_time=False)
         self.nNonce = struct.unpack("<Q", f.read(8))[0]
         self.strSubVer = deser_string(f)
 
@@ -930,8 +930,8 @@ class msg_version():
         r += struct.pack("<i", self.nVersion)
         r += struct.pack("<Q", self.nServices)
         r += struct.pack("<q", self.nTime)
-        r += self.addrTo.serialize(False)
-        r += self.addrFrom.serialize(False)
+        r += self.addrTo.serialize(with_time=False)
+        r += self.addrFrom.serialize(with_time=False)
         r += struct.pack("<Q", self.nNonce)
         r += ser_string(self.strSubVer)
         r += struct.pack("<i", self.nStartingHeight)

</details>

WIP: looking into how to test for https://github.com/bitcoin/bitcoin/pull/14033/files#r214007626

re-utACK 7fb962c4c1946caf0b4546bcfb6901b9d41d1443

<!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 243 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

utACK 60c1519

Rebased, I think the x86_64 Linux build failure is spurious but I'm not able to rerun it.

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.

ACK 57b0c0a9

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.

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.

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.

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

Also, pull request description needs to be updated

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

Code reivew ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

PR description indeed needs updating.

Code review ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

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

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.

post-merge ACK 57b0c0a93a243769beb306c89560d1eda61f54bd