net: make the list of known message types a compile time constant #29421

pull vasild wants to merge 2 commits into bitcoin:master from vasild:message_types_compile_constant changing 6 files +80 −131
  1. vasild commented at 3:40 pm on February 11, 2024: contributor

    Turn the std::vector to std::array because it is cheaper and allows us to have the number of the messages as a compile time constant: ALL_NET_MESSAGE_TYPES.size() which can be used in future code to build other std::arrays with that size.


    This change is part of #29418 but it makes sense on its own and would be good to have it, regardless of the fate of #29418. Also, if this is merged, that would reduce the size of #29418, thus the current standalone PR.

  2. DrahtBot commented at 3:40 pm on February 11, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack, maflcko, willcl-ark, achow101
    Concept ACK epiccurious, Empact

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)

    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.

  3. DrahtBot added the label P2P on Feb 11, 2024
  4. epiccurious commented at 11:40 pm on February 11, 2024: none
    Concept ACK 2e312ea909ae6c466007acf791ea0e3356e954cb.
  5. in src/protocol.cpp:50 in 2e312ea909 outdated
    46@@ -47,47 +47,6 @@ const char* WTXIDRELAY = "wtxidrelay";
    47 const char* SENDTXRCNCL = "sendtxrcncl";
    48 } // namespace NetMsgType
    49 
    50-/** All known message types. Keep this in the same order as the list of
    


    willcl-ark commented at 11:37 am on February 14, 2024:

    There is a comment in src/protocol.h#56 which could be updated to reflect the new constant too:

    0/**
    1 * Bitcoin protocol message types. When adding new message types, don't forget
    2 * to update allNetMessageTypes in protocol.cpp.
    3 */
    

    vasild commented at 4:47 pm on February 14, 2024:
    Updated, thanks!
  6. willcl-ark approved
  7. willcl-ark commented at 11:46 am on February 14, 2024: member

    ACK 2e312ea909ae6c466007acf791ea0e3356e954cb

    This is a clean tidy-up, and as mentioned useful for #29418 but stands on it’s own (as a smalle improvement) too.

    Left one req for comment update if touched again.

  8. vasild force-pushed on Feb 14, 2024
  9. vasild commented at 4:47 pm on February 14, 2024: contributor
    2e312ea909...808fe4ef80: update comment, see #29421 (review)
  10. in src/net.cpp:3704 in 808fe4ef80 outdated
    3700@@ -3701,7 +3701,7 @@ CNode::CNode(NodeId idIn,
    3701 {
    3702     if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND);
    3703 
    3704-    for (const std::string &msg : getAllNetMessageTypes())
    3705+    for (const auto& msg : g_all_net_message_types)
    


    maflcko commented at 8:46 am on February 15, 2024:
    0    for (const auto& msg : g_all_net_message_types) {
    

    nit: Add missing { while touching?


    vasild commented at 2:47 pm on February 15, 2024:
    Done.
  11. in src/protocol.h:271 in 808fe4ef80 outdated
    266@@ -267,8 +267,44 @@ extern const char* WTXIDRELAY;
    267 extern const char* SENDTXRCNCL;
    268 }; // namespace NetMsgType
    269 
    270-/* Get a vector of all valid message types (see above) */
    271-const std::vector<std::string>& getAllNetMessageTypes();
    272+/** All known message types (see above). Keep this in the same order as the list of messages above. */
    273+static const std::array g_all_net_message_types{
    


    maflcko commented at 8:53 am on February 15, 2024:

    Why not constexpr?

    Also, this may put a copy of this array in all translation units, but I guess this is fine, as there is no other way to achieve it.


    vasild commented at 2:48 pm on February 15, 2024:

    Done as a separate commit because it required changing NetMsgType::* constants to constexpr as well.

    A +36 / -77 change, thanks!


    maflcko commented at 2:55 pm on February 15, 2024:
    Now it may put all string literals in all translation units as well. Would be good to benchmark the effect, if any.

    vasild commented at 7:00 pm on February 15, 2024:

    This is relevant: https://stackoverflow.com/questions/45987571/difference-between-constexpr-and-static-constexpr-global-variable

    I made the following experiment:

    s.h:

    0constexpr const char* s{"aabbcc"};
    

    a.cc:

    0#include "s.h"
    1#include <stdio.h>
    2void a()
    3{
    4    printf("a(): %p, %s\n", &s, s);
    5}
    

    b.cc:

    0#include "s.h"
    1#include <stdio.h>
    2void b()
    3{
    4    printf("b(): %p, %s\n", &s, s);
    5}
    

    main.cc:

    0void a();
    1void b();
    2int main(int, char**)
    3{
    4    a();
    5    b();
    6    return 0;
    7}
    
    0$ clang++-devel -c a.cc -o a.o
    1$ clang++-devel -c b.cc -o b.o
    2$ clang++-devel -c main.cc -o main.o
    3$ clang++-devel a.o b.o main.o -o main
    4$ ./main
    5a(): 0x200578, aabbcc
    6b(): 0x200580, aabbcc
    

    Also checked that the main executable contains the string aabbcc just once, so it is not duplicated. But there are two different pointers pointing to it.

    Then I changed the header s.h to:

    0inline constexpr const char* s{"aabbcc"};
    

    And re-ran:

    0$ ./main
    1a(): 0x200578, aabbcc
    2b(): 0x200578, aabbcc
    

    now it is the same pointer.

    So, I added inline to the variables involved.


    vasild commented at 1:55 pm on February 28, 2024:
    In our case the variables are inside a namespace. Hinted by #29421 (review) I changed the above to put s inside a namespace and got the same result as above. Tried gcc 12, clang 15/17/19.
  12. in src/test/fuzz/p2p_transport_serialization.cpp:30 in 808fe4ef80 outdated
    27 void initialize_p2p_transport_serialization()
    28 {
    29     ECC_Start();
    30     SelectParams(ChainType::REGTEST);
    31-    g_all_messages = getAllNetMessageTypes();
    32     std::sort(g_all_messages.begin(), g_all_messages.end());
    


    maflcko commented at 8:55 am on February 15, 2024:
    Why the sort? Also, are you sure that sorting pointers is comparing the content it points to, as opposed to the memory address value?

    vasild commented at 2:50 pm on February 15, 2024:

    Why the sort?

    #28165 (review)

    You are right that comparing the pointers here is wrong. I added a custom sort function to compare the contents. Thanks!

  13. vasild force-pushed on Feb 15, 2024
  14. in src/test/fuzz/p2p_transport_serialization.cpp:33 in c29350f4d7 outdated
    31-    g_all_messages = getAllNetMessageTypes();
    32-    std::sort(g_all_messages.begin(), g_all_messages.end());
    33+    // Sort, so that changes in the order of elements in g_all_net_message_types
    34+    // do not cause changes in fuzzing for the same fuzz input.
    35+    std::sort(g_all_messages.begin(), g_all_messages.end(), [](const char* a, const char* b) {
    36+        return strcmp(a, b) < 0;
    


    maflcko commented at 2:53 pm on February 15, 2024:
    nit: If string_view are stored, the compiler can derive this code and you won’t have to change this line of code.

    vasild commented at 2:58 pm on February 15, 2024:

    Just tried that - if I store the constants as string_view, e.g. constexpr std::string_view VERSION{"version"};, then I get this greeting:

    0test/util/net.cpp:34:9: error: no matching function for call to 'Make'
    1   34 |         NetMsg::Make(NetMsgType::VERSION,
    2      |         ^~~~~~~~~~~~
    3./netmessagemaker.h:14:23: note: candidate function template not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'std::string' (aka 'basic_string<char>') for 1st argument
    4   14 |     CSerializedNetMsg Make(std::string msg_type, Args&&... args)
    5      |                       ^    ~~~~~~~~~~~~~~~~~~~~
    

    maflcko commented at 1:12 pm on February 19, 2024:

    I meant just for the array, for now.

    0inline constexpr std::array g_all_net_message_types{std::to_array<std::string_view>({
    

    vasild commented at 1:36 pm on February 28, 2024:

    Then another:

    0net.cpp:3705:31: error: no viable overloaded operator[] for type 'mapMsgTypeSize' (aka 'map<basic_string<char>, unsigned long>')
    1 3705 |         mapRecvBytesPerMsgType[msg] = 0;
    2      |         ~~~~~~~~~~~~~~~~~~~~~~^~~~
    3/usr/include/c++/v1/map:1203:18: note: candidate function not viable: no known conversion from 'const value_type' (aka 'const std::string_view') to 'const key_type' (aka 'const std::string') for 1st argument
    4 1203 |     mapped_type& operator[](const key_type& __k);
    5      |                  ^          ~~~~~~~~~~~~~~~~~~~
    

    It is possible to get over that by changing mapMsgTypeSize and NET_MESSAGE_TYPE_OTHER to:

    0inline constexpr std::string_view NET_MESSAGE_TYPE_OTHER{"*other*"};
    1using mapMsgTypeSize = std::map</* message type */ std::string_view, /* total bytes */ uint64_t>;
    

    but then there is another error from rpc/net.cpp and I think this would spill off the scope of this PR.


    maflcko commented at 1:46 pm on February 28, 2024:

    You are replacing a vector of std::string objects with an array of raw pointers. I don’t think this is a benefit, as it will lead to silently wrong code. (Recall that you forgot to call strcmp initially?)

    Reviewers will have to review everything in any case. Whether the code is touched or not doesn’t matter. What matters is whether the code changes behavior or not, and whether the change in behavior is intentional.


    vasild commented at 5:04 pm on February 28, 2024:
    Changed to std::string and dropped the std::sort modification. Thanks!
  15. vasild force-pushed on Feb 15, 2024
  16. in src/protocol.h:65 in c7c613c414 outdated
    63 /**
    64  * The version message provides information about the transmitting node to the
    65  * receiving node at the beginning of a connection.
    66  */
    67-extern const char* VERSION;
    68+inline constexpr const char* VERSION{"version"};
    


    Empact commented at 6:58 am on February 27, 2024:

    You can drop inline as constexpr is implicitly inline.

    A static data member (i.e. static class member or namespace-scope variable) declared constexpr on its first declaration is implicitly an inline variable. https://en.cppreference.com/w/cpp/language/inline


    vasild commented at 2:07 pm on February 28, 2024:

    constexpr is implicitly inline

    No, static constexpr is implicitly inline. And there is no static in our case. If I add static, then those would be defined in each .cpp file. See #29421 (review). The best is to leave it inline constexpr (without static) because then they are defined just once for all .cpp files.

  17. in src/protocol.h:270 in c7c613c414 outdated
    304 }; // namespace NetMsgType
    305 
    306-/* Get a vector of all valid message types (see above) */
    307-const std::vector<std::string>& getAllNetMessageTypes();
    308+/** All known message types (see above). Keep this in the same order as the list of messages above. */
    309+inline constexpr std::array g_all_net_message_types{
    


    Empact commented at 7:07 am on February 27, 2024:
    As a constant rather than a variable, seems this should be ALL_NET_MESSAGE_TYPES. See developer-notes.md.

    vasild commented at 2:38 pm on February 28, 2024:
    Done
  18. Empact commented at 7:12 am on February 27, 2024: member
    Concept ACK
  19. maflcko commented at 1:51 pm on February 28, 2024: member
    Seems fine, but NACK on the approach. This switches a vector of std::string objects (which is type-safe) to an array of raw pointers (not type-safe). This has already lead to a bug in this pull request, and is asking for more bugs to happen in the future, so I don’t think it is worth it as-is.
  20. vasild force-pushed on Feb 28, 2024
  21. vasild commented at 2:38 pm on February 28, 2024: contributor
    c7c613c414...fd6a481995: rename g_all_net_message_types to ALL_NET_MESSAGE_TYPES as per #29421 (review)
  22. vasild force-pushed on Feb 28, 2024
  23. vasild commented at 5:00 pm on February 28, 2024: contributor
    @maflcko good point! Changed from const char* to std::string in fd6a481995...546d13274d (can’t have constexpr with std::string, thus dropped the constexpr, it was never the point of this PR).
  24. net: make the list of known message types a compile time constant
    Turn the `std::vector` to `std::array` because it is cheaper and
    allows us to have the number of the messages as a compile time
    constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in
    future code to build other `std::array`s with that size.
    2fa9de06c2
  25. protocol: make message types constexpr
    As a side effect the names of the constants do not have to be
    repeated in `src/protocol.cpp`.
    b3efb48673
  26. vasild force-pushed on Feb 28, 2024
  27. vasild commented at 5:05 pm on February 28, 2024: contributor
    546d13274d...b3efb48673: remove now unnecessary explicit conversion to std::string in SimulationTest().
  28. DrahtBot added the label CI failed on Feb 28, 2024
  29. DrahtBot removed the label CI failed on Feb 29, 2024
  30. vasild commented at 7:53 am on March 8, 2024: contributor
    @maflcko, this has a NACK from you and I have addressed your concern. If not ACK, it would be nice if you can at least remove your NACK. Thanks for the review so far!
  31. vasild commented at 6:58 pm on May 2, 2024: contributor
    ping @maflcko
  32. jonatack approved
  33. jonatack commented at 7:29 pm on May 16, 2024: contributor
    ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
  34. DrahtBot requested review from willcl-ark on May 16, 2024
  35. DrahtBot requested review from Empact on May 16, 2024
  36. in src/protocol.cpp:10 in b3efb48673
     6@@ -7,87 +7,6 @@
     7 
     8 #include <common/system.h>
     9 
    10-#include <atomic>
    


    maflcko commented at 2:42 pm on May 17, 2024:
    unrelated change, but looks fine
  37. maflcko approved
  38. maflcko commented at 2:47 pm on May 17, 2024: member

    Sorry for missing the ping. Now it looks good, because the only change is vector->array and some style fixups.

    utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊
    3SkLePLuDXM+ymjKH3KbyoYmBU/CTns4Bd4K8JycRF2zRDl2t3QMwQrGtZcNnOZ88NGBjgAWuIpsGTMsQzwHKBw==
    
  39. willcl-ark approved
  40. willcl-ark commented at 1:47 pm on May 20, 2024: member

    ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef

    Based on range diff from previous ACK, all looks good! range-diff 2e312ea...b3efb48

  41. achow101 commented at 5:50 pm on May 21, 2024: member
    ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
  42. achow101 merged this on May 21, 2024
  43. achow101 closed this on May 21, 2024

  44. in src/protocol.h:65 in b3efb48673
    63 /**
    64  * The version message provides information about the transmitting node to the
    65  * receiving node at the beginning of a connection.
    66  */
    67-extern const char* VERSION;
    68+inline constexpr const char* VERSION{"version"};
    


    laanwj commented at 6:56 pm on May 21, 2024:
    Might as well make these std::string? Or is there still a good reason that these are char*? (it used to be that the message-making functions took char*, but i don’t think that’s the case anymore)

    willcl-ark commented at 7:38 pm on May 21, 2024:
    IIRC this was left because constexpr std::string was not available, but perhaps it is now in c++20?

    laanwj commented at 7:57 pm on May 21, 2024:
    i mean it’s defining an array of std::string constexpr below it, so i would expect single std::strings to be possible too, i haven’t tried it though

    maflcko commented at 10:32 am on May 22, 2024:
    Yeah, it could make sense to switch this from brittle raw pointers literals to type-safe string_view or string in a follow-up.

    maflcko commented at 10:34 am on May 22, 2024:
    Ah, IIRC string_view was tried in this pull and needed more patches, so it was left for a follow-up what to do here.

    vasild commented at 10:32 am on May 28, 2024:

    This was tried: #29421 (review)

    Here is a draft that changes to std::string_view the following:

    • NetMsgType::*
    • ALL_NET_MESSAGE_TYPES
    • CMessageHeader:pchCommand (renamed to CMessageHeader:m_command)
    • CNetMessage::m_type
    • V2_MESSAGE_IDS
    • V2MessageMap::m_map
    • V2Transport::m_send_type

    and the functions that handle message types.

    The scope of this can be reduced by converting the std::string_view to std::string at some point. The previous code uses std::string (or char*) everywhere.

    https://github.com/vasild/bitcoin/commits/NetMsgType_string_view/

  45. vasild deleted the branch on May 24, 2024

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: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me