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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept ACK 2e312ea909ae6c466007acf791ea0e3356e954cb.
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:
/**
* Bitcoin protocol message types. When adding new message types, don't forget
* to update allNetMessageTypes in protocol.cpp.
*/
Updated, thanks!
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)
for (const auto& msg : g_all_net_message_types) {
nit: Add missing { while touching?
Done.
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!
Now it may put all string literals in all translation units as well. Would be good to benchmark the effect, if any.
This is relevant: https://stackoverflow.com/questions/45987571/difference-between-constexpr-and-static-constexpr-global-variable
I made the following experiment:
s.h:
constexpr const char* s{"aabbcc"};
a.cc:
#include "s.h"
#include <stdio.h>
void a()
{
printf("a(): %p, %s\n", &s, s);
}
b.cc:
#include "s.h"
#include <stdio.h>
void b()
{
printf("b(): %p, %s\n", &s, s);
}
main.cc:
void a();
void b();
int main(int, char**)
{
a();
b();
return 0;
}
$ clang++-devel -c a.cc -o a.o
$ clang++-devel -c b.cc -o b.o
$ clang++-devel -c main.cc -o main.o
$ clang++-devel a.o b.o main.o -o main
$ ./main
a(): 0x200578, aabbcc
b(): 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:
inline constexpr const char* s{"aabbcc"};
And re-ran:
$ ./main
a(): 0x200578, aabbcc
b(): 0x200578, aabbcc
now it is the same pointer.
So, I added inline to the variables involved.
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.
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? Also, are you sure that sorting pointers is comparing the content it points to, as opposed to the memory address value?
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;
nit: If 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:
test/util/net.cpp:34:9: error: no matching function for call to 'Make'
34 | NetMsg::Make(NetMsgType::VERSION,
| ^~~~~~~~~~~~
./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
14 | CSerializedNetMsg Make(std::string msg_type, Args&&... args)
| ^ ~~~~~~~~~~~~~~~~~~~~
I meant just for the array, for now.
inline constexpr std::array g_all_net_message_types{std::to_array<std::string_view>({
Then another:
net.cpp:3705:31: error: no viable overloaded operator[] for type 'mapMsgTypeSize' (aka 'map<basic_string<char>, unsigned long>')
3705 | mapRecvBytesPerMsgType[msg] = 0;
| ~~~~~~~~~~~~~~~~~~~~~~^~~~
/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
1203 | mapped_type& operator[](const key_type& __k);
| ^ ~~~~~~~~~~~~~~~~~~~
It is possible to get over that by changing mapMsgTypeSize and NET_MESSAGE_TYPE_OTHER to:
inline constexpr std::string_view NET_MESSAGE_TYPE_OTHER{"*other*"};
using 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.
Changed to 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{
As a constant rather than a variable, seems this should be ALL_NET_MESSAGE_TYPES. See developer-notes.md.
Done
Concept ACK
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.
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().
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
6 | @@ -7,87 +7,6 @@ 7 | 8 | #include <common/system.h> 9 | 10 | -#include <atomic>
unrelated change, but looks fine
Sorry for missing the ping. Now it looks good, because the only change is vector->array and some style fixups.
utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊
SkLePLuDXM+ymjKH3KbyoYmBU/CTns4Bd4K8JycRF2zRDl2t3QMwQrGtZcNnOZ88NGBjgAWuIpsGTMsQzwHKBw==
</details>
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
Based on range diff from previous ACK, all looks good!
range-diff 2e312ea...b3efb48
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
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"};
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)
IIRC this was left because constexpr std::string was not available, but perhaps it is now in c++20?
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
Yeah, it could make sense to switch this from brittle raw pointers literals to type-safe string_view or string in a follow-up.
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.
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/