Fix `prop_{create,extend}` in `SelectionSpec`. #2632

jonathanknowles · 2021-04-30T07:30:16Z

Issue Number

#2630 / ADP-898

Summary

This PR fixes the following flaky tests:

Migration.SelectionSpec.prop_create
Migration.SelectionSpec.prop_extend

These tests would both fail around 2% of the time, for the same underlying reason.

Testing

This PR was tested by running the entire SelectionSpec 1000 times. No failures were observed.

Analysis

Both Selection.create and Selection.extend are required to produce a balanced Selection where the fee (and fee excess) is minimized.

Both of these functions rely on minimizeFee: this function is required to be idempotent, so that:

∀ s. minimizeFee (minimizeFee s) == minimizeFee s.

To check that create and extend produce selections with minimal fees, it's sufficient to verify that the selections they return have fees that cannot be minimized further. To do this, our pre-existing property tests already call minimizeFee and verify that the result does not change:

    verify (feeExcess selection == feeExcessExpected)
  where
    (feeExcessExpected, _) =
        minimizeFee constraints (feeExcess selection, outputs selection)

However, in addition to this check, we were needlessly asserting that balance should be idempotent. The balance function is not actually designed for this: it's designed to always be called with outputs that are minimal in their ada quantities.

Changes

This PR:

removes the unnecessary and invalid assertion for the idempotency of balance.
makes the pre-condition for balance explicit with a documentation comment.
adds an explicit idempotency test for minimizeFee.
improves error reporting in case of test failures, by displaying the actual fee excess.

Running `minimizeFee` a second time should not change the fee excess.

For any selection created by `create` or `extend`, we require that the fee excess cannot be minimized further. So we check the following condition: >>> feeExcess == feeExcessExpected If this condition is ever violated in a test, it's important to know what the actual value of `feeExcess` was. (The value of `feeExcessExpected` is calculated by attempting to minimize the fee excess a second time: this should have no effect, since `minimizeFee` is designed to be idempotent.)

These checks are not only unnecessary, they are incorrect. The original motivation for including them was to check that `create` and `extend` actually do minimize the fee (rather than leaving it unbalanced). But this can be adequately checked by verifying that the fee excess cannot be minimized further, i.e., that `minimizeFee` has no effect, which we already do. Furthermore, these checks are incorrect, as `balance` expects to be provided with outputs with ada quantities that are minimal.

The `balance` function expects to be given outputs whose ada quantities are minimal.

paweljakubas

👍

jonathanknowles · 2021-04-30T08:22:52Z

bors r+

2632: Fix `prop_{create,extend}` in `SelectionSpec`. r=jonathanknowles a=jonathanknowles # Issue Number #2630 / ADP-898 # Summary This PR fixes the following flaky tests: - `Migration.SelectionSpec.prop_create` - `Migration.SelectionSpec.prop_extend` These tests would both fail around 2% of the time, for the same underlying reason. # Testing This PR was tested by running the entire `SelectionSpec` **1000** times. No failures were observed. # Analysis Both `Selection.create` and `Selection.extend` are required to produce a balanced `Selection` where the fee (and fee excess) is minimized. Both of these functions rely on `minimizeFee`: this function is required to be **idempotent**, so that: > `∀ s. minimizeFee (minimizeFee xs) == minimizeFee s`. To check that `create` and `extend` produce selections with minimal fees, it's sufficient to verify that the selections they return have fees that _cannot be minimized further_. To do this, our pre-existing property tests _already_ call `minimizeFee` and verify that the result does not change: ```hs verify (feeExcess selection == feeExcessExpected) where (feeExcessExpected, _) = minimizeFee constraints (feeExcess selection, outputs selection) ``` However, in addition to this check, we were needlessly asserting that `balance` should be idempotent. The `balance` function is not actually designed for this: it's designed to always be called with outputs that are minimal in their ada quantities. # Changes This PR: - removes the unnecessary and invalid assertion for the idempotency of `balance`. - makes the pre-condition for `balance` explicit with a documentation comment. - adds an explicit idempotency test for `minimizeFee`. - improves error reporting in case of test failures, by displaying the actual fee excess. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-30T12:16:01Z

