[RCD-45] & [RCD-44] Review fee calculation entirely #3861

KtorZ · 2018-11-16T18:16:31Z

Description

So, basically, by conflating a bit the selected entries and changes, a
lot of things become easier. At the price of one thing: fairness.

The previous code was splitting fee across change proportionally to
inputs. So here, I just split the fee across all changes, regardless of
the input.

One could see it as another type of fairness 🙃 ... But
that's also a lot simpler to handle, because we can just manipulate all
inputs and all changes directly and compute fee for those directly.

Linked issue

RCD-45
RCD-44

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

Wallet unit tests
  Coin selection policies unit tests
    largestFirst
      one payee, SenderPaysFee, fee = 0
        +++ OK, passed 1000 tests.
      one payee, ReceiverPaysFee, fee = 0
        +++ OK, passed 1000 tests.
      multiple payees, SenderPaysFee, fee = 0
        +++ OK, passed 1000 tests.
      multiple payees, ReceiverPaysFee, fee = 0
        +++ OK, passed 1000 tests.
      one payee, SenderPaysFee, fee = 1 Lovelace
        +++ OK, passed 1000 tests.
      one payee, ReceiverPaysFee, fee = 1 Lovelace
        +++ OK, passed 1000 tests.
      multiple payees, SenderPaysFee, fee = 1 Lovelace
        +++ OK, passed 1000 tests.
      multiple payees, ReceiverPaysFee, fee = 1 Lovelace
        +++ OK, passed 1000 tests.
    random
      one payee, SenderPaysFee, fee = 0
        +++ OK, passed 2000 tests.
      one payee, ReceiverPaysFee, fee = 0
        +++ OK, passed 2000 tests.
      multiple payees, SenderPaysFee, fee = 0
        +++ OK, passed 2000 tests.
      multiple payees, ReceiverPaysFee, fee = 0
        +++ OK, passed 2000 tests.
      one payee, SenderPaysFee, fee = 1 Lovelace
        +++ OK, passed 2000 tests.
      multiple payees, SenderPaysFee, fee = 1 Lovelace
        +++ OK, passed 2000 tests.
      one payee, ReceiverPaysFee, fee = linear
        +++ OK, passed 2000 tests.
      multiple payees, ReceiverPaysFee, fee = linear
        +++ OK, passed 2000 tests.
      one payee, SenderPaysFee, fee = cardano
        +++ OK, passed 2000 tests.
      multiple payees, SenderPaysFee, fee = cardano
        +++ OK, passed 2000 tests.
      one payee, ReceiverPaysFee, fee = cardano
        +++ OK, passed 2000 tests.
      multiple payees, ReceiverPaysFee, fee = cardano
        +++ OK, passed 2000 tests.
    Expected failures
      Paying a redeem address should always be rejected
        +++ OK, passed 2000 tests.
      Paying somebody not having enough money should fail
        +++ OK, passed 2000 tests.
      Restricting too much the number of inputs results in an hard error for a single payee
        +++ OK, passed 2000 tests.
      Restricting too much the number of inputs results in an hard error for multiple payees
        +++ OK, passed 2000 tests.
    Fiddly Addresses
      multiple payees, SenderPaysFee, fee = cardano
        +++ OK, passed 200 tests.
      multiple payees, ReceiverPaysFee, fee = cardano
        +++ OK, passed 200 tests.
    Input Grouping
      Require grouping, fee = 0, one big group depletes the Utxo completely
        +++ OK, passed 2000 tests.
      Require grouping, fee = cardano, one big group depletes the Utxo completely
        +++ OK, passed 2000 tests.
      Require grouping, fee = 0, several groups allows the payment to be fullfilled
        +++ OK, passed 2000 tests.
      Prefer grouping, fee = 0
        +++ OK, passed 2000 tests.
      IgnoreGrouping, fee = 0 must not deplete the utxo
        +++ OK, passed 2000 tests.
    Estimating the maximum number of inputs
      estimateMaxTxInputs yields a lower bound.
        +++ OK, passed 100 tests.
      estimateMaxTxInputs yields a relatively tight bound.
        +++ OK, passed 100 tests.
      estimateHardMaxTxInputs is close to estimateMaxTxInputs.
        +++ OK, passed 1000 tests.

Finished in 123.3834 seconds
34 examples, 0 failures

How to merge

Send the message bors r+ to merge this PR. For more information, see
docs/how-to/bors.md.

So, basically, by conflating a bit the selected entries and changes, a lot of things become easier. At the price of one thing: fairness. The previous code was splitting fee across change proportionally to inputs. So here, I just split the fee across all changes, regardless of the input. So everyone's got to pay the same part of for the transaction. One could see it as another type of fairness 🙃 ... But that's also a lot simpler to handle, because we can just manipulate all inputs and all changes directly and compute fee for those directly.

KtorZ · 2018-11-16T23:43:25Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

-            changes = changesRemoveDust (csoDustThreshold opts) changesWithDust
-        return . Right $ CoinSelFinalResult allInps
-                                            originalOuts
-                                            changes


Checked moved inside adjustForFees

KtorZ · 2018-11-16T23:43:58Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

-                newRemaining = unsafeValueSub remaining piece -- unsafe because of div.
-            in case valueAdd piece a of
-                Just newChange -> newChange : go newRemaining as
-                Nothing        -> a : go remaining as


Moved in Generic/Fee and is now part of the senderPaysFee algorithm.

KtorZ · 2018-11-16T23:45:06Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

@@ -70,6 +70,7 @@ instance IsValue Core.Coin where
                                   else a `Core.unsafeSubCoin` b
  valueRatio  = \  a b -> coinToDouble a / coinToDouble b
  valueAdjust = \r d a -> coinFromDouble r (d * coinToDouble a)
+  valueDiv    = divCoin


splitChange ranked higher up in the abstraction, so we need a way to divide Value dom into k pieces.

KtorZ · 2018-11-16T23:46:35Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

+
+type PickUtxo m utxo
+    =  Value (Dom utxo)
+    -> CoinSelT utxo CoinSelHardErr m (Maybe (UtxoEntry (Dom utxo)))


Just a type-alias

KtorZ · 2018-11-17T00:14:38Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

-    , csrOutputs :: NonEmpty Core.TxOutAux
-      -- ^ Picked outputs
-    , csrChange  :: [Core.Coin]
-    }


Moved one level higher in the abstraction as CoinSelFinalResult dom

KtorZ · 2018-11-19T08:12:18Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

+        -- 2/
+        -- We try to cover fee with the available change by substracting equally
+        -- across all inputs. There's no fairness in that in the case of a
+        -- multi-account transaction. Everyone pays the same part.


TODO: Amend this comment. Not really on the spot. We still divvy fee proportionally across all changes. Thus, the output:change ratio is more-or-less preserved. In the end, it may be less accurate but fairness still applies here.

edsko

LGTM, just some requests for some comments clarifying a few points.

edsko · 2018-11-19T08:33:33Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

-        senderPaysFee pickUtxo upperBound css
-  where
-    upperBound = feeUpperBound feeOptions css
+    let inps = concatMap (selectedEntries . coinSelInputs) css


Suggested comment: We lose the relationship between the transaction outputs and their corresponding inputs/change outputs here. This is a decision we may wish to revisit later. For now however note that since (a) coin selection tries to establish a particular ratio between payment outputs and change outputs (currently it aims for an average of 1:1), and moreover (b) coin selection currently only generates a single change output per payment output, distributing the fee proportionally across all change outputs is roughly equivalent to distributing it proportionally over the payment outputs (roughly, not exactly, because the 1:1 proportion is best effort only, and may in some cases be wildly different). Note that for (a) we don't need the ratio to be 1:1, the above reasoning will remain true for any proportion 1:n. For (b) however, if coin selection starts creating multiple outputs, and this number may vary, then losing the connection between outputs and change outputs will mean that that some outputs may pay a larger percentage of the fee (depending on how many change outputs the algorithm happened to choose).

