refactor: Txid type safety (parent PR) #32189

pull marcofleon wants to merge 15 commits into bitcoin:master from marcofleon:2025/03/full-txid-type-safety changing 67 files +485 −437
  1. marcofleon commented at 6:47 pm on April 1, 2025: contributor

    🚨🚨 UNDER NO CIRCUMSTANCES SHOULD THIS BE MERGED!! 🚨🚨

    This is the full Txid type safety refactor. Any transaction that was using uint256 should now be either a Txid, Wtxid, or GenTxid. The implicit conversion in transaction_identifier.h from the transaction types to uint256 is removed and any conversions are now explicit, using FromUint256 or ToUint256(). In general, I try to only convert a transaction to uint256 in contexts where the type information isn’t preserved. Examples would be bloom filters or the txid hasher.

    The GenTxid class changes to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either type now use GenTxid, instead of uint256.

    I’ll be splitting this up into a few smaller PRs, but any feedback on the approach or questions about specifics are welcome here.

  2. DrahtBot commented at 6:47 pm on April 1, 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/32189.

    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:

    • #32631 (refactor: Convert GenTxid to std::variant by marcofleon)
    • #32497 (merkle: pre‑reserve leaves to prevent reallocs with odd vtx count by l0rinc)
    • #32468 (rpc: generateblock to allow multiple outputs by polespinasa)
    • #31829 (p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs by glozow)
    • #31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)
    • #31385 (package validation: relax the package-not-child-with-unconfirmed-parents rule by glozow)
    • #30442 ([IBD] precalculate SipHash constant salt calculations by l0rinc)
    • #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
    • #30079 (Fee Estimation: Ignore all transactions that are CPFP’d by ismaelsadeeq)
    • #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 Apr 1, 2025
  4. marcofleon marked this as a draft on Apr 1, 2025
  5. DrahtBot added the label CI failed on Apr 1, 2025
  6. DrahtBot commented at 9:46 pm on April 1, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39796036093

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. marcofleon force-pushed on Apr 2, 2025
  8. marcofleon force-pushed on Apr 2, 2025
  9. in src/qt/transactiontablemodel.cpp:278 in 017aa1bb2e outdated
    275@@ -276,8 +276,7 @@ void TransactionTableModel::updateAmountColumnTitle()
    276 
    277 void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction)
    278 {
    279-    uint256 updated;
    280-    updated.SetHexDeprecated(hash.toStdString());
    281+    Txid updated = Txid::FromHex(hash.toStdString()).value();
    


    maflcko commented at 6:40 pm on April 2, 2025:
    nit in 017aa1bb2e26c92dac87c3e9c1f87eac0452adff: This is probably fine, but not trivially clear that it is a “refactor” or maybe a behavior change? Could make sense to split this up into its own commit/pull?

    marcofleon commented at 5:06 pm on April 3, 2025:
    Thanks for taking a look. Makes sense to separate it out, agreed. I’ll figure out tomorrow whether it’s easier to just do a separate commit or a whole other PR.
  10. DrahtBot removed the label CI failed on Apr 2, 2025
  11. hebasto referenced this in commit e1dfa4faeb on Apr 10, 2025
  12. DrahtBot added the label Needs rebase on Apr 10, 2025
  13. marcofleon force-pushed on Apr 10, 2025
  14. DrahtBot removed the label Needs rebase on Apr 10, 2025
  15. DrahtBot added the label CI failed on Apr 29, 2025
  16. DrahtBot removed the label CI failed on May 2, 2025
  17. DrahtBot added the label Needs rebase on May 7, 2025
  18. fanquake referenced this in commit 51be79c42b on May 16, 2025
  19. Implement GenTxid as a variant
    Reimplements the GenTxid class as a variant for better type safety.
    Also adds two temporary functions that convert from the old GenTxid
    to the new variant and vice versa. This is only used for intermediate
    commits, to split them into conceptual chunks.
    b91b9ba9e9
  20. Convert txmempool `exists` to GenTxidVariant 1015e13fd8
  21. Convert `info` and `info_for_relay` to GenTxidVariant b0ec066fdd
  22. Convert `CompareInvMempoolOrder` in net_processing to GenTxidVariant
    This completes the conversion to the variant in the txmempool. In
    order to build and pass all tests, `m_tx_inventory_to_send` and
    `RelayTransaction` had to be converted as well.
    fa4dbd20b0
  23. Convert `txdownloadman_impl` to GenTxidVariant
    Convert all of `txdownloadman_impl` to the new variant except for
    `GetRequestsToSend`, which will be easier to switch at the same
    time as `txrequest`.
    d7d7d7e766
  24. Convert `txrequest` to GenTxidVariant
    Switch all instances of GenTxid to GenTxid variant in
    `txrequest` and complete `txdownloadman_impl` by
    converting `GetRequestsToSend`.
    6c9753ed66
  25. Convert the rest of `net_processing` to GenTxidVariant be6dea45d2
  26. Remove old GenTxid class 5bcfcafd24
  27. scripted-diff: Replace GenTxidVariant with GenTxid
    -BEGIN VERIFY SCRIPT-
    sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant')
    -END VERIFY SCRIPT-
    8f7787d17e
  28. Convert RPCs from uint256 to Txid 4b63929299
  29. Convert mini_miner from uint256 to Txid 9cf699d192
  30. mempool, refactor: Convert uint256 to Txid fa30e200f6
  31. Convert all policy from uint256 to Txid 7ee628ba3a
  32. Convert remaining instances of uint256 to Txid
    These remaining miscellaneous changes were discovered by commenting
    out the implicit conversion in `transaction_identifier`.
    f7f731cc32
  33. Remove implicit conversion functions in `transaction_identifier` edff83a95f
  34. marcofleon force-pushed on May 22, 2025
  35. DrahtBot removed the label Needs rebase on May 22, 2025
  36. DrahtBot added the label CI failed on May 22, 2025
  37. DrahtBot commented at 4:43 pm on May 22, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/42717598129 LLM reason (✨ experimental): The CI failure is due to multiple build errors during the compilation process.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.


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: 2025-05-29 18:12 UTC

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