Add prop_balanceTransactionBalanced #2996

Anviking · 2021-11-01T20:37:21Z

Overview

Add preconditions to balanceTransaction such that we fail instead of producing imbalanced results...
- when there are withdrawals with different network tags
- when there are deposits or refunds (pool reg, stake key reg/dereg) (apparently not pool retirement)
- when collateral is pre-selected
- when we underestimate the fee (likely related to variable length coin encoding and the padding)
Tweak generators in Cardano.Api.Gen
Add (pending) prop_balanceTransactionBalanced
- Decent shrinking and failure-presentation
- Used to discover the preconditions above
- Generator coverages of some corner cases are missing
  - empty txIns
  - empty txOuts
  - gens never cause balanceTransaction to select collateral for some reason (!)
  - PParams are currently static
- In-the-end marked pending because of a new variable-length-coin-encoding corner cases

Comments

The problems we theorised during the coarse first implementation are indeed caught by the property (collateral, fee encoding corner cases, deposits), which is nice. Discovering additional problems was nice too.

While the coverage is still not ideal, I believe it's good enough to give re-writing the implementation a go.

Issue Number

ADP-1214

lib/core/src/Cardano/Wallet/Primitive/Types/Coin/Gen.hs

jonathanknowles · 2021-11-05T03:47:55Z

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

        { getFeePolicy = mockFeePolicy
        , getTxMaxSize = Quantity 16384
        , getTokenBundleMaxSize = TokenBundleMaxSize $ TxSize 4000
        , getMaxExecutionUnits = ExecutionUnits 10000000 10000000000
        }
+    , minimumUTxOvalue = MinimumUTxOValue $ Coin 1000000


Suggestion: use NumericUnderscores for large values such as these.

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

Anviking · 2021-12-01T09:29:54Z

lib/core/test/unit/Cardano/Api/GenSpec.hs

@@ -433,7 +432,7 @@ instance Arbitrary SlotNo where
    arbitrary = genSlotNo

 genLovelaceCoverage :: Lovelace -> Property
-genLovelaceCoverage = unsignedCoverage (maxBound @Word16) "lovelace"
+genLovelaceCoverage = unsignedCoverage (maxBound @Word32) "lovelace"


Lovelace is unbounded — is having to pass in a Word32 bound weird?

This is just bad UX on my part. You don't need to pass in the maxBound of the unsigned you are testing the coverage of, but you do need to pass in some value that the test can use to delineate between "large" and "small" values (with things above the value being considered large).

I'm not really happy with this code.

Anviking · 2021-12-01T09:39:33Z

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

+                    $ classify (txFee sealedTx > Cardano.Lovelace 1_000_000)
+                        "fee above 1 ada"
+                    (txBalance sealedTx combinedUTxO === 0)
+                    .&&. (abs (txFee sealedTx) .<= 4_000_000)


TODO: Test fee >= minFee but also not too much of an over-estimate. Perhaps another property (and definitely another PR/ticket)

Anviking · 2021-12-03T19:26:25Z

bors try

iohk-bors · 2021-12-03T19:37:30Z

try

Build failed:

ci/hydra-build:required

Anviking · 2021-12-06T14:02:30Z

bors try

iohk-bors · 2021-12-06T14:14:37Z

try

Build failed:

ci/hydra-build:required

lib/core/src/Cardano/Wallet.hs

Anviking · 2021-12-08T13:14:01Z

bors try

iohk-bors · 2021-12-08T13:31:08Z

try

Build failed:

buildkite/cardano-wallet

Anviking · 2021-12-08T15:21:18Z

bors try

sevanspowell · 2021-12-10T06:36:00Z

Thanks for your work on this Johannes 🎉 I think I'm ok with this, as long as my understanding of it is correct: This is a first step in fixing bugs with the current balanceTransaction implementation.

I'm not sure how I feel about the coverage of the generators (which is my fault too!), but that's unimportant I think. The important thing is that the generators and the test have found issues, and we should use these going forward to fix the implementation of balance transaction.

