Declarative construction of test data #36692

This is a tracking issue for individual refactorings of unit tests.

Rationale: Primitive types like CTxOut currently provide read and write access to their data members like scriptPubKey. Consensus validation does not require write access. Hence, it is forced to depend on functionality that it does not use, which is a violation of the Interface Segregation Principle (ISP). A whole category of bugs could be eliminated if this functionality was removed.

One case where write access to those data members is extensively used, is the imperative building of test data in unit tests like this:

https://github.com/bitcoin/bitcoin/blob/9bf4815b94b9eaf8807f4e569091778da6c338d5/src/bench/ccoins_caching.cpp#L36-L52

The imperative building could be replaced with declarative construction:

const CTransaction tx_1{
    std::array{
        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::array{
        CTxOut{90 * COIN, CScript{} << OP_1},
        CTxOut{},
    },
};

This example shows several advantages:

• It reduces the amount of necessary boilerplate.
• It is necessarily honest: The fact that the outputs contain an empty element is plain to see rather than hidden in a resize(2) call.
• It removes the need for a dedicated "builder" type, like CMutableTransaction in this case.

Another case where write access is currently used, is the reuse of test data, like in this example:

https://github.com/bitcoin/bitcoin/blob/9bf4815b94b9eaf8807f4e569091778da6c338d5/src/test/script_tests.cpp#L1287-L1339

The test data txFrom is constructed in line 1287 and later modified in lines 1315 and 1331, through the scriptPubKey reference defined in line 1289. Notably, each block of code that uses a different state of the test data is introduced by a comment containing the word "case" ("Single signature case", "P2SH, single-signature case", "Hardest case: Multisig 2-of-3").

Separating the individual "cases" into actually independent test cases would remove the need for write access in addition to providing additional advantages:

• Easier to reason about: The test input data is visible in a single place. It does not depend on modifications elsewhere or side effects of other tests.
• Easier to parallelize: Individual test cases can run concurrently.

Refactoring the imperative code in unit tests is a precondition for solving the ISP issues of the primitive types. The two examples above show that such a refactoring is self-contained (it only affects test code, and in most cases only individual tests, so it can be done one test at a time). It also shows that it provides benefits for each individual test on its own.

This change is tedious to review, but I think worth it. Having tests that use mutable structs over hundreds of lines and separate unit test cases and each case may modify some fields of the struct leads to test bugs (copy-paste errors) and makes it harder to read the test, because more time is spent on trying to figure out the current state than the test logic itself.

So rough Concept ACK, but obviously the approach may differ on a case-by-case basis.

declarative construction:

const CTransaction tx_1{ std::array{ CTxIn{...}...

btw, I am not sure on forcing the call sites to specify a container. This is verbose and distracting in test code. If this is done, the ctor should simply accept a std::vector<T> vin/out. This keeps all call sites clean and possibly also avoids a copy on the stack (not that performance is relevant here)? Of course, fancy callers can still do fancy stuff like passing (std::views::concat(a, b) | std::ranges::to<std::vector>());, but then the fancy stuff only requires verbose code at the fancy call sites. So my preference would be to keep this plain and simple C++11.

the ctor should simply accept a std::vector<T> vin/out. This keeps all call sites clean and possibly also avoids a copy @maflcko, the problem is that moving a vector into place allows the following:

std::vector<CTxIn> inputs = ...;
CTxIn& backdoor = inputs.back();

const CTransaction tx{std::move(inputs), ...};

backdoor = ...; // <- modify `tx`, potentially breaking its invariant

So it is better to not try to avoid the copy. But if a copy needs to be made, why should we enforce the type of the temporary? The compiler cannot deduce template arguments from bare braced initializer lists, which I would not consider a big problem here. I think it actually helps readability to specify a type because it communicates to the reader: "I am creating a container" rather than "I am creating some object". Using std::array seems to have less overhead than std::vector here and is easier to type than std::initializer_list<CTxIn>, though all three would work in the same way.

If performance of the copy becomes a concern, we should strive for making construction constexpr (not proposed here).

Right, but I'd still prefer the plain and less verbose C++11 ctor here. Recall the goal is to mostly catch accidental bugs in tests, not intentional bugs. If we are worried about the mutable reference backdoor, I think it can be trivially fixed in C++11 by just doing the copy (performance doesn't matter for this test-only code). While clang-tidy use-after-free doesn't catch such partial moves, I am sure human or LLM reviewers will catch it, so I wouldn't worry too much about it.