[CBR-464] Raise Correct `CoinSelHardErr` during coin selection #3710

KtorZ · 2018-10-04T14:31:39Z

Description

In a previous PR (#3704 - CBR-462), we patched the coin selection and remap the UTxODepleted error to a CannotCoverFee error.
From this, the frontend team discovered a new bug caused because again, UTxODepleted was thrown instead of a correct UTxOExhausted.

Instead of patching again and re-mapping the error to the correct one, I took a step back and slightly reviewed the original abstraction such that we would now have:

type PickUtxo m = Core.Coin  -- ^ Fee to still cover
               -> CoinSelT Core.Utxo CoinSelHardErr m (Maybe (Core.TxIn, Core.TxOutAux))

instead of

type PickUtxo m = Core.Coin  -- ^ Fee to still cover
               -> CoinSelT Core.Utxo CoinSelHardErr m (Core.TxIn, Core.TxOutAux)

The rational here is that, the outcome may depend of the context. We use this function in
multiple places where running out of UTxO may have a different meaning. Therefore, instead
of introducing the 'UtxoDepleted' error, we return a raw Maybe and let the caller decides
what error should be thrown (here, most likely: CannotCoverFee or UtxoExhausted)

Note that in theory, we could simply have the following to make it obvious that PickUtxo can't fail:

type PickUtxo m = Core.Coin  -- ^ Fee to still cover
               -> StrictStateT Core.Utxo m (Maybe (Core.TxIn, Core.TxOutAux))

-- | Hoist (m ~> ExceptT e m) to handle non failing implementation
hoistExceptT :: Functor m => StrictStateT utxo m a -> CoinSelT utxo e m a
hoistExceptT = CoinSelT . mapStrictStateT (ExceptT . fmap Right)

However, this exposes the StrictStateT from CoinSelT which is opaque.. So, we could go even further and have exposed instead.

newtype NonFailingCoinSelT utxo m a :: NonFailingCoinSelT (StrictStateT utxo m a)
  deriving ( Functor
           , Applicative
           , Monad
           , MonadState utxo
           )

Linked issue

[CBR-464]

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)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
~~CHANGELOG entry has been added and is linked to the correct PR on GitHub.~~

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

This makes it available for other tests within this context.

…utput' The rational here is that, the outcome may depend of the context. We use this function in multiple places where running out of UTxO may have a different meaning. Therefore, instead of introducing the 'UtxoDepleted' error, we return a raw Maybe and let the caller decides what error should be thrown (here, most likely: CannotCoverFee or UtxoExhausted)

KtorZ · 2018-10-04T14:37:11Z

fails if you don't have any money 
    ClientWalletError (NotEnoughMoney (ErrAvailableBalanceIsInsufficient 0))
    +++ OK, passed 1 tests.

fails if you spend more money than your available balance
    ClientWalletError (NotEnoughMoney (ErrAvailableBalanceIsInsufficient 42))
    +++ OK, passed 1 tests.

fails if you can't cover fee with a transaction
    ClientWalletError (NotEnoughMoney ErrCannotCoverFee)
    +++ OK, passed 1 tests.

adinapoli-iohk

On the surface it LGTM, albeit I would be interested to hear with @edsko thinks as he might recall some edge case in the coin selection and tell us if our changes are sound or not. He should be back next week, so that's worth the wait 😉

edsko · 2018-10-09T07:47:09Z

wallet-new/src/Cardano/Wallet/API/V1/ReifyWalletError.hs

@@ -267,15 +268,13 @@ newTransactionError e = case e of
                V1.TooBigTransaction

          ex@(CoinSelHardErrUtxoExhausted balance _payment) ->
-              case (readMaybe $ T.unpack balance) of
+              -- NOTE balance & payment are "prettified" coins representation (e.g. "42 coin(s)")
+              case (readMaybe $ T.unpack $ T.dropWhileEnd (not . C.isDigit) balance) of


