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.
<!--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.
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.
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{};
Why not 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*/) {}
There's not really any need for a 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.
Fair enough
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).
Could move 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.
@theuni I think my idea there was that 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:
Oh, I see you'd add the function as a method to PeerManagerImpl. That should be simple enough to push here as well. I'll try to do that next week.
Done
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.
ACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452
Removed the class and replaced it with a template function in a namespace to address the feedback of both reviewers (Thanks!)
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));
There is a stale 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.
Oof, yes, you're right.
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:
template <typename... Args>
void PushMessage(CNode& node, std::string msg_type, Args&&... args) const
{
PushMessage(node, NetMsg::Make(std::move(msg_type), args...));
}
cf https://github.com/ajtowns/bitcoin/commit/0fe129b1b437d39221cef843f969f1adc31b543e
Correct, but I wasn't sure if reviewers preferred the slightly more verbose way to clarify that serialization is involved on every call.
Could give it a distinct name like 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)) {}
Maybe add explicit to this?
Thanks, will do in one of the follow-up pulls, or here if I have to re-touch, whichever happens earlier.
According to https://corecheck.dev/bitcoin/bitcoin/pulls/28892 there were three instances in total, so I went ahead and fixed them all.
ACK fafd5e5becf035426a45179ff592c11ccf2d5da5
reACK fa79a881ce0537e1d74da296a7513730438d2a02