#35994 Primitives: Combine assignments

ReviewabilityReviewsAgreementSize
#35994 purpleKarrot Primitives: Combine assignmentsAuthor silent 72d0Mild22 + 372 tests
  • Prevent accidental reuse of dirty state in tests by combining consecutive member assignments into constructor calls
  • Pave the way for making core transaction primitive types immutable across unit tests and benchmarks
  • Needs rebase, CI is failing, and the author has been silent for over two months
  • General concept support for declarative test setup and immutable structs (josibake, l0rinc, maflcko)
  • Unaddressed bug: automated transformation accidentally drops witness data, confirmed with no fix pushed (alexanderwiederin)
  • Suggests closing the draft PR due to failing CI and prolonged inactivity (maflcko)
  • 22 lines added or modified outside tests
  • 372 lines added or modified in tests
  • 366 lines removed in total
  • 42 files, 8 commits
Test infrastructure#54 of 54
P4 · cleanup
  • P4 because it is low-priority test cleanup aimed at avoiding dirty state reuse rather than fixing flakiness or coverage
  • Ranking pass: Moved to the bottom: the author prefers abandoning it in favour of the manual rewrite (#36694), and the clang-tidy check introduced a witness-dropping bug.
Ranking notes
Tools and scripts#16 of 16
P4 · cleanup
  • P4 because it adds a niche mechanical refactoring check rather than essential developer tooling
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

This PR adds a bitcoin-tidy check to detect consecutive assignments to object members and suggest constructor calls instead. It annotates COutPoint, CTxIn, and CTxOut data members with CONSTRUCTOR_ARGUMENT macros and applies the fix mechanically across 42 files.

Problem: Procedural mutation of primitive data members in tests hinders making core transaction types immutable. Developers wanting declarative test setups currently have to refactor procedural assignments manually.

Reviewability

Stale · Author silent 72d

No author activity for 72 days; a review may wait a long time for a response.

Waiting on (from the thread read):

  • author: response to maflcko's query about closing the PR, and unresolved claims 2026-10-29
  • author: response to maflcko asking whether the inactive draft can be closed 2026-10-29

Agreement

Mild

objection that needs a change was answered, no reply from the objector (alexanderwiederin: the automated replacement drops witness data)

Thread notes: Brought claims into alignment with current rules: rc:3801307204 reclassified from objection to suggestion (requests brace initialization). rc:3807467505 updated to clears_with change as dropping witness data is a correctness bug. Added missing suggestions and questions from maflcko and alexanderwiederin. / Re-read the discussion under current rules: converted rc:3801307204 from objection to suggestion (accepted), updated clears_with to change on alexanderwiederin's dropped-witness objection (whi

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
maflcko (member)suggestion, usefulnessquestioned the value of expanding immutability to other structs beyond CTransaction, suggesting leaving it as mostly test-only cleanupanswered · purpleKarrot replied arguing types should be immutable with procedural test code rewritten declarativelyansweredyes2026-08-18: 'However, for other structs, is there much value/benefit? https://github.com/bitcoin/bitcoin/pull/35569 seems a bit controversial, so expanding it to stuff that has even less benefit may not be the best approach?'
Status from: 2026-08-18: purpleKarrot: 'Those types should be made immutable, with no associated builder (no CMutableTransaction)'
alexanderwiederin (member)question, approachwhether the checks need to be run to a fixed point or if a single pass is sufficientopenansweredno2026-08-18: 'Do the checks here need to be run to a fixed point or is a single pass sufficient?'
alexanderwiederin (member)suggestion, maintenanceadd the clang-tidy invocation in commit messages so mechanical changes can be verifiedopenansweredno2026-08-18: 'Might be worth adding the invocation in the commit messages, so the mechanical changes can be verified.'
maflcko (member)objection, maintenancemanual CONSTRUCTOR_ARGUMENT macro annotations duplicate information and can go staleanswered · author replied explaining preference against heuristics and suggested alternative approaches where annotations are unnecessaryansweredyes2026-08-18: '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.'
Status from: 2026-08-18: purpleKarrot: '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'
maflcko (member)suggestion, safetyuse brace initialization to retain C++11 narrowing checksaccepted · author confirmed aggregate init wouldn't compile everywhere; reviewer accepted manual updates could be worth itansweredyes2026-08-18: '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.'
Status from: 2026-08-18: maflcko: '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'
maflcko (member)suggestion, interfacerename the clang-tidy check to bitcoin-prefer-ctor to avoid confusionopenansweredno2026-08-18: 'Given that the was confusion about the name, maybe name it differently?'
alexanderwiederin (member)objection, correctnessthe automated replacement drops witness dataanswered · author confirmed it indeed drops the witness, but no fix push followedchangedyes2026-08-18: 'Does this drop the witness?'
Status from: 2026-08-18: purpleKarrot: 'Indeed.'
maflcko (member)suggestion, styleuse init-list in duplicate_inputs.cppopenansweredno2026-08-24: 'should probably use init-list, like https://github.com/bitcoin/bitcoin/pull/36019#discussion_r3811388178'
maflcko (member)suggestion, scopeclose the draft PR as CI is failing and it has been inactive for monthsopenansweredno2026-10-29: 'CI is red and this draft is inactive for months. I guess it can be closed?'

Support:

  • l0rinc (member) (Concept ACK, 2026-08-17) [read]: reading the code seems to make sense
  • josibake (member) (Concept ACK, 2026-08-18): cleaning up tests is a convention and does nothing to stop buggy behaviour from repeating, making structs immutable is better
  • maflcko (member) (2026-08-18) [read]: should reduce test issues due to re-using dirty state accidentally
  • alexanderwiederin (member) (Concept ACK, 2026-08-18): (no reason given) [not substantive]

Participants:

  • l0rinc (member, support, 1 statements 2026-08-17 to 2026-08-17): Concept ACK; suggested testing on fork to avoid failing CI notifications
  • alexanderwiederin (member, objection, 4 statements 2026-08-18 to 2026-08-19): Concept ACK; noticed dropping witness data; supported manual rewrite approach C
  • janb84 (member, neutral, 1 statements 2026-08-18 to 2026-08-18): summarized three possible approaches and expressed preference for option B (one class at a time)
  • josibake (member, support, 1 statements 2026-08-18 to 2026-08-18): Concept ACK; argued structs should be immutable to prevent buggy behavior
  • maflcko (member, objection, 8 statements 2026-08-18 to 2026-10-29): raised concerns about CONSTRUCTOR_ARGUMENT annotations, brace initialization, check naming, and whether making structs immutable is worth it; suggested closing draft

Files

719 lines under test/bench/ci.

  • contrib/devtools/bitcoin-tidy/combine-assignments.cpp +183/-0
  • src/test/script_p2sh_tests.cpp +30/-60
  • src/test/transaction_tests.cpp +20/-40
  • src/test/mempool_tests.cpp +19/-38
  • src/test/txvalidationcache_tests.cpp +18/-36
  • src/test/miner_tests.cpp +13/-28
  • src/bench/mempool_eviction.cpp +11/-22
  • src/test/orphanage_tests.cpp +8/-16
  • src/test/txpackage_tests.cpp +6/-12
  • src/test/util/transaction_utils.cpp +5/-12
  • src/primitives/transaction.h +7/-7
  • contrib/devtools/bitcoin-tidy/combine-assignments.h +12/-0
  • src/test/txvalidation_tests.cpp +4/-8
  • src/attributes.h +9/-0
  • src/bench/ccoins_caching.cpp +3/-6
  • src/test/blockencodings_tests.cpp +3/-6
  • src/test/sigopcount_tests.cpp +3/-6
  • contrib/devtools/bitcoin-tidy/CMakeLists.txt +7/-1
  • src/test/fuzz/cmpctblock.cpp +2/-5
  • src/bench/duplicate_inputs.cpp +2/-4
  • src/bench/mempool_stress.cpp +2/-4
  • src/bench/wallet_create_tx.cpp +2/-4
  • src/psbt.cpp +2/-4
  • src/test/multisig_tests.cpp +2/-4
  • src/test/rbf_tests.cpp +2/-4
  • src/test/txospenderindex_tests.cpp +2/-4
  • src/test/miniminer_tests.cpp +1/-4
  • src/test/fuzz/package_eval.cpp +1/-3
  • src/test/fuzz/tx_pool.cpp +1/-3
  • src/test/fuzz/util.cpp +1/-3
  • src/bench/rpc_mempool.cpp +1/-2
  • src/bitcoin-tx.cpp +1/-2
  • src/kernel/chainparams.cpp +1/-2
  • src/rpc/rawtransaction_util.cpp +1/-2
  • src/test/fuzz/coinscache_sim.cpp +1/-2
  • src/test/sighash_tests.cpp +1/-2
  • src/test/util/mining.cpp +1/-2
  • src/test/validation_block_tests.cpp +1/-2
  • src/validation.cpp +1/-2
  • src/wallet/test/fuzz/spend.cpp +1/-2
  • src/wallet/test/group_outputs_tests.cpp +1/-2
  • contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp +2/-0

Card

The short text the ranking pass compares across PRs.

Draft PR adding a clang-tidy check to replace sequential member assignments with constructor calls, applying it mechanically across 40+ test and primitive files. The goal is to facilitate making primitives like COutPoint, CTxIn, and CTxOut immutable. However, the check requires intrusive macro annotations and introduced a bug dropping witness data in test helpers. The author has expressed preference for abandoning the PR in favor of manual declarative rewrites (option C), and CI is failing with merge conflicts.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-30T01:14, confidence high, input hash 4d6d979d41262244
thread read through 2026-10-29 (20 statements), head 95ea8fca11
labels: Needs rebase, CI failed · draft

Categories

Test infrastructure

P4 · cleanup (ranking pass: Moved to the bottom: the author prefers abandoning it in favour of the manual rewrite (#36694), and the clang-tidy check introduced a witness-dropping bug.)

Test code cleanliness ranks lowest among test infrastructure priorities. While intended to prevent 're-using dirty state accidentally' and encourage declarative test setups, it addresses no flakiness, coverage deficit, or test framework bottleneck.

Membership: Adds a linter check targeting procedural test setups and mechanically cleans up assignments across dozens of unit tests and benchmarks.

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

Tools and scripts

P4 · cleanup

This is a niche developer transformation tool for code cleanup rather than an essential operational tool. It automates procedural assignment refactoring into constructor calls, carrying minimal claim on reviewer bandwidth.

Membership: Adds a new Clang-Tidy check under contrib/devtools/bitcoin-tidy/ to combine consecutive member assignments into constructor calls.

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