Allow mixed input scripts in v2 #367

spacebear21 · 2024-10-16T04:33:31Z

Closes #39 and #358

Adds support for mixed input script types in payjoin v2, and disables that option in v1.

Additionally, renames contribute_witness_inputs to contribute_inputs and changes the function signature to better support non-witness and nested segwit inputs.

DanGould · 2024-10-16T15:09:30Z

Coming in hot with the double whammy 🔥

Is the whole issue blocked by taproot? It doesn't seem to me that taproot mixed input logic would be different than other AddressTypes, only that it would affect weight calculation which I beleive you've already written unit tests for.

bitcoind is quite old now and we should update but perhaps that can be a less urgent issue of its own

spacebear21 · 2024-10-16T15:24:49Z

I wouldn't say it's a blocker for this issue but I'd like to follow up on it eventually. It looks like bitcoind might be on its way out so I'm fine with addressing this as a separate issue. I think I'll leave the commented out taproot test in as a reminder though.

This PR should be in a reviewable state by EOD.

DanGould

This is one of the most exemplary PRs I've ever had the pleasure of reviewing. Excellent commit messages, clean code, and it's clear you went above and beyond to take care of making these commits individual works of digital art. Kudos for setting the standard. Honored to have the privilege of teaming up.

The one significant change that pops out to me is to skip the checked_no_mixed_input_scripts check in the v2 state machine instead of adding a v2-specific condition. I left a question or two and then some minor comments to clean things up further. Big thanks for fixing docstrings and redundant code in that last commit.

I think your recent PRs are getting us ready to consider the "Many unit tests" goal in the readme as completed.

payjoin/tests/integration.rs

payjoin/src/receive/mod.rs

payjoin/tests/integration.rs

payjoin/src/receive/mod.rs

payjoin/tests/integration.rs

DanGould · 2024-10-17T03:49:45Z

payjoin/src/receive/mod.rs

+        // Allow mixed input scripts in payjoin v2
+        if self.params.v == 2 {
+            return Ok(MaybeInputsSeen { psbt: self.psbt, params: self.params });
+        }
+


I'd rather receive::v2::MaybeInputsOwned::check_inputs_not_owned return MaybeInputsSeen, skipping check_no_mixed_input_scripts in the v2 state machine. We get the same logical result with a code deletion instead of an addition, and don't end up with a sometimes-contradictory function name.

I like that. More code deletes!

I think the initial suggestion here may be misguided because v2 still needs to handle v1 backwards compatibility. Leaving the state machine as-is, but moving the self.params.v == 2 conditional to the v2 state machine leaves the opportunity to handle a Mixed Inputs check for v2 without breaking the layer abstraction between v1 and v2 state machines.

Reading through the BIP78 receiver checklist again, a receiver should only return an error if all sender inputs are the same type and the receiver can't match that script type. It shouldn't error if there are mixed input types.

If the sender's inputs are all from the same scriptPubKey type, the receiver must match the same type. If the receiver can't match the type, they must return error unavailable.

It seems to me the MaybeMixedInputScripts typestate goes against the spec and should be removed. Instead v1 receivers should check within contribute_inputs whether the sender inputs are all the same type and only error if the selected inputs don't match that type. v2 receivers may skip this check.

sounds good to me

Worth noting that the BIP78 sender checklist is consistent with your finding

Verify that the payjoin proposal did not introduce mixed input's type.

emphasis my own

Previously an "input" consisted of a (OutPoint, TxOut) tuple, which limits the caller to only contributing witness inputs. Instead, redefine an input as a (psbt::Input, TxIn) tuple, which is more flexible as it allows for any necessary information to be included in the PSBT input. Two PSBT input fields we are particularly interested in are `non_witness_utxo` and `redeem_script`, which enable legacy inputs and nested segwit (p2wpkh-in-p2sh) respectively. `contribute_witness_inputs` is renamed to `contribute_inputs`.

To determine whether an input script uses nested segwit (p2wpkh-in-p2sh), we need the redeem script that unlocks it. This can be obtained from either the input signature (if the input is signed, e.g. the sender inputs ), or the redeem_script field on the PSBT input which can be populated by the wallet that owns it (e.g. the receiver inputs). When making weight predictions for a P2SH input, we now check the final_script_sig first and fallback to the redeem_script if there is no signature. Additionally, the order of operations in `finalize_proposal` is updated such that we don't clear the sender's signatures until after the weight predictions/fee estimations are completed.

Add an integration test for each supported input script type.

The payjoin v1 spec disallows mixed input script types. Add a test that validates this behavior.

Add support for mixed input script types, which are allowed in the payjoin v2 spec and enable more batching use cases.

The BIP78 spec says: "If the sender's inputs are all from the same scriptPubKey type, the receiver must match the same type. If the receiver can't match the type, they must return error unavailable." Instead of rejecting an original PSBT that contains mixed input scripts types, we should be checking if all input script types are the same and only error if the receiver input contributions would *introduce* mixed script types. This check is now done in `contribute_inputs` (for v1 receivers only) and the previous mixed input scripts check is removed.

