Repository navigation
Leios: bounded size TxHash, EndorserBlockHash and RankingBlockHash - #2440
Conversation
4a6c88d to
82a9a98
Compare
| -- 'RankingBlockHash'. | ||
| -- | ||
| -- TODO Rule out a 'HeaderHash' of another size at compile time, as Peras does | ||
| -- with @HashSize blk ~ 32@, without requiring it of every 'RunNode' block. |
There was a problem hiding this comment.
Not done because not worth it (right now)
| -- | Uniquely identifies an endorser block in Leios. Could use 'Block SlotNo | ||
| -- EbHash' eventually, but a dedicated type is better to explore. | ||
| data LeiosPoint = MkLeiosPoint {pointSlotNo :: SlotNo, pointEbHash :: EbHash} | ||
| -- EndorserBlockHash' eventually, but a dedicated type is better to explore. |
There was a problem hiding this comment.
Maybe I should stop carrying forward clearly prototype-level comments? 🤔
82a9a98 to
2fdc037
Compare
| signDSIGN leiosSignContext rbHash (genKeyDSIGN (mkSeedFromBytes (BS.replicate 32 0))) | ||
| signDSIGN | ||
| leiosSignContext | ||
| (signableRankingBlockHash rbHash) |
There was a problem hiding this comment.
Must: Switch to signing the bytes directly and only keep an adapter branch
Also update the blueprint and the CIP with this refinement
There was a problem hiding this comment.
cardano-foundation/CIPs@7b9b55c and cardano-scaling/cardano-blueprint#98
Also a leios-main-compat branch ahead of consensus main now.
2fdc037 to
ab8010d
Compare
57b3164 to
5c40b47
Compare
jasagredo
left a comment
There was a problem hiding this comment.
The changes look reasonable, and most of them just mechanical. Approving.
The ledger now encodes IPv4 relay addresses in network byte order, which changes the example blocks and so their headers and ledger tips. The ledger state gains the last tick slot, the pools' BLS key hashes and the last endorser block announcement, and the Dijkstra example parameters change the Leios limits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Leios hashes were ByteStrings of any length, so a peer could send a hash that was not a Blake2b-256 digest. The audit's LEI-004 found such hashes reaching a fixed-width transaction cache on the prototype. Consensus also had its own endorser block type, LeiosEb. Both are now the ledger's types, whose decoders refuse what the ledger refuses: - EndorserBlockHash, TxHash and RankingBlockHash are the ledger's 32-byte hashes. toRankingBlockHash converts a HeaderHash. - EndorserBlock is a newtype over the ledger's EbReferences, a non-empty map from transaction hash to size that names no transaction twice. It keeps the bytes it was received in, so its hash is of those bytes. To hand the decoder those bytes, LeiosFetch is now an annotated codec. The wire format does not change. RankingBlockHash is a newtype whose SignableRepresentation is the hash's 32 bytes, which votes sign. The Leios prototype signs their CBOR encoding instead. The endorser block functions drop their Leios prefix and spell out EndorserBlock where the name stays short. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dijkstra no longer has StakePoolCostTooLowPOOL (ledger 992167eacb), and foldl' is in the Prelude from GHC 9.10, so import it the repo's usual way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ledger's annotator slices by offsets from the start of the message, so it must be given the whole message, not the endorser block's bytes.
FreeVars gained the minimum pool cost and margin in the ledger bump.
bfcce53 to
268df0c
Compare
nfrisby
left a comment
There was a problem hiding this comment.
Various non-blocking suggestions.
I'll post continuation of this Review after my meeting
| eb_hash = bytes .size 32 | ||
|
|
||
| ; the hash and size of each transaction the endorser block names | ||
| ; the hash and size of each transaction the endorser block names, as the |
There was a problem hiding this comment.
They specify this via the omap alias, which means the order is important. But as a mere alias, it doesn't itself enforce anything.
Should this CDDL file at least comment about order mattering/duplicates being rejected/etc?
| -- | Whatever 'encodeLeiosEb' writes around the references stays within | ||
| -- 'encodeLeiosEbMaxFramingSize', the one-off amount capacities subtract. | ||
| -- | Whatever 'encodeEndorserBlock' writes around the references stays within | ||
| -- the 'framing' that 'referencesCapacity' subtracts. |
There was a problem hiding this comment.
'framing' seems wrong here; it's a where-bound variable.
Perhaps you mean 'maxFraming' below?
|
|
||
| -- | The framing 'referencesCapacity' subtracts from a parameter. | ||
| maxFraming :: BytesSize | ||
| maxFraming = maxBound - referencesCapacity maxBound |
There was a problem hiding this comment.
This definition seems very mysterious. I'd suggest the following refactor and maybe a short comment.
| maxFraming = maxBound - referencesCapacity maxBound | |
| maxFraming = capacity - referencesCapacity capacity | |
| where capacity = maxBound |
And why is it called maxFraming? Is there maybe an explanation of why capacity = maxBound would have the biggest possible reservation for framing---some monotonicity? If it weren't CBOR, then we'd know for sure that more capacity => longer list => more framing. But due to CBOR's variable-sized integers, maybe it's possible that a longer list could take fewer bytes 🤔?
There was a problem hiding this comment.
I peeked at referencesCapacity... maybe "framing" here excludes the CBOR-encoding of ints but includes the CBOR encoding of the sequence itself?
At this point, I think I'm struggling with the fact that "framing" seems ambiguous (just the seq framing? or also the framing of each txref?), when its in the various names. So perhaps the explanatory comment could resolve that ambiguity.
| , cPeerSharingCodec :: Codec (PeerSharing addr) e m bPS | ||
| , cLeiosNotifyCodec :: Codec (LeiosNotify LeiosPoint (Header blk) LeiosVote) e m bLN | ||
| , cLeiosFetchCodec :: Codec (LeiosFetch LeiosPoint LeiosEb LeiosTx) e m bLF | ||
| , cLeiosFetchCodec :: AnnotatedCodec (LeiosFetch LeiosPoint EndorserBlock LeiosTx) e m bLF |
There was a problem hiding this comment.
Deserves a comment about why this code is AnnotatedCodec and the others aren't.
| , Ord addrNTN | ||
| , Exception e | ||
| , NFData e | ||
| , Monoid bLF |
There was a problem hiding this comment.
I guess the drivers for AnnotatedCodec require this, but the others don't 🤷
| (AnyMessage (PeerSharing addr)) | ||
| (AnyMessage (LeiosNotify LeiosPoint (Header blk) LeiosVote)) | ||
| (AnyMessage (LeiosFetch LeiosPoint LeiosEb LeiosTx)) | ||
| -- The annotated LeiosFetch codec needs the bytes, too. |
There was a problem hiding this comment.
I'd drop the "too", since this line provides only the bytes instead of the message type.
| , testCase "a definite endorser block" $ | ||
| decodeBlock (definite [CBOR.encodeWord 2, snd oneRefEb]) >>= (@?= Just (fst oneRefEb)) | ||
| , testCase "rejects an empty indefinite endorser block" $ | ||
| , testCase "keeps the bytes of an indefinite endorser block" $ do |
| decodeBlockRequest (definite [CBOR.encodeWord 1, pointOf 31]) >>= (@?= Nothing) | ||
| , testCase "an endorser block hash of 33 bytes" $ | ||
| decodeBlockRequest (definite [CBOR.encodeWord 1, pointOf 33]) >>= (@?= Nothing) | ||
| ] |
There was a problem hiding this comment.
Maybe it's too awkward/expensive to test: can we test that the decoder rejects the too-long bytes before it decodes/allocates them?
| genBenchEb ebIdx measured = do | ||
| _ <- evaluate $ V.foldl' (\n (MkTxHash h, _) -> n + BS.length h) 0 (leiosEbTxs eb) | ||
| _ <- | ||
| evaluate $ List.foldl' (\n (h, _) -> n + BS.length (hashToBytes h)) 0 (endorserBlockReferences eb) |
There was a problem hiding this comment.
Maybe use force instead of byte length? I almost suggested replacing this with hashBytes.
There was a problem hiding this comment.
All of these are requests for minor refactors, improved comments, clearer changelog fragments, etc.
Except: my question on the encodeEndorserBlockSize function might reveal a bug: I think it's assuming definite length encoding, but now we're memoizing the received bytes and those my not use definition length encoding.
Edit: I failed to notice the PR was merging while I was finishing my review. So I turned my comments in this Review and the previous into a follow-up PR #2458
| byteLimitsLeiosFetch = ProtocolSizeLimits $ \case | ||
| SingIdle -> smallByteLimit | ||
| SingBlock -> withMargin maxLeiosEbBytesSize | ||
| -- A 'MsgLeiosBlock' adds two bytes to an endorser block. Bounding its |
There was a problem hiding this comment.
You dropped withMargin. I copied withMargin from other BytesLimits in ouroboros-network. Not sure whether it should still be here or not.
There was a problem hiding this comment.
A 'MsgLeiosBlock' adds two bytes to an endorser block.
It'd be nice if the comment spelled that out: [tag, _] is 1 byte for the short definite-length sequence and one byte for the tag of value <24, right?
| StateToken st -> | ||
| forall s. | ||
| CBOR.Decoder s (SomeMessage st) | ||
| CBOR.Decoder s (Annotator ByteString st) |
There was a problem hiding this comment.
Unfortunate that Annotator isnt SomeMessageAnnotator or something like that, but alas that's upstream
| Int -> CBOR.Decoder s (Annotator ByteString st) -> CBOR.Decoder s (Int, Annotator ByteString st) | ||
| fields n = fmap ((,) n) | ||
|
|
||
| plain :: |
There was a problem hiding this comment.
| plain :: | |
| dontNeedAnnotatorBytes :: |
| , "intendedRetirementEpoch" .= String (textShow intendedRetireEpoch) | ||
| , "maxEpochForRetirement" .= String (textShow maxRetireEpoch) | ||
| ] | ||
| Dijkstra.StakePoolCostTooLowPOOL Mismatch{mismatchSupplied = certCost, mismatchExpected = protCost} -> |
There was a problem hiding this comment.
Just an upstream Dijkstra refinement, I guess?
| {-# LANGUAGE GeneralizedNewtypeDeriving #-} | ||
| {-# LANGUAGE TypeApplications #-} | ||
| {-# LANGUAGE TypeSynonymInstances #-} | ||
| {-# OPTIONS_GHC -Wno-deprecations #-} |
There was a problem hiding this comment.
Could we get a comment explaining why this Wno-deprecations is here?
Also, I generally find it preferable to avoid TypeSynonymInstances and just use the expanded type in the instance head.
| endorserBlockFromTxBytes = | ||
| mkEndorserBlock . map reference . toList | ||
| where | ||
| reference bytes = (castHash $ hashWith @HASH id bytes, fromIntegral $ BS.length bytes) |
There was a problem hiding this comment.
I don't know how much overhead it would be to explicitly involve the TxHash type here, but that'd be preferable, in my opinion.
| -- | The widest framing 'encodeEndorserBlock' writes around the references: the | ||
| -- map header. CBOR map headers have the same widths as 'cborIntBytesSize', and | ||
| -- the reference count fits 'BytesSize'. | ||
| maxFramingSize :: BytesSize |
There was a problem hiding this comment.
See my other comment about "framing" being somewhat ambiguous (just the outer seq? or also each inner hash and size's headers?)
| @@ -381,26 +374,27 @@ leiosReferencesCapacity paramLimit = paramLimit - min paramLimit framing | |||
| -- 'Ouroboros.Consensus.MiniProtocol.LeiosFetch.Type.byteLimitsLeiosFetch' | |||
| -- rejects. Make it the constitution's guardrail on that parameter, and refuse | |||
There was a problem hiding this comment.
Do we have the guardrails values yet?
There was a problem hiding this comment.
Not final, but I proposed the upper end for maxEndorserBlockReferencesSize pparam to be 600_000. So we can use that as a design ceiling.
| where | ||
| ByteSize32 minItemSize = sizeEndorserBlockReference (ByteSize32 0) | ||
|
|
||
| -- | The size of an endorser block in its CBOR encoding. |
There was a problem hiding this comment.
- Can we just as for the serialize it and take its length? Won't that use the memoized bytes, so it's not as expensive as it sounds? .... is that true even for
EndorserBlocks that we construct in the forge? ... Does aMemoBytesencode the data as its constructed? - If we're not using the memobytes, then this arithmetic should be accompanied by a comment that explains its assumption about the encoder: eg the map is always encoded as a definite-length map, etc.
There was a problem hiding this comment.
Yeah this was still a port from when we always re-encoded .. I agree that this is dangerous and needs to be likely done differently
| `RankingBlockHash` replace `EbHash` and `RbHash`, and `TxHash` is now the | ||
| ledger's transaction hash. The decoders refuse a hash of any other length. |
There was a problem hiding this comment.
| `RankingBlockHash` replace `EbHash` and `RbHash`, and `TxHash` is now the | |
| ledger's transaction hash. The decoders refuse a hash of any other length. | |
| `RankingBlockHash` replace `EbHash` and `RbHash`, and `TxHash` is now the | |
| ledger's transaction hash (full hash, not just body). The decoders refuse a hash of any other length. |
I was slow to review PR #2440 and also too unaware to notice that its merge commit was being validated while I was reviewing it. I lost the race, even though I had some good suggestions and arguably noticed a decoder bug. This PR addresses all my too-late comments on PR 2440.
Addresses LEI-004 which was found on the prototype now also on the
main-line codebase. Partly resolves input-output-hk/ouroboros-leios#1125, which now only misses the leios fetch related decoding to be bounded.As many call sites need changing anyways, this PR also uses the opportunity to name things better by avoiding abbreviations and unnecessary Leios prefixes. If you feel strong about the names, point it out, I'll change them back / to what you believe is consistent.
I'm sorry about the big diff 🤗