withdrawals as part of the coin selection #1865

KtorZ · 2020-07-06T15:43:22Z

Issue Number

Overview

12b28f7
📍 take withdrawals into account, one level earlier, during the coin selection
Still to be done:
- Make sure it's correctly done in the largest-first algorithm
- Add some test scenario that show the influence of the withdrawal
547b130
📍 unit-test the newly introduced 'proportionallyTo'
Getting this one wrong would be quite bad :s
975f726
📍 add unit tests showing how withdrawal impacts the random coin selection

💡 NOTE

I've chosen not to treat the withdrawal as a single input, but more as a "money pot" that is proportionally distributed amongst change output, so that it contributes to every output, based on their size. This is to avoid having a small output consuming the entire withdrawal for itself. Note sure if I'll keep the approach in the end, I'll have the night to think about it.

Comments

TODO: take into account the withdrawal when doing largest-first (and testing it)
TODO: echo the error message change on "ErrInputsDepleted" to integration tests relying on that message.

KtorZ · 2020-07-06T15:44:23Z

lib/core/src/Cardano/Wallet/Api/Server.hs

-                , "Try sending a smaller amount."
+                [ "I cannot select enough UTxO from your wallet to construct "
+                , "an adequate transaction. Try sending a smaller amount or "
+                , "increasing the number of available UTxO."


ErrInputsDepleted is now used also when trying to select coins on a null UTxO, despite enough balance in the reward account.

KtorZ · 2020-07-06T15:45:19Z

lib/core/src/Cardano/Wallet/Primitive/CoinSelection/Random.hs

+            (\acc out ->
+                let
+                    withdraw = totalWithdraw
+                        `proportionallyTo` (getCoin (coin out) % totalOut)


So here, we are guaranteed that the ratio given as second argument is always <= 1

real question, maybe trivial... but why do we need to use proportionallyTo at all? why this rule to transfer upon withdrawing proportionally to relative weight in total outs? Why not to transfer randomly (adding up to withdrawal)? Or almost everything to one plus 1 + 1 + ... + 1? Or evenly? Or create one change?

KtorZ · 2020-07-06T15:47:57Z

lib/core/src/Cardano/Wallet/Primitive/CoinSelection/Random.hs

-            guard sel $> (sel, remUtxo)
+            -- NOTE re-assigning total withdrawal to cope with potential
+            -- rounding issues.
+            guard sel $> (sel { withdrawal = totalWithdraw }, remUtxo)


sel here is constructed by appending multiple selection together from left to right, using the Monoid instance on CoinSelection. So in the end, the total withdrawal that we get is the sum of all withdrawals that were assigned to each output, but because proportionallyTo rounds down, the final sum may end up being slightly less than the total (an error of 1 per output at most) yet, we need it to be precisely equal to the reward's balance without what the Ledger will reject the withdrawal.

Thus, this is reassigned here, and the possible exceeding will be balanced out during fee balancing anyway.

if we transfer withdraw to one change we would not have this problem;-) But, when used proportionallyTo indeed we need to take care of this... So what happens in detail when proportionallyTo rounding does consume not all withdraw? To which changes this additional 1s go ? To those that correspond to outputs that were affected by this rounding, right?

To which changes this additional 1s go

Most likely, they'll be used to partially cover fee in the fee balancing algorithm here. What will happen in case of rounding issues is that the transaction will be slightly unbalanced (positively) and have a few lovelace extra on the input side assigned to "nothing". Following the fee balancing algorithm, it'll start by consuming that delta, and then, starts to deplete (proportionally again) change outputs. So in the end, the little exceeding isn't lost but goes directly for fee; which is roughly similar to assigning it to any of the change output, since they'll be depleted for fees regardless.

paweljakubas · 2020-07-06T17:21:46Z

lib/core/src/Cardano/Wallet/Primitive/CoinSelection.hs

@@ -117,12 +123,35 @@ changeBalance = foldl' addCoin 0 . change
 feeBalance :: CoinSelection -> Word64
 feeBalance sel = inputBalance sel - outputBalance sel - changeBalance sel

+-- | Total UTxO balance + withdrawal.
+totalBalance :: Quantity "lovelace" Word64 -> [(TxIn, TxOut)] -> Word64
+totalBalance (Quantity withdraw) inps = balance' inps + withdraw


paweljakubas · 2020-07-06T17:24:10Z

lib/core/src/Cardano/Wallet/Primitive/CoinSelection.hs

+--
+-- >>> 10 `proportionallyTo` 1%3
+-- 3
+proportionallyTo :: Integral a => a -> Ratio a -> a


any chance to have unit test for this or something that uses this inside?

it is checked below

paweljakubas · 2020-07-06T17:25:11Z

lib/core/src/Cardano/Wallet/Primitive/CoinSelection/LargestFirst.hs

@@ -59,11 +69,15 @@ largestFirst opt outs utxo = do
            guard s $> (s, UTxO $ Map.fromList utxo')
        Nothing -> do
            let moneyRequested = sum $ (getCoin . coin) <$> (descending outs)
-            let utxoBalance = fromIntegral $ balance utxo
+            let utxoList = Map.toList $ getUTxO utxo
+            let total = totalBalance withdraw utxoList


paweljakubas · 2020-07-06T17:54:15Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/RandomSpec.hs

+        coinSelectionUnitTest random "withdrawal simple"
+            (Right $ CoinSelectionResult
+                { rsInputs = [1]
+                , rsChange = []


why it is not [1] ?

ah, ok. we have input 1 plus withdraw 1 to cover 2

paweljakubas · 2020-07-06T17:54:49Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/RandomSpec.hs

+        coinSelectionUnitTest random "withdrawal multi-output"
+            (Right $ CoinSelectionResult
+                { rsInputs = [1,1]
+                , rsChange = []


why it is not [1,1] ?

inps + withdrawals : [1,1,2] to cover [2,2] gives [] change

These examples are indeed a bit tricky to follow, I tried to left some note above each test case to make it a bit easier.

paweljakubas · 2020-07-06T17:59:17Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/RandomSpec.hs

+                , totalWithdrawal = 20
+                })
+
+        coinSelectionUnitTest random "withdrawal requires at least one input"


this one I understand 👍

paweljakubas · 2020-07-06T18:07:05Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/RandomSpec.hs

+            -- - 2 Ada goes to the other output ( 4/14 * 10 ~= 2)
+            (Right $ CoinSelectionResult
+                { rsInputs = [5,5]
+                , rsChange = [2,3]


and here we see that in spite of missing 1 by proportionallyTo the money is counting right

paweljakubas · 2020-07-06T18:09:02Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/RandomSpec.hs

+                { maxNumOfInputs = 100
+                , validateSelection = noValidation
+                , utxoInputs = [1]
+                , txOutputs = 10 :| [10]


in old days it would trigger not enough fragmentation if the number of inputs does not match number of outputs. now I see withdrawals help in this sense 👍 wonder if there are some integration tests checking this edge conditions that needs to be updated

what about this example :

(CoinSelectionFixture { maxNumOfInputs = 100 , validateSelection = noValidation , utxoInputs = [1] , txOutputs = 1 :| [1,1,1,1,1] , totalWithdrawal = 5 })

?

what about this example

Following the current logic, each output would receive 0 withdrawal bonus (1/6 * 5, rounded down) and therefore, this would fail on the second output because there wouldn't be enough inputs to select. Although indeed, if instead of using the withdrawal proportionally, we were using them fully to cover "just" the right amount, it'd give better result.

paweljakubas · 2020-07-06T18:10:57Z

lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelectionSpec.hs

@@ -99,6 +109,16 @@ spec = do
        prop "All inputs are used" prop_allInputsAreUsed
        prop "All inputs are used per transaction" prop_allInputsAreUsedPerTx
        prop "Addresses are recycled fairly" prop_fairAddressesRecycled
+
+    describe "proportionallyTo" $ do


ah, this one I was asking above - persuasive 💯

paweljakubas · 2020-07-06T18:13:32Z

lib/core/test/unit/Cardano/Wallet/Primitive/FeeSpec.hs

@@ -707,10 +709,10 @@ genSelectionFor :: NonEmpty TxOut -> Gen CoinSelection
 genSelectionFor outs = do
    let opts = CS.CoinSelectionOptions (const 100) (const $ pure ())
    utxo <- vector (NE.length outs * 3) >>= genUTxO
-    case runIdentity $ runExceptT $ largestFirst opts outs utxo of
+    withdrawal_ <- genWithdrawal
+    case runIdentity $ runExceptT $ largestFirst opts outs (Quantity withdrawal_) utxo of


Dusting is going to affect this somehow ? Or we are confident it is going to be fine?

I am not sure to follow your concern on this one 🤔 ?

KtorZ · 2020-07-07T15:37:01Z

bors try

iohk-bors · 2020-07-07T16:14:45Z

try

Build failed

buildkite/cardano-wallet

paweljakubas

LGTM!

KtorZ · 2020-07-08T09:28:48Z

bors r+

1865: withdrawals as part of the coin selection r=KtorZ a=KtorZ # Issue Number  #1861 # Overview  - 12b28f7 📍 **take withdrawals into account, one level earlier, during the coin selection** Still to be done: - Make sure it's correctly done in the largest-first algorithm - Add some test scenario that show the influence of the withdrawal - 547b130 📍 **unit-test the newly introduced 'proportionallyTo'** Getting this one wrong would be quite bad :s - 975f726 📍 **add unit tests showing how withdrawal impacts the random coin selection** > 💡 NOTE > > I've chosen not to treat the withdrawal as a single _input_, but more as a "money pot" that is proportionally distributed amongst change output, so that it contributes to every output, based on their size. This is to avoid having a small output consuming the entire withdrawal for itself. Note sure if I'll keep the approach in the end, I'll have the night to think about it. # Comments  - [ ] TODO: take into account the withdrawal when doing largest-first (and testing it) - [ ] TODO: echo the error message change on "ErrInputsDepleted" to integration tests relying on that message.  Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-07-08T10:57:51Z

Build failed

buildkite/cardano-wallet

…ection Still to be done: - Make sure it's correctly done in the largest-first algorithm - Add some test scenario that show the influence of the withdrawal

Getting this one wrong would be quite bad :s

It is a bit more work to get this right, but in the end, it solves the same problem (being able to use the withdrawal on several outputs) with more elegance and efficiency. Now the withdrawal is used gradually, and only what's needed at every step so in that sense, it is optimal.

iohk-bors · 2020-07-08T22:40:06Z