-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix termination condition in makeChangeRepeatedly
.
#2676
Conversation
08848c3
to
e566c8a
Compare
f61d74a
to
d623623
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 lgtm
Right change | length change == length outputsToCover -> | ||
-- We've succeeded in making the optimal number of change outputs. | ||
-- Terminate here. | ||
Right change | length change >= length outputsToCover -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah 😄
-- | ||
-- - when change bundles are not split. | ||
-- - when change bundles are split. | ||
-- | ||
prop_makeChange_length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't fail with git revert 35b2dee55ca6d58176e8b64cf18605aeb4ce5070
, but I guess defining an upper bound on length changeSplit
on the split is tricky and is not intended to...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't fail with git revert 35b2dee, but I guess defining an upper bound on length changeSplit on the split is tricky and is not intended to...
You're quite right!
The motivation for changing this property test was more about correcting the broken assertion that the resultant list is always shorter or equal in length to the original list, and improving the coverage of the case where we have to split bundles (which was never tested before). At least now the property test is consistent with the expected behaviour.
As for checking that there is no runaway UTxO consumption in the case of bundle splitting, we use the boundary regression tests to check that this does not occur. These regression tests should indeed fail if we remove 35b2dee.
Ideally, we'd also be able to augment these regression tests with more generalized property tests. But I think that would require more invasive changes to the makeChangeRepeatedly
function.
This initial termination condition was incorrect: ``` Right change | length change == length outputsToCover -> -- We've succeeded in making the optimal number of change outputs. -- Terminate here. pure $ Right $ mkSelectionResult change ``` It was based on the faulty assumption that `makeChange` always returns a list of change outputs whose length is less than or equal to the number of user-specified outputs. However, this assumption is *NOT* valid. In cases where there are many unique assets in the selected inputs, it's possible for `makeChange` to return a list whose length is greater than the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits: 1. A change output has a quantity of a particular asset that exceeds maxBound :: Word64. 2. A change output has a serialized length that is greater than the upper limit defined by the protocol (4000 bytes). If `makeChange` encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. This means that `makeChange` may return more change outputs than the number of user-specified outputs. Hence, we need to adjust the termination condition to allow for this possibility. The original justification for generating one change output per user-specified output was to avoid shrinking the size of the UTxO unnecessarily. By default `makeChange` will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced.
The `makeChange` function is required to split bundles that are oversized. This means that the number of change bundles may exceed the number of user-specified outputs.
In the event that generated change bundles must be split, demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected.
In the event that generated change bundles must be split, demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected.
d623623
to
47dcb5b
Compare
bors r+ |
2676: Fix termination condition in `makeChangeRepeatedly`. r=jonathanknowles a=jonathanknowles # Issue Number ADP-890 # Summary This PR fixes a termination condition in `RoundRobin.makeChangeRepeatedly`. This initial termination condition was incorrect: ```hs Right change | length change == length outputsToCover -> -- We've succeeded in making the optimal number of change outputs. -- Terminate here. pure $ Right $ mkSelectionResult change ``` This equality check was based on the faulty assumption that `makeChange` always returns a list of change outputs whose length is _less than or equal_ to the number of user-specified outputs. However, this assumption is **NOT** valid. In cases where there are many unique assets in the selected inputs, it's possible for `makeChange` to return a list whose length is _greater than_ the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits: 1. A change output has a quantity of a particular asset that exceeds `maxBound :: Word64`. 2. A change output has a serialized length that is greater than the upper limit defined by the protocol (`4000` bytes). If `makeChange` encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. (See `splitOversizedMaps`.) This means that the number of change outputs generated by `makeChange` can _exceed_ the number of user-specified outputs. Hence, we need to adjust the termination condition of `makeChangeRepeatedly` to allow for this possibility. # QA This PR: - [x] Adds regression tests `boundaryTest_largeTokenQuantities_{5,6}`. These tests perform coin selections that can only succeed if change bundles are split, in order to demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected. - [x] Adjusts the property test `prop_makeChange_length` to account for splitting of bundles. # Background The original motivation for generating one change output per user-specified output was to avoid shrinking the size of the UTxO set unnecessarily. By default `makeChange` will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced. # Performance Impact The faulty termination condition would cause `makeChangeRepeatedly` to repeatedly expand the set of selected inputs in an effort to optimize the set of change outputs, continuing until the entire UTxO set was exhausted. ## Example This example uses a `testnet` wallet with: - 47 UTxO entries, of which: - 1 entry is a pure ada entry - 46 entries are multi-asset entries. - around 700 unique assets. We make a payment request for the exact balance of the pure-ada entry, the value of which is around 20% of the total ada available in the UTxO set (when all entries are considered). Specifying this amount means that the coin selection algorithm is guaranteed to select from among the multi-asset entries when generating the initial selection. ### Before applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":244013,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":2143653,"unit":"lovelace"}} real 4m46.720s user 0m0.004s sys 0m0.011s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 3 numberOfSelectedInputs: 2 3 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 2 numberOfSelectedInputs: 6 2 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 9 2 numberOfSelectedInputs: 10 3 numberOfSelectedInputs: 12 4 numberOfSelectedInputs: 13 3 numberOfSelectedInputs: 14 4 numberOfSelectedInputs: 15 2 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 2 numberOfSelectedInputs: 18 1 numberOfSelectedInputs: 19 1 numberOfSelectedInputs: 20 2 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 2 numberOfSelectedInputs: 23 2 numberOfSelectedInputs: 25 1 numberOfSelectedInputs: 26 1 numberOfSelectedInputs: 29 1 numberOfSelectedInputs: 30 2 numberOfSelectedInputs: 31 1 numberOfSelectedInputs: 32 1 numberOfSelectedInputs: 38 49 numberOfSelectedInputs: 47 ``` In the above test run, in 49 cases out of 100, the entire UTxO set was consumed. ### After applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":231206,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":1927085,"unit":"lovelace"}} real 0m19.462s user 0m0.003s sys 0m0.005s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 5 numberOfSelectedInputs: 2 4 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 3 numberOfSelectedInputs: 6 1 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 8 2 numberOfSelectedInputs: 9 1 numberOfSelectedInputs: 10 1 numberOfSelectedInputs: 12 3 numberOfSelectedInputs: 13 1 numberOfSelectedInputs: 14 1 numberOfSelectedInputs: 15 1 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 4 numberOfSelectedInputs: 18 2 numberOfSelectedInputs: 19 5 numberOfSelectedInputs: 20 3 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 3 numberOfSelectedInputs: 23 1 numberOfSelectedInputs: 24 2 numberOfSelectedInputs: 25 2 numberOfSelectedInputs: 27 3 numberOfSelectedInputs: 29 3 numberOfSelectedInputs: 30 1 numberOfSelectedInputs: 31 3 numberOfSelectedInputs: 32 2 numberOfSelectedInputs: 33 2 numberOfSelectedInputs: 34 3 numberOfSelectedInputs: 35 5 numberOfSelectedInputs: 36 2 numberOfSelectedInputs: 37 2 numberOfSelectedInputs: 39 4 numberOfSelectedInputs: 40 5 numberOfSelectedInputs: 41 5 numberOfSelectedInputs: 42 4 numberOfSelectedInputs: 43 3 numberOfSelectedInputs: 44 2 numberOfSelectedInputs: 45 ``` Now the number of selected inputs has a much flatter distribution, and we never hit the case where the entire UTxO set is consumed. 2679: Add UTxO snapshot API endpoint and CLI command. r=jonathanknowles a=jonathanknowles # Issue Number Related to ADP-890. # Overview This PR adds the `getWalletUtxoSnapshot` API endpoint and associated CLI command. The intended use is for debugging and development. The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries. It produces JSON output similar to the following: ```json "entries": [ { "ada": {"quantity": 8, "unit": "lovelace"} , "ada_minimum": {"quantity": 1, "unit": "lovelace"} , "assets": [] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" , "asset_name": "APPLE" , "quantity": 10 } ] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" , "asset_name": "BANANA" , "quantity": 20 } ] } ] ``` Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>
Build failed (retrying...): Timed out here:
|
bors r- |
Canceled. |
bors r+ |
Build succeeded: |
2676: Fix termination condition in `makeChangeRepeatedly`. r=jonathanknowles a=jonathanknowles # Issue Number ADP-890 # Summary This PR fixes a termination condition in `RoundRobin.makeChangeRepeatedly`. This initial termination condition was incorrect: ```hs Right change | length change == length outputsToCover -> -- We've succeeded in making the optimal number of change outputs. -- Terminate here. pure $ Right $ mkSelectionResult change ``` This equality check was based on the faulty assumption that `makeChange` always returns a list of change outputs whose length is _less than or equal_ to the number of user-specified outputs. However, this assumption is **NOT** valid. In cases where there are many unique assets in the selected inputs, it's possible for `makeChange` to return a list whose length is _greater than_ the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits: 1. A change output has a quantity of a particular asset that exceeds `maxBound :: Word64`. 2. A change output has a serialized length that is greater than the upper limit defined by the protocol (`4000` bytes). If `makeChange` encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. (See `splitOversizedMaps`.) This means that the number of change outputs generated by `makeChange` can _exceed_ the number of user-specified outputs. Hence, we need to adjust the termination condition of `makeChangeRepeatedly` to allow for this possibility. # QA This PR: - [x] Adds regression tests `boundaryTest_largeTokenQuantities_{5,6}`. These tests perform coin selections that can only succeed if change bundles are split, in order to demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected. - [x] Adjusts the property test `prop_makeChange_length` to account for splitting of bundles. # Background The original motivation for generating one change output per user-specified output was to avoid shrinking the size of the UTxO set unnecessarily. By default `makeChange` will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced. # Performance Impact The faulty termination condition would cause `makeChangeRepeatedly` to repeatedly expand the set of selected inputs in an effort to optimize the set of change outputs, continuing until the entire UTxO set was exhausted. ## Example This example uses a `testnet` wallet with: - 47 UTxO entries, of which: - 1 entry is a pure ada entry - 46 entries are multi-asset entries. - around 700 unique assets. We make a payment request for the exact balance of the pure-ada entry, the value of which is around 20% of the total ada available in the UTxO set (when all entries are considered). Specifying this amount means that the coin selection algorithm is guaranteed to select from among the multi-asset entries when generating the initial selection. ### Before applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":244013,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":2143653,"unit":"lovelace"}} real 4m46.720s user 0m0.004s sys 0m0.011s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 3 numberOfSelectedInputs: 2 3 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 2 numberOfSelectedInputs: 6 2 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 9 2 numberOfSelectedInputs: 10 3 numberOfSelectedInputs: 12 4 numberOfSelectedInputs: 13 3 numberOfSelectedInputs: 14 4 numberOfSelectedInputs: 15 2 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 2 numberOfSelectedInputs: 18 1 numberOfSelectedInputs: 19 1 numberOfSelectedInputs: 20 2 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 2 numberOfSelectedInputs: 23 2 numberOfSelectedInputs: 25 1 numberOfSelectedInputs: 26 1 numberOfSelectedInputs: 29 1 numberOfSelectedInputs: 30 2 numberOfSelectedInputs: 31 1 numberOfSelectedInputs: 32 1 numberOfSelectedInputs: 38 49 numberOfSelectedInputs: 47 ``` In the above test run, in 49 cases out of 100, the entire UTxO set was consumed. ### After applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":231206,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":1927085,"unit":"lovelace"}} real 0m19.462s user 0m0.003s sys 0m0.005s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 5 numberOfSelectedInputs: 2 4 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 3 numberOfSelectedInputs: 6 1 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 8 2 numberOfSelectedInputs: 9 1 numberOfSelectedInputs: 10 1 numberOfSelectedInputs: 12 3 numberOfSelectedInputs: 13 1 numberOfSelectedInputs: 14 1 numberOfSelectedInputs: 15 1 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 4 numberOfSelectedInputs: 18 2 numberOfSelectedInputs: 19 5 numberOfSelectedInputs: 20 3 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 3 numberOfSelectedInputs: 23 1 numberOfSelectedInputs: 24 2 numberOfSelectedInputs: 25 2 numberOfSelectedInputs: 27 3 numberOfSelectedInputs: 29 3 numberOfSelectedInputs: 30 1 numberOfSelectedInputs: 31 3 numberOfSelectedInputs: 32 2 numberOfSelectedInputs: 33 2 numberOfSelectedInputs: 34 3 numberOfSelectedInputs: 35 5 numberOfSelectedInputs: 36 2 numberOfSelectedInputs: 37 2 numberOfSelectedInputs: 39 4 numberOfSelectedInputs: 40 5 numberOfSelectedInputs: 41 5 numberOfSelectedInputs: 42 4 numberOfSelectedInputs: 43 3 numberOfSelectedInputs: 44 2 numberOfSelectedInputs: 45 ``` Now the number of selected inputs has a much flatter distribution, and we never hit the case where the entire UTxO set is consumed. 2679: Add UTxO snapshot API endpoint and CLI command. r=jonathanknowles a=jonathanknowles # Issue Number Related to ADP-890. # Overview This PR adds the `getWalletUtxoSnapshot` API endpoint and associated CLI command. The intended use is for debugging and development. The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries. It produces JSON output similar to the following: ```json "entries": [ { "ada": {"quantity": 8, "unit": "lovelace"} , "ada_minimum": {"quantity": 1, "unit": "lovelace"} , "assets": [] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" , "asset_name": "APPLE" , "quantity": 10 } ] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" , "asset_name": "BANANA" , "quantity": 20 } ] } ] ``` Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]> f94657c
Issue Number
ADP-890
Summary
This PR fixes a termination condition in
RoundRobin.makeChangeRepeatedly
.This initial termination condition was incorrect:
This equality check was based on the faulty assumption that
makeChange
always returns a list of change outputs whose length is less than or equal to the number of user-specified outputs.However, this assumption is NOT valid.
In cases where there are many unique assets in the selected inputs, it's possible for
makeChange
to return a list whose length is greater than the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits:maxBound :: Word64
.4000
bytes).If
makeChange
encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. (SeesplitOversizedMaps
.)This means that the number of change outputs generated by
makeChange
can exceed the number of user-specified outputs. Hence, we need to adjust the termination condition ofmakeChangeRepeatedly
to allow for this possibility.QA
This PR:
boundaryTest_largeTokenQuantities_{5,6}
. These tests perform coin selections that can only succeed if change bundles are split, in order to demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected.prop_makeChange_length
to account for splitting of bundles.Background
The original motivation for generating one change output per user-specified output was to avoid shrinking the size of the UTxO set unnecessarily. By default
makeChange
will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced.Performance Impact
The faulty termination condition would cause
makeChangeRepeatedly
to repeatedly expand the set of selected inputs in an effort to optimize the set of change outputs, continuing until the entire UTxO set was exhausted.Example
This example uses a
testnet
wallet with:We make a payment request for the exact balance of the pure-ada entry, the value of which is around 20% of the total ada available in the UTxO set (when all entries are considered).
Specifying this amount means that the coin selection algorithm is guaranteed to select from among the multi-asset entries when generating the initial selection.
Before applying this PR
Time required to generate a fee estimate:
Number of inputs selected:
In the above test run, in 49 cases out of 100, the entire UTxO set was consumed.
After applying this PR
Time required to generate a fee estimate:
Number of inputs selected:
Now the number of selected inputs has a much flatter distribution, and we never hit the case where the entire UTxO set is consumed.