refactor: ensure type safety for txid and wtxid in RelayTransaction #31001

1. 
   marcofleon commented at 11:24 am on September 30, 2024: contributor

   This PR updates the RelayTransaction function to replace uint256 with the Txid and Wtxid types, improving type safety and helping with the gradual transition to using these types throughout the codebase.
2. refactor: ensure type safety for txid and wtxid in RelayTransaction 31017d57fa
3. 
   DrahtBot commented at 11:24 am on September 30, 2024: 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/31001.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	jonatack

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
   • #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.

4. DrahtBot added the label Refactoring on Sep 30, 2024
5. 
   instagibbs commented at 2:16 pm on September 30, 2024: member

   is this in view of some follow-on work or a one-off? Easier to swallow these useful pills if the code is going to have churn anyways.
6. 
   maflcko commented at 2:32 pm on September 30, 2024: member

   It may be good to add some context to the pull request description. Something like: Currently there are still about $N places with unsafe types and this fixes a good chunk ($nn%) of them. If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
7. 
   marcofleon commented at 2:36 pm on September 30, 2024: contributor

   is this in view of some follow-on work or a one-off?

   I noticed it while doing a bit of review on Erlay (https://github.com/bitcoin/bitcoin/pull/30116). I was planning on continuing to address places in the code where this switch is needed.

   If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?

   Agreed that it may be too small a change to warrant a PR. I was trying to keep the scope within RelayTransaction only. But I can do some more investigation and see if it makes sense to expand a bit. Or at least add context as you said.

8. 
   dergoegge commented at 2:48 pm on September 30, 2024: member

   Easier to swallow these useful pills if the code is going to have churn anyways

   I understand where this is coming from but if we only do these refactors if we’re already touching the code, then I don’t think we’ll ever finish the migration (there is simply too much code to be touched). I’d prefer for us to move to the new types as soon as possible and remove the const uint256& conversion function, so that we end up with actual type-safety.

   I suggested to Marco to work on this, so that the txid type-safety project makes some progress. He has to get started somewhere and I don’t think it’s unreasonable to pick a small self-contained change to start with and then increase the size/scope for the next PRs.

9. 
   instagibbs commented at 3:00 pm on September 30, 2024: member

   @dergoegge that’s an argument for bigger bites then?

   Not against these or more changes.

10. 
    marcofleon commented at 3:04 pm on September 30, 2024: contributor

    I’ll look to expand this one a bit then. I also think the suggestion to add more context is a good one. That way I can refer back to this PR on future ones as a way to track progress.
11. in src/net_processing.cpp:1746 in 31017d57fa

    1742@@ -1743,7 +1743,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
    1743         CTransactionRef tx = m_mempool.get(txid);
    1744 
    1745         if (tx != nullptr) {
    1746-            RelayTransaction(txid, tx->GetWitnessHash());
    1747+            RelayTransaction(tx->GetHash(), tx->GetWitnessHash());
    

    ---

    ---

    

    jonatack commented at 4:49 pm on September 30, 2024:

    tx only needed inside the conditional

    0-        CTransactionRef tx = m_mempool.get(txid);
    1-
    2-        if (tx != nullptr) {
    3+        if (CTransactionRef tx = m_mempool.get(txid)) {
    

    or

    0+        if (CTransactionRef tx = m_mempool.get(txid); tx != nullptr) {
    

12. 
    jonatack commented at 4:51 pm on September 30, 2024: member

    Concept ACK
13. 
    stickies-v commented at 4:43 pm on November 5, 2024: contributor

    I’ll look to expand this one a bit then.

    Are you planning to rework this PR, or is it ready for review as is? If the former, perhaps best to mark this as draft until that’s done?

14. marcofleon marked this as a draft on Nov 5, 2024

Contributors
marcofleon DrahtBot instagibbs maflcko dergoegge jonatack stickies-v

Labels
Refactoring