[CSL-2526] update changes after covering remaining fees

[kderme]

Description

This pr tries to fix a bug on coin and fee selection, which was reported by QA.
The bug was caused because in some cases some changes were not included in the transaction, resulting in huge fees.

The CoinSelection works in many different stages:

• first given some payees, some inputs are selected which cover the expenses (fees are ignored for now). Changes are computed at this stage.
• then fees are computed and we reduce changes to cover the fees. If they are enough, we are done.
• if not new inputs are selected.

The problem is that these additional inputs (last step) which cover the fees, may not be entirely consumed for the fees: we need to return what's left as change.

TODO:

• Decide exactly how these changes will be returned: Do we create new additional change outputs or do we distribute them to the existing changes? If we decide the latter, do we need to equally distribute the additional change to all existing one, or just give everything to one of them?
  UPDATE: we split additional changes to existing change outputs. We do this in FromGeneric module (instead of the Generic or Fees) for 2 reason: in this module change outputs are flatten to a list (instead of list of lists) and also it is less polymorphic, which means we can directly use divide function from Core.
• Another issue is the additional fees that we may need to pay for these additional utxos and changes. This issue becomes too complex.
• Add a sanity check after the coin selection, which reports an internal error if fees are not within a reasonable range.
• check round vs ceiling in the use of calculateTxSizeLinear.
• add unit tests

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CSL-2526

QA Steps

Here are some steps to reproduce the bug with tome good probabilities:

• Create a wallet.
• send 1 tx with amount > 1 ADA to it.
• send 2 txs with ammount in [100..1000] Lovelace to it.
• try to send < 100 Lovelace from it.

After trying the last step with different amounts and addresses, at some point the bug will appear and fees will be > 1 ADA.

The bug appears in these cases because the inputs in [100..1000] are enough to cover the <100 tx, but not enough to cover the fees. So an additional input needs to be selected. If we are "lucky" this input will be the > 1 ADA in which case we will have huge fees (without the fix).

Type of change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• 🛠 New feature (non-breaking change which adds functionality)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
• 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
• 🔨 New or improved tests for existing code
• ⛑ git-flow chore (backport, hotfix, etc)

Screenshots (if available)

before

after

[KtorZ]

typo here I believe returnAdditioncalChange -> returnAdditionalChange

[KtorZ]

To be honest, I'd rather add that logic to runCoinSelT from Cardano.Wallet.Kernel.CoinSelection.FromGeneric.

When we construct the final result:

        let css  = map (coinSelRemoveDust (csoDustThreshold opts)) cssWithDust
            inps = concatMap selectedEntries
                     (additionalUtxo : map coinSelInputs css)
            outs = map coinSelOutput css
        let allInps = case inps of
                        []   -> error "runCoinSelT: empty list of inputs"
                        i:is -> i :| is
            originalOuts = case outs of
                               []   -> error "runCoinSelT: empty list of outputs"
                               o:os -> o :| os
        return . Right $ CoinSelFinalResult allInps
                                            originalOuts
                                            (concatMap coinSelChange css)

This would avoid fiddling with CoinSelResult here and "artificially" add the change. To my understanding, we've separated additional inputs yielded by the fee policy for a reason; We have conflated them with the CoinSelResult inputs already but we didn't. Instead, we reconcialte the CoinSelFinalResult later and therefore, I see this as a similar case.

[kderme]

Should I create an additional change output just for these additional UTXO's instead of just adding it to the existing one? In this case should I also check if this new change is dust? Not sure 🤔

[KtorZ]

Cf @edsko inputs I believe. Let's distribute the extra change across all changes.

[KtorZ]

The change looks good; I simply think it's not applied to the right place.
Also, let's have some hard invariant check after computing the fees, to make sure that we aren't constructing a transaction with crazy fees.

[tatyanavych]

We need a fix on release/2.0.0 branch ASAP

[commandodev]

Are we planning to add a unit or property tests to exercise this bug and prove that it's fixed? I can't really see from the code changes here what the specific logic error was or that it has been fixed.

[KtorZ]

Yes we are. And this is why this PR is still blocked and under review.
@tatyanavych We'll provide a fix but we'll provide a proper one which includes:

• guards to prevent this from happening again
• an acceptance test
• a valid patch that fits correctly with the existing code and abstraction

We aren't going to ship something to fix it again later 👍

[edsko]

LGTM, nice catch, and I like the logic of the solution. One concern: by modifying a single change output with the remaining change, if that remaining change is very large, it becomes easily identifiable as the change output. Might wish to consider divvy to distribute it evenly over all change outputs.

[edsko]

Very minor concern: possibility of overflow when adding change to existing outputs.

[kderme]

Sanity check breaks unit tests :/

[KtorZ]

Some minor comments and some concerns about the hardcoded min fee.
This is a chain parameter that can in theory, change with chain updates. The value should come from the chain as much as possible.

[KtorZ]

The error would be a bit misleading here and I wouldn't define any particular new user-facing error either; this is an invariant violation. If we reach that case, it means we fucked up hard and everything should blow up!

[KtorZ]

Worth extending the comment here explaining what that check is all about.

[KtorZ]

This can (ought to) be extracted from the NodeStateAdaptor:

      -- | Get fee policy
    , getFeePolicy :: m TxFeePolicy

[KtorZ]

LGTM 👍