test, bench: construct inputs/outputs declaratively in context-dependent cases #36606

alexanderwiederin commented at 11:16 AM on October 22, 2026: contributor

Part of the effort discussed in #35994: replace procedural, field-by-field construction of COutPoint, CTxIn and CTxOut in test code with constructor calls.

This PR covers cases that are context-dependent and need manual changes, independent of a scripted-diff for the trivial bulk:

test/util/transaction_utils: in BuildSpendingTransaction, the scriptWitness assignment has to be moved after vin is replaced, otherwise the witness is dropped.
test/util/mining: in CreateBlockChain, the coinbase input's fields were split by the output assignments, with comments that are now kept next to the constructor arguments.
sigopcount_tests: BuildTxs has the same witness ordering issue as BuildSpendingTransaction.
bench/ccoins_caching: The scriptSigs are built with << appends, and the second output was created by vout.resize(2) but never assigned, so it is kept as a null CTxOut{}.
transaction_tests: test_Get builds the same transaction as bench/ccoins_caching, with the same << appends and null second output.

This change is test- and bench-only and does not change the constructed transactions.

Notes for reviewers:

CTxIn has no witness constructor argument, so scriptWitness is assigned after vin is replaced.
BuildTxs is called repeatedly on the same transaction objects. Previously, resize(1) kept the existing inputs and only overwrote some of their fields; now they are replaced entirely. The fields that were left untouched by BuildTxs are never modified by the test, so the transactions are unchanged.
The null second output in ccoins_caching is copied from test_Get in transaction_tests and looks like an oversight. It is kept here to avoid behaviour changes.

DrahtBot added the label Tests on Oct 22, 2026

DrahtBot commented at 11:16 AM on October 22, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36606
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36606.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36606#author-ralph

Reviews

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

Type	Reviewers
Stale ACK	purpleKarrot

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:

#35569 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35569.svg"></sub> (Encapsulation for CTransaction by purpleKarrot)
#35511 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg"></sub> (RFC: consensus: Make CAmount a class by hodlinator)

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.

maflcko commented at 11:43 AM on October 22, 2026: member

Seems fine, but what is the great picture here:

How many further changes are required to reach the goal?
Should larger chunks be done, possibly limited to vin, then another commit for vout?
Can the changes be done with a scripted-diff? (Possibly a match on vin.resize(1) and then vin[0].<_> = <_>? Or would that be not worth it?)
...

alexanderwiederin commented at 12:20 PM on October 22, 2026: contributor

Thanks for the feedback!

How many further changes are required to reach the goal?

Rough numbers: 593 lines assign individual fields via vin[...]/vout[...] across 50 files. The largest are: transaction_tests (122), miner_tests (90), script_p2sh_tests (72), mempool_tests (58) and bench/mempool_eviction (38).

Should larger chunks be done, possibly limited to vin, then another commit for vout?

Single commits per file seems more intuitive to me, but happy to take a different approach.

Can the changes be done with a scripted-diff? (Possibly a match on vin.resize(1) and then vin[0].<> = <>? Or would that be not worth it?)

The script would need to reorder fields into constructor arguments, fill in defaults, and handle << appends, and it can't preserve interleaved comments. I chose transaction_utils because it's where the mechanical rewrite in #35994 went wrong: BuildSpendingTransaction assigned scriptWitness before vin was replaced, which dropped the witness. That shows context matters, which is why I would do it manually.

What do you think?

maflcko commented at 12:37 PM on October 22, 2026: member

I am mostly thinking that 50 pulls/commits for this won't scale/review. My preference would be to just handle the trivial bulk of the changes with a trivial scripted-diff (that is, excluding the transformations that have lines in-between).

Then, the tedious transformations can be done manually later, hopefully being a small amount of line changes.

Happy to create an alternative pull to show it.

alexanderwiederin commented at 12:44 PM on October 22, 2026: contributor

Sounds good.

alexanderwiederin marked this as a draft on Oct 22, 2026

alexanderwiederin renamed this:
~~test: construct transaction_utils inputs/outputs declaratively~~
test, bench: construct inputs/outputs declaratively in context-dependent cases
on Oct 23, 2026

alexanderwiederin marked this as ready for review on Oct 23, 2026

alexanderwiederin commented at 2:20 PM on October 23, 2026: contributor

@maflcko I've added four more commits covering cases that need manual work regardless of the scripted-diff.

I've marked this ready for review, since these are independent of the trivial bulk. Happy to rebase on top of your change if that lands first.

alexanderwiederin force-pushed on Oct 23, 2026

purpleKarrot commented at 9:02 AM on October 26, 2026: contributor

This PR is a prime example of why I think Core needs a different development process, where a plan is decided before work starts. Assuming we had agreed on the plan that bitcoin's vocabulary types shall be made immutable. Then the review of this PR would be trivial:

