Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
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.
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.
1984@@ -1985,7 +1985,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr<const CBlock> &blo
1985 void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock)
1986 {
1987 auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);
1988- const CNetMsgMaker msgMaker(PROTOCOL_VERSION);
1989+ const NetMsgMaker msgMaker{};
const NetMsgMaker msgMaker; with no curly brackets?
I like {}, because it has many benefits:
--word-diff-regex=., when in the future a designated initializer or constructor argument is added11@@ -12,14 +12,14 @@
12 class CNetMsgMaker
13 {
14 public:
15- explicit CNetMsgMaker(int nVersionIn) : nVersion(nVersionIn){}
16+ explicit CNetMsgMaker(int /*unused*/) {}
NetMsgMaker objects anymore after this change – NetMsgMaker::Make() could just be a global/static function.
Correct, but it seems easier to make any such cleanups in your follow-up when you move the PushMsg method from connman to cnode/connection. You could then also add a second PushMsg template overload that takes the message parts and internally calls the NetMsgMaker::Make(), thus completely removing it in most call places.
Happy to do that here, if you think this pull is too small to review, or would otherwise benefit from it.
I haven’t tried it, but I presume that introducing the second template overload requires some header/module shuffling to avoid circular dependencies.
Happy to take a look, I just didn’t want to step on your toes of the cnode/connection pull request (or anything else you are working on).
NetMsgMaker entirely into PushMessage at that point, perhaps. shrug
Could move
NetMsgMakerentirely intoPushMessageat that point, perhaps. shrug
I don’t think so, because a message may be constructed once and then sent to several peers, so this wouldn’t be a refactor and potentially cause a performance penalty?
Edit: See for example the code around const std::shared_future<CSerializedNetMsg> lazy_ser.
There’s not really any need for a
NetMsgMakerobjects anymore after this change –NetMsgMaker::Make()could just be a global/static function.
Thanks, done in #28892 (comment)
Could move
NetMsgMakerentirely intoPushMessageat that point, perhaps. shrug
FWIW, NetMsgMaker was originally created exactly to prevent this for the sake of logical separation. The idea is/was that net shouldn’t have any idea how to serialize a (for example) CBlock, rather it should just receive the dumb bytes.
template<typename T...> PushMessage(CNode& node, T... stuff) that accepts CBlock and so on would be in net_processing.cpp, and that would call CNode::PushMessage(msg) after serializing? That would fit with https://github.com/ajtowns/bitcoin/commit/6be38ef045cd1b974ae13d705260e83b769a48b5 at least. I’m just making this up as I go along though :man_shrugging:
PeerManagerImpl. That should be simple enough to push here as well. I’ll try to do that next week.
The field is unused, so remove it.
This is also required for future commits.
utACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452
I’m also ok with a commit in this PR to drop NetMsgMaker entirely.
856@@ -858,13 +857,13 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
857 std::chrono::microseconds time_received_dummy{0};
858
859 const auto msg_version =
860- msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
861+ NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
PROTOCOL_VERSION here I think.
There is a stale
PROTOCOL_VERSIONhere I think.
Are you sure? According to the Bitcoin wiki the first 4 bytes in the version message are there to " Identifies protocol version being used by the node “, so changing this may change the test, so better done in a follow-up?
The goal here is only to remove the serialize version and type.
1279@@ -1277,14 +1280,14 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
1280 // As per BIP152, we only get 3 of our peers to announce
1281 // blocks using compact encodings.
1282 m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){
1283- m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
1284+ PushMessage(*pnodeStop, NetMsg::Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
Calling NetMsg::Make within PushMessage seems unnecessary; just add an extra PeerManagerImpl helper:
0 template <typename... Args>
1 void PushMessage(CNode& node, std::string msg_type, Args&&... args) const
2 {
3 PushMessage(node, NetMsg::Make(std::move(msg_type), args...));
4 }
cf https://github.com/ajtowns/bitcoin/commit/0fe129b1b437d39221cef843f969f1adc31b543e
void MakeAndPushMessage(node, msg_type, args) to make that clear, I guess?
MakeAndPushMessage https://github.com/ajtowns/bitcoin/commit/0fe129b1b437d39221cef843f969f1adc31b543e
Thanks, done both. Looks like you forgot std::forward? Added it, and added you as co-author
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
242 uint32_t m_message_size{0}; //!< size of the payload
243 uint32_t m_raw_message_size{0}; //!< used wire size of the message (including header/checksum)
244 std::string m_type;
245
246- CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {}
247+ CNetMessage(DataStream&& recv_in) : m_recv(std::move(recv_in)) {}
explicit to this?