Implement `sizeOfCoin` and `costOfIncreasingCoin` #3215

Anviking · 2022-04-04T10:37:23Z

Implement and test the following functions:

-- | Calculate the cost of increasing a CBOR-encoded Coin-value by another Coin
-- with the lovelace/byte cost given by the 'FeePolicy'.
--
-- Outputs values in the range of [0, 8 * perByteFee]
--
-- >>> let p = FeePolicy (Quantity 0) (Quantity 44)
--
-- >>> costOfIncreasingCoin p 4294967295 1
-- Coin 176 -- (9 bytes - 5 bytes) * 44 lovelace/byte
--
-- >>> costOfIncreasingCoin p 0 4294967296
-- Coin 352 -- 8 bytes * 44 lovelace/byte
costOfIncreasingCoin
    :: FeePolicy
    -> Coin -- ^ Original coin
    -> Coin -- ^ Increment
    -> Coin

-- | Calculate the size of a coin when encoded as CBOR.
sizeOfCoin :: Coin -> TxSize

Future work

The next PR will add a function to TransactionLayer with the following shape:

    , distributeSurplus
        :: FeePolicy
        -> Coin -- Surplus to distribute
        -> TxFeeAndChange
        -> Either Coin TxFeeAndChange

Issue number

ADP-1514

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

jonathanknowles

Hi @Anviking

I've made some general comments and suggestions. Will have another look in a bit!

lib/core/src/Cardano/Api/Gen.hs

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

jonathanknowles · 2022-04-06T05:14:16Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+        it "costs 176 lovelace to increase 4294967295 by one on mainnet" $ do
+            let feePolicy = LinearFee $ LinearFunction
+                    { intercept = 150_000, slope = 44 }
+            costOfIncreasingCoin feePolicy (Coin 4294967295) (Coin 1)


Suggestion: use an explicit power of 2 here.

Justification: it's now really obvious why this is a boundary. (Some readers of the code might not recognize that these values are related to the powers of 2.)

Suggested change

costOfIncreasingCoin feePolicy (Coin 4294967295) (Coin 1)

costOfIncreasingCoin feePolicy (Coin $ 2^32 - 1) (Coin 1)

#3215 (comment), however because of the test title I think it in particular makes sense to use 4294967295 here. I tweaked the title a bit: 1303e02

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

lib/core/src/Cardano/Api/Gen.hs

Anviking · 2022-04-06T15:14:15Z

@jonathanknowles about changing 4294967296 to 2^32 — we could do that. To me, however, it is much easier to correlate 4_294_967_296 and 65_536 with imagined scenarios on mainnet. For instance, the first could be a common output value, whereas the second should practically never be seen. This stands out less clearly when seeing 2^32 or 2^16.

Also I believe the real values are easier to correlate to failures from prop_balanceTransactionValid, which may look like:

