Txid type-safety refactors

[dergoegge]

We introduced type-safe transaction identifiers in bitcoin#28107 to avoid type confusion bugs between txids and wtxids. We are in the process of converting more of our code to use these new types. In the following sections, I am laying out some thoughts on areas that need changing:

blockencodings.{h, cpp}

In blockencodings.h you will find the definition of the class CBlockHeaderAndShortTxIDs, which represents the BIP 152 “cmpctblock” p2p message. Its method GetShortID converts a wtxid to a BIP 152 short id. At the moment, this method takes a const uint256& as a parameter which is not type-safe (e.g. one might incorrectly pass a txid and the compiler would not complain).

Converting the method signature from uint64_t GetShortID(const uint256& txhash) const to uint64_t GetShortID(const Wtxid& wtxid) const would be one of the steps required to make the blockencodings interface type-safe w.r.t. to transaction identifiers.

The second method that needs changing is PartiallyDownloadedBlock::InitData. One of its parameters is a const std::vector<std::pair<uint256, CTransactionRef>>&, representing a list of optimistically kept transactions that might be useful for block reconstruction. It is a vector of pairs containing a wtxid and a shared pointer to the corresponding transaction.

One option to make this type-safe, is to change the signature to be const std::vector<std::pair<Wtxid, CTransactionRef>>& (notice the Wtxid type instead of uint256). A second option might arise from considering the following question: why are we passing pairs of (wtxid, tx) when the transaction itself is sufficient to retrieve the wtxid (i.e tx->GetWitnessHash())? We might be able to just pass a vector of transactions instead of the pairs. Regardless of which option is chosen, changes to the type of PeerManagerImpl::vExtraTxnForCompact will have to be made.

GenTxid

GenTxid (a generic txid type) existed before the new txid types. It can hold either a txid or wtxid but it does not enforce types at compile time. It will likely be a bigger refactor but this type should probably become a std::variant<Txid, Wtxid>. To start, one could replace the current definition of the class with using GenTxid = std::variant<Txid, Wtxid> and then compile to observe the errors (this should reveal all locations that need changing).