Encapsulation for CTransaction #35569

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

CTransaction currently exposes its data members publicly and marks them const. This makes the implementation details of the type part of its interface, while using const data members to enforce immutability has several undesirable consequences.

This PR encapsulates the data members behind public observer functions. The existing users of CTransaction are migrated to the observers, after which the data members are made private and non-const.

The refactoring is deliberately split into three steps:

Add observer functions corresponding to the existing data members.
Migrate all users from direct data-member access to the observers.
Make the data members private.

The migration is automated with a new bitcoin-tidy use-observers check. This both makes the large mechanical change straightforward and provides a reusable tool for similar refactorings.

The split into separate commits allows reviewers to review handwritten changes independently of the automated changes. It also simplifies rebasing: if the mechanical migration conflicts, individual files can be reset to their new base versions and the migration tool can be run on them again. This works because the old interface remains available until the final commit, so the reset code is still valid while the mechanical migration is reapplied.

This PR does not change the semantics of CTransaction. In particular, regular value semantics are intentionally left for a subsequent change.

Why Encapsulation? Why `CTransaction`?

Public data members make the representation of a type part of its API. Once access goes through observers, the representation can change without requiring its users to change as well.

This establishes a clean boundary between the transaction's data and functionality that operates on it. In particular, it allows serialization/deserialization and other functionality to be separated from the type itself in subsequent refactorings.

This work is part of the broader interface-segregation direction described in #35904, but is not a prerequisite for the stateless validation library proposed there. The encapsulation has value independently of that work.

CTransaction is a good type to start this migration with: it is widely used, its interface is relatively simple, and its data members are already effectively immutable. This allows the migration to establish the pattern and tooling without first requiring changes to the semantics of the type.

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 and AI policy for information on the review process.

Type	Reviewers
Concept NACK	ajtowns
Concept ACK	w0xlt, janb84, willcl-ark, ryanofsky
Approach NACK	l0rinc
Stale ACK	josibake

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36030 (util: move function calls outside standard assertions by l0rinc)
#36015 (txorphanage: account memory usage instead of weight by brunoerg)
#35994 (Primitives: Combine assignments by purpleKarrot)
#35975 (wallet: Fix CWalletTx malleated transaction metadata sync by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35675 (mining: add block template manager by ismaelsadeeq)
#35662 (script: prevent stale sighash caches across transactions by l0rinc)
#35580 (bugfix: compare non-adjusted chunk weight against block weight limit by ismaelsadeeq)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#34864 (coins: tighten cache entry state invariants by l0rinc)
#32958 (wallet/refactor: Update SignPSBTInput to return util::Expected<void, PSBTError> and remove PSBTError:Ok by kevkevinpal)
#32729 (test,script: add sigop helpers (without consensus migration) by l0rinc)
#32575 (consensus: Remove special treatment for single threaded script checking by fjahr)
#31252 (rpc: print P2WSH and P2SH redem Script in getrawtransaction and getblock by polespinasa)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)
#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
emplace_back(COutPoint(...), CScript(), 0) in src/test/validation_block_tests.cpp

2026-08-21 03:51:26

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>

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.

janb84 commented at 11:34 AM on June 21, 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?

I do not think you can do this, as far as I can tell the linter container image does not contain cmake. Drop the scripted diff validation ?

purpleKarrot force-pushed on Jun 21, 2026

purpleKarrot commented at 4:24 PM on June 21, 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.

Just to make sure there is no misunderstanding here. All builtin types in C++ as well as nearly all types in the standard library have regular semantics. You are saying that is a bad design, right?

we don't want [an object that can represent different transactions over its lifetime] for things like the mempool

So instead, the mempool uses CTransactionRef, an object that can not only represent different transactions over its lifetime, but can also change whether it actually holds a transaction or not. I don't get how that is better.

purpleKarrot force-pushed on Jun 21, 2026

DrahtBot removed the label CI failed on Jun 21, 2026

w0xlt commented at 8:29 PM on June 21, 2026: contributor

Concept ACK

ajtowns commented at 6:54 AM on June 22, 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.

Just to make sure there is no misunderstanding here. All builtin types in C++ as well as nearly all types in the standard library have regular semantics. You are saying that is a bad design, right?

No, I said exactly what I intended to say: making CTransaction mutable is a bad idea. We already have CMutableTransaction for cases where mutability is needed.

we don't want [an object that can represent different transactions over its lifetime] for things like the mempool

So instead, the mempool uses CTransactionRef, an object that can not only represent different transactions over its lifetime, but can also change whether it actually holds a transaction or not. I don't get how that is better.

Passing a CTransactionRef around prevents the recipient from modifying it (ignoring const casts), but doesn't prevent the creator from doing so.

    std::shared_ptr<CTransaction> foo;
    vRecv >> TX_WITH_WITNESS(foo);

    ProcessTransaction(foo); // add to mempool

    foo->version = 42; // mempool copy is automatically updated, how neat!

josibake commented at 9:28 AM on June 22, 2026: member

Concept ACK

Moving the data members behind observers, vs having the class layout define the API seems a solid improvement.

The result is that CTransaction has regular semantics for copy, move, and assignment ..making CTransaction mutable is a bad idea

Porque no los dos? Turns out, afaict, there is a C++ idiom for this: handle/body idiom with shared immutable representation! What I mean by this is we want to make the exposed transaction type a regular value handle to immutable transaction data^1. Concretely, something like this:

class CTransaction {
public:
    CTransaction(const CTransaction&) = default;
    CTransaction(CTransaction&&) noexcept = default;
    CTransaction& operator=(const CTransaction&) = default;
    CTransaction& operator=(CTransaction&&) noexcept = default;

    // the observers added in this PR 
   ...

private:
    std::shared_ptr<const TransactionData> m_data;
};

TransactionData would of course contain vin, vout, version, nLockTime, cached hashes, etc., and would be immutable after construction from CMutableTransaction or deserialisation. Now assignment has normal value semantics:

auto a = tx1;
auto b = a;
a = tx2; // b still observes tx1

That is different from mutating a shared transaction object:

auto p = std::shared_ptr<CTransaction>{...};
ProcessTransaction(p);
*p = tx2; // bad: existing users may observe changed transaction contents

While we still use using CTransactionRef = std::shared_ptr<const CTransaction>, perhaps its best to have CTransaction assignment deleted, or the ref type needs to be updated so the shared thing is the immutable TransactionData, not the assignable wrapper.

Based on my understanding of the linked blogs, this seems in line with their stated goals. We can get rid of the "class layout is the API," eventually allowing serialisation/deserialisation to move behind an explicit construction/codec boundary as described in the blogs. It also plays nice with the vocabulary type direction: the eventual public transaction type can be regular and idiomatic, while preserving the invariant of once a transaction contents are published/shared, they are immutable.

purpleKarrot commented at 9:50 AM on June 22, 2026: contributor

@josibake, exactly. As highlighted in my blog post, the next refactoring step would be moving the std::shared_ptr<const> behind the API boundary. This would result in the exact same runtime indirection as currently, it is just a different level of abstraction. However,

While we still use using CTransactionRef = std::shared_ptr<const CTransaction>, perhaps its best to have CTransaction assignment deleted.

for what benefit? It is pointing to a const object. The pointer can be changed to point to a different object (or no object at all) but the shared pointee can not be changed. There can be no spooky action at a distance.

josibake commented at 11:17 AM on June 22, 2026: member

for what benefit? It is pointing to a const object. The pointer can be changed to point to a different object (or no object at all) but the shared pointee can not be changed. There can be no spooky action at a distance.

The benefit is removing a footgun while refactoring. As long as CTransactionRef shares the CTransaction wrapper, making that wrapper assignable leaves a possible non-const-alias footgun. So we either keep assignment deleted during that transition, or move the shared state behind the API boundary first.

Once we reach stage 7 detailed in your bitcoin-tidy blog (which more or less matches my earlier comment, perhaps sans a few details), CTransaction is the value handle and the shared state is behind the API boundary and thus default assignment is fine (and desirable!) because it only rebinds to the handle.

in src/primitives/transaction.h:273 in 18b0efc5ad

janb84 commented at 11:55 AM on June 22, 2026:

    return std::accumulate(tx.GetOutputs().cbegin(), tx.GetOutputs().cend(), CAmount{0}, [](CAmount sum, const auto& txout) { return sum + txout.nValue; });

