1. 
   sr-gi commented at 3:12 pm on March 20, 2025: member

   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

2. 
   DrahtBot commented at 3:12 pm on March 20, 2025: contributor

   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:

   • #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.

3. DrahtBot added the label Refactoring on Mar 20, 2025
4. in src/net_processing.cpp:4241 in 1b6c586adf outdated

   4240                 } else {
   4241                     LogPrintf("Force relaying tx %s (wtxid=%s) from peer=%d\n",
   4242-                              tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId());
   4243-                    RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
   4244+                        txid.ToString(), wtxid.ToString(), pfrom.GetId());
   4245+                    RelayTransaction(txid, wtxid);
   

   ---

   ---

   

   maflcko commented at 3:25 pm on March 20, 2025:

   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 commented at 3:50 pm on March 20, 2025:

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

   ---

   

   maflcko commented at 4:16 pm on March 20, 2025:

   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 commented at 6:19 pm on March 20, 2025:

   I’ve updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
5. 
   Jassor909 commented at 5:54 pm on March 20, 2025: none

   @DrahtBot
6. sr-gi force-pushed on Mar 20, 2025
7. sr-gi force-pushed on Mar 20, 2025
8. sr-gi renamed this:
   refactor: Simplifies ProcessMessage for NetMsgType::TX
   refactor: Enforces Txid and Wtxid types in RelayTransactio
   on Mar 20, 2025
9. sr-gi renamed this:
   refactor: Enforces Txid and Wtxid types in RelayTransactio
   refactor: Enforces Txid and Wtxid types in RelayTransaction
   on Mar 20, 2025
10. refactor: Enforces Txid and Wtxid types in RelayTransaction

    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

    1b586965f7
11. sr-gi force-pushed on Mar 20, 2025
12. 
    Jassor909 commented at 6:35 pm on March 20, 2025: none

13. 
    fanquake commented at 2:22 am on March 21, 2025: member

    cc @marcofleon & #31001.
14. 
    sr-gi commented at 2:29 am on March 21, 2025: member

    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
15. 
    marcofleon commented at 6:49 pm on March 24, 2025: contributor

    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.

Contributors
sr-gi DrahtBot maflcko Jassor909 fanquake marcofleon

Labels
Refactoring