#36224 test: Add test coverage for `PartiallySignedTransaction::Merge()`
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36224 test: Add test coverage for `PartiallySignedTransaction::Merge()` | Ready | 0 | Crickets | 0 + 96 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 unit test coverage for `PartiallySignedTransaction::Merge()` in `src/test/psbt_tests.cpp`. The new tests check that PSBTs with mismatched versions fail to merge, that `fallback_locktime` values are preserved or adopted as expected, and that `m_tx_modifiable` flags are merged across inputs.
Problem: Portions of `PartiallySignedTransaction::Merge()` had no unit test coverage, leaving version mismatch rejection, fallback locktime propagation, and modifiable flag bitwise operations open to undetected regressions.
Evidence of importance:
- PR description demonstrates three specific mutations in `src/psbt.cpp` (allowing version mismatches, inverting fallback locktime conditions, and altering modifiable bitflag combination conditions) that pass existing test suites without failing.
Risk of a mistake: low (tests). Touches only unit tests in test/psbt_tests.cpp, so any defect would only affect the test suite.
Changed since the previous assessment: Minor rebase and adjustments to test setup across the three commits.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: unanswered suggestion to test the positive AND case for m_tx_modifiable 2026-09-28
- reviewer: polespinasa to check the implemented unique ID check 2026-10-01
Agreement
Crickets
no substantive comment either way
Thread notes: nebula-21 answered polespinasa's suggestion to check matching unique IDs and added them as co-author. The suggestion to increase test coverage for m_tx_modifiable remains open.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| polespinasa (member) | suggestion, approach | Add a check verifying that psbt_v0 and psbt_v2 have matching unique IDs | answered · author implemented the suggestion and added reviewer as co-author | answered | yes | 2026-09-28: 'BOOST_REQUIRE(psbt_v0.GetUniqueID() == psbt_v2.GetUniqueID());' Status from: 2026-10-01: nebula-21: 'Thanks! I’ve made the change and added you as a co-author.' |
| polespinasa (member) | suggestion, approach | Increase test coverage by testing the positive AND case for m_tx_modifiable | open | answered | no | 2026-09-28: 'maybe can increase the coverage with positive and case?' |
Participants:
- polespinasa (member, neutral, 3 statements 2026-09-28 to 2026-09-28): suggested unique ID check and additional test coverage
Files
96 lines under test/bench/ci.
- src/test/psbt_tests.cpp +96/-0
Card
The short text the ranking pass compares across PRs.
Adds unit tests for PartiallySignedTransaction::Merge() in src/test/psbt_tests.cpp. It covers edge cases that previously had no test coverage, specifically rejecting merges between PSBTs of different versions, resolving fallback_locktime values, and combining m_tx_modifiable bitflags. The author provides mutation test examples in src/psbt.cpp that previously passed undetected.
Data
thread read through 2026-10-01 (4 statements), head 9b78edce89
labels: Tests