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)

• 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()

[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]

This is a great and I think almost there! just a few comments, and perhaps a QoL change i'd like is some doc strings that organize the tests perhaps mirror the checklist in some way by just adding the checklist string above each test that is relevant to that. Most notably the fee tests are a bit confusing in their names albeit they do what they say they will just harder to track.

[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

[spacebear21]

This is awesome! I haven't gone through every test to check the logic yet but at a high level it looks great.

@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.

[spacebear21]

Isn't this assert/panic pattern redundant? I think this is effectively the same thing as:

Suggested change

	assert!(
	result.is_err(),
	"Process proposal should reject proposal since the version is different than the original psbt"
	);
	match result {
	Ok(_) => panic!("Expected error, got success"),
	match result {
	Ok(_) => panic!("Process proposal should reject proposal since the version is different than the original psbt"),

[shinghim]

I think so. There was an existing test for one of the sender checks in this format - I kept the new tests in the same format but I'll update these to remove the redundancy

[DanGould]

Even beyond @spacebear21 's comment, this pattern may be simplified in many places with

assert_eq!(
    ctx.process_proposal(proposal).unwrap_err().to_string(),
    InternalProposalError::LockTimesDontMatch {
        proposed: proposed_locktime,
        original: original_locktime
    }.to_string()
);

Even better would be an Internal[T]Error match versus a to_string match if the Internal[T]Error is visible in the test

[DanGould]

I discovered even more appropriate syntax

            assert!(matches!(
                ctx.process_proposal(proposal),
                Err(InternalProposalError::LockTimesDontMatch {
                    proposed,
                    original
                }) if proposed == proposed_locktime && original == original_locktime
            ));

which does not match on the display impl but on the variant itself, and avoids the unwrap, but I suppose that can be saved for a follow up. An LLM should be able to do this conversion and then you can self review before asking for one here, but it should be one-shot.

[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

[DanGould]

ACK b8d7aab

I did go through the logic of each test individually, though I didn't check with @benalleng 's checklist

@benalleng , does that checklist become an issue or attach to the adversarial tests issue? Or the issue that says this can finally check [ ] has many unit tests, if one exists?

@shinghim How would you like to handle the suggested assert!(matches! syntax or the ensure! for sequence? Would you like to grab them or shall I find a time to follow up? You mentioned the receiver side too.

Thanks for keeping these piecemeal and easy to review

[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