Fix CPFP + Cancel Regression Tests

[soosr]

Fixes #11557

[soosr]

Behavior is the same, numbers are different... I don't know what is higher than what it's worth to speed up this transaction means in this context... what is the number that is worth it, and how can we calculate it.

Ping @nopara73

[soosr]

It seems like as the percentage is getting closer to 100%, the values are closer to each other. So it might be fine.

[nopara73]

I have concerns about the changes made to the expected results in the test assertions. Modifying the expected outcomes essentially alters the nature of what is being tested. If you are confident that your code implementation is correct, it would be more appropriate to adjust the test setup rather than changing the expected results.

For example, in CancelTests.cs, the expected value for cancellingTx.FeePercentOfSent has been changed from 0 to 0.00066m. Similarly, in ReceiveSpeedupTests.cs and SelfSpendSpeedupTests.cs, the exceptions being thrown have been changed. These changes could potentially mask issues in the code being tested.

I believe it's crucial that these tests remain reliable indicators of the code's functionality.

[soosr]

For example, in CancelTests.cs, the expected value for cancellingTx.FeePercentOfSent has been changed from 0 to 0.00066m

https://github.com/zkSNACKs/WalletWasabi/blob/c2774d2af624a02796ffd6c8082ad31adbe051f5/WalletWasabi.Tests/RegressionTests/CancelTests.cs#L149-L151

• To be honest L151 Assert.Equal(0, cancellingTx.FeePercentOfSent); doesn't make much sense, moreover it indicates that there is a bug somewhere. It should not be possible that a transaction pays 0% in fees.
• L150 Assert.NotEqual(0, txToCancel.FeePercentOfSent); also questionable for the reason above, this line should always be true, if that is not the case, then there is a bug somewhere.
• L149 Assert.True(txToCancel.Fee < cancellingTx.Fee); this makes so much sense and should be enough alone.

https://github.com/zkSNACKs/WalletWasabi/blob/c2774d2af624a02796ffd6c8082ad31adbe051f5/WalletWasabi.Tests/RegressionTests/CancelTests.cs#L211-L213

• I believe the same is true here.
• txToCancel.Fee < cancellingTx.Fee condition should be true, be in percentages we expect the opposite (??)

I removed the unnecessary checks a5522db

[soosr]

Similarly, in ReceiveSpeedupTests.cs and SelfSpendSpeedupTests.cs, the exceptions being thrown have been changed. These changes could potentially mask issues in the code being tested.

Yes, you are right. I found some cases where InvalidOperationException would have been thrown, but TransactionFeeOverpaymentException would not, and also found a case when the opposite happened.

What I don't understand is why do we check AssertMaxCpfpFee after the transaction is created?
IMO it should be around TransactionFactory where the fee percentage is calculated so both check would work in symphony.

There are 3 things in my mind that we can do:

• Revert TransactionFactory - Fix fee percentage calculation for self spend #11507 and live with the issue in a edge case
• Add a flag to TransactionBuilder ignoreFeeOverpayment, so it can be ignored manually in the case of CPFP
  • So when it is ignored, AssertMaxCpfpFee method would still make sure the fee is correct.
• During TransactionBuilding based on the inputs (and their history), we should be able to tell if it is going to be a CPFP transaction, and the logic in AssertMaxCpfpFee should be moved there.
  • It is better because the logic would be in the proper place. We would detect the high CPFP fee way earlier. It would even work if someone did a manual CPFP transaction.
  • I don't have the knowledge to move the logic there, just because I don't fully understand what is happening in AssertMaxCpfpFee. So it should be done by someone else.