p2p: Unify Send and Receive protocol versions #17785

hebasto · 2019-12-20T18:43:48Z

On master (6fef85b) 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.

maflcko · 2019-12-20T18:51:47Z

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.

hebasto · 2019-12-20T18:54:31Z

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?

laanwj · 2020-01-13T10:36:26Z

If correct, this is a very nice simplification.

DrahtBot · 2020-01-17T06:57:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

eBPF Linux tracepoints #19866 (eBPF Linux tracepoints by jb55)
net, rpc: expose high bandwidth mode state via getpeerinfo #19776 (net, rpc: expose high bandwidth mode state via getpeerinfo by theStack)
Per-Peer Message Capture #19509 (Per-Peer Message Logging by troygiorshev)
p2p: Move all header verification into the network layer, extend logging #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 · 2020-02-06T12:13:58Z

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 · 2020-02-06T16:21:43Z

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

hebasto · 2020-06-06T06:57:52Z

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

jnewbery · 2020-09-04T08:04:04Z

utACK 646ddf1

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

There is no change in behavior on the P2P network.

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

hebasto · 2020-09-07T18:08:43Z

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

benthecarman

utACK ddefb5c

jnewbery · 2020-09-08T10:44:11Z

ACK ddefb5c

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

hebasto · 2020-09-08T11:06:22Z

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

naumenkogs · 2020-09-08T14:31:09Z

ACK ddefb5c
good refactoring

amitiuttarwar

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: amitiuttarwar@ef2b862. 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.

src/net_processing.cpp

amitiuttarwar · 2020-09-08T23:28:28Z

src/net.h

+        m_greatest_common_version = greatest_common_version;
    }
-    int GetRecvVersion() const
+    int GetCommonVersion() const


nit: missing newline between functions

amitiuttarwar · 2020-09-08T23:52:45Z

src/net.h

    bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete);

-    void SetRecvVersion(int nVersionIn)
+    void SetCommonVersion(int greatest_common_version)


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.

hebasto · 2020-09-09T06:45:57Z

@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 · 2020-09-09T08:45:37Z

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 · 2020-09-09T13:56:16Z

Code review ACK ddefb5c

Nice cleanup!

amitiuttarwar · 2020-09-10T00:40:08Z

code review but untested ACK ddefb5c

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.

