Improve Sender Input Validation Tests and Error Types

[Jolah1]

Part of #678.

What

Check that every original sender outpoint is still present in the proposal, in its original order, before classifying any proposal input as the receiver's. Existing receiver-input tests now modify the proposal's actual receiver input instead of the sender's.

Why

Master already rejects proposals with missing or shuffled sender inputs, but only after the input loop. An altered sender outpoint is classified as a receiver input first, so the sender sees an unrelated error ("an input in proposed transaction belonging to the receiver is not finalized"). Checking ordering up front makes the error name the actual problem.

The UTXO-data check was dropped per review; that part of #678 remains open.

Tests

cargo test -p payjoin ordered_subsequence --all-features
cargo test -p payjoin test_sender_input_outpoint_changed --all-features
cargo test -p payjoin test_receiver_input_ --all-features
cargo test -p payjoin --all-features
cargo fmt --all -- --check
cargo clippy -p payjoin --all-targets --all-features -- -D warnings
codespell

Disclosure: code and PR text drafted with AI assistance (Codex, Claude); reviewed and tested by me.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MRG6vHFsY9Ny88eRtTQ2vz

[coveralls]

Coverage Report for CI Build 34593798280

Coverage increased (+0.02%) to 86.669%

Details

• Coverage increased (+0.02%) from the base build.
• Patch coverage: 47 of 47 lines across 1 file are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	16578
Covered Lines:	14368
Line Coverage:	86.67%
Coverage Strength:	341.27 hits per line

---

💛 - Coveralls

[caarloshenriq]

Concept ACK d576e05

The security logic is sound. The outpoint pre-check correctly uses a single advancing iterator with any() to enforce that all original outpoints appear as an ordered subsequence in the proposal, catching modified outpoints early instead of misclassifying them as receiver inputs. The UTXO validation with is_none_or correctly accepts stripped fields (per BIP 78) while rejecting modified or injected data. The fix to the existing receiver-input tests (operating on input[1] instead of mangling input[0]) is a genuine improvement: those tests now exercise the actual receiver input.

Two commit hygiene points:

This commit does three things: outpoint pre-check, UTXO data validation, and existing test fixes. Per project conventions each logical unit should be its own commit. At minimum the test fixes should be separate, since they're independently correct and useful regardless of the new validation.

The commit message has no body. For a security-relevant change to PSBT validation, it would be worth explaining the threat model (receiver modifying UTXO data to invalidate signatures or cause the sender to sign a different transaction). The context in #678 covers this well and the body could reference it.

[Jolah1]

Concept ACK d576e05

The security logic is sound. The outpoint pre-check correctly uses a single advancing iterator with any() to enforce that all original outpoints appear as an ordered subsequence in the proposal, catching modified outpoints early instead of misclassifying them as receiver inputs. The UTXO validation with is_none_or correctly accepts stripped fields (per BIP 78) while rejecting modified or injected data. The fix to the existing receiver-input tests (operating on input[1] instead of mangling input[0]) is a genuine improvement: those tests now exercise the actual receiver input.

Two commit hygiene points:

This commit does three things: outpoint pre-check, UTXO data validation, and existing test fixes. Per project conventions each logical unit should be its own commit. At minimum the test fixes should be separate, since they're independently correct and useful regardless of the new validation.

The commit message has no body. For a security-relevant change to PSBT validation, it would be worth explaining the threat model (receiver modifying UTXO data to invalidate signatures or cause the sender to sign a different transaction). The context in #678 covers this well and the body could reference it.

Thanks — split into three commits: the test fixes first (they pass on master's validation logic unchanged, so they stand alone as you said), then the outpoint pre-check, then the UTXO data validation with its error variant.
Each builds and passes cargo test --all-features + clippy independently; verified the three together reproduce the original tree exactly. Commit bodies now cover the threat model for each change and reference #678. Also
rebased onto current master while I was rewriting.

[caarloshenriq]

tACK bd2186a

All sender-input validation tests (test_sender_input_outpoint_changed, test_sender_input_witness_utxo_changed, test_sender_input_non_witness_utxo_changed) and the corrected receiver-input tests pass.

[xstoicunicornx]

utACK bd2186a.

PR addresses some deficiencies in accurate error reporting when the sender is validating the payjoin proposal. None of these changes are absolutely necessary but do improve the clarity of our tests and error reporting.

Provided some small feedback.

[Jolah1]

utACK bd2186a.

PR addresses some deficiencies in accurate error reporting when the sender is validating the payjoin proposal. None of these changes are absolutely necessary but do improve the clarity of our tests and error reporting.

Provided some small feedback.

updated bd2186a → 6c62fb2: extracted is_ordered_subsequence with a unit test, split the UTXO check into two ensures, and added a test for unchanged/stripped data. Both folded into their existing commits, so the three-commit structure is unchanged. Re-ACK when you have a moment.

[xstoicunicornx]

BIP 78 doesn't currently say anything about these fields, we are enforcing this arbitrarily. Which I think is fine, but thinking about it more would like others to weigh in too.

[Jolah1]

Agreed, this is policy, not spec, and the comment now says so. It also doesn't close a hole: restore_original_utxos overwrites these fields two lines after the check, so the sender signs its own values either way. The gain is a named error instead of a silent overwrite. Happy to drop this commit if the preference is status quo.

[xstoicunicornx]

Hmm... yes could we please drop this commit for now? Feel free to open as an issue to follow up on this question of whether we should enforce this policy even if not in the specification. Careful, I think some updates for the Reject proposals missing original sender inputs commit got entangled in this commit.

[Jolah1]

Hmm... yes could we please drop this commit for now? Feel free to open as an issue to follow up on this question of whether we should enforce this policy even if not in the specification. Careful, I think some updates for the Reject proposals missing original sender inputs commit got entangled in this commit.

Dropped, and moved the is_ordered_subsequence test cases back into the outpoint commit.

[xstoicunicornx]

utACK 283663e

Improves a few tests by removing hacks that altered sender inputs just to get them to be treated as receiver inputs and instead just references the actual receiver inputs instead. Also improves accuracy in the error type returned when there are missing or shuffled sender inputs.

[Christopherdominic]

tACK 283663e

I traced the new check_inputs() flow and ran:

cargo test -p payjoin --lib send::test

All 40 tests passed, including the new ordered_subsequence_requires_original_order and test_sender_input_outpoint_changed tests.

The new check makes sense to me: it ensures the sender's original inputs are still present and ordered before the proposal inputs are classified, so altered sender outpoints get the appropriate MissingOrShuffledInputs error instead of being handled as receiver inputs.