#35569 Encapsulation for CTransaction

• Decouple transaction internal storage representation from caller code
• Lay the groundwork for future transaction memory optimizations

This pull request encapsulates `CTransaction` data members behind public `const` observer functions (`GetInputs()`, `GetOutputs()`, `GetVersion()`, `GetLockTime()`) and makes the underlying fields private and non-`const`. A new `bitcoin-tidy` check (`use-observers`) is introduced to automate the mechanical migration across 86 files. It also adds mirror accessors to `CMutableTransaction` to allow generic template call sites to compile.

Problem: `CTransaction` exposes its internal member layout publicly, making storage representation part of its public API. This coupling prevents optimizing internal transaction storage, such as using a single allocation arena for blocks or implementing a regular value handle over immutable data.

P3 · new feature

• P3 because it adds an automated tooling check to rewrite member access
• The tool can be reused for future encapsulation refactorings across the codebase

The PR introduces a generalized Clang-Tidy check to contrib/devtools/bitcoin-tidy that uses Clang annotations to automate migrating direct member accesses to accessor methods across the codebase, which can be reused for subsequent refactorings.

Membership: Adds a generalized AST rewrite check (use-observers) in contrib/devtools/bitcoin-tidy.

Factors: security/stability 0, bug 0, performance 0, user value 1, leverage 2

P3 · cleanup

• P3 because it offers no immediate performance gains or bug fixes
• It establishes the foundation for future block memory allocation improvements

The PR encapsulates CTransaction members behind observer methods, removing reliance on direct field layout as the public interface. While it introduces no immediate bug fixes or performance speedups, it establishes an architectural foundation for future optimizations such as block-level arena memory allocation and immutable value types.

Membership: Labeled Validation; changes the core transaction primitive class (CTransaction in src/primitives/transaction.h) used throughout consensus and validation.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2

• Ready to review
• CI is passing and the mechanical migration is verified reproducible

The PR is clean against master, CI is passing, and the automated tidy migration has been verified as reproducible by independent reviewers.

Author status: Active, addressed technical questions in a detailed summary and engaged with reviewer feedback.

Open concerns:

• ajtowns issued a Concept NACK, arguing the PR causes massive churn across consensus-critical code for zero immediate benefit and opposes making member fields non-const.
• l0rinc issued an Approach NACK, arguing the refactoring is premature, adds duplicate getters to CMutableTransaction, and should be broken down into smaller review units retaining protocol naming like vin() and vout().
• mzumsande questioned renaming protocol-level terminology like vin and vout to GetInputs and GetOutputs.

Resolved concerns:

• Assignment operators on CTransaction were explicitly deleted to prevent accidental non-const aliasing while CTransactionRef remains in use.
• Move constructor was removed to avoid leaving partially moved witness state.
• The bitcoin-tidy check was generalized to use Clang attribute annotations rather than hardcoded class names.
• LIFETIMEBOUND annotations were added to accessor methods returning references.

• Concept objection: massive churn in consensus code with no immediate benefit (ajtowns)
• Approach objection: premature churn across call sites that should be split up (l0rinc)
• Supports decoupling caller code from transaction internal layout (josibake, ryanofsky)
• Verified the automated migration is reproducible and tests pass (alexanderwiederin)

Disputed: ajtowns (Concept NACK) and l0rinc (Approach NACK) oppose the churn and direction, while josibake, alexanderwiederin, and ryanofsky support it.

Two experienced contributors have registered blocking objections against the concept and approach due to churn in consensus-critical code and lack of immediate payoffs, while several other contributors have ACKed the encapsulation architecture.

• ajtowns: 'Concept NACK. At this point my impression is that providing the kernel API is primarily acting as a supply-chain attack vector, encouraging multiple significant refactors into consensus critical code for extremely spurious reasons...'
• l0rinc: 'Approach NACK for now. I don't see a concrete benefit here that justifies rewriting so many transaction call sites in consensus, policy, mempool, wallet, tests, fuzzers, and kernel-facing code.'
• josibake: 'ACK 402500baad... Adding observer functions for data members moves towards an overall better design where the caller does not depend on the class layout.'
• alexanderwiederin: 'ACK 402500baad... Verified fixup reproduces... Unit and Functional tests green'
• ryanofsky: 'Concept ACK. I don't see practical downsides worth blocking over, and the techniques and direction seem valuable.'

Review verdicts (DrahtBot): 2 -2

• ACK: josibake, alexanderwiederin
• Concept ACK: w0xlt, janb84, willcl-ark, ryanofsky
• Approach NACK: l0rinc
• Concept NACK: ajtowns

Enables:

• #35904
• #36015

330 lines under test/bench/ci.

