-
Notifications
You must be signed in to change notification settings - Fork 25
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
Expose ValidationPolicy
interface
#1390
base: main
Are you sure you want to change the base?
Conversation
969dc3a
to
e325331
Compare
ef95f0a
to
8eb0fbe
Compare
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/byron/Ouroboros/Consensus/Byron/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I've left one question and several naming suggestions.
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Init.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
6d16c7d
to
2ac4c78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning some things that might lead to a much different diff.
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Ledger/Abstract.hs
Outdated
Show resolved
Hide resolved
@@ -97,6 +103,17 @@ pureLedgerResult a = LedgerResult { | |||
-- Types that inhabit this family will come from the Ledger code. | |||
type family LedgerCfg l :: Type | |||
|
|||
-- | Whether we tell the ledger layer to compute ledger events | |||
-- | |||
-- At the moment events are not emitted in any case, so they are not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by "At the moment events are not emitted in any case" and then "By passing 'OmintLedgerEvents'" being in the same comment. Are those referring to two different things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to clarify this haddock. Let me know if it looks good
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/HardFork/Combinator/B.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/unstable-mock-block/Ouroboros/Consensus/Mock/Ledger/Block.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/unstable-tutorials/Ouroboros/Consensus/Tutorial/Simple.lhs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/unstable-tutorials/Ouroboros/Consensus/Tutorial/WithEpoch.lhs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/LocalStateQuery/Server.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/byron/Ouroboros/Consensus/Byron/Ledger/Ledger.hs
Outdated
Show resolved
Hide resolved
ApplyOpts
interfaceValidationPolicy
interface
This PR is ready for another round of reviews. The final change is that validation options are exposed for the low-level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just sharing one key high-level idea.
-> LedgerCfg l | ||
-> blk | ||
-> Ticked l | ||
-> (LedgerResult l l) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary parens
-- | ||
-- Users of this function can set any validation level allowed by the | ||
-- @small-steps@ package. See "Control.State.Transition.Extended". | ||
applyBlockLedgerResultWithValidation :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my previous comments, I was envisioning something that tightly mimics applySTSOpts
https://github.com/IntersectMBO/cardano-ledger/blob/a6f276a9e3df45d69be3a593d3db6cd9dfaa7a02/libs/small-steps/src/Control/State/Transition/Extended.hs#L506-L513 --- so that eg ledger devs can have their same familiar interface
applyStsOptsBlock :: forall ep.
ApplySTSOpts ep
-> LedgerCfg l
-> blk
-> Ticked l
-> WithEvents ep (AuxLedgerEvent l) (l, [LedgerErr l]))
type family WithEvents ep ev a where
WithEvents EPReturn a = (a, [ev])
WithEvents EPDiscard a = a
where WithEvents
replaces https://github.com/IntersectMBO/cardano-ledger/blob/a6f276a9e3df45d69be3a593d3db6cd9dfaa7a02/libs/small-steps/src/Control/State/Transition/Extended.hs#L278C1-L280C28
We could even preserve their exact same signature --- except still taking a config, a block, and a ticked ledger state instead of their RuleContext
--- if we wanted to expose their sts
/s
parameter as an associated type of blk
and l
, I suppose?
type STS_ApplyBlock blk l :: Type
applySTSOpts_ApplyBlock :: forall ep s.
(s ~ STS_ApplyBlock blk l) =>
ApplySTSOpts ep ->
(LedgerCfg l, blk, Ticked l) ->
BaseM s (EventReturnType ep s (State s, [PredicateFailure s]))
I'm relatively sure that there's no redundancy in that set of inputs, but if there were, then we could change the ApplySTSOpts ep
argument to be ``ApplySTSOpts ep -> ApplySTSOpts ep, which the method definition would use to override the default
ApplySTSOpts ep` implied by the other arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we could go one step further and mimick their interface, but do we need to? I think the expressiveness of the current code is practically the same, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertions are the only difference. There'd be two benefits:
- it'd be more obvious why this function exists
- Ledger could change the definition of these extra arguments (eg add a new parameter) as they see fit, and the only thing we'd need to change is the "default values" we use in a couple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- So Alexey said explicitly that assertions should use
globalAssertionPolicy
always and nothing else. - As for changing the definition, I think the definition is almost entirely stable in the coming future.
- For the why, I could elaborate on the haddock
I pushed a commit that modifies the Shelley interface to apply ticks and blocks, and I think the result is satisfactory now. It removes the swizzle helpers. For now I defined the functions from the Ledger PR as |
Description
Exposes
ValidationPolicy
andComputeLedgerEvents
that can be passed to control whether events are computed by the ledger and whether we should enable validation in the ledger layer.This allows clients of our library to customize these arguments when using our classes (for example someone using the consensus layer to apply blocks in a streamed fashion).