Intentional skipped ? (It's ok as is)

purpleKarrot commented at 12:18 PM on June 22, 2026:

Interesting catch. The clang-tidy check was not fired on this code and making the data members private did not break it. Apparently, the function template is never instantiated with CTransaction; only with CMutableTransaction. There is also CTransaction::GetValueOut().

josibake commented at 8:35 AM on June 23, 2026:

I don't see anything that would prevent this from being used with CTransaction, though? Seems we could handle this case cleanly by recognising TxType in a template argument as actually a CTx. This would avoid more broadly rewriting vout, vin, version, etc in other places.

josibake commented at 11:46 AM on June 23, 2026:

Per #35569 (comment), I convinced myself this isn't an issue.

janb84 commented at 11:56 AM on June 22, 2026: contributor

Concept ACK

The benefit is removing a footgun while refactoring. As long as CTransactionRef shares the CTransaction wrapper, making that wrapper assignable leaves a possible non-const-alias footgun. So we either keep assignment deleted during that transition, or move the shared state behind the API boundary first.

I second this approach. Also reaching "stage 7" can take a while, if ever reached.

purpleKarrot commented at 12:28 PM on June 22, 2026: contributor

auto p = std::shared_ptr<CTransaction>{...};
ProcessTransaction(p);
*p = tx2; // bad: existing users may observe changed transaction contents

I get it now. CTransactionRef being a shared_ptr<const> can only prevent mutation through that handle, it cannot prove that no non-const alias exists. Yes, as long as shared_ptr<const> is used, it would be better to explicitly disallow both assignment operators. I'll update that.

purpleKarrot force-pushed on Jun 22, 2026

purpleKarrot renamed this:
~~Making CTransaction a Regular Type~~
Encapsulation for CTransaction
on Jun 22, 2026

purpleKarrot force-pushed on Jun 22, 2026

DrahtBot added the label CI failed on Jun 22, 2026

DrahtBot removed the label CI failed on Jun 22, 2026

in src/primitives/transaction.h:291 in 926618a85c

 308 |      /** Convert a CMutableTransaction into a CTransaction. */
 309 |      explicit CTransaction(const CMutableTransaction& tx);
 310 |      explicit CTransaction(CMutableTransaction&& tx);
 311 |  
 312 | +    CTransaction(const CTransaction&) = default;
 313 | +    CTransaction(CTransaction&&) = default;

josibake commented at 2:50 PM on June 22, 2026:

As part of reviewing this, I nerd sniped myself into reading about move semantics and had the thought: are vectors treated differently than fixed length values? Turns out they are! So for uint256 moves are copy like, but for vin/vout, being vectors, move could actually mean move the underlying storage. In light of that, I think we should delete the move constructer during the transition.

Concretely, if vin/vout are no longer const then this would be a memberwise move. Shouldn't matter for the moved to transaction, but the moved from one could be left with m_witness_hash, m_has_witness etc members that still describe the pre-moved state, but the vin/vouts of the moved from could now be empty.

Obviously, this would no longer be a problem once CTransaction represents a value reference to an immutable struct, at which point the move constructer can be added back along with the assignment operation.

I applied this suggestion locally and everything compiled, so doesn't seem to break any existing behaviour.

purpleKarrot commented at 3:01 PM on June 22, 2026:

Good point! But instead of deleting the move constructor, we should simply not provide it.

maflcko commented at 4:06 PM on June 26, 2026:

Just read this comment, and deleting (or better not providing it) is fine, because a move ctor didn't exist before either. Though, in this project it should be fine to provide one, as bugprone-use-after-move prevents unwanted access to the moved-from object.

purpleKarrot force-pushed on Jun 22, 2026

DrahtBot added the label CI failed on Jun 22, 2026

DrahtBot removed the label CI failed on Jun 22, 2026

josibake commented at 8:09 AM on June 23, 2026: member

I find the commit message in a35a2b1 a bit confusing, specifically: "Make sure to add the same member functions to CMutableTransaction for symmetry, so that they can be used in generic code."

It seems clear from the PR description and the code that the observer is not concerned with CMutableTransaction in this PR. Is this a stale commit message or a directive for a follow-up? If its a directive for a follow-up, I think it could be made more clear, e.g., "CMutableTransaction should have the the same member functions to allow both classes to be used in generic code. This is deferred to a follow-up."

purpleKarrot commented at 9:01 AM on June 23, 2026: contributor

@josibake, adding the observers to CMutableTransaction cannot be deferred to a follow-up. Adding them to both CTransaction and CMutableTransaction is a precondition for the automatic refactoring that follows. The reason is that we have code like this:

template <typename TxType>
void foo(const TxType& tx) {
  for (const auto& in : tx.vin) {
    ...
  }
  ...
}

If that .vin is refactored to .GetInputs(), the function template can no longer be instantiated with CMutableTransaction unless the observer is added there as well.

josibake commented at 11:42 AM on June 23, 2026: member

@purpleKarrot thanks for clarifying, I realised I was confusing myself. I do think a nice polish would be to update the tx-observer code to handle the template case. I started tinkering with it as a way of getting more familiar with the code and came up with this:

diff --git a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp
index 57a8824c0c..379a550dec 100644
--- a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp
+++ b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp
@@ -6,6 +6,7 @@
 #include "observers.h"

 #include <clang-tidy/ClangTidyModule.h>
+#include <clang-tidy/ClangTidyModuleRegistry.h>

 class BitcoinModule final : public clang::tidy::ClangTidyModule
 {
diff --git a/contrib/devtools/bitcoin-tidy/observers-base.cpp b/contrib/devtools/bitcoin-tidy/observers-base.cpp
index 075e2c1d70..0b2c73eb5a 100644
--- a/contrib/devtools/bitcoin-tidy/observers-base.cpp
+++ b/contrib/devtools/bitcoin-tidy/observers-base.cpp
@@ -1,5 +1,6 @@
 #include "observers-base.h"

+#include <clang/AST/ExprCXX.h>
 #include <clang/Lex/Lexer.h>

 using namespace clang;
@@ -13,10 +14,34 @@ void ObserversBase::registerMatchers(MatchFinder* Finder)
       member(fieldDecl(hasParent(cxxRecordDecl(hasName(ClassName))))))
       .bind("member"),
     this);
