refactor: Txid type safety (parent PR)

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.

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:

#32238 (qt, wallet: Convert uint256 to Txid by marcofleon)
#32128 (Draft: CCoinMap Experiments by martinus)
#32043 ([IBD] Tracking PR for speeding up Initial Block Download by l0rinc)
#31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)
#31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
#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)
#29680 (wallet: fix unrelated parent conflict doesn’t cause child tx to be marked as conflict by Eunovo)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

DrahtBot added the label Refactoring on Apr 1, 2025

marcofleon marked this as a draft on Apr 1, 2025

DrahtBot added the label CI failed on Apr 1, 2025

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.

marcofleon force-pushed on Apr 2, 2025

in src/qt/transactiontablemodel.cpp:279 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.

DrahtBot removed the label CI failed on Apr 2, 2025

hebasto referenced this in commit e1dfa4faeb on Apr 10, 2025

DrahtBot added the label Needs rebase on Apr 10, 2025

qt, refactor: Convert uint256 to Txid in the GUI

Switch all instances of a transaction from a uint256 to the Txid type.
The added implicit conversion in transaction_identifier.h is only
temporary to allow for commits to be split up neatly, versus doing the
type conversion for the entire wallet, GUI, and interfaces in a single
commit.

cf6f102cd3

wallet, refactor: Convert uint256 to Txid in wallet interfaces ff88abdaa7

wallet, refactor: Convert uint256 to Txid in wallet

Switch all instances of transactions from uint256 to Txid in the
wallet and relevant tests. Also remove the added implicit conversion
line in transaction_identifier.h

641165fdd1

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.

c5da76b54b

Convert txmempool `exists` to GenTxidVariant a3b6eda613

Convert `info` and `info_for_relay` to GenTxidVariant e798bab289

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.

5b9e7ad932

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`.

221e60144b

Convert `txrequest` to GenTxidVariant

Switch all instances of GenTxid to GenTxid variant in
`txrequest` and complete `txdownloadman_impl` by
converting `GetRequestsToSend`.

ab739dd4fa

Convert the rest of `net_processing` to GenTxidVariant 90a1a6f405

Remove old GenTxid class 4a5c30710d

scripted-diff: Replace GenTxidVariant with GenTxid

-BEGIN VERIFY SCRIPT-
sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant')
-END VERIFY SCRIPT-

42e78f8f8a

Convert RPCs from uint256 to Txid 89808184a2

Convert mini_miner from uint256 to Txid 8b6f9e3365

mempool, refactor: Convert uint256 to Txid e78e3764f1

Convert all policy from uint256 to Txid 339685e1ab

Convert remaining instances of uint256 to Txid

These remaining miscellaneous changes were discovered by commenting
out the implicit conversion in `transaction_identifier`.

13f95efab5

Remove implicit conversion functions in `transaction_identifier` acbd6faa78

marcofleon force-pushed on Apr 10, 2025

DrahtBot removed the label Needs rebase on Apr 10, 2025

DrahtBot added the label CI failed on Apr 29, 2025

DrahtBot removed the label CI failed on May 2, 2025

DrahtBot added the label Needs rebase on May 7, 2025

DrahtBot commented at 2:21 pm on May 7, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

refactor: Txid type safety (parent PR) #32189

Code Coverage & Benchmarks

Reviews

Conflicts