Random input selection impl #140

paweljakubas · 2019-04-02T10:18:06Z

Issue Number

Overview

I have implemented random input selection
I have added respective unit and property tests

Comments

paweljakubas · 2019-04-02T10:24:10Z

src/Cardano/Wallet/CoinSelection/Random.hs

+    pure res
+
+
+-- Selecting coins to cover at least the specified value


This is how I understand how this algorithm operates

paweljakubas · 2019-04-02T10:24:49Z

src/Cardano/Wallet/CoinSelection/Random.hs

+        withIx m' ix = Just (Map.elemAt ix m', Map.deleteAt ix m')
+
+
+instance MonadRandom m => MonadRandom (ExceptT e m) where


need this unfortunately

paweljakubas · 2019-04-03T21:28:59Z

test/unit/Cardano/Wallet/CoinSelection/RandomSpec.hs

+                })
+
+    describe "Coin selection properties : Random algorithm" $ do
+        xit "forall (UTxO, NonEmpty TxOut), \


it stucks when random is called, probably because of wrong impl of MonadRandom instance of Identity at the end of this file

paweljakubas · 2019-04-03T21:29:27Z

test/unit/Cardano/Wallet/CoinSelection/RandomSpec.hs

+            <*> vectorOf n arbitrary
+        return $ UTxO $ Map.fromList utxo
+
+instance MonadRandom Identity where


this is probably wrong

it makes sense to me that this will loop forever. On the right side you are calling the same getRandomBytes defined in the instance MonadRandom Identity , with the same params.

Looking at https://www.stackage.org/haddock/lts-13.15/cryptonite-0.25/Crypto-Random-Types.html what I believe you should have aimed for is to use one of two implementations of class DRG (from the same module) for implementation of MonadRandom Identity as you would like to use some deterministic pseudo random generator in this pure code.

I can have a closer look later and experiment if this doesn't give any clue (hope its the right one)

thanks @akegalj for the comment. Indeed, random monad within random algo needs more effects power than the monad within largestFirst. there is a need of more effects to enable randomness - be in IO or have initial seed/generator.

akegalj · 2019-04-02T13:13:08Z

src/Cardano/Wallet/CoinSelection/Random.hs

+            $ NE.sortBy (flip $ comparing coin) txOutputs
+    let n = maximumNumberOfInputs opt
+
+    let moneyRequested = sum $ (getCoin . coin) <$> txOutputsSorted


I think parens can be dropped here

sum $ getCoin . coin <$> txOutputsSorted

akegalj · 2019-04-04T05:53:15Z

test/unit/Cardano/Wallet/CoinSelection/RandomSpec.hs

+            <*> vectorOf n arbitrary
+        return $ UTxO $ Map.fromList utxo
+
+instance MonadRandom Identity where


it makes sense to me that this will loop forever. On the right side you are calling the same getRandomBytes defined in the instance MonadRandom Identity , with the same params.

Looking at https://www.stackage.org/haddock/lts-13.15/cryptonite-0.25/Crypto-Random-Types.html what I believe you should have aimed for is to use one of two implementations of class DRG (from the same module) for implementation of MonadRandom Identity as you would like to use some deterministic pseudo random generator in this pure code.

I can have a closer look later and experiment if this doesn't give any clue (hope its the right one)

paweljakubas · 2019-04-04T08:42:57Z

resigned from using runIdentity with random and used newDRG and withDRG instead

paweljakubas · 2019-04-04T14:19:01Z

src/Cardano/Wallet/CoinSelection/Random.hs

+--    step.)
+--
+-- 2. Randomly select outputs from the UTxO, considering for each output if that
+--    output is animprovement. If it is, add it to the transaction, and keep


animprovement -> an improvement

paweljakubas · 2019-04-04T14:23:57Z

src/Cardano/Wallet/CoinSelection/Random.hs

-            targetAim <-
-                if isValidCoin (Coin $ 2*v) then Just (Coin $ 2*v) else Nothing
-            targetMax <-
-                if isValidCoin (Coin $ 3*v) then Just (Coin $ 3*v) else Nothing


we are not afraid of situation that for instance targetMax will exceed upperBound of what Coin can have???

Add NotEnoughMoney and UtxoNotEnoughFragmented errors

Don't use Identity with MonadRandom Fix maxNumberOfInput in propertyFragmentation Hush hlint

…ation

