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:

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

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

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

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

fanquake referenced this in commit 51be79c42b on May 16, 2025

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

Convert txmempool `exists` to GenTxidVariant 1015e13fd8

Convert `info` and `info_for_relay` to GenTxidVariant b0ec066fdd

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

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

Convert `txrequest` to GenTxidVariant

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

6c9753ed66

Convert the rest of `net_processing` to GenTxidVariant be6dea45d2

Remove old GenTxid class 5bcfcafd24

scripted-diff: Replace GenTxidVariant with GenTxid

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

8f7787d17e

Convert RPCs from uint256 to Txid 4b63929299

Convert mini_miner from uint256 to Txid 9cf699d192

mempool, refactor: Convert uint256 to Txid fa30e200f6

Convert all policy from uint256 to Txid 7ee628ba3a

Convert remaining instances of uint256 to Txid

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

f7f731cc32

Remove implicit conversion functions in `transaction_identifier` edff83a95f

marcofleon force-pushed on May 22, 2025

DrahtBot removed the label Needs rebase on May 22, 2025

DrahtBot added the label CI failed on May 22, 2025

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.

refactor: Txid type safety (parent PR) #32189

Code Coverage & Benchmarks

Reviews

Conflicts