CAD-2000: Make the protocol era-independent #1915

nc6 · 2020-10-15T13:50:02Z

Addresses https://jira.iohk.io/browse/CAD-2000

There are some additional comments in #1902 which may need to be addressed before merging.

mrBliss

Fantastic! I have been propagating Jared's branch to consensus and now everything fits nicely together 🙂

There are two problems, though (copying them from #1902). For the first one, see the inline comment about chainChecks. The second one:

We have the following class in consensus:

class ( Crypto c
      , SL.DSignable    c (OCertSignable c)
   -- , SL.DSignable    c (Hash c (TxBody era)) TODO
      , SL.KESignable   c (BHBody c)
      , SL.VRFSignable  c Seed
      ) => TPraosCrypto c

It would be a lot more convenient if that era wasn't there 🙂.

Maybe we can do something similar to what you did for the block body? I.e.:

newtype HashBBody crypto = UnsafeHashBBody {unHashBody :: (Hash crypto EraIndependentBodyHash)}

Alternatively, adding SL.DSignable (EraCrypto era) (Hash (EraCrypto era) (TxBody era)) to ShelleyBased would also well.

mrBliss · 2020-10-15T14:09:05Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Validation.hs

@@ -52,7 +52,7 @@ chainChecks ::
 ) =>
 Globals ->
 PParams era ->


Consensus needs access to the PParams when calling chainChecks. Before this PR, we could extract them from the LedgerView, but that's no longer possible with this PR. Consensus doesn't have anything else in scope where it could get the PParams from.

Potential solution: create a record with the fields needed by chainChecks (_maxBHSize, _maxBBSize, _protocolVersion), let chainChecks that record as an argument and store the record in the LedgerView.

(Without a fix, the integration in consensus is broken.)

JaredCorduan · 2020-10-15T18:29:40Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Chain.hs

@@ -245,25 +251,52 @@ instance
 initialRules = []
 transitionRules = [chainTransition]

+data ChainChecksData = ChainChecksData


I'm open to help naming this thing...

JaredCorduan · 2020-10-16T02:00:37Z

@mrBliss I've added two commits, one for each of the problems you mentioned. The second commit still has failing tests (I'm having a problem where shelley-spec-ledger-test/test/Test/Shelley/Spec/Ledger/Rules/TestChain.hs fails for me locally but not in CI, thus masking the real problems...).

I've added a ChainChecksData type to the LedgerView record, which contains the three (era independent!) protocol parameters needed by chainChecks. I'm not going to be winning any creative writing awards for my variable names here...
I have replace all the DSignable c (Hash c (TxBody era)) constraints with DSignable c (Hash c EraIndependentTxBody), and added a function eraIndTxBodyHash = coerce . hashAnnotated. I hope that this is solves the TPraosCrypto problem.

mrBliss · 2020-10-16T06:51:59Z

@JaredCorduan Thanks! I'll try them out in consensus later today.

mrBliss · 2020-10-16T06:52:45Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Protocol.hs


-instance Era era => FromCBOR (LedgerView era) where
+instance CC.Crypto crypto => FromCBOR (LedgerView crypto) where


BTW, consensus doesn't need to (de)serialise LedgerView anymore, so you can get rid of these instances (and the ones for ChainChecksData too).

mrBliss · 2020-10-16T11:40:17Z

Thanks for the fixes for the two issues I brought up, I have verified that they work out well in consensus 👍. As mentioned, I would remove the To/FromCBOR instances for LedgerView and related types, because they're no longer needed and give the false impression that we're breaking binary compatibility.

JaredCorduan · 2020-10-16T15:57:29Z

There was a quickcheck failure:

Ledger with Delegation
  Minimal Property Tests
    Chain and Ledger traces cover the relevant cases:                                                   FAIL (73.68s)
      *** Failed! (after 85 tests):
      Exception:
        out of bounds : -23
        CallStack (from HasCallStack):
          error, called at src/Shelley/Spec/Ledger/Coin.hs:77:15 in shelley-spec-ledger-0.1.0.0-Ml0lahSGZq7uCSKV4sLyI:Shelley.Spec.Ledger.Coin
      Use --quickcheck-replay=882383 to reproduce.

I am fairly certain that it is unrelated to this PR (our trace generators have somehow generated a negative coin). I think it makes sense to note the failure (we have the seed now) and rerun the tests.

mrBliss · 2020-10-16T13:02:20Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Protocol.hs

-instance CC.Crypto crypto => FromCBOR (ChainDepState crypto) where
- fromCBOR =
- decodeRecordNamed
- "ChainDepState"
- (const 3)
- ( ChainDepState
- <$> fromCBOR
- <*> fromCBOR
- <*> fromCBOR
- )
-
-instance CC.Crypto crypto => ToCBOR (ChainDepState crypto) where
- toCBOR
- ChainDepState
- { csProtocol,
- csTickn,
- csLabNonce
- } =
- mconcat
- [ encodeListLen 3,
- toCBOR csProtocol,
- toCBOR csTickn,
- toCBOR csLabNonce
- ]
-


Oh, not this, we still need this 🙂

I will resurrect it! 🧟‍♂️

mrBliss · 2020-10-16T13:03:10Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Chain.hs

+instance ToCBOR ChainChecksData where
+ toCBOR (ChainChecksData bhs bbs pv) =
+ encodeListLen 3 <> toCBOR bhs <> toCBOR bbs <> toCBOR pv
+
+instance FromCBOR ChainChecksData where
+ fromCBOR = do
+ decodeRecordNamed "ChainChecksData" (const 3) $ do
+ bhs <- fromCBOR
+ bbs <- fromCBOR
+ pv <- fromCBOR
+ pure $ ChainChecksData bhs bbs pv


This can be removed, though.

oh goodness, yes, I mistook ChainDepState for ChainChecksData. thank you @mrBliss !

ok, we are now only removing LedgerView from API/Protocol.hs, and the CBOR instances for ChainChecksData are removed from the commit that introduced them.

Highlights: * IntersectMBO/cardano-ledger#1915 * IntersectMBO/cardano-ledger#1922 * IntersectMBO/cardano-ledger#1902 * IntersectMBO/cardano-ledger#1923 * IntersectMBO/cardano-ledger#1927 * IntersectMBO/cardano-ledger#1929

2702: Update dependencies r=mrBliss a=mrBliss Highlights: * IntersectMBO/cardano-ledger#1915 * IntersectMBO/cardano-ledger#1922 * IntersectMBO/cardano-ledger#1902 * IntersectMBO/cardano-ledger#1923 * IntersectMBO/cardano-ledger#1927 * IntersectMBO/cardano-ledger#1929 Co-authored-by: Thomas Winant <thomas@well-typed.com>

nc6 requested review from JaredCorduan and uroboros as code owners October 15, 2020 13:50

mrBliss suggested changes Oct 15, 2020

View reviewed changes

JaredCorduan reviewed Oct 15, 2020

View reviewed changes

JaredCorduan force-pushed the jc/remove-era-from-protocol branch 2 times, most recently from 0c1c9af to b505135 Compare October 16, 2020 01:48

mrBliss reviewed Oct 16, 2020

View reviewed changes

mrBliss approved these changes Oct 16, 2020

View reviewed changes

JaredCorduan force-pushed the jc/remove-era-from-protocol branch from 0093810 to 2a5cf4d Compare October 16, 2020 13:58

Jared Corduan added 2 commits October 16, 2020 10:02

remove pparams from ledger view

82f31a4

remove era parametrization from PRTCL rule

3a1e5f7

JaredCorduan force-pushed the jc/remove-era-from-protocol branch from 2a5cf4d to 117f199 Compare October 16, 2020 14:12

mrBliss reviewed Oct 16, 2020

View reviewed changes

Jared Corduan added 3 commits October 16, 2020 12:40

chainChecks to use nice subset of pparams

6da3cf8

use era independent tx body hash

cd6bf41

remove LedgerView serialization

b3e74c8

JaredCorduan force-pushed the jc/remove-era-from-protocol branch from 117f199 to b3e74c8 Compare October 16, 2020 16:40

JaredCorduan merged commit bc0d9f4 into master Oct 16, 2020

iohk-bors bot deleted the jc/remove-era-from-protocol branch October 16, 2020 17:41

JaredCorduan mentioned this pull request Oct 16, 2020

Formal Spec - correct chainChecks and positional arguments in NewEpochState and EpochState #1918

Merged

mrBliss mentioned this pull request Oct 22, 2020

Update dependencies IntersectMBO/ouroboros-network#2702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAD-2000: Make the protocol era-independent #1915

CAD-2000: Make the protocol era-independent #1915

nc6 commented Oct 15, 2020

mrBliss left a comment •

edited

Loading

mrBliss Oct 15, 2020

JaredCorduan Oct 15, 2020

JaredCorduan commented Oct 16, 2020

mrBliss commented Oct 16, 2020

mrBliss Oct 16, 2020

mrBliss commented Oct 16, 2020

JaredCorduan commented Oct 16, 2020

mrBliss Oct 16, 2020

JaredCorduan Oct 16, 2020

mrBliss Oct 16, 2020

JaredCorduan Oct 16, 2020

JaredCorduan Oct 16, 2020 •

edited

Loading


		instance Era era => FromCBOR (LedgerView era) where
		instance CC.Crypto crypto => FromCBOR (LedgerView crypto) where

CAD-2000: Make the protocol era-independent #1915

CAD-2000: Make the protocol era-independent #1915

Conversation

nc6 commented Oct 15, 2020

mrBliss left a comment • edited Loading

Choose a reason for hiding this comment

mrBliss Oct 15, 2020

Choose a reason for hiding this comment

JaredCorduan Oct 15, 2020

Choose a reason for hiding this comment

JaredCorduan commented Oct 16, 2020

mrBliss commented Oct 16, 2020

mrBliss Oct 16, 2020

Choose a reason for hiding this comment

mrBliss commented Oct 16, 2020

JaredCorduan commented Oct 16, 2020

mrBliss Oct 16, 2020

Choose a reason for hiding this comment

JaredCorduan Oct 16, 2020

Choose a reason for hiding this comment

mrBliss Oct 16, 2020

Choose a reason for hiding this comment

JaredCorduan Oct 16, 2020

Choose a reason for hiding this comment

JaredCorduan Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

mrBliss left a comment •

edited

Loading

JaredCorduan Oct 16, 2020 •

edited

Loading