diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee95b340b3..e81d60b809a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs index 49d92f52861..3f46a60a6ff 100644 --- a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs +++ b/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) diff --git a/wallet/src/Cardano/Wallet/Kernel/Transactions.hs b/wallet/src/Cardano/Wallet/Kernel/Transactions.hs index 267bbb580db..ece25918608 100644 --- a/wallet/src/Cardano/Wallet/Kernel/Transactions.hs +++ b/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 diff --git a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs b/wallet/test/integration/Test/Integration/Scenario/Transactions.hs index 7ebcafbefdf..c5883399fcb 100644 --- a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs +++ b/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 = diff --git a/wallet/test/integration/configuration.yaml b/wallet/test/integration/configuration.yaml index 52c2a58a9de..797bd983111 100644 --- a/wallet/test/integration/configuration.yaml +++ b/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