p2p: change `CInv::type` from `int` to `uint32

jonatack commented at 11:11 am on July 28, 2020: member

Credit to Crypt-iQ for finding and reporting the UBSan implicit-integer-sign-change issue and to vasild for the original review suggestion in #19590#pullrequestreview-455788826.

Closes #19678.

DrahtBot added the label P2P on Jul 28, 2020

DrahtBot commented at 5:01 pm on July 28, 2020: 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:

#19596 (Deduplicate parent txid loop of requested transactions and missing parents of orphan transactions by sdaftuar)

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 referenced this in commit 17de75b028 on Jul 30, 2020

jonatack force-pushed on Jul 30, 2020

jonatack commented at 3:59 pm on July 30, 2020: member

Rebased on merged #19590.

sidhujag referenced this in commit fc7030c8fe on Jul 31, 2020

in src/net_processing.cpp:1440 in dc5fcf2f29 outdated

1463-                if (g_recent_confirmed_transactions->contains(inv.hash)) return true;
1464+            LOCK(g_cs_orphans);
1465+            if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) {
1466+                return true;
1467+            } else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) {
1468+                return true;

vasild commented at 11:09 am on July 31, 2020:

nit: this was the code before, but since you are re-indenting/modifying it:

0if (A) {
1    C;
2} else if (B) {
3    C;
4}

is the same as

0if (A || B) {
1   C;
2}

jonatack commented at 2:33 pm on August 3, 2020:

Yes, I deliberately did not touch it to keep the concepts separate.

in src/protocol.h:419 in 8bd1256cdc outdated

430@@ -431,12 +431,20 @@ class CInv
431     std::string ToString() const;
432 
433     // Single-message helper methods
434-    bool IsMsgTx()        const { return type == MSG_TX; }
435-    bool IsMsgWtx()       const { return type == MSG_WTX; }
436+    bool IsUndefined() const { return type == UNDEFINED; }
437+    bool IsMsgTx() const { return type == MSG_TX; }
438+    bool IsMsgBlk() const { return type == MSG_BLOCK; }

vasild commented at 11:17 am on July 31, 2020:

nit: maybe use “Block” to be in line with the surrounding code, e.g. LookupBlockIndex(), MSG_BLOCK which uses the full word. “Blk” looks like “bulk” to me rather than “block”.

jonatack commented at 2:32 pm on August 3, 2020:

Yes, I thought about this, but that would make the methods names quite a bit longer, and ISTM “blk” is an established abbreviation for “block”, e.g. the filenames in ~/.bitcoin/blocks, and in the codebase git grep -i blk. Let’s see what other reviewers say before changing (that change is actually in #19610, which needs to go in before this PR).

in src/net_processing.cpp:2805 in 8a4cbef2f1 outdated

2804-            inv.type = State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK;
2805-            inv.hash = req.blockhash;
2806-            pfrom.vRecvGetData.push_back(inv);
2807+
2808+            uint32_t inv_type{State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK};
2809+            pfrom.vRecvGetData.emplace_back(CInv(inv_type, req.blockhash));

vasild commented at 11:38 am on July 31, 2020:

This call to emplace_back() will create a CInv temporary, copy-construct the object in the vector from the temporary and then destroy the temporary. Whereas emplace_back(inv_type, req.blockhash) would directly construct the object in the vector with the supplied arguments.

jonatack commented at 2:34 pm on August 3, 2020:

Good point, thank you! Done.

in src/protocol.h:404 in 8a4cbef2f1 outdated

418@@ -419,6 +419,8 @@ enum GetDataMsg : uint32_t {
419 /** inv message data */
420 class CInv
421 {
422+private:
423+    uint32_t type;

vasild commented at 11:38 am on July 31, 2020:

nit: maybe also rename it to m_type

jonatack commented at 2:34 pm on August 3, 2020:

It is indeed the preferred style for new code, but here it seems like a noisy change for little gain that would make the methods in the class longer and less readable. Also, the companion class member variable hash would also need to be changed. Holding off for now unless other reviewers think they are both worth changing.

in src/protocol.h:452 in 8a4cbef2f1 outdated

447@@ -446,8 +448,8 @@ class CInv
448     bool IsMsgBlkOrMsgWitnessBlk() const { return type == MSG_BLOCK || type == MSG_WITNESS_BLOCK; }
449     bool IsGenBlkMsg() const { return type == MSG_BLOCK || type == MSG_FILTERED_BLOCK || type == MSG_CMPCT_BLOCK || type == MSG_WITNESS_BLOCK; }
450 
451-    uint32_t type;
452     uint256 hash;
453+    void SetTypeBitwiseOr(uint32_t flags) { type |= flags; }

vasild commented at 11:39 am on July 31, 2020:

Does it make sense to do some sanity checking of the supplied flags here?

jonatack commented at 2:34 pm on August 3, 2020:

I’m looking at removing this completely (per #19596 (review)).

jonatack commented at 2:58 pm on August 4, 2020:

Removed unused nFetchFlags from TX and INV net processing in ProcessMessages().

vasild changes_requested

jonatack commented at 2:37 pm on August 3, 2020: member

Thanks for reviewing @vasild! Appreciated. This PR is still being updated pending merge of #19610, so don’t hesitate to ACK there.

jonatack force-pushed on Aug 4, 2020

p2p, refactoring: add CInv block message helper methods f6b83acf70

jonatack force-pushed on Aug 4, 2020

p2p, refactoring: use CInv block helpers in net_processing.cpp e21c3cef50

p2p, refactoring: change CInv::type from int to uint32_t 953f6990a5

p2p, refactoring: make CInv::type private 0b5842c5d3

p2p: remove unused nFetchFlags from NetMsgType::TX processing 3cc943b87f

p2p: remove unused nFetchFlags from NetMsgType::INV processing 83de92e8d8

jonatack force-pushed on Aug 4, 2020

jonatack commented at 1:53 pm on August 8, 2020: member

These changes are now all in #19610, so this can be closed for now.

jonatack closed this on Aug 8, 2020

jonatack renamed this:
~~p2p: refactor CInv::type from public int to private uint32_t~~
p2p: change `CInv::type` from `int` to `uint32_t` to fix UBSan warning
on Aug 27, 2020

jonatack renamed this:
~~p2p: change `CInv::type` from `int` to `uint32_t` to fix UBSan warning~~
p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning
on Aug 27, 2020

jonatack commented at 9:08 am on August 27, 2020: member

Tried to re-open this PR with an updated version, but apparently after a rebase/force-push on a closed PR, GitHub requires a new PR to be opened.

jonatack commented at 9:11 am on August 27, 2020: member

Done in #19818.

DrahtBot locked this on Feb 15, 2022

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19611

Conflicts

p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning #19611