This parsing is very unfortunate. I think the nicer solution is to add a valueToInt :: v -> Int (or Word64 or whatever) and then replace the Text in CoinSelHardErr by Int (or Word64). That was never meant to be parsed.

That's what I understood from this indeed. Any arguments about having Coin here instead of Int since it conveys more meaning. I don't see any reasons why we would do an early conversion when throwing the error.

Coin is not applicable, because that is specific to the Cardano domain. Other domains will not use Coin for their representation of values.

Ho true, forgot that the errors were generics here 👍

edsko · 2018-10-09T07:49:10Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

@@ -239,7 +239,7 @@ repack (txIn, aux) = (Core.toaOut aux, txIn)

 -- | Pick an element from the UTxO to cover any remaining fee
 type PickUtxo m = Core.Coin  -- ^ Fee to still cover
-               -> CoinSelT Core.Utxo CoinSelHardErr m (Core.TxIn, Core.TxOutAux)
+               -> CoinSelT Core.Utxo CoinSelHardErr m (Maybe (Core.TxIn, Core.TxOutAux))


I like this Maybe here, I think it makes perfect sense. I think we can express easily that this function should never return an error by changing the signature to

type PickUtxo m = forall e. Core.Coin -- ^ Fee to still cover -> CoinSelT Core.Utxo e m (Maybe (Core.TxIn, Core.TxOutAux))

without having to expose the underlying implementation.

edsko · 2018-10-09T07:49:44Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

@@ -288,7 +288,6 @@ runCoinSelT opts pickUtxo policy request utxo = do
    policy' :: CoinSelT Core.Utxo CoinSelHardErr m
                 ([CoinSelResult Cardano], SelectedUtxo Cardano)
    policy' = do
-        when (Map.null utxo) $ throwError CoinSelHardErrUtxoDepleted


Great, I didn't like this special case and didn't understand why it was necessary :)

Me neither 🙃 ... Hence the need for this PR.
The previous patches felt a bit like "monkey-patching" without really tackling the actual issue.

edsko · 2018-10-09T07:52:54Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

@@ -387,10 +386,10 @@ largestFirst opts maxInps =
    pickUtxo val = search . Map.toList =<< get
      where
        search :: [(Core.TxIn, Core.TxOutAux)]
-               -> CoinSelT Core.Utxo CoinSelHardErr m (Core.TxIn, Core.TxOutAux)
+               -> CoinSelT Core.Utxo CoinSelHardErr m (Maybe (Core.TxIn, Core.TxOutAux))
        search [] = throwError CoinSelHardErrCannotCoverFee


Shouldn't this return Nothing here?

edsko · 2018-10-09T07:53:54Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs

-      inRange maxNumInputs (target privacyMode (outVal goal))
+random privacyMode initMaxNumInputs goals = do
+    balance <- gets utxoBalance
+    when (balance == valueZero) $ throwError (errUtxoExhausted balance)


Why do we need this special case here? Surely if we reach a zero balance after, say, the first output, we'd have to deal with that anyway?

You are correct. This isn't needed anymore with the introduction of the Maybe for findRandomUTxO and this get caught correctly by levels below 👍

edsko · 2018-10-09T07:59:48Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs

@@ -98,18 +111,27 @@ atLeastNoFallback :: forall utxo m. (PickFromUtxo utxo, MonadRandom m)
                  => Word64
                  -> Value (Dom utxo)
                  -> CoinSelT utxo CoinSelErr m (SelectedUtxo (Dom utxo))
-atLeastNoFallback maxNumInputs targetMin = go emptySelection
+atLeastNoFallback maxNumInputs targetMin = do
+    utxo <- get


I think this is correct, in the sense that it's good that we first get the initial UTxO, and use that when reporting the balance to the user; however, I'd feel a little happier if we just did initBalance <- utxoBalance <$> utxo here and then thread that initBalance around, because we don't need the entire UTxO (and this opens the door to misunderstandings later). I'm also somewhat worried that we don't report this initial balance correctly in other places, but that might not be justified.

