Get TokenBundleMaxSize from Alonzo PParams #2822

Anviking · 2021-08-12T13:21:45Z

Get maximumTokenBundleSize over LSQ instead of hard-coding it, such
that we can handle different values in the alonzo geneis, as well as
if it is changed by an update proposal.
Expose maximumTokenBundleSize through the GET /network/parameters endpoint.
Test that it is still 4000 bytes on our local Alonzo cluster.
The fact that existing tests pass on both Alonzo and Mary vouches for
the correctness.

I also tested setting maximumTokenBundleSize to 800 manually in the
Alonzo genesis, and got these errors:

src/Test/Integration/Scenario/API/Shelley/Migrations.hs:442:23:
3) API Specifications, SHELLEY_MIGRATIONS, SHELLEY_CREATE_MIGRATION_PLAN_07 - Can create a complete migration plan for a wallet with a large number of freerider UTxO entries, but with just enough non-freerider entries to enable the entire UTxO set to be migrated.
     expected: 2
      but got: 4

To rerun use: --match "/API Specifications/SHELLEY_MIGRATIONS/SHELLEY_CREATE_MIGRATION_PLAN_07 - Can create a complete migration plan for a wallet with a large number of freerider UTxO entries, but with just enough non-freerider entries to enable the entire UTxO set to be migrated./"

src/Test/Integration/Scenario/API/Shelley/Migrations.hs:569:23:
4) API Specifications, SHELLEY_MIGRATIONS, SHELLEY_CREATE_MIGRATION_PLAN_08 - Can create a partial migration plan for a wallet with a large number of freerider UTxO entries, but with not quite enough non-freerider entries to enable the entire UTxO set to be migrated.
     expected: 1
      but got: 2

To rerun use: --match "/API Specifications/SHELLEY_MIGRATIONS/SHELLEY_CREATE_MIGRATION_PLAN_08 - Can create a partial migration plan for a wallet with a large number of freerider UTxO entries, but with not quite enough non-freerider entries to enable the entire UTxO set to be migrated./"

src/Test/Integration/Scenario/API/Shelley/Transactions.hs:715:5:
5) API Specifications, SHELLEY_TRANSACTIONS, TRANS_ASSETS_CREATE_02c - Send SeaHorses
     uncaught exception: ProcessHasExited
     ProcessHasExited "cardano-cli failed: Command failed: transaction submit  Error: Error while submitting tx: ShelleyTxValidationError ShelleyBasedEraAlonzo (ApplyTxError [UtxowFailure (WrappedShelleyEraFailure (UtxoFailure (OutputTooBigUTxO [(1950,800,(Addr Mainnet (KeyHashObj (KeyHash \"7ecad06eb492b6eeb86c678889ab6551fdd4271b9d7a4f05cc55fbd5\")) (StakeRefBase (KeyHashObj (KeyHash \"7a8358d230288891a5892310e8c4e7d3adfebc2e6e475372ea403bec\"))),Value 1000000000 (fromList [(PolicyID {policyID = ScriptHash \"4ff049585c4b3070563966370f5427d4a2f3588bce4146d57a93c7d3\"},fromList [(\"00000000000000000SeaHorse1\",1),(\"00000000000000000SeaHorse10\",1),(\"00000000000000000SeaHorse11\",1),(\"00000000000000000SeaHorse12\",1),(\"00000000000000000SeaHorse13\",1),(\"00000000000000000SeaHorse14\",1),(\"00000000000000000SeaHorse15\",1),(\"00000000000000000SeaHorse16\",1),(\"00000000000000000SeaHorse17\",1),(\"00000000000000000SeaHorse18\",1),(\"00000000000000000SeaHorse19\",1),(\"00000000000000000SeaHorse2\",1),(\"00000000000000000SeaHorse20\",1),(\"00000000000000000SeaHorse21\",1),(\"00000000000000000SeaHorse22\",1),(\"00000000000000000SeaHorse23\",1),(\"00000000000000000SeaHorse24\",1),(\"00000000000000000SeaHorse25\",1),(\"00000000000000000SeaHorse26\",1),(\"00000000000000000SeaHorse27\",1),(\"00000000000000000SeaHorse28\",1),(\"00000000000000000SeaHorse29\",1),(\"00000000000000000SeaHorse3\",1),(\"00000000000000000SeaHorse30\",1),(\"00000000000000000SeaHorse31\",1),(\"00000000000000000SeaHorse32\",1),(\"00000000000000000SeaHorse33\",1),(\"00000000000000000SeaHorse34\",1),(\"00000000000000000SeaHorse35\",1),(\"00000000000000000SeaHorse36\",1),(\"00000000000000000SeaHorse37\",1),(\"00000000000000000SeaHorse38\",1),(\"00000000000000000SeaHorse39\",1),(\"00000000000000000SeaHorse4\",1),(\"00000000000000000SeaHorse40\",1),(\"00000000000000000SeaHorse41\",1),(\"00000000000000000SeaHorse42\",1),(\"00000000000000000SeaHorse43\",1),(\"00000000000000000SeaHorse44\",1),(\"00000000000000000SeaHorse45\",1),(\"00000000000000000SeaHorse46\",1),(\"00000000000000000SeaHorse47\",1),(\"00000000000000000SeaHorse48\",1),(\"00000000000000000SeaHorse49\",1),(\"00000000000000000SeaHorse5\",1),(\"00000000000000000SeaHorse50\",1),(\"00000000000000000SeaHorse51\",1),(\"00000000000000000SeaHorse52\",1),(\"00000000000000000SeaHorse53\",1),(\"00000000000000000SeaHorse54\",1),(\"00000000000000000SeaHorse55\",1),(\"00000000000000000SeaHorse56\",1),(\"00000000000000000SeaHorse57\",1),(\"00000000000000000SeaHorse58\",1),(\"00000000000000000SeaHorse59\",1),(\"00000000000000000SeaHorse6\",1),(\"00000000000000000SeaHorse60\",1),(\"00000000000000000SeaHorse61\",1),(\"00000000000000000SeaHorse62\",1),(\"00000000000000000SeaHorse63\",1),(\"00000000000000000SeaHorse64\",1),(\"00000000000000000SeaHorse7\",1),(\"00000000000000000SeaHorse8\",1),(\"00000000000000000SeaHorse9\",1)])]),SNothing))])))])\n" (ExitFailure 1)

To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_ASSETS_CREATE_02c - Send SeaHorses/"

src/Test/Integration/Scenario/API/Shelley/Network.hs:78:52:
6) API Specifications, SHELLEY_NETWORK, NETWORK_PARAMS - Able to fetch network parameters
     expected: Quantity 4000
      but got: Quantity 800

To rerun use: --match "/API Specifications/SHELLEY_NETWORK/NETWORK_PARAMS - Able to fetch network parameters/"

which all seem fine / expected. E.g. we need more txs to perform migrations when less can fit in each tx.

Issue Number

ADP-959

Comments

paweljakubas

LGTM.

Anviking · 2021-08-12T14:32:53Z

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

 computeTokenBundleSerializedLengthBytes :: TokenBundle.TokenBundle -> Int
 computeTokenBundleSerializedLengthBytes =
+    -- FIXME: Do we need an Alonzo-specific verison as well?


Shouldn't forget to check 👆but I suspect the answer is no.

I checked alonzo.cddl and it's the same as shelley-ma.cddl for multiassets. And there is no toAlonzoValue in cardano-api, only toMaryValue.

👍 Thanks!

rvl

Looks great. 👍

rvl · 2021-08-14T09:40:17Z

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

 computeTokenBundleSerializedLengthBytes :: TokenBundle.TokenBundle -> Int
 computeTokenBundleSerializedLengthBytes =
+    -- FIXME: Do we need an Alonzo-specific verison as well?


I checked alonzo.cddl and it's the same as shelley-ma.cddl for multiassets. And there is no toAlonzoValue in cardano-api, only toMaryValue.

Anviking · 2021-08-17T09:37:05Z

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

+
+instance NFData TokenBundleMaxSize
+
+instance Arbitrary TokenBundleMaxSize where


☝️ Sneaked in this non-orphan instance 🙂 Think it makes perfect sense for a simple type like this.

jonathanknowles

Hi @Anviking

This looks good to me! Though I have one minor suggestion (see comments).

One thing you could consider (which might make the code a bit simpler) is to reuse the pre-existing TxSize type, which is designed to do what you want: specify the size of part of a transaction in bytes.

You could use the type directly, or you could wrap it like you've done:

newtype TokenBundleMaxSize = TokenBundleMaxSize
    { unTokenBundleMaxSize :: TxSize }
    deriving (Bounded, Eq, Generic, Show)

Then the implementation for TxConstraints.txOutputMaximumSize becomes a bit easier.

jonathanknowles · 2021-08-18T07:27:57Z

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

+newtype TokenBundleMaxSize = TokenBundleMaxSize
+    { unTokenBundleMaxSize :: Quantity "byte" Word16 }
+    deriving (Bounded, Eq, Generic, Show)


Perhaps it would be good to use the pre-existing TxSize type (which specifies the size in bytes of part of all of a transaction):

Suggested change

newtype TokenBundleMaxSize = TokenBundleMaxSize

{ unTokenBundleMaxSize :: Quantity "byte" Word16 }

deriving (Bounded, Eq, Generic, Show)

newtype TokenBundleMaxSize = TokenBundleMaxSize

{ unTokenBundleMaxSize :: TxSize }

deriving (Bounded, Eq, Generic, Show)

jonathanknowles · 2021-08-18T07:42:33Z

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

+    txOutputMaximumSize = (txOutputSize mempty <>)
+        . TxSize
+        . fromIntegral
+        . getQuantity
+        . unTokenBundleMaxSize
+        $ view (#txParameters . #getTokenBundleMaxSize) protocolParams


If TokenBundleMaximumSize were defined in terms of TxSize, then we could write this more simply:

Suggested change

txOutputMaximumSize = (txOutputSize mempty <>)

. TxSize

. fromIntegral

. getQuantity

. unTokenBundleMaxSize

$ view (#txParameters . #getTokenBundleMaxSize) protocolParams

txOutputMaximumSize = (<>)

(view (#txParameters . #getTokenBundleMaxSize) protocolParams)

(txOutputSize mempty)

- Get maximumTokenBundleSize over LSQ instead of hard-coding it, such that we can handle different values in the alonzo geneis, as well as if it is changed by an update proposal. - Expose maximumTokenBundleSize through the GET /network/parameters endpoint. - Test that it is still 4000 bytes on our local Alonzo cluster. - The fact that existing tests pass on both Alonzo and Mary vouch for the correctness. I also tested setting maximumTokenBundleSize to 800 manually in the Alonzo genesis, and got these errors: src/Test/Integration/Scenario/API/Shelley/Migrations.hs:442:23: 3) API Specifications, SHELLEY_MIGRATIONS, SHELLEY_CREATE_MIGRATION_PLAN_07 - Can create a complete migration plan for a wallet with a large number of freerider UTxO entries, but with just enough non-freerider entries to enable the entire UTxO set to be migrated. expected: 2 but got: 4 To rerun use: --match "/API Specifications/SHELLEY_MIGRATIONS/SHELLEY_CREATE_MIGRATION_PLAN_07 - Can create a complete migration plan for a wallet with a large number of freerider UTxO entries, but with just enough non-freerider entries to enable the entire UTxO set to be migrated./" src/Test/Integration/Scenario/API/Shelley/Migrations.hs:569:23: 4) API Specifications, SHELLEY_MIGRATIONS, SHELLEY_CREATE_MIGRATION_PLAN_08 - Can create a partial migration plan for a wallet with a large number of freerider UTxO entries, but with not quite enough non-freerider entries to enable the entire UTxO set to be migrated. expected: 1 but got: 2 To rerun use: --match "/API Specifications/SHELLEY_MIGRATIONS/SHELLEY_CREATE_MIGRATION_PLAN_08 - Can create a partial migration plan for a wallet with a large number of freerider UTxO entries, but with not quite enough non-freerider entries to enable the entire UTxO set to be migrated./" src/Test/Integration/Scenario/API/Shelley/Transactions.hs:715:5: 5) API Specifications, SHELLEY_TRANSACTIONS, TRANS_ASSETS_CREATE_02c - Send SeaHorses uncaught exception: ProcessHasExited ProcessHasExited "cardano-cli failed: Command failed: transaction submit Error: Error while submitting tx: ShelleyTxValidationError ShelleyBasedEraAlonzo (ApplyTxError [UtxowFailure (WrappedShelleyEraFailure (UtxoFailure (OutputTooBigUTxO [(1950,800,(Addr Mainnet (KeyHashObj (KeyHash \"7ecad06eb492b6eeb86c678889ab6551fdd4271b9d7a4f05cc55fbd5\")) (StakeRefBase (KeyHashObj (KeyHash \"7a8358d230288891a5892310e8c4e7d3adfebc2e6e475372ea403bec\"))),Value 1000000000 (fromList [(PolicyID {policyID = ScriptHash \"4ff049585c4b3070563966370f5427d4a2f3588bce4146d57a93c7d3\"},fromList [(\"00000000000000000SeaHorse1\",1),(\"00000000000000000SeaHorse10\",1),(\"00000000000000000SeaHorse11\",1),(\"00000000000000000SeaHorse12\",1),(\"00000000000000000SeaHorse13\",1),(\"00000000000000000SeaHorse14\",1),(\"00000000000000000SeaHorse15\",1),(\"00000000000000000SeaHorse16\",1),(\"00000000000000000SeaHorse17\",1),(\"00000000000000000SeaHorse18\",1),(\"00000000000000000SeaHorse19\",1),(\"00000000000000000SeaHorse2\",1),(\"00000000000000000SeaHorse20\",1),(\"00000000000000000SeaHorse21\",1),(\"00000000000000000SeaHorse22\",1),(\"00000000000000000SeaHorse23\",1),(\"00000000000000000SeaHorse24\",1),(\"00000000000000000SeaHorse25\",1),(\"00000000000000000SeaHorse26\",1),(\"00000000000000000SeaHorse27\",1),(\"00000000000000000SeaHorse28\",1),(\"00000000000000000SeaHorse29\",1),(\"00000000000000000SeaHorse3\",1),(\"00000000000000000SeaHorse30\",1),(\"00000000000000000SeaHorse31\",1),(\"00000000000000000SeaHorse32\",1),(\"00000000000000000SeaHorse33\",1),(\"00000000000000000SeaHorse34\",1),(\"00000000000000000SeaHorse35\",1),(\"00000000000000000SeaHorse36\",1),(\"00000000000000000SeaHorse37\",1),(\"00000000000000000SeaHorse38\",1),(\"00000000000000000SeaHorse39\",1),(\"00000000000000000SeaHorse4\",1),(\"00000000000000000SeaHorse40\",1),(\"00000000000000000SeaHorse41\",1),(\"00000000000000000SeaHorse42\",1),(\"00000000000000000SeaHorse43\",1),(\"00000000000000000SeaHorse44\",1),(\"00000000000000000SeaHorse45\",1),(\"00000000000000000SeaHorse46\",1),(\"00000000000000000SeaHorse47\",1),(\"00000000000000000SeaHorse48\",1),(\"00000000000000000SeaHorse49\",1),(\"00000000000000000SeaHorse5\",1),(\"00000000000000000SeaHorse50\",1),(\"00000000000000000SeaHorse51\",1),(\"00000000000000000SeaHorse52\",1),(\"00000000000000000SeaHorse53\",1),(\"00000000000000000SeaHorse54\",1),(\"00000000000000000SeaHorse55\",1),(\"00000000000000000SeaHorse56\",1),(\"00000000000000000SeaHorse57\",1),(\"00000000000000000SeaHorse58\",1),(\"00000000000000000SeaHorse59\",1),(\"00000000000000000SeaHorse6\",1),(\"00000000000000000SeaHorse60\",1),(\"00000000000000000SeaHorse61\",1),(\"00000000000000000SeaHorse62\",1),(\"00000000000000000SeaHorse63\",1),(\"00000000000000000SeaHorse64\",1),(\"00000000000000000SeaHorse7\",1),(\"00000000000000000SeaHorse8\",1),(\"00000000000000000SeaHorse9\",1)])]),SNothing))])))])\n" (ExitFailure 1) To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_ASSETS_CREATE_02c - Send SeaHorses/" src/Test/Integration/Scenario/API/Shelley/Network.hs:78:52: 6) API Specifications, SHELLEY_NETWORK, NETWORK_PARAMS - Able to fetch network parameters expected: Quantity 4000 but got: Quantity 800 To rerun use: --match "/API Specifications/SHELLEY_NETWORK/NETWORK_PARAMS - Able to fetch network parameters/" which all seem fine / expected.

From Jonathan's suggestion. Seems particularly nice that we get fewer conversions in the property tests from being able to change computeTokenBundleSerializedLengthBytes to return `TxSize` rather than `Int`

Anviking · 2021-08-18T12:00:21Z

@jonathanknowles Thanks! I think it turned out nicely, particularly when also changing computeTokenBundleSerializedLengthBytes to return TxSize rather than Int, which avoided conversions in both the TransactionLayer assessor, and in the property tests (and need to worry about over/underflow)

Anviking · 2021-08-18T12:00:27Z

bors r+

iohk-bors · 2021-08-18T13:03:04Z

Build succeeded:

Anviking self-assigned this Aug 12, 2021

Anviking changed the title ~~Get maximumTokenBundleSize from Alonzo PParams~~ Get TokenBundleMaxSize from Alonzo PParams Aug 12, 2021

paweljakubas approved these changes Aug 12, 2021

View reviewed changes

Anviking commented Aug 12, 2021

View reviewed changes

rvl approved these changes Aug 14, 2021

View reviewed changes

Anviking commented Aug 17, 2021

View reviewed changes

Anviking force-pushed the anviking/ADP-959/maxValSize branch 3 times, most recently from 7de9e91 to e96fdcb Compare August 17, 2021 14:03

jonathanknowles reviewed Aug 18, 2021

View reviewed changes

Anviking force-pushed the anviking/ADP-959/maxValSize branch from e96fdcb to 9657096 Compare August 18, 2021 09:17

Anviking marked this pull request as ready for review August 18, 2021 11:32

Anviking force-pushed the anviking/ADP-959/maxValSize branch from 1cdae98 to 82191d0 Compare August 18, 2021 11:40

Leverage TxSize more for TokenBundleMaxSize logic

407375a

From Jonathan's suggestion. Seems particularly nice that we get fewer conversions in the property tests from being able to change computeTokenBundleSerializedLengthBytes to return `TxSize` rather than `Int`

Anviking force-pushed the anviking/ADP-959/maxValSize branch from 82191d0 to 407375a Compare August 18, 2021 11:57

iohk-bors bot merged commit 857e928 into master Aug 18, 2021

iohk-bors bot deleted the anviking/ADP-959/maxValSize branch August 18, 2021 13:03

WilliamKingNoel-Bot pushed a commit that referenced this pull request Aug 18, 2021

docs: Merge #2822

9a68c53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get TokenBundleMaxSize from Alonzo PParams #2822

Get TokenBundleMaxSize from Alonzo PParams #2822

Anviking commented Aug 12, 2021 •

edited

Loading

paweljakubas left a comment

Anviking Aug 12, 2021

rvl Aug 14, 2021

Anviking Aug 17, 2021

rvl left a comment

rvl Aug 14, 2021

Anviking Aug 17, 2021 •

edited

Loading

jonathanknowles left a comment

jonathanknowles Aug 18, 2021

jonathanknowles Aug 18, 2021

Anviking commented Aug 18, 2021 •

edited

Loading

Anviking commented Aug 18, 2021

iohk-bors bot commented Aug 18, 2021


		instance NFData TokenBundleMaxSize

		instance Arbitrary TokenBundleMaxSize where

Get TokenBundleMaxSize from Alonzo PParams #2822

Get TokenBundleMaxSize from Alonzo PParams #2822

Conversation

Anviking commented Aug 12, 2021 • edited Loading

Issue Number

Comments

paweljakubas left a comment

Choose a reason for hiding this comment

Anviking Aug 12, 2021

Choose a reason for hiding this comment

rvl Aug 14, 2021

Choose a reason for hiding this comment

Anviking Aug 17, 2021

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

rvl Aug 14, 2021

Choose a reason for hiding this comment

Anviking Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles Aug 18, 2021

Choose a reason for hiding this comment

jonathanknowles Aug 18, 2021

Choose a reason for hiding this comment

Anviking commented Aug 18, 2021 • edited Loading

Anviking commented Aug 18, 2021

iohk-bors bot commented Aug 18, 2021

Anviking commented Aug 12, 2021 •

edited

Loading

Anviking Aug 17, 2021 •

edited

Loading

Anviking commented Aug 18, 2021 •

edited

Loading