Review Fee Sanity Check #199

KtorZ · 2019-01-04T12:57:52Z

Overview

I have move the fee sanity check inside the adjustForFees method such that it's actually ran for each CoinSelection and is triggered by the appropriate tests
I have removed some tests introduced in cardano-sl #3815 as they're now redundant with the ones done in the CoinSelection
I have removed the feeSanityCheck option from the CoinSelectionOption

Comments

KtorZ · 2019-01-04T12:58:59Z

src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

-        maxCoeff :: Int = 2
-        minFees = Core.mkCoin $ floor $ txSizeLinearMinValue linearFeePolicy
-    in
-        (fees >= minFees) && (fees <= Core.unsafeMulCoin minFees maxCoeff)


So, this was the absolute check checking a value against a particular config, regardless of the underlying transaction. Now removed in favor of a relative one.

KtorZ · 2019-01-04T12:59:31Z

src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs

      csoEstimateFee       = estimateFee
-    , csoFeesSanityCheck   = check


The check is now "hardcoded" in the form of an invariant in the generic code.

KtorZ · 2019-01-04T13:07:08Z

src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs

+    let actualFee = getFee $ computeFee inps' outs' chgs'
+    if actualFee `valueDiv` 2 > estimatedFee then -- Using `valueDiv` instead of `valueMul` to avoid overflow
+        error $ "adjustForFees: fee out of bounds: " <> pretty actualFee <> " while expecting ~" <> pretty estimatedFee
+    else do


So this is the new check that should never be triggered. It's not an exception because that's not something we want to communicate to user in theory.
Testing ensure that this is never triggered. Instead of comparing to an absolute value, we control that our actual fees are not unrealistically big.

