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

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2015_12_network_messages changing 5 files +283 −73
  1. laanwj commented at 3:30 PM on December 7, 2015: member
    • 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: https://github.com/bitcoin-dot-org/bitcoin.org/issues/1153

  2. laanwj added the label Docs and Output on Dec 7, 2015
  3. laanwj added the label P2P on Dec 7, 2015
  4. laanwj commented at 3:49 PM on December 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.

  5. laanwj force-pushed on Dec 7, 2015
  6. laanwj force-pushed on Dec 7, 2015
  7. in src/protocol.cpp:None in dbbdb279cf outdated 
     171 | @@ -140,3 +172,9 @@ std::string CInv::ToString() const
 172 |  {
 173 |      return strprintf("%s %s", GetCommand(), hash.ToString());
 174 |  }
 175 | +
    GIJensen commented at 4:38 PM on December 7, 2015:

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

    laanwj commented at 5:40 PM on December 7, 2015:

    No ,not on purpose, will remove it

  8. jonasschnelli commented at 4:38 PM on December 7, 2015: contributor

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

  9. GIJensen commented at 4:40 PM on December 7, 2015: none

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

  10. laanwj force-pushed on Dec 7, 2015
  11. paveljanik commented at 5:32 PM on December 7, 2015: contributor

    ACK

  12. jgarzik commented at 8:58 PM on December 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.

  13. pstratem commented at 11:58 PM on December 7, 2015: contributor

    er... why?

  14. harding commented at 1:05 AM on December 8, 2015: contributor

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

  15. in src/protocol.cpp:None in b875758590 outdated 
      52 | +    NET_MSG_MERKLEBLOCK, NET_MSG_GETBLOCKS, NET_MSG_GETHEADERS, NET_MSG_TX, NET_MSG_HEADERS,
  53 | +    NET_MSG_BLOCK, NET_MSG_GETADDR, NET_MSG_MEMPOOL, NET_MSG_PING, NET_MSG_PONG, NET_MSG_ALERT,
  54 | +    NET_MSG_NOTFOUND, NET_MSG_FILTERLOAD, NET_MSG_FILTERADD, NET_MSG_FILTERCLEAR, NET_MSG_REJECT,
  55 | +    NET_MSG_SENDHEADERS
  56 | +    };
  57 | +const static std::vector<std::string> allNetMessageTypesVec(allNetMessageTypes, allNetMessageTypes+ARRAYLEN(allNetMessageTypes));
    dcousens commented at 2:02 AM on December 8, 2015:

    1 day, initializer lists.

  16. dcousens commented at 2:03 AM on December 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.

  17. laanwj commented at 8:44 AM on December 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.

  18. laanwj force-pushed on Dec 8, 2015
  19. laanwj commented at 9:31 AM on December 8, 2015: member

    Rebased and squashed after #7180

  20. laanwj commented at 11:59 AM on December 8, 2015: member

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

  21. laanwj commented at 12:53 PM on December 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)

  22. paveljanik commented at 12:56 PM on December 8, 2015: contributor

    +1!

  23. jonasschnelli commented at 12:59 PM on December 8, 2015: contributor

    Agree with using a namespace instead of the NET_MSG_ prefix.

  24. pstratem commented at 8:56 PM on December 8, 2015: contributor

    @laanwj hmm ok

    utACK 399551f2c246844f8a8c4994142c6e04bf283778

  25. jgarzik commented at 1:20 AM on December 9, 2015: contributor

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

  26. dcousens commented at 2:30 AM on December 9, 2015: contributor

    reACK 399551f

  27. 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.
    9bbe71b641
  28. laanwj force-pushed on Dec 10, 2015
  29. morcos commented at 8:29 PM on December 10, 2015: member

    utACK. Thanks!

  30. laanwj merged this on Dec 11, 2015
  31. laanwj closed this on Dec 11, 2015
  32. laanwj referenced this in commit d1e17ff640 on Dec 11, 2015
  33. laanwj referenced this in commit 8fc174aae6 on Dec 11, 2015
  34. UdjinM6 referenced this in commit a443d4e2d0 on Jun 29, 2017
  35. spencerlievens referenced this in commit a3648a6d0f on Nov 12, 2017
  36. AmirolAhmad referenced this in commit 833a603996 on Dec 11, 2017
  37. AmirolAhmad referenced this in commit 2016a96d58 on Dec 12, 2017
  38. furszy referenced this in commit 87feface84 on May 8, 2020
  39. zkbot referenced this in commit 89d9e557e1 on Feb 17, 2021
  40. DrahtBot locked this on Sep 8, 2021
Contributors
laanwjGIJensenjonasschnellipaveljanikjgarzikpstratemhardingdcousensmorcos
Labels
DocsP2P
Linked (view graph)
#6589 log bytes recv/sent per command
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-04-13 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me