Primitives: Combine assignments #35994

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

#35904 mentions that validation code does not require the data members of COutPoint, CTxOut, and CTxIn to be individually mutable. This requirement instead comes from test code being written in a procedural rather than declarative style.

This PR presents a bitcoin-tidy check that identifies consecutive assignments to annotated data members of the same object and suggests rewriting them with a constructor invocation. This allows a mechanical refactoring of the majority of mutations and it paves the road for making the types fully immutable.

DrahtBot commented at 1:22 PM on August 17, 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/35994.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	l0rinc, josibake, alexanderwiederin

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:

#35580 (bugfix: compare non-adjusted chunk weight against block weight limit by ismaelsadeeq)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#35477 (test: exercise Schnorr signature cache in txvalidationcache_tests.cpp by theStack)

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.

purpleKarrot force-pushed on Aug 17, 2026

bitcoin-tidy: Add combine-assignments check

Add a check to bitcoin-tidy that detects multiple consecutive assignments
to data members of the same base and suggests a constructor call as a
replacement.

9ce1ec841a

purpleKarrot force-pushed on Aug 17, 2026

attributes: Add CONSTRUCTOR_ARGUMENT annotation helper a1454bb4cf

COutPoint: Add CONSTRUCTOR_ARGUMENT annotation b91732cfb1

COutPoint: Apply combine-assignments fixup 4c621e04fa

CTxOut: Add CONSTRUCTOR_ARGUMENT annotation c5e2da62ef

CTxOut: Apply combine-assignments fixup 39815d11dd

CTxIn: Add CONSTRUCTOR_ARGUMENT annotation 7874446795

CTxIn: Apply combine-assignments fixup da83f7092f

purpleKarrot force-pushed on Aug 17, 2026

DrahtBot added the label CI failed on Aug 17, 2026

DrahtBot commented at 7:30 PM on August 17, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/32057506973/job/95470930565</sub> <sub>LLM reason (✨ experimental): CI failed at compile time because transaction.h is inconsistent with the COutPoint/CTxIn/CTxOut class definitions (missing fields like hash/n, prevout, scriptSig, etc.).</sub>

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

l0rinc commented at 8:22 PM on August 17, 2026: contributor

I understand that it's not always trivial to run every check locally, but we're getting notifications about every push and it doesn't really increase the confidence in changes like this if they don't even pass CI. I usually push these to my own fork first to make sure the change makes sense before pushing here. Also, the Primitives: Combine assignments give no clue about what this PR is about, but reading the code seems to make sense, concept ACK.

DrahtBot removed the label CI failed on Aug 17, 2026

