pokt-network / pocket

Official implementation of the Pocket Network Protocol v1
https://pokt.network
MIT License
61 stars 33 forks source link

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

Closed h5law closed 11 months ago

h5law commented 1 year ago

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

Deliverable

Non-goals / Non-deliverables

General issue deliverables

Testing Methodology


Creator: @h5law Co-Owners: @h5law

Olshansk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 #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.