net: Log unknown CInv in PushInventory #15945

Bushstar commented at 10:12 AM on May 3, 2019: contributor

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

fanquake added the label P2P on May 3, 2019

Bushstar commented at 11:22 AM on May 3, 2019: contributor

Travis ran out of time on the last build, please press rebuild on Travis.

https://travis-ci.org/bitcoin/bitcoin/builds/527704947?utm_source=github_status&utm_medium=notification

Bushstar commented at 12:36 PM on May 3, 2019: contributor

From Travis

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

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.

Bushstar force-pushed on May 3, 2019

DrahtBot commented at 1:41 PM on May 3, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15253 (Net: Consistently log outgoing INV messages by Empact)
#14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
#14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)

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.

laanwj commented at 6:50 AM on May 6, 2019: member

Is this purely an implementation bug if this happens or can it be network-triggered?

in src/net.h:853 in 7f0d7e7ed5 outdated

 847 | @@ -848,6 +848,8 @@ class CNode
 848 |              }
 849 |          } else if (inv.type == MSG_BLOCK) {
 850 |              vInventoryBlockToSend.push_back(inv.hash);
 851 | +        } else {
 852 | +            LogPrint(BCLog::NET, "%s: Unknown CInv type:%s\n", __func__, inv.ToString());

promag commented at 2:03 PM on May 6, 2019:

This is the only log call, should include loggin.h if this is accepted.

Bushstar commented at 5:13 PM on May 6, 2019:

It's including LogPrint in logging.h indirectly via system.h, for example addrman.h > system.h > logging.h.

promag commented at 5:18 PM on May 6, 2019:

We tend to https://include-what-you-use.org/.

Bushstar commented at 5:27 PM on May 6, 2019:

Good to know, thanks.

promag commented at 2:12 PM on May 6, 2019: member

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();

Bushstar commented at 5:15 PM on May 6, 2019: contributor

Is this purely an implementation bug if this happens or can it be network-triggered?

This will only be triggered via implementation most likely downstream of Bitcoin!

laanwj commented at 10:19 AM on May 8, 2019: member

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.

net: Log unknown CInv in PushInventory 9811ff83e8

Bushstar force-pushed on May 8, 2019

Bushstar commented at 12:35 PM on May 8, 2019: contributor

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 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

in src/net.h:854 in 9811ff83e8

 848 | @@ -848,6 +849,9 @@ class CNode
 849 |              }
 850 |          } else if (inv.type == MSG_BLOCK) {
 851 |              vInventoryBlockToSend.push_back(inv.hash);
 852 | +        } else {
 853 | +            LogPrint(BCLog::NET, "%s: Unknown CInv type:%s\n", __func__, inv.ToString());
 854 | +            assert(false); // Should never happen in Bitcoin, only MSG_TX/MSG_BLOCK expected.

MarcoFalke commented at 1:04 PM on May 8, 2019:

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).

Bushstar commented at 2:01 PM on May 8, 2019:

The PR below splits PushInventory into two.

https://github.com/bitcoin/bitcoin/pull/15253

promag commented at 10:36 PM on May 9, 2019: member

Closed by #15992?

laanwj commented at 3:44 PM on May 20, 2019: member

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.

laanwj closed this on May 20, 2019

DrahtBot locked this on Dec 16, 2021