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

vasild · 2024-02-11T15:40:54Z

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.

DrahtBot · 2024-02-11T15:40:56Z

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.

epiccurious · 2024-02-11T23:40:47Z

Concept ACK 2e312ea909ae6c466007acf791ea0e3356e954cb.

willcl-ark

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.

src/protocol.cpp

vasild · 2024-02-14T16:47:52Z

2e312ea909...808fe4ef80: update comment, see #29421 (comment)

src/net.cpp

src/protocol.h

src/test/fuzz/p2p_transport_serialization.cpp

src/protocol.h

Empact · 2024-02-27T07:12:48Z

Concept ACK

maflcko · 2024-02-28T13:51:12Z

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.

vasild · 2024-02-28T14:38:17Z

c7c613c414...fd6a481995: rename g_all_net_message_types to ALL_NET_MESSAGE_TYPES as per #29421 (comment)

vasild · 2024-02-28T17:00:00Z

@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).

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`.

vasild · 2024-02-28T17:05:48Z

546d13274d...b3efb48673: remove now unnecessary explicit conversion to std::string in SimulationTest().

vasild · 2024-03-08T07:53:38Z

@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!

vasild · 2024-05-02T18:58:22Z

ping @maflcko

jonatack

ACK b3efb48

maflcko · 2024-05-17T14:42:56Z

src/protocol.cpp


 #include <common/system.h>

-#include <atomic>


unrelated change, but looks fine

maflcko

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

utACK b3efb48 🎊

Show signature

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==

willcl-ark

ACK b3efb48

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

achow101 · 2024-05-21T17:50:49Z

ACK b3efb48

laanwj · 2024-05-21T18:56:21Z

src/protocol.h

 * receiving node at the beginning of a connection.
 */
-extern const char* VERSION;
+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 (comment)

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/

DrahtBot added the P2P label Feb 11, 2024

DrahtBot mentioned this pull request Feb 12, 2024

rpc: add 'getnetmsgstats' RPC #28926

Closed

willcl-ark approved these changes Feb 14, 2024

View reviewed changes

src/protocol.cpp Outdated Show resolved Hide resolved

vasild force-pushed the message_types_compile_constant branch from 2e312ea to 808fe4e Compare February 14, 2024 16:45

maflcko reviewed Feb 15, 2024

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

src/test/fuzz/p2p_transport_serialization.cpp Outdated Show resolved Hide resolved

vasild mentioned this pull request Feb 15, 2024

net: transport abstraction #28165

Merged

vasild force-pushed the message_types_compile_constant branch from 808fe4e to c29350f Compare February 15, 2024 14:47

maflcko reviewed Feb 15, 2024

View reviewed changes

src/test/fuzz/p2p_transport_serialization.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 15, 2024

rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' #29418

Open

vasild force-pushed the message_types_compile_constant branch from c29350f to c7c613c Compare February 15, 2024 18:58

Empact reviewed Feb 27, 2024

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

Empact reviewed Feb 27, 2024

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

vasild force-pushed the message_types_compile_constant branch from c7c613c to fd6a481 Compare February 28, 2024 14:37

vasild force-pushed the message_types_compile_constant branch from fd6a481 to 546d132 Compare February 28, 2024 16:57

vasild added 2 commits February 28, 2024 18:03

protocol: make message types constexpr

b3efb48

As a side effect the names of the constants do not have to be repeated in `src/protocol.cpp`.

vasild force-pushed the message_types_compile_constant branch from 546d132 to b3efb48 Compare February 28, 2024 17:04

DrahtBot added CI failed and removed CI failed labels Feb 28, 2024

jonatack approved these changes May 16, 2024

View reviewed changes

DrahtBot requested review from Empact and willcl-ark May 16, 2024 19:29

maflcko reviewed May 17, 2024

View reviewed changes

src/protocol.cpp

#include <common/system.h>

#include <atomic>

Copy link

Member

maflcko May 17, 2024

Choose a reason for hiding this comment

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

unrelated change, but looks fine

maflcko approved these changes May 17, 2024

View reviewed changes

willcl-ark approved these changes May 20, 2024

View reviewed changes

achow101 merged commit 6c13b13 into bitcoin:master May 21, 2024

laanwj reviewed May 21, 2024

View reviewed changes

vasild deleted the message_types_compile_constant branch May 24, 2024 14:54

bitcoin locked and limited conversation to collaborators May 28, 2025

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

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

Uh oh!

Conversation

vasild commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

epiccurious commented Feb 11, 2024

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Feb 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Empact commented Feb 27, 2024

Uh oh!

maflcko commented Feb 28, 2024

Uh oh!

vasild commented Feb 28, 2024

Uh oh!

vasild commented Feb 28, 2024

Uh oh!

vasild commented Feb 28, 2024

Uh oh!

vasild commented Mar 8, 2024

Uh oh!

vasild commented May 2, 2024

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko May 17, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 21, 2024

Uh oh!

laanwj May 21, 2024

Choose a reason for hiding this comment

Uh oh!

willcl-ark May 21, 2024

Choose a reason for hiding this comment

Uh oh!

laanwj May 21, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko May 22, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko May 22, 2024

Choose a reason for hiding this comment

Uh oh!

vasild May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

vasild commented Feb 11, 2024 •

edited

Loading

DrahtBot commented Feb 11, 2024 •

edited

Loading

vasild May 28, 2024 •

edited

Loading