Does the PR refer to an agreed plan? Check.
Is the PR a step in the right direction towards the goal laid out in the plan (no matter how small)? Check.
Does the PR preserve existing behavior/contracts (modulo changes that the plan demands)? Check.
Bonus points: It has less lines of code in total compared to the existing code.

ACK 4da6b9af11634f01a72098ca2d0e3e63fdf2b210

We need to reverse the approach where people are discouraged from opening PRs and encouraged to build a reputation through making code reviews. I know that reviews in the current process are a bottleneck, but the assumption that we can solve it with more reviewers is naive. Imagine a carpentry shop where apprentices watch the master at work and are supposed to weigh in with comments.

We need to agree on a plan and then let people work on it in small increments. Such increments are easy to review (example above) and should allow timely integration, which gives a motivating boost to continue and may attract more developers. This process scales way better than opening PRs containing hundreds of commits or offering contributors an "alternative pull".

in src/bench/ccoins_caching.cpp:42 in 4da6b9af11

  50 | +    t1.vin = {
  51 | +        CTxIn{COutPoint{dummyTransactions[0].GetHash(), 1}, CScript() << std::vector<unsigned char>(65, 0)},
  52 | +        CTxIn{COutPoint{dummyTransactions[1].GetHash(), 0}, CScript() << std::vector<unsigned char>(65, 0) << std::vector<unsigned char>(33, 4)},
  53 | +        CTxIn{COutPoint{dummyTransactions[1].GetHash(), 1}, CScript() << std::vector<unsigned char>(65, 0) << std::vector<unsigned char>(33, 4)},
  54 | +    };
  55 | +    t1.vout = {CTxOut{90 * COIN, CScript() << OP_1}, CTxOut{}};

maflcko commented at 10:46 AM on October 28, 2026:

    t1.vout = {CTxOut{90 * COIN, CScript() << OP_1}, CTxOut{},};

nit: I find this harder to read. I think all vector assignments here should have an init-list with a trailing comma (even for N=1). Previously, it was easy to see there are two outputs, now one has to manually parse this line and count the commas. Also, appending more elements in the future is easier with line-broken init-lists.

alexanderwiederin commented at 12:36 PM on October 28, 2026:

Done - thanks!

maflcko commented at 10:46 AM on October 28, 2026: member

Ok, fair enough. I took a look at a scripted-diff, but I couldn't find a common pattern. (There is << vs =, skipped fields, random orders, etc) So manual review will be needed anyway on a case-by-case basis.

test: construct transaction_utils inputs/outputs declaratively

Replace resize() followed by per-field assignments to COutPoint, CTxIn
and CTxOut with constructor calls in an initializer list.

In BuildSpendingTransaction, the scriptWitness assignment moves after
vin is replaced, since CTxIn has no witness constructor argument and
assigning it first would discard it.

No behavior change.

c91158a8c1

test: construct CreateBlockChain coinbase declaratively

Replace the field-by-field construction of the coinbase input and output
in CreateBlockChain with CTxIn and CTxOut constructor calls.

The constructed transaction is unchanged.

ed16701f14

test: construct sigopcount_tests BuildTxs inputs/outputs declaratively

Replace the field-by-field construction of the inputs and outputs in
BuildTxs with COutPoint, CTxIn and CTxOut constructor calls.

As in BuildSpendingTransaction, the scriptWitness assignment must come
after vin is replaced, since CTxIn has no witness constructor.

BuildTxs is called repeatedly on the same transaction objects. Before,
resize(1) kept the existing input and output and only overwrote some of
their fields. Now they are replaced entirely. This does not change the
constructed transactions, since the test never modifies the fields that
were left untouched.

3a0318af87

bench: construct ccoins_caching inputs/outputs declaratively

Replace the field-by-field construction of the inputs and outputs in
CCoinsCaching with COutPoint, CTxIn and CTxOut constructor calls.

The second output was previously only created by vout.resize(2) and
never assigned, leaving it null (nValue = -1). It is kept as a
default-constructed CTxOut so the transaction is unchanged.

8dd3fb8232

test: construct transaction_tests test_Get inputs/outputs declaratively

Replace the field-by-field construction of the inputs and outputs with
COutPoint, CTxIn and CTxOut constructor calls.

The second output was previously only created by vout.resize(2) and
never assigned, leaving it null (nValue = -1). It is kept as a
default-constructed CTxOut so the transaction is unchanged.

e7320faded

alexanderwiederin force-pushed on Oct 28, 2026

alexanderwiederin commented at 12:39 PM on October 28, 2026: contributor

Ok, fair enough. I took a look at a scripted-diff, but I couldn't find a common pattern. (There is << vs =, skipped fields, random orders, etc) So manual review will be needed anyway on a case-by-case basis.

Thanks for taking a look. For the remaining cases I’ll group several files per PR. Let me know if you have a preference for how to group them.