p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater #14033
pull Empact wants to merge 1 commits into bitcoin:master from Empact:nVersion-checks changing 3 files +3 −15-
Empact commented at 5:10 pm on August 23, 2018: memberAs 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: memberI’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: memberI 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
-
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: contributorUnless 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.
-
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 insrc/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: memberutACK 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, 2018Empact commented at 7:00 am on September 1, 2018: memberWIP: looking into how to test for https://github.com/bitcoin/bitcoin/pull/14033/files#r214007626MarcoFalke commented at 3:36 pm on September 4, 2018: memberre-utACK 7fb962c4c1946caf0b4546bcfb6901b9d41d1443in 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! DroppedGetAddressCount
as well.Empact force-pushed on Sep 5, 2018DrahtBot commented at 5:06 pm on May 7, 2019: memberDrahtBot closed this on May 7, 2019
DrahtBot reopened this on May 7, 2019
DrahtBot added the label Needs rebase on Sep 7, 2019Empact force-pushed on Jan 17, 2020DrahtBot removed the label Needs rebase on Jan 17, 2020vasild commented at 10:11 am on April 13, 2020: memberutACK 60c1519DrahtBot added the label Needs rebase on Jun 4, 2020Empact force-pushed on Jun 22, 2020Empact commented at 8:18 pm on June 22, 2020: memberRebased, I think the x86_64 Linux build failure is spurious but I’m not able to rerun it.Empact force-pushed on Jun 22, 2020DrahtBot removed the label Needs rebase on Jun 22, 2020vasild commented at 7:26 am on June 23, 2020: memberThis looks good - it only changes e.g.
A && B
toB
if we knowA
will always betrue
.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 57b0c0a93aEmpact force-pushed on Jun 23, 2020vasild approvedvasild commented at 8:10 am on June 23, 2020: memberACK 57b0c0a9jnewbery commented at 1:55 am on July 1, 2020: memberI think the change here: #14033 (comment) should be applied to remove all uses ofCADDR_TIME_VERSION
. I’d also suggest adding comments to theCAddress
serializer/deserializer to say that the time is only omitted forCAddress
objects in the initial VERSION message.vasild commented at 6:24 am on July 1, 2020: memberIn this patch so far we assume
nVersion >= CADDR_TIME_VERSION
is always going to betrue
andnVersion < CADDR_TIME_VERSION
is always going to befalse
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 bynVersion != INIT_PROTO_VERSION (209)
? IfnVersion
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: memberWhy 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 aGETADDR
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:
- 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:
- There are therefore two ways to serialize a
CAddress
object for transmission on the network:INIT_PROTO_VERSION
for theCAddress
objects in theVERSION
message, and >=MIN_PEER_PROTO_VERSION
forCAddress
objects in anADDR
message. This is why changingnVersion >= CADDR_TIME_VERSION
(31402) tonVersion != 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 thanCADDR_TIME_VERSION
duringCAddress
serialization - it could beINIT_PROTO_VERSION
and we want to keep the expressionfalse
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: memberAlso, pull request description needs to be updatedjnewbery 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
andGETHEADERS_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: memberCode reivew ACK 57b0c0a93a243769beb306c89560d1eda61f54bd
PR description indeed needs updating.
jnewbery commented at 5:04 pm on July 10, 2020: memberCode 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, 2020MarcoFalke closed this on Jul 10, 2020
Empact deleted the branch on Jul 10, 2020jonatack commented at 1:10 pm on July 11, 2020: memberpost-merge ACK 57b0c0a93a243769beb306c89560d1eda61f54bdsidhujag referenced this in commit 4e1f19c06c on Jul 11, 2020deadalnix referenced this in commit b3acb54e6c on Nov 24, 2020DrahtBot locked this on Feb 15, 2022
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 06:12 UTC
More mirrored repositories can be found on mirror.b10c.me