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

Convert all of `txdownloadman_impl` to the new variant except for `GetRequestsToSend`, which will be easier to switch at the same time as `txrequest`.

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

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

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

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 11 commits May 22, 2025 15:01

Convert txdownloadman_impl to GenTxidVariant

d7d7d7e

Convert all of `txdownloadman_impl` to the new variant except for `GetRequestsToSend`, which will be easier to switch at the same time as `txrequest`.

Convert txrequest to GenTxidVariant

6c9753e

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

Convert the rest of net_processing to GenTxidVariant

be6dea4

Remove old GenTxid class

5bcfcaf

scripted-diff: Replace GenTxidVariant with GenTxid

8f7787d

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

Convert RPCs from uint256 to Txid

4b63929

Convert mini_miner from uint256 to Txid

9cf699d

mempool, refactor: Convert uint256 to Txid

fa30e20

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

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

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