I have one unimportant suggestion here: #3052, but otherwise, looks good!

Using a not-yet-committed prop_balanceTransactionBalanced it was discovered that balanceTransaction would produce imbalanced transactions under several scenarios. The guards/preconditions added here ensure the limitations are clear to users who run into them. It also makes the limitations clearly known and documented, allowing us to come up with an appropriate strategy to fix.

- Tests that balanceTransaction produces balanced results - Decent shrinking and failure-presentation - Used to discover the preconditions added in a prev commit - Generator coverages of some corner cases are missing - empty txIns - empty txOuts - gens never cause balanceTransaction to select collateral for some reason (!) - PParams are currently static - In-the-end marked pending because of a new variable-length-coin-encoding corner cases

- Make the semantics of "Maybe Coin" more obvious by using a descriptive data type.

Co-authored-by: Samuel Evans-Powell <[email protected]>

Anviking · 2021-12-10T18:50:31Z

Thanks! Yes, this is a first step!

bors r+

2996: Add prop_balanceTransactionBalanced r=Anviking a=Anviking ## Overview - Add preconditions to `balanceTransaction` such that we fail instead of producing imbalanced results... - when there are withdrawals with different network tags - when there are deposits or refunds (pool reg, stake key reg/dereg) (apparently not pool retirement) - when collateral is pre-selected - when we underestimate the fee (likely related to variable length coin encoding and the padding) - Tweak generators in `Cardano.Api.Gen` - Add (pending) `prop_balanceTransactionBalanced` - Decent shrinking and failure-presentation - Used to discover the preconditions above - Generator coverages of some corner cases are missing - empty txIns - empty txOuts - gens never cause balanceTransaction to select collateral for some reason (!) - PParams are currently static - In-the-end marked pending because of a new variable-length-coin-encoding corner cases ## Comments The problems we theorised during the coarse first implementation are indeed caught by the property (collateral, fee encoding corner cases, deposits), which is nice. Discovering additional problems was nice too. While the coverage is still not ideal, I believe it's good enough to give re-writing the implementation a go. ### Issue Number ADP-1214  Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-12-10T19:21:01Z

Build failed:

ci/hydra-build:required

Anviking · 2021-12-10T20:02:27Z

bors r+

2996: Add prop_balanceTransactionBalanced r=Anviking a=Anviking ## Overview - Add preconditions to `balanceTransaction` such that we fail instead of producing imbalanced results... - when there are withdrawals with different network tags - when there are deposits or refunds (pool reg, stake key reg/dereg) (apparently not pool retirement) - when collateral is pre-selected - when we underestimate the fee (likely related to variable length coin encoding and the padding) - Tweak generators in `Cardano.Api.Gen` - Add (pending) `prop_balanceTransactionBalanced` - Decent shrinking and failure-presentation - Used to discover the preconditions above - Generator coverages of some corner cases are missing - empty txIns - empty txOuts - gens never cause balanceTransaction to select collateral for some reason (!) - PParams are currently static - In-the-end marked pending because of a new variable-length-coin-encoding corner cases ## Comments The problems we theorised during the coarse first implementation are indeed caught by the property (collateral, fee encoding corner cases, deposits), which is nice. Discovering additional problems was nice too. While the coverage is still not ideal, I believe it's good enough to give re-writing the implementation a go. ### Issue Number ADP-1214  Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-12-10T20:40:30Z

Build failed:

buildkite/cardano-wallet

Due to renaming of cardano-ledger-specs repo

Anviking · 2021-12-14T12:07:24Z

bors r+

