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

net: Add and document network messages in protocol.h #7181

Merged
merged 1 commit into from Dec 11, 2015

Conversation

Projects
None yet
9 participants
@laanwj
Member

laanwj commented Dec 7, 2015

  • Avoids string typos (by making the compiler check).
  • Makes it easier to grep for handling/generation of a certain message type.
  • Refer directly to documentation by following the symbol in IDE or doxygen.
  • Make sure we have an overview of all handled/emitted messages, for functionality like #6589.

@harding I've used your descriptions of the message types, hope you don't mind :)

Edit: sendheaders is not yet documented in the developer docs, this is tracked here: bitcoin-dot-org/bitcoin.org#1153

@laanwj laanwj added Docs P2P labels Dec 7, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 7, 2015

Member

@jonasschnelli second commit moves your logAllowIncomingMsgCmds to protocol.cpp, I think this makes sense to have everything related to message types together.

Member

laanwj commented Dec 7, 2015

@jonasschnelli second commit moves your logAllowIncomingMsgCmds to protocol.cpp, I think this makes sense to have everything related to message types together.

@@ -140,3 +172,9 @@ std::string CInv::ToString() const
{
return strprintf("%s %s", GetCommand(), hash.ToString());
}

This comment has been minimized.

@GIJensen

GIJensen Dec 7, 2015

Is this double newline supposed to be here? I noticed it earlier in the file too (unrelated to this change).

@GIJensen

GIJensen Dec 7, 2015

Is this double newline supposed to be here? I noticed it earlier in the file too (unrelated to this change).

This comment has been minimized.

@laanwj

laanwj Dec 7, 2015

Member

No ,not on purpose, will remove it

@laanwj

laanwj Dec 7, 2015

Member

No ,not on purpose, will remove it

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 7, 2015

Member

Careful Code Review ACK.
Nice. This change was long overdue!

Member

jonasschnelli commented Dec 7, 2015

Careful Code Review ACK.
Nice. This change was long overdue!

@GIJensen

This comment has been minimized.

Show comment
Hide comment
@GIJensen

GIJensen Dec 7, 2015

utACK, I was thinking about how we needed this today!

GIJensen commented Dec 7, 2015

utACK, I was thinking about how we needed this today!

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Dec 7, 2015

Contributor

ACK

Contributor

paveljanik commented Dec 7, 2015

ACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Dec 7, 2015

Contributor

ut ACK

Cosmetic nit: It would be nice to break up allNetMessageTypes[] into one-per-line in the source code, permitting a more simple future patch of

 + NET_MSG_MYNEWMSG,

versus

 - NET_MSG_A, NET_MSG_B, NET_MSG_C, NET_MSG_D
 + NET_MSG_A, NET_MSG_B, NET_MSG_MYNEWMSG, NET_MSG_C, NET_MSG_D

for future patch readability and simplicity.

Contributor

jgarzik commented Dec 7, 2015

ut ACK

Cosmetic nit: It would be nice to break up allNetMessageTypes[] into one-per-line in the source code, permitting a more simple future patch of

 + NET_MSG_MYNEWMSG,

versus

 - NET_MSG_A, NET_MSG_B, NET_MSG_C, NET_MSG_D
 + NET_MSG_A, NET_MSG_B, NET_MSG_MYNEWMSG, NET_MSG_C, NET_MSG_D

for future patch readability and simplicity.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 7, 2015

Contributor

er... why?

Contributor

pstratem commented Dec 7, 2015

er... why?

@harding

This comment has been minimized.

Show comment
Hide comment
@harding

harding Dec 8, 2015

Member

Note, PR bitcoin-dot-org/bitcoin.org#1154 has been opened in the Bitcoin.org repository to add brief sendheaders documentation at the URL referenced in this PR. Thanks!

Member

harding commented Dec 8, 2015

Note, PR bitcoin-dot-org/bitcoin.org#1154 has been opened in the Bitcoin.org repository to add brief sendheaders documentation at the URL referenced in this PR. Thanks!

@dcousens

View changes

Show outdated Hide outdated src/protocol.cpp
NET_MSG_NOTFOUND, NET_MSG_FILTERLOAD, NET_MSG_FILTERADD, NET_MSG_FILTERCLEAR, NET_MSG_REJECT,
NET_MSG_SENDHEADERS
};
const static std::vector<std::string> allNetMessageTypesVec(allNetMessageTypes, allNetMessageTypes+ARRAYLEN(allNetMessageTypes));

This comment has been minimized.

@dcousens

dcousens Dec 8, 2015

Contributor

1 day, initializer lists.

@dcousens

dcousens Dec 8, 2015

Contributor

1 day, initializer lists.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 8, 2015

Contributor

utACK f5b1d97 (and verified consistent constant usage).

Agreed this will make grepping way easier, plus it documents each command with clear entry points for new features etc.

Contributor

dcousens commented Dec 8, 2015

utACK f5b1d97 (and verified consistent constant usage).

Agreed this will make grepping way easier, plus it documents each command with clear entry points for new features etc.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 8, 2015

Member

er... why?

Is there anything unclear from the opening post?
If you really want to know the secret reason is: "makes maintenance for me easier".

Cosmetic nit: It would be nice to break up allNetMessageTypes[] into one-per-line in the source code, permitting a more simple future patch of

Makes sense.

Member

laanwj commented Dec 8, 2015

