Drop support for balancing transactions in eras more recent than the node tip #3923

Anviking · 2023-05-11T13:56:11Z

Always balance transactions in the era of the local node tip

Comments

Previously we used to leverage the conversion between Cardano.ProtocolParameters and Ledger.PParams era to potentially "upcast" the current protocol parameters to Ledger.PParams to a newer era.

This may have been useful for testing features in new eras on mainnet before a HF, even if the resulting txs would have been unsubmittable. Instead of doing this one can test on a public testnet, local cluster or on the unit level.

Issue Number

ADP-2990

Anviking · 2023-05-15T14:22:07Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/Tx/SealedTx.hs

@@ -179,6 +185,18 @@ cardanoTxIdeallyNoLaterThan
    -> InAnyCardanoEra Cardano.Tx
 cardanoTxIdeallyNoLaterThan era = unsafeCardanoTx . ideallyNoLaterThan era

+-- | Re-deserialises the bytes 'SealedTx' as a transaction in the provided era


The whole SealedTx ugly and should probably be rethought. Perhaps using something like https://hackage.haskell.org/package/these-1.2/docs/Data-These.html

Anviking · 2023-05-19T15:29:07Z

lib/wallet/src/Cardano/Wallet.hs

+data ErrWriteTxEra
+    = ErrOldEraNotSupported Cardano.AnyCardanoEra
+    | ErrTxNotInNodeEra Write.AnyRecentEra
+    deriving (Show, Eq)


Could be moved to the Write.Tx.Balance module with a function like:

data EraDependentPair f g era = EraDependentPair (f era) (g era) chooseEraForBalancing :: InAnyRecentEra PParams -> InOneOrMoreRecentEra Tx -> InAnyRecentEra (PParams `EraDependentPair` Tx) chooseEraForBalancing = undefined

and we'd want to use it if we were to provide FFI bindings for balanceTransaction.

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

Anviking · 2023-05-23T14:45:18Z

lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs

@@ -89,6 +89,7 @@ data ApiErrorInfo
    | BalanceTxExistingCollateral
    | BalanceTxExistingKeyWitnesses
    | BalanceTxExistingReturnCollateral
+    | TxNotInNodeEra


I opted to break the pattern of BalanceTx prefixes, which I wasn't sure sure were needed.

I think it's okay for now, if we really think that this kind of error is more general than "balanceTx".

But, perhaps we should look at these API errors again in future, and clean them up a bit.

Unisay · 2023-05-23T17:25:07Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

+    parsePartialTx
+        :: Write.IsRecentEra era
+        => Write.RecentEra era
+        -> Handler (Write.PartialTx era)


Suggested change

parsePartialTx

:: Write.IsRecentEra era

=> Write.RecentEra era

-> Handler (Write.PartialTx era)

parsePartialTx :: Write.IsRecentEra era => Write.RecentEra era -> Handler (Write.PartialTx era)

Unisay · 2023-05-23T17:28:53Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/Tx/SealedTx.hs

@@ -179,6 +185,18 @@ cardanoTxIdeallyNoLaterThan
    -> InAnyCardanoEra Cardano.Tx
 cardanoTxIdeallyNoLaterThan era = unsafeCardanoTx . ideallyNoLaterThan era

+-- | Re-deserialises the bytes of the 'SealedTx' as a transaction in the
+-- provided era, and that era only.
+cardanoTxInExactEra


I'd name this function unsealTransaction or unsealTx . The fact that its in the exact era is already captured by the type, so no need to express it in name.

I'd also vote for unsealTx, provided that this really is a form of "unsealing".

(The Tx shorthand is already prevalent throughout our code, so we might as well take advantage of this common shorthand.)

I feel this function is too weird to simply be called unsealTx plus there's symmetry with cardanoTxIdeallyNoLaterThan, so keeping.

Perhaps unsealTx will be a good name if we rethink SealedTx in the future and/or if we remove cardanoTxIdeallyNoLaterThan.

jonathanknowles

Hi @Anviking

I think the direction of this PR is great!

I just have some niggling reservations about the names of errors and the error messages: I feel that we could make the direction of time inequality a bit more explicit. What do you think? (See comments.)

jonathanknowles · 2023-05-24T05:24:58Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

+            apiError err403 TxNotInNodeEra $ T.unwords
+                [ "The provided transaction could be deserialised, just not in"
+                , showT era <> ","
+                , "the era the local node currently is in."


Suggested change

, "the era the local node currently is in."

, "the era the local node is currently in."

jonathanknowles · 2023-05-24T05:25:37Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

+                [ "The provided transaction could be deserialised, just not in"
+                , showT era <> ","
+                , "the era the local node currently is in."
+                , "If the node is not yet fully synced, try waiting."


Suggested change

, "If the node is not yet fully synced, try waiting."

, "If the node is not yet fully synchronised, try waiting."

jonathanknowles · 2023-05-24T05:47:08Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

+                , "If the node is not yet fully synced, try waiting."
+                , "If you're constructing a transaction for a future era for"
+                , "testing purposes, try doing so on a testnet in that era"
+                , "instead."


👍🏻

Nicely-written message!

One suggestion: perhaps we could transform this into a rich error, by encoding the node and transaction era as fields of the error. So something like this:

data ApiErrorInfo = AddressAlreadyExists | ... | TxNotInNodeEra + ApiErrorTxNotInNodeEra

data ApiErrorTxNotInNodeEra = ApiErrorTxNotInNodeEra { eraOfNode :: !Era , eraOfTx :: !Era } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorTxNotInNodeEra

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

jonathanknowles · 2023-05-24T05:50:40Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

+                (Write.UTxOAssumptions
+                    genInpScripts
+                    mScriptTemplate
+                    (txWitnessTagFor @k)
+                    )


Suggested change

(Write.UTxOAssumptions

genInpScripts

mScriptTemplate

(txWitnessTagFor @k)

)

(Write.UTxOAssumptions

genInpScripts

mScriptTemplate

(txWitnessTagFor @k)

)

jonathanknowles · 2023-05-24T05:52:38Z

lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs

@@ -89,6 +89,7 @@ data ApiErrorInfo
    | BalanceTxExistingCollateral
    | BalanceTxExistingKeyWitnesses
    | BalanceTxExistingReturnCollateral
+    | TxNotInNodeEra


I think it's okay for now, if we really think that this kind of error is more general than "balanceTx".

But, perhaps we should look at these API errors again in future, and clean them up a bit.

jonathanknowles · 2023-05-24T06:00:10Z

lib/wallet/src/Cardano/Wallet.hs

+    = ErrOldEraNotSupported Cardano.AnyCardanoEra
+    | ErrTxNotInNodeEra Write.AnyRecentEra


How about the following names:

Suggested change

= ErrOldEraNotSupported Cardano.AnyCardanoEra

| ErrTxNotInNodeEra Write.AnyRecentEra

= ErrTxEraTooOld Cardano.AnyCardanoEra

-- ^ The transaction is from an era that is too old,

-- and no longer supported by 'balanceTransaction'.

| ErrTxEraTooNew Write.AnyRecentEra

-- ^ The transaction is from an era that is too new,

-- from the point of view of the node (at its current

-- level of synchronisation).

The thing that bugs me about the current names is that TxNotInNodeEra doesn't really make the time asymmetry clear.

My interpretation: this error can only happen if the node has not yet caught up with the transaction era: in other words, the transaction is from an era that is "too new" from the point of view of the node. Is that also your interpretation?

time asymmetry

No. ErrTxNotInNodeEra could currently also mean the tx is in an older era compared to the node.

For instance: if the recent eras are [Babbage, Conway], the node & pparams are in Babbage, and the supplied tx binary only is valid in [Shelley, Allegra, Mary], then 1) the node-era validation would not throw ErrOldEraNotSupported but 2) we would get a ErrTxNotInNodeEra.

Would be happy to discuss options to make this clearer

jonathanknowles · 2023-05-24T06:02:28Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/Tx/SealedTx.hs

@@ -179,6 +185,18 @@ cardanoTxIdeallyNoLaterThan
    -> InAnyCardanoEra Cardano.Tx
 cardanoTxIdeallyNoLaterThan era = unsafeCardanoTx . ideallyNoLaterThan era

+-- | Re-deserialises the bytes of the 'SealedTx' as a transaction in the
+-- provided era, and that era only.
+cardanoTxInExactEra


I'd also vote for unsealTx, provided that this really is a form of "unsealing".

(The Tx shorthand is already prevalent throughout our code, so we might as well take advantage of this common shorthand.)

jonathanknowles · 2023-05-24T06:06:36Z

specifications/api/swagger.yaml

+  properties:
+    message:
+      type: string
+      description: The transaction could be deserialised, just not in the era of the local node.


Further to previous comments, I wonder if it's possible to make the direction of time inequality explicit here. ^^^

(Both in the name of the error, and the description.)

For example, I'm guessing that this error cannot be produced in a situation where the node era is later than the transaction era?

Anviking · 2023-05-25T07:37:40Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

                ]
-        ErrOldEraNotSupported (Cardano.AnyCardanoEra era) ->
+        ErrNodeNotInRecentEra (Cardano.AnyCardanoEra era) ->
            apiError err403 BalanceTxEraNotSupported $ T.unwords
                [ "Balancing transactions in ", showT era


Maybe drop "Balancing"?

jonathanknowles

Hi @Anviking!

This PR is looking better and better!

I noticed that we currently don't test the laws for (hand-written) instances of AnyRecentEra, so I added some tests in #3958. (This PR incidentally fixes a couple of compile errors relating to unused definitions and imports.)

Also, it seems like we're missing a swagger.yaml definition for NodeNotYetInRecentEra, which was fortunately caught by the API test suite. (See comments.)

jonathanknowles · 2023-05-26T05:24:54Z

lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs

+    deriving (Data, Eq, Generic, Show, Typeable)
+    deriving (FromJSON, ToJSON)
+        via DefaultRecord ApiErrorNodeNotYetInRecentEra
+    deriving anyclass NFData


Nice! 👍🏻

However, I couldn't find an equivalent definition for NodeNotYetInRecentEra in the swagger specification.

For example, for UTxOTooSmall we have:
https://github.com/input-output-hk/cardano-wallet/blob/0a0ba43b1ed96ca3e207c75b97d6b66c2a905c59/specifications/api/swagger.yaml#L5057-L5080

Luckily, this is caught by our test suite:

cardano-wallet-test-unit> Failures: cardano-wallet-test-unit> test/unit/Cardano/Wallet/Api/TypesSpec.hs:975:9: cardano-wallet-test-unit> 1) Cardano.Wallet.Api.Types, Api Errors, Every ApiErrorInfo constructor has a corresponding schema type cardano-wallet-test-unit> Falsified (after 1 test): cardano-wallet-test-unit> Missing ApiErrorInfo constructors for: cardano-wallet-test-unit> ["NodeNotYetInRecentEra"] cardano-wallet-test-unit> Each of these need a corresponding swagger type of the form: cardano-wallet-test-unit> x-errConstructorName cardano-wallet-test-unit> To rerun use: --match "/Cardano.Wallet.Api.Types/Api Errors/Every ApiErrorInfo constructor has a corresponding schema type/" cardano-wallet-test-unit> Randomized with seed 45294572

https://buildkite.com/input-output-hk/cardano-wallet/builds/25493#01885697-ddc2-47bd-845e-25c1712bb536/6-3443

jonathanknowles · 2023-05-26T05:52:05Z

lib/wallet/src/Cardano/Wallet/Write/Tx.hs

+            ]
+instance Bounded AnyRecentEra where
+    minBound = AnyRecentEra RecentEraBabbage
+    maxBound = AnyRecentEra RecentEraConway


Hi @Anviking

I've added tests for the Eq, Bounded, and Enum instances in #3958.

(That PR incidentally also fixes a couple of errors relating to unused imports and function definitions.)

jonathanknowles

Approving preemptively on the basis that we can also include in this PR:

property tests as included in Test class laws for AnyRecentEra. #3958
a complete swagger specification for the NodeNotYetInRecentEra error.

Otherwise, LGTM!

Anviking · 2023-05-26T14:09:42Z

bors r+

3923: Drop support for balancing transactions in eras more recent than the node tip r=Anviking a=Anviking - [x] Always balance transactions in the era of the local node tip ### Comments Previously we used to leverage the conversion between `Cardano.ProtocolParameters` and `Ledger.PParams era` to potentially "upcast" the current protocol parameters to `Ledger.PParams` to a newer era. This may have been useful for testing features in new eras on mainnet before a HF, even if the resulting txs would have been unsubmittable. Instead of doing this one can test on a public testnet, local cluster or on the unit level. ### Issue Number ADP-2990 Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2023-05-26T14:48:08Z

Build failed:

buildkite/cardano-wallet

Anviking · 2023-05-26T14:49:04Z

bors r+

3923: Drop support for balancing transactions in eras more recent than the node tip r=Anviking a=Anviking - [x] Always balance transactions in the era of the local node tip ### Comments Previously we used to leverage the conversion between `Cardano.ProtocolParameters` and `Ledger.PParams era` to potentially "upcast" the current protocol parameters to `Ledger.PParams` to a newer era. This may have been useful for testing features in new eras on mainnet before a HF, even if the resulting txs would have been unsubmittable. Instead of doing this one can test on a public testnet, local cluster or on the unit level. ### Issue Number ADP-2990 Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2023-05-26T14:55:40Z

Build failed:

buildkite/cardano-wallet

Anviking · 2023-05-26T15:08:04Z

bors r+

Anviking · 2023-05-26T15:26:17Z

bors r+

iohk-bors · 2023-05-26T15:26:19Z

Already running a review

iohk-bors · 2023-05-26T15:31:44Z

Build succeeded:

buildkite/cardano-wallet

…as more recent than the node tip r=Anviking a=Anviking - [x] Always balance transactions in the era of the local node tip ### Comments Previously we used to leverage the conversion between `Cardano.ProtocolParameters` and `Ledger.PParams era` to potentially "upcast" the current protocol parameters to `Ledger.PParams` to a newer era. This may have been useful for testing features in new eras on mainnet before a HF, even if the resulting txs would have been unsubmittable. Instead of doing this one can test on a public testnet, local cluster or on the unit level. ### Issue Number ADP-2990 Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> Source commit: af82118

Anviking self-assigned this May 11, 2023

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch 8 times, most recently from 446f89f to 924ddf6 Compare May 14, 2023 17:43

Anviking commented May 15, 2023

View reviewed changes

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch 4 times, most recently from 666413c to 226d906 Compare May 19, 2023 15:25

Anviking commented May 19, 2023

View reviewed changes

Anviking marked this pull request as ready for review May 19, 2023 15:29

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch from 226d906 to 1d26ef5 Compare May 22, 2023 09:55

Anviking requested review from jonathanknowles and Unisay May 22, 2023 10:00

Unisay reviewed May 22, 2023

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs Outdated Show resolved Hide resolved

Unisay reviewed May 22, 2023

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs Outdated Show resolved Hide resolved

Unisay reviewed May 22, 2023

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch from bade1dc to ac8b6e7 Compare May 23, 2023 14:12

Anviking requested a review from Unisay May 23, 2023 14:15

Anviking commented May 23, 2023

View reviewed changes

Unisay reviewed May 23, 2023

View reviewed changes

jonathanknowles reviewed May 24, 2023

View reviewed changes

Anviking commented May 25, 2023

View reviewed changes

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch from c5f3c35 to 1a267b7 Compare May 25, 2023 13:44

jonathanknowles reviewed May 26, 2023

View reviewed changes

jonathanknowles self-requested a review May 26, 2023 08:52

jonathanknowles approved these changes May 26, 2023

View reviewed changes

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch 2 times, most recently from b4fa552 to f062875 Compare May 26, 2023 14:07

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch from f062875 to cdff4f6 Compare May 26, 2023 15:06

Anviking and others added 6 commits May 26, 2023 17:07

Add missing golden & roundtrip for ApiErrorBalanceTxUnderestimatedFee

800d987

Move OldEraNotSupported outside of ErrBalanceTx

ec56aad

Rename/improve ErrNodeNotYetInRecentEra error

ef73ee3

Don't support balanceTx in eras ahead of node tip

98c1da1

Test class laws for AnyRecentEra.

0a53a48

fixup: Move "Arbitrary instances" section

a3613e8

Anviking force-pushed the anviking/ADP-2990/no-balanceTx-in-future-eras branch from cdff4f6 to a3613e8 Compare May 26, 2023 15:07

iohk-bors bot merged commit af82118 into master May 26, 2023

iohk-bors bot deleted the anviking/ADP-2990/no-balanceTx-in-future-eras branch May 26, 2023 15:31

Anviking mentioned this pull request May 31, 2023

Complete removal of Cardano.ProtocolParameters dependency for tx construction #3874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for balancing transactions in eras more recent than the node tip #3923

Drop support for balancing transactions in eras more recent than the node tip #3923

Anviking commented May 11, 2023 •

edited

Loading

Anviking May 15, 2023

Anviking May 19, 2023

Anviking May 23, 2023 •

edited

Loading

jonathanknowles May 24, 2023

Unisay May 23, 2023

Unisay May 23, 2023

jonathanknowles May 24, 2023

Anviking May 26, 2023

jonathanknowles left a comment

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023 •

edited

Loading

Anviking May 24, 2023

Anviking May 24, 2023

jonathanknowles May 24, 2023

jonathanknowles May 24, 2023

Anviking May 25, 2023

jonathanknowles left a comment

jonathanknowles May 26, 2023

jonathanknowles May 26, 2023

jonathanknowles May 26, 2023 •

edited

Loading

jonathanknowles May 26, 2023

jonathanknowles left a comment •

edited

Loading

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

Anviking commented May 26, 2023

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

iohk-bors bot commented May 26, 2023

	, "the era the local node currently is in."
	, "the era the local node is currently in."

	, "If the node is not yet fully synced, try waiting."
	, "If the node is not yet fully synchronised, try waiting."

		= ErrOldEraNotSupported Cardano.AnyCardanoEra
		\| ErrTxNotInNodeEra Write.AnyRecentEra

-    = ErrOldEraNotSupported Cardano.AnyCardanoEra
-    | ErrTxNotInNodeEra Write.AnyRecentEra
+    = ErrTxEraTooOld Cardano.AnyCardanoEra
+    -- ^ The transaction is from an era that is too old,
+    -- and no longer supported by 'balanceTransaction'.
+    | ErrTxEraTooNew Write.AnyRecentEra
+    -- ^ The transaction is from an era that is too new,
+    -- from the point of view of the node (at its current
+    -- level of synchronisation).

Drop support for balancing transactions in eras more recent than the node tip #3923

Drop support for balancing transactions in eras more recent than the node tip #3923

Conversation

Anviking commented May 11, 2023 • edited Loading

Comments

Issue Number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

Anviking commented May 26, 2023

Anviking commented May 26, 2023

iohk-bors bot commented May 26, 2023

iohk-bors bot commented May 26, 2023

Anviking commented May 11, 2023 •

edited

Loading

Anviking May 23, 2023 •

edited

Loading

jonathanknowles May 24, 2023 •

edited

Loading

jonathanknowles May 26, 2023 •

edited

Loading

jonathanknowles left a comment •

edited

Loading