We don't need these various targets to be 'Coin', some may actually not be; in the end, we will select several coins, so it's not impossible that the sum of their value will overflow the max coin value. In the rare case this would happen, we can simply fallback to splitting the output into two change coins.

I found calling the latter 'atLeast' quite confusing with the 'largestFirst' algorithm.

paweljakubas added this to the Transaction creation, submission & Coin Selection milestone Apr 2, 2019

paweljakubas requested a review from KtorZ April 2, 2019 10:18

paweljakubas commented Apr 2, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/92/adding-random-input-selection branch from 720a1dc to b5de826 Compare April 2, 2019 10:42

KtorZ mentioned this pull request Apr 2, 2019

Port coin selection and fee calculation from legacy #92

Closed

7 tasks

paweljakubas self-assigned this Apr 2, 2019

paweljakubas force-pushed the paweljakubas/92/adding-random-input-selection branch from b5de826 to c672899 Compare April 3, 2019 18:54

paweljakubas commented Apr 3, 2019

View reviewed changes

akegalj reviewed Apr 4, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/92/adding-random-input-selection branch from cd287bd to 939f3f6 Compare April 4, 2019 08:39

paweljakubas force-pushed the paweljakubas/92/adding-random-input-selection branch 2 times, most recently from fc03426 to 328976e Compare April 4, 2019 09:08

paweljakubas commented Apr 4, 2019

View reviewed changes

paweljakubas and others added 13 commits April 4, 2019 17:35

Random input selection impl

96985b2

Add NotEnoughMoney and UtxoNotEnoughFragmented errors

Use fallback not per step but optionally in the end

2e605ae

Add unit tests

c80a56f

Add property tests

55507fe

Don't use Identity with MonadRandom Fix maxNumberOfInput in propertyFragmentation Hush hlint

remove extra space in module export

1552b7e

remove unecessary orphan instance

0cd5653

update docstring comment for 'random' selection to match the specific…

35e9564

…ation

add comments to 'TargetRange' type

e7a3cdb

use of 'MaybeT' to simplify writing of 'processTxOut'

74e5e07

replace 'Map TxIn TxOut' with 'UTxO' to ease transaction signatures

38fecdc

re-indent where clause on a 4-space indent (instead of 8-space)

fb23bad

remove currently useless fee-related options and types definitions

0a9f56d

KtorZ added 3 commits April 4, 2019 17:35

some minor naming revision: in particular 'atLeast' -> 'coverRandomly'

cddc877

I found calling the latter 'atLeast' quite confusing with the 'largestFirst' algorithm.

factor out common test logic in largest first and random coin selection

6596d6e

get system DRG through Hspec bracket mechanism (instead of unsafe IO)

a5bddd3

KtorZ force-pushed the paweljakubas/92/adding-random-input-selection branch from fd04299 to a5bddd3 Compare April 4, 2019 15:35

KtorZ approved these changes Apr 4, 2019

View reviewed changes

KtorZ merged commit 548a039 into master Apr 4, 2019

KtorZ deleted the paweljakubas/92/adding-random-input-selection branch April 4, 2019 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Random input selection impl #140

Random input selection impl #140

paweljakubas commented Apr 2, 2019 •

edited

Loading

paweljakubas Apr 2, 2019

paweljakubas Apr 2, 2019

paweljakubas Apr 3, 2019

paweljakubas Apr 3, 2019

akegalj Apr 4, 2019 •

edited

Loading

paweljakubas Apr 4, 2019

akegalj Apr 2, 2019

paweljakubas Apr 4, 2019

akegalj Apr 4, 2019 •

edited

Loading

paweljakubas commented Apr 4, 2019 •

edited

Loading

paweljakubas Apr 4, 2019

paweljakubas Apr 4, 2019

		pure res


		-- Selecting coins to cover at least the specified value

		withIx m' ix = Just (Map.elemAt ix m', Map.deleteAt ix m')


		instance MonadRandom m => MonadRandom (ExceptT e m) where

Random input selection impl #140

Random input selection impl #140

Conversation

paweljakubas commented Apr 2, 2019 • edited Loading

Issue Number

Overview

Comments

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

akegalj Apr 4, 2019 • edited Loading

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

akegalj Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

paweljakubas commented Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Apr 2, 2019 •

edited

Loading

akegalj Apr 4, 2019 •

edited

Loading

akegalj Apr 4, 2019 •

edited

Loading

paweljakubas commented Apr 4, 2019 •

edited

Loading