edsko · 2018-11-19T08:36:16Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

+        -- 2/
+        -- We try to cover fee with the available change by substracting equally
+        -- across all inputs. There's no fairness in that in the case of a
+        -- multi-account transaction. Everyone pays the same part.


The "There's no fairness.." comment is not quite right, see my suggested comment above. Might want to refer back to that here.

edsko · 2018-11-19T08:40:26Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

-        let (change', fee') = reduceChangeOutputs fee (coinSelChange cs)
-        in (cs { coinSelChange = change' }, fee')
+
+-- we should have (x + (sum ls) = sum result), but this check could overflow.


splitChange needs a comment what it does. Note that it makes no attempt to divvy the new output proportionally over the change outputs. This means that if we happen to pick a very large UTxO entry, adding this evenly rather than proportionally might skew the payment:change ratio a lot. Could consider defining this in terms of divvy instead (but if you do, we need to be wary of lower bounds/upper bounds in the division calculation, since the requirements may not be the same for subtracting stuff and adding stuff.). Can be reconsidered later.

KtorZ · 2018-11-19T08:55:12Z

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

-    = round $ calculateTxSizeLinear linearFeePolicy
-            $ hi $ estimateSize boundAddrAttrSize boundTxAttrSize ins outs
+    = ceiling $ calculateTxSizeLinear linearFeePolicy
+              $ hi $ estimateSize boundAddrAttrSize boundTxAttrSize ins outs


Core nodes use ceiling in this computation and not round.

@duncan

The 'estimateCardanoFee' was using 'round' but as pointed out by @duncan, core nodes use 'ceiling' which may cause some divergence in the fee computation.

kderme

Simple dust filtering may fix the following issue: I had a wallet with 2 utxos: 1Ada and 10Lovelace. I tried to empty the wallet and the tx failed (not accepted on the network). This is the tx generated (note the output with amount = 0)
{"data":[
{
"id":"37bdc959ff2e35a85dd6abdb196b3431d46a5fb67ff2e3d1e197692188204661",
"confirmations":0,
"amount":1000010,"inputs":[{"address":"DdzFFzCqrhsnFXjLjbgFbZHmnwvexoQLXZkFQ9RQeGuNWsLEkUDFvyYZy1bBzfguya752AT9TbzXYXiVrYnU5CdXDzptuj3RA236SLY4","amount":10},{"address":"DdzFFzCqrhsnFXjLjbgFbZHmnwvexoQLXZkFQ9RQeGuNWsLEkUDFvyYZy1bBzfguya752AT9TbzXYXiVrYnU5CdXDzptuj3RA236SLY4","amount":1000000}],"outputs":[{"address":"DdzFFzCqrht7ZQTTrMXbvdQFt9k4FYJEwNdoRhucjiBCCQn2fGvt4Heu9CJCURPbD6c6ACKvTzw74o4uo2U1Wg2o785DoKuSPEArzLwv","amount":0},{"address":"DdzFFzCqrhsnryDPMxqHDS8zRe8Lrw1AtsXyJh25iZvt2tRzgtBKCdfBe2WUNCyUacuqFEykXKpALCY6eR3ENhxozg2mrE68iRUWXDTB","amount":819800}],"type":"foreign","direction":"outgoing","creationTime":"2018-11-19T10:21:55.06173","status":{"tag":"applying","data":{}}}

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

kderme

LGTM! I manually tested cases where multiple inputs are needed, cases where the wallet empties and combination of these cases. It always works ok.

kderme

LGTM! I manually tested cases where multiple inputs are needed, cases where the wallet empties and combination of these cases. It always works ok.

KtorZ · 2018-11-19T14:57:44Z

Merging this into release/2.0.0, will have another PR against develop which extends integration tests to reflect the scenarios fixed by this patch.

KtorZ added the release-2.0.0 label Nov 16, 2018

KtorZ force-pushed the KtorZ/RCD-45-RCD-44/review-fee-calculation branch from 1e88b75 to dc35732 Compare November 16, 2018 18:17

KtorZ requested review from kderme and edsko November 16, 2018 18:17

KtorZ force-pushed the KtorZ/RCD-45-RCD-44/review-fee-calculation branch from dc35732 to 02bd85a Compare November 16, 2018 18:18

KtorZ changed the title ~~Ktor z/rcd 45 rcd 44/review fee calculation~~ [RCD-45] & [RCD-44] Review fee calculation entirely Nov 16, 2018

KtorZ force-pushed the KtorZ/RCD-45-RCD-44/review-fee-calculation branch from 02bd85a to 2dbcd7a Compare November 16, 2018 23:41

KtorZ force-pushed the KtorZ/RCD-45-RCD-44/review-fee-calculation branch from 2dbcd7a to 01c16c7 Compare November 17, 2018 00:12

KtorZ commented Nov 17, 2018

View reviewed changes

KtorZ commented Nov 19, 2018

View reviewed changes

edsko approved these changes Nov 19, 2018

View reviewed changes

KtorZ commented Nov 19, 2018

View reviewed changes

fixup: Review comments & adjust estimateCardanoFee

b6c7208

The 'estimateCardanoFee' was using 'round' but as pointed out by @duncan, core nodes use 'ceiling' which may cause some divergence in the fee computation.

KtorZ force-pushed the KtorZ/RCD-45-RCD-44/review-fee-calculation branch from 4a32438 to b6c7208 Compare November 19, 2018 08:58

kderme suggested changes Nov 19, 2018

View reviewed changes

wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs Show resolved Hide resolved

Filter out dust change when reducing them

132e63d

kderme reviewed Nov 19, 2018

View reviewed changes

kderme approved these changes Nov 19, 2018

View reviewed changes

KtorZ merged commit 5c21b10 into release/2.0.0 Nov 19, 2018

KtorZ deleted the KtorZ/RCD-45-RCD-44/review-fee-calculation branch November 19, 2018 14:58

KtorZ mentioned this pull request Nov 20, 2018

[RCD-44] & [RCD-45] & [RCD-46] - Review fee calculation and Fix prefiltering not ignoring inputs' ids [develop] #3875

Merged

12 tasks

kderme mentioned this pull request Nov 20, 2018

[CBR-486] adjust fees when adding additional utxos for fees #3857

Closed

1 task

KtorZ mentioned this pull request Nov 21, 2018

Review Fee Calculation (Backport from Cardano-SL) input-output-hk/cardano-wallet-legacy#70

Merged

6 tasks

KtorZ mentioned this pull request Jan 4, 2019

Jc/merge/release 2.0.0/develop #3982

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RCD-45] & [RCD-44] Review fee calculation entirely #3861

[RCD-45] & [RCD-44] Review fee calculation entirely #3861

KtorZ commented Nov 16, 2018 •

edited

Loading

KtorZ Nov 16, 2018

KtorZ Nov 16, 2018

KtorZ Nov 16, 2018

KtorZ Nov 16, 2018

KtorZ Nov 17, 2018

KtorZ Nov 19, 2018

edsko left a comment

edsko Nov 19, 2018

edsko Nov 19, 2018

edsko Nov 19, 2018

KtorZ Nov 19, 2018

kderme left a comment

kderme left a comment •

edited

Loading

kderme left a comment

KtorZ commented Nov 19, 2018

[RCD-45] & [RCD-44] Review fee calculation entirely #3861

[RCD-45] & [RCD-44] Review fee calculation entirely #3861

Conversation

KtorZ commented Nov 16, 2018 • edited Loading

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

How to merge

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsko left a comment

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

Choose a reason for hiding this comment

kderme left a comment

Choose a reason for hiding this comment

kderme left a comment • edited Loading

Choose a reason for hiding this comment

kderme left a comment

Choose a reason for hiding this comment

KtorZ commented Nov 19, 2018

KtorZ commented Nov 16, 2018 •

edited

Loading

kderme left a comment •

edited

Loading