refactor: Txid type safety (parent PR) by marcofleon · Pull Request #32189 · bitcoin/bitcoin

marcofleon · 2025-04-01T18:47:14Z

🚨🚨 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 · 2025-04-01T18:47:17Z

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:

#32941 (p2p: TxOrphanage revamp cleanups by glozow)
#32844 (RPC/txoutproof: Support including (and verifying) proofs of wtxid by luke-jr)
#32730 (p2p: avoid traversing blocks (twice) during IBD by furszy)
#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 by glozow)
#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)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)

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 · 2025-04-01T21:46:36Z

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

Hints

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.

868816d refactor: Remove SetHexDeprecated (marcofleon) 6b63218 qt: Update SetHexDeprecated to FromHex (marcofleon) Pull request description: This is part of #32189. I'm separating this out because it's not immediately obvious that it's just a refactor. `SetHexDeprecated()` doesn't do any correctness checks on the input, while `FromHex()` does, so it's theoretically possible that there's a behavior change. Replaces `uint256::SetHexDeprecated()` calls with `Txid::FromHex()` in four locations: - `TransactionTableModel::updateTransaction` - `TransactionView::contextualMenu` - `TransactionView::abandonTx` - `TransactionView::bumpFee` The input strings in these cases aren't user input, so they should only be valid hex strings from `GetHex()` (through `TransactionRecord::getTxHash()`). These conversions should be safe without additional checks. ACKs for top commit: laanwj: Code review ACK 868816d w0xlt: Code review ACK 868816d BrandonOdiwuor: Code Review ACK 868816d TheCharlatan: ACK 868816d hebasto: ACK 868816d, I have reviewed the code and it looks OK. Tree-SHA512: 121f149dcc7358231d0327cb3212ec96486a88410174d3c74ab8cbd61bad35185bc0a9740d534492b714811f72a6736bc7ac6eeae590c0ea1365c61cc791da37

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

DrahtBot · 2025-05-22T16:43:13Z

🚧 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.}

Hints

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.

a60f863 scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon) c8ba199 Remove old GenTxid class (marcofleon) 072a198 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon) 1b52839 Convert `txrequest` to GenTxidVariant (marcofleon) bde4579 Convert `txdownloadman_impl` to GenTxidVariant (marcofleon) c876a89 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon) de858ce move-only: make GetInfo a private CTxMemPool member (stickies-v) eee473d Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon) 243553d refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v) fcf92fd refactor: make CTxMemPool::GetIter strongly typed (marcofleon) 11d28f2 Implement GenTxid as a variant (marcofleon) Pull request description: Part of the [type safety refactor](#32189). This PR changes the GenTxid class 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 a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256. ACKs for top commit: w0xlt: ACK a60f863 dergoegge: Code review ACK a60f863 maflcko: review ACK a60f863 🎽 theStack: Code-review ACK a60f863 Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047

DrahtBot · 2025-07-11T19:16:55Z

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

marcofleon · 2025-08-01T13:59:06Z

Closing this, as the final complete refactor can be seen in #32238, #32631, #33005, and #33116. Glad the warning here was heeded 🚨

de0675f refactor: Move `transaction_identifier.h` to primitives (marcofleon) 6f068f6 Remove implicit uint256 conversion and comparison (marcofleon) 9c24cda refactor: Convert remaining instances from uint256 to Txid (marcofleon) d2ecd68 policy, refactor: Convert uint256 to Txid (marcofleon) f6c0d1d mempool, refactor: Convert uint256 to Txid (marcofleon) aeb0f78 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon) 326f244 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon) 49b3d3a Clean up `FindTxForGetData` (marcofleon) Pull request description: This is the final leg of the [type safety refactor](#32189). All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR. ACKs for top commit: stickies-v: re-ACK de0675f, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits. janb84: re ACK de0675f dergoegge: re-ACK de0675f theStack: Code-review ACK de0675f Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08

bad8527 refactor: Remove SetHexDeprecated (marcofleon) 5907a49 qt: Update SetHexDeprecated to FromHex (marcofleon) Pull request description: This is part of bitcoin/bitcoin#32189. I'm separating this out because it's not immediately obvious that it's just a refactor. `SetHexDeprecated()` doesn't do any correctness checks on the input, while `FromHex()` does, so it's theoretically possible that there's a behavior change. Replaces `uint256::SetHexDeprecated()` calls with `Txid::FromHex()` in four locations: - `TransactionTableModel::updateTransaction` - `TransactionView::contextualMenu` - `TransactionView::abandonTx` - `TransactionView::bumpFee` The input strings in these cases aren't user input, so they should only be valid hex strings from `GetHex()` (through `TransactionRecord::getTxHash()`). These conversions should be safe without additional checks. ACKs for top commit: laanwj: Code review ACK bad8527 w0xlt: Code review ACK bitcoin/bitcoin@bad8527 BrandonOdiwuor: Code Review ACK bad8527 TheCharlatan: ACK bad8527 hebasto: ACK bad8527, I have reviewed the code and it looks OK. Tree-SHA512: 121f149dcc7358231d0327cb3212ec96486a88410174d3c74ab8cbd61bad35185bc0a9740d534492b714811f72a6736bc7ac6eeae590c0ea1365c61cc791da37

3022b83 wallet, refactor: Convert uint256 to Txid in wallet (marcofleon) 6083d3d wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon) edd375d qt, refactor: Convert uint256 to Txid in the GUI (marcofleon) Pull request description: This is part of bitcoin/bitcoin#32189. Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces. ACKs for top commit: stickies-v: re-ACK 3022b83, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase. achow101: ACK 3022b83 furszy: Code review ACK 3022b83 Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60