Build failed:

ci/hydra-build:required

For some reason the log file is unavailable: https://hydra.iohk.io/build/6225961/log/tail

Textual encoding
    Can perform roundtrip textual encoding & decoding
      can perform roundtrip textual encoding and decoding for values of type 'Iso8601Time'
        +++ OK, passed 100 tests.
      can perform roundtrip textual encoding and decoding for values of type 'SortOrder'

Seems like
#2472
to me.

jonathanknowles · 2021-04-30T12:19:23Z

bors r+

2632: Fix `prop_{create,extend}` in `SelectionSpec`. r=jonathanknowles a=jonathanknowles # Issue Number #2630 / ADP-898 # Summary This PR fixes the following flaky tests: - `Migration.SelectionSpec.prop_create` - `Migration.SelectionSpec.prop_extend` These tests would both fail around 2% of the time, for the same underlying reason. # Testing This PR was tested by running the entire `SelectionSpec` **1000** times. No failures were observed. # Analysis Both `Selection.create` and `Selection.extend` are required to produce a balanced `Selection` where the fee (and fee excess) is minimized. Both of these functions rely on `minimizeFee`: this function is required to be **idempotent**, so that: > `∀ s. minimizeFee (minimizeFee s) == minimizeFee s`. To check that `create` and `extend` produce selections with minimal fees, it's sufficient to verify that the selections they return have fees that _cannot be minimized further_. To do this, our pre-existing property tests _already_ call `minimizeFee` and verify that the result does not change: ```hs verify (feeExcess selection == feeExcessExpected) where (feeExcessExpected, _) = minimizeFee constraints (feeExcess selection, outputs selection) ``` However, in addition to this check, we were needlessly asserting that `balance` should be idempotent. The `balance` function is not actually designed for this: it's designed to always be called with outputs that are minimal in their ada quantities. # Changes This PR: - removes the unnecessary and invalid assertion for the idempotency of `balance`. - makes the pre-condition for `balance` explicit with a documentation comment. - adds an explicit idempotency test for `minimizeFee`. - improves error reporting in case of test failures, by displaying the actual fee excess. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-30T12:53:34Z

Build failed:

ci/hydra-build:required

