Merge pull request #3730 from input-output-hk/adinapoli/always-sort-outputs-coin-selection

Always sort payees by amount

Author: adinapoli-iohk

src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

@@ -1,4 +1,5 @@
-{-# LANGUAGE RankNTypes #-}
+{-# LANGUAGE RankNTypes   #-}
+{-# LANGUAGE ViewPatterns #-}
 {-# OPTIONS_GHC -fno-warn-orphans #-}
 
 module Cardano.Wallet.Kernel.CoinSelection.FromGeneric (
@@ -211,7 +212,23 @@ runCoinSelT :: forall m. Monad m
                   => NonEmpty (Output (Dom utxo))
                   -> CoinSelT utxo CoinSelHardErr m [CoinSelResult (Dom utxo)])
             -> CoinSelPolicy Core.Utxo m CoinSelFinalResult
-runCoinSelT opts pickUtxo policy request utxo = do
+runCoinSelT opts pickUtxo policy (NE.sortBy (flip (comparing outVal)) -> request) utxo = do
+    -- NOTE: we sort the payees by output value, to maximise our chances of succees.
+    -- In particular, let's consider a scenario where:
+    --
+    -- 1. We have a payment request with two outputs, one smaller and one
+    --    larger (but both relatively large)
+    -- 2. Random selection tries to cover the smaller one first, fail because
+    --    it exceeds the maximum number of inputs, fall back on largest first,
+    --    which will pick the n largest outputs, and then traverse from from
+    --    small to large to cover the payment.
+    -- 3. It then tries to deal with the larger output, and fail because in
+    --    step (2) we picked an output we needed due to only consider the n
+    --    largest outputs (rather than sorting the entire UTxO,
+    --    which would be much too expensive).
+    --
+    -- Therefore, just always considering them in order from large to small
+    -- is probably a good idea.
     mSelection <- unwrapCoinSelT policy' utxo
     case mSelection of
       Left err -> return (Left err)

test/unit/Test/Spec/CoinSelection.hs

@@ -495,16 +495,7 @@ payRestrictInputsTo :: Word64
 payRestrictInputsTo maxInputs genU genP feeFunction adjustOptions bal amount policy =
     withDefConfiguration $ \genesisConfig -> do
         utxo  <- genU bal
-        -- Sort the payees by amount, so that biggest entries comes first. This
-        -- is to mitigate some pathological generator's corner cases in
-        -- @largestFirst@ where the selection would pick the largest to cover
-        -- the smallest inputs, not having enough Utxo to cover the rest. We do
-        -- this here in the tests and not by modifying the coin selection code
-        -- because we never run @largestFirst@ in production; in @random@ we
-        -- merely piggyback on the @atLeast@ function (a low-level API used
-        --for @largestFirst') but the @random@ policy doesn't require outputs
-        -- to be sorted, and doing so every time would be a waste of CPU cycles.
-        payee <- sortByAmount <$> genP utxo amount
+        payee <- genP utxo amount
         key   <- arbitrary
         let options = adjustOptions (newOptions feeFunction)
         res <- policy options
@@ -521,9 +512,6 @@ payRestrictInputsTo maxInputs genU genP feeFunction adjustOptions bal amount pol
                                    outputs
                                    change
                     return (utxo, payee, bimap STB identity txAux)
-    where
-        sortByAmount :: NonEmpty Core.TxOut -> NonEmpty Core.TxOut
-        sortByAmount = NE.sort
 
 pay :: (InitialBalance -> Gen Core.Utxo)
     -> (Core.Utxo -> Pay -> Gen (NonEmpty Core.TxOut))