2996: Add prop_balanceTransactionBalanced r=Anviking a=Anviking ## Overview - Add preconditions to `balanceTransaction` such that we fail instead of producing imbalanced results... - when there are withdrawals with different network tags - when there are deposits or refunds (pool reg, stake key reg/dereg) (apparently not pool retirement) - when collateral is pre-selected - when we underestimate the fee (likely related to variable length coin encoding and the padding) - Tweak generators in `Cardano.Api.Gen` - Add (pending) `prop_balanceTransactionBalanced` - Decent shrinking and failure-presentation - Used to discover the preconditions above - Generator coverages of some corner cases are missing - empty txIns - empty txOuts - gens never cause balanceTransaction to select collateral for some reason (!) - PParams are currently static - In-the-end marked pending because of a new variable-length-coin-encoding corner cases ## Comments The problems we theorised during the coarse first implementation are indeed caught by the property (collateral, fee encoding corner cases, deposits), which is nice. Discovering additional problems was nice too. While the coverage is still not ideal, I believe it's good enough to give re-writing the implementation a go. ### Issue Number ADP-1214  Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-12-14T12:22:47Z

Build failed:

ci/hydra-build:required

[ 77 of 106] Compiling Data.Time.Text   ( src/Data/Time/Text.hs, dist/build/Data/Time/Text.o, dist/build/Data/Time/Text.dyn_o )
[ 78 of 106] Compiling Cardano.Wallet.Api.Types ( src/Cardano/Wallet/Api/Types.hs, dist/build/Cardano/Wallet/Api/Types.o, dist/build/Cardano/Wallet/Api/Types.dyn_o )
/nix/store/7gdji02y6rbnzv5mcxcf3mwksk785y8j-stdenv-linux/setup: line 1311:   119 Killed                  $SETUP_HS build lib:cardano-wallet-core -j$(($NIX_BUILD_CORES > 4 ? 4 : $NIX_BUILD_CORES))

Anviking · 2021-12-14T12:24:12Z

bors r+

2996: Add prop_balanceTransactionBalanced r=Anviking a=Anviking ## Overview - Add preconditions to `balanceTransaction` such that we fail instead of producing imbalanced results... - when there are withdrawals with different network tags - when there are deposits or refunds (pool reg, stake key reg/dereg) (apparently not pool retirement) - when collateral is pre-selected - when we underestimate the fee (likely related to variable length coin encoding and the padding) - Tweak generators in `Cardano.Api.Gen` - Add (pending) `prop_balanceTransactionBalanced` - Decent shrinking and failure-presentation - Used to discover the preconditions above - Generator coverages of some corner cases are missing - empty txIns - empty txOuts - gens never cause balanceTransaction to select collateral for some reason (!) - PParams are currently static - In-the-end marked pending because of a new variable-length-coin-encoding corner cases ## Comments The problems we theorised during the coarse first implementation are indeed caught by the property (collateral, fee encoding corner cases, deposits), which is nice. Discovering additional problems was nice too. While the coverage is still not ideal, I believe it's good enough to give re-writing the implementation a go. ### Issue Number ADP-1214  Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-12-14T12:32:01Z

Build failed:

ci/hydra-build:required

Anviking · 2021-12-14T13:09:07Z

bors r+

iohk-bors · 2021-12-14T13:53:37Z

Build succeeded:

Anviking self-assigned this Nov 1, 2021

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch from 2b6baeb to 0bb51d6 Compare November 4, 2021 16:19

jonathanknowles reviewed Nov 5, 2021

View reviewed changes

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch 3 times, most recently from 87bc8ff to 12e2320 Compare November 16, 2021 19:54

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch 3 times, most recently from 8564d8e to 8abd7a1 Compare November 30, 2021 20:14

jonathanknowles reviewed Dec 1, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Show resolved Hide resolved

Anviking commented Dec 1, 2021

View reviewed changes

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch from 9ee89ad to 71865b0 Compare December 2, 2021 16:49

iohk-bors bot added a commit that referenced this pull request Dec 3, 2021

Try #2996:

647339c

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch 2 times, most recently from 47ef500 to de9b123 Compare December 6, 2021 13:14

iohk-bors bot added a commit that referenced this pull request Dec 6, 2021

Try #2996:

d7d4059

