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 #505 (comment)

[benalleng]

BIP78 sender checklist test coverage

• Verify that the absolute fee of the payjoin proposal is equals or higher than the original PSBT.
  test_absolute_fee_less_than_original_psbt()
• If the receiver's BIP21 signalled pjos=0, disable payment output substitution.
• Verify that the transaction version, and the nLockTime are unchanged.
  test_transaction_versions_dont_match()
test_transaction_locktimes_dont_match()
• Check that the sender's inputs' sequence numbers are unchanged.
• For each input in the proposal:
  • Verify that no keypaths are in the PSBT input
    test_key_path_found_in_proposal()
  • Verify that no partial signature has been filled
    test_partial_sig_found_in_proposal()
  • If it is one of the sender's inputs:
    • Verify that input's sequence is unchanged.
      test_sender_input_sequence_number_changed()
    • 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:
    • Verify the PSBT input is finalized
      test_receiver_input_is_not_finalized()
    • Verify that non_witness_utxo or witness_utxo are filled in.
      test_receiver_input_missing_witness_info()
  • Verify that the payjoin proposal inputs all specify the same sequence value.
  • 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:
  • Verify that no keypaths are in the PSBT output
    test_process_proposal_when_output_contains_key_path()
  • If the output is the fee output:
    • 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()
    • 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()
    • 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,
    • Do not make any check
  • Else
    • Make sure the output's value did not decrease.
  • 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()
• 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 #663 (comment) (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.