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

Jump to bottom

Expose ledger event logging #3292

Merged
merged 3 commits into from Aug 31, 2021
Merged

Expose ledger event logging #3292

merged 3 commits into from Aug 31, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Aug 10, 2021 •
edited

Resolves CAD-3294.

@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 12, 2021 •
edited

@JaredCorduan @goolord I've altered the Consensus code here to use applyBlockOpts and applyTickOpts. Is there anything more than that I should be using, at least to get the crucial new events?

@erikd @kderme I think the new (re)applyBlockLedgerM and tickAndThen(Re)ApplyLedgerM (along with the evalLedgerT and lrEvents functions) -- all defined in Ouroboros.Consensus.Ledger.Abstract and Ouroboros.Consensus.Ledger.Monad should give you access to the ledger events. Would you look over that and please let me know if you think something is missing?

@jasagredo @dnadales Please review this, as best you can without having talked to me about it. Let me know if something looks totally wrong eg. Thanks!

@nfrisby nfrisby force-pushed the nfrisby/cls-events branch 4 times, most recently from 39920d5 to 99ef352 Compare August 12, 2021 04:21
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 12, 2021

The windows GitHub Action failed on test-cddl, but the ubuntu Action passed. I'm guessing timeout, but it wasn't clear. So I've clicked Re-run.

@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 12, 2021

@JaredCorduan Would you also review the concrete Opts choices I made for each of applyBlockLedgerM, reapplyBlockLedgerM, and applyChainTickLedgerM? Thanks.

@JaredCorduan
Copy link
Contributor

@JaredCorduan @goolord I've altered the Consensus code here to use applyBlockOpts and applyTickOpts. Is there anything more than that I should be using, at least to get the crucial new events?

That is all I am aware of, that will get you the new events.

@JaredCorduan
Copy link
Contributor

@JaredCorduan Would you also review the concrete Opts choices I made for each of applyBlockLedgerM, reapplyBlockLedgerM, and applyChainTickLedgerM? Thanks.

@goolord can correct me if I am wrong, but I was expecting EPDiscard to be used during the normal operation of the node. I see that the new applyLedgerBlock just throws away the events.

JaredCorduan
JaredCorduan reviewed Aug 12, 2021
View reviewed changes
cabal.project Outdated Show resolved Hide resolved
@goolord
Copy link
Contributor

goolord commented Aug 12, 2021 •
edited

@goolord can correct me if I am wrong, but I was expecting EPDiscard to be used during the normal operation of the node. I see that the new applyLedgerBlock just throws away the events.

yes, this is the case

@JaredCorduan @goolord I've altered the Consensus code here to use applyBlockOpts and applyTickOpts. Is there anything more than that I should be using, at least to get the crucial new events?

anything that calls applySTSOpts internally could emit events:
https://github.com/input-output-hk/ouroboros-network/blob/2e801bd2fd136267906f5662717f6090444e8298/ouroboros-consensus-byronspec/src/Ouroboros/Consensus/ByronSpec/Ledger/Rules.hs#L145
https://github.com/input-output-hk/ouroboros-network/blob/2e801bd2fd136267906f5662717f6090444e8298/ouroboros-consensus-byronspec/src/Ouroboros/Consensus/ByronSpec/Ledger/Rules.hs#L259

what we want to emit events is outside the scope of my understanding

the goal of the event policy was twofold: introduce minimal change to the api, and keep evaluation cheap in the case the user doesn't want events.

there are some places in the ledger spec code where events are implicitly discarded (usually anywhere where predicates or assertions are not checked, in reapplySTS for example) but hopefully none of these are problematic.

@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 13, 2021

@goolord

anything that calls applySTSOpts internally could emit events:

Can you enumerate that list for me? I think you know the call graph better than I do. Thanks.

Right now this PR only catches events from validating a block and from ticking the ledger state through time. I'm specifically trying to determine if that's sufficient for the "key events" your recent PR added, the ones that db-sync are waiting for.

@nc6
Copy link
Contributor

nc6 commented Aug 18, 2021

This PR was sufficient for me to thread events through cardano-api and expose them. So looks good to me!

@nfrisby nfrisby marked this pull request as ready for review August 19, 2021 18:34
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 19, 2021

@EncodePanda @jasagredo @dnadales Would one of you please review this for typos/preservation of core semantics? The main change: we added a ([AuxLedgerEvent l], a) layer to applyBlock, reapplyBlock, and applyChainTick. So I'm specifically asking you to confirm that the a part of that tuple (ie the lrResult field) is correct.

Normally I'd ask Clarke for review, but he and I have basically been taking turns contributing to this PR.

Remaining work before merge: clean-up git history. (Right now, I suggest reviewing the entire diff all at once -- don't look at individual commits.)

@nc6 With this latest commit, it's just fmap lrResult instead of evalLedgerT. And reapplyBlock* is no longer monadic at all (so no more runIdentity either).

@nfrisby nfrisby mentioned this pull request Aug 19, 2021
Merged
@EncodePanda EncodePanda self-requested a review August 20, 2021 13:08
jasagredo
jasagredo reviewed Aug 23, 2021
View reviewed changes
Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks fine to me. Everything seems to be returning what it already was plus the new events when relevant, which is as it should be.

I just have some small comments here and there.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/HardFork/Combinator/A.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Basics.hs Outdated Show resolved Hide resolved
nfrisby
nfrisby commented Aug 24, 2021
View reviewed changes
Copy link
Contributor Author

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are from our collective phone call reviewing this PR.

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Dual.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Basics.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Basics.hs Outdated Show resolved Hide resolved
@nfrisby nfrisby force-pushed the nfrisby/cls-events branch 2 times, most recently from b84b4b2 to 95a11a7 Compare August 24, 2021 22:46
@nfrisby nfrisby mentioned this pull request Aug 24, 2021
Open
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 24, 2021

I've updated the commits and I think the question about SL.reapplyBlockOpts is the only thing preventing Approval and Merge. If you get around to that before I'm online tomorrow, please feel free to do both: @jasagredo @dnadales @nc6.

nfrisby and others added 2 commits August 31, 2021 05:54
@nfrisby
consensus: define sequenceHardForkState
f75a8a4 
This is a thin wrapper around `Telescope.sequence`, so it is also essentially a
variant of 'hsequence' that only requires `Functor` instead of `Monad`.
@nc6 @nfrisby
consensus: expose the ledger's events for ticks and block applications
c1abf51 
- This commit adds minimal new functions for passing through the ledger's
events when (re)applying a block or ticking the ledger state.

- We preserve the signatures of old functions that do the same thing but
without exposing the events.

- Specifically, the old functions immediately discard the events; so we're
  likely only paying for a couple short-lived thunks. Which should be
  negligible compared to the actual ledger computations.

- The new functions that expose the ledger events are not used by the node
  itself. They're only used by analysis tools such as db-sync.
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 31, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 31, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit af14789 into master Aug 31, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/cls-events branch August 31, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaredCorduan JaredCorduan JaredCorduan left review comments

@jasagredo jasagredo jasagredo left review comments

@nc6 nc6 nc6 approved these changes

@goolord goolord goolord approved these changes

@dnadales dnadales Awaiting requested review from dnadales

@EncodePanda EncodePanda Awaiting requested review from EncodePanda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nfrisby @JaredCorduan @goolord @nc6 @jasagredo