Failures:

  src/Test/Integration/Framework/DSL.hs:2186:7: 
  1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_JOIN_01rewards - Can join a pool, earn rewards and collect them
       expected a successful response but got an error: DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [LedgerFailure (DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash \\\"56cbdb31d60c0c0f7ccfcf444e40d10efe6c0cb5ea87e9613bd553c3\\\")},Coin 15367820)])))]}))))\"}"
       While verifying (Status {statusCode = 500, statusMessage = "Internal Server Error"},Left (DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [LedgerFailure (DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash \\\"56cbdb31d60c0c0f7ccfcf444e40d10efe6c0cb5ea87e9613bd553c3\\\")},Coin 15367820)])))]}))))\"}"))

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_JOIN_01rewards - Can join a pool, earn rewards and collect them/"

Randomized with seed 1895285687

Finished in 1653.5906 seconds

#2467

Anviking · 2021-04-30T12:54:38Z

bors r+

2632: Fix `prop_{create,extend}` in `SelectionSpec`. r=Anviking a=jonathanknowles # Issue Number #2630 / ADP-898 # Summary This PR fixes the following flaky tests: - `Migration.SelectionSpec.prop_create` - `Migration.SelectionSpec.prop_extend` These tests would both fail around 2% of the time, for the same underlying reason. # Testing This PR was tested by running the entire `SelectionSpec` **1000** times. No failures were observed. # Analysis Both `Selection.create` and `Selection.extend` are required to produce a balanced `Selection` where the fee (and fee excess) is minimized. Both of these functions rely on `minimizeFee`: this function is required to be **idempotent**, so that: > `∀ s. minimizeFee (minimizeFee s) == minimizeFee s`. To check that `create` and `extend` produce selections with minimal fees, it's sufficient to verify that the selections they return have fees that _cannot be minimized further_. To do this, our pre-existing property tests _already_ call `minimizeFee` and verify that the result does not change: ```hs verify (feeExcess selection == feeExcessExpected) where (feeExcessExpected, _) = minimizeFee constraints (feeExcess selection, outputs selection) ``` However, in addition to this check, we were needlessly asserting that `balance` should be idempotent. The `balance` function is not actually designed for this: it's designed to always be called with outputs that are minimal in their ada quantities. # Changes This PR: - removes the unnecessary and invalid assertion for the idempotency of `balance`. - makes the pre-condition for `balance` explicit with a documentation comment. - adds an explicit idempotency test for `minimizeFee`. - improves error reporting in case of test failures, by displaying the actual fee excess. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-30T13:42:38Z

Build failed:

ci/hydra-build:required

Extract from log (https://hydra.iohk.io/build/6227922/nixlog/1/tail):

        While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiWallet {id = ApiT {getApiT = WalletId {getWalletId = a5edc895da6853244414591cececcea763ded9f3}}, addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 20}}, balance = ApiWalletBalance {available = Quantity {getQuantity = 100000000000}, total = Quantity {getQuantity = 100000000000}, reward = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, delegation = ApiWalletDelegation {active = ApiWalletDelegationNext {status = NotDelegating, target = Nothing, changesAt = Nothing}, next = []}, name = ApiT {getApiT = WalletName {getWalletName = "MIR Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-04-30 13:18:46.831907 UTC}), state = ApiT {getApiT = Ready}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 4798}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 95}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 48}}}, time = 2021-04-30 13:20:22.6 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 2320}}}}))
       Waited longer than 90s to resolve action: "MIR wallet: wallet is 100% synced ".

  To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/SHELLEY_TX_REDEEM_03 - Can't redeem rewards from other if none left/"

Looks like it could be related to: 2564

Re-tagging as
#2720

jonathanknowles · 2021-04-30T13:49:23Z

bors r+

iohk-bors · 2021-04-30T14:37:40Z

Build succeeded:

jonathanknowles added 4 commits April 30, 2021 07:04

Add idempotency test to prop_minimizeFee.

f0597af

Running `minimizeFee` a second time should not change the fee excess.

Make the pre-condition for Selection.balance explicit.

1ac9174

The `balance` function expects to be given outputs whose ada quantities are minimal.

jonathanknowles self-assigned this Apr 30, 2021

jonathanknowles requested review from Anviking and paweljakubas April 30, 2021 07:30

paweljakubas approved these changes Apr 30, 2021

View reviewed changes

iohk-bors bot merged commit 5d386b6 into master Apr 30, 2021

iohk-bors bot deleted the jonathanknowles/adp-898 branch April 30, 2021 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `prop_{create,extend}` in `SelectionSpec`. #2632

Fix `prop_{create,extend}` in `SelectionSpec`. #2632

jonathanknowles commented Apr 30, 2021 •

edited

Loading

paweljakubas left a comment

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading

Anviking commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

Fix prop_{create,extend} in SelectionSpec. #2632

Fix prop_{create,extend} in SelectionSpec. #2632

Conversation

jonathanknowles commented Apr 30, 2021 • edited Loading

Issue Number

Summary

Testing

Analysis

Changes

paweljakubas left a comment

Choose a reason for hiding this comment

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 • edited by Anviking Loading

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 • edited by Anviking Loading

Anviking commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021 • edited by Anviking Loading

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

Fix `prop_{create,extend}` in `SelectionSpec`. #2632

Fix `prop_{create,extend}` in `SelectionSpec`. #2632

jonathanknowles commented Apr 30, 2021 •

edited

Loading

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading

iohk-bors bot commented Apr 30, 2021 •

edited by Anviking

Loading