Split change bundles with excessive asset counts #2552

jonathanknowles · 2021-03-04T07:49:00Z

Issue Number

Overview

This PR has two main areas:

Refactoring

Relocates all existing type-specific equipartition functions into the modules that define their types. We now have:

Module	Function
`Numeric.Util`	`equipartitionNatural`
`Coin`	`equipartition`
`TokenQuantity`	`equipartition`
`TokenMap`	`equipartitionQuantities`
`TokenMap`	`equipartitionQuantitiesWithUpperBound`
`TokenBundle`	`equipartitionQuantitiesWithUpperBound`

Justification:

This PR will add more functions for splitting up bundles and maps, and the existing naming scheme was becoming too cumbersome to extend.
Equipartitioning a value of type T is something that relates to the definition of type T, so the natural home for such a function is within module T.

Adjusts makeChange to accept a record type, and unifies that type with MakeChangeData from the test suite.

Justification:
- Adding further arguments to this function is now less cumbersome, since the compiler will tell us that a field is missing in all the required call sites (both in implementation code and in test code), rather than producing a series of long type errors.
- We cut down on some boilerplate from the test suite.

New Functionality

Adds the following functions for equipartitioning the asset sets of maps and bundles:

Module Function

TokenMap equipartitionAssets

TokenBundle equipartitionAssets

Adds an assessBundleSize parameter to makeChange and performSelection:

assessBundleSize :: TokenBundle -> TokenBundleSizeAssessment
    
data TokenBundleSizeAssessment
    = TokenBundleSizeWithinLimit
    -- ^ Indicates that the size of a token bundle does not exceed the maximum
    -- size that can be included in a transaction output.
    | TokenBundleSizeExceedsLimit
    -- ^ Indicates that the size of a token bundle exceeds the maximum size
    -- that can be included in a transaction output.

Adds function splitBundlesWithExcessiveAssetCounts to split change bundles with excessive asset counts, and calls it from makeChange.
Provides a definition of assessTokenBundleSize based on functionality exported by Cardano.Api.

Testing

This PR adds:

Property tests for equipartitionAssets functions.
Boundary unit tests for performSelection to show that change bundles are correctly split when they exceed the maximum size.
Unit tests for Shelley.Compatibility.tokenBundleSizeAssessor to verify that it gives sensible results for token bundles of various fixed sizes.
Property tests for Shelley.Compatibility.tokenBundleSizeAssessor to verify that it has the properties expected by performSelection.

jonathanknowles · 2021-03-05T07:59:38Z

bors try

jonathanknowles · 2021-03-05T11:08:13Z

bors try

iohk-bors · 2021-03-05T11:43:12Z

try

Build succeeded:

For greater guarantee that #2552 calculates the max number of asset per bundle in the same way as the node, the idea was to have an integration test. This commit adds a special mnemonic which is funded with the assets SeaHorse1 .. SeaHorse200, and a stub test.

lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs

For greater guarantee that #2552 calculates the max number of asset per bundle in the same way as the node, the idea was to have an integration test. This commit adds a special mnemonic which is funded with the assets SeaHorse1 .. SeaHorse200, and a stub test.

These were defined in multiple places. Now we define them in one place: `Primitive.Types.Tx`.

This will allow it to be reused by other test suites.

This change adds an `assessTokenBundleSize` function to the transaction layer, and to the `Shelley.Compatibility` module. It also adds unit tests to verify that the `assessTokenBundleSize` gives reasonable results for token bundles of fixed sizes.

When assessing the size of a change map to determine if it is excessively large, we don't yet know how large the associated ada quantity will be, since ada quantities are assigned at a later stage (in 'assignCoinsToChangeMaps'). Therefore, we err on the side of caution, and assess the size of a change map combined with the maximum possible ada quantity. This means that when presented with a very large change map, we have a small chance of splitting the map even if that map would be within the limit when combined with its final ada quantity. However, oversplitting a change map is preferable to creating a bundle that is marginally over the limit, which would cause the resultant transaction to be rejected. In response to review feedback and discussion: #2552 (comment)

…quently. This change adjusts the `genFixedSizeTokenBundle` generator so that it has a much higher probability of generating boundary token quantities (the minimum or maximum quantities that can appear in valid transaction outputs). As a consequence of this change, it was necessary to adjust the following unit test to accept a range of expected sizes: `unit_assessTokenBundleSize_fixedSizeBundle` The following unit test remains unaffected: `unit_computeMinimumAdaQuantity_fixedSizeBundle`

This constant should be 4000 bytes (4 kB), and not 4096 bytes (4 KiB). Source: https://github.com/input-output-hk/cardano-ledger-specs/blob/5bccddb0ef0a1ae270b72fe0782a8ad680ed5852/shelley-ma/impl/src/Cardano/Ledger/ShelleyMA/Rules/Utxo.hs#L310 Fixed in response to: https://github.com/input-output-hk/cardano-wallet/pull/2552/files#r590109750

This type acts as a central point of reference for the expected properties of token bundle size assessment functions. This change also adjusts multiple function type signatures, replacing `TokenBundle -> TokenBundleSizeAssessment` with `TokenBundleSizeAssessor`. In response to review feedback: #2552 (comment)

In particular, we test that: * Enlarging a token bundle that is over the size limit should yield a token bundle that is still over the size limit. * Shrinking a token bundle that is within the size limit should yield a token bundle that is still within the size limit.

jonathanknowles · 2021-03-10T06:31:33Z

bors r+

2552: Split change bundles with excessive asset counts r=jonathanknowles a=jonathanknowles # Issue Number ADP-727 # Overview This PR has two main areas: ## Refactoring - [x] Relocates all existing type-specific `equipartition` functions into the modules that define their types. We now have: | Module | Function | | -- | -- | | `Numeric.Util` | `equipartitionNatural` | | `Coin` | `equipartition` | | `TokenQuantity` | `equipartition` | | `TokenMap` | `equipartitionQuantities` | | `TokenMap` | `equipartitionQuantitiesWithUpperBound` | | `TokenBundle` | `equipartitionQuantitiesWithUpperBound` | **Justification:** - This PR will add **_more_** functions for splitting up bundles and maps, and the existing naming scheme was becoming too cumbersome to extend. - Equipartitioning a value of type `T` is something that relates to the definition of type `T`, so the natural home for such a function is within module `T`. - [x] Adjusts `makeChange` to accept a record type, and unifies that type with `MakeChangeData` from the test suite. **Justification:** - Adding further arguments to this function is now less cumbersome, since the compiler will tell us that **_a field is missing_** in all the required call sites (both in implementation code and in test code), rather than producing a series of long type errors. - We cut down on some boilerplate from the test suite. ## New Functionality - [x] Adds the following functions for equipartitioning the asset sets of maps and bundles: | Module | Function | | -- | -- | | `TokenMap` | `equipartitionAssets` | | `TokenBundle` | `equipartitionAssets` | - [x] Adds an `assessBundleSize` parameter to `makeChange` and `performSelection`: ```hs assessBundleSize :: TokenBundle -> TokenBundleSizeAssessment data TokenBundleSizeAssessment = TokenBundleSizeWithinLimit -- ^ Indicates that the size of a token bundle does not exceed the maximum -- size that can be included in a transaction output. | TokenBundleSizeExceedsLimit -- ^ Indicates that the size of a token bundle exceeds the maximum size -- that can be included in a transaction output. ``` - [x] Adds function `splitBundlesWithExcessiveAssetCounts` to split change bundles with excessive asset counts, and calls it from `makeChange`. - [x] Provides a definition of `assessTokenBundleSize` based on functionality exported by `Cardano.Api`. ## Testing This PR adds: - [x] Property tests for `equipartitionAssets` functions. - [x] Boundary unit tests for `performSelection` to show that change bundles are correctly split when they exceed the maximum size. - [x] Unit tests for `Shelley.Compatibility.tokenBundleSizeAssessor` to verify that it gives sensible results for token bundles of various fixed sizes. - [x] Property tests for `Shelley.Compatibility.tokenBundleSizeAssessor` to verify that it has the properties expected by `performSelection`. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-03-10T07:36:33Z

Build failed:

ci/hydra-build:required

        Valid ISO 8601 extended UTC times
          2008-09-15T15:53:00Z
            +++ OK, passed 1 test.
          2008-09-15T15:53:00.1Z
            +++ OK, passed 1 test.
          2008-09-15T15:53:00.12Z
            +++ OK, passed 1 test.
        Non-time examples
          Data.Time.TextSpec[56:21]
            +++ OK, passed 1 test.
          w
            +++ OK, passed 1 test.
          wibble
            +++ OK, passed 1 test.
        ISO 8601 basic format without times
          2008
            +++ OK, passed 1 test.
          200809
            +++ OK, passed 1 test.
          20080915
            +++ OK, passed 1 test.
        ISO 8601 basic format without timezones
          20080915T155300
            +++ OK, passed 1 test.
          20080915T155300.1
            +++ OK, passed 1 test.
          20080915T155300.12
            +++ OK, passed 1 test.
building of '/nix/store/zy6scpy44agjf03q800lfc9mrk9zlyq1-cardano-wallet-core-test-unit-2021.3.4-check' timed out after 900 seconds of silence

#2472

jonathanknowles · 2021-03-10T07:41:56Z

bors r+

iohk-bors · 2021-03-10T08:30:16Z

Build succeeded:

To ensure that our calculation matches the one of the ledger. See #2552 (comment) for what it helped catch.

2555: Start adding a integration test for large asset counts r=Anviking a=Anviking # Issue Number  Relates to #2552 # Overview  - [x] Add integration test for covering the border in-between where we don't need to split token-bundle change, and where we do need to, to ensure we have the calculation correct # Comments ```bash stack ghci cardano-wallet-core-integration cardano-wallet:integration ``` ``` λ> import System.Environment λ> setEnv "NO_POOLS" "1" λ> :main --match SeaHorse ```   Co-authored-by: Johannes Lund <[email protected]>

To ensure that our calculation matches the one of the ledger. See #2552 (comment) for what it helped catch.

2555: Start adding a integration test for large asset counts r=Anviking a=Anviking # Issue Number  Relates to #2552 # Overview  - [x] Add integration test for covering the border in-between where we don't need to split token-bundle change, and where we do need to, to ensure we have the calculation correct # Comments ```bash stack ghci cardano-wallet-core-integration cardano-wallet:integration ``` ``` λ> import System.Environment λ> setEnv "NO_POOLS" "1" λ> :main --match SeaHorse ```   Co-authored-by: Johannes Lund <[email protected]>

2555: Add a integration test for large asset counts r=Anviking a=Anviking # Issue Number  Relates to #2552 # Overview  - [x] Add integration test for covering the border in-between where we don't need to split token-bundle change, and where we do need to, to ensure we have the calculation correct # Comments ```bash stack ghci cardano-wallet-core-integration cardano-wallet:integration ``` ``` λ> import System.Environment λ> setEnv "NO_POOLS" "1" λ> :main --match SeaHorse ```   Co-authored-by: Johannes Lund <[email protected]>

2560: Validate output bundles when processing coin selection requests r=jonathanknowles a=jonathanknowles # Issue Number ADP-727 ADP-782 # Overview This PR adds validation to coin selection requests, checking all **user-specified outputs** meet the following pre-conditions: - output bundles do not include token quantities in excess of the maximum bound: `maxBound :: Word64` - output bundles, when serialized, are within the size limit specified by the protocol: `4000` bytes If either of the above pre-conditions are violated, an API error is returned, with a message that informs the user how they can solve the problem (usually by breaking up bundles into smaller bundles): - `ErrOutputTokenBundleSizeExceedsLimit` - `ErrOutputTokenQuantityExceedsLimit` # Testing This PR adds integration tests for both of the above scenarios: - `WALLETS_COIN_SELECTION_07` - `WALLETS_COIN_SELECTION_08` # Refactoring This PR also performs a small refactoring in the same spirit as the refactorings included in PR #2552. We extract out `partition` functions for `Coin` and `TokenQuantity` values into the modules that define their types, similarly to the `X.equipartition` functions. Co-authored-by: Jonathan Knowles <[email protected]>

jonathanknowles self-assigned this Mar 4, 2021

jonathanknowles marked this pull request as ready for review March 4, 2021 08:09

jonathanknowles marked this pull request as draft March 4, 2021 08:16

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch 3 times, most recently from 5ce874b to 7d1a632 Compare March 5, 2021 07:09

jonathanknowles marked this pull request as ready for review March 5, 2021 07:09

iohk-bors bot added a commit that referenced this pull request Mar 5, 2021

Try #2552:

d6a369f

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch 2 times, most recently from 296f0c2 to a3541b3 Compare March 5, 2021 07:19

iohk-bors bot added a commit that referenced this pull request Mar 5, 2021

Try #2552:

c29f745

cardano-foundation deleted a comment from iohk-bors bot Mar 5, 2021

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch from a3541b3 to c455887 Compare March 5, 2021 07:24

cardano-foundation deleted a comment from iohk-bors bot Mar 5, 2021

iohk-bors bot added a commit that referenced this pull request Mar 5, 2021

Try #2552:

30be165

cardano-foundation deleted a comment from iohk-bors bot Mar 5, 2021

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch from c455887 to 7e29b19 Compare March 5, 2021 07:40

iohk-bors bot added a commit that referenced this pull request Mar 5, 2021

Try #2552:

b650847

This comment has been minimized.

Sign in to view

jonathanknowles requested a review from Anviking March 5, 2021 10:54

iohk-bors bot added a commit that referenced this pull request Mar 5, 2021

Try #2552:

a5ca510

Anviking mentioned this pull request Mar 5, 2021

Add an integration test for large asset counts #2555

Merged

1 task

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch from 7e29b19 to f556051 Compare March 8, 2021 09:27

Anviking reviewed Mar 8, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs Show resolved Hide resolved

jonathanknowles added 8 commits March 10, 2021 06:05

Consolidate the various definitions of txOut{Min,Max}TokenQuantity.

fd09e80

These were defined in multiple places. Now we define them in one place: `Primitive.Types.Tx`.

Move genFixedSizeTokenBundle to TokenBundle.Gen.

091c606

This will allow it to be reused by other test suites.

jonathanknowles force-pushed the jonathanknowles/split-bundles-with-excessive-asset-counts branch from 1d8f422 to 7f00590 Compare March 10, 2021 06:09

Regenerate nix

95a4b4b

iohk-bors bot merged commit 63eb2fc into master Mar 10, 2021

iohk-bors bot deleted the jonathanknowles/split-bundles-with-excessive-asset-counts branch March 10, 2021 08:30

Anviking added a commit that referenced this pull request Mar 10, 2021

Add integration test for large asset counts

53cbbfb

To ensure that our calculation matches the one of the ledger. See #2552 (comment) for what it helped catch.

Anviking added a commit that referenced this pull request Mar 10, 2021

Add integration test for large asset counts

ea30a8b

To ensure that our calculation matches the one of the ledger. See #2552 (comment) for what it helped catch.

Anviking added a commit that referenced this pull request Mar 11, 2021

Add integration test for large asset counts

17c3a48

To ensure that our calculation matches the one of the ledger. See #2552 (comment) for what it helped catch.

jonathanknowles mentioned this pull request Mar 12, 2021

Validate user-specified output bundles when processing coin selection requests #2560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split change bundles with excessive asset counts #2552

Split change bundles with excessive asset counts #2552

jonathanknowles commented Mar 4, 2021 •

edited

Loading

jonathanknowles commented Mar 5, 2021

This comment has been minimized.

jonathanknowles commented Mar 5, 2021

iohk-bors bot commented Mar 5, 2021

jonathanknowles commented Mar 10, 2021

iohk-bors bot commented Mar 10, 2021 •

edited by Anviking

Loading

jonathanknowles commented Mar 10, 2021

iohk-bors bot commented Mar 10, 2021

Module	Function
`TokenMap`	`equipartitionAssets`
`TokenBundle`	`equipartitionAssets`

Split change bundles with excessive asset counts #2552

Split change bundles with excessive asset counts #2552

Conversation

jonathanknowles commented Mar 4, 2021 • edited Loading

Issue Number

Overview

Refactoring

New Functionality

Testing

jonathanknowles commented Mar 5, 2021

This comment has been minimized.

jonathanknowles commented Mar 5, 2021

iohk-bors bot commented Mar 5, 2021

try

jonathanknowles commented Mar 10, 2021

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

jonathanknowles commented Mar 10, 2021

iohk-bors bot commented Mar 10, 2021

jonathanknowles commented Mar 4, 2021 •

edited

Loading

iohk-bors bot commented Mar 10, 2021 •

edited by Anviking

Loading