net: Add and document network messages in protocol.h by laanwj · Pull Request #7181 · bitcoin/bitcoin

laanwj · 2015-12-07T15:30:36Z

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 log bytes recv/sent per command #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 · 2015-12-07T15:49:16Z

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

GIJensen · 2015-12-07T16:38:25Z

src/protocol.cpp

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

No ,not on purpose, will remove it

jonasschnelli · 2015-12-07T16:38:33Z

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

GIJensen · 2015-12-07T16:40:57Z

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

paveljanik · 2015-12-07T17:32:37Z

ACK

jgarzik · 2015-12-07T20:58:44Z

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 · 2015-12-07T23:58:50Z

er... why?

harding · 2015-12-08T01:05:03Z

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 · 2015-12-08T02:02:14Z

src/protocol.cpp

1 day, initializer lists.

dcousens · 2015-12-08T02:03:24Z

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 · 2015-12-08T08:44:59Z

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 · 2015-12-08T09:31:42Z

Rebased and squashed after #7180

laanwj · 2015-12-08T11:59:43Z

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

laanwj · 2015-12-08T12:53:00Z

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 · 2015-12-08T12:56:56Z

+1!

jonasschnelli · 2015-12-08T12:59:08Z

Agree with using a namespace instead of the NET_MSG_ prefix.

pstratem · 2015-12-08T20:56:55Z

@laanwj hmm ok

utACK 399551f2c246844f8a8c4994142c6e04bf283778

jgarzik · 2015-12-09T01:20:56Z

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

dcousens · 2015-12-09T02:30:46Z

reACK 399551f

- 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 · 2015-12-10T20:29:38Z

utACK. Thanks!

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

- 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

… 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#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 <[email protected]>

* 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 <[email protected]>

… 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 <[email protected]>

26b1d9e Apply clang-format (Fuzzbawls) f12f540 Add and document layer 2 network messages to protocol.h (Fuzzbawls) 7a176ed net: Add and document network messages in protocol.h (Wladimir J. van der Laan) Pull request description: Ported from bitcoin#7181 with layer 2 messages in a followup commit, then ran both commits through `clang-format-diff.py` > - 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. ACKs for top commit: random-zebra: Very nice. utACK 26b1d9e furszy: ACK 26b1d9e Tree-SHA512: a1d08ffb940a66b03856779180e00c212234adc8af3f175e67f694b040e426f35de2cfe6725a3bcdcb606c0468cc33e17e1882a0e2d7f657eebfdb273f0f523c

Use SipHash for node eviction Backport of bitcoin/bitcoin#8173 Commits are listed in stack order. - pick bitcoin/bitcoin@eebc232187 - pick bitcoin/bitcoin@888483098e - pick bitcoin/bitcoin@c31b24f745 - pick bitcoin/bitcoin@9bf156bb9e - pick bitcoin/bitcoin@053930ffc4 - missing bitcoin/bitcoin#5697 (not a candidate for direct inclusion), using pieces directly - conflicts with #1258 which it removes part of (this is ok) - ignore bitcoin/bitcoin#6374 - requires bitcoin/bitcoin#7906, or at least bitcoin/bitcoin@cca221fd21 - pick bitcoin/bitcoin@cca221fd21 - conflicts with bitcoin/bitcoin#7181, not needed until later if at all

laanwj added Docs P2P labels Dec 7, 2015

laanwj force-pushed the 2015_12_network_messages branch 2 times, most recently from 2746d63 to dbbdb27 Compare December 7, 2015 15:54

GIJensen reviewed Dec 7, 2015
View reviewed changes

laanwj force-pushed the 2015_12_network_messages branch from dbbdb27 to 68a3b9f Compare December 7, 2015 17:15

dcousens reviewed Dec 8, 2015
View reviewed changes

src/protocol.cpp Outdated

Copy link

Contributor

dcousens Dec 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 day, initializer lists.

laanwj force-pushed the 2015_12_network_messages branch from 2e5ecc1 to 985ac4b Compare December 8, 2015 09:08

laanwj force-pushed the 2015_12_network_messages branch from 399551f to 9bbe71b Compare December 10, 2015 11:15

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

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

Merge pull request #7181

d1e17ff

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

str4d mentioned this pull request Jun 28, 2017

Fix banning and disallow sending messages before receiving verack zcash/zcash#2479

Open

OlegGirko mentioned this pull request Jun 29, 2017

Backport Bitcoin PRs #6589, #7180 and remaining part of #7181: enable per-command byte counters in CNode dashpay/dash#1496

Merged

str4d mentioned this pull request Jul 14, 2017

Bitcoin 0.12 P2P/Net PRs 1 zcash/zcash#2534

Draft

dagurval mentioned this pull request Jun 8, 2018

More net cherries bitcoinxt/bitcoinxt#444

Merged

This was referenced Apr 20, 2020

[Net] Add and document network messages in protocol.h PIVX-Project/PIVX#1542

Merged

Tracking: Backport upstream network encapsulation PIVX-Project/PIVX#1374

Closed

PRCYDev mentioned this pull request Jan 25, 2021

Tracking: Backport upstream network encapsulation PRCYCoin/PRCYCoin#4

Open

21 tasks

This was referenced Feb 11, 2021

Use SipHash for node eviction zcash/zcash#4980

Closed

Use SipHash for node eviction zcash/zcash#4985

Merged

nuttycom mentioned this pull request Feb 23, 2021

Verify that expected services are available from peer nodes & disconnect otherwise. zcash/zcash#5004

Draft

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

laanwj commented Dec 7, 2015

Uh oh!

laanwj commented Dec 7, 2015

Uh oh!

GIJensen Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 7, 2015

Uh oh!

GIJensen commented Dec 7, 2015

Uh oh!

paveljanik commented Dec 7, 2015

Uh oh!

jgarzik commented Dec 7, 2015

Uh oh!

pstratem commented Dec 7, 2015

Uh oh!

harding commented Dec 8, 2015

Uh oh!

dcousens Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

dcousens commented Dec 8, 2015

Uh oh!

laanwj commented Dec 8, 2015

Uh oh!

laanwj commented Dec 8, 2015

Uh oh!

laanwj commented Dec 8, 2015

Uh oh!

laanwj commented Dec 8, 2015

Uh oh!

paveljanik commented Dec 8, 2015

Uh oh!

jonasschnelli commented Dec 8, 2015

Uh oh!

pstratem commented Dec 8, 2015

Uh oh!

jgarzik commented Dec 9, 2015

Uh oh!

dcousens commented Dec 9, 2015

Uh oh!

morcos commented Dec 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants