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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--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
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
It would be good to explain whether you are modifying the behavior or if this is a refactor. Currently your change forces nodes to de serialize 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.:
@@ -3153,12 +3153,6 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, BlockTransactions& block_transactions, Peer& peer,
const CNetMsgMaker& msg_maker)
{
- // Ignore blocktxn received while importing
- if (fImporting || fReindex) {
- LogPrint(BCLog::NET, "Unexpected blocktxn message received from peer %d\n", pfrom.GetId());
- return;
- }
-
std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
bool fBlockRead = false;
{
@@ -4460,6 +4454,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
if (msg_type == NetMsgType::BLOCKTXN)
{
+ if (fImporting || fReindex) {
+ LogPrint(BCLog::NET, "Unexpected blocktxn message received from peer %d\n", pfrom.GetId());
+ return;
+ }
BlockTransactions resp;
vRecv >> resp;
Done it in last force-pushed.
Rebased
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);
Can't this call be moved up where fProcessBLOCKTXN=true, and this variable removed altogether, in a pure refactoring way?
I don't think so. fProcessBLOCKTXN exists to call ProcessMessage out of cs_main block, but do you think we could call ProcessCompactBlockTxns there?
ahhhh you're right, it's the 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);
should have !cs_main, no?
why?
nevermind.
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));
if you add a return here, I think you can get rid of fBlockRead and do ProcessBlock unconditionally (!fBlockRead would be equivalent to leaving this function already).
I agree, will address 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)
void ProcessBlockTxns(CNode& pfrom, const Peer& peer, const BlockTransactions& block_transactions)
Just create a new CNetMsgMaker, no need to pass it.
Done, thanks!
Force-pushed addressing #26969 (review) (@dergoegge)
Rebased
Rebased
@instagibbs this conflicts with #27626, thoughts?
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.
Upon further consultation by @fanquake , I've been advised to just PR the change straight. keep this PR as is.
Concept ACK. Light preference for splitting this commit into a pure move-only and the (much smaller) change of calling ProcessCompactBlockTxns instead of ProcessMessage. In the future that makes it easier to follow the history of changes.
ACK 355bc6098a7373feb1d59f9651a79e1477d22243
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)
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.
Make sense. I'm going to address it
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.
Force-pushed addressing #26969 (review)
re ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa
Code review ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa
Rebased
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;
Shouldn't this be declared and initialized just prior to calling ProcessCompactBlockTxns ?
Sounds good, gonna address it.
reACK 89478af481b2e6d4652b2fe50ff5fe5b92cbd69e
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.
Force-pushed addressing #26969 (review)
reACK 77d6d89d43cc5969c98d9b4b56a1e877b473e731
utACK 77d6d89d43cc5969c98d9b4b56a1e877b473e731
ACK 77d6d89d43cc5969c98d9b4b56a1e877b473e731
Verified diff is primarily a moveonly that removes the recursion for BLOCKTXN.