ddefb5c p2p: Use the greatest common version in peer logic (Hennadii Stepanov) e084d45 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov) 8d20267 refactor: Rename local variable nSendVersion (Hennadii Stepanov) e9a6d8b p2p: Unify Send and Receive protocol versions (Hennadii Stepanov) Pull request description: On master (6fef85b) `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. ACKs for top commit: jnewbery: ACK ddefb5c naumenkogs: ACK ddefb5c fjahr: Code review ACK ddefb5c amitiuttarwar: code review but untested ACK ddefb5c benthecarman: utACK `ddefb5c` Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d

maflcko · 2020-10-12T11:14:26Z

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

maflcko · 2020-10-12T11:17:23Z

src/net.cpp

-    // once a version message has been successfully processed. Any attempt to
-    // set this twice is an error.
-    if (nSendVersion != 0) {
-        error("Send version already set for node: %i. Refusing to change from %i to %i", id, nSendVersion, nVersionIn);


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

Fixed in #20138

…e per peer fa0f415 net: Assume that SetCommonVersion is called at most once per peer (MarcoFalke) Pull request description: This restores the check removed in bitcoin/bitcoin#17785 (comment) Instead of using `error`, which was used previously, it uses a newly introduced `Assume()`. `error` had several issues: * It logs unconditionally to the debug log * It doesn't abort the program when the error is hit in tests ACKs for top commit: practicalswift: cr ACK fa0f415: patch looks correct jnewbery: utACK fa0f415 Tree-SHA512: cd7424a9485775e8c7093b725f8f52a90d47485185e79bac80f7810e450d0b3fda608d8805e9239094929f7bad2dca3fe772fb78ae606c2399d15405521e136b

…ost once per peer fa0f415 net: Assume that SetCommonVersion is called at most once per peer (MarcoFalke) Pull request description: This restores the check removed in bitcoin#17785 (comment) Instead of using `error`, which was used previously, it uses a newly introduced `Assume()`. `error` had several issues: * It logs unconditionally to the debug log * It doesn't abort the program when the error is hit in tests ACKs for top commit: practicalswift: cr ACK fa0f415: patch looks correct jnewbery: utACK fa0f415 Tree-SHA512: cd7424a9485775e8c7093b725f8f52a90d47485185e79bac80f7810e450d0b3fda608d8805e9239094929f7bad2dca3fe772fb78ae606c2399d15405521e136b

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#17785 | PR17785]]. Depends on D9193. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9195

fanquake added the P2P label Dec 20, 2019

maflcko added the Refactoring label Dec 20, 2019

ohld approved these changes Dec 21, 2019

View reviewed changes

hebasto force-pushed the 20191220-unify-protocol-versions branch from 8698f92 to 25b93f1 Compare February 6, 2020 16:20

This was referenced Feb 11, 2020

Use wtxid for transaction relay #18044

Merged

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

Merged

DrahtBot mentioned this pull request Mar 20, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

This was referenced Mar 27, 2020

test: Add test for wtxid transaction relay #18446

Closed

net processing: Make it more obvious that we'll never upgrade a pre-segwit node to high-bandwidth #18461

Closed

DrahtBot mentioned this pull request May 7, 2020

Refactor and slightly stricter p2p message processing #15197

Closed

This was referenced May 22, 2020

refactor: replace CNode pointers by references within net_processing.{h,cpp} #19053

Merged

p2p: Move all header verification into the network layer, extend logging #19107

Merged

DrahtBot added the Needs rebase label Jun 4, 2020

hebasto force-pushed the 20191220-unify-protocol-versions branch from 25b93f1 to 5a89c17 Compare June 5, 2020 07:26

DrahtBot removed the Needs rebase label Jun 5, 2020

This was referenced Jun 5, 2020

refactor: replace CConnman pointers by references in net_processing.cpp #19174

Merged

p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS #19070

Merged

hebasto force-pushed the 20191220-unify-protocol-versions branch from 5a89c17 to cc5dba8 Compare June 6, 2020 06:57

This was referenced Jun 17, 2020

[net] Cleanup logic around connection types #19316

Merged

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

DrahtBot mentioned this pull request Jul 8, 2020

[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() #19472

Merged

DrahtBot added the Needs rebase label Jul 10, 2020

hebasto force-pushed the 20191220-unify-protocol-versions branch from cc5dba8 to e268f64 Compare July 10, 2020 18:13

This was referenced Sep 4, 2020

eBPF Linux tracepoints #19866

Merged

[net processing] Move Misbehaving() to PeerManager #19791

Merged

DrahtBot added the Needs rebase label Sep 7, 2020

hebasto added 4 commits September 7, 2020 21:03

p2p: Unify Send and Receive protocol versions

e9a6d8b

There is no change in behavior on the P2P network.

refactor: Rename local variable nSendVersion

8d20267

p2p: Remove SetCommonVersion() from VERACK handler

e084d45

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

p2p: Use the greatest common version in peer logic

ddefb5c

hebasto force-pushed the 20191220-unify-protocol-versions branch from 646ddf1 to ddefb5c Compare September 7, 2020 18:07

DrahtBot removed the Needs rebase label Sep 7, 2020

benthecarman approved these changes Sep 7, 2020

View reviewed changes

amitiuttarwar reviewed Sep 9, 2020

View reviewed changes

laanwj merged commit 7737603 into bitcoin:master Sep 21, 2020

hebasto deleted the 20191220-unify-protocol-versions branch September 22, 2020 06:35

maflcko reviewed Oct 12, 2020

View reviewed changes

maflcko mentioned this pull request Oct 12, 2020

net: Assume that SetCommonVersion is called at most once per peer #20138

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

p2p: Unify Send and Receive protocol versions #17785

p2p: Unify Send and Receive protocol versions #17785

Uh oh!

Conversation

hebasto commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 20, 2019

Uh oh!

hebasto commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jan 13, 2020

Uh oh!

DrahtBot commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Feb 6, 2020

Uh oh!

hebasto commented Feb 6, 2020

Uh oh!

hebasto commented Jun 6, 2020

Uh oh!

jnewbery commented Sep 4, 2020

Uh oh!

hebasto commented Sep 7, 2020

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 8, 2020

Uh oh!

hebasto commented Sep 8, 2020

Uh oh!

naumenkogs commented Sep 8, 2020

Uh oh!

amitiuttarwar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amitiuttarwar Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 9, 2020

Uh oh!

jnewbery commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Sep 9, 2020

Uh oh!

amitiuttarwar commented Sep 10, 2020

Uh oh!

maflcko commented Oct 12, 2020

Uh oh!

maflcko Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

hebasto commented Dec 20, 2019 •

edited

Loading

hebasto commented Dec 20, 2019 •

edited

Loading

DrahtBot commented Jan 17, 2020 •

edited

Loading

jnewbery commented Sep 9, 2020 •

edited

Loading