[CO-450] Fix random-improve fallback (develop)

CHANGELOG.md

@@ -6,6 +6,12 @@
 
 - Fix issue that caused the node to lose sync with the chain in the OBFT era and then fail to re-sync once out of sync ([CBR-525]https://iohk.myjetbrains.com/youtrack/issue/CBR-525) [#4153](https://github.com/input-output-hk/cardano-sl/pull/4153))
 
+- Fix wrong balance and payment reporting on coin selection failures ([CO-450](https://iohk.myjetbrains.com/youtrack/issue/CBR-450) [#4159](https://github.com/input-output-hk/cardano-sl/pull/4159))
+
+- Additional coin selection fallback to cope with transaction filling up during random-improve selection ([CO-450](https://iohk.myjetbrains.com/youtrack/issue/CBR-450) [#4159](https://github.com/input-output-hk/cardano-sl/pull/4159))
+
+- Additional guard to prevent too big transactions to be submitted ([CO-449](https://iohk.myjetbrains.com/youtrack/issue/CBR-449) [#4131](https://github.com/input-output-hk/cardano-sl/pull/4131))
+
 ## Cardano SL 3.0.1
 
 ### Fixes

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

@@ -1,4 +1,5 @@
-{-# LANGUAGE BangPatterns #-}
+{-# LANGUAGE LambdaCase          #-}
+{-# LANGUAGE ScopedTypeVariables #-}
 
 module Cardano.Wallet.Kernel.CoinSelection.Generic.Random (
     PrivacyMode(..)
@@ -33,10 +34,48 @@ random :: forall utxo m. (MonadRandom m, PickFromUtxo utxo)
        -> Word64              -- ^ Maximum number of inputs
        -> [Output (Dom utxo)] -- ^ Outputs to include
        -> CoinSelT utxo CoinSelHardErr m [CoinSelResult (Dom utxo)]
-random privacyMode = coinSelPerGoal $ \maxNumInputs goal ->
-    defCoinSelResult goal <$>
-      inRange maxNumInputs (target privacyMode (outVal goal))
+random privacyMode maxNumInputs outs = do
+    balance <- gets utxoBalance
+    mapCoinSelErr (withTotalBalance balance) $
+        coinSelPerGoal step maxNumInputs outs `catchError`
+            (\_ -> LargestFirst.largestFirst maxNumInputs outs)
   where
+    -- | Perform a coin selection on the next output using the remaining
+    -- inputs. `coinSelPerGoal` reduces the UTxO (and the number of allowed)
+    -- inputs as it maps over the outputs. So, in the first iteration we have:
+    --
+    -- `remainingNumInputs == maxNumInputs`, and for the second one, we have
+    --
+    -- `remainingNumInputs == maxNumInputs - k`, where `k` is the number of
+    -- inputs selected during the first iteration.
+    step
+        :: Word64
+        -> Output (Dom utxo)
+        -> CoinSelT utxo CoinSelHardErr m (CoinSelResult (Dom utxo))
+    step remainingNumInputs out = defCoinSelResult out <$>
+        inRange remainingNumInputs (target privacyMode (outVal out))
+
+    -- | Because of the recursive and stateful nature of `coinSelPerGoal`,
+    -- errors are thrown within each step using values available at the moment
+    -- where the error gets thrown. As a result, errors reports non-sensical
+    -- balances and UTxO state.
+    -- As a work-around, we remap errors to what they ought to be...
+    withTotalBalance
+        :: Value (Dom utxo)
+        -> CoinSelHardErr
+        -> CoinSelHardErr
+    withTotalBalance balance = \case
+        e@CoinSelHardErrOutputCannotCoverFee{} -> e
+        e@CoinSelHardErrOutputIsRedeemAddress{} -> e
+        e@CoinSelHardErrCannotCoverFee{} -> e
+        CoinSelHardErrMaxInputsReached _ -> CoinSelHardErrMaxInputsReached
+            (show maxNumInputs)
+        CoinSelHardErrUtxoExhausted _ _ -> CoinSelHardErrUtxoExhausted
+            (pretty balance)
+            (pretty payment)
+      where
+        payment = unsafeValueSum $ outVal <$> outs
+
     target :: PrivacyMode -> Value (Dom utxo) -> TargetRange (Dom utxo)
     target PrivacyModeOn  val = fromMaybe (target PrivacyModeOff val)
                                           (idealRange val)

wallet/src/Cardano/Wallet/Kernel/Transactions.hs

@@ -35,6 +35,7 @@ import           Data.Default (def)
 import qualified Data.List.NonEmpty as NonEmpty
 import qualified Data.Map.Strict as Map
 import qualified Data.Vector as V
+import           Serokell.Data.Memory.Units (toBytes)
 import qualified System.Random.MWC (GenIO, asGenIO, initialize, uniformVector)
 import           Test.QuickCheck (Arbitrary (..), oneof)
 
@@ -82,6 +83,7 @@ import           Pos.Crypto (EncryptedSecretKey, PassPhrase, ProtocolMagic,
                      PublicKey, RedeemSecretKey, SafeSigner (..),
                      ShouldCheckPassphrase (..), Signature (..), hash,
                      redeemToPublic)
+
 import           UTxO.Util (shuffleNE)
 
 {-------------------------------------------------------------------------------
@@ -176,7 +178,7 @@ pay activeWallet spendingPassword opts accountId payees = do
              Left e      -> return (Left $ PaymentNewTransactionError e)
              Right (txAux, partialMeta, _utxo) -> do
                  let sz = fromIntegral $ BL.length $ serialize txAux
-                 maxSz <- Node.getMaxTxSize (walletPassive activeWallet ^. walletNode)
+                 maxSz <- toBytes <$> Node.getMaxTxSize (walletPassive activeWallet ^. walletNode)
                  if sz >= maxSz then
                      return
                      . Left

wallet/test/integration/Test/Integration/Scenario/Transactions.hs

@@ -12,6 +12,92 @@ import           Test.Integration.Framework.Scenario (Scenario)
 
 spec :: Scenarios Context
 spec = do
+    let oneAda = 1000000
+
+    scenario "REG CO-449: extra guard for too big transactions" $ do
+        fixtureSource <- setup $ defaultSetup
+            & initialCoins .~ [oneAda, oneAda, oneAda, oneAda, oneAda]
+        fixtureDest <- setup $ defaultSetup
+        accountDest <- successfulRequest $ Client.getAccount
+            $- (fixtureDest ^. wallet . walletId)
+            $- defaultAccountId
+        let distribution = customDistribution $ NonEmpty.fromList
+                [ (accountDest, 2*oneAda)
+                , (accountDest, 2*oneAda)
+                ]
+        resp <- request $ Client.postTransaction $- Payment
+            (defaultSource fixtureSource)
+            distribution
+            defaultGroupingPolicy
+            (Just $ fixtureSource ^. spendingPassword)
+        verify resp
+            [ expectWalletError TooBigTransaction
+              -- Additionally, check logs for 'CoinSelHardErrMaxInputsReached2'
+            ]
+
+    scenario "REG CO-450: fallback works correctly" $ do
+        fixtureSource <- setup $ defaultSetup
+            & initialCoins .~ [oneAda, oneAda, oneAda, oneAda]
+        fixtureDest <- setup $ defaultSetup
+        accountDest <- successfulRequest $ Client.getAccount
+            $- (fixtureDest ^. wallet . walletId)
+            $- defaultAccountId
+
+        -- NOTE (1)
+        -- We use an artificially low `maxTxSize` in integration
+        -- tests which limit the maximum number of inputs to `4`.
+        --
+        -- NOTE (2)
+        -- The random-improve algorithm tries to improve the selection on each
+        -- output, one after the other. "Improving" the selection means, trying
+        -- to select new inputs to get closer to an ideal value (2x the output).
+
+        -- Before any fix, this would fail with:
+        -- CoinSelHardErrUtxoExhausted
+        --   { balance: 4000000 coin(s)
+        --   , value:   2500000 coin(s)
+        --   }
+        -- because the random-improve algorithm would consume the first 4 inputs
+        -- to satisfy the first output. And then, nothing would be left for the
+        -- second output. After the fix, the algorithm will fallback to a
+        -- largest-first approach for all outputs, and pick only two coins for
+        -- the first input, and another one for the second input.
+        let distribution = customDistribution $ NonEmpty.fromList
+                [ (accountDest, 2*oneAda)
+                , (accountDest, oneAda `div` 2)
+                ]
+        resp <- request $ Client.postTransaction $- Payment
+            (defaultSource fixtureSource)
+            distribution
+            defaultGroupingPolicy
+            (Just $ fixtureSource ^. spendingPassword)
+        verify resp
+            [ expectSuccess
+            ]
+
+    scenario "REG CO-450: coin selection error reports correct balance" $ do
+        fixtureSource <- setup $ defaultSetup
+            & initialCoins .~ [oneAda, oneAda, oneAda]
+        fixtureDest <- setup $ defaultSetup
+
+        accountDest <- successfulRequest $ Client.getAccount
+            $- (fixtureDest ^. wallet . walletId)
+            $- defaultAccountId
+
+        let distribution = customDistribution $ NonEmpty.fromList
+                [ (accountDest, oneAda)
+                , (accountDest, 3*oneAda)
+                ]
+        resp <- request $ Client.postTransaction $- Payment
+            (defaultSource fixtureSource)
+            distribution
+            defaultGroupingPolicy
+            (Just $ fixtureSource ^. spendingPassword)
+
+        verify resp
+            [ expectWalletError $
+                NotEnoughMoney $ ErrAvailableBalanceIsInsufficient $ 3*oneAda
+            ]
 
     -- estimated fee amount for this transaction is 187946
     multioutputTransactionScenario
@@ -323,7 +409,7 @@ spec = do
     --   fee+   : 171905         fee+   : 228815
     --
     --           Actual fee: 228903 (+88)
-    coinSelectionScenario "needs many extra inputs" (replicate 8 30000) 42
+    coinSelectionScenario "needs many extra inputs" (replicate 4 50000) 42
   where
     coinSelectionScenario :: String -> [Word64] -> Word64 -> Scenarios Context
     coinSelectionScenario title coins amt =

wallet/test/integration/configuration.yaml

@@ -22,10 +22,10 @@ default: &default
           seed: 0
         blockVersionData: &default_core_genesis_spec_blockVersionData
           scriptVersion:     0
-          slotDuration:      10000
+          slotDuration:      5000
           maxBlockSize:      2000000
           maxHeaderSize:     2000000
-          maxTxSize:         4096 # 4 Kb
+          maxTxSize:         1024 # 1kb
           maxProposalSize:   700 # 700 bytes
           mpcThd:            0.01 # 1% of stake
           heavyDelThd:       0.005 # 0.5% of stake