Support payjoin PSBT with multiple sender inputs

[spacebear21]

Compatibility testing with PDK revealed that the JoinMarket payjoin receiver doesn't support signing for PSBTs that contain multiple sender inputs. This patch fixes that.

[DanGould]

This is important because senders often spend more than one UTXO in order to pay for the requested amount.

Separately, I ask any reviewer to consider whether the assumption that a payjoin sender only supplies one input affects any other part of JoinMarket payjoin support like fee calculation. I doubt that it does, but it might.

[kristapsk]

Would be good to add also test to test/jmclient/test_payjoin.py.

[spacebear21]

There is actually already a test for this case, but the sender is skipping some checks from the BIP78 checklist that would catch this issue (specifically, "If it is one of the sender's input, Verify that non_witness_utxo and witness_utxo are not specified).

In fact it manually re-adds the utxo data prior to performing the checks (L621). I added an assert right before this step, which fails without this patch and passes with. I believe this check should be in check_receiver_proposal, but as mentioned the sender manually re-adds the utxo data prior to these checks, so I'm not really sure about the correct way forward.

[kristapsk]

ACK 67ba0f5. This solution seems good enough to me.

[kristapsk]

@AdamISZ Any comments on this?

[AdamISZ]

First, thanks @grizznaut for looking at it! My best attempt at remembering (obviously it was years ago and I don't) is it was a case of "I'll do the simple version now and change the code later" and then failed to note it down and actually fix it later (I obviously should have put a TODO in this case).

I'll spend some time probably tomorrow trying to check the logic as I understand it.

[AdamISZ]

OK, after an initial review, I think this is the situation:

Joinmarket does "support" such payjoins (multiple sender inputs), but doesn't do so according to BIP78 spec. So for interoperability tests you would definitely see an error if the other implementation is following the spec fully. More specifically:

The line

https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1700/files#diff-716a25cd19c63997f8abcf020e7a08356b79c08bb953068b375b9434c30c175aL906

serves the role of simply overwriting an existing PSBTInput object in the PSBT with a new, blank one. This is to support the requirement of BIP78 that "... non_witness_utxo and witness_utxo are not specified. " (see "Sender's payjoin proposal checklist")

So this PR is definitely correct from that point of view, it is a simple change that makes sure that that "blanking out" is occurring for all the sender's inputs and not only one.

(note that test_payjoin does test a multi-sender-input case and it passes, because JM's code accepts these PSBT inputs; something I can address myself; note that the detailed checklist here ignores this point; probably a side-effect of the fact that I didn't see why this psbt input modification was necessary...), although best is to substantially change and improve the tests for payjoin of course)

[spacebear21]

@AdamISZ Thanks for the thorough review! Want me to remove that assert 67ba0f5 then? I mainly put it there to highlight the check that should happen somewhere, likely in that checklist you linked to.

[AdamISZ]

@AdamISZ Thanks for the thorough review! Want me to remove that assert 67ba0f5 then? I mainly put it there to highlight the check that should happen somewhere, likely in that checklist you linked to.

Ah thanks, yes. Since it is part of that set of checks, it should instead be in this section, logically (keeping the same response format etc.). And to reaffirm, yes, I agree you are of course correct to add that check.

There is already another check (final status of PSBT) that's also an assert in that same function (I think that was me thinking "this would be a logical/coding error not a usage pattern error so it's appropriate to just assert", but I'm not sure if I was really right about that).

Edit: also your code tests fine for me, unsurprisingly.

[spacebear21]

OK, makes sense and agreed. I removed that assert.

[AdamISZ]

tACK 865247c

[kristapsk]

re-ACK 865247c