is it safe to skip minimum bound check? What if fees are too small? (the thing we were testing here https://github.com/input-output-hk/cardano-wallet/pull/199/files#diff-393909f5cc8109550702f2c76a1b4afdL405 ). Is it safe to assume that fees won't be less then estimated fees?

Safe for the users that's for sure. Safe for us, that's debatable. But indeed, the estimatedFee are actually a minimal fees so we are quite guaranteed that the actual one are at least equal, if not bigger.
I'll add the min bound check, oversight from me 👍

KtorZ · 2019-01-04T13:07:55Z

src/Cardano/Wallet/Kernel/Transactions.hs

    let fees = computeFeesOfUnsignedTx tx
-    if csoFeesSanityCheck options fees
-    then return (snapshot, tx, fees, availableUtxo)
-    else error $ "fees out of bound " <> show fees


So this was move into the generic coin selection algorithm.

except we don't check minimum bound (https://github.com/input-output-hk/cardano-wallet/pull/199/files#r245325794) which might be ok?

EDIT: already answered cardano-foundation/cardano-wallet#199 (comment)

KtorZ · 2019-01-04T13:19:48Z

test/unit/Test/Spec/CoinSelection.hs

-minFee _ _ = Core.mkCoin 1
-
-minFeeCheck :: Core.Coin -> Bool
-minFeeCheck c = c == Core.mkCoin 1


The checker weren't ran before so we never noticed but assuming that actual fee will be 1 when the fee estimator was also returning 1 was wrong. This works fine when there's only one payee, but because fee gets distributed "proportionnally" over payees, when multiple payees, we end up splitting 1 lovelace (which is the smallest) value we can have across multiple entries. Resulting in each of them rounding up to 1.

So in the end, when fees are at the smallest, our algorithm makes each payees pay for them. Since this is quite an unrealistic scenario (we won't ever have a policy that charges only 1 lovelace per transaction...) I haven't tried to "fix" the fee adjustment algorithm. Instead, falling back to a constant fee that is big enough to allow for fee to be divvied across multiple payees (note that the generator generates usually between 1 and 10 payees).

KtorZ · 2019-01-04T13:22:13Z

test/unit/Test/Spec/CoinSelection.hs

                ) $ \(utxo, payee, res) ->
                  paymentSucceededWith utxo payee res [feeWasPayed SenderPaysFee]
            prop "multiple payees, SenderPaysFee, fee = cardano" $ \pm -> forAll (
-                payBatch pm cardanoFee cardanoFeeCheck identity (InitialADA 1000) (PayADA 100) random
+                pay pm genFragmentedUtxo genPayees cardanoFee identity (InitialADA 1000) (PayADA 100) random


So here's the main "trick" here to highlight the scenario we had were multiple payees would generate too many fees. By using a different generator (payBatch relied on genUtxoWithAtLeast which generate around 10 payees at most), we end up generating more payees for each transaction (got up to 500) and I was able to successfully trigger the bug.
Now that this is now fixed with the relative comparison (estimated fees vs actual fees), even with loads of payees.

KtorZ · 2019-01-04T13:23:01Z

test/unit/Test/Spec/CoinSelection/Generators.hs

+                , stakeMaxEntries       = Just 1000
+                , fiddlyAddresses       = False
+                , allowRedeemAddresses  = False
+            }


Making the UTxO more fragmented here forces more inputs to be selected and more payees to be generated which serves our purpose for testing edge cases of fee sanity checking.

Running a test on cardano-foundation/cardano-wallet@22129c0:

Coin selection policies unit tests random multiple payees, SenderPaysFee, fee = cardano FAILED [1] Fiddly Addresses multiple payees, SenderPaysFee, fee = cardano +++ OK, passed 5 tests. Failures: test/unit/Test/Spec/CoinSelection.hs:666:13: 1) Coin selection policies unit tests, random, multiple payees, SenderPaysFee, fee = cardano uncaught exception: ErrorCall fee out of boundCoin {getCoin = 1072341} CallStack (from HasCallStack): error, called at src/Universum/Debug.hs:60:11 in universum-1.2.0-2E8EL6k5Fv214PFWOWwkIj:Universum.Debug error, called at src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs:244:14 in cardano-wallet-2.0.0-H4X2696IYufKTiT2WKvm3c:Cardano.Wallet.Kernel.CoinSelection.FromGeneric (after 1 test) ProtocolMagic {getProtocolMagicId = ProtocolMagicId {unProtocolMagicId = 1}, getRequiresNetworkMagic = RequiresNoMagic} Exception thrown while showing test case: fee out of boundCoin {getCoin = 1072341} CallStack (from HasCallStack): error, called at src/Universum/Debug.hs:60:11 in universum-1.2.0-2E8EL6k5Fv214PFWOWwkIj:Universum.Debug error, called at src/Cardano/Wallet/Kernel/CoinSelection/FromGeneric.hs:244:14 in cardano-wallet-2.0.0-H4X2696IYufKTiT2WKvm3c:Cardano.Wallet.Kernel.CoinSelection.FromGeneric

convinced me its a superset of old property https://github.com/input-output-hk/cardano-wallet/pull/199/files#diff-13c14a11ea8258a566fc79e3b7ed54c9L149

KtorZ · 2019-01-04T13:24:04Z

test/unit/Test/Spec/GetTransactions.hs

@@ -146,62 +144,6 @@ prepareFixtures nm initialBalance = do
            , fixturePw = pw
        }



Code and tests below were introduced in input-output-hk/cardano-sl#3815 and are now completely redundant with the one in Spec/CoinSelection

akegalj

LGTM

Remaining things to do before approving:

https://github.com/input-output-hk/cardano-wallet/pull/199/files#r245327717