in src/primitives/transaction.h:32 in da83f7092f

  27 | @@ -28,8 +28,8 @@
  28 |  class COutPoint
  29 |  {
  30 |  public:
  31 | -    Txid hash;
  32 | -    uint32_t n;
  33 | +    CONSTRUCTOR_ARGUMENT(1, 2) Txid hash;
  34 | +    CONSTRUCTOR_ARGUMENT(2, 2) uint32_t n;

maflcko commented at 5:35 AM on August 18, 2026:

Is the annotation really required? I'd guess that clang-tidy should be able to iterator over all ctors with args itself and then derive this itself?

The benefit would be that there is only a single source of truth and this won't go stale. Also, manual marking wouldn't be needed. (If you want to limit this to a selected set of classes, it would be better to pass the list as an option to clang-tidy?)

purpleKarrot commented at 7:46 AM on August 18, 2026:

I don't want to build too many heuristics into the clang-tidy check. But depending on how we approach the change, it may be less of an issue or not be needed at all (see #35994 (comment)).

in src/bench/ccoins_caching.cpp:41 in da83f7092f

  42 | -    t1.vin[1].prevout.n = 0;
  43 | +    t1.vin[1].prevout = COutPoint(dummyTransactions[1].GetHash(), 0);
  44 |      t1.vin[1].scriptSig << std::vector<unsigned char>(65, 0) << std::vector<unsigned char>(33, 4);
  45 | -    t1.vin[2].prevout.hash = dummyTransactions[1].GetHash();
  46 | -    t1.vin[2].prevout.n = 1;
  47 | +    t1.vin[2].prevout = COutPoint(dummyTransactions[1].GetHash(), 1);

maflcko commented at 5:36 AM on August 18, 2026:

Is there a reason to not use the C++11 narrowing checks for new code? Other places use a static_cast<uint32_t>() for the second arg, and I think this is useful to know about and be explicit.

    t1.vin[2].prevout = COutPoint{dummyTransactions[1].GetHash(), 1};

purpleKarrot commented at 7:35 AM on August 18, 2026:

Yes, there are a few places where aggregate initialization would not compile, unfortunately.

maflcko commented at 10:45 AM on August 18, 2026:

Ok, I see. I wonder if it is worth it to update them manually (either by adding a static_cast, or by updating the outer type to unsigned, or so)?

Given that such a patch should be easy to review and test-only, with other benefits, it seems fine to submit it and I'd be happy to review it, but no strong opinion.

alexanderwiederin commented at 7:55 PM on August 18, 2026:

I tried switching the fix-it to emit {}. From what I can tell everything that then fails to compile is in test/bench.

maflcko commented at 5:39 AM on August 18, 2026: member

Looks like this is mostly a test-only cleanup (modulo ~6 instances in real code).

Seems fine, as this should reduce test issues due to re-using dirty state accidentally (c.f. #35863 et al)

in contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp:15 in da83f7092f

  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<CombineAssignments>("bitcoin-combine-assignments");

maflcko commented at 5:40 AM on August 18, 2026:

        CheckFactories.registerCheck<CombineAssignments>("bitcoin-prefer-ctor");

Given that the was confusion about the name, maybe name it differently?

purpleKarrot commented at 7:29 AM on August 18, 2026: contributor

I pushed this both to back the claim that most mutations appear in test code, but also as a base for discussion about the approach.

One approach would be to submit this PR as is.

An alternative approach would be to focus on one class at a time, like "COutPoint: Make type immutable". Such a PR would then apply bitcoin-prefer-ctor as introduced here, bitcoin-use-observers from #35569, some manual fixes, plus the change to make the data members private. In this approach, #35994 (review) would be less of an issue, because the annotations would be temporary anyway.

Yet another approach would be to just use this PR as a backlog to identify "places of interest" and then rewrite the code manually, one file at a time, in a declarative style. For example, this code:

https://github.com/bitcoin/bitcoin/blob/6751a323c0130310880c84e03f7104f6bb86385d/src/bench/ccoins_caching.cpp#L35-L51

could be rewritten (assuming the necessary constructors are made available) as:

const CTransaction tx_1{
    std::vector{
        CTxIn{COutPoint{dummyTransactions[0].GetHash(), 1}, CScript{} << std::vector<unsigned char>(65, 0)},
        CTxIn{COutPoint{dummyTransactions[1].GetHash(), 0}, CScript{} << std::vector<unsigned char>(65, 0) << std::vector<unsigned char>(33, 4)},
        CTxIn{COutPoint{dummyTransactions[1].GetHash(), 1}, CScript{} << std::vector<unsigned char>(65, 0) << std::vector<unsigned char>(33, 4)},
    },
    std::vector{
        CTxOut{90 * COIN, CScript{} << OP_1},
        CTxOut{},
    },
};

In this approach, #35994 (review) can be applied directly, with explicit conversions where necessary.

janb84 commented at 11:56 AM on August 18, 2026: contributor

If i'm not mistaken, I read 3 options from the comment of purpleKarrot:

A. Merge this PR as is. The tooling lands in this PR but needs annotations to work. The drawback is that the CONSTRUCTOR_ARGUMENT annotations stay in primitives/transaction.h with no guarantee that a follow-up removes them, and reviewers approve a large mechanical diff with the justification in a future PR.

B. One class at a time, ending in "immutability". For example "COutPoint: Make type immutable": annotate, run the check, run use-observers from #35569, fix the rest by hand, make the members private, and delete the annotations in the same PR. Each PR is self contained and can be judged as a whole if the change is worth the churn.

C. Use the check as a backlog only. Never merge the annotations. Run the check locally to find candidate files and rewrite them by hand in declarative style.

--

Personally I would opt for option B, where a tracking issue can be used to identify and coordinate the effort, keep this bitcoin-tidy code etc. If i'm not mistaken (or mis-understood the comment) this will result in PR's that are self contained with no, or little, follow ups so that the benefits, churn and results are clearly visible in one PR.

maflcko commented at 12:28 PM on August 18, 2026: member

I am not sure about throwing this in the same bucket as #35569. I think it is pretty clear that a CTransaction should not be mutable (otherwise there wouldn't be a MutableTransaction).

However, for other structs, is there much value/benefit? #35569 seems a bit controversial, so expanding it to stuff that has even less benefit may not be the best approach?

I'd leave this a mostly test-only cleanup (plus maybe #35994 (review)), but no strong opinion.

josibake commented at 6:01 PM on August 18, 2026: member

Concept ACK

However, for other structs, is there much value/benefit?

Cleaning up the tests is a convention. It does nothing to stop the same buggy behaviour being repeated in the future. Can you explain what benefit you see to keeping the structs in this PR mutable?

in src/test/util/transaction_utils.cpp:33 in da83f7092f

  34 | -    txSpend.vin[0].prevout.n = 0;
  35 | -    txSpend.vin[0].scriptSig = scriptSig;
  36 | -    txSpend.vin[0].nSequence = CTxIn::SEQUENCE_FINAL;
  37 | -    txSpend.vout[0].scriptPubKey = CScript();
  38 | -    txSpend.vout[0].nValue = txCredit.vout[0].nValue;
  39 | +    txSpend.vin[0] = CTxIn(COutPoint(txCredit.GetHash(), 0), scriptSig, CTxIn::SEQUENCE_FINAL);

alexanderwiederin commented at 7:57 PM on August 18, 2026:

Does this drop the witness?

purpleKarrot commented at 8:22 PM on August 18, 2026:

Indeed.

alexanderwiederin commented at 8:16 PM on August 18, 2026: contributor

Concept ACK

Do the checks here need to be run to a fixed point or is a single pass sufficient? Might be worth adding the invocation in the commit messages, so the mechanical changes can be verified.

purpleKarrot commented at 8:38 PM on August 18, 2026: contributor

I am not sure about throwing this in the same bucket as #35569. I think it is pretty clear that a CTransaction should not be mutable (otherwise there wouldn't be a MutableTransaction).

They do belong into the same bucket. As I wrote in #35569 (comment):

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

Which is exactly the claim that I wanted to back with this draft.

Concerning the approach, I actually lean towards C.