spacebear21 · 2024-10-18T20:50:36Z

payjoin/tests/integration.rs

+            let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+                handle_v1_pj_request(req, headers, &receiver, None, None, None)
+            }));
+            assert!(result.is_err());


handle_v1_pj_request unwraps all errors and it seemed non-trivial to make the actual error bubble up instead. This is a workaround to catch the panic and ensure some error occurred.

All the panics in the integration test are annoying, but the fix is probably simpler than you thought. Most of those errors can be boxed and bubbled up without a custom error type as Result<_, Box<dyn std::error::Error + 'static> and the ones that can't can be addressed with map_err.

Returning a (bool, value) tuple is a code smell vs returning an Option. CLean up the check with a helper function for clarity.

Avoid unwrap() by returning a Result<_, BoxError>

DanGould · 2024-10-19T16:39:33Z

payjoin/src/receive/mod.rs

+            payjoin_psbt
+                .unsigned_tx
+                .input
+                .insert(index, TxIn { sequence: original_sequence, ..txin });


Copying the original_sequence rather than using what's supplied in txin might be unexpected behavior.

Ideally BIP 77 should allow mixed sequence in the same way mixed script types are allowed, leaving it up to a client decision. I wonder what @nothingmuch thinks. It seems proper to allow mixed input and mixed sequence in fragment paremeters for v2, but also a completely unnecessary complexity for 80%+ use cases, which I imagine should be permissive rather than restrictive.

DanGould · 2024-10-21T01:18:59Z

Probably good to get your review on my two changes @spacebear21 but I think this is good to go. Sequence is a distinct consideration and this PR doesn't change the original behavior that I brought into question

spacebear21 · 2024-10-21T14:56:12Z

Thanks for making those updates! Reviewed and merged.

spacebear21 force-pushed the allow-mixed-input-scripts branch from aab2ccd to d1c9c09 Compare October 16, 2024 18:55

spacebear21 mentioned this pull request Oct 16, 2024

Upgrade or replace bitcoind in integration tests #369

Open

spacebear21 marked this pull request as ready for review October 16, 2024 19:00

spacebear21 requested a review from DanGould October 16, 2024 19:01

DanGould reviewed Oct 17, 2024

View reviewed changes

spacebear21 added 7 commits October 18, 2024 11:44

Test each input script type

47400ac

Add an integration test for each supported input script type.

Test mixed input scripts in v1

18c37cd

The payjoin v1 spec disallows mixed input script types. Add a test that validates this behavior.

Allow mixed input scripts in payjoin v2

ffa6297

Add support for mixed input script types, which are allowed in the payjoin v2 spec and enable more batching use cases.

Cleanup comments and fix doc links

38eab07

spacebear21 force-pushed the allow-mixed-input-scripts branch from d1c9c09 to 0af90e6 Compare October 18, 2024 20:47

spacebear21 commented Oct 18, 2024

View reviewed changes

Check uniform_sender_input_type as an Option

b86b639

Returning a (bool, value) tuple is a code smell vs returning an Option. CLean up the check with a helper function for clarity.

DanGould force-pushed the allow-mixed-input-scripts branch from e07f10f to 065e92b Compare October 19, 2024 16:30

Bubble up many integration.rs errors

065e92b

Avoid unwrap() by returning a Result<_, BoxError>

DanGould reviewed Oct 19, 2024

View reviewed changes

DanGould approved these changes Oct 21, 2024

View reviewed changes

spacebear21 merged commit 9ccc274 into payjoin:master Oct 21, 2024
4 checks passed

spacebear21 mentioned this pull request Oct 21, 2024

Rethink fee estimation to ensure accuracy and prevent overspending #358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mixed input scripts in v2 #367

Allow mixed input scripts in v2 #367

spacebear21 commented Oct 16, 2024 •

edited

Loading

DanGould commented Oct 16, 2024

spacebear21 commented Oct 16, 2024

DanGould left a comment

DanGould Oct 17, 2024

spacebear21 Oct 17, 2024

DanGould Oct 17, 2024

spacebear21 Oct 18, 2024 •

edited

Loading

DanGould Oct 18, 2024

DanGould Oct 18, 2024

spacebear21 Oct 18, 2024

DanGould Oct 19, 2024

DanGould Oct 19, 2024

DanGould commented Oct 21, 2024

spacebear21 commented Oct 21, 2024

Allow mixed input scripts in v2 #367

Allow mixed input scripts in v2 #367

Conversation

spacebear21 commented Oct 16, 2024 • edited Loading

DanGould commented Oct 16, 2024

spacebear21 commented Oct 16, 2024

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacebear21 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould commented Oct 21, 2024

spacebear21 commented Oct 21, 2024

spacebear21 commented Oct 16, 2024 •

edited

Loading

spacebear21 Oct 18, 2024 •

edited

Loading