The strategy here is: - 1/ To actually test the fee sanity check :( ... - 2/ To create a more fragmented UTxO such that more inputs are needed, increasing the overall fees

The main idea here is to relocate this check to be done during fee calculation. After adjusting fees, we simply verify that actual fees are somewhere close to the original estimation. If not, we throw hard and call that an invariant violation. This is a security measure as we do not expect such deviation from the original estimation. We may have to pick some extra inputs to cover for fees but if done in such way that the fees are more than double, we should better run the fee calculation on a different selection. This commits also removes some tests from the Spec/GetTransactions module that are now redundant with the checks occuring in Spec/CoinSelection.

3993: [CO-446] Fix Fee Sanity Check r=KtorZ a=KtorZ ## Description  When solving CSL-2526, we introduced an extra sanity check to make the API fails if it was to generate a transaction with too many fees (to prevent us from creating illed transactions because of a programming error in the coin selection / fee calculation). Yet, it turns out this check is a bit too strict / wrong for it compare transaction fees to an absolute value. Therefore, some transactions with many inputs and, actually expensive fees, fail to be created despite being totally valid. Backport of [cardano-wallet #199](cardano-foundation/cardano-wallet#199) ## Linked issue  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

3993: [CO-446] Fix Fee Sanity Check r=rvl a=KtorZ ## Description  When solving CSL-2526, we introduced an extra sanity check to make the API fails if it was to generate a transaction with too many fees (to prevent us from creating illed transactions because of a programming error in the coin selection / fee calculation). Yet, it turns out this check is a bit too strict / wrong for it compare transaction fees to an absolute value. Therefore, some transactions with many inputs and, actually expensive fees, fail to be created despite being totally valid. Backport of [cardano-wallet #199](cardano-foundation/cardano-wallet#199) ## Linked issue  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

KtorZ requested a review from akegalj January 4, 2019 12:57

KtorZ changed the title ~~Ktor z/bug fee sanity check~~ Review Fee Sanity Check Jan 4, 2019

KtorZ commented Jan 4, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/bug-fee-sanity-check branch from f2c85aa to e8a2503 Compare January 4, 2019 13:06

KtorZ commented Jan 4, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/bug-fee-sanity-check branch from e8a2503 to 450bfab Compare January 4, 2019 13:19

KtorZ commented Jan 4, 2019

View reviewed changes

akegalj reviewed Jan 4, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/bug-fee-sanity-check branch from 450bfab to 384cc6d Compare January 4, 2019 17:29

KtorZ added 2 commits January 4, 2019 18:39

Make CoinSelection unit test fail

e9ffd30

The strategy here is: - 1/ To actually test the fee sanity check :( ... - 2/ To create a more fragmented UTxO such that more inputs are needed, increasing the overall fees

KtorZ force-pushed the KtorZ/bug-fee-sanity-check branch from 384cc6d to 172a198 Compare January 4, 2019 17:40

akegalj approved these changes Jan 4, 2019

View reviewed changes

parsonsmatt approved these changes Jan 4, 2019

View reviewed changes

KtorZ merged commit 7077764 into develop Jan 4, 2019

KtorZ deleted the KtorZ/bug-fee-sanity-check branch January 4, 2019 18:53

KtorZ mentioned this pull request Jan 7, 2019

[CO-446] Fix Fee Sanity Check input-output-hk/cardano-sl#3993

Merged

12 tasks

KtorZ mentioned this pull request Jan 7, 2019

Fee sanity check is triggered more than it should #191

Closed

3 tasks

KtorZ self-assigned this Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review Fee Sanity Check #199

Review Fee Sanity Check #199

KtorZ commented Jan 4, 2019

KtorZ Jan 4, 2019

KtorZ Jan 4, 2019 •

edited

Loading

KtorZ Jan 4, 2019

akegalj Jan 4, 2019

KtorZ Jan 4, 2019

KtorZ Jan 4, 2019

akegalj Jan 4, 2019 •

edited

Loading

KtorZ Jan 4, 2019 •

edited

Loading

KtorZ Jan 4, 2019

KtorZ Jan 4, 2019

akegalj Jan 4, 2019 •

edited

Loading

KtorZ Jan 4, 2019

akegalj left a comment

		@@ -146,62 +144,6 @@ prepareFixtures nm initialBalance = do
		, fixturePw = pw
		}

Review Fee Sanity Check #199

Review Fee Sanity Check #199

Conversation

KtorZ commented Jan 4, 2019

Overview

Comments

Choose a reason for hiding this comment

KtorZ Jan 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

Choose a reason for hiding this comment

akegalj Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Jan 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

akegalj Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj left a comment

Choose a reason for hiding this comment

KtorZ Jan 4, 2019 •

edited

Loading

akegalj Jan 4, 2019 •

edited

Loading

KtorZ Jan 4, 2019 •

edited

Loading

akegalj Jan 4, 2019 •

edited

Loading