• src/validation.cpp +38/-38
• src/script/interpreter.cpp +34/-34
• src/primitives/transaction.h +39/-28
• src/wallet/wallet.cpp +33/-33
• contrib/devtools/bitcoin-tidy/use-observers.cpp +55/-0
• src/wallet/spend.cpp +19/-19
• src/policy/policy.cpp +18/-18
• src/rpc/blockchain.cpp +18/-18
• src/txmempool.cpp +18/-18
• src/consensus/tx_verify.cpp +17/-17
• src/test/script_tests.cpp +17/-17
• src/wallet/receive.cpp +15/-15
• src/policy/truc_policy.cpp +12/-12
• src/test/fuzz/package_eval.cpp +12/-12
• src/test/fuzz/txorphan.cpp +12/-12
• src/core_io.cpp +11/-11
• src/kernel/bitcoinkernel.cpp +11/-11
• src/test/transaction_tests.cpp +11/-11
• src/test/coinsviewoverlay_tests.cpp +9/-9
• src/wallet/feebumper.cpp +8/-8
• src/wallet/interfaces.cpp +8/-8
• src/node/txorphanage.cpp +7/-7
• src/qt/transactiondesc.cpp +7/-7
• contrib/devtools/bitcoin-tidy/use-observers.h +13/-0
• src/consensus/tx_check.cpp +6/-6
• src/index/coinstatsindex.cpp +6/-6
• src/policy/packages.cpp +6/-6
• src/psbt.cpp +6/-6
• src/test/fuzz/utxo_total_supply.cpp +6/-6
• src/coins.cpp +5/-5
• src/signet.cpp +5/-5
• src/wallet/rpc/spend.cpp +5/-5
• src/attributes.h +9/-0
• contrib/devtools/bitcoin-tidy/CMakeLists.txt +7/-1
• src/policy/ephemeral_policy.cpp +4/-4
• src/test/fuzz/script_flags.cpp +4/-4
• src/test/util/txmempool.cpp +4/-4
• src/common/bloom.cpp +3/-3
• src/core_memusage.h +3/-3
• src/node/miner.cpp +3/-3
• src/test/coins_tests.cpp +3/-3
• src/test/fuzz/coins_view.cpp +3/-3
• src/test/fuzz/tx_pool.cpp +3/-3
• src/test/miniminer_tests.cpp +3/-3
• src/test/util/transaction_utils.cpp +3/-3
• src/wallet/test/wallet_tests.cpp +3/-3
• src/bench/mempool_stress.cpp +2/-2
• src/consensus/validation.h +2/-2
• src/index/txospenderindex.cpp +2/-2
• src/node/mini_miner.cpp +2/-2
• src/node/txdownloadman_impl.cpp +2/-2
• src/policy/policy.h +2/-2
• src/psbt.h +2/-2
• src/qt/transactionrecord.cpp +2/-2
• src/rpc/mining.cpp +2/-2
• src/rpc/rawtransaction.cpp +2/-2
• src/test/fuzz/cmpctblock.cpp +2/-2
• src/test/fuzz/mini_miner.cpp +2/-2
• src/test/fuzz/utxo_snapshot.cpp +2/-2
• src/test/script_assets_tests.cpp +2/-2
• src/test/txvalidation_tests.cpp +2/-2
• src/test/validation_block_tests.cpp +2/-2
• src/wallet/rpc/coins.cpp +2/-2
• src/wallet/rpc/transactions.cpp +2/-2
• src/wallet/scriptpubkeyman.cpp +2/-2
• src/wallet/test/spend_tests.cpp +2/-2
• contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp +2/-0
• src/bench/blockencodings.cpp +1/-1
• src/bench/coin_selection.cpp +1/-1
• src/bench/mempool_ephemeral_spends.cpp +1/-1
• src/bench/txorphanage.cpp +1/-1
• src/blockfilter.cpp +1/-1
• src/node/psbt.cpp +1/-1
• src/node/transaction.cpp +1/-1
• src/policy/truc_policy.h +1/-1
• src/primitives/transaction.cpp +1/-1
• src/qt/walletmodeltransaction.cpp +1/-1
• src/rpc/mempool.cpp +1/-1
• src/test/fuzz/script_interpreter.cpp +1/-1
• src/test/fuzz/script_sign.cpp +1/-1
• src/test/fuzz/txdownloadman.cpp +1/-1
• src/test/fuzz/util.cpp +1/-1
• src/test/miner_tests.cpp +1/-1
• src/test/sighash_tests.cpp +1/-1
• src/test/sigopcount_tests.cpp +1/-1
• src/test/txospenderindex_tests.cpp +1/-1
• src/test/txvalidationcache_tests.cpp +1/-1
• src/test/util/setup_common.cpp +1/-1
• src/test/validation_tests.cpp +1/-1
• src/util/rbf.cpp +1/-1
• src/wallet/test/coinselector_tests.cpp +1/-1
• src/wallet/test/group_outputs_tests.cpp +1/-1
• src/wallet/transaction.h +1/-1

This pull request encapsulates CTransaction data members behind public const observer methods and makes the underlying member variables private, migrating all call sites across the codebase via an automated bitcoin-tidy check. It addresses the architectural coupling where the in-memory layout of transactions acts as the public API, which prevents alternative internal representations such as arena allocation or value-handle semantics. Opponents including ajtowns and l0rinc argue the change causes widespread code churn in consensus-critical code with zero immediate benefit, while supporters including josibake, ryanofsky, and alexanderwiederin see it as a necessary foundation for future immutability and memory optimizations. The PR has working CI and reproducible tidy transformations, but remains disputed on concept and approach.