From 514c0c3c12010b5e9d02b0a329e684cf05435490 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 14:07:39 +0200 Subject: [PATCH 1/7] [CO-450] reproduce random-improve bug --- .../Test/Integration/Scenario/Transactions.hs | 67 ++++++++++++++++++- wallet/test/integration/configuration.yaml | 4 +- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs b/wallet/test/integration/Test/Integration/Scenario/Transactions.hs index 7ebcafbefdf..370bb835f13 100644 --- a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs +++ b/wallet/test/integration/Test/Integration/Scenario/Transactions.hs @@ -12,6 +12,71 @@ import Test.Integration.Framework.Scenario (Scenario) spec :: Scenarios Context spec = do + let oneAda = 1000000 + + 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 +388,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 From 363ce8d6f483f116535ab57a63c44f4a2bf8cf04 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 14:20:41 +0200 Subject: [PATCH 2/7] [CO-450] slightly rewrite 'random' to better illustrate how 'coinSelPerGoal' works --- .../Kernel/CoinSelection/Generic/Random.hs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs index 49d92f52861..aa93cf56d1a 100644 --- a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs +++ b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs @@ -1,4 +1,4 @@ -{-# LANGUAGE BangPatterns #-} +{-# LANGUAGE ScopedTypeVariables #-} module Cardano.Wallet.Kernel.CoinSelection.Generic.Random ( PrivacyMode(..) @@ -33,10 +33,24 @@ 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 = + coinSelPerGoal step 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)) + target :: PrivacyMode -> Value (Dom utxo) -> TargetRange (Dom utxo) target PrivacyModeOn val = fromMaybe (target PrivacyModeOff val) (idealRange val) From d4cd80d8fb27d46788df315fddd9dd9b29fdb0f7 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 14:46:27 +0200 Subject: [PATCH 3/7] [CO-450] fix erroneous balance reporting when running selection on multiple successive outputs --- .../Kernel/CoinSelection/Generic/Random.hs | 28 +++++++++++++++++-- .../src/Cardano/Wallet/Kernel/Transactions.hs | 1 + 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs index aa93cf56d1a..1029cb63e09 100644 --- a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs +++ b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs @@ -1,3 +1,4 @@ +{-# LANGUAGE LambdaCase #-} {-# LANGUAGE ScopedTypeVariables #-} module Cardano.Wallet.Kernel.CoinSelection.Generic.Random ( @@ -33,8 +34,10 @@ 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 maxNumInputs outs = - coinSelPerGoal step maxNumInputs outs +random privacyMode maxNumInputs outs = do + balance <- gets utxoBalance + mapCoinSelErr (withTotalBalance balance) $ + coinSelPerGoal step maxNumInputs outs where -- | Perform a coin selection on the next output using the remaining -- inputs. `coinSelPerGoal` reduces the UTxO (and the number of allowed) @@ -51,6 +54,27 @@ random privacyMode maxNumInputs outs = 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..2b30fd08469 100644 --- a/wallet/src/Cardano/Wallet/Kernel/Transactions.hs +++ b/wallet/src/Cardano/Wallet/Kernel/Transactions.hs @@ -82,6 +82,7 @@ import Pos.Crypto (EncryptedSecretKey, PassPhrase, ProtocolMagic, PublicKey, RedeemSecretKey, SafeSigner (..), ShouldCheckPassphrase (..), Signature (..), hash, redeemToPublic) + import UTxO.Util (shuffleNE) {------------------------------------------------------------------------------- From 2e974d958906f9f401b40fdfeab52e84738a5d31 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 15:02:48 +0200 Subject: [PATCH 4/7] [CO-450] fix error message for TooBigTransaction guard (remove doubled unit) --- wallet/src/Cardano/Wallet/Kernel/Transactions.hs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/wallet/src/Cardano/Wallet/Kernel/Transactions.hs b/wallet/src/Cardano/Wallet/Kernel/Transactions.hs index 2b30fd08469..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) @@ -177,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 From af7bb0b489ebeed943cf158df74abb772138993d Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 15:56:03 +0200 Subject: [PATCH 5/7] [CO-450] Fallback to largest first for each output from the start when random-improve fails --- .../src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs index 1029cb63e09..3f46a60a6ff 100644 --- a/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs +++ b/wallet/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Random.hs @@ -37,7 +37,8 @@ random :: forall utxo m. (MonadRandom m, PickFromUtxo utxo) random privacyMode maxNumInputs outs = do balance <- gets utxoBalance mapCoinSelErr (withTotalBalance balance) $ - coinSelPerGoal step maxNumInputs outs + 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) From a263539a52b6b5b96730aa95bbd60478a5f2ac28 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Wed, 5 Jun 2019 16:00:43 +0200 Subject: [PATCH 6/7] [CO-449] Add regression test for extra guard on too big transactions --- .../Test/Integration/Scenario/Transactions.hs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs b/wallet/test/integration/Test/Integration/Scenario/Transactions.hs index 370bb835f13..c5883399fcb 100644 --- a/wallet/test/integration/Test/Integration/Scenario/Transactions.hs +++ b/wallet/test/integration/Test/Integration/Scenario/Transactions.hs @@ -14,6 +14,27 @@ 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] From 5047a62f17183c3f18d9dd879cbae30ee8561d17 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Mon, 10 Jun 2019 10:53:02 +0200 Subject: [PATCH 7/7] [CO-450][CO-449] Add entries to CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) 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