p2p: Unify Send and Receive protocol versions

hebasto commented at 6:43 pm on December 20, 2019: member

On master (6fef85bfa3cd7f76e83b8b57f9e4acd63eb664ec) CNode has two members to keep protocol version:

nRecvVersion for received messages
nSendVersion for messages to send

After exchanging with VERSION and VERACK messages via protocol version INIT_PROTO_VERSION, both nodes set nRecvVersion and nSendVersion to the same value which is the greatest common protocol version.

This PR:

replaces two CNode members, nRecvVersion nSendVersion, with m_greatest_common_version
removes duplicated getter and setter

There is no change in behavior on the P2P network.

fanquake added the label P2P on Dec 20, 2019

MarcoFalke commented at 6:51 pm on December 20, 2019: member

The change in behavior: with this PR VERACK message is sent and received with the common protocol version rather than INIT_PROTO_VERSION.

How does this change behavior? verack has no payload, so serialization flags can’t possibly change the serialization.

MarcoFalke added the label Refactoring on Dec 20, 2019

hebasto commented at 6:54 pm on December 20, 2019: member

How does this change behavior? verack has no payload, so serialization flags can’t possibly change the serialization.

You are right. I did not mean change of external behavior. Bad wording. Mind help me re-word it? Or just skip it?

ohld approved

laanwj commented at 10:36 am on January 13, 2020: member

If correct, this is a very nice simplification.

DrahtBot commented at 6:57 am on January 17, 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:

#19866 (eBPF Linux tracepoints by jb55)
#19776 (net, rpc: expose high bandwidth mode state via getpeerinfo by theStack)
#19509 (Per-Peer Message Logging by troygiorshev)
#19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)

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.

laanwj commented at 12:13 pm on February 6, 2020: member

You are right. I did not mean change of external behavior. Bad wording. Mind help me re-word it? Or just skip it?

I’d suggest removing that, and explicitly say that there is no change in behavior on the P2P network.

hebasto force-pushed on Feb 6, 2020

hebasto commented at 4:21 pm on February 6, 2020: member

@laanwj

You are right. I did not mean change of external behavior. Bad wording. Mind help me re-word it? Or just skip it?

I’d suggest removing that, and explicitly say that there is no change in behavior on the P2P network.

Done. Thank you for your suggestion.

DrahtBot added the label Needs rebase on Jun 4, 2020

hebasto force-pushed on Jun 5, 2020

DrahtBot removed the label Needs rebase on Jun 5, 2020

hebasto force-pushed on Jun 6, 2020

hebasto commented at 6:57 am on June 6, 2020: member

Rebased 25b93f13f08fba0a25c3480e158d131eea8795e5 -> cc5dba8e1fd2586dee32e1d19e8867e5f6b03754 (pr17785.02 -> pr17785.04) due to the conflict with #19053.

DrahtBot added the label Needs rebase on Jul 10, 2020

hebasto force-pushed on Jul 10, 2020

hebasto commented at 6:18 pm on July 10, 2020: member

Rebased cc5dba8e1fd2586dee32e1d19e8867e5f6b03754 -> e268f6451fd2e5b57e8d7409c2853bea984a1ba9 (pr17785.04 -> pr17785.05) due to the conflict with #14033.

DrahtBot removed the label Needs rebase on Jul 10, 2020

hebasto commented at 7:57 pm on July 10, 2020: member

@jnewbery Mind looking into this PR?

jnewbery commented at 8:41 pm on July 10, 2020: member

Concept ACK. The version fields in CNode are confusing. After version negotiation, I think we should only need one field to track common version. A couple of comments:

there’s also an nVersion field. Can that also be merged?
would it make sense for CNode or CNodeState to store a CNetMsgMaker rather than constructing it in every function it needs to use it in?

MarcoFalke commented at 11:47 am on July 11, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Jul 15, 2020

hebasto force-pushed on Aug 21, 2020

hebasto commented at 10:50 am on August 21, 2020: member

Rebased e268f6451fd2e5b57e8d7409c2853bea984a1ba9 -> 7b12f25d34e7af6a518e17456a48be493282deb3 (pr17785.05 -> pr17785.06) due to the conflicts with #19316, #19174, #18044, #19472, #19512.

hebasto commented at 10:55 am on August 21, 2020: member

@jnewbery

there’s also an nVersion field. Can that also be merged?

If our node is not upgraded, comparing to its peer, nVersion and m_greatest_common_version would differ.

hebasto commented at 10:57 am on August 21, 2020: member

@jnewbery

would it make sense for CNode or CNodeState to store a CNetMsgMaker rather than constructing it in every function it needs to use it in?

I think yes. The same idea came into my head while was working on this PR. But not in this PR scope.

DrahtBot removed the label Needs rebase on Aug 21, 2020

jnewbery commented at 10:17 am on August 24, 2020: member

If our node is not upgraded, comparing to its peer, nVersion and m_greatest_common_version would differ.