7abd6dc scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon) 9490a26 Remove old GenTxid class (marcofleon) fceba5b Convert remaining instances of GenTxid to GenTxidVariant (marcofleon) f5a831a Convert `txrequest` to GenTxidVariant (marcofleon) 7b87bfd Convert `txdownloadman_impl` to GenTxidVariant (marcofleon) 6b3e9e1 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon) 2c04761 move-only: make GetInfo a private CTxMemPool member (stickies-v) 9af2db3 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon) d46edd4 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v) c056bae refactor: make CTxMemPool::GetIter strongly typed (marcofleon) dcd61ff Implement GenTxid as a variant (marcofleon) Pull request description: Part of the [type safety refactor](bitcoin/bitcoin#32189). This PR changes the GenTxid class 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 a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256. ACKs for top commit: w0xlt: ACK bitcoin/bitcoin@7abd6dc dergoegge: Code review ACK 7abd6dc maflcko: review ACK 7abd6dc 🎽 theStack: Code-review ACK 7abd6dc Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047

1a57c96 refactor: Move `transaction_identifier.h` to primitives (marcofleon) e2ba7d0 Remove implicit uint256 conversion and comparison (marcofleon) d4696d2 refactor: Convert remaining instances from uint256 to Txid (marcofleon) 35e3ce1 policy, refactor: Convert uint256 to Txid (marcofleon) fac3cf8 mempool, refactor: Convert uint256 to Txid (marcofleon) 92a422e refactor: Convert `mini_miner` from uint256 to Txid (marcofleon) 5322a45 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon) b1c636e Clean up `FindTxForGetData` (marcofleon) Pull request description: This is the final leg of the [type safety refactor](bitcoin/bitcoin#32189). All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR. ACKs for top commit: stickies-v: re-ACK 1a57c96, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits. janb84: re ACK 1a57c96 dergoegge: re-ACK 1a57c96 theStack: Code-review ACK 1a57c96 Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08

ac4a0bb refactor: Remove SetHexDeprecated (marcofleon) b88377f qt: Update SetHexDeprecated to FromHex (marcofleon) Pull request description: This is part of bitcoin/bitcoin#32189. I'm separating this out because it's not immediately obvious that it's just a refactor. `SetHexDeprecated()` doesn't do any correctness checks on the input, while `FromHex()` does, so it's theoretically possible that there's a behavior change. Replaces `uint256::SetHexDeprecated()` calls with `Txid::FromHex()` in four locations: - `TransactionTableModel::updateTransaction` - `TransactionView::contextualMenu` - `TransactionView::abandonTx` - `TransactionView::bumpFee` The input strings in these cases aren't user input, so they should only be valid hex strings from `GetHex()` (through `TransactionRecord::getTxHash()`). These conversions should be safe without additional checks. ACKs for top commit: laanwj: Code review ACK ac4a0bb w0xlt: Code review ACK bitcoin/bitcoin@ac4a0bb BrandonOdiwuor: Code Review ACK ac4a0bb TheCharlatan: ACK ac4a0bb hebasto: ACK ac4a0bb, I have reviewed the code and it looks OK. Tree-SHA512: 121f149dcc7358231d0327cb3212ec96486a88410174d3c74ab8cbd61bad35185bc0a9740d534492b714811f72a6736bc7ac6eeae590c0ea1365c61cc791da37

3c9209f wallet, refactor: Convert uint256 to Txid in wallet (marcofleon) 97d7a39 wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon) 9eabbf7 qt, refactor: Convert uint256 to Txid in the GUI (marcofleon) Pull request description: This is part of bitcoin/bitcoin#32189. Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces. ACKs for top commit: stickies-v: re-ACK 3c9209f, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase. achow101: ACK 3c9209f furszy: Code review ACK 3c9209f Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60