Incidentally, @adinapoli-iohk tells me that for debugging the "balance at the point of failure" was actually a useful bit of information for him; that is now no longer available. Don't know how much it's worth trying to recover that.

KtorZ · 2018-10-09T11:50:00Z

@edsko @adinapoli-iohk

Addressed most comments you had on this PR and the two related others. Divvy fees now just throws if the second invariant is broken (a zero output is given) and enforced by the callers.
I now pass around the utxo's balance instead of the complete utxo to construct a more meaningful error.
Tweaked a bit the type for PickUtxo to make it clear that it cannot throw (with an appropriate note).
Opened a ticket to fix the updateForeign / updatePending calls and have the foreign transaction appears in the pre-filtered input list (in which case we can simply ditch updateForeign entirely).

I didn't fix the parsing of the balance from the CoinSelHarderrUtxoExhausted

          ex@(CoinSelHardErrUtxoExhausted balance _payment) ->
              case (readMaybe $ T.unpack balance) of
              -- NOTE balance & payment are "prettified" coins representation (e.g. "42 coin(s)")
              case (readMaybe $ T.unpack $ T.dropWhileEnd (not . C.isDigit) balance) of

I'll open a ticket for this as it requires a bit more change and is quite isolated from this particular PR.

KtorZ · 2018-10-09T12:24:04Z

Huhuh, unit tests aren't happy with the new invariant from divvyFee

adinapoli-iohk

General braindump.

adinapoli-iohk · 2018-10-09T12:53:49Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

  where
+    -- divvyFee only accepts non-zero output values as it doesn't make sense
+    -- to apply fees to those.
+    (cs0, cs') = partition (== valueZero) cs


Do we need this partition, at all? With @edsko we initially thought a simple filter for (> valueZero) might have been enough. What are we missing?

As far as I understand, we still want to return those zero value as part of the change, hence the change from

bimap identity unsafeFeeSum

to

bimap (++ cs0) unsafeFeeSum

We just don't run the divvyFee repartition function on them... I mean, we would filter out the zero value at this stage, but I'd expect this function to return the same number of inputs I gave it, no ?

adinapoli-iohk · 2018-10-09T12:54:35Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic.hs

-divvyFee f fee [a] | f a == valueZero = [(fee, a)]
-divvyFee f fee as                     = map (\a -> (feeForOut a, a)) as
+divvyFee _ _   []                             = error "divvyFee: empty list"
+divvyFee f _ as | any ((== valueZero) . f) as = error "divvyFee: some outputs are null"


Do we still need this special guard? I suspect that now that we pushed the main logic fix inside reduceChangeOutputs , we can try to be more liberal here?

Well, this is an invariant we want to enforce. If we don't have that, then we might get a division by zero silently coerced to Word64 and turned to 0.

λ> ceiling (0 * (14 / 0 :: Double)) :: Word64 0

So, I'll better leave a guard that explicitly fails and tell us that we did something wrong 👍 (like it seems to happen in the unit test 😅 )

KtorZ · 2018-10-09T14:44:08Z

Okay, fixed the genPayees generator which was generating null payments 👍

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.

KtorZ · 2018-10-09T16:28:14Z

wallet-new/src/Cardano/Wallet/Kernel/DB/Spec/Read.hs

-    pendingIns   = Set.union
-        (Pending.txIns $ c ^. cpPending)
-        (Pending.txIns $ c ^. cpForeign)
+    pendingIns   = Pending.txIns $ c ^. cpPending


Reverting from #3672

edsko

LGTM!

KtorZ · 2018-10-09T18:45:43Z

Alles Goed! Not waiting for Darwin here 👍

…rect-coinselerr [CBR-464] Raise Correct `CoinSelHardErr` during coin selection

…hk/KtorZ/CBR-464/raise-correct-coinselerr [CBR-464] Raise Correct `CoinSelHardErr` during coin selection

[CBR-464] Move TransactionSpec DSL under 'where'

f219b68

This makes it available for other tests within this context.

KtorZ requested a review from adinapoli-iohk October 4, 2018 14:31

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch from 4c76bd8 to 7c4ed1c Compare October 4, 2018 14:35

nikolaglumac mentioned this pull request Oct 5, 2018

[DDW-427] Handle new transaction fee calculation error messages input-output-hk/daedalus#1111

Merged

26 tasks

adinapoli-iohk approved these changes Oct 5, 2018

View reviewed changes

KtorZ added the DO NOT MERGE label Oct 5, 2018

edsko reviewed Oct 9, 2018

View reviewed changes

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch from 7c4ed1c to c2a4272 Compare October 9, 2018 11:35

KtorZ removed the DO NOT MERGE label Oct 9, 2018

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch from c2a4272 to 85adb36 Compare October 9, 2018 11:42

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch 2 times, most recently from 51c0a22 to 85adb36 Compare October 9, 2018 12:20

adinapoli-iohk reviewed Oct 9, 2018

View reviewed changes

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch from 85adb36 to bcb7c49 Compare October 9, 2018 14:43

KtorZ mentioned this pull request Oct 9, 2018

[CO-395] Redeem Ada Fix Pending #3672

Merged

11 tasks

KtorZ force-pushed the KtorZ/CBR-464/raise-correct-coinselerr branch from bcb7c49 to 81e4323 Compare October 9, 2018 16:27

KtorZ commented Oct 9, 2018

View reviewed changes

edsko self-requested a review October 9, 2018 16:31

edsko approved these changes Oct 9, 2018

View reviewed changes

KtorZ merged commit 993d6be into develop Oct 9, 2018

KtorZ deleted the KtorZ/CBR-464/raise-correct-coinselerr branch October 9, 2018 18:45

KtorZ added a commit that referenced this pull request Nov 9, 2018

Merge pull request #3710 from input-output-hk/KtorZ/CBR-464/raise-cor…

48f548b

…rect-coinselerr [CBR-464] Raise Correct `CoinSelHardErr` during coin selection

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CBR-464] Raise Correct `CoinSelHardErr` during coin selection #3710

