[Peras 25.5] Introduce Bytes32RealPoint#2033
Conversation
6b1337f to
0dfc5c8
Compare
| RealPoint blk -> | ||
| Bytes32RealPoint | ||
| toBytes32RealPoint (RealPoint s h) = | ||
| assert (ByteString.length (coerce h) == 32) $ |
There was a problem hiding this comment.
I am not sure this is how it should be. I think this is unsafe as assertions are not checked in production
There was a problem hiding this comment.
I would have expected this to be an invariant for any blk such that:
type instance HeaderHash blk = ShortByteStringBut if that's not the case I guess we will need to deal with the partial conversion more explicitly.
Would you prefer this to be discharged with a Maybe or with an error?
There was a problem hiding this comment.
IIRC:
- For any
blkof a concrete era (OneEra x); or theHardForkBlock xs,HeaderHash blkis semantically isomorphic to a 32-byte bytestring, but it can take slightly more compact forms (e.g. 4Word64zipped together for some eras) - For other
blks (e.g.TestBlock), we can't make any assumption on the shape ofHeaderHash blk
There was a problem hiding this comment.
Thanks @tbagrel1 for the extra context.
FTR: we still need to decide how to discharge errors when coercing from a (potentially) non-32bytes-long ShortByteString into Bytes32RealPoint, as this is not captured at the type level.
What we need to address concretely here is whether we:
- Capture this with a
Maybeand let the client code (likely the HFC?) handle it. - Assume it can't happen and put an
errorfor the impossible case. - Assume it can't happen and wrap the expression with an assertion.
- Something else?
Then (1) and (2) will come with some minor performance penalty (length check and optionally wrapping on Maybe) that we can avoid if we already know that forall blk. Coercible (HeaderHash blk) ShortByteString implies len(header_hash) == 32 in any production context where this function could be invoked. On the other hand (3) should have no performance penalty in production, while still being able to detect issues in testing.
I'm OK with any of these approaches, but I would like to hear back from @jasagredo before making any change :)
There was a problem hiding this comment.
Hey we just discussed this with @agustinmista, and here's a more refined context/question:
- We will only convert from
(Real)Point blktoBytes32RealPointin contexts in which it's morally safe to do it; that is, forblkof a concrete era or the HardForkBlock. So ideally the conversion functions should probably NOT return aMaybe(to avoid having to deal with impossible cases at call site). Do you agree? - Is it ok to assume that
OneEra blkorAllEra blks(ForHardForkBlock blks) are the right classes that carry the moral invariant that a header hash carries 32 bytes of information? If not, what would be the right class? - If the previous one is correct, would it be ok to create a conversion class from
(Real)Point blktoBytes32RealPointand add it as a superclass constraint forOneEra blk? This way I think it would be easier to implement the converter for the different concrete shape of 32-bytes headerhash (e.g. Bytestring, 4Word64s, etc)
0dfc5c8 to
eb09af0
Compare
eb09af0 to
4fd81d9
Compare
5236102 to
ed23ba7
Compare
4fd81d9 to
e204347
Compare
ed23ba7 to
f47566d
Compare
This commit introduces a specialization of RealPoint for blocks where the header hash is always 32 bytes long.
e204347 to
186331b
Compare
f47566d to
f8288a3
Compare
|
Hey @jasagredo! ✋ Do you think you could take a look at the remaining comment on this PR? This is currently blocking Peras 26 from being merged 😅 Thanks in advance! CC @dnadales in case you have time to chime in instead. |
This PR introduces a specialization of
RealPointfor blocks where the header hash is always 32 bytes long.