Right, but there’s nothing we can do with that information. If a node has a higher version than us, it means that it has some functionality that we’re unaware of. We can’t do anything with that information except perhaps report it in logs and in getpeerinfo. All peer logic in net_processing should be based on the highest common version.

DrahtBot added the label Needs rebase on Aug 24, 2020

hebasto force-pushed on Aug 24, 2020

hebasto commented at 11:29 pm on August 24, 2020: member

Updated 7b12f25d34e7af6a518e17456a48be493282deb3 -> 659a90cb93cbe9a894cae461e0401ed32b25caf3 (pr17785.06 -> pr17785.07):

rebased due to the conflict with #19704
implemented @jnewbery’s suggestion:

All peer logic in net_processing should be based on the highest common version.

DrahtBot removed the label Needs rebase on Aug 24, 2020

jnewbery commented at 10:32 am on August 25, 2020: member

utACK 659a90cb93cbe9a894cae461e0401ed32b25caf3. Nice tidy-up. Thanks hebasto!

DrahtBot added the label Needs rebase on Sep 1, 2020

hebasto force-pushed on Sep 1, 2020

hebasto commented at 7:30 am on September 1, 2020: member

Rebased 659a90cb93cbe9a894cae461e0401ed32b25caf3 -> dc56e956fb988003777a949350f08b7964bd3452 (pr17785.07 -> pr17785.08) due to the conflict with #19668.

hebasto force-pushed on Sep 1, 2020

hebasto commented at 8:09 am on September 1, 2020: member

Rebased dc56e956fb988003777a949350f08b7964bd3452 -> e68912bc559197d3c82c581a1f64ec4c06f119da (pr17785.08 -> pr17785.09) due to the conflict with #19067.

jnewbery commented at 8:35 am on September 1, 2020: member

utACK e68912bc559197d3c82c581a1f64ec4c06f119da

DrahtBot removed the label Needs rebase on Sep 1, 2020

DrahtBot added the label Needs rebase on Sep 3, 2020

hebasto force-pushed on Sep 3, 2020

hebasto commented at 6:14 pm on September 3, 2020: member

Rebased e68912bc559197d3c82c581a1f64ec4c06f119da -> 646ddf115a7d82d5b2e05452e25736fb65cf9f03 (pr17785.09 -> pr17785.10) due to the conflict with #19724.

DrahtBot removed the label Needs rebase on Sep 3, 2020

jnewbery commented at 8:04 am on September 4, 2020: member

utACK 646ddf115a7d82d5b2e05452e25736fb65cf9f03

(verified git range-diff e68912bc55~4..e68912bc55 646ddf115a~4..646ddf115a. This was a trivial rebase)

DrahtBot added the label Needs rebase on Sep 7, 2020

p2p: Unify Send and Receive protocol versions

There is no change in behavior on the P2P network.

e9a6d8b13b

refactor: Rename local variable nSendVersion 8d2026796a

p2p: Remove SetCommonVersion() from VERACK handler

SetCommonVersion() is already called from the VERSION message handler.
There is no change in behavior on the P2P network.

e084d45562

p2p: Use the greatest common version in peer logic ddefb5c0b7

hebasto force-pushed on Sep 7, 2020

hebasto commented at 6:08 pm on September 7, 2020: member

Rebased 646ddf115a7d82d5b2e05452e25736fb65cf9f03 -> ddefb5c0b759950942ac03f28c43b548af7b4033 (pr17785.10 -> pr17785.11) due to the conflict with #19791.

DrahtBot removed the label Needs rebase on Sep 7, 2020

benthecarman approved

benthecarman commented at 8:41 pm on September 7, 2020: contributor

utACK ddefb5c

jnewbery commented at 10:44 am on September 8, 2020: member

ACK ddefb5c0b759950942ac03f28c43b548af7b4033

Verified git range-diff 646ddf115a~4..646ddf115a ddefb5c0b7~4..ddefb5c0b7

hebasto commented at 11:06 am on September 8, 2020: member

@MarcoFalke @amiti @sipa Mind looking into this PR?

naumenkogs commented at 2:31 pm on September 8, 2020: member

ACK ddefb5c0b759950942ac03f28c43b548af7b4033 good refactoring

in src/net_processing.cpp:2409 in e9a6d8b13b outdated

2405@@ -2406,10 +2406,10 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2406             PushNodeVersion(pfrom, m_connman, GetAdjustedTime());
2407 
2408         if (nVersion >= WTXID_RELAY_VERSION) {
2409-            m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::WTXIDRELAY));

amitiuttarwar commented at 11:01 pm on September 8, 2020:

I’m slightly confused about why this was originally sending INIT_PROTO_VERSION (and below). the patch is fixing it to match my expectations, but I think this means a small change in the p2p message right?

haven’t looked into the history yet, am I missing something?

hebasto commented at 7:06 am on September 9, 2020:

I’m slightly confused about why this was originally sending INIT_PROTO_VERSION (and below).

I don’t know the whole historical context, but it seems the reason is that on that stage the INIT_PROTO_VERSION is the only known version on which both the node and its peer are agree as version is changed later: https://github.com/bitcoin/bitcoin/blob/4f229d8904f8e3494cab30d61df9f11f1cc06388/src/net_processing.cpp#L2432-L2434

I think this means a small change in the p2p message right?

See: #17785 (comment)

jnewbery commented at 8:32 am on September 9, 2020:

I believe the only object that is serialized differently based on p2p version is a CAddress, so the addresses in the version message don’t have timestamps.

Changing the serialization version for a wtxid message has no effect.

amitiuttarwar commented at 0:01 am on September 10, 2020:

ah, okay, that clears up my confusion about possible change. thank you!

still doesn’t make sense why it was that way,

it seems the reason is that on that stage the INIT_PROTO_VERSION is the only known version on which both the node and its peer are agree as version is changed later:

yeah, but the local nSendVersion was set earlier

but I don’t think that’s super important now that I understand the context better.

thanks!

in src/net.h:1071 in ddefb5c0b7

1069     {
1070-        nRecvVersion = nVersionIn;
1071+        m_greatest_common_version = greatest_common_version;
1072     }
1073-    int GetRecvVersion() const
1074+    int GetCommonVersion() const

amitiuttarwar commented at 11:28 pm on September 8, 2020:

nit: missing newline between functions

in src/net.h:1067 in ddefb5c0b7

1063@@ -1065,16 +1064,14 @@ class CNode
1064 
1065     bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete);
1066 
1067-    void SetRecvVersion(int nVersionIn)
1068+    void SetCommonVersion(int greatest_common_version)

amitiuttarwar commented at 11:52 pm on September 8, 2020:

just noting that we lose the error logging previously present for nSendVersion (don’t set more than once, don’t get before setting). I think thats fine though.

amitiuttarwar commented at 0:04 am on September 9, 2020: contributor

I’ve taken a look at the code. Overall it looks like a very nice cleanup. I just have one question I’d like to better understand (about wtxidrelay / verack message) before I’m ready to leave a proper ACK.

I wondering if after these changes we can get rid of the CNode.nVersion field entirely. I gave it a quick shot here: https://github.com/amitiuttarwar/bitcoin/commit/ef2b8620dcc4123a3f62be48943490f72c40cfaf. It compiles & I think it makes sense (maybe one clarification in a log), but haven’t evaluated super closely.

thanks for this cleanup! its a very nice simplification.

hebasto commented at 6:45 am on September 9, 2020: member

@amitiuttarwar

I wondering if after these changes we can get rid of the CNode.nVersion field entirely. I gave it a quick shot here: amitiuttarwar@ef2b862. It compiles & I think it makes sense (maybe one clarification in a log), but haven’t evaluated super closely. @jnewbery raised the same question. This could be resolved in a follow-up PR. I think we need smth like bool CNode::VersionMsgAlreadyReceived().

jnewbery commented at 8:45 am on September 9, 2020: member

I think nVersion is still useful for getpeerinfo to display which version was received from the peer, although perhaps it could be renamed to m_version_received or similar in a future PR.

fjahr commented at 1:56 pm on September 9, 2020: member

Code review ACK ddefb5c0b759950942ac03f28c43b548af7b4033

Nice cleanup!

amitiuttarwar commented at 0:40 am on September 10, 2020: contributor

code review but untested ACK ddefb5c0b7

for another PR: @hebasto

@jnewbery raised the same question. This could be resolved in a follow-up PR. I think we need smth like bool CNode::VersionMsgAlreadyReceived().

yeah, but I got slightly confused because based on this comment I thought you might have been getting rid of nVersion entirely. no worries :) I just wanted to highlight for future improvements.

laanwj merged this on Sep 21, 2020

laanwj closed this on Sep 21, 2020

sidhujag referenced this in commit bf463b4f86 on Sep 22, 2020

hebasto deleted the branch on Sep 22, 2020

MarcoFalke commented at 11:14 am on October 12, 2020: member

Post merge ACK, but would be good to remove nVersion from the tests as well in the last commit. See #20131

in src/net.cpp:632 in e9a6d8b13b outdated

627-    // only one version message is allowed per session. We can therefore
628-    // treat this value as const and even atomic as long as it's only used
629-    // once a version message has been successfully processed. Any attempt to
630-    // set this twice is an error.
631-    if (nSendVersion != 0) {
632-        error("Send version already set for node: %i. Refusing to change from %i to %i", id, nSendVersion, nVersionIn);

MarcoFalke commented at 11:17 am on October 12, 2020:

Any reason to remove this debug log? I think it would be good to keep this to avoid regressing on this in the future.

MarcoFalke commented at 8:43 pm on October 12, 2020:

Fixed in #20138

MarcoFalke referenced this in commit 00f4dcd552 on Dec 7, 2020

sidhujag referenced this in commit 6a7d7717e8 on Dec 7, 2020

Fabcien referenced this in commit 21862c3cda on Feb 9, 2021

DrahtBot locked this on Feb 15, 2022

p2p: Unify Send and Receive protocol versions #17785

Conflicts