er... why?

Is there anything unclear from the opening post?
If you really want to know the secret reason is: "makes maintenance for me easier".

Cosmetic nit: It would be nice to break up allNetMessageTypes[] into one-per-line in the source code, permitting a more simple future patch of

Makes sense.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 8, 2015

Member

Rebased and squashed after #7180

Member

laanwj commented Dec 8, 2015

Rebased and squashed after #7180

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 8, 2015

Member

@theuni Hope this doesn't collide with your P2P work?

Member

laanwj commented Dec 8, 2015

@theuni Hope this doesn't collide with your P2P work?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 8, 2015

Member

Any opinion on putting the constants in a namespace instead of NET_MSG_ prefix?
NET_MSG_SENDHEADERS would become e.g. NetMsgType::SENDHEADERS. This is syntactically more in line with C++11 strongly typed enumerations.
(could make it an actual enumeration at some point provided something else does conversion from/to string, which would make comparison/switching easier, but that's not the goal here)

Member

laanwj commented Dec 8, 2015

Any opinion on putting the constants in a namespace instead of NET_MSG_ prefix?
NET_MSG_SENDHEADERS would become e.g. NetMsgType::SENDHEADERS. This is syntactically more in line with C++11 strongly typed enumerations.
(could make it an actual enumeration at some point provided something else does conversion from/to string, which would make comparison/switching easier, but that's not the goal here)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Dec 8, 2015

Contributor

+1!

Contributor

paveljanik commented Dec 8, 2015

+1!

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 8, 2015

Member

Agree with using a namespace instead of the NET_MSG_ prefix.

Member

jonasschnelli commented Dec 8, 2015

Agree with using a namespace instead of the NET_MSG_ prefix.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 8, 2015

Contributor

@laanwj hmm ok

utACK 399551f2c246844f8a8c4994142c6e04bf283778

Contributor

pstratem commented Dec 8, 2015

@laanwj hmm ok

utACK 399551f2c246844f8a8c4994142c6e04bf283778

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Dec 9, 2015

Contributor

@laanwj Yes - we should start using namespaces. IMO first step is Bitcoin:: and second step is Net:: under that.

Contributor

jgarzik commented Dec 9, 2015

@laanwj Yes - we should start using namespaces. IMO first step is Bitcoin:: and second step is Net:: under that.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 9, 2015

Contributor

reACK 399551f

Contributor

dcousens commented Dec 9, 2015

reACK 399551f

net: Add and document network messages in protocol.h
- Avoids string typos (by making the compiler check)
- Makes it easier to grep for handling/generation of a certain message type
- Refer directly to documentation by following the symbol in IDE
- Move list of valid message types to protocol.cpp:
    protocol.cpp is a more appropriate place for this, and having
    the array there makes it easier to keep things consistent.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 10, 2015

Member

utACK. Thanks!

Member

morcos commented Dec 10, 2015

utACK. Thanks!

@laanwj laanwj merged commit 9bbe71b into bitcoin:master Dec 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 11, 2015

Merge pull request #7181
9bbe71b net: Add and document network messages in protocol.h (Wladimir J. van der Laan)

laanwj added a commit that referenced this pull request Dec 11, 2015

net: Add and document network messages in protocol.h
- Avoids string typos (by making the compiler check)
- Makes it easier to grep for handling/generation of a certain message type
- Refer directly to documentation by following the symbol in IDE
- Move list of valid message types to protocol.cpp:
    protocol.cpp is a more appropriate place for this, and having
    the array there makes it easier to keep things consistent.

Github-Pull: #7181
Rebased-From: 9bbe71b

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 29, 2017

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable…
… per-command byte counters in `CNode` (#1496)

* log bytes recv/sent per command

* net: Account for `sendheaders` `verack` messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

spencerlievens added a commit to duality-solutions/Dynamic that referenced this pull request Nov 12, 2017

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181:
* enable per-command byte counters in  (#1496)

* log bytes recv/sent per command

* net: Account for   messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

AmirolAhmad pushed a commit to AmirolAhmad/gobytes-test that referenced this pull request Dec 11, 2017

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable…
… per-command byte counters in `CNode` (#1496)

* log bytes recv/sent per command

* net: Account for `sendheaders` `verack` messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

AmirolAhmad pushed a commit to AmirolAhmad/gobytes-test that referenced this pull request Dec 12, 2017

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable…
… per-command byte counters in `CNode` (#1496)

* log bytes recv/sent per command

* net: Account for `sendheaders` `verack` messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

TriKriSta added a commit to ivansib/sibcoin that referenced this pull request Mar 5, 2018

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable…
… per-command byte counters in `CNode` (#1496)

* log bytes recv/sent per command

* net: Account for `sendheaders` `verack` messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

TriKriSta added a commit to ivansib/sibcoin that referenced this pull request Mar 6, 2018

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable…
… per-command byte counters in `CNode` (#1496)

* log bytes recv/sent per command

* net: Account for `sendheaders` `verack` messages

Looks like these were forgotten in #6589.

* Backport remaining part of Bitcoin PR bitcoin/bitcoin#7181.

Most of this PR is already merged, but a small part remaining
that makes per-command byte counts in CNode working.

Signed-off-by: Oleg Girko <ol@infoserver.lv>

@dagurval dagurval referenced this pull request Jun 8, 2018

Merged

More net cherries #444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment