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

[IBC] Add Event Logging System (ICS-24) #824

Closed
14 tasks
h5law opened this issue Jun 14, 2023 · 5 comments · Fixed by #893
Closed
14 tasks

[IBC] Add Event Logging System (ICS-24) #824

h5law opened this issue Jun 14, 2023 · 5 comments · Fixed by #893
Assignees
Labels
ibc IBC specific changes

Comments

@h5law
Copy link
Contributor

h5law commented Jun 14, 2023

Objective

ICS-24 details an Event Logging system that is to be used by the relayers to retrieve packet data and timeout information. As the IBC stores are used to generate proofs and these proofs are what is committed to the blockchain state. The Event logging system will be added to whenever a new event is processed (ie. creating a new client, or sending a ICS-20 token transfer), these can then be queried by height and topic by relayers to retrieve the actual data in question.

This PR should focus on creating an EventManager interface and struct to interact with different Event types. These should be implemented in a general fashion such as:

type Attribute struct {
    Key []byte
    Value []byte
}

type Event struct {
    Type string
    Height uint64
    Attributes []Attribute
}

The Event logging system should use an efficient data structure to underpin the Event storage to maximise IO ops and should also implement pruning to keep the number of events stored to the minimum number required.

Origin Document

ICS-24 defines the Event logging system and PR #795 mentions that the ICS-24 implementation is not complete. This PR should address this aspect of ICS-24

Reference ibc-go events and cosmos-sdk event manager

Goals

  • Create an EventManager interface and implementation
  • Create generic typed Events
  • Enable the interaction with the event manager from any part of the IBC module through the host
  • Enable the addition of events
  • Enable the efficient pruning of old events

Deliverable

  • Create the EventManager interface and its struct implementation
  • Create the Event interface and its implementation
  • Add methods to interact with the EventManager through the host like the StoreManager
  • Add an efficient underlying data structure to back the Event logging system
  • Enable pruning

Non-goals / Non-deliverables

  • Implement any other aspect of ICS-24 not related to the event logging system
  • Implement specific event types or event creators
  • Implement any relayer behaviours to use events

General issue deliverables

  • Update the appropriate CHANGELOG(s)
  • Update any relevant local/global README(s)
  • Update relevant source code tree explanations
  • Add or update any relevant or supporting mermaid diagrams

Testing Methodology

  • Task specific tests or benchmarks: make ...
  • New tests or benchmarks: make ...
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md
  • k8s LocalNet: verify a k8s LocalNet is still functioning correctly by following the instructions here

Creator: @h5law
Co-Owners: @h5law

@h5law h5law added the ibc IBC specific changes label Jun 14, 2023
@h5law h5law added this to the M7: Pocket NoS (North Star) milestone Jun 14, 2023
@h5law h5law self-assigned this Jun 14, 2023
@h5law h5law moved this to In Research in V1 Dashboard Jun 14, 2023
@Olshansk
Copy link
Member

I looked into the link of ICS-24 provided and just sharing additional info regarding the event logging system:

Screenshot 2023-06-14 at 1 34 41 PM

Wanted to call out that @adshmh is working on adding PersistenceLocalContext in #803 which will be useful for this work and other. cc @h5law @dylanlott

@h5law
Copy link
Contributor Author

h5law commented Jun 14, 2023

PersistenceLocalContext

I think if the PersistenceLocalContext remains specific to the storage or any particular things with exposed functions for them we miss the point of the Event Logging system. Generalising the local context to be the Event manager interface allowing us to append typed events could be useful for the relay storage.

However by generalising it this way we would likely need additional methods added to query the relays specifically (this is what we will need to do for the different event types anyway).

To give context the cosmos-sdk EventManger simply keeps an in memory list of the Events it stores. But I imagine there could be a more efficient way of doing this.

CC: @adshmh @Olshansk @dylanlott

@Olshansk
Copy link
Member

@h5law To confirm, you're saying that using PersistenceLocalContext would be overkill and you'll probably just go with a simple in-memory map sort of structure, correct?

@h5law
Copy link
Contributor Author

h5law commented Jun 15, 2023

To confirm, you're saying that using PersistenceLocalContext would be overkill and you'll probably just go with a simple in-memory map sort of structure, correct?

I am saying that there may be a better solution to the in memory map used by Cosmos depending on how we do pruning (ie how long should events be persisted for). But also the PersistenceLocalContext could be a great means of implementing the EventManager but we would ideally want the interface to be something minimal like:

type EventManager interface {
    StoreEvent(event Event)
    GetEvents(type Type, height uint64) Event
}

All I meant was that the current implementation with the relay specific methods could be removed and instead have a function like StoreRelayEvent() which creates a relay typed event and stores this in the event manager - as it is general purposed and typed no need to excluded non IBC events

@Olshansk
Copy link
Member

Appreciate the extra context. Here is my suggestion for an initial approach once you get to implementing git

  1. Add EventManager as you describe in shared/modules
  2. Have StoreEvent and GetEvents use in-memory dicts in implementation prop: the agreed upon repository structure #1 -> we can optimize it later
  3. Whether we choose to add GetEventManager to PersistenceLocalCOntext or keep it separate we can figure out depending on how it matures.

@h5law h5law moved this from In Research to Up Next in V1 Dashboard Jun 21, 2023
@h5law h5law mentioned this issue Jul 9, 2023
20 tasks
@h5law h5law moved this from Up Next to In Review in V1 Dashboard Jul 10, 2023
h5law added a commit that referenced this issue Jul 13, 2023
## Description

<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 13 Jul 23 12:29 UTC
This pull request introduces several changes across multiple files. Here
is a summary of the diff:

1. In the file `benchmark_state_test.go`, a slice of `uint8` is modified
to generate a random value using the `rand.Intn` function, preventing
duplicate keys.
2. In the file `persistence_module.go`, new methods `SetIBCEvent` and
`GetIBCEvents` are added to enhance the persistence module's
functionality for handling IBC events.
3. In a different file related to IBC functionality, imports are added,
and new methods `SetIBCEvent` and `GetIBCEvents` are added for storing
and retrieving IBC events at a specific height and topic.
4. In the file `persistence/types/ibc.go`, multiple changes include
adding constants, new functions for generating queries, renaming a
function, and modifying existing functions for handling IBC events and
entries.
5. In the file `bus.go`, a new function `GetEventLogger` is added to the
`bus` struct, returning a value of type `modules.EventLogger`.
6. Another file diff involves adding imports, initializing variables
related to the IBC submodule, and updating the `Create()` function in
the `ibc/host` package.
7. In `ibc_test.go`, there are modifications to test functions, adding
new test functions, and adding an `attribute` struct.
8. A new file `ibc_event_module.go` is added, defining a package,
imports, constants, types, and methods related to an event logging
system.
9. Changes in `bus_module.go` show the addition of a new method
`GetEventLogger` to the `Bus` interface.
10. In `persistence/debug.go`, functions `ClearAllIBCQuery`,
`ClearAllIBCStoreQuery`, and `ClearAllIBCEventsQuery` are added/removed,
and a modification is made to the `HandleDebugMessage` function
signature.
11. A new file `event_manager.go` is added, containing the definition of
the `EventManager` struct and implementing the `EventLogger` interface.
12. A change is made to add a new table creation statement for the
IBCEventLog table in an unspecified file.
13. Changes in the file `ics24.md` introduce a new section for an event
logging system in the IBC module, with an overview of its implementation
and usage of a persistence layer.
14. A new file `ibc_events.proto` is added, defining message types for
attributes and IBC events in the shared/core/types/proto directory.

These changes aim to introduce and enhance functionality related to IBC
events, event logging, and persistence in the project.
<!-- reviewpad:summarize:end -->

## Issue

Fixes #824 

## Type of change

Please mark the relevant option(s):

- [x] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Adds the `EventLogger` interface and submodule
- Adds methods to add events to the persistence DB
- Adds methods to query the persistence DB for events by height and
topic
- Adds `IBCEvent` protobuf type
- Registers `EventLogger` to the bus for retrieval

## Testing

- [x] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [x] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [x] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [x] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [x] I have updated the corresponding README(s); local and/or global
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@github-project-automation github-project-automation bot moved this from In Review to Done in V1 Dashboard Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ibc IBC specific changes
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants