p2p: Unify Send and Receive protocol versions #17785

pull hebasto wants to merge 4 commits into bitcoin:master from hebasto:20191220-unify-protocol-versions changing 7 files +54 −92
  1. 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.

  2. fanquake added the label P2P on Dec 20, 2019
  3. 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.

  4. MarcoFalke added the label Refactoring on Dec 20, 2019
  5. 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?

  6. ohld approved
  7. laanwj commented at 10:36 AM on January 13, 2020: member

    If correct, this is a very nice simplification.

  8. DrahtBot commented at 6:57 AM on January 17, 2020: contributor

    <!--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:

    • #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.

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

  10. hebasto force-pushed on Feb 6, 2020
  11. 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.

  12. DrahtBot cross-referenced this on Feb 11, 2020 from issue Use wtxid for transaction relay by sdaftuar
  13. DrahtBot cross-referenced this on Feb 11, 2020 from issue p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater by Empact
  14. DrahtBot cross-referenced this on Mar 20, 2020 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  15. DrahtBot cross-referenced this on Mar 27, 2020 from issue test: Add test for wtxid transaction relay by fjahr
  16. DrahtBot cross-referenced this on Mar 29, 2020 from issue net processing: Make it more obvious that we'll never upgrade a pre-segwit node to high-bandwidth by jnewbery
  17. DrahtBot cross-referenced this on May 7, 2020 from issue Refactor and slightly stricter p2p message processing by jonasschnelli
  18. DrahtBot cross-referenced this on May 22, 2020 from issue refactor: replace CNode pointers by references within net_processing.{h,cpp} by theStack
  19. DrahtBot cross-referenced this on May 29, 2020 from issue p2p: Move all header verification into the network layer, extend logging by troygiorshev
  20. DrahtBot added the label Needs rebase on Jun 4, 2020
  21. hebasto force-pushed on Jun 5, 2020
  22. DrahtBot removed the label Needs rebase on Jun 5, 2020
  23. DrahtBot cross-referenced this on Jun 5, 2020 from issue refactor: replace CConnman pointers by references in net_processing.cpp by theStack
  24. DrahtBot cross-referenced this on Jun 5, 2020 from issue p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS by jnewbery
  25. hebasto force-pushed on Jun 6, 2020
  26. hebasto commented at 6:57 AM on June 6, 2020: member

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

  27. DrahtBot cross-referenced this on Jun 17, 2020 from issue [net] Cleanup logic around connection types by amitiuttarwar
  28. DrahtBot cross-referenced this on Jun 17, 2020 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
  29. DrahtBot cross-referenced this on Jul 8, 2020 from issue [net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() by jnewbery
  30. DrahtBot added the label Needs rebase on Jul 10, 2020
  31. hebasto force-pushed on Jul 10, 2020
  32. hebasto commented at 6:18 PM on July 10, 2020: member

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

  33. DrahtBot removed the label Needs rebase on Jul 10, 2020
  34. hebasto commented at 7:57 PM on July 10, 2020: member

    @jnewbery Mind looking into this PR?

  35. 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?
  36. jnewbery cross-referenced this on Jul 11, 2020 from issue Remove unused constants `CADDR_TIME_VERSION` and `GETHEADERS_VERSION` by jnewbery
  37. MarcoFalke commented at 11:47 AM on July 11, 2020: member

    Concept ACK

  38. DrahtBot cross-referenced this on Jul 13, 2020 from issue Add parameter feature to serialization and use it for CAddress by sipa
  39. DrahtBot cross-referenced this on Jul 14, 2020 from issue Per-Peer Message Capture by troygiorshev
  40. DrahtBot cross-referenced this on Jul 14, 2020 from issue p2p: banscore updates to gui, tests, release notes by jonatack
  41. DrahtBot added the label Needs rebase on Jul 15, 2020
  42. hebasto force-pushed on Aug 21, 2020
  43. 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.

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

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

  46. DrahtBot removed the label Needs rebase on Aug 21, 2020
  47. DrahtBot cross-referenced this on Aug 21, 2020 from issue net, rpc: expose high bandwidth mode state via getpeerinfo by theStack
  48. DrahtBot cross-referenced this on Aug 21, 2020 from issue [net] Cleanup connection types- followups by amitiuttarwar
  49. DrahtBot cross-referenced this on Aug 21, 2020 from issue Net processing: move ProcessMessage() to PeerLogicValidation by jnewbery
  50. DrahtBot cross-referenced this on Aug 21, 2020 from issue Do not hide compile-time thread safety warnings by hebasto
  51. 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.

  52. DrahtBot added the label Needs rebase on Aug 24, 2020
  53. hebasto force-pushed on Aug 24, 2020
  54. hebasto commented at 11:29 PM on August 24, 2020: member

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

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

  55. DrahtBot removed the label Needs rebase on Aug 24, 2020
  56. DrahtBot cross-referenced this on Aug 25, 2020 from issue p2p: Remove fGetAddr flag by mzumsande
  57. jnewbery commented at 10:32 AM on August 25, 2020: member

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

  58. DrahtBot added the label Needs rebase on Sep 1, 2020
  59. hebasto force-pushed on Sep 1, 2020
  60. hebasto commented at 7:30 AM on September 1, 2020: member

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

  61. hebasto force-pushed on Sep 1, 2020
  62. hebasto commented at 8:09 AM on September 1, 2020: member

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

  63. jnewbery commented at 8:35 AM on September 1, 2020: member

    utACK e68912bc559197d3c82c581a1f64ec4c06f119da

  64. DrahtBot removed the label Needs rebase on Sep 1, 2020
  65. DrahtBot cross-referenced this on Sep 1, 2020 from issue Periodically make block-relay connections and sync headers by sdaftuar
  66. DrahtBot cross-referenced this on Sep 2, 2020 from issue p2p: Improve diversification of new connections by naumenkogs
  67. DrahtBot added the label Needs rebase on Sep 3, 2020
  68. hebasto force-pushed on Sep 3, 2020
  69. hebasto commented at 6:14 PM on September 3, 2020: member

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

  70. DrahtBot removed the label Needs rebase on Sep 3, 2020
  71. 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)

  72. DrahtBot cross-referenced this on Sep 4, 2020 from issue eBPF Linux tracepoints by jb55
  73. DrahtBot cross-referenced this on Sep 7, 2020 from issue [net processing] Move Misbehaving() to PeerManager by jnewbery
  74. DrahtBot added the label Needs rebase on Sep 7, 2020
  75. p2p: Unify Send and Receive protocol versions
    There is no change in behavior on the P2P network.
    e9a6d8b13b
  76. refactor: Rename local variable nSendVersion 8d2026796a
  77. 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
  78. p2p: Use the greatest common version in peer logic ddefb5c0b7
  79. hebasto force-pushed on Sep 7, 2020
  80. hebasto commented at 6:08 PM on September 7, 2020: member

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

  81. DrahtBot removed the label Needs rebase on Sep 7, 2020
  82. benthecarman approved
  83. benthecarman commented at 8:41 PM on September 7, 2020: contributor

    utACK ddefb5c

  84. jnewbery commented at 10:44 AM on September 8, 2020: member

    ACK ddefb5c0b759950942ac03f28c43b548af7b4033

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

  85. hebasto commented at 11:06 AM on September 8, 2020: member

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

  86. naumenkogs commented at 2:31 PM on September 8, 2020: member

    ACK ddefb5c0b759950942ac03f28c43b548af7b4033 good refactoring

  87. 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 12: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!

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

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

  90. amitiuttarwar commented at 12: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.

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

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

  93. fjahr commented at 1:56 PM on September 9, 2020: contributor

    Code review ACK ddefb5c0b759950942ac03f28c43b548af7b4033

    Nice cleanup!

  94. amitiuttarwar commented at 12: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.

  95. laanwj merged this on Sep 21, 2020
  96. laanwj closed this on Sep 21, 2020
  97. sidhujag referenced this in commit bf463b4f86 on Sep 22, 2020
  98. hebasto deleted the branch on Sep 22, 2020
  99. 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

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

  101. MarcoFalke cross-referenced this on Oct 12, 2020 from issue net: Assume that SetCommonVersion is called at most once per peer by MarcoFalke
  102. MarcoFalke referenced this in commit 00f4dcd552 on Dec 7, 2020
  103. sidhujag referenced this in commit 6a7d7717e8 on Dec 7, 2020
  104. Fabcien referenced this in commit 21862c3cda on Feb 9, 2021
  105. bitcoin locked this on Feb 15, 2022
Contributors
hebastoMarcoFalkeohldlaanwjDrahtBotjnewberybenthecarmannaumenkogsamitiuttarwarfjahr
Labels
RefactoringP2P
Linked (view graph)
#19067 tests: Add fuzzing harness for CNode#19486 Remove unused constants `CADDR_TIME_VERSION` and `GETHEADERS_VERSION`#19791 [net processing] Move Misbehaving() to PeerManager#20131 test: Remove unused nVersion=1 in p2p tests#20138 net: Assume that SetCommonVersion is called at most once per peer
github-metadata-mirror

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: 2026-05-20 06:54 UTC