Making CTransaction a Regular Type #35569

purpleKarrot commented at 3:52 PM on June 19, 2026: contributor

CTransaction currently has public data members that are marked const. Details why this is problematic, what the alternatives are, and how to fix it are given here or here.

The code is refactored using the following steps:

Public observer functions are added to CTransaction for each data member.
A bitcoin-tidy check is added to rewrite direct data member access to use an observer function.
A "scripted-diff" that is the result of running the bitcoin-tidy check with -fix.
Making CTransaction's data members private.

The result is that CTransaction has regular semantics for copy, move, and assignment. The refactoring is a prerequisite for further refactoring that will untangle the type from serialization logic (read about those plans here or here) which will result in better reasoning and faster compilation.

DrahtBot commented at 3:52 PM on June 19, 2026: 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/35569.

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:

#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35501 (wallet: store all witness variants of a transaction by achow101)
#35295 (validation: fetch block input prevouts in parallel during ConnectBlock by andrewtoth)
#34875 (refactor: separate deferred script check collection from CheckInputScripts by l0rinc)
#32958 (wallet/refactor: Update SignPSBTInput to return util::Expected<void, PSBTError> and remove PSBTError:Ok by kevkevinpal)
#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32575 (consensus: Remove special treatment for single threaded script checking by fjahr)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
#31252 (rpc: print P2WSH and P2SH redem Script in getrawtransaction and getblock by polespinasa)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#27865 (wallet: Track no-longer-spendable TXOs separately 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

Coin(ptx->GetOutputs()[outpoint.n], MEMPOOL_HEIGHT, false) in src/txmempool.cpp
Coin(tx->GetOutputs()[n], MEMPOOL_HEIGHT, false) in src/txmempool.cpp
coinbase_spend.vin.emplace_back(COutPoint(pblock->vtx[0]->GetHash(), 0), CScript(), 0) in src/test/validation_block_tests.cpp

2026-06-20 03:43:27

fanquake commented at 4:51 PM on June 19, 2026: member

More details at

Can you put all the parts relevant to reviewers into the PR description?

DrahtBot added the label CI failed on Jun 19, 2026

DrahtBot commented at 5:07 PM on June 19, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/27835663586/job/82382956280 LLM reason (✨ experimental): CI failed because the lint scripted-diff check errored out with cmake: not found (and run-clang-tidy: not found), indicating missing tools in the lint container.

<details><summary>Hints</summary>

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.

</details>

CTransaction: Add observer member functions

Make sure that for each data member of `CTransaction`, there exists
a public member function that is marked `const` and returns the data
member either by value or by reference to const.
Make sure to add the same member functions to `CMutableTransaction`
for symmetry, so that they can be used in generic code.

2f8186ff9e

bitcoin-tidy: Add tx-observers check

Add a check to bitcoin-tidy that detects when a data member of a
particular class is accessed outside of a member function and
rewrites that access with an observer function.
Parameterize the check for `CTransaction` with a mapping of its
data members to observer functions added previously.

6bd95c0c70

scripted-diff: Use CTransaction observer functions

Perform an automated replacement of all direct accesses of
`CTransaction`'s data members, each with it's associated
observer function.

-BEGIN VERIFY SCRIPT-
export CC="$(which clang)"
export CXX="$(which clang)"
cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_GUI=ON -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON
cmake -B build/tidy -S contrib/devtools/bitcoin-tidy
cmake --build build
cmake --build build/tidy
run-clang-tidy -p build -load='build/tidy/libbitcoin-tidy.so' -checks='-*,bitcoin-tx-observers' -fix
-END VERIFY SCRIPT-

c6bddf18dc

CTransaction: Make data members private

Now that `CTransaction`'s data members are no longer accessed
outside of member functions, they can be made non-`const` and
`private`.

853db19049

purpleKarrot force-pushed on Jun 20, 2026

purpleKarrot marked this as ready for review on Jun 20, 2026

purpleKarrot commented at 5:12 AM on June 20, 2026: contributor

export CXX="$(which clang)"

should be clang++. But before I push that change and trigger another rebuild, I will wait for more comments.

The linter fails validating the scripted_diff due to cmake and run-clang-tidy not being found. How can this be fixed?

ajtowns commented at 4:07 PM on June 20, 2026: contributor

The result is that CTransaction has regular semantics for copy, move, and assignment.

This doesn't seem like a good idea at all to me. If you want an object that can represent different transactions over its lifetime, you should be using CMutableTransaction, and afaics we don't want that for things like the mempool, or for anything we reference via a CTransactionRef.

Note that the comment ("CTransaction is not actually immutable; deserialization and assignment are implemented, and bypass the constness.") was made incorrect by #8580 in 2016.