Add Peras certificate to the block body #5439
Conversation
agustinmista
force-pushed
the
peras/add-peras-cert-to-dijkstra-block-body
branch
from
ac6af5e to
b97a8eb
Compare
agustinmista
force-pushed
the
peras/add-peras-cert-to-dijkstra-block-body
branch
from
b97a8eb to
86f443c
Compare
teodanciu
left a comment
teodanciu
left a comment
There was a problem hiding this comment.
I don't know if on purpose, but the new BlockBlody is not actually used for Dijkstra. Other changes are also necessary once it's plugged in (like BBODY rule, as I explained in a comment).
| , AlonzoEraTx era | ||
| ) => | ||
| Lens' (BlockBody era) (StrictSeq (Tx TopTx era)) | ||
| txSeqBlockBodyDijkstraL = lens dbbTxs (\_ s -> DijkstraBlockBody s SNothing) |
There was a problem hiding this comment.
Why Nothing here? Do you want it to reset the certificate when setting the transactions? Otherwise, it should be:
| txSeqBlockBodyDijkstraL = lens dbbTxs (\_ s -> DijkstraBlockBody s SNothing) | |
| txSeqBlockBodyDijkstraL = lens dbbTxs (\bb p -> bb { dbbTxs = p}) |
Also, I think the general style and naming for this would be something like this (instead of using the type equality):
txSeqDijkstraBlockBodyL :: Lens' (DijkstraBlockBody era) (StrictSeq (Tx TopTx era))
txSeqDijkstraBlockBodyL = lens dbbTxs (\bb p -> bb { dbbTxs = p})
Or you could even inline the definition in EraBlockBody instance at txSeqBlockBodyL =
| ) => | ||
| Lens' (BlockBody era) (StrictMaybe PerasCert) | ||
| perasCertBlockBodyDijkstraL = | ||
| lens dbbPerasCert (\_ s -> DijkstraBlockBody mempty s) |
There was a problem hiding this comment.
| lens dbbPerasCert (\_ s -> DijkstraBlockBody mempty s) | |
| lens dbbPerasCert (\bb p -> bb {dbbPerasCert = p}) |
And again, a version that is more consistent would be:
perasCertDijkstraBlockBodyL :: Lens' (DijkstraBlockBody era) (StrictMaybe PerasCert)
perasCertDijkstraBlockBodyL = lens dbbPerasCert (\bb p -> bb {dbbPerasCert = p})
or if we don't need to export this function, you can just define this in DijkstraEraBlockBody method.
| , "dbbTxsBodyBytes" | ||
| , "dbbTxsWitsBytes" | ||
| , "dbbTxsAuxDataBytes" | ||
| , "dbbTxsIsValidBytes" |
There was a problem hiding this comment.
| , "dbbTxsIsValidBytes" | |
| , "dbbTxsIsValidBytes" | |
| , "dbbPerasCertBytes" |
There was a problem hiding this comment.
This module is defined, but not used! So as it is now, it is still the case that:
type BlockBody DijkstraEra = AlonzoBlockBody DijkstraEra
because of Cardano.Ledger.Dijkstra.BlockBody module.
In order to switch to the new BlockBody type, the latter should just import Cardano.Ledger.Dijkstra.BlockBody.Internal and remove the EraBlockBody DijkstraEra instance.
There was a problem hiding this comment.
I believe this entails the BBODY rule to be written for Dijkstra (or reworked to not use AlonzoBlockBody if that's possible) , because it now depends on AlonzoBlockBody - so when you plug in DijkstraBlockBody as BlockBody for DijkstraEra, BBOdy rule will fail to compile.
There was a problem hiding this comment.
Both of those points are valid, however suggested solution for BBODY is not quite right. Instead of changing the signal of DijkstraBBODY to DijkstraBlockBody, it is better to remove redundant constraint from Alonzo and Conway. See #5460
@agustinmista feel free to rebase on the above branch, or simply wait for it to get merged. That will take care of BBODY rule, however other changes are still needed:
- - Setting
type BlockBody DijkstraEra = DijkstraBlockBody DijkstraEra - - Adding
import Cardano.Ledger.Dijkstra.BlockBody ()toCardano.Ledger.Dijkstramodule. This will ensureEraBlockBodyinstance is not orphan (see EnsureEraBlockBodyinstances are not orphans. #5460) - - Re-exporting
DijkstraBlockBody(..)fromCardano.Ledger.Dijkstra.Coremodule. (Also see EnsureEraBlockBodyinstances are not orphans. #5460)
There was a problem hiding this comment.
Thanks @teodanciu and @lehins for the comments. I was rather unsure about this part, and I'm glad we don't need to touch the rules for now 😅
@lehins, regarding the changes you requested:
-
Setting type
BlockBody DijkstraEra = DijkstraBlockBody DijkstraEra- This was already the case in
Cardano.Ledger.Dijkstra.BlockBody.Internaland it's now re-exported byCardano.Ledger.Dijkstra.BlockBodyin (b580503). I hope this is what you meant.
- This was already the case in
-
Adding import Cardano.Ledger.Dijkstra.BlockBody () to Cardano.Ledger.Dijkstra module. This will ensure EraBlockBody instance is not orphan (see
EnsureEraBlockBodyinstances are not orphans. #5460)- This was already the case as of 831d2a2 😄
-
Re-exporting DijkstraBlockBody(..) from Cardano.Ledger.Dijkstra.Core module. (Also see
EnsureEraBlockBodyinstances are not orphans. #5460)- Done in b580503
lehins
left a comment
lehins
left a comment
There was a problem hiding this comment.
Aside form comments that were made in this PR reviews, it looks good.
I'm not entirely sure what you had in mind w.r.t. also storing the certificate bytes in the block body or not
I'm not super confident about the DecCBOR instance proposed here
Let's not worry about serialization too much for now, since Dijkstra serialization is very much in flux and DijkstraBlockBody serialization will likely need to be completely rewritten. So, whatever serialization has been implemented in this PR let's just leave it as-is and we'll fix it as part of this ticket #5380
There was a problem hiding this comment.
Both of those points are valid, however suggested solution for BBODY is not quite right. Instead of changing the signal of DijkstraBBODY to DijkstraBlockBody, it is better to remove redundant constraint from Alonzo and Conway. See #5460
@agustinmista feel free to rebase on the above branch, or simply wait for it to get merged. That will take care of BBODY rule, however other changes are still needed:
- - Setting
type BlockBody DijkstraEra = DijkstraBlockBody DijkstraEra - - Adding
import Cardano.Ledger.Dijkstra.BlockBody ()toCardano.Ledger.Dijkstramodule. This will ensureEraBlockBodyinstance is not orphan (see EnsureEraBlockBodyinstances are not orphans. #5460) - - Re-exporting
DijkstraBlockBody(..)fromCardano.Ledger.Dijkstra.Coremodule. (Also see EnsureEraBlockBodyinstances are not orphans. #5460)
| listLen blockBody = | ||
| 4 + if isSJust (dbbPerasCert blockBody) then 1 else 0 |
There was a problem hiding this comment.
This won't work. See https://github.com/IntersectMBO/cardano-ledger/pull/5383/files#diff-b64caef14c18f96482d8c6d1e0473f6bb389a11b2a400d65ba1ff6c750791c72R43
| listLen blockBody = | |
| 4 + if isSJust (dbbPerasCert blockBody) then 1 else 0 | |
| listLen _ = 5 |
agustinmista
force-pushed
the
peras/add-peras-cert-to-dijkstra-block-body
branch
from
86f443c to
48ff12c
Compare
|
Thanks @teodanciu and @lehins for the feedback! I have now rebased this PR on top of #5460 and addressed most of your comments (with the exception of @teodanciu's one regarding cddls) in b580503 (to be squashed down before merging). Looking forward to hear what you think 😄 |
teodanciu
left a comment
teodanciu
left a comment
There was a problem hiding this comment.
The commit you rebased against is now in master, so if you rebase against master it should disappear from this PR.
I added a commit with changelog entries - it was easier than to explain the changes, hope you don't mind.
This looks good to me now, I will let Alexey give the final approval.
This commit introduces Cardano.Ledger.Dijkstra.BlockBody.Internal as a spiritual copy of Cardano.Ledger.Alonzo.BlockBody.Internal that can be later extended with new fields needed by Peras. With the exception of `AlonzoEraTx`, which was kept verbatim, the differences between the original and copied module are: - `s/Alonzo/Dijkstra/g` - `s/alonzo/dijkstra/g` - `s/abb/dbb/g`
This commit adds an optional PerasCert to the Dijkstra block body. In addition, it defines and instantiates a DijkstraEraBlockBody type class to expose this certificate via the perasCertBlockBodyL lens. At this point, serialization does not yet account for certificates, and will be implemented and tested in a separate commit.
This commit tweaks the serialization instance of Dijkstra block bodies to take optional Peras certificates into account. This should be later enhanced with round-trip tests to ensure backwards compatibility.
agustinmista
force-pushed
the
peras/add-peras-cert-to-dijkstra-block-body
branch
from
6ffc6ec to
be67e4b
Compare
Done 😄
To the contrary. Thanks!
Great, I will wait for his approval before squashing b580503 |
Description
This PR addresses #5436 by:
DijkstraBlockBodyas a spiritual copy ofAlonzoBlockBodycardano-base). Serialization of these certificates is also mocked up at this point.Some notes:
hashDijkstraSegWitsso it also counts towards the block body hash.DecCBORinstance proposed here, especially w.r.t. backwards compatibility. Is there an existing set of tests I could try adapting for this purpose?Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).