Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BinaryInfo should not be a functor #2145

Closed
edsko opened this issue May 25, 2020 · 0 comments · Fixed by #2187
Closed

BinaryInfo should not be a functor #2145

edsko opened this issue May 25, 2020 · 0 comments · Fixed by #2187
Labels
consensus issues related to ouroboros-consensus technical debt

Comments

@edsko
Copy link
Contributor

edsko commented May 25, 2020

At the moment we have

data BinaryInfo blob = BinaryInfo
  { binaryBlob   :: !blob
  , headerOffset :: !Word16
    -- ^ The offset within the 'binaryBlob' at which the header starts.
  , headerSize   :: !Word16
    -- ^ How many bytes the header is long. Extracting the 'headerSize' bytes
    -- from 'binaryBlob' starting from 'headerOffset' should yield the header.

    -- In the future, i.e. Shelley, we might want to extend this to include a
    -- field to tell where the transaction body ends and where the transaction
    -- witnesses begin so we can only extract the transaction body.
  }

but that feels wrong: there is no way that a Functor instance could preserve any kind of internal invariants of this type, and even if this isn't true in general, I suspect even for specific use cases of fmap those invariants are very unlikely to be preserved.

@edsko edsko added consensus issues related to ouroboros-consensus technical debt labels May 25, 2020
iohk-bors bot added a commit that referenced this issue Jun 1, 2020
2187: Decouple BinaryInfo from the block encoder r=edsko a=mrBliss

Closes #2145.

Split

    nodeEncodeBlockWithInfo  :: CodecConfig blk -> blk -> BinaryInfo Encoding

Up into

    nodeGetBinaryBlockInfo :: blk -> BinaryBlockInfo
    nodeEncodeBlock        :: CodecConfig blk -> blk -> Encoding

where `BinaryBlockInfo` is the former `BinaryInfo` without its `blob`
argument. This also means `Binary(Block)Info` is no longer a functor, closing
issue #2145.

The reason both were coupled is that we thought that the information would be
computed during encoding, making it more efficient to do both at the same
time. However, for all real ledgers, we use annotations, making it cheap to
compute the `BinaryBlockInfo` without any real encoding work to be done.

Splitting them up will make later refactorings of the encoders/decoders
easier.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Edsko de Vries <edsko@well-typed.com>
@iohk-bors iohk-bors bot closed this as completed in 558255a Jun 1, 2020
@iohk-bors iohk-bors bot closed this as completed in #2187 Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant