Support Byron Epoch boundary blocks #704

nc6 · 2019-07-03T14:02:50Z

This PR introduces the following:

Updates to cardano-ledger. This means changing a few things which have been updated in that repo, such as the naming for the proxy delegation certificates. The updates are needed to expose functions for dealing with serialisation of epoch boundary blocks.
Introduce a protocol extension WithEBBs. The reason for this abstraction is that it still makes more sense to associate the "raw" ByronBlock with PBFT. If instead we used ByronBlockOrEBB, everything would have to have associated Nothing cases where the block turned out to be an EBB. Instead, we keep the association of PBFT with the raw ByronBlock and associate ByronBlockOrEBB with the extension WithEBBs PBFT.

In spite of retaining ByronBlock for this reason, in most cases everything defined over ByronBlock is now defined over ByronBlockOrEBB.

This is still in draft, since there's an assertion failure in AnchoredFragment which I don't yet understand.

mrBliss

Nice work!

I'll take a look at #705 too, because without fixing that, we can't test this.

mrBliss · 2019-07-03T14:46:35Z

cabal.project

@@ -44,25 +44,25 @@ source-repository-package
 source-repository-package
  type: git
  location: https://github.com/input-output-hk/cardano-ledger
-  tag: 7f5263eac329d73a0626fc0d9603dec2cd51d352
+  tag: ebc1a24f5535d237616b92023eab80b998816807


Did you manage to use @erikd's cardano-repo-tool for this?

ouroboros-consensus/ouroboros-consensus.cabal

ouroboros-consensus/src/Ouroboros/Consensus/Demo/Ledger/Byron/Config.hs

ouroboros-consensus/src/Ouroboros/Consensus/Protocol/WithEBBs.hs

ouroboros-network/src/Ouroboros/Network/AnchoredFragment.hs

ouroboros-consensus/src/Ouroboros/Consensus/Demo/HasCreator.hs

ouroboros-consensus/src/Ouroboros/Consensus/Protocol/WithEBBs.hs

ouroboros-consensus/src/Ouroboros/Consensus/Demo/Ledger/Byron/Forge.hs

mrBliss · 2019-07-03T15:34:07Z

This depends on cardano-ledger#566, which still needs to be merged.

nc6 · 2019-07-03T15:37:40Z

Rebased atop master, addressed a bunch of the comments.

avieth · 2019-07-03T15:34:20Z

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Byron.hs

+type ByronGiven = (
+    Given Crypto.ProtocolMagicId
+  , Given CC.Slot.EpochSlots
+  )


It was done this way in cardano-sl, as an improvement over the original design that used template haskell. Both are less than ideal: with TH, each build is fixed to one protocol magic; with reflections, it can be changed by runtime configuration, but it's not at all clear whether one process is capable of using two different protocol magics.

Would be considered to be old fashioned to suggest that we just use parameter passing for this?

That'd be my ideal solution. But since the design uses the typeclass HasHeader, something that makes these parameters seem static is needed (otherwise the instances couldn't be implemented).

Something more like an ML module would be nice here. Then the particular values of protocol magic and epoch slots could determine a new type for a byron block.

mrBliss · 2019-07-03T16:34:01Z

stack.yaml

@@ -47,6 +52,7 @@ extra-deps:
    commit: e8bfc1294e088f90e5ae0b4aedbc82ee46ac5ee4

  - bimap-0.4.0
+  - binary-0.8.7.0


Why is this needed?

Whenever I don't have it, I can't build. Not sure why nobody else seems to have this problem...

Aarrgh, this line has cost me soooo much time rebuilding when switching branches: stack always has to build +100 dependencies whenever the version of binary changes in stack.yaml. Why can't stack remember that it has built these dependencies before? (I'm not blaming you, just venting 🙂 )

I would like to, but using cabal new-build in a nix-shell is still recompiling my dependencies, which is defeating the point 🤦‍♂️

