Add adverserial tests

[spacebear21]

The official BIP78 test vector checks the happy path where both the sender and receiver act honestly. To catch and prevent bugs that may result in loss of funds, we should add tests that uphold the sender/receiver checklists for when one of the parties behaves maliciously, e.g. tries to steal funds from the other. One such test was added here but it would be great to cover more (all?) code paths in the checklists.

If we can come up with good test vectors for these, maybe consider adding them to the BIP.

[DanGould]

every single ensure! in basic_checks, check_inputs, check_outputs and check_fees, should have a corresponding unit test

— @spacebear21 https://github.com/payjoin/rust-payjoin/pull/505#discussion_r1927082146

[benalleng]

BIP78 sender checklist test coverage

• [x] Verify that the absolute fee of the payjoin proposal is equals or higher than the original PSBT. test_absolute_fee_less_than_original_psbt()
• [x] If the receiver's BIP21 signalled pjos=0, disable payment output substitution.
• [x] Verify that the transaction version, and the nLockTime are unchanged. test_transaction_versions_dont_match() test_transaction_locktimes_dont_match()
• [x] Check that the sender's inputs' sequence numbers are unchanged.
• For each input in the proposal:
  • [x] Verify that no keypaths are in the PSBT input test_key_path_found_in_proposal()
  • [x] Verify that no partial signature has been filled test_partial_sig_found_in_proposal()
  • If it is one of the sender's inputs:
    • [x] Verify that input's sequence is unchanged. test_sender_input_sequence_number_changed()
    • [x] Verify the PSBT input is not finalized test_sender_input_final_script_sig_is_present() test_sender_input_final_script_witness_is_present()
  • If it is one of the receiver's inputs:
    • [x] Verify the PSBT input is finalized test_receiver_input_is_not_finalized()
    • [x] Verify that non_witness_utxo or witness_utxo are filled in. test_receiver_input_missing_witness_info()
  • [x] Verify that the payjoin proposal inputs all specify the same sequence value.
  • [x] Verify that all of sender's inputs from the original PSBT are in the proposal. test_process_proposal_when_missing_original_inputs()
• For each output in the proposal:
  • [x] Verify that no keypaths are in the PSBT output test_process_proposal_when_output_contains_key_path()
  • If the output is the fee output:
    • [x] The amount that was subtracted from the output's value is less than or equal to maxadditionalfeecontribution. Let's call this amount actual contribution. test_receiver_steals_sender_change()
    • [x] Make sure the actual contribution is only going towards fees: The actual contribution is less than or equals to the difference of absolute fee between the payjoin proposal and the original PSBT. test_payee_took_contributed_fee()
    • [x] Make sure the actual contribution is only paying for fees incurred by additional inputs: actual contribution is less than or equal to originalPSBTFeeRate * vsize(sender_input_type) * (count(payjoin_proposal_inputs) - count(original_psbt_inputs)). test_fee_contribution_pays_output_size_increase()
  • If the output is the payment output and payment output substitution is allowed,
    • [x] Do not make any check
  • Else
    • [x] Make sure the output's value did not decrease.
  • [x] Verify that all sender's outputs (ie, all outputs except the output actually paid to the receiver) from the original PSBT are in the proposal. test_process_proposal_when_output_missing()
• [x] Once the proposal is signed, if minfeerate was specified, check that the fee rate of the payjoin transaction is not less than this value. test_fee_rate_below_minimum()

sourced from https://github.com/payjoin/rust-payjoin/pull/663#issuecomment-2842147013 (I am going to archive the original comment so I don't need to update it everywhere)

[benalleng]

Are we satisfied that test_process_proposal_when_payee_output_has_disallowed_output_substitution() is sufficient in its completeness even though it doesn't look exactly for a uri param and simply assumes pjos=0 works as expected. I would say its ok just because I know we have tests that ensure our pjos param is working correctly https://github.com/payjoin/rust-payjoin/blob/master/payjoin/src/send/mod.rs#L910-L926

[spacebear21]

Are we satisfied that test_process_proposal_when_payee_output_has_disallowed_output_substitution() is sufficient in its completeness

It looks complete to me (and also potentially duplicated by test_output_substitution?).

Have the other open items in the checklist been addressed @benalleng ?

[benalleng]

test_output_substitution ensures some additional mutants do not occur, I merged them both together into the existing test_process_proposal_when_payee_output_has_disallowed_output_substitution() test and added some comments to clarify what is occuring.

[spacebear21]

I believe all checklist items are now covered by our tests 💪 Closing this issue.

[DanGould]

The checklist appears to be just for the sender. Are you satisfied that the receiver is sufficiently covered by adversarial tests @spacebear21

[spacebear21]

The receiver checklist from BIP78 is comparatively barebones:

• Non-interactive receivers (like a payment processor) need to check that the original PSBT is broadcastable. *
• If the sender included inputs in the original PSBT owned by the receiver, the receiver must either return error original-psbt-rejected or make sure they do not sign those inputs in the payjoin proposal.
• Make sure that the inputs included in the original transaction have never been seen before.
  • This prevents probing attacks.
  • This prevents reentrant payjoin, where a sender attempts to use payjoin transaction as a new original transaction for a new payjoin.

These checks are enforced by the receiver state machine and their "correctness" is in the hands of the implementation, so I don't think unit tests would add much value re: those checklist items.

Outside of the BIP78 checklists there may be other "adverserial" scenarios we'd like to test, but this issue title is too open-ended so IMO better to scope it just for the checklists mentioned in OP, and open more specific new issues if necessary.