754ed54 scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon) 4812883 Remove old GenTxid class (marcofleon) 09871fb Convert remaining instances of GenTxid to GenTxidVariant (marcofleon) 554472a Convert `txrequest` to GenTxidVariant (marcofleon) 969d7de Convert `txdownloadman_impl` to GenTxidVariant (marcofleon) 6917b93 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon) c5540f0 move-only: make GetInfo a private CTxMemPool member (stickies-v) 0ff6a83 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon) 4ceb673 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v) b21a409 refactor: make CTxMemPool::GetIter strongly typed (marcofleon) 74dac7e Implement GenTxid as a variant (marcofleon) Pull request description: Part of the [type safety refactor](bitcoin/bitcoin#32189). This PR changes the GenTxid class 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 a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256. ACKs for top commit: w0xlt: ACK bitcoin/bitcoin@754ed54 dergoegge: Code review ACK 754ed54 maflcko: review ACK 754ed54 🎽 theStack: Code-review ACK 754ed54 Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047

f206ab8 refactor: Move `transaction_identifier.h` to primitives (marcofleon) 026cd0b Remove implicit uint256 conversion and comparison (marcofleon) c606a62 refactor: Convert remaining instances from uint256 to Txid (marcofleon) 31084ca policy, refactor: Convert uint256 to Txid (marcofleon) 96c223a mempool, refactor: Convert uint256 to Txid (marcofleon) eef7f50 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon) cb809c2 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon) d30f822 Clean up `FindTxForGetData` (marcofleon) Pull request description: This is the final leg of the [type safety refactor](bitcoin/bitcoin#32189). All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR. ACKs for top commit: stickies-v: re-ACK f206ab8, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits. janb84: re ACK f206ab8 dergoegge: re-ACK f206ab8 theStack: Code-review ACK f206ab8 Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08

DrahtBot added the Refactoring label Apr 1, 2025

marcofleon marked this pull request as draft April 1, 2025 18:47

DrahtBot added the CI failed label Apr 1, 2025

marcofleon force-pushed the 2025/03/full-txid-type-safety branch from 232657d to 68d7aaa Compare April 2, 2025 12:53

This was referenced Apr 2, 2025

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

Implement BIP 370 PSBTv2 #21283

Merged

marcofleon force-pushed the 2025/03/full-txid-type-safety branch from 68d7aaa to 48948fc Compare April 2, 2025 16:33

maflcko reviewed Apr 2, 2025

View reviewed changes

Comment thread src/qt/transactiontablemodel.cpp Outdated

DrahtBot removed the CI failed label Apr 2, 2025

DrahtBot mentioned this pull request Apr 2, 2025

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

This was referenced Apr 8, 2025

qt: Update SetHexDeprecated to FromHex #32237

Merged

qt, wallet: Convert uint256 to Txid #32238

Merged

DrahtBot mentioned this pull request Apr 9, 2025

[IBD] Tracking PR for speeding up Initial Block Download #32043

Closed

13 tasks

DrahtBot added the Needs rebase label Apr 10, 2025

marcofleon force-pushed the 2025/03/full-txid-type-safety branch from 48948fc to acbd6fa Compare April 10, 2025 14:51

DrahtBot removed the Needs rebase label Apr 10, 2025

marcofleon added 3 commits May 22, 2025 15:01

Convert all policy from uint256 to Txid

7ee628b

Convert remaining instances of uint256 to Txid

f7f731c

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

Remove implicit conversion functions in transaction_identifier

edff83a

marcofleon force-pushed the 2025/03/full-txid-type-safety branch from acbd6fa to edff83a Compare May 22, 2025 14:28

DrahtBot added CI failed and removed Needs rebase labels May 22, 2025

This was referenced May 23, 2025

merkle: pre‑reserve leaves to prevent reallocs with odd vtx count #32497

Merged

rpc: generateblock to allow multiple outputs #32468

Draft

p2p: improve TxOrphanage denial of service bounds #31829

Merged

marcofleon mentioned this pull request May 28, 2025

refactor: Convert GenTxid to std::variant #32631

Merged

DrahtBot mentioned this pull request May 30, 2025

ci, iwyu: Treat warnings as errors for specific directories #31308

Merged

DrahtBot mentioned this pull request Jun 17, 2025

p2p: avoid traversing blocks (twice) during IBD #32730

Closed

DrahtBot mentioned this pull request Jul 1, 2025

RPC/txoutproof: Support including (and verifying) proofs of wtxid #32844

Closed

DrahtBot mentioned this pull request Jul 10, 2025

p2p: TxOrphanage revamp cleanups #32941

Merged

DrahtBot added the Needs rebase label Jul 11, 2025

marcofleon mentioned this pull request Aug 1, 2025

refactor: Convert uint256 to Txid #33116

Merged

marcofleon closed this Aug 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Txid type safety (parent PR)#32189

refactor: Txid type safety (parent PR)#32189
marcofleon wants to merge 15 commits into
bitcoin:masterfrom
marcofleon:2025/03/full-txid-type-safety

marcofleon commented Apr 1, 2025

Uh oh!

DrahtBot commented Apr 1, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Apr 1, 2025

Uh oh!

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

DrahtBot commented Jul 11, 2025

Uh oh!

marcofleon commented Aug 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

marcofleon commented Apr 1, 2025

Uh oh!

DrahtBot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Apr 1, 2025

Uh oh!

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

DrahtBot commented Jul 11, 2025

Uh oh!

marcofleon commented Aug 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DrahtBot commented Apr 1, 2025 •

edited

Loading