Skip to content

Commit

Permalink
Fix bug in burnSurplusAsFees where shortfall could be zero.
Browse files Browse the repository at this point in the history
Fixes issue #3242.

This bug caused the following property to sporadically fail:

    "distributeSurplusDelta, when no change output is present, will burn
    surplus as excess fees"

Example failure seeds:

    --seed 862218635
    --seed 1926926873
    --seed 404630283
    --seed 1602363838
    --seed 1472406660
    --seed 31096920
    --seed 166643919

Failures were always of the form:

```hs
       expected: Right (TxFeeAndChange {fee = Coin 1, change = []})
        but got: Left (ErrMoreSurplusNeeded (Coin 0))
```

This indicates that it was possible for `burnSurplusAsFees` to fail with
an indicated shortfall of zero.

To fix this, we update `burnSurplusAsFees` to only fail if the computed
shortfall is non-zero.

We also adjust the following property to check that failure cases are
covered, and to verify that the shortfall is correct:

`prop_distributeSurplusDelta_coversCostIncreaseAndConservesSurplus`
  • Loading branch information
jonathanknowles committed Apr 21, 2022
1 parent f5fbc6a commit e0ba91a
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 9 deletions.
11 changes: 6 additions & 5 deletions lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1609,13 +1609,14 @@ burnSurplusAsFees
-> Coin -- Surplus
-> TxFeeAndChange ()
-> Either ErrMoreSurplusNeeded (TxFeeAndChange ())
burnSurplusAsFees feePolicy surplus (TxFeeAndChange fee0 ()) =
case costOfBurningSurplus `Coin.subtract` surplus of
Just shortfall -> Left $ ErrMoreSurplusNeeded shortfall
Nothing ->
Right $ TxFeeAndChange surplus ()
burnSurplusAsFees feePolicy surplus (TxFeeAndChange fee0 ())
| shortfall > Coin 0 =
Left $ ErrMoreSurplusNeeded shortfall
| otherwise =
Right $ TxFeeAndChange surplus ()
where
costOfBurningSurplus = costOfIncreasingCoin feePolicy fee0 surplus
shortfall = costOfBurningSurplus `Coin.difference` surplus

-- | Estimates the final size of a transaction based on its skeleton.
--
Expand Down
16 changes: 12 additions & 4 deletions lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ import Crypto.Hash.Utils
import Data.ByteString
( ByteString )
import Data.Either
( isRight )
( isLeft, isRight )
import Data.Function
( on, (&) )
import Data.Functor.Identity
Expand Down Expand Up @@ -2409,10 +2409,18 @@ prop_distributeSurplusDelta_coversCostIncreaseAndConservesSurplus
:: FeePolicy -> Coin -> Coin -> [Coin] -> Property
prop_distributeSurplusDelta_coversCostIncreaseAndConservesSurplus
feePolicy surplus fee0 change0 =
checkCoverage $
cover 2 (isLeft mres) "Failure" $
cover 50 (isRight mres) "Success" $
report mres "Result" $
counterexample (show mres) $ case mres of
Left _ ->
label "unable to distribute surplus" $
property (surplus < (maxCoinCost <> maxCoinCost))
Left (ErrMoreSurplusNeeded shortfall) ->
conjoin
[ property $ surplus < (maxCoinCost <> maxCoinCost)
, property $ shortfall > Coin 0
, costOfIncreasingCoin feePolicy fee0 surplus
=== shortfall <> surplus
]
Right (TxFeeAndChange feeDelta changeDeltas) -> do
let feeRequirementIncrease = mconcat
[ costOfIncreasingCoin feePolicy fee0 feeDelta
Expand Down

0 comments on commit e0ba91a

Please sign in to comment.