#36696 CTransaction: Add constructor taking inputs and outputs
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36696 CTransaction: Add constructor taking inputs and outputs | Ready | 0 | Strong | 49 + 38 tests | |||
|
|
|
|
| |||
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
Adds a constructor to CTransaction that accepts input ranges of CTxIn and CTxOut, and extends CTxIn constructors with an optional script witness parameter. It introduces RangeOf and ToVector helpers in util/vector.h, and demonstrates declarative transaction construction in sigopcount_tests.
Problem: Creating an immutable CTransaction currently requires initializing and modifying an intermediate CMutableTransaction, which adds boilerplate code when constructing test fixtures.
Evidence of importance:
- Part of #36692 (Declarative construction of test data) and referenced by #36694 (test, bench: construct mempool inputs/outputs declaratively)
Risk of a mistake: medium (consensus). Touches core transaction primitive constructors and hash computations; flawed construction logic or invariant breakage could corrupt transaction data or hashing.
Changed since the previous assessment: Diff is now available, showing changes isolated to transaction primitives, vector utility helpers, and sigopcount_tests.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review of the pushed fix and updated example commits 2026-10-30
Agreement
Strong
substantive support, no open objection (alexanderwiederin)
Thread notes: purpleKarrot pushed a fix for alexanderwiederin's objection to avoid moving elements out of spans, moving only when the range yields rvalues. maflcko expanded on init-list formatting but reiterated leaving the decision to other reviewers.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | suggestion, approach | Prefer simpler C++11 std::vector over generic ranges | accepted · maflcko discussed init-list formatting but reiterated leaving the decision to other reviewers | answered | yes | 2026-10-29: 'nit: I'd still prefer the easier C++11 std::vector here (https://github.com/bitcoin/bitcoin/issues/36692#issuecomment-6501722895) but I'll leave it to other reviewers to decide.' Status from: 2026-10-30: maflcko: 'But again, up to other reviewers to pick the side here :sweat_smile:' |
| alexanderwiederin (member) | objection, correctness | Copy instead of moving elements for views so std::span does not move elements out | fixed · fix pushed · author pushed a fix moving only when range yields rvalues | changed | yes | 2026-10-29: 'I think std::span{inputs} would still move the elements out of inputs. Copying for views avoids that. What do you think?' Status from: 2026-10-30: purpleKarrot: 'Fixed it. Instead of moving when `R` is an rvalue, we want to move when R *yields* rvalues.' |
| alexanderwiederin (member) | question, interface | Should the CTransaction constructor taking inputs and outputs be marked explicit? | answered · purpleKarrot explained that implicit conversion cannot happen with two required arguments | answered | yes | 2026-10-29: 'Should this be explicit like the existing ones?' Status from: 2026-10-29: purpleKarrot: 'Implicit conversion cannot happen because the constructor requires at least two arguments.' |
Support:
- alexanderwiederin (member) (2026-10-29) [read: CTxIn]: The CTxIn witness argument is a nice addition that removes workarounds needed in #36606 and #36694
Participants:
- alexanderwiederin (member, objection, 3 statements 2026-10-29 to 2026-10-29): supported CTxIn witness addition, asked if constructor should be explicit, and objected that passing span moves elements out of inputs
- maflcko (member, neutral, 2 statements 2026-10-29 to 2026-10-30): suggested C++11 std::vector over ranges but left decision to other reviewers
Dependencies
Enables:
Files
91 lines under test/bench/ci.
- src/test/sigopcount_tests.cpp +38/-53
- src/primitives/transaction.cpp +17/-4
- src/util/vector.h +20/-0
- src/primitives/transaction.h +12/-2
Card
The short text the ranking pass compares across PRs.
CTransaction gains a constructor taking generic ranges of inputs and outputs directly, and CTxIn constructors gain an optional script witness parameter. This allows declarative construction of immutable transactions without round-tripping through CMutableTransaction. The change is part of tracking issue #36692 and provides constructor primitives used by test cleanups like #36694.
Data
thread read through 2026-10-30 (10 statements), head 705d5d5737
labels: none
Categories
Utilities (logging, arguments, libraries)
P4 · cleanup
The PR adds small container conversion helpers to support declarative construction of transactions. It cleans up minor boilerplate without addressing any broader architectural burden or safety issue in the utilities library.
Membership: Adds general-purpose RangeOf and ToVector range-handling helper templates in src/util/vector.h.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1