[CBR-464] Raise Correct `CoinSelHardErr` during coin selection #3710

KtorZ commented Oct 4, 2018 •

edited

Loading

KtorZ commented Oct 4, 2018

adinapoli-iohk left a comment

edsko Oct 9, 2018

KtorZ Oct 9, 2018

edsko Oct 9, 2018

KtorZ Oct 9, 2018

edsko Oct 9, 2018

edsko Oct 9, 2018

KtorZ Oct 9, 2018

edsko Oct 9, 2018

KtorZ Oct 9, 2018

edsko Oct 9, 2018

KtorZ Oct 9, 2018

edsko Oct 9, 2018

edsko Oct 9, 2018

KtorZ commented Oct 9, 2018

KtorZ commented Oct 9, 2018

adinapoli-iohk left a comment

adinapoli-iohk Oct 9, 2018

KtorZ Oct 9, 2018

adinapoli-iohk Oct 9, 2018

KtorZ Oct 9, 2018 •

edited

Loading

KtorZ commented Oct 9, 2018

KtorZ Oct 9, 2018

edsko left a comment

KtorZ commented Oct 9, 2018

[CBR-464] Raise Correct CoinSelHardErr during coin selection #3710

[CBR-464] Raise Correct CoinSelHardErr during coin selection #3710

Conversation

KtorZ commented Oct 4, 2018 • edited Loading

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

KtorZ commented Oct 4, 2018

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Oct 9, 2018

KtorZ commented Oct 9, 2018

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

KtorZ commented Oct 9, 2018

Choose a reason for hiding this comment

edsko left a comment

Choose a reason for hiding this comment

KtorZ commented Oct 9, 2018

[CBR-464] Raise Correct `CoinSelHardErr` during coin selection #3710

[CBR-464] Raise Correct `CoinSelHardErr` during coin selection #3710

KtorZ commented Oct 4, 2018 •

edited

Loading

KtorZ Oct 9, 2018 •

edited

Loading