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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, 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.
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
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 */
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.
2e312ea909...808fe4ef80: update comment, see #29421 (review)
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)
0 for (const auto& msg : g_all_net_message_types) {
nit: Add missing { while touching?
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{
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.
Done as a separate commit because it required changing NetMsgType::* constants to constexpr as well.
A +36 / -77 change, thanks!
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.
s inside a namespace and got the same result as above. Tried gcc 12, clang 15/17/19.
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());
Why the sort?
You are right that comparing the pointers here is wrong. I added a custom sort function to compare the contents. Thanks!
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;
string_view are stored, the compiler can derive this code and you won’t have to change this line of code.
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 | ^ ~~~~~~~~~~~~~~~~~~~~
I meant just for the array, for now.
0inline constexpr std::array g_all_net_message_types{std::to_array<std::string_view>({
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.
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.
std::string and dropped the std::sort modification. Thanks!
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"};
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
constexpris implicitlyinline
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.
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{
ALL_NET_MESSAGE_TYPES. See developer-notes.md.
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.
c7c613c414...fd6a481995: rename g_all_net_message_types to ALL_NET_MESSAGE_TYPES as per #29421 (review)
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.
As a side effect the names of the constants do not have to be
repeated in `src/protocol.cpp`.
546d13274d...b3efb48673: remove now unnecessary explicit conversion to std::string in SimulationTest().
6@@ -7,87 +7,6 @@
7
8 #include <common/system.h>
9
10-#include <atomic>
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==
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
Based on range diff from previous ACK, all looks good!
range-diff 2e312ea...b3efb48
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"};
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)
constexpr std::string was not available, but perhaps it is now in c++20?
std::string constexpr below it, so i would expect single std::strings to be possible too, i haven’t tried it though
string_view or string in a follow-up.
string_view was tried in this pull and needed more patches, so it was left for a follow-up what to do here.
This was tried: #29421 (review)
Here is a draft that changes to std::string_view the following:
NetMsgType::*ALL_NET_MESSAGE_TYPESCMessageHeader:pchCommand (renamed to CMessageHeader:m_command)CNetMessage::m_typeV2_MESSAGE_IDSV2MessageMap::m_mapV2Transport::m_send_typeand 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/