Add tests for BIP-78 sender checklist

[shinghim]

Adding tests for each ensure! for the sender checks. Will partially address #339 - I'll follow up with another PR for the receiver side

[coveralls]

Pull Request Test Coverage Report for Build 15142770410

Details

• 342 of 342 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.2%) to 83.435%

---

Totals
Change from base Build 15123468832:	1.2%
Covered Lines:	5903
Relevant Lines:	7075

---

💛 - Coveralls

[benalleng]

Archived check #339 for the updated checklist

Checklist for review (These checks are not confirming that the logic in each test is sound, just organization and completeness)

• [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()
• [ ] 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,
    • [ ] Do not make any check
  • Else
    • [ ] 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()

[benalleng]

A few questions and comments

• Do we think its necessary to have some tests follow the checklist as closely as possible? test_process_proposal_when_payee_output_has_disallowed_output_substitution() as an example does not parse the pjos=0 param and instead just sets the OutputSubstitution::Disabled enum and proceeds from there, we already have tests confirming these param parsing so I think that should ultimately be ok but wanted to bring it up as to why it is not yet checked off.
• Is there anything additional we should do for a "happy path" that differs from test_official_vectors() in these tests?
• The overall proposal test for ensure sequence mismatch MixedSequence not just sender inputs is missing
• I think the output value check for disable output substitution is also missing

[benalleng]

Its rather disappointing bitcoin::psbt::Error is the only thing holding us back from being able to apply PartialEq, Eq on the InternalProposalError enum

[benalleng]

it's worth bringing up again now that the sender checks that use the ensure! macro do not get picked up by cargo mutants as explained in #539 (comment). It would be great to address this so we can get include all of these critical paths in mutation coverage now that we have proper tests in place.

I'd like to put these mutant cathces into a different PR, there are enough failed mutants that I think it's worth separating it from here

[shinghim]

Thanks @benalleng for the checklist. I incorrectly assumed that there'd be an ensure for each check in the checklist, but maybe it'd make sense to add the rest of the ensures in this PR as well

[DanGould]

The tests are a great improvement in coverage as-is. I'm not even sure the missing checks are candidates for ensure other than "Verify that the payjoin proposal inputs all specify the same sequence value."

That's not to say additional checks aren't welcome, i'd just hate for them to slow you down.

[shinghim]

Updated to reformat the assertions and add the test for MixedSequence

[benalleng]

Transitioning these tests to use just assert_eq has certainly made them nice and readable!

I would like to maybe use expect_err instead of unwrap_err just for some easier debugging when things go wrong.

[shinghim]

I would like to maybe use expect_err instead of unwrap_err just for some easier debugging when things go wrong.

I think the custom message is useful in expect_err, but in these tests, the message would always something like "Expected an error, but received a valid response" or something similar since the assert_eq is doing the comparison to check if it's actually the type of error we're expecting. I think unwrap_err is sufficient since there's not much gained from having the same custom message

[benalleng]

I think it's Ok to tag onto #339 as there is already some discussion there about these test there. As for creating a higher level [ ] has many unit tests in the readme, I think that could be helpful to keep everything like this, mutants, etc organized. And we can decide there whether it deserves to be checked

[shinghim]

@DanGould I'll do it in a follow up - created #707 to track