Repository navigation
Conversation
agustinmista
left a comment
There was a problem hiding this comment.
Looking good! Just some small nits.
fd59e15 to
7f29c17
Compare
agustinmista
left a comment
There was a problem hiding this comment.
This looks great! Thanks for expanding the previous test suite 🧪
It looks like cabal-format is complaining in CI, though.
7000c81 to
e4bb2dd
Compare
I think you should be doing this in this PR to prove that claim and so we end up with the same number of bitfield implementations. |
|
|
||
| -- | A compact bitmap representation using serialisation-ready ByteStrings. | ||
| -- | ||
| -- Adapted from @Cardano.Leios.BitMapPV@ in the @leios-wfa-ls-demo@ package. |
There was a problem hiding this comment.
This was a proof of concept. You should instead base your implementation on the production grade version introduced in cardano-crypto-leios
|
|
||
| -- | A bitmap over a number of indexes known to its users: @⌈n/8⌉@ bytes, | ||
| -- MSB-first, bit @i@ set iff index @i@ is set. | ||
| newtype Bitmap = Bitmap {bitmapBytes :: ByteString} |
There was a problem hiding this comment.
This is not a strict container if at its core is a lazy bytestring. Should be a strict bytestring or a ByteArray
There was a problem hiding this comment.
It was actually a strict ByteString, but now uses a ByteArray as the cardano-crypto-leios impl anyway.
…ith an explicit number of indexes
e4bb2dd to
d2ff278
Compare
@ch1bo I have modified the implementation essentially the same as what was in current One remark about well-formedness: |
bladyjoker
left a comment
There was a problem hiding this comment.
Thanks @gdeest !
I have one topic to discuss, and that is to make a safe constructor for the Bitmap that checks its properties and hide the data constructor.
Since that data constructor would be given an Int denoting the cardinality/size, we can strip this argument from the rest of the API and be explicit with what this Bitmap is (ie. wellFormed).
In short
bitmapFromByteArray :: Int -> ByteArray -> Either String Bitmap
data Bitmap = Bitmap { bitmapBytes :: ByteArray, bitmapSize :: Int }| deriving (NoThunks) via OnlyCheckWhnfNamed "Bitmap" Bitmap | ||
|
|
||
| -- | The number of indexes set (flipped to 1) in the bitmap. | ||
| numSetBits :: Bitmap -> Int |
There was a problem hiding this comment.
Not sure what that function is intended to do, atm it can count set padding bit as valid indices which is probably incorrect. I think we either need n here as well, otherwise when enforced a zero padding this can then also work.
| -- | A 'LeiosCert' signers bitfield: @⌈leiosCommitteeSize\/8⌉@ bytes, MSB-first, | ||
| -- bit @i@ set iff seat @i@ signed. Newtype so it isn't confused with raw bytes. | ||
| newtype BitField = BitField {bitFieldBytes :: ByteArray} | ||
| newtype BitField = BitField {bitFieldBitmap :: Bitmap} |
There was a problem hiding this comment.
I'm considering that we should leave this a ByteArray (as in, we don't know much about this) and then use a a safe bitmapFromByteArray :: Int -> Either String Bitmap constructor?
| , testBit w (7 - bitIx) | ||
| ] | ||
|
|
||
| -- | Whether a bitmap is well-formed over the given number of indexes: it has |
There was a problem hiding this comment.
This is a bit under commented, can you expand on the invariants it checks for?
Consider wellFormed :: Int -> Bitmap -> Either String () and have a description of what went wrong.
| Left (MalformedCommittee n) | ||
| -- Bitfield length is fixed at ⌈committee/8⌉ bytes; anything else is malformed. | ||
| when (sizeofByteArray (bitFieldBytes cert.leiosCertSigners) /= (n + 7) `div` 8) $ | ||
| when (Bitmap.sizeInBytes cert.leiosCertSigners.bitFieldBitmap /= Bitmap.numBytes n) $ |
There was a problem hiding this comment.
Please introduce a wellFormed check here rather.
Adds
Data.Bitmaptocardano-strict-containers(moved fromOuroboros.Consensus.Util.Bitmap).Now uses MSB-encoding so that it can be easily re-used by Leios
BitFieldrequirements.Used by #701 .
Checklist
CHANGELOG.mdfor the affected packages.New section is never added with the code changes. (See RELEASING.md)
.cabalandCHANGELOG.mdfiles according to theversioning process.
.cabalfiles for all affected packages are updated.If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)