refactor: Enforces Txid and Wtxid types in RelayTransaction

[sr-gi]

RelayTransaction expects to pass a TxId and Wtxid as arguments, but any uint256 type is really accepted. When calling RelayTransaction, most times we are already passing tx->GetHash() and tx->GetWitnessHash() to it, and their value are downcasted.

Updates RelayTransaction and make sure we are consistent with its arguments

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32104.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32189 (refactor: Txid type safety (parent PR) by marcofleon)
• #30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
• #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
• #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
• #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

[maflcko]

not sure. This seems like a pessimization that makes it harder to use the type-safe Txid and Wtxid in RelayTransaction

It would be better to make this code type-safe, to avoid having to touch it again in the future.

[sr-gi]

If that is the case, shouldn't we be enforcing RelayTransaction to take Txid and Wtxid instead of uint256?

[maflcko]

Yes. Alternatively, at a minimum the txid and wtxid types should be adjusted here. Otherwise, this refactor may cause a silent merge conflict with the type-safety refactor.

[sr-gi]

I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method

[Jassor909]

@DrahtBot

[Jassor909]

[fanquake]

cc @marcofleon & #31001.

[sr-gi]

Oh, I didn't realize Marco was already working on this. I saw this when updating the Erlay PR and thought it may be good to standalone fix. I'm happy to close it if you're planning to get it addressed @marcofleon

[marcofleon]

Lgtm as a standalone change. It's the same change I implement in fa8400494e3c411a55375e71abaaa2637a22e216 as part of the complete refactor. I actually missed all the tx.GetHash() and tx.GetWitnessHash() in ProcessMessage so good catch there.

My final refactor does end up altering RelayTransaction in a more involved way, but that includes changes (switching GenTxid to a variant) that haven't been discussed yet. I'll open a draft PR of that branch this week.

[sr-gi]

Closing in favor of #32189