Fee sanity check is triggered more than it should

[KtorZ]

Release	Operating System	Cause
1.4	Windows & OSX & Linux)	Code

Context

When solving CSL-2526, we introduced an extra sanity check to make the API fails if it was to generate a transaction with too many fees (to prevent us from creating illed transactions because of a programming error in the coin selection / fee calculation).
Yet, it turns out this check is a bit too strict / wrong for it compare transaction fees to an absolute value. Therefore, some transactions with many inputs and, actually expensive fees, fail to be created despite being totally valid.

Steps to Reproduce

1. Submit a (valid) transaction with many inputs to the API

Expected behavior

The transaction is successfully created with corresponding fees.

Actual behavior

The API fails (500 error) with "fees out of bound".

---

Resolution Plan

• Extend current test suite with a valid case of valid transaction with out-of-bound fees.
• Make the fee sanity check relative to the transaction's size itself instead of being absolute
• Backport this change to cardano-sl

PR

Number	Base
#199	develop
#207	develop
cardano-sl #3993	release/2.0.1

QA

In order to make the current test scenarios fail (and shed a light on the issue), we had to do a few things:

• Make sure the sanity check was included earlier in the coin selection algorithm such that it would occur at the very root of the selection, even if next steps are mocked by test scenarios

• Generated more fragmented UTxO to allow for many inputs to be selected (leading to high fees, out of the absolute bound). See (https://github.com/input-output-hk/cardano-wallet/pull/199/files#diff-82f6927d7e5a4cfec96683f543e2cb58R210) and @akegalj 's comment https://github.com/input-output-hk/cardano-wallet/pull/199/files#r245356862.

With this, our test suite was able to identify the issue and trigger the wrong sanity check. By then changing the sanity check to a relative one, I haven't been able to make them fail again even with 50K+ more tests.

Note that the sanity check is rather arbitrary but is now comparing the actual computed fee amount with the estimated one. Cf the corresponding note in the code:

https://github.com/input-output-hk/cardano-wallet/blob/develop/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs#L102-L114

Retrospective

Turns ou that our testing scenarios for CoinSelection weren't catching any of this but still, were making us believe they did (the fee sanity checkers were defined and required in the tests themselves but were never needed to run the tests).
This is a pretty big issue in terms of QA which may be also happening in other parts of the testing suite. We will need to invest time ASAP in reviewing our current unit tests and see exactly what are they testing and how exactly do they do it.

[piotr-iohk]

OK. Closing!