( SealedTx
    ( InAnyCardanoEra AlonzoEra
        ( ShelleyTx ShelleyBasedEraAlonzo
            ( ValidatedTx
                { body = TxBodyConstr TxBodyRaw
                    { _inputs = fromList
                        [ TxInCompact
                            ( TxId
                                { _unTxId = SafeHash "01f4b788593d4f70de2a45c2e1e87088bfbdfa29577ae1b62aba60e095e3ab53" }
                            ) 1
                        ]
                    , _collateral = fromList []
                    , _outputs = StrictSeq
                        { fromStrict = fromList
                            [
                                ( Addr Mainnet
                                    ( ScriptHashObj
                                        ( ScriptHash "13a0e59ca61ab277e909ffd20def27d296051fbc634cf623c02322a5" )
                                    ) StakeRefNull
                                , Value 4295103487
                                    ( fromList [] )
                                , SNothing
                                )
                            ]
                        }
                    , _certs = StrictSeq
                        { fromStrict = fromList [] }
                    , _wdrls = Wdrl
                        { unWdrl = fromList [] }
                    , _txfee = Coin 164269
                    , _vldt = ValidityInterval
                        { invalidBefore = SJust
                            ( SlotNo 1 )
                        , invalidHereafter = SJust
                            ( SlotNo 1 )
                        }

So in my mind "why are these values magic" is less important than "these are the magic values", although I admit there's some neatness to #3215 (comment). What do you think?

This improves coverage of boundary cases in sizeOfCoin properties.

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

Use 4-space indents, and put exports into alphabetical order.

This commit also uses these named constants in `genLovelace` to replace hard-coded values. If a developer is new to the codebase, they can understand the choice of these constants by navigating to their definitions and reading the comments.

Since `Lovelace` is an instance of `Num` and `Random`, we can avoid wrapping and unwrapping `Lovelace` values when using `choose`.

…tants.

In the implementation of `sizeOfCoin`, we use hard-coded constants for powers of two, which avoids computing these values on each call. However, in the test code, we can be even clearer about our intent, by explicitly stating which powers of two we're interested in. For someone new to the codebase, being able to see the relationship with the powers of two is arguably helpful for being able to comprehend why we're testing these particular values. At a glance, it's arguably quite hard to see the the difference between: - 4294967295 - 4294967296 But it's very easy to see the difference between: - 2^32 - 1 - 2^32

In this commit, we also remove the expected cost increase from the name of the test. Otherwise, if someone updates this constant in future, it's possible that they'll forget to update the name of the test case, and the compiler won't tell them.

- Lead with "[it] costs" - Clearly specify the ≈4.29 ada value in the title

jonathanknowles

Nice work! 👍🏻

jonathanknowles · 2022-04-08T02:08:24Z

bors r+

iohk-bors · 2022-04-08T02:57:24Z

Build succeeded:

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

Anviking changed the title ~~Add sizeOfCoin and costOfIncreasingCoin~~ Implement sizeOfCoin and costOfIncreasingCoin Apr 4, 2022

Anviking self-assigned this Apr 4, 2022

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from fe581ab to 0a3e29c Compare April 4, 2022 10:44

Anviking commented Apr 4, 2022

View reviewed changes

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from 1346eab to b13793b Compare April 4, 2022 10:47

Anviking requested a review from jonathanknowles April 4, 2022 10:48

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch 2 times, most recently from 3eea663 to b17b6a8 Compare April 4, 2022 14:13

jonathanknowles reviewed Apr 6, 2022

View reviewed changes

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from d496b19 to 98a73ec Compare April 6, 2022 14:25

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from 21e87d7 to 1303e02 Compare April 6, 2022 15:14

Anviking mentioned this pull request Apr 6, 2022

Implement distributeSurplus and use in balanceTransaction #3223

Merged

5 tasks

jonathanknowles mentioned this pull request Apr 7, 2022

Suggestions for PR #3215 #3224

Merged

Anviking and others added 16 commits April 7, 2022 16:01

Add sizeOfCoin function

f841fbc

Implement costOfIncreasingCoin

f4ed245

Tweak genEncodingBoundaryLovelace for greater coverage

a3801c4

This improves coverage of boundary cases in sizeOfCoin properties.

fixup: remove redundant parenthesis

300066c

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

fixup: use ceiling rather than round for fee

555c91b

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

Use NumericUnderscores and right-alignment in sizeOfCoin

d9a1331

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

fixup: adjust wording in costOfIncreasingCoin test

12525b0

Correct indentation for exports of Cardano.Api.Gen.

b6994c7

Use 4-space indents, and put exports into alphabetical order.

Remove now-redundant use of Lovelace constructor.

9b0697c

Since `Lovelace` is an instance of `Num` and `Random`, we can avoid wrapping and unwrapping `Lovelace` values when using `choose`.

Use statically-checked intCast to replace fromIntegral.

fb51e44

Simplify generator by moving the location of a type application.

1c7346c

Use NumericUnderscores and add comments to inline power-of-two cons…

f04f993

…tants.

Add comments to sizeOfCoin explaining the choices of constants.

03dee0b

Add monomorphic function power.

7642fa1

jonathanknowles and others added 3 commits April 7, 2022 16:01

Remove redundant comment in sizeOfCoin

122c5ae

Tweak title of costOfIncreasingCoin test

ab82b9d

- Lead with "[it] costs" - Clearly specify the ≈4.29 ada value in the title

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from a285cec to ab82b9d Compare April 7, 2022 14:04

Anviking requested a review from jonathanknowles April 7, 2022 18:51

jonathanknowles approved these changes Apr 8, 2022

View reviewed changes

iohk-bors bot merged commit a854531 into master Apr 8, 2022

iohk-bors bot deleted the anviking/ADP-1514/costOfIncreasingCoin branch April 8, 2022 02:57

WilliamKingNoel-Bot pushed a commit that referenced this pull request Apr 8, 2022

docs: Merge #3215

a79d3bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `sizeOfCoin` and `costOfIncreasingCoin` #3215

Implement `sizeOfCoin` and `costOfIncreasingCoin` #3215

Anviking commented Apr 4, 2022 •

edited

Loading

jonathanknowles left a comment

jonathanknowles Apr 6, 2022

Anviking Apr 6, 2022

Anviking commented Apr 6, 2022

jonathanknowles left a comment

jonathanknowles commented Apr 8, 2022

iohk-bors bot commented Apr 8, 2022

	costOfIncreasingCoin feePolicy (Coin 4294967295) (Coin 1)
	costOfIncreasingCoin feePolicy (Coin $ 2^32 - 1) (Coin 1)

Implement sizeOfCoin and costOfIncreasingCoin #3215

Implement sizeOfCoin and costOfIncreasingCoin #3215

Conversation

Anviking commented Apr 4, 2022 • edited Loading

Future work

Issue number

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles Apr 6, 2022

Choose a reason for hiding this comment

Anviking Apr 6, 2022

Choose a reason for hiding this comment

Anviking commented Apr 6, 2022

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles commented Apr 8, 2022

iohk-bors bot commented Apr 8, 2022

Implement `sizeOfCoin` and `costOfIncreasingCoin` #3215

Implement `sizeOfCoin` and `costOfIncreasingCoin` #3215

Anviking commented Apr 4, 2022 •

edited

Loading