Anviking commented Dec 6, 2021

View reviewed changes

lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch 2 times, most recently from dcd74d5 to f154313 Compare December 8, 2021 13:13

iohk-bors bot added a commit that referenced this pull request Dec 8, 2021

Try #2996:

1a608c5

Anviking changed the title ~~WIP: Stub prop_balanceTx~~ Add prop_balanceTransactionBalanced Dec 8, 2021

Anviking marked this pull request as ready for review December 8, 2021 13:32

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch from f154313 to 7854206 Compare December 8, 2021 14:55

sevanspowell self-requested a review December 10, 2021 09:14

sevanspowell approved these changes Dec 10, 2021

View reviewed changes

Anviking and others added 6 commits December 10, 2021 19:36

Add (Buildable PartialTx) instance

d5fb134

Workaround pointless conversion in updateSealedTx

9e991b4

Add Shelley.Compatibility.fromCardanoTxOut

1c7d64f

Improve Cardano.Api generators

d6671c3

Further tweaks to Cardano.Api.Gen

1951383

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch from 34c3640 to 32bc654 Compare December 10, 2021 18:37

Anviking and others added 3 commits December 10, 2021 19:38

Change fee update rep from "Maybe Coin" to explicit data type

f2bf909

- Make the semantics of "Maybe Coin" more obvious by using a descriptive data type.

Polish and review suggestions

f5bf2a1

Co-authored-by: Samuel Evans-Powell <[email protected]>

Anviking force-pushed the anviking/ADP-1214/prop_balanceTx branch from 32bc654 to f5bf2a1 Compare December 10, 2021 18:40

iohk-bors bot merged commit bc866ee into master Dec 14, 2021

iohk-bors bot deleted the anviking/ADP-1214/prop_balanceTx branch December 14, 2021 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prop_balanceTransactionBalanced #2996

Add prop_balanceTransactionBalanced #2996

Anviking commented Nov 1, 2021 •

edited

Loading

jonathanknowles Nov 5, 2021

Anviking Dec 1, 2021 •

edited

Loading

sevanspowell Dec 9, 2021

Anviking Dec 1, 2021 •

edited

Loading

Anviking commented Dec 3, 2021

iohk-bors bot commented Dec 3, 2021

Anviking commented Dec 6, 2021

iohk-bors bot commented Dec 6, 2021

Anviking commented Dec 8, 2021

iohk-bors bot commented Dec 8, 2021

Anviking commented Dec 8, 2021

sevanspowell commented Dec 10, 2021

Anviking commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021

Anviking commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021 •

edited by Anviking

Loading

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021 •

edited by Anviking

Loading

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021

Add prop_balanceTransactionBalanced #2996

Add prop_balanceTransactionBalanced #2996

Conversation

Anviking commented Nov 1, 2021 • edited Loading

Overview

Comments

Issue Number

jonathanknowles Nov 5, 2021

Choose a reason for hiding this comment

Anviking Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

sevanspowell Dec 9, 2021

Choose a reason for hiding this comment

Anviking Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Anviking commented Dec 3, 2021

iohk-bors bot commented Dec 3, 2021

try

Anviking commented Dec 6, 2021

iohk-bors bot commented Dec 6, 2021

try

Anviking commented Dec 8, 2021

iohk-bors bot commented Dec 8, 2021

try

Anviking commented Dec 8, 2021

sevanspowell commented Dec 10, 2021

Anviking commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021

Anviking commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021 • edited by Anviking Loading

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021 • edited by Anviking Loading

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021

Anviking commented Dec 14, 2021

iohk-bors bot commented Dec 14, 2021

Anviking commented Nov 1, 2021 •

edited

Loading

Anviking Dec 1, 2021 •

edited

Loading

Anviking Dec 1, 2021 •

edited

Loading

iohk-bors bot commented Dec 10, 2021 •

edited by Anviking

Loading

iohk-bors bot commented Dec 14, 2021 •

edited by Anviking

Loading