+  Finder->addMatcher(
+    cxxDependentScopeMemberExpr(
+      anyOf(hasMemberName("vin"),
+            hasMemberName("vout"),
+            hasMemberName("version"),
+            hasMemberName("nLockTime")),
+      unless(hasAncestor(functionDecl(hasName("UnserializeTransaction")))))
+      .bind("dependent_member"),
+    this);
 }

 void ObserversBase::check(MatchFinder::MatchResult const& Result)
 {
+  if (auto const* DME = Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>("dependent_member")) {
+    if (DME->isImplicitAccess()) {
+      return;
+    }
+
+    auto It = MemberToAccessor.find(DME->getMember().getAsString());
+    if (It == MemberToAccessor.end()) {
+      return;
+    }
+
+    diag(DME->getMemberLoc(), "replace direct member access with accessor")
+      << FixItHint::CreateReplacement(DME->getMemberLoc(), It->second);
+    return;
+  }
+
   auto const* ME = Result.Nodes.getNodeAs<MemberExpr>("member");
   if (!ME || ME->isImplicitAccess()) {
     return;

I ran this and it does catch the case pointed out by @janb84.

This, however, is a non-blocking suggestion and rather a nice to have, or even just a review tool. I mainly did this to convince myself there weren't other template functions that could take both CMutableTransaction and CTransaction that had been missed. In doing so, I ended up convincing myself its not really an issue considering there is no current usage of CTransaction in these template functions and if someone were to attempt it in the future, it would be a compile error.

josibake commented at 12:06 PM on June 23, 2026: member

ACK https://github.com/bitcoin/bitcoin/pull/35569/commits/c2dbec1ed279d73c76a28d23d02fcc3bdd433a1f

Adding observer functions for data members moves towards an overall better design where the caller does not depend on the class layout. While not fully implemented in this PR, this is the first step towards a transaction type with regular semantics.

More concretely, direct access to vin, vout, version, and nLockTime effectively makes the class layout the public API. Rather, we should have serialisation and the transaction API be expressed in an implementation agnostic interface instead of tightly coupled to today's concrete implementation.

Thanks for taking the suggestions regarding removing the assignment operator, and removing the default move constructor. These changes ensure we preserve the same class invariants while refactoring.

DrahtBot requested review from janb84 on Jun 23, 2026

purpleKarrot commented at 12:43 PM on June 23, 2026: contributor

@josibake, I don't want to add the names of one particular class into ObserversBase. That class is intended to be a common base and provides the logic for checks that inherit from it and parameterize it with names in their constructor.

The mutation check in the current implementation is not powerful; it has some false negatives. For CTransaction, that is not an issue, because its members cannot be mutated anyway. But if I fix it and add a checker for CMutableTransaction, I get changes in another 119 lines, including the one that @janb84 identified.

I can integrate that into this PR, or keep this PR focused on CTransaction and do CMutableTransaction as a follow-up.

josibake commented at 12:55 PM on June 23, 2026: member

I don't want to add the names of one particular class into ObserversBase

Absolutely! "tinkering" was meant to imply this was not the proper way to do it, rather I do think it would be nice to detect the template case in a more clean way. However, once I ran the hacked code and realised there were no other such cases, it started to feel like a waste of time.

do CMutableTransaction as a follow-up.

I think this is the right call. As you said, it keeps this PR focused, and it may be that CMutableTransaction is made unnecessary as this refactor progress, but that is not a topic for this PR.

sedited added this to a project on Jun 25, 2026

github-project-automation[bot] changed the project status on Jun 25, 2026

in src/primitives/transaction.h:312 in a35a2b108a

 308 | @@ -309,6 +309,11 @@ class CTransaction
 309 |      explicit CTransaction(const CMutableTransaction& tx);
 310 |      explicit CTransaction(CMutableTransaction&& tx);
 311 |  
 312 | +    [[nodiscard]] auto GetVersion() const -> uint32_t { return version; }

maflcko commented at 12:43 PM on June 25, 2026:

a35a2b108acf355293443d5ad42cbf84152a3363: I don't think nodiscard should be added here, because the boilerplate adds basically no value. It isn't in the dev notes, but the pattern is documented in:

https://github.com/bitcoin/bitcoin/blob/7b84e5106c38c146f2393b9a337e2d604fd64faf/src/kernel/bitcoinkernel.h#L32-L40

purpleKarrot commented at 1:53 PM on June 26, 2026:

I can remove it if there is consensus that [[nodiscard]] is not wanted here. I just got used to following clang-tidy's modernize-use-nodiscard, which adds it to all const member functions.

maflcko commented at 2:19 PM on June 26, 2026:

I think the modernize-use-discard is harmful for promoting the exact opposite of a meaningful policy. I wonder if anyone has this check enabled in a useful way. Let's recall that in modern C++ (smart pointers and optional values), the isn't really a risk of resource leaks (like in the kernel C-header). So the remaining use cases are legacy code, or error/status codes, but both of those use-cases are excluded in modernize-use-discard.

If we wanted to follow the clang-tidy rule, it should be enabled in the config, but again I don't think that is useful.

Instead, it could make sense to copy-paste the existing kernel header policy to the dev notes, so that it is clear it applies to all C++ code in this repo?

purpleKarrot commented at 2:29 PM on June 26, 2026:

I don't consider passing around smart pointers modern C++. Pointers ("smart" or not) are low level utilities that should be used exclusively for implementing composite types and never appear in function signatures, neither as arguments nor as return type.

The purpose of modernize-use-nodiscard is not to avoid resource leaks, but to indicate: "This function has no observable side effect. Only call this function if you are interested in its returned value."

in src/primitives/transaction.h:313 in a35a2b108a

 308 | @@ -309,6 +309,11 @@ class CTransaction
 309 |      explicit CTransaction(const CMutableTransaction& tx);
 310 |      explicit CTransaction(CMutableTransaction&& tx);
 311 |  
 312 | +    [[nodiscard]] auto GetVersion() const -> uint32_t { return version; }
 313 | +    [[nodiscard]] auto GetInputs() const -> const std::vector<CTxIn>& { return vin; }

maflcko commented at 9:46 AM on June 26, 2026:

a35a2b108acf355293443d5ad42cbf84152a3363: You say that a span can not be returned here in the blog, but I wonder why that is. The blog says that some call sites have hard-coded vector::iterator types. However, those should be trivial to adjust as well.

I think the real reason is that span serialization doesn't support non-Byte spans, and also doesn't support the length prefix.

a bit unrelated, but I wonder if it could make sense to allow all spans to be serialized (iff the element can be serialized), but require an explicit mode to be set: Either without serializing the size prefix, or without.

In any case, I think LIFETIMEBOUND could be added here?

maflcko commented at 10:21 AM on June 26, 2026:

edit: I read your other blog post and I see you want to replace this by encode_range(w, tx.vin, encode_txin); in which case my alternative suggestion may be stale.

purpleKarrot commented at 2:00 PM on June 26, 2026:

some call sites have hard-coded vector::iterator types. However, those should be trivial to adjust as well.

Yes, they could be automatically refactored with clang-tidy's modernize-use-auto and further with modernize-loop-convert. But those cleanups are orthogonal. They could be done as a follow-up, or they could be done before this one. But I'd rather not squash them into this PR.

in src/policy/truc_policy.h:20 in c2dbec1ed2

  16 | @@ -17,7 +17,7 @@
  17 |  
  18 |  // This module enforces rules for BIP 431 TRUC transactions which help make
  19 |  // RBF abilities more robust. A transaction with version=3 is treated as TRUC.
  20 | -static constexpr decltype(CTransaction::version) TRUC_VERSION{3};
  21 | +static constexpr std::uint32_t TRUC_VERSION{3};

maflcko commented at 10:03 AM on June 26, 2026:

nit in the last commit: Using the std:: prefix may be minimally more correct, but there is no place in the current codebase that uses the prefix for such integral types. It may be better to drop it for consistency. Also, while it doesn't help type safety, I minimally prefer to have named types here. It is unlikely that the type is going to change again (27e70f1f5be1f536f2314cd2ea42b4f80d927fbd), but it can't hurt. Just a nit, though.

maflcko commented at 11:00 AM on June 26, 2026: member

left some nits, but feel free to ignore.

purpleKarrot force-pushed on Jun 26, 2026

purpleKarrot commented at 3:14 PM on June 26, 2026: contributor

I removed [[nodiscard]] and std:: as per @maflcko's comments. I still think they make sense in both places, but maybe it is better to add them with a repository wide cleanup, by applying modernize-use-nodiscard.

Edit: Also added LIFETIMEBOUND.

purpleKarrot force-pushed on Jun 26, 2026

DrahtBot added the label CI failed on Jun 26, 2026

DrahtBot removed the label CI failed on Jun 26, 2026

purpleKarrot requested review from josibake on Jun 26, 2026

purpleKarrot requested review from maflcko on Jun 26, 2026

maflcko commented at 10:55 AM on June 27, 2026: member

I am still thinking about this change conceptually. As of this pull request, the changes are mostly a no-op cleanup. Yes, it is a bit nicer to use private over const here, but externally, the same copy and move ctors are provided, so I think this change is mostly a style change. It seems there are several goals:

https://purplekarrot.net/blog/bitcoin-tidy-transaction.html (step 4): "Implement Deserialization". I think this is equally a mostly style-wise cleanup and otherwise a no-op. I think it can probably be skipped as optional.
"Step 7: One Indirection": I think this is nice. Practically in most places CTransactionRef is used (but it is not really a reference), so hiding the shared pointer and allowing to pass CTransaction as-is with shared pointer semantics seems simpler and nicer.

I am happy to review this pull as-is, and it seems fine to merge as-is, but I wonder if other reviewers think that step 7 is worthwhile, (and whether step 4 is worthwhile or can be skipped)?

alexanderwiederin commented at 10:34 AM on June 29, 2026: contributor

I think this PR can be assessed independent from steps 4 and 7. The old design violated the C++ Core Guidelines C.12.

josibake commented at 11:14 AM on June 29, 2026: member

I am still thinking about this change conceptually. As of this pull request, the changes are mostly a no-op cleanup. Yes, it is a bit nicer to use private over const here, but externally, the same copy and move ctors are provided, so I think this change is mostly a style change

Perhaps we use the word style differently, but I tend to think of style as two different representations of the same architectural principle.

I'd argue that style is the wrong framing here since we are talking about an architectural refactor, specifically encapsulation. This PR makes the invariant the responsibility of the class, enforced through a private representation rather than a const keyword on a public field. This fundamentally changes which designs are possible, now and in the future. Identical semantics today, as you point out, but the set of possibilities open for discussion is now open.

Said differently, I don't think reviewers being interested in exactly 4 and 7 as they are proposed today is particularly relevant for merging this PR. Rather, being interested in discussing 4 and 7 at all and perhaps other follow ups is the strongest argument for this PR on its own: these follow up discussions are now possible.

josibake commented at 11:15 AM on June 29, 2026: member

reACK https://github.com/bitcoin/bitcoin/pull/35569/commits/a2b339547f6c8acc34ec18d6b44abf0d728a9886

ajtowns commented at 6:30 PM on June 29, 2026: contributor

This still seems a waste of review resources to me. If you want to be able to mutate a transaction, use CMutableTransaction, don't add a +644-513 PR because you dislike the coding style.

The old design violated the C++ Core Guidelines C.12.

That guideline only says "Don’t make data members const or references in a copyable or movable type" which is fine -- CTransaction doesn't have much need to be copied or moved (if different parts of the code want different handles on the same tx, that's what CTransactionRef is for), and copying/moving is only possible because of implicit constructors. Deleting those constructors catches a few mistakes: https://github.com/ajtowns/bitcoin/commits/202606-del-tx-copy-cons/

josibake commented at 11:12 AM on June 30, 2026: member

If you want to be able to mutate a transaction, use CMutableTransaction

I disagree with this framing, and I don't think your objections apply here. This PR is not talking about mutating a transaction. The new API is const observer access, assignment remains deleted, and move is not provided.

Furthermore, CMutableTransaction existing is not a reason for CTransaction to expose its layout. The question here is not whether mutation should exist somewhere. It is whether callers of the immutable transaction type should depend directly on vin, vout, version, and nLockTime. They should not. Those fields make up the invariant that transaction data, hash, m_witness_hash, and m_has_witness all describe the same object. That invariant belongs behind the CTransaction API boundary.

CTransactionRef is also not a sufficient answer. CTransactionRef is std::shared_ptr<const CTransaction> exposed as a public vocabulary type. What I mean here is APIs that want “a transaction” therefore inherit pointer semantics: ownership, nullability, aliasing, .get(), .reset(), and use_count(). Those are not transaction semantics. If shared immutable storage is useful (it is), it should be an implementation detail behind a transaction API, not something every caller has to pull in.

This is why I said “coding style” is the wrong characterisation. Public fields vs private fields behind observers are not two styles of implementing the same abstraction. Public fields make the class layout the API. Private fields make CTransaction responsible for its own representation and invariants. I very much disagree that reviewing these types of architectural changes are a waste of reviewer time as they have implications on what is possible or not possible in the codebase today and into the future. They also matter for the safety, correctness, and maintainability of the code. These seem like topics reviewers would be interested in, to me.

Regarding the copy/move branch you posted, this reinforces the C.12 point rather than refuting it. Public const data members create an irregular type surface where copy exists, assignment does not, and move behaviour is surprising. If making those operations explicit exposes call sites that need cleanup (which your branch does), then the current design already has accidental special member semantics. C.12’s point is that those semantics should be intentional, not side effects of const fields. Deleting copy/move might be an interesting follow up, but it is not an alternative to this PR. It addresses one symptom while leaving layout as API (the root cause) untouched. This PR instead establishes the boundary: callers should not depend on the storage layout of a type whose fields participate in an invariant. Said differently, your branch seems to be treating a symptom, not addressing the root cause.

I will reiterate: we do not need agreement on those future designs for this PR to be valuable today. Reviewers can still argue later for non copyable CTransaction, an internal shared immutable body, a different CTransactionRef, or a value handle over an immutable body. We could also agree on no further change. This PR only establishes the prerequisite boundary for those discussions: callers stop depending on concrete layout, while today’s immutability and aliasing needs are preserved.

purpleKarrot commented at 11:32 AM on June 30, 2026: contributor

That guideline only says

I can give a few more guidelines that are violated in the current design:

Should I continue and dig out more C++ Core Guidelines that are violated here? Or would you rather prefer examples from C++ literature? Conference talks? You name it.

ajtowns commented at 1:11 PM on June 30, 2026: contributor

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, and wasting limited development resources on low and negative impact activities.

I can give a few more guidelines that are violated in the current design:

Perhaps you should review the introductory section: "We do not suffer the delusion that every one of these rules can be effectively applied to every code base.", "The rules are not perfect. A rule can do harm by prohibiting something that is useful in a given situation."

* [C.3: Represent the distinction between an interface and an implementation using a class](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-interface)

I could see the argument for declaring CTransaction as-is as a struct rather than a class per C.2 or C.8, but that's not a rule we consistently use in this codebase, and because we don't do that, the follow-on assumptions about how classes should act are also not justified:

* [C.4: Make a function a member only if it needs direct access to the representation of a class](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-member)

* [C.9: Minimize exposure of members](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-private)

* [C.11: Make concrete types regular](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-regular)

* [C.81: Use =delete when you want to disable default behavior (**without wanting an alternative**)](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-delete)

Yes, that one is a good idea, and it catches real implementation flaws, and I included it in the patchset I linked.

Should I continue and dig out more C++ Core Guidelines that are violated here? Or would you rather prefer examples from C++ literature? Conference talks? You name it.

"Should I continue with arguments to authority?" No, you shouldn't. You should justify changes with the practical benefits they achieve.

In this case, separating an implementation and interface allows you to change the implementation without impacting the places where that interface is used, but that's not the case here: we shouldn't be changing the implementation because "create a CTransaction, cache its hashes, treat it as immutable, and keep it around as a shared_ptr" is already what we want. Instead, for 0% benefit, this PR is hitting 100% of the cost by changing every place that touches the interface. The only way you could get a worse tradeoff is by actively introducing bugs, as well.

I disagree with this framing, and I don't think your objections apply here. This PR is not talking about mutating a transaction. The new API is const observer access, assignment remains deleted, and move is not provided.

The only reason for making that change now is to prepare for making it mutable in future. Pretending otherwise just comes across as dishonest. I am not currently very convinced that the proposed serialization changes are a worthwhile improvement (eg, where are the benchmarks, if compile time improvements are the main argument?), but claiming that is the rationale while constantly arguing about style guidelines and private fields also comes across as fairly dishonest.

Furthermore, CMutableTransaction existing is not a reason for CTransaction to expose its layout.

The reason for CTransaction to expose its layout is that adding an abstraction layer over the top buys nothing but complexity and code churn. Dealing with transactions is pretty much the primary job of our codebase, so the implementation details are relevant to pretty much every part of the code, not something that should be abstracted away.

What I mean here is APIs that want “a transaction” therefore inherit pointer semantics

APIs that want "a transaction" can accept const CTransaction& if they simply want to use it for the lifetime of that call, or a CTransactionRef if they want to be able to extend the lifetime of that transaction, at which point ownership/lifetime/etc are worth worrying about anyway. If they want to modify the transaction, they can either copy it or accept some form of CMutableTransaction in the first place.

ryanofsky commented at 1:43 PM on June 30, 2026: contributor

re: #35569 (comment)

The only reason for making that change now is to prepare for making it mutable in future.

For the record, josibake's description is accurate: as of the latest revision, assignment is deleted and the observers are const.

On the direction: step 6 from the blog post would have CTransaction hold shared_ptr<const TransactionData> internally, making the shared body immutable at the type level. That reads to me as increasing immutability guarantees rather than decreasing them. Am I reading the end goal right? If there's a specific harm you see in the steps between here and there, it would be useful to understand concretely.

EDIT: Corrected reference to step 6 instead of step 7 in blog post.

purpleKarrot commented at 1:57 PM on June 30, 2026: contributor

Am I reading the end goal right?

100%, @ryanofsky. My goal is to make CBlock, CTransaction and a few more primitive types immutable. But a properly immutable type is a type that has no accessible modifier functions. It is not a type with deleted copy/assignment.

Making CTransaction non-copyable and then storing it as a pointer in CBlock and making it publicly accessible does not gain any immutability at all, as clients can freely reassign it to another transaction object or even to nullptr. I understand that some clients need nullability, but CBlock is not one of them and consensus code is not defined to operate on nullable transactions.

purpleKarrot commented at 2:09 PM on June 30, 2026: contributor

compile time improvements are the main argument

Where did you read that, @ajtowns? My blog mentions compile times, but then proceeds with:

The most fundamental problem, however, is architectural. The current design treats the C++ class layout as the source of truth for the serialization format.

josibake commented at 2:16 PM on June 30, 2026: member

The only reason for making that change now is to prepare for making it mutable in future. Pretending otherwise just comes across as dishonest.

The proposed direction is not toward mutable transaction data. It is toward a value handle over immutable transaction data, as explained in the blogs, by @purpleKarrot , and myself in this PR. This is a stronger immutability model than exposing shared pointer semantics as the transaction API.

alexanderwiederin commented at 5:45 PM on July 2, 2026: contributor

Verified 858899e reproduces.

Checked out ede288d1 and ran the documented fixup with clang/clang-tidy 22.1.8 (per ci/test/00_setup_env_native_tidy.sh):

export CC="$(which clang-22)" 
export CXX="$(which clang++-22)"
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-22 -p build -load='build/tidy/libbitcoin-tidy.so' -checks='-*,bitcoin-tx-observers' -fix

The resulting git diff is identical to 858899e.

Tests: suite passes on tip

Ran the full unit test suite. The tip (CTransaction: Make data members private) depends on every external access having been correctly converted to observer calls in the prior commit.

mzumsande commented at 8:46 PM on July 2, 2026: contributor

I will reiterate: we do not need agreement on those future designs for this PR to be valuable today.

I disagree with that. In my opinion, refactors of the very basic consensus code should have a concrete advantage and should not be done on general grounds such as C++ guidelines alone. So I do think that this would need to be justified by possible follow-ups, or at least some non-generic examples such as preventable bugs or footguns (which @ajtowns alternative branch does).

On a language level, terms such as vin, vout etc. have long become part of the public protocol: For example encapsulating vin through GetInputs() internally within bitcoin core, just to expose them as vin again to the outside via RPC (getrawtransaction) seems weird to me and it would probably be a nuisance for devs to remember the right term on the right level.

In summary, I think that CTransaction is not just a random C++ class, but at the heart of the bitcoin protocol itself, so I don't think arguing only with generic C++ guidelines re: encapsulation is sufficient here.

providing the kernel API (...)

"this might set the direction to a bunch of further work" from Kernel WG IRC update

The kernel project is mentioned neither in the PR description nor the blog post. If this helps the kernel project to design their API better (or if there is any other relation to the kernel work) it would be good to explain this in more detail.

in src/primitives/transaction.h:359 in a2b339547f outdated

 354 | +    uint32_t nLockTime;
 355 | +
 356 | +    /** Memory only. */
 357 | +    bool m_has_witness;
 358 | +    Txid hash;
 359 | +    Wtxid m_witness_hash;

l0rinc commented at 11:08 PM on July 2, 2026:

What's the reason for dropping the consts from these?

    const bool m_has_witness;
    const Txid hash;
    const Wtxid m_witness_hash;

maflcko commented at 12:07 PM on August 20, 2026:

Yeah, seems fine to leave all of them const? The benefit is that initialization is enforced by the compiler and assignment-ctor is deleted anyway?

in contrib/devtools/bitcoin-tidy/observers-base.cpp:8 in a2b339547f outdated

   0 | @@ -0,0 +1,72 @@
   1 | +#include "observers-base.h"
   2 | +
   3 | +#include <clang/Lex/Lexer.h>
   4 | +
   5 | +using namespace clang;
   6 | +using namespace clang::ast_matchers;
   7 | +
   8 | +void ObserversBase::registerMatchers(MatchFinder* Finder)

l0rinc commented at 11:14 PM on July 2, 2026:

I agree that this check is useful during development, but I'm not sure it should remain in the tree after the migration.

After the final commit, the compiler enforces the CTransaction boundary, so this check is only scaffolding unless we plan to keep using it for other classes. If it is only scaffolding, could the PR end with a commit that removes it, or could the helper live outside the repo?

If it is meant to stay as reusable bitcoin-tidy infrastructure, could we align it with the existing check style first? The new files currently use different conventions from nontrivial-threadlocal (no MIT header, #pragma once, global namespace, 2-space indentation). Or is that also something we should change because other other guidelines request it?

purpleKarrot commented at 12:03 PM on August 11, 2026:

I have rewritten the check in a way that it no longer hardcodes any concrete symbol names. This makes the check generally useful for other places and should stay.

I have no strong opinion on the code style of bitcoin-tidy. Ideally, it would follow LLVM's style just in case we want to upstream any of those.

in src/primitives/transaction.h:301 in a2b339547f outdated

 318 | +    CTransaction& operator=(const CTransaction&) = delete;
 319 | +
 320 | +    auto GetVersion() const -> uint32_t { return version; }
 321 | +    auto GetInputs() const LIFETIMEBOUND -> const std::vector<CTxIn>& { return vin; }
 322 | +    auto GetOutputs() const LIFETIMEBOUND -> const std::vector<CTxOut>& { return vout; }
 323 | +    auto GetLockTime() const -> uint32_t { return nLockTime; }

l0rinc commented at 11:16 PM on July 2, 2026:

What's the purpose of introducing trailing-return syntax here? What role does auto serve, apart from aligning the names? Seems aggressive...

    uint32_t GetVersion() const { return version; }
    const std::vector<CTxIn>& GetInputs() const LIFETIMEBOUND { return vin; }
    const std::vector<CTxOut>& GetOutputs() const LIFETIMEBOUND { return vout; }
    uint32_t GetLockTime() const { return nLockTime; }

More broadly, as @mzumsande also mentioned, I am not convinced that introducing new names for vin, vout, version, and nLockTime is an improvement. Could we keep the protocol names instead?

uint32_t version() const { return version; }
const std::vector<CTxIn>& vin() const LIFETIMEBOUND { return vin; }
const std::vector<CTxOut>& vout() const LIFETIMEBOUND { return vout; }
uint32_t nLockTime() const { return nLockTime; }

With indexed helpers, the diff becomes even closer (and safer) to the current code:

const CTxIn& vin(size_t index) const LIFETIMEBOUND { return m_vin.at(index); }
const CTxOut& vout(size_t index) const LIFETIMEBOUND { return m_vout.at(index); }

For example:

- if (tx.vin.empty())
+ if (tx.vin().empty())

- if (tx.vout.empty())
+ if (tx.vout().empty())

- for (const auto& txout : tx.vout)
+ for (const auto& txout : tx.vout())

- for (const auto& txin : tx.vin) {
+ for (const auto& txin : tx.vin()) {

- if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100)
+ if (tx.vin(0).scriptSig.size() < 2 || tx.vin(0).scriptSig.size() > 100)

- for (const auto& txin : tx.vin)
+ for (const auto& txin : tx.vin())

That would make the mechanical diff much easier to review manually. The renaming question can be debated separately, but as-is the change reads like a broad spelling rewrite with non-negligible review cost and little immediate benefit.

purpleKarrot commented at 4:37 AM on July 3, 2026:

What's the purpose of introducing trailing-return syntax here?

Readability. Did you see how the names of the function names are nicely aligned? Is this not something that directly jumps to the eye?

Could we keep the protocol names instead?
uint32_t version() const { return version; }

I have explained the naming choice in my blog referenced in the PR description. It is not possible in C++ because it creates a naming conflict. You cannot overload a member function with a data member.

With indexed helpers ...

Let's not create more technical debt. We currently have lots of index based loops that could be rewritten as range based loops or algorithms. Introducing "helpers" complicates such modernization. When only the first element is needed, .front() should be preferred.

l0rinc commented at 5:11 AM on July 3, 2026:

Did you see how the names of the function names are nicely aligned

Sure, I even mentioned it on the same line: "What role does auto serve, apart from aligning the names?"

Is this not something that directly jumps to the eye?

Are you planning on changing it in the whole codebase? Or will this be the only place where we use "readable" code?

It is not possible in C++ because it creates a naming conflict

Not sure what you mean, I simply didn't migrate CMutableTransaction and renamed the fields to start with m_ prefix locally.

purpleKarrot commented at 5:25 AM on July 3, 2026:

... renamed the fields to start with m_ prefix locally.

Sure, the naming conflict can be solved by changing the data members, but then the three-step refactoring approach is not possible:

Introduce new API without any other changes.
Migrate from old API to new API fully automated with no manual intervention.
Retire old API.

Since you said you disagree with the "Approach", can you explain another approach and also explain what problems you see with the approach that I follow?

l0rinc commented at 6:10 AM on July 3, 2026:

The approach that I'm objecting to is aiming to do things "by the book" without understanding the project specifics. I fell into the same trap at the beginning, and I also received a lot of pushback. This is a huge refactor at the most critical part of the code, and it's not obvious what we're getting in return (besides appeals to authority, which most of us are allergic to). Especially since the refactor revealed incidental loose coupling (CMutableTransaction and CTransaction having to have the same field names) which should be hardened instead of papered over by adding yet another getter to the already public fields. It's also trying to force new names instead of considering what the domain already has. When I suggest alternatives, the reply is just a patronizing "that's impossible in C++". This approach seems too aggressive and dismissive to me, hence my nack.

purpleKarrot commented at 12:08 PM on August 11, 2026:

The motivation for this change is mentioned in #35904. Please stop making those unfounded claims. This is a tiny, straightforward change, as most if it is automated and reproducible.

in src/consensus/tx_verify.cpp:24 in a2b339547f outdated

  21 |          return true;
  22 | -    if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
  23 | +    if ((int64_t)tx.GetLockTime() < ((int64_t)tx.GetLockTime() < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
  24 |          return true;
  25 |  
  26 |      // Even if tx.nLockTime isn't satisfied by nBlockHeight/nBlockTime, a

l0rinc commented at 11:33 PM on July 2, 2026:

It's awkward to keep these references now that the field is hidden:

- // Even if tx.nLockTime() isn't satisfied by nBlockHeight/nBlockTime, a
+ // Even if the transaction's nLockTime isn't satisfied by nBlockHeight/nBlockTime, a

- // Note these tests were originally written with tx.version=1
+ // Note these tests were originally written with transaction version 1

- // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients.
+ // We want to make sure the transaction outputs are not used now that we are passing outputs as a vector of recipients.

- // Clear tx.vout since it is not meant to be used now that we are passing outputs directly.
+ // Clear the transaction outputs since they are not meant to be used now that we are passing outputs directly.

- // Clear tx.vout since it is not meant to be used now that we are passing outputs directly.
+ // Clear the transaction outputs since they are not meant to be used now that we are passing outputs directly.

- // Clear tx.vout since it is not meant to be used now that we are passing outputs directly.
+ // Clear the transaction outputs since they are not meant to be used now that we are passing outputs directly.

- // txouts needs to be in the order of tx.vin
+ // txouts needs to be in the order of the transaction inputs

- // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients.
+ // We want to make sure the transaction outputs are not used now that we are passing outputs as a vector of recipients.

purpleKarrot commented at 12:13 PM on August 11, 2026:

I agree. Lesson to learn: Avoid referring to concrete symbol names in technical writing. But this is a much larger issue that should be approached independently. See also https://github.com/bitcoin/bips/pull/2195#discussion_r3747735099.

in src/primitives/transaction.h:372 in 2e69721360 outdated

 368 | @@ -364,6 +369,11 @@ struct CMutableTransaction
 369 |      explicit CMutableTransaction();
 370 |      explicit CMutableTransaction(const CTransaction& tx);
 371 |  
 372 | +    auto GetVersion() const -> uint32_t { return version; }

l0rinc commented at 12:18 AM on July 3, 2026:

2e69721 CTransaction: Add observer member functions:

These extra accessors feel awkward, especially on CMutableTransaction.

add the same member functions to CMutableTransaction for symmetry

This seems like a code smell worth addressing before adding more API surface. If the issue is that some generic read-only code is instantiated with both CTransaction and CMutableTransaction, could we make that dependency explicit instead of adding duplicate read APIs to the mutable builder type?

Or could the generic read-only code use an immutable transaction view instead? For example, serialization/read-only helpers could depend on a cheap non-owning transaction view constructible from both types, or mutable transactions could be moved into an immutable serializable type at the boundary where that is needed. That would let CTransaction own its private representation while CMutableTransaction remains the straightforward mutable construction type.

Another option would be to move a mutable transaction into an immutable serializable type at the boundaries where serialization is needed.

purpleKarrot commented at 12:18 PM on August 11, 2026:

The code smell is CMutableTransaction as a whole. There is no real use-case for a mutable transaction type. As explained in #35904, there are only a limited amount of places where transactions are mutated, and those actually benefit from being rewritten anyway.

l0rinc commented at 6:02 PM on August 11, 2026:

The code smell is CMutableTransaction as a whole

Yes, so let's clean that up first - please see my branch where I did just that.

purpleKarrot commented at 7:38 AM on August 12, 2026:

You did not remove CMutableTransaction in your branch. Instead, you added yet another layer of indirection on top, proving my theory of the intervention spiral. #35904 explains what refactoring steps are needed to make CMutableTransaction obsolete.

in src/policy/policy.h:151 in a2b339547f outdated

 147 | @@ -148,8 +148,8 @@ std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
 148 |  // Changing the default transaction version requires a two step process: first
 149 |  // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later
 150 |  // allowing the new transaction version in the wallet/RPC.
 151 | -static constexpr decltype(CTransaction::version) TX_MIN_STANDARD_VERSION{1};
 152 | -static constexpr decltype(CTransaction::version) TX_MAX_STANDARD_VERSION{3};
 153 | +static constexpr uint32_t TX_MIN_STANDARD_VERSION{1};

l0rinc commented at 12:35 AM on July 3, 2026:

a2b3395 CTransaction: Make data members private:

I would be for these changes in a separate PR.

purpleKarrot commented at 12:14 PM on August 11, 2026:

I have moved them to a separate commit.

l0rinc changes_requested

l0rinc commented at 1:03 AM on July 3, 2026: contributor

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.

Technically, the current patch seems mostly behavior-preserving: callers could not mutate CTransaction before because the public fields were const, and callers still cannot mutate it after because the new observers are const. Assignment remains deleted and move is not provided. So the practical safety delta appears small, while the review cost is high.

I also share the concern raised above about “arguments to authority.” Guidelines and literature can be useful for framing, but they should not substitute for showing why this particular change improves Bitcoin Core specifically.

The PR also has knock-on effects that make the abstraction feel premature: CMutableTransaction gets duplicate const observers just so generic code keeps compiling.

If the goal is a later shared immutable transaction body or a new serialization boundary, I think that design should be motivated directly first, with concrete examples of why it is worth doing. Without that, this feels like a broad mechanical refactor of very central code for speculative follow-up work.

josibake commented at 8:24 AM on July 3, 2026: member

I disagree with that. In my opinion, refactors of the very basic consensus code should have a concrete advantage and should not be done on general grounds such as C++ guidelines alone.

Your response seems to be arguing against a claim I did not make. My argument is not “merge this because the C++ Core Guidelines say so.” The guidelines are excellent supporting evidence, but not the justification. The justification I am arguing for is that CTransaction currently exposes the layout as the API. By making fields participating in the invariant public, we are tightly coupling all callers to the representation. This is a fragile design. Rather, the representation of the invariant should be owned entirely by CTransaction behind an API, i.e. encapsulation.

The question is not whether encapsulation is idiomatic in the abstract, or performative to some set of guidelines. The question is whether CTransaction’s concrete layout defining its public API is better than CTransaction exposing transaction behaviour while owning its representation and invariants internally. I have not seen an argument for why the current public layout design is better. Most objections seem to defend the status quo by pointing to review cost or familiarity, but those are not arguments that public storage is the better abstraction. @ajtowns 's branch, as I already pointed out, illustrates my point. It found places where the current design allows accidental full transaction copies when the intent was to keep or pass a transaction reference. Deleting copy/move catches some of those mistakes, which is good, but it addresses one symptom. The broader issue remains: callers depend directly on the storage layout of a type whose fields participate in an invariant. Encapsulation gives us a boundary where those semantics can be made intentional, whether the followup is deleting copy/move, using an internal immutable body (as explained in the blogs and the design I strongly prefer!), changing CTransactionRef, or something else. The C++ core guidelines are relevant here because they describe the class of failure mode ajs branch attempts to fix. This is not to say "oh we must have strict adherence to the guidelines," rather its the C++ core guidelines pointing out: "you guys aren't the first project to make this mistake."

The naming concern you raise is orthogonal, imo. Whether the observers should be called GetInputs() / GetOutputs() or use familiar vocabulary such as vin() / vout() is a separate API naming discussion (and likely a mechanical rename/scripted diff). It is not an argument for keeping the storage layout public. Likewise, vin and vout being protocol vocabulary does not mean the in-memory C++ representation must expose public fields. I do not agree with the the framing of CTransaction as being the heart of the bitcoin protocol. Its not. Its a C++ class in this project that represents protocol data. The protocol and serialisation format must remain precise and stable (and implementation agnostic!), but that does not require class layout to be the public API. This is mentioned explicitly in the serialisation blog.

As I argued before: this refactor stands own its own because it establishes a clear API boundary not tightly coupled to representation. This enables the discussion and implementation of future design, e.g. the stronger immutability model detailed by @purpleKarrot in his blogs. The status quo keeps API defined by class layout, which is fragile and forces us into suboptimal design space.

in contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp:15 in ede288d1c5 outdated

  11 | @@ -11,6 +12,7 @@ class BitcoinModule final : public clang::tidy::ClangTidyModule
  12 |  public:
  13 |      void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override
  14 |      {
  15 | +        CheckFactories.registerCheck<TxObservers>("bitcoin-tx-observers");

l0rinc commented at 3:10 AM on July 6, 2026:

ede288d bitcoin-tidy: Add tx-observers check:

will this be executed by tidy on every run now?

l0rinc changes_requested

l0rinc commented at 2:17 AM on July 7, 2026: contributor

To be clear, I can agree with the concept of moving CTransaction away from public storage given good motivation and easy review. My main concern is the approach: I could agree with it if the path were broken into smaller manually reviewable steps with non-general, concrete Bitcoin Core reasoning for each step: what each step enables, what reviewers should check, and why it is worth doing even if the later design changes. General C++ best-practice arguments are not enough for me here.

It would also help to have the rest of the intended series prepared as draft PRs or demonstration branches, so reviewers can evaluate the path end-to-end instead of reviewing a preparatory refactor against a series of "to be continued" blog posts.

For comparison, I tried a smaller shape here over the weekend: l0rinc/bitcoin#212 commits. It differs from this PR in three main ways:

It first routes generic mutable and immutable transaction reads through explicit helpers, so CMutableTransaction can stay the mutable builder instead of getting mirror observers just to preserve shared public field spelling.
It uses a reusable, parameterized, tested bitcoin-tidy field-observer helper and runs it one field at a time (nLockTime, version, vout, vin), so each scripted diff is a smaller, CI-passing review unit.
It keeps the protocol names as observers (tx.vout(), tx.vin(), etc.) instead of renaming the API to GetOutputs() / GetInputs(), and updates the related code comments.

That shape makes the call-site diff mostly adding parentheses after the generic-read split, keeps mutable writes and exceptional serialization paths out of the immutable-transaction migration, and should reduce the need for repository-specific macro skips such as READWRITE or VARINT in the tidy check. If compile-time improvements, a future serialization boundary, kernel API shape, or stronger immutability motivate this refactor, it would be helpful to state that in the PR with concrete examples or evidence. Linked posts can provide background, but the GitHub PR itself should carry the reviewer-relevant motivation and tradeoffs.

ajtowns commented at 11:51 AM on July 8, 2026: contributor

compile time improvements are the main argument

Where did you read that, @ajtowns?

"The refactoring is a prerequisite for further refactoring that will untangle the type from serialization logic (...) which will result in ... faster compilation."

DrahtBot added the label Needs rebase on Jul 9, 2026

willcl-ark added the label Refactoring on Jul 9, 2026

willcl-ark added the label Validation on Jul 9, 2026

jonatack commented at 5:16 PM on July 21, 2026: member

Needs rebase.

purpleKarrot force-pushed on Jul 27, 2026

DrahtBot added the label CI failed on Jul 27, 2026

DrahtBot commented at 3:05 PM on July 27, 2026: contributor

🚧 At least one of the CI tasks failed. Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/30276063951/job/90010322508 LLM reason (✨ experimental): CI failed during the C++ build because src/coins.cpp tried to access CTransaction::vin, which is private (error: ... is private within this context).

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

DrahtBot removed the label Needs rebase on Jul 27, 2026

maflcko commented at 7:25 AM on July 30, 2026: member

Maybe turn into draft while CI is red?

purpleKarrot marked this as a draft on Aug 3, 2026

purpleKarrot force-pushed on Aug 3, 2026

purpleKarrot marked this as ready for review on Aug 3, 2026

purpleKarrot commented at 4:13 PM on August 3, 2026: contributor

The clang-tidy check is now using annotations rather than hardcoding CTransaction and the names of the data members. @theuni, should bitcoin-tidy live in a separate repository?

DrahtBot removed the label CI failed on Aug 3, 2026

DrahtBot added the label Needs rebase on Aug 4, 2026

purpleKarrot force-pushed on Aug 6, 2026

purpleKarrot commented at 9:09 AM on August 6, 2026: contributor

@ajtowns, @mzumsande, @l0rinc, and others:

I want to assure you, that this refactoring is not motivated on "general grounds such as C++ guidelines" or "appeals to authority". It was also not done "without understanding the project specifics" or "without considering why certain decisions were taken".

It is a byproduct of a very deep analysis of the consensus validation code with the goal of providing a stateless validation component: #35904

I do understand the motivations that led to the data members being const, which then led to the split between CTransaction and CMutableTransaction, which then led to the requirement for certain function templates taking a generic transaction type, which then led to the problem that it is no longer obvious whether the template instantiations that are tested are actually the ones that are used in production. It is an intervention spiral: A hack always demands further hacks.

I also do understand the project's requirements on stability and performance and I have identified places where those requirements are not fulfilled, like the errors that are thrown during deserialization, for which I have provided a proof-of-concept solution that also does not rely on overload resolution and is therefore easier to reason about than the current deserialization code.

Am I not allowed to identify such problems just because the original authors provided more proof of work? How is that not "appeals to authority"?

DrahtBot removed the label Needs rebase on Aug 6, 2026

theuni commented at 4:41 PM on August 6, 2026: member

This is an amazing use of bitcoin-tidy. Nice work getting the attribute/annotation machinery working. It's a thing of beauty :)

At least for now, I think it makes sense to keep it part of this repo. Allowing the plugin to be updated in the same PR as the changes that use it makes life substantially easier. I realize it could be helpful (and more secure) to separate the concerns, but I think it makes sense to defer that move until there's a concrete reason to do it.

purpleKarrot requested review from l0rinc on Aug 11, 2026

purpleKarrot commented at 8:48 AM on August 12, 2026: contributor

terms such as vin, vout etc. have long become part of the public protocol

Is this the vocabulary that we want Bitcoin to maintain for the next hundred years across implementations? In that case we should specify that in BIP453. But if we assume that coming generations will find terms like inputs and outputs easier to reason about, we should specify those instead and then slowly converge code bases and RPC protocols towards that BIP. A function like GetInputs() can be interpreted as applying a coding style on the standardized term inputs.

mzumsande commented at 12:17 PM on August 12, 2026: contributor

Is this the vocabulary that we want Bitcoin to maintain for the next hundred years across implementations?

Sure, why not - it's just words, I have no intentions of telling people what terms to use or not to use.

In that case we should specify that in https://github.com/bitcoin/bips/pull/2195. But if we assume that coming generations will find terms like inputs and outputs easier to reason about, we should specify those instead and then slowly converge code bases and RPC protocols towards that BIP.

First, it is already specified in the linked BIP453 draft, as one of multiple synonyms, see "transaction output list"/"transaction input list". More importantly, BIP453, as I understand it, is descriptive and not normative in nature. If many code bases would start using a different term for something, we should amend the BIP by adding this term as a synonym instead of telling the code bases to change.

purpleKarrot commented at 1:17 PM on August 12, 2026: contributor

I have no intentions of telling people what terms to use or not to use.

Well, in your original comment you had a very strong opinion that vin and vout have to stay, because they "have long become part of the public protocol". That sounds very normative.

mzumsande commented at 2:38 PM on August 12, 2026: contributor

Well, in your original comment you had a very strong opinion that vin and vout have to stay, because they "have long become part of the public protocol". That sounds very normative.

I'm not sure if you are serious, and I will leave this unproductive discussion after this, but normative/descriptive are categories for universal language-defining documents such as BIPs, and I only brought them up in response to you talking about "future generations" using terms. ("we should specify those instead and then slowly converge code bases and RPC protocols towards that BIP").

Me having an opinion to a proposed change in a single local project that I contribute to has nothing to do with that. Besides, I did not even utter a "very strong opinion that vin and vout have to stay" as you allege, but pointed out the potential confusion of encapsulating things on an intermediate level under a different name, that are then exposed under the internal name with the RPC interface: I wrote "For example encapsulating vin through GetInputs() internally within bitcoin core, just to expose them as vin again to the outside via RPC (getrawtransaction) seems weird to me".

I did not opine on whether to resolve this confusion by keeping the terms, or by removing vin also from the rpc. And I certainly don't care what terms other projects or future generations use. So please stop putting words in my mouth that I didn't say.

l0rinc commented at 5:39 PM on August 12, 2026: contributor

@purpleKarrot, you're receiving valuable feedback, but instead of learning from it, you act offended and try to force your subjective opinion even harder. The responsibility falls on you to argue for the change, but completely ignoring the established culture and trying to force your own opinion won't scale in this environment. There are many things we need to fix here (especially now that AI scanning reveals hundreds of potential attack angles), and these opinionated refactors are not the best use of our time. I will unsubscribe from this thread. I'm not here to continue arguing.

purpleKarrot commented at 7:05 PM on August 12, 2026: contributor

In the "valuable feedback", the change is descibed as:

"not a good idea at all"
"a waste of review resources"
"acting as a supply-chain attack vector"
"wasting limited development resources"
"low and negative impact"
"arguments to authority" (3x)
"0% benefit, 100% of the cost"
"only actively introducing bugs would be worse"
"comes across as dishonest" (2x)
"buys nothing but complexity and code churn"
"done on general grounds such as C++ guidelines alone" (2x)
"seems weird to me"
"no concrete benefit"
"small practical safety delta"
"premature"
"without understanding the project specifics"
👎

It also continued in a similar style in private conversation.

Not one single comment was articulating that in this particular case, public constdata members should be preferred. The only complains were about

how the change was approached,
how the change was articulated,
that this change was proposed in the first place, and foremost
that I dared to propose this change without going through some form of trial first.

willcl-ark commented at 8:31 AM on August 13, 2026: member

Concept ACK

hodlinator commented at 9:38 AM on August 13, 2026: contributor

The larger motivation is still not mentioned in this PR description and I think that's part of the issue here causing disagreement.

My impression is that the author's view is that this refactoring by itself serves the long term good, and that PRs should stay very "low-level" building blocks which may be referenced by larger projects but for some reason shouldn't spell out the larger goal. Is the expectation that people reviewing should either be aware of the larger goal anyway, and otherwise not review?

It's not immediately obvious why the Kernel API would be concerned with internals of Bitcoin Core. Kernel as it stands today is a bigger higher level wrapper of Core features than the prior libconsensus approach. But I've heard rumors of a surviving subdomain of Kernel which aspires to expose a stateless/pure block/transaction validation API, with part of the goal being that Bitcoin Core should use that API. That would in turn justify "reaching in" to refactor these Core types. The validation library is mentioned in the linked serialization article, but I think spelling it out in the PR description together with the end goal of making Bitcoin Core use it is not too much to ask.

I'm not experienced with the benefits and pitfalls of public interface with private shared_ptr<implementation> member vs passing around shared_ptr<type>, nor const variants. The former promotes hidden indirection/pointer chasing while the latter has explicit pointer chasing but also risks accidental copying (https://github.com/bitcoin/bitcoin/pull/35569#issuecomment-4835740603). I think many other seasoned devs here (both proponents and opponents of the PR) are also not deeply familiar with the subculture of "regular types" and their trade-offs.

Especially without knowing the overarching desired end state of Bitcoin Core using the public validation library, I can really understand judging this refactoring as overrated off-hand.

(In the past I've been annoyed at CTransaction on master and thought a better name would be ImmutableTransaction).

purpleKarrot commented at 1:55 PM on August 13, 2026: contributor

The larger motivation is still not mentioned in this PR description and I think that's part of the issue here causing disagreement.

The PR is older than #35904 and I haven't updated the description yet to mention it. It is also not strictly required for a stateless validation library. The issue description makes a clear separation between the necessary refactoring steps and additional "interface segregation" steps. This PR falls into the second category.

But I doubt that the missing link in the description is the root cause of the disagreement here.

My impression is that the author's view is that this refactoring by itself serves the long term good, and that PRs should stay very "low-level" building blocks which may be referenced by larger projects

This is correct.

but for some reason shouldn't spell out the larger goal. Is the expectation that people reviewing should either be aware of the larger goal anyway, and otherwise not review?

I did not make it a secret that this serves #35904. But I also don't want to frame it as a requirement of that issue. Encapsulation has benefits by itself.

It's not immediately obvious why the Kernel API ... But I've heard rumors ...

It has got little to do with the Kernel API and it is not a rumor. It has its own issue. Discussion about validation should take place there. This PR is about providing encapsulation for CTransaction.

I'm not experienced with ... many other seasoned devs here are also not deeply familiar with ... "regular types".

That is a different stance than "you don't understand the project's specifics". For those willing to get familiar and experienced, I am happy to provide explanations or point to articles, books, videos. But those that dismiss any best practice of the larger C++ community as "preachy", or "appeal to authority", I cannot help.

Especially without knowing the overarching desired end state of Bitcoin Core using the public validation library, I can really understand judging this refactoring as overrated off-hand.

That should not be the case. As I stated above, and as @josibake has pointed out repeatedly above, this PR provides benefits by itself.

(In the past I've been annoyed at CTransaction on master and thought a better name would be ImmutableTransaction).

The idea of having an immutable type associated with a "builder" is alien to C++. It is a necessary evil in languages built on reference semantics, where this pattern is very common. You can compare how strings work differently in C++ vs C#. The fundamental idea is that mutability and sharing should be mutually exclusive to prevent conflicts. Where everything is shared by default (C#), you want to restrict mutation. When things are copied by default (C++), mutation is less of an issue.

For CTransaction however (and also for CBlock, CTxIn, CTxOut, and COutPoint), mutation is a rare use case. Those types should be made immutable, with no associated builder (no CMutableTransaction), and code that "builds" those types in procedural code should be rewritten in a declarative style (this mostly affects test code).

The following code builds a transaction using the CMutableTransaction builder:

https://github.com/bitcoin/bitcoin/blob/11090c8bb359f894ef7d97b65aff52fe8191aec1/src/test/miner_tests.cpp#L162-L168

The same logic in declarative style does not need a builder:

    const auto tx = CTransaction{
        CTransaction::CURRENT_VERSION,
        std::vector{
            CTxIn{
                COutPoint{txFirst[0]->GetHash(), 0},
                CScript() << OP_1,
                CTxIn::SEQUENCE_FINAL,
            },
        },
        std::vector{
            CTxOut{
                5000000000LL - 1000,
                CScript{},
            },
        },
        0,
    };

The best way to make a type immutable in C++ is to not provide any mutating member function. This does not include operator=, however. Removing operator= does not make a type less mutable, it makes it less usable. For example, it can no longer be stored in a vector. One workaround (note: intervention spiral!), is to wrap it in a std::shared_ptr, which is what CTransactionRef is doing. But this has two nasty consequences: It introduces sharing, so now you really need to prevent mutation. The much bigger issue is that it introduces emptyness. Each CTransactionRef can hold a transaction, it can be changed to a different transaction, and it can hold not a transaction at all. Making the std::shared_ptr an implementation detail of CTransaction allows providing a never-empty guarantee, which makes this type easier and safer to use.

Those benefits are completely orthogonal to #35904. It should be possible to judge this PR even without knowing the desired shape of the validation library.

ajtowns commented at 10:18 PM on August 13, 2026: contributor

This does not include operator=, however. Removing operator= does not make a type less mutable

An object with a functioning operator= is mutable.

purpleKarrot commented at 5:34 AM on August 14, 2026: contributor

This does not include operator=, however. Removing operator= does not make a type less mutable

An object with a functioning operator= is mutable.

Operations are defined on the type, not on the object. A type that does not provide any mutating member functions is immutable. An immutable type can still be regular (regularity requires operator=).

Whether an object can be modified through a variable, depends on the declaration of that variable (controlled with const and mutable).

This is consistent with other languages: In C# or JavaScript, it is possible to reassign to a variable that holds an object of an immutable type, unless reassignment is explicitly prevented through that variable. Further, this is the current behaviour of CTransactionRef.

josibake commented at 11:35 AM on August 14, 2026: member

reACK https://github.com/bitcoin/bitcoin/commit/ef3a175126164d05c82a0b41d166a523fa4c9459

I find the new approach with clang-tidy annotations to be much easier to follow and, as you mention, gets rid of hardcoding the class names. Nicely done!

DrahtBot requested review from willcl-ark on Aug 14, 2026

ryanofsky commented at 12:53 PM on August 14, 2026: contributor

Concept ACK. I don't see practical downsides worth blocking over, and the techniques and direction seem valuable.

I've also been thinking that CBlock might be an interesting place to apply the same ideas. It's mutable right now, so unlike CTransaction the benefits of locking down the interface would be more visible, especially in mining code. If this PR is stuck, maybe CBlock could be a more useful place to apply the same techniques?

josibake commented at 1:06 PM on August 14, 2026: member

@ryanofsky CBlock, along with a few other primitive types, are mentioned in #35904 to be made immutable. Not sure if there is a required / desirable order to do them in, I'll let @purpleKarrot speak to that.

EDIT: Although, I suspect its better to have CTransaction be a regular, immutable type first since a block is a collection of transactions.

DrahtBot added the label Needs rebase on Aug 14, 2026

purpleKarrot force-pushed on Aug 14, 2026

purpleKarrot commented at 9:03 PM on August 14, 2026: contributor

@ryanofsky CBlock, along with a few other primitive types, are mentioned in #35904 to be made immutable. Not sure if there is a required / desirable order to do them in

CTransaction is the easiest start, because all member accesses are read accesses. COutPoint is also fairly easy, because most member accesses are either read accesses or assignments. Most assignments of one member are directly followed by an assignment of the other member (the order is not always the same), which can be rewritten with another bitcoin-tidy check (WIP: https://github.com/purpleKarrot/bitcoin/commits/outpoint-encapsulation).

CTxIn and CTxOut are a bit more tricky, because it is much harder to determine whether a member access is mutating or not. It may be obvious when looking at the code, but for clang-tidy, it requires much deeper analysis of the AST.

CBlock involves even more work. If we order this from lowest to highest hanging fruits, it would be CTransaction, COutPoint, CTxOut, CTxIn, CBlock. But it would just as well be possible to parallelize some work. Coin may also be an interesting candidate to look at.

DrahtBot removed the label Needs rebase on Aug 14, 2026

purpleKarrot commented at 3:58 AM on August 15, 2026: contributor

Note: The three step approach, where

a new interface is added,
the code base is migrated to the new interface,
the old interface is removed,

really pays off when rebasing: Conflicting files can be reset completely and the bitcoin-tidy check can be rerun on them.

With the approach that does all three changes in a single clang-tidy check as suggested by @l0rinc, resetting individual files leaves them in an uncompilable state, as they use an interface that is no longer available.

l0rinc commented at 4:51 AM on August 15, 2026: contributor

resetting individual files leaves them in an uncompilable state

The commits in https://github.com/l0rinc/bitcoin/pull/212 all compile just fine.

purpleKarrot commented at 6:13 AM on August 15, 2026: contributor

resetting individual files leaves them in an uncompilable state

The commits in https://github.com/l0rinc/bitcoin/pull/212 all compile just fine.

Sure, each commit compiles as a whole. But how do you resolve merge conflicts? When resetting a subset of a commit, it does not compile and the clang-tidy is not able to run either. You either need to resolve manually, or you need to reset completely.

ajtowns commented at 6:30 PM on August 15, 2026: contributor

This does not include operator=, however. Removing operator= does not make a type less mutable

An object with a functioning operator= is mutable.

Operations are defined on the type, not on the object. A type that does not provide any mutating member functions is immutable. An immutable type can still be regular (regularity requires operator=).

Being deliberately obtuse is not helpful. If you're going to insist on unnecessary precision: mutable types are types whose objects can be mutated after construction. In C++, objects can be mutated by non-member functions, so not providing mutating member functions is no guarantee of immutability; in particular operator= is often not a member function.

Whether an object can be modified through a variable, depends on the declaration of that variable (controlled with const and mutable).

This is consistent with other languages: In C# or JavaScript, it is possible to reassign to a variable that holds an object of an immutable type, unless reassignment is explicitly prevented through that variable. Further, this is the current behaviour of CTransactionRef.

CTransactionRef is an alias for shared_ptr, it is not surprising that you a pointer can point at different objects over its lifetime. Other languages put different priorities on the relationship between references and the objects they reference; eg python makes numbers immutable, with every change to a number resulting in the reference pointing at a different object.

purpleKarrot commented at 6:13 AM on August 16, 2026: contributor

In C++, objects can be mutated by non-member functions, so not providing mutating member functions is no guarantee of immutability; in particular operator= is often not a member function.

Edited

I redact my comment. I still want to highlight this quote, but leave it uncommented.

josibake commented at 5:43 PM on August 17, 2026: member

Being deliberately obtuse is not helpful. If you're going to insist on unnecessary precision: mutable types are types whose objects can be mutated after construction. In C++, objects can be mutated by non-member functions, so not providing mutating member functions is no guarantee of immutability; in particular operator= is often not a member function.

I don't think anyones being obtuse, I think you're not following the discussion. Furthermore, what you claim is "often" isn't possible in C++. You cannot have a non-member operator=, as a language design choice.

This PR encapsulates CTransaction by making the data members private and and providing access through const readers. Making the data members non-const here in no way weakens the immutability of CTransaction. If you disagree, an example specific to this PR would be great.

It was also stated in the original blog, reiterated by me, and corroborated by @ryanofsky that the desired end goal stated by the author is a stronger immutability guarantee than what we have today. To reiterate again:

class CTransaction {
public:
    CTransaction(const CTransaction&) = default;
    CTransaction(CTransaction&&) noexcept = default;
    CTransaction& operator=(const CTransaction&) = default;
    CTransaction& operator=(CTransaction&&) noexcept = default;

    // the observers added in this PR 
   ...

private:
    std::shared_ptr<const TransactionData> m_data;
};

This gives a CTransaction type with regular value semantics, which serves as a handle to an immutable transaction body. If you believe this introduces mutability or is worse than the current design, please provide an example specific to what I reiterated above.

As I was testing this PR, inspired by my comment on the bugprone-use-after-move behaviour, I noticed the flag isn't smart enough to catch aliases. So its possible to create something like:

CMutableTransaction mtx;
mtx.vin.resize(1);
mtx.vin[0].scriptSig = CScript() << OP_1;

CTxIn& in = mtx.vin[0]; // alias!

const CTransactionRef tx = MakeTransactionRef(std::move(mtx));
const Txid og_hash = tx->GetHash();

in.scriptSig << OP_2; // mutate!

assert(&tx->vin[0] == &in);
assert(tx->vin[0].scriptSig == (CScript() << OP_1 << OP_2));   // not the same!
assert(tx->GetHash() == og_hash); // but the same hash!

So its possible to write perfectly valid code that does not exploit UB etc, that compiles and violates the invariant of the class by mutating CTransaction after construction. I've only spent a day or two looking into this so I don't want to overstate the claim, but I do feel increasingly strongly that moving towards regular types with value semantics and more explicit ownership will help us fix these bugs. By more explicit, I mean the constructor needs to own its storage exclusively. This is the thing that matters for closing footguns like the one mentioned above, afaict. Why does it matter that CTxIn and CTxOut are regular types? Cheap copies!

josibake commented at 5:44 PM on August 17, 2026: member

reACK https://github.com/bitcoin/bitcoin/commit/13b62f714a08e4a87f226a1ec232d9de4dbbe505

Last push was a rebase.

DrahtBot requested review from ryanofsky on Aug 17, 2026

sipa commented at 8:25 PM on August 19, 2026: member

Without wading into the meta-discussion here, or picking a specific approach to support, I'd like to bring up a concrete potential advantage of better CTransaction encapsulation (not as a suggestion for inclusion in this PR, but perhaps as motivation for it).

One of the ideas I had when I introduced the CTransaction / CMutableTransaction split, was that CTransaction could use more efficient memory allocation. For example, all dynamically-allocated memory within a transaction could be placed in a single allocator arena owned by the CTransaction, avoiding the overhead of individual vectors + allocation overheads for all inputs, outputs, scriptsigs, scriptpubkeys, witnesses, and witness stack elements, also reducing memory fragmentation and improving locality.

I never explored that idea further at the time, but #36015 makes this suddenly a lot more relevant again.

maflcko commented at 6:47 AM on August 20, 2026: member

As I was testing this PR, inspired by my comment on the bugprone-use-after-move behaviour, I noticed the flag isn't smart enough to catch aliases. So its possible to create something like:

I think this is a good point, but I don't think it is fixed by this pull. IIUC this is only fixed by the next pull that re-writes the deserialize approach. If we wanted to fix it today, it could be done by passing the mutable tx as const& (at the cost of a copy). An alternative to fix it today would be to implement a simple/stricter version of use-after-move that detects use after moved fields (partial moves) or partial uses after full moves.

Also, if we wanted to implement an arena, it could be done today, without changing code outside of src/primitives, with something like:

<details><summary>Encapsulate only the heap</summary>

#include <iostream>
#include <memory>
#include <span>
#include <vector>
#include <cstdint>

namespace detail {
struct Body {
    std::vector<int> m_vin{};
    std::vector<int> m_vout{};
};
} // namespace detail

struct MutTx {
    uint32_t version{1};
    std::vector<int> vin{};
    std::vector<int> vout{};
};

struct Tx {
private:
    const std::shared_ptr<const detail::Body> m_body;

public:
    const uint32_t version;

    // Initialized from not-null m_body. (Note: Span-serialize is incompatible with vector-serialize, so this would need to be fixed somehow)
    const std::span<const int> vin{m_body->m_vin};
    const std::span<const int> vout{m_body->m_vout};

    // Deep-copy to decouple heap buffers and guarantee true immutability, alternatively implement a stronger use-after-move static analysis.
    explicit Tx(const MutTx& tx)
        : m_body(std::make_shared<const detail::Body>(
              detail::Body{tx.vin, tx.vout})),
          version{tx.version} {}

    // Default copy/move constructors work out of the box
    Tx(const Tx&) = default;
    Tx(Tx&&) noexcept = default;

    // Wasn't present before, keep deleted
    Tx& operator=(const Tx&) = delete;
    Tx& operator=(Tx&&) = delete;
};

</details>

(No strong opinion, just wanted to drop this off here)

josibake commented at 8:39 AM on August 20, 2026: member

I think this is a good point, but I don't think it is fixed by this pull. IIUC this is only fixed by the next pull that re-writes the deserialize approach.

Correct. My claim wasn't that this is fixed by this pull, rather the direction this pull moves us in should make it easier for us to fix this bug. I kept riffing on that idea here #35904 (comment) and came up with something that composes well with the deserialisation approach proposed by @purpleKarrot (still working out some rough edges) and doesn't sacrifice performance (via copies) where it matters, e.g., block validation and reading transactions off the wire.

josibake commented at 9:54 AM on August 20, 2026: member

For example, all dynamically-allocated memory within a transaction could be placed in a single allocator arena owned by the CTransaction, avoiding the overhead of individual vectors + allocation overheads for all inputs, outputs, scriptsigs, scriptpubkeys, witnesses, and witness stack elements, also reducing memory fragmentation and improving locality.

I don't want to understate the benefits of the other follow ups proposed by the author, but I am strongly in favour of what you mention here, and it is my primary interest in better encapsulation for our primitive types. This idea has been gnawing at me since Research Week a few years back where, if you recall, myself and a few others proposed an idea of completely getting rid of transactions and thinking only in terms of blocks and block chunks.

Obviously, we still need a transaction API for this codebase, but encapsulating the concrete layout allows us to have a single transaction type that works with an arena that is allocated for an entire block and also for a single transaction off the wire (perhaps even a cluster, but I haven't thought about this deeply yet):

<details><summary>Transactions as views over a block backed arena</summary>

// the arena owns the bytes once; everything else is an offset into them. nothing
// inside a transaction is separately allocated, so there is no caller supplied container
// to allow for the nasty modify-through-reference-after-move!
struct tx_meta {
    std::uint32_t byte_off, byte_len, wit_off, wit_count;
};

struct wit_meta {
    std::uint32_t off, len;
};

// read off the wire / disk / etc once, and then only need one allocation per
// column. this example is simplified but in total its something like 6 allocations
struct arena {
    std::vector<std::byte> bytes;
    std::vector<tx_meta> tx;
    std::vector<wit_meta> wit;
};

class transaction;
auto parse_transaction(std::span<std::byte const> wire) -> transaction;

// shared handle and an index. thats the whole type and a copy is a refcount bump
class transaction {
  public:
    transaction(std::shared_ptr<arena const> a, std::uint32_t i)
        : _arena{std::move(a)}, _index{i} {}

    // all observers resolve through the same meta, no
    // "which backing am I" branch
    auto witness_count() const -> std::uint32_t { return meta().wit_count; }
    auto witness_item(std::uint32_t k) const -> std::span<std::byte const> {
        auto const& w = _arena->wit[meta().wit_off + k];
        return std::span{_arena->bytes}.subspan(w.off, w.len);
    }
    auto serialized() const -> std::span<std::byte const> {
        return std::span{_arena->bytes}.subspan(meta().byte_off, meta().byte_len);
    }

    // rehoming out of a block arena is just parsing your own bytes
    auto compact() const -> transaction { return parse_transaction(serialized()); }

  private:
    auto meta() const -> tx_meta const& { return _arena->tx[_index]; }
    std::shared_ptr<arena const> _arena;
    std::uint32_t _index{};
};

auto build(std::span<std::byte const> wire, std::uint32_t n_tx) -> std::shared_ptr<arena const> {
    auto const shape = count_shape(wire, n_tx);
    auto a = std::make_shared<arena>();
    a->bytes.assign(wire.begin(), wire.end());
    a->tx.resize(shape.txs);
    a->wit.resize(shape.wit_items);
    fill(*a, wire, n_tx); // indexed writes only
    return a;
}

// only difference between the two representations is n_tx!
auto parse_block(std::span<std::byte const> wire) -> std::vector<transaction> {
    auto const n_tx = std::to_integer<std::uint32_t>(wire[0]);
    auto const a = build(wire.subspan(1), n_tx);
    std::vector<transaction> out;
    out.reserve(n_tx);
    for (std::uint32_t i = 0; i < n_tx; ++i) {
        out.emplace_back(a, i);
    }
    return out;
}

auto parse_transaction(std::span<std::byte const> wire) -> transaction {
    return transaction{build(wire, 1), 0}; 
}

</details>

This is a code golf'd version of a proof of concept I've been working on to benchmark, and to illustrate the idea. Needs more refinement, but the early benchmark numbers I've been getting from toy code is ~15x improvement on block parsing. I also strongly suspect block validation itself can be improved by >10% because of how much easier bytes can be slurped into hashes, signatures validated, arrays can be traversed etc etc, and as a long term goal having a block representation in memory that is set up for batch validation. Said differently, having proper encapsulation allows us to maintain a stable API for testing and use, while having the freedom to adapt the concrete representation of the data over time to adapt to the needs of the workflow.

Its also worth point out how it reduces opportunities for error: ask the OS 250k times for allocations, you have 250k opportunites for error. Ask 6 times, you have 6 opportunities for error.

DrahtBot added the label Needs rebase on Aug 20, 2026

bitcoin-tidy: Add use-observers check

Add a check to bitcoin-tidy that detects when an annotated data member
is accessed outside of a member function and rewrites that access with
an observer function.

37b4e00c1c

attributes: Add USE_OBSERVER annotation helper 426fb38f65

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.

6aa81e23fe

CTransaction: Annotate data members with their corresponding observer 389d3e7ace

bitcoin-tidy: Apply use-observers fixup

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

Produce the changeset with the following commands:

```sh
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-use-observers' -fix
```

49b070669a

policy: Use explicit version type 15d21aee98

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

2a700a6723

purpleKarrot force-pushed on Aug 21, 2026

DrahtBot removed the label Needs rebase on Aug 21, 2026

Encapsulation for CTransaction #35569

Why Encapsulation? Why CTransaction?

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Why Encapsulation? Why `CTransaction`?