[CBR-462] Fix 0 fee transaction #3704
Conversation
…t's balance When calling ``` reduceChangeOutputs :: forall dom. CoinSelDom dom => Fee dom -> [Value dom] -> ([Value dom], Fee dom) ``` If the `[Value dom]` is a singleton array with a zero coin, we're going to end up dividing by zero and silently obliterating fee as we do it. This happens when making a transaction with no change, i.e., when the transaction's amount matches exactly the source account's balance. This commit provides a fix handling this special edge-case.
In this context, @CoinSelHardErrUtxoDepleted@ might be thrown by `pickUtxo` as we iterate over the selected change outputs which here means that we are running out of UTxOs to cover the fee. Hence, we ought to remap the error accordingly to @CoinSelHardErrCannotCoverFee@.
| if total == valueZero then | ||
| error "divyyFee: invalid set of coins, total is 0" | ||
| else | ||
| total |
There was a problem hiding this comment.
This shouldn't happen, but ... just-in-case :|
There was a problem hiding this comment.
@KtorZ the divvyFee fix looks good, but let's see if we can keep the polymorphic type signature around 😉
| -- with one 0 coin. This is artifically created during input selection when | ||
| -- the transaction's amount matches exactly the source's balance. | ||
| -- In such case, we can't really compute any ratio for fees and simply return | ||
| -- the whole fee back with the given change value. |
There was a problem hiding this comment.
I would add a final section to this comment explaining why we do that: we return the whole fee because at the end of the day "it doesn't matter": as we are doomed anyway (we have depleted our Utxo to cover the amount, no chances we have money to cover the fee, too) this will be caught "one layer up" and the whole coin selection algorithm will fail accordingly, as it should.
| -> Fee (Dom utxo) | ||
| -> CoinSelT utxo e m (SelectedUtxo (Dom utxo)) | ||
| -> CoinSelT utxo CoinSelHardErr m (SelectedUtxo (Dom utxo)) |
There was a problem hiding this comment.
This is the only thing I don't like about this PR: Edsko worked really hard to make the code completely generic, even in the error e, but here we are now specialising it to CoinSelHardErr. Is there any way we can keep it generic? It would be better, as I also don't have any clue if this would preclude along the lines the possibility to run more interesting simulations, for example.
There was a problem hiding this comment.
Not sure here. I just followed GHC complaints when started to "catchError" and "throwError". We can't keep the generic forall e here, because well, we use explicit CoinSelHardErr constructors in the function body :/
Exactly like functions around actually:
I believe this part can't really be 100% generic, unless we provide also "Generic errors"
There was a problem hiding this comment.
Right, good point. coverRemainingFee is used by senderPaysFee which is already specialised, so I guess this is fine. Alright 😉
There was a problem hiding this comment.
The need for error handling when trying to cover the fee is indeed an oversight on my part, apologies :) (More on that in #3710 though). However, I feel the change to divvy is not correct. The comment says "This is artificially created during input selection when the transaction's amount matches exactly the source's balance. In such case, we can't really compute any ratio for fees and simply return the whole fee back with the given change value." but this is not accurate. When during coin selection we have x left to cover, and we happen to have a UTxO entry available for exactly x, then indeed we may end up with a change output of precisely zero. This by itself is however not problematic, this isn't necessary an error case. It's entirely possible that we have additional UTxO entries available to cover the fee (or indeed we might just pay the fee from the output itself, if the recipient pays fees). So the comment is misleading. In a way the fix is not wrong, however; indeed, it could just be simplified to
divvyFee _ _ [] = error "divvyFee: empty list"
divvyFee f fee [a] = [(fee, a)]After all, divvyFee is meant to proportionally allocate the fee, and if there is a single entry in the list, that single entry should the entire fee.
However, I feel this is fixing the problem in the wrong place, and will break again if, for example, coin selection was modified to create multiple change outputs (for instance, for obscurity), we may in principle end up with multiple change outputs of value zero, and we'd be back with a wrong calculation.
Instead, I think divvyFee should have a precondition that none of the outputs may be zero (just like it has a precondition the list shouldn't be empty). Then when we call divvyFee we have an obligation to satisfy this precondition. This happens (as far as I can tell) in two places: once, when we divvy the fee over all the outputs of the transaction; this will be a non-empty list and the values will be non-zero, so that's fine. Then we do it once more for each output, to divvy the fee (for that particular output) over the change values (of that output). This happens in reduceChangeOutputs; this function already takes care to satisfy the precondition of divvyFee to deal with the empty list of change outputs, but it doesn't deal with the zero change case. So I think reduceChangeOutputs should look at the list of change outputs it gets, filter by non-zero, then check if the resulting list is non-empty, and if so proceed as normal, and if not, have the same special case that it already has for the empty list.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in #3704 & #3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Note that this revert a few things introduced in input-output-hk/cardano-sl#3704 & input-output-hk/cardano-sl#3672. We moved the zero-output check from divvyFee to its callers as it makes more sense. Also, with the introduction of `Maybe` in the `PickUtxo` signature, we can remove the corner-case check for empty UTxO which now correctly get caught by layers below.
Description
commit 527afc7
Author: KtorZ [email protected]
Date: Wed Oct 3 17:12:00 2018 +0200
commit 78c113e
Author: KtorZ [email protected]
Date: Wed Oct 3 17:05:48 2018 +0200
Linked issue
[CBR-462]
Type of change
Developer checklist
[ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub.Testing checklist
QA Steps
Screenshots (if available)