ProcessCompactBlockTxns
#26969
When processing CMPCTBLOCK message, at some moments we can need to process compact block txns / BLOCKTXN, since all messages are handled by ProcessMessage, so we call ProcessMessage all over again.
https://github.com/bitcoin/bitcoin/blob/ab98673f058853e00c310afad57925f54c1ecfae/src/net_processing.cpp#L4331-L4348
This PR creates a function called ProcessCompactBlockTxns to process it to avoid calling ProcessMessage for it - this function is also called when processing BLOCKTXN msg.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, ajtowns, achow101 |
| Concept ACK | Sjors |
| Stale ACK | dergoegge |
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.
4421@@ -4343,7 +4422,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4422 } // cs_main
4423
4424 if (fProcessBLOCKTXN) {
4425- return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, interruptMsgProc);
4426+ return ProcessBlockTxnMessage(pfrom, blockTxnMsg, *peer, msgMaker);
This isn’t really processing a BLOCKTXN message though – it’s completing the block immediately because a BLOCKTXN message wasn’t necessary?
Wouldn’t it be better to call it ProcessCompactBlockTxns and pass in the BlockTransactions directly, rather than serializing then unserializing it?
Wouldn’t it be better to call it ProcessCompactBlockTxns and pass in the BlockTransactions directly, rather than serializing then unserializing it?
I think you’re right, it would be better to pass BlockTransactions directly and not deal with serialization.
4459@@ -4386,77 +4460,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4460
4461 if (msg_type == NetMsgType::BLOCKTXN)
4462 {
4463- // Ignore blocktxn received while importing
BLOCKTXN messages fully before throwing them away. Previously the message was thrown away early.
Nice point, I think it makes sense to not let ProcessCompactBlockTxns check if we received blocktxnwhile importing, we can leave it for ProcessMessage. E.g.:
0@@ -3153,12 +3153,6 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
1 void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, BlockTransactions& block_transactions, Peer& peer,
2 const CNetMsgMaker& msg_maker)
3 {
4- // Ignore blocktxn received while importing
5- if (fImporting || fReindex) {
6- LogPrint(BCLog::NET, "Unexpected blocktxn message received from peer %d\n", pfrom.GetId());
7- return;
8- }
9-
10 std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
11 bool fBlockRead = false;
12 {
13@@ -4460,6 +4454,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
14
15 if (msg_type == NetMsgType::BLOCKTXN)
16 {
17+ if (fImporting || fReindex) {
18+ LogPrint(BCLog::NET, "Unexpected blocktxn message received from peer %d\n", pfrom.GetId());
19+ return;
20+ }
21 BlockTransactions resp;
22 vRecv >> resp;
4420@@ -4353,7 +4421,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4421 } // cs_main
4422
4423 if (fProcessBLOCKTXN) {
4424- return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, interruptMsgProc);
4425+ return ProcessCompactBlockTxns(pfrom, txn, *peer, msgMaker);
fProcessBLOCKTXN=true, and this variable removed altogether, in a pure refactoring way?
fProcessBLOCKTXN exists to call ProcessMessage out of cs_main block, but do you think we could call ProcessCompactBlockTxns there?
cs_main thing.
An easy refactoring (splitting ProcessBlock out of ProcessCompactBlockTxns) would allow avoid one unlock/lock operation, but I’m not sure it’s worth it.
907@@ -908,6 +908,10 @@ class PeerManagerImpl final : public PeerManager
908 /** Process a new block. Perform any post-processing housekeeping */
909 void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked);
910
911+ /** Process compact block txns */
912+ void ProcessCompactBlockTxns(CNode& pfrom, BlockTransactions& block_transactions, Peer& peer, const CNetMsgMaker& msg_maker)
913+ EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex, !m_most_recent_block_mutex);
3187+ return;
3188+ } else if (status == READ_STATUS_FAILED) {
3189+ // Might have collided, fall back to getdata now :(
3190+ std::vector<CInv> invs;
3191+ invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(peer), block_transactions.blockhash));
3192+ m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::GETDATA, invs));
return here, I think you can get rid of fBlockRead and do ProcessBlock unconditionally (!fBlockRead would be equivalent to leaving this function already).
907@@ -908,6 +908,10 @@ class PeerManagerImpl final : public PeerManager
908 /** Process a new block. Perform any post-processing housekeeping */
909 void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked);
910
911+ /** Process compact block txns */
912+ void ProcessCompactBlockTxns(CNode& pfrom, BlockTransactions& block_transactions, Peer& peer, const CNetMsgMaker& msg_maker)
0 void ProcessBlockTxns(CNode& pfrom, const Peer& peer, const BlockTransactions& block_transactions)
Just create a new CNetMsgMaker, no need to pass it.
At first glance, this doesn’t seem to interfere in any meaningful way with #27626
I promise to review this as a follow-up cleanup :pray:
Code review ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/355bc6098a7373feb1d59f9651a79e1477d22243
reviewed with git diff master --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra. very straight forward set of changes.
We have a small bug that you could do in a fixup commit as well perhaps: #27626 (review)
We have a small bug that you could do in a fixup commit as well perhaps: #27626 (review)
Gonna address it, thanks.
ProcessCompactBlockTxns instead of ProcessMessage. In the future that makes it easier to follow the history of changes.
918@@ -919,6 +919,10 @@ class PeerManagerImpl final : public PeerManager
919 /** Process a new block. Perform any post-processing housekeeping */
920 void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked);
921
922+ /** Process compact block txns */
923+ void ProcessCompactBlockTxns(CNode& pfrom, BlockTransactions& block_transactions, Peer& peer)
0 void ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const BlockTransactions& block_transactions)
I would prefer this order of the params (matches the other methods) and the BlockTransactions should be marked const.
re ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/686629d2be5545ef59cf0e97f4f3a74c6cde2efa
only differences beyond rebase is suggested changes for ProcessCompactBlockTxns argument ordering and const-ness of BlockTransactions arg.
4371- // message we typically jump to the BLOCKTXN handling code, with a
4372- // dummy (empty) BLOCKTXN message, to re-use the logic there in
4373- // completing processing of the putative block (without cs_main).
4374 bool fProcessBLOCKTXN = false;
4375- CDataStream blockTxnMsg(SER_NETWORK, PROTOCOL_VERSION);
4376+ BlockTransactions txn;
ProcessCompactBlockTxns ?
When processing `CMPCTBLOCK` message, at some moments
we can need to process cmpct block txns, since all messages
are handled by ProcessMessage, we call ProcessMessage
all over again. For this reason, it creates a function called
`ProcessCompactBlockTxns` to process it.
ACK 77d6d89d43cc5969c98d9b4b56a1e877b473e731
Verified diff is primarily a moveonly that removes the recursion for BLOCKTXN.