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

pull marcofleon wants to merge 1 commits into bitcoin:master from marcofleon:2024/09/relaytx-type-safety changing 3 files +6 −6
  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

    For detailed information about the code coverage, see the test coverage report.

    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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me