#35994 Primitives: Combine assignments
https://github.com/bitcoin/bitcoin/pull/35994 · · +394/-366 in 42 files, 8 commits · labels: Needs rebase, CI failed · draft
Goal
- Help developers replace procedural member mutation with declarative constructors across tests and tools
- Paves the way toward making core transaction primitives immutable
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.
Category: Test infrastructure (#39 of 45)
P4 · cleanup
- P4 because cleanup across test files is speculative and introduced bugs like dropped witness data
- Author prefers doing manual test rewrites rather than relying on this automated cleanup
P4 because the linter and test cleanup are speculative and introduced bugs such as dropped witnesses in test helpers. The author indicated an inclination toward abandoning the check in favor of manual rewrites.
Membership: Adds a clang-tidy linter module and mechanically modifies dozens of unit test, fuzzing, and benchmark files.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Category: Tools and scripts (#19 of 22)
P4 · cleanup
- P4 because the developer tool requires intrusive header annotations
- Author views the linter check as better suited for local use rather than being committed to the repo
P4 because the developer tool requires intrusive annotations in header files and is seen even by its author as better suited for temporary local use rather than being committed.
Membership: Adds a new clang-tidy check under contrib/devtools/bitcoin-tidy and edits bitcoin-tx.cpp.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Reviewability: Stale: Needs rebase
- Needs rebase, CI is failing, and author expressed preference to abandon rather than merge
The PR has merge conflicts with open PRs, failing CI, and the author expressed a preference for manual rewrites instead of merging this PR.
Author status: silent since 2026-08-21 after stating preference for manual rewrites rather than merging this PR
Open concerns:
- CONSTRUCTOR_ARGUMENT macro annotations pollute primitive headers and might be better handled without manual markup (maflcko)
- The automated fixup erroneously dropped witness data in test transaction utility functions (alexanderwiederin)
- Author noted in discussion that they lean toward abandoning this PR in favor of manual declarative rewrites (purpleKarrot)
Resolved concerns:
- Clarified that clang-tidy fixes using brace initialization would fail on narrowing conversions without explicit casts (alexanderwiederin, purpleKarrot)
Agreement: Mild
- General support for declarative setup in principle, but implementation has bugs and unwanted annotations
- Concept approval without stated reasons, noting unclear naming (l0rinc)
- Concerned intrusive macro annotations pollute core headers (maflcko)
- Automated rewrite erroneously dropped witness data in test helpers (alexanderwiederin)
- Author leans toward manual rewrites instead of merging this PR (purpleKarrot)
alexanderwiederin noted dropped witness data; author prefers manual rewrites over merging this PR
Reviewers support declarative initialization in principle, but the check requires intrusive annotations and dropped witness data in test helpers. The author has since favored manual rewrites over landing this automated tooling.
- l0rinc ACKed the concept but noted unclear naming and failing CI
- maflcko questioned the need for manual CONSTRUCTOR_ARGUMENT annotations
- alexanderwiederin found that CTxIn replacement dropped scriptWitness in test/util/transaction_utils.cpp
- purpleKarrot admitted the bug and stated 'Concerning the approach, I actually lean towards C' (using the check only as a local backlog)
Review verdicts (DrahtBot): 0
- Concept ACK: l0rinc, josibake, alexanderwiederin
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
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.