avieth · 2019-07-03T21:05:30Z

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Byron.hs

+
+newtype ByronBlockOrEBB cfg = ByronBlockOrEBB
+  { unByronBlockOrEBB :: CC.Block.ABlockOrBoundary ByteString
+  } deriving (Eq, Show)


For byron-proxy, I'll need a type which includes Byron blocks and EBBs, but which also can be serialised. Since there's no way to serialise an ABlockOrBoundary ByteString, what I've done there is use

type Block = Annotated (ABlockOrBoundary ByteString) ByteString

as the block type, and encoded it using CBOR tag 24 so that when I decode it from the wire I can give the top-level annotation (I have the bytes).

Now I'm working on using ChainDB in byron-proxy, so I'll face the same problem with the block type chosen to go in the ChainDB. I'll need the top-level annotation there as well. Perhaps we could change ByronBlockOrEBB to include this?

This is also important for using the ChainDB to serve a Byron peer. That requires coming up with legacy cardano-sl Block and BlockHeader values, but if I'm using a ChainDB that uses new cardano-ledger variants, I can't do that without having the bytes from which they were decoded.

I don't understand this comment - why can't you serialise an ABlockOrBoundary ByteString? It's already annotated, and we have explicit serialisers for it (toCBORABOBBlock and toCBORABOBBoundary, but we could easily wrap these together).

ABlockOrBoundary ByteString couldn't be serialized, but now can since you recently merged a pull request into cardano-ledger.

For BoundaryValidationData a the annotation a is for the header bytes only, so ABOBBoundary (BoundaryValidationData ByteString) doesn't have the bytes for the whole block. That's why I put the annotation out front. Looks like the toCBORABOBBoundary just makes up a body... I'm not sure if that's actually ok. The Byron peers might not accept it.

ouroboros-consensus/src/Ouroboros/Consensus/Demo/Ledger/Byron/Forge.hs

mrBliss · 2019-07-04T09:05:44Z

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Byron.hs

+annotateBoundary :: Crypto.ProtocolMagicId
+                 -> CC.Block.BoundaryValidationData ()
+                 -> CC.Block.BoundaryValidationData ByteString
+annotateBoundary pm =


While debugging the demo with --real-pbft (#705): I discovered that there must be a bug somewhere in this function (or in cardano-ledger#566): if I give it an EBB with boundaryPrevHash = Left _, the block I get back after annotateBoundary suddenly has boundaryPrevHash = Right _. If I replace annotateBoundary with fmap (const mempty), then boundaryPrevHash remains Left _. This is the reason why the first produced block doesn't fit onto the chain.

Can you add a round-trip QC property for fromCBOR.. . toCBOR.. + find the actual bug? 🙂

mrBliss · 2019-07-15T12:33:34Z

ouroboros-network/src/Ouroboros/Network/Chain.hs

@@ -123,7 +123,10 @@ validExtension
 validExtension c b = blockInvariant b
                  && headHash c == blockPrevHash b
                  && headSlot c <  blockSlot b
-                  && headBlockNo c == pred (blockNo b)
+                  -- TODO This is a hack which should be addressed through 'WithOrigin'


@avieth when can we expect the sequel of #706? 🙂 We need WithOrigin for BlockNo (in addition to SlotNo) too, as the genesis EBB has BlockNo 0, the empty chain cannot have BlockNo 0 too.

If you're busy with the byron-proxy, I could take a stab at it instead.

I did this, for ouroboros-network only #774

It would probably be best if you did the follow up for ouroboros-consensus. I found there were some non-obvious choices to be made in there.

mrBliss · 2019-07-16T09:53:22Z

Sorry @nc6, but #727 caused some merge conflicts. It's mostly moving/splitting/renaming files, but some identifiers were renamed too.

- Updates to cardano-ledger. This means changing a few things which have been updated in that repo, such as the naming for the proxy delegation certificates. The updates are needed to expose functions for dealing with serialisation of epoch boundary blocks. - Introduce a protocol extension WithEBBs. The reason for this abstraction is that it still makes more sense to associate the "raw" ByronBlock with PBFT. If instead we used ByronBlockOrEBB, everything would have to have associated Nothing cases where the block turned out to be an EBB. Instead, we keep the association of PBFT with the raw ByronBlock and associate ByronBlockOrEBB with the extension WithEBBs PBFT. In spite of retaining ByronBlock for this reason, in most cases everything defined over ByronBlock is now defined over ByronBlockOrEBB. - Change a few constraints which require block numbers to be sequential (whereas EBBs share the same block - and slot - numbers as other blocks). These constraints should ultimately become part of a new 'blockInvariant' method.

nc6 requested a review from mrBliss July 3, 2019 14:02

nc6 marked this pull request as ready for review July 3, 2019 14:36

mrBliss suggested changes Jul 3, 2019

View reviewed changes

nc6 force-pushed the nc/ebb branch from f99996a to 56f68b1 Compare July 3, 2019 15:37

avieth reviewed Jul 3, 2019

View reviewed changes

mrBliss reviewed Jul 3, 2019

View reviewed changes

avieth reviewed Jul 3, 2019

View reviewed changes

mrBliss reviewed Jul 4, 2019

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Demo/Ledger/Byron/Forge.hs Outdated Show resolved Hide resolved

mrBliss reviewed Jul 4, 2019

View reviewed changes

nc6 force-pushed the nc/ebb branch from dd53b3b to ce5a642 Compare July 11, 2019 11:00

mrBliss mentioned this pull request Jul 15, 2019

Convert old mainnet epoch files into ImmutableDB epoch files #756

Closed

nc6 force-pushed the nc/ebb branch from 1708a1f to a4481a2 Compare July 15, 2019 10:28

mrBliss reviewed Jul 15, 2019

View reviewed changes

nc6 force-pushed the nc/ebb branch 2 times, most recently from 343a733 to ad4f2b0 Compare July 16, 2019 13:49

mrBliss approved these changes Jul 17, 2019

View reviewed changes

nc6 force-pushed the nc/ebb branch from 8463753 to 6dbad85 Compare July 17, 2019 12:40

nc6 force-pushed the nc/ebb branch from 6dbad85 to d90ae0c Compare July 17, 2019 12:41

nc6 merged commit 3d9fbb3 into master Jul 17, 2019

iohk-bors bot deleted the nc/ebb branch July 17, 2019 13:27

mrBliss added the consensus issues related to ouroboros-consensus label Aug 2, 2019

nfrisby mentioned this pull request Aug 15, 2019

Resolve semantic conflict between WithEBBs and CS intersections at EBBs #925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Byron Epoch boundary blocks #704

Support Byron Epoch boundary blocks #704

nc6 commented Jul 3, 2019

mrBliss left a comment

mrBliss Jul 3, 2019

mrBliss commented Jul 3, 2019

nc6 commented Jul 3, 2019

avieth Jul 3, 2019

erikd Jul 4, 2019

avieth Jul 9, 2019

avieth Jul 9, 2019

mrBliss Jul 3, 2019

nc6 Jul 4, 2019

mrBliss Jul 4, 2019

avieth Jul 12, 2019

mrBliss Jul 12, 2019

avieth Jul 3, 2019

avieth Jul 10, 2019

nc6 Jul 12, 2019

avieth Jul 12, 2019

avieth Jul 12, 2019

mrBliss Jul 4, 2019

mrBliss Jul 15, 2019

avieth Jul 15, 2019

mrBliss commented Jul 16, 2019

Support Byron Epoch boundary blocks #704

Support Byron Epoch boundary blocks #704

Conversation

nc6 commented Jul 3, 2019

mrBliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss commented Jul 3, 2019

nc6 commented Jul 3, 2019

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

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

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

mrBliss commented Jul 16, 2019