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

Introduce class for GetBinaryInfo #2239

Closed
edsko opened this issue Jun 9, 2020 · 0 comments · Fixed by #2435
Closed

Introduce class for GetBinaryInfo #2239

edsko opened this issue Jun 9, 2020 · 0 comments · Fixed by #2435
Labels
consensus issues related to ouroboros-consensus technical debt

Comments

@edsko
Copy link
Contributor

edsko commented Jun 9, 2020

So we can delete it from RunNode.

@edsko edsko added consensus issues related to ouroboros-consensus technical debt priority medium labels Jun 9, 2020
@edsko edsko added this to the S16 2020-07-02 milestone Jun 9, 2020
@mrBliss mrBliss removed this from the S16 2020-07-02 milestone Jun 30, 2020
@mrBliss mrBliss removed their assignment Jun 30, 2020
iohk-bors bot added a commit that referenced this issue Jul 18, 2020
2435: Introduce HasBinaryBlockInfo and make it overridable in SerialiseHFC r=dcoutts a=mrBliss

Fixes #2239.

Instead of having a `RunNode` method to get the `BinaryBlockInfo` of a
block (offset and size of the header within the block), create a separate type
class `HasBinaryBlockInfo` for it. This is another step in the clean-up process
of `RunNode` that started in the weeks before the HFC was merged.

This means we can provide a generic implementation of `HasBinaryBlockInfo` for
`HardForkBlock` that takes the HFC wrapper into account. This wrapper adds a
listlen 2 and a tag indicating the era to the start of each block, so we need to
shift the offset of the header by 2 bytes.

More importantly, we now allow overriding the `getBinaryBlockInfo` method in
`SerialiseHFC`, just like we allow overriding the disk en/decoder for blocks.
This allows us to do the right thing for the Byron era: no era wrapper for
Byron, but start using era wrappers for the eras after it.

This also fixes a bug: `cardano-node` running in pure Shelley mode uses
`ShelleyHFC`, i.e., a `ShelleyBlock` wrapped in the HFC. Since the generic
`RunNode` instance for `HardForkBlock '[blk]` forwards everything to the
instance for `blk`, we were using the `BinaryBlockInfo` implementation for
Shelley, which didn't account for the era wrapper in the header offset. Since
we *were* using the era wrapper to serialise `ShelleyHFC` blocks on disk, this
meant we were extracting the header from the on-disk blocks incorrectly,
resulting in deserialisation failures when reading headers from disk.

This refactoring allows us to fix it properly: now the defaults for
`SerialiseHFC` do the right thing for `ShelleyHFC`, and for `ByronHFC` we
forward to `ByronBlock`, just like for all the other methods. This means
`ShelleyHFC` starts out with an era wrapper around blocks from the first era (to
allow for easier transitions to later eras, which is the right thing to do from
the start), but `ByronHFC`'s block format is binary compatible with
`ByronBlock`.

2437: Allow effects after SendMsgRelease in the local state query wrapper r=dcoutts a=dcoutts

In the LocalStateQueryClient wrapper type, allow effects in one state
where we did not allow them before. This is useful for doing effects
before looping back to the idle state.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Duncan Coutts <duncan@well-typed.com>
@iohk-bors iohk-bors bot closed this as completed in 43703d3 Jul 18, 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.

2 participants