p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO

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)

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

laanwj added the label P2P on Aug 23, 2018

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.

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.

Empact force-pushed on Aug 23, 2018

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

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?

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.

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.

Empact commented at 10:35 pm on August 23, 2018: member

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

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)

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.

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

MarcoFalke commented at 1:03 pm on August 27, 2018: member

utACK 66bbc8ce26547348906d4d648e7a22c6f9e3fc7a

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)

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.

Empact force-pushed on Sep 1, 2018

Empact commented at 7:00 am on September 1, 2018: member

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

MarcoFalke commented at 3:36 pm on September 4, 2018: member

re-utACK 7fb962c4c1946caf0b4546bcfb6901b9d41d1443

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.

Empact force-pushed on Sep 5, 2018

DrahtBot commented at 5:06 pm on May 7, 2019: member

DrahtBot closed this on May 7, 2019

DrahtBot reopened this on May 7, 2019

DrahtBot added the label Needs rebase on Sep 7, 2019

Empact force-pushed on Jan 17, 2020

DrahtBot removed the label Needs rebase on Jan 17, 2020

vasild commented at 10:11 am on April 13, 2020: member

utACK 60c1519

DrahtBot added the label Needs rebase on Jun 4, 2020

Empact force-pushed on Jun 22, 2020

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.

Empact force-pushed on Jun 22, 2020

DrahtBot removed the label Needs rebase on Jun 22, 2020

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.

Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater 57b0c0a93a

Empact force-pushed on Jun 23, 2020

vasild approved

vasild commented at 8:10 am on June 23, 2020: member

ACK 57b0c0a9

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.

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.

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.

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

MarcoFalke commented at 1:00 pm on July 2, 2020: member

Also, pull request description needs to be updated

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.

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

Code reivew ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

PR description indeed needs updating.

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.

MarcoFalke merged this on Jul 10, 2020

MarcoFalke closed this on Jul 10, 2020

Empact deleted the branch on Jul 10, 2020

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.

jonatack commented at 1:10 pm on July 11, 2020: member

post-merge ACK 57b0c0a93a243769beb306c89560d1eda61f54bd

sidhujag referenced this in commit 4e1f19c06c on Jul 11, 2020

deadalnix referenced this in commit b3acb54e6c on Nov 24, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts