fix ValueNotConservedUTxO serialization #1955
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JaredCorduan
commented
Oct 29, 2020
| !(Core.Value era) -- the Coin consumed by this transaction | ||
| !(Core.Value era) -- the Coin produced by this transaction | ||
| !(Delta (Core.Value era)) -- the Coin consumed by this transaction | ||
| !(Delta (Core.Value era)) -- the Coin produced by this transaction |
There was a problem hiding this comment.
@mrBliss - is it okay that I have changed the serialization for this predicate failure? I do not know who all uses the failure serializations.
There was a problem hiding this comment.
I would expect forall (x :: Word64). toCBOR (word64ToCoin x) == toCBOR (toDelta (word64ToCoin x) to be true, in which case this change is fine.
UPDATE: QuickCheck doesn't find a counterexample on my machine:
prop_DeltaCoin_binary_compat :: Word64 -> Property
prop_DeltaCoin_binary_compat x =
serialize' (word64ToCoin x) === serialize' (toDelta (word64ToCoin x))
> quickCheckWith prop_DeltaCoin_binary_compat
+++ OK, passed 100 tests.So I believe this change is safe.
There was a problem hiding this comment.
ok, great! thank you for (quick) checking!
JaredCorduan
commented
Oct 29, 2020
| tx = Tx @C txbody wits SNothing | ||
| st = runShelleyBase $ applySTSTest @(LEDGER C) (TRC (ledgerEnv, (utxoState, dpState), tx)) | ||
| -- We test that the serialization of the predicate failure does not return bottom | ||
| in serialize' st @?= serialize' st |
There was a problem hiding this comment.
is there a less silly way to enforce that the serialization does not return bottom?
There was a problem hiding this comment.
You could use rnf, deepseq, or force on serialize' st. What you have works too.
(there are two spaces before the second st)
mrBliss
reviewed
Oct 30, 2020
| @@ -40,6 +41,10 @@ type ShelleyBased era = | |||
| ChainData (Value era), | |||
| SerialisableData (Value era), | |||
| SerialisableData (CompactForm (Value era)), | |||
| ChainData (Delta (Value era)), | |||
| SerialisableData (Delta (Value era)), | |||
| Eq (Delta (Value era)), | |||
There was a problem hiding this comment.
ChainData includes Eq already.
| !(Core.Value era) -- the Coin consumed by this transaction | ||
| !(Core.Value era) -- the Coin produced by this transaction | ||
| !(Delta (Core.Value era)) -- the Coin consumed by this transaction | ||
| !(Delta (Core.Value era)) -- the Coin produced by this transaction |
There was a problem hiding this comment.
I would expect forall (x :: Word64). toCBOR (word64ToCoin x) == toCBOR (toDelta (word64ToCoin x) to be true, in which case this change is fine.
UPDATE: QuickCheck doesn't find a counterexample on my machine:
prop_DeltaCoin_binary_compat :: Word64 -> Property
prop_DeltaCoin_binary_compat x =
serialize' (word64ToCoin x) === serialize' (toDelta (word64ToCoin x))
> quickCheckWith prop_DeltaCoin_binary_compat
+++ OK, passed 100 tests.So I believe this change is safe.
mrBliss
reviewed
Oct 30, 2020
| instance Torsor.Torsor Coin where | ||
| type Delta Coin = DeltaCoin | ||
| addDelta = addDelta | ||
| toDelta = toDelta |
There was a problem hiding this comment.
There's a danger of the following happening in modules that import both:
Ambiguous occurrence ‘toDelta’
It could refer to
either ‘Cardano.Ledger.Torsor.toDelta’,
imported from ‘Cardano.Ledger.Torsor’
or ‘Shelley.Spec.Ledger.Coin.toDelta’,
imported from ‘Shelley.Spec.Ledger.Coin’
toDelta :: Coin -> DeltaCoin could be renamed to toDeltaCoin?
dcoutts
approved these changes
Oct 30, 2020
The ValueNotConservedUTxO predicate failure contains two values of type Coin (in the Shelley era, but Core.Value in general). Though the Coin values in the ledger state are always within the proper bounds (ie Word64), the failure could produce values outside of this range. This caused a serialization error for ValueNotConservedUTxO, since we do not allow the serialization of coins out of the Word64 range. For the Coin type, we already had a wrapper DeltaCoin which can be used when we want to intentionally serialize any Coin. This is now generalized to a Torsor type class, which ValueNotConservedUTxO now uses. Resolves: CAD-2168
JaredCorduan
force-pushed
the
jc/fix-value-not-conserved-serialization
branch
from
70339b9
to
b57ab4e
Compare
|
bors merge |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 30, 2020
1955: fix ValueNotConservedUTxO serialization r=mrBliss a=JaredCorduan
The `ValueNotConservedUTxO` predicate failure contains two values of type `Coin` (in the Shelley era, but `Core.Value` in general). Though the `Coin` values in the ledger state are always within the proper bounds (ie `Word64`), the failure could produce values outside of this range. This caused a serialization error for `ValueNotConservedUTxO`, since we do not allow the serialization of coins out of the `Word64` range.
For the `Coin` type, we already had a wrapper `DeltaCoin` which can be used when we want to intentionally serialize any `Coin`. This is now generalized to a `Torsor` type class, which `ValueNotConservedUTxO` now uses (I'm happy to have help with the name...).
Co-authored-by: Jared Corduan <jared.corduan@iohk.io>
|
Sorry bors, too slow |
|
bors cancel |
mrBliss
added a commit
to IntersectMBO/ouroboros-network
that referenced
this pull request
Oct 30, 2020
iohk-bors bot
added a commit
to IntersectMBO/ouroboros-network
that referenced
this pull request
Oct 30, 2020
2719: Update dependency on cardano-ledger-specs r=mrBliss a=mrBliss Reason: IntersectMBO/cardano-ledger#1955 Co-authored-by: Thomas Winant <thomas@well-typed.com>
mrBliss
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 30, 2020
This updates the dependencies on `cardano-ledger-specs` and `ouroboros-network` to bring in IntersectMBO/cardano-ledger#1955 Other visible change: IntersectMBO/ouroboros-network#2714
|
This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried. |
|
bors cancel |
mrBliss
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 30, 2020
This updates the dependencies on `cardano-ledger-specs` and `ouroboros-network` to bring in IntersectMBO/cardano-ledger#1955 Other visible change: IntersectMBO/ouroboros-network#2714
iohk-bors bot
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 30, 2020
2039: Update: fix ValueNotConservedUTxO serialization r=mrBliss a=mrBliss This updates the dependencies on `cardano-ledger-specs` and `ouroboros-network` to bring in IntersectMBO/cardano-ledger#1955 Other visible change: IntersectMBO/ouroboros-network#2714 Co-authored-by: Thomas Winant <thomas@well-typed.com>
JaredCorduan
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 3, 2020
This updates the dependencies on `cardano-ledger-specs` and `ouroboros-network` to bring in IntersectMBO/cardano-ledger#1955 Other visible change: IntersectMBO/ouroboros-network#2714
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ValueNotConservedUTxOpredicate failure contains two values of typeCoin(in the Shelley era, butCore.Valuein general). Though theCoinvalues in the ledger state are always within the proper bounds (ieWord64), the failure could produce values outside of this range. This caused a serialization error forValueNotConservedUTxO, since we do not allow the serialization of coins out of theWord64range.For the
Cointype, we already had a wrapperDeltaCoinwhich can be used when we want to intentionally serialize anyCoin. This is now generalized to aTorsortype class, whichValueNotConservedUTxOnow uses (I'm happy to have help with the name...).