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

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:

if (A) {
    C;
} else if (B) {
    C;
}

is the same as

if (A || B) {
   C;
}

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

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

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

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.

 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.

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