Add tests to catch mutants#1622
Conversation
Coverage Report for CI Build 27160592459Coverage increased (+0.2%) to 85.541%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
| assert!(input.tap_key_sig.is_some(), "tap_key_sig should be preserved"); | ||
| assert!(!input.tap_script_sigs.is_empty(), "tap_script_sigs should be preserved"); | ||
| assert!(input.tap_merkle_root.is_some(), "tap_merkle_root should be preserved"); | ||
| } |
There was a problem hiding this comment.
Should we check the sensative fields are removed as well? Same thing for the outputs?
| assert!(input.sighash_type.is_some(), "sighash_type should be preserved"); | ||
| assert!(input.tap_key_sig.is_some(), "tap_key_sig should be preserved"); | ||
| assert!(!input.tap_script_sigs.is_empty(), "tap_script_sigs should be preserved"); | ||
| assert!(input.tap_merkle_root.is_some(), "tap_merkle_root should be preserved"); |
There was a problem hiding this comment.
Missing the final input field types that are preserved
final_script_sig: input.final_script_sig.clone(),
final_script_witness: input.final_script_witness.clone(),There was a problem hiding this comment.
Is it not odd that the receiver would keep anyting but these fields? Its essentially offloading the responsiblity of witness building to the sender. Motivating example: liana may fillout tap_merkle_root but the key spend is used. Here it would be preserved and your taptree strucuture leaked to the counter party
There was a problem hiding this comment.
Just to be clear you think we should be stripping
sighash_typetap_key_sigtap_script_sigstap_merkle_root
And just preserve these fields?
for input in &processed_psbt.inputs {
filtered_psbt.inputs.push(bitcoin::psbt::Input {
witness_utxo: input.witness_utxo.clone(),
non_witness_utxo: input.non_witness_utxo.clone(),
final_script_sig: input.final_script_sig.clone(),
final_script_witness: input.final_script_witness.clone(),
..Default::default()
});
}
There was a problem hiding this comment.
We have to do one or the other not both. The receiver either gives the sender the partial fields and the sender constructs witness OR the receiver provides fully constructed witness. The latter does break both bip174/370 definition of the finalizer role. i.e all the inputs should be finalizable. So its possible the rust-bitcoin ecosystem does not have support for this which is why we have the sender finalize today.
Thats why i found it odd that we preserve the final_ fields and the partial fields.
There was a problem hiding this comment.
According to The BIP 78 spec protocol section receiver checklist subsection, which BIP 77 references
The payjoin proposal MUST:
...
- Only finalize the inputs added by the receiver. (Referred later as
additional inputs)
There was a problem hiding this comment.
Additionally, in the sender checklist section it says:
For each input in the [payjoin] proposal:
...
Verify that no partial signature has been filled
So per the spec we want non_witness_utxo or witness_utxo and final_* fields.
benalleng
force-pushed
the
cargo-mutants-06-08
branch
from
d0ebd2d to
311a783
Compare
benalleng
force-pushed
the
cargo-mutants-06-08
branch
from
311a783 to
753cf51
Compare
I determined that a TODO exclusion was infact a trivial mutant and
decided to mark it for permanent exclusion. Here is a test below that
could solve it but I just beleive that the actual behavior being caught
is too minor for this to matter
fn test_avoid_uih_requires_strictly_greater() {
let original = original_from_test_vector();
let mut wants_inputs = WantsOutputs::new(original, vec![0]).commit_outputs();
let dummy_script =
ScriptBuf::from_hex("0014ffffffffffffffffffffffffffffffffffffffffff").unwrap();
let candidate_script =
ScriptBuf::from_hex("0014aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap();
let min_out = Amount::from_sat(100);
let other_out = Amount::from_sat(200);
// Set outputs so min_out_sats == prior_payment == 100
assert_eq!(wants_inputs.change_vout, 0);
wants_inputs.payjoin_psbt.unsigned_tx.output[0] =
TxOut { value: min_out, script_pubkey: dummy_script.clone() };
wants_inputs.payjoin_psbt.unsigned_tx.output[1] =
TxOut { value: other_out, script_pubkey: dummy_script };
// Candidate with value == min_out hits the boundary condition:
// candidate_min_in = min(min_in_sats, 100) = 100 (existing inputs are >> 100)
// candidate_min_out = min(100, 100 + 100) = 100
// With ">": 100 > 100 → false → NotFound
// With ">=": 100 >= 100 → true → incorrectly accepts
let candidate_txout = TxOut { value: min_out, script_pubkey: candidate_script };
let candidate = InputPair::new(
TxIn {
previous_output: OutPoint::default(),
sequence: Sequence::MAX,
..Default::default()
},
Input { witness_utxo: Some(candidate_txout), ..Default::default() },
None,
)
.unwrap();
let result = wants_inputs.avoid_uih([candidate]);
assert_eq!(
result.unwrap_err(),
CoinSelectionError::from(InternalCoinSelectionError::NotFound),
);
}
benalleng
force-pushed
the
cargo-mutants-06-08
branch
from
753cf51 to
8e8f00c
Compare
4 participants
Closes #1616
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.