Skip check_payment in Receiver<Monitor> if the sender is using non-segwit address

[mehmetefeumit]

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

Overview

The previous implementation of the check_payment function assumed that if the outpoints of the Payjoin transaction have been removed from the UTXO set, it is an indication of the Payjoin being broadcasted by the sender. Moreover, it relied on the same closure to detect whether there is a double-spend attempt from the sender, if only a subset of the outpoints have been spent.

Both of the usages of the outpoint closure is incorrect. If the sender does RBF on the fallback transaction, this would change the transaction ID and cause the previous implementation to incorrectly detect double-spend. Moreover, the sender can use some of the UTXOs in the Payjoin session if they wish without necessarily "attacking" the receiver.

This change removes the outpoint closure, and instead skips the check if
the fallback transaction has any inputs which does not have witness
data. This assumes that the fallback transaction has been signed which
is a certainty at this point in the Payjoin session.

Closes #1214.

On the side: this PR also contains another small commit for completing a TODO in the unit tests.

List of Changes

• Remove the outpoint check closure from the check_payment function and add skip if the fallback transaction is non-segwit.
• Add a new receiver SessionOutcome value to indicate the skip.
• Change the consumers of the function (i.e. cli, ffi) to adhere to the new signature.
• Add unit tests.

Testing

• Added new unit tests with P2PKH PSBTs for testing this specific scenario.

Next Step

#1211 contains a documentation update for the check_payment function. Will need to update that if and when this PR gets approved and merged, and also make sure that there are integration tests for covering p2pkh as well.

[coveralls]

Pull Request Test Coverage Report for Build 20173359100

Details

• 46 of 48 (95.83%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 83.307%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	5	7	71.43%

Totals
Change from base Build 20148625592:	0.1%
Covered Lines:	9657
Relevant Lines:	11592

---

💛 - Coveralls

[arminsabouri]

ConceptAck. This fixes the immediate bug in our monitoring type state.
I am assuming #1211 shold probably preceed this PR?

[mehmetefeumit]

ConceptAck. This fixes the immediate bug in our monitoring type state.
I am assuming #1211 shold probably preceed this PR?

Yeah. Going to add new test scenarios for non-SegWit to this PR once the integration test changes are merged.

[arminsabouri]

cACK.
Just one comment about copy displayed in the history subcommand

[arminsabouri]

utACK fb368c0

[DanGould]

Has there been any discussion about not allowing this state

ReceiverSessionOutcome::PayjoinProposalSent =>
"Payjoin proposal sent, skipping monitoring as the sender is spending non-SegWit inputs"`

And instead requiring that clients that support non-SegWit inputs instead rely on an address index like esplora, allowing the state machine and clients to be simplified. I understand that this would limit our reference implementation, but the index is a sort of prerequisite for any full on wallet that we've integrated with in the past.

[mehmetefeumit]

We did briefly talk about how since not all clients have an address index, we could not reliably change this into something which would provide consistent experience. If I got it right, your suggestion is to only remove the new state, and change the documentation, etc. around the state to reference an address index usage to solve the non-SegWit problem.

That makes sense to me. Trying to wrap my head around how it looks like in two different cases:

1. In the case of the reference implementation in the CLI (or any other RPC usages), the monitor function would simply return a Success if the original proposal had non-SegWit inputs, and otherwise continue monitoring the network.
2. In the case of clients with address index, I need to understand whether the closure API would be consistent among different clients, and if not, what is the signature for those closure parameters to allow different clients to do the same thing.

For the second point, let me know if you have any information! Otherwise I understand that real world usages will have address index capabilities to make this possible. Let me create an issue for it to move the discussion there.

[DanGould]

1. In the case of the reference implementation in the CLI (or any other RPC usages), the monitor function would simply return a Success if the original proposal had non-SegWit inputs, and otherwise continue monitoring the network.

I think we can simplify by disallowing non-SegWit inputs in the reference implmentation.

2. In the case of clients with address index, I need to understand whether the closure API would be consistent among different clients, and if not, what is the signature for those closure parameters to allow different clients to do the same thing.

I think the closure would change to be |address| instead of |txid|, in the case of an address index, or |outpoint| or |script| with an outpoint or script index. More research needs to be done to enumerate the indexes wallets and services are relying on.