IntersectMBO / IntersectMBO/ouroboros-consensus
De-concretize usages of LedgerState in the Consensus codebase
- Dominant language
- Haskell
- Stars
- 67
- Forks
- 43
- Avg merge
- 5d 13h
- Merged PRs (30d)
- 43
Description
(This doesn't have to be only for the `LedgerState` data family but I thought of it as the first one that could benefit from this approach)
The Consensus codebase makes a pervasive use of the `LedgerState` data family virtually *everywhere*. This introduces a huge dependency on the stability of that data family, where any change to it will propagate quickly all through the codebase.
There is also a concern of encapsulation on each one of the components. At the time, Consensus is a very big component on its own, as everything uses the same "view" on the `LedgerState`, namely the `LedgerState` itself.
As an example we could see chain selection, we roughly need to know:
- the point for a ledger state to see if it is in the future
- the hard fork summary, which as noted in the comment is mainly a projection on real blocks
- something to apply a block on top and that tells us if it applied succesfully
For chain sync we only need something that can get us forecasts of ledger views.
For the mempool we only need something to which we can apply transactions.
... and so on.
The actual types used would be the same as they are now but with the "dependency" inverted so to say. For example, the mempool now uses a `LedgerState` because it knows a `LedgerState` is an instance of `LedgerSupportsMempool` which shows how to apply transactions. Therefore the internal state defines:
```haskell
data InternalState blk = IS {
...
, isLedgerState :: !(TickedLedgerState blk)
```
What if instead it would say:
```haskell
data InternalState blk where
IS :: (LedgerSupportsMempool l)
=> ...
-> l
-> ...
```
And then the code would work blindly with something that supports applying transactions. This way we have defined the Mempool modules without having to deal with `LedgerState` and it will be some component above the one that finally instantiates this to be `LedgerState`.
Another example would be to define a class:
```haskell
class SupportsChainSelection ... where
canGetTip :: ..
canApplyBlock :: ...
canForecast :: ...
```
which then would be required when working in chain selection, and in particular the ChainSelectionEnv won't have:
```haskell
data ChainSelEnv m blk = ChainSelEnv
{ lgrDB :: LgrDB m blk
```
but instead something like:
```haskell
data ChainSelEnv m blk where
ChainSelEnv :: SupportsChainSelection l
=> { lgrDB :: l, ...}
```
Some benefits I could see from this approach are:
- Better separation of concerns. Each component would declare what it needs to work and then `LedgerState blk` would fulfill those interfaces
- Better approach for testing? Sometimes we need full instantiation of values, what if for each test we could just provide the bare minimum to fulfill the interface.
- Easier reworks at least for `LedgerState` and in particular for UTxO-HD: if some component doesn't care for the UTxO set, on UTxO-HD we can instantiate their "required class" with `LedgerState blk mk` without specifying a concrete mk. This would be true almost for every component except block application, transaction application and utxo queries.
However I don't know:
- if I would end up with a non-resoluble/ambiguous type constraints/dependencies somewhere
- if this would have an impact on performance because GHC inlines or specializes this worse than the current code
- if this would create a super complex class dependency graph
- if all this is doable with GADTs and current GHC
Opinions?
Contributor guide
Research direction
Start by mapping the existing InternalState, ChainSelEnv, LedgerSupportsMempool, and Consensus usages of LedgerState across the Consensus codebase. Compare the proposed interfaces with the current component boundaries and tests; a concrete scope and acceptance criteria are not specified in this issue.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- haskell
- Domain
- blockchain, distributed-systems
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100