net: Log unknown CInv in PushInventory#15945
Conversation
|
Travis ran out of time on the last build, please press rebuild on Travis. |
|
From Travis
I'll recreate the commit and force push to get it to build again, I'd expect this to pass as every other build completed and the change is trivial. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
Is this purely an implementation bug if this happens or can it be network-triggered? |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
NACK, temporarily add the log if you are pushing other CInv. Unless you want to abort after the log:
} else {
fprintf(stderr, "%s: Unknown CInv type:%s\n", __func__, inv.ToString());
abort();
This will only be triggered via implementation most likely downstream of Bitcoin! |
In that case, I think if this merits any change at all, an assertion (this is invalid input for this function) makes more sense. |
|
Updated to throw an assert if the CInv type is neither MSG_BLOCK or MSG_TX, all five calls to PushInventory pass a CInv with its type member set to one of MSG_BLOCK or MSG_TX. I've linked all instance below where the CInv is created. I'll add this to the first comment for quick reference. https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/interfaces/chain.cpp#L286-L287 |
| vInventoryBlockToSend.push_back(inv.hash); | ||
| } else { | ||
| LogPrint(BCLog::NET, "%s: Unknown CInv type:%s\n", __func__, inv.ToString()); | ||
| assert(false); // Should never happen in Bitcoin, only MSG_TX/MSG_BLOCK expected. |
There was a problem hiding this comment.
Block relay and transaction relay have little in common. Over a run time assert, I'd rather make it impossible at compile time that no other types can be pushed. One way to achieve this is to remove PushInventory and add a method CNode::PushInventoryBlock(const&uint256 hash) (and PushInventoryTx respectively).
There was a problem hiding this comment.
The PR below splits PushInventory into two.
|
Closed by #15992? |
|
Closing, sorry, this is seemingly too controversial and hogging too much reviewer time for a change that doesn't accomplish anything in the current code. |
6 participants
Message for anyone adding an experimental or malformed CInv and using the PushInventory. Experimental Cinv should probably be added to its own vector for processing elsewhere like setInventoryTxToSend and vInventoryBlockToSend.
Update: Including the five locations below that call PushInventory with a CInv with its member variable 'type' set to either MSG_BLOCK or MSG_TX.
https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/interfaces/chain.cpp#L286-L287
https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/net_processing.cpp#L1206-L1210
https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/net_processing.cpp#L2264
https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/net_processing.cpp#L3680
https://github.com/bitcoin/bitcoin/blob/7f0d7e7ed5b2a6a14a3fc9e5bf4d99073017b01e/src/node/transaction.cpp#L72-L75