-
Notifications
You must be signed in to change notification settings - Fork 35.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
p2p: Unify Send and Receive protocol versions #17785
p2p: Unify Send and Receive protocol versions #17785
Conversation
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? |
If correct, this is a very nice simplification. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
I'd suggest removing that, and explicitly say that there is no change in behavior on the P2P network. |
8698f92
to
25b93f1
Compare
Done. Thank you for your suggestion. |
25b93f1
to
5a89c17
Compare
5a89c17
to
cc5dba8
Compare
Rebased 25b93f1 -> cc5dba8 (pr17785.02 -> pr17785.04) due to the conflict with #19053. |
cc5dba8
to
e268f64
Compare
utACK 646ddf1 (verified |
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.
646ddf1
to
ddefb5c
Compare
Rebased 646ddf1 -> ddefb5c (pr17785.10 -> pr17785.11) due to the conflict with #19791. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ddefb5c
ACK ddefb5c Verified |
@MarcoFalke @amiti @sipa Mind looking into this PR? |
ACK ddefb5c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
int GetRecvVersion() const | ||
int GetCommonVersion() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing newline between functions
@@ -1065,16 +1064,14 @@ class CNode | |||
|
|||
bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete); | |||
|
|||
void SetRecvVersion(int nVersionIn) | |||
void SetCommonVersion(int greatest_common_version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@jnewbery raised the same question. This could be resolved in a follow-up PR. |
I think |
Code review ACK ddefb5c Nice cleanup! |
code review but untested ACK ddefb5c for another PR:
yeah, but I got slightly confused because based on this comment I thought you might have been getting rid of |
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
Post merge ACK, but would be good to remove nVersion from the tests as well in the last commit. See #20131 |
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to remove this debug log? I think it would be good to keep this to avoid regressing on this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
On master (6fef85b)
CNode
has two members to keep protocol version:nRecvVersion
for received messagesnSendVersion
for messages to sendAfter exchanging with
VERSION
andVERACK
messages via protocol versionINIT_PROTO_VERSION
, both nodes setnRecvVersion
andnSendVersion
to the same value which is the greatest common protocol version.This PR:
CNode
members,nRecvVersion
nSendVersion
, withm_greatest_common_version
There is no change in behavior on the P2P network.