soroban-rpc: Add in-memory events storage #355

tamirms · 2023-01-24T15:53:58Z

Add in-memory events db with a retention window specified in number of ledgers. For example, the event store can be configured to have a retention window of 17280 ledgers, which corresponds to approximately 24 hours assuming an average ledger close time of 5 seconds.

The event store is implemented using a circular buffer.

Close stellar/go#4718

cmd/soroban-rpc/internal/events/events.go

tsachiherman · 2023-01-24T21:16:07Z

cmd/soroban-rpc/internal/ledgerentry_storage/migrations/01_init.sql

-
-CREATE INDEX ledger_entries_key ON ledger_entries(key);
-
+CREATE TABLE ledger_close_meta (


I don't see why both the ledger entries and the events needs to live in the same database; they have quite a distinct an unrelated "characteristics" and not ever accessed together.
I'm concerned that placing both in the same database would result in a database-level locking between the two that isn't needed.

they are not accessed together but they are updated together using the same stream of ledgers coming from captive core. it's better if the two tables are in the same database because we can ensure both are updated within the same transaction. otherwise, if one table is behind the other it will complicate the ingestion code to ensure both are synced to the same ledger.

I do like the fact that we can commit the entire ledger changes in a single commit. In this particular case, I don't believe that it really give us much advantage. The events are only being "added" and not really requiring any synchronization with the rest of the data. You have already noticed that the current ledger data tend to become very large. That's because we have lots of write/delete. In order to avoid that, we need to perform periodic full vacuuming.
Unfortunately, performing the full vacuuming on a large database would take a long time - which is why it's advisable to break out the tables that have a different insert/delete profile into a separate database files.

you're right that the ledgers (from which we derive the events) don't need to be synchronized with the ledger entries for any of our sql queries. My point was that it would require more code to manage the ingestion.

If we cannot assume that the ledger entries and ledgers tables are not synchronized to the same ledger sequence then, we need to catchup which ever table is behind and then ingest into both tables ledger by ledger. I was thinking that at this stage of soroban-rpc we could avoid that complexity. would you be ok if I created an issue for this concern and we can decide to implement it at a later point?

That sounds like the right thing to do. It would allow us to release this as a working prototype while we improve on the implementation.

tsachiherman · 2023-01-24T21:17:35Z

cmd/soroban-rpc/internal/ledgerentry_storage/ledgerentry_storage.go


+		reader.Rewind()


seems wrong that we need to rewind and re-process. But maybe I'm not understanding the codeflow here correctly.

tsachiherman

I've made several comments. I'd be happy to review them together if any questions comes up !

cmd/soroban-rpc/internal/events/events.go

sreuland · 2023-01-24T21:59:22Z

cmd/soroban-rpc/internal/events/events.go

+				continue
+			}
+			for eventIndex, opEvent := range op.Events {
+				events = append(events, event{


is it worthwhile to offer an external config to be used here as event topic filters? as this is crunching events for the whole network, maybe rpc hosting use cases will only be interested in events for their contract or known topics, etc?

cmd/soroban-rpc/internal/events/events.go

2opremio · 2023-01-25T12:59:21Z

cmd/soroban-rpc/internal/ledgerentry_storage/ledgerentry_storage.go

+	events            *events.MemoryStore
+	retentionWindow   uint32


Adding events processing to ledgerentry_storage.go doesn't feel right, both because of the naming and because it complicates the logic.

If possible, I would separate the event ingestion from ledgerentry_storage.go. We can have a common daemon consuming CloseMeta entries, which are passed to ledgerentry_storage/ledgerentry_storage.go and, say events_storage/events_storage.go.

Something similar (but probably more lightweight) than the Horizon processors would do.

2opremio · 2023-01-25T13:06:06Z

cmd/soroban-rpc/internal/ledgerentry_storage/db.go

+	TrimLedgers(retentionWindow uint32) error
+	InsertLedger(ledger xdr.LedgerCloseMeta) error


I would move this into a separate interface since this is for Ledger entries

I have a similar request : can we avoid calling these "Ledgers" ?
i.e.

ApplyLedgerEventsRetentionWindow(retentionWindow uint32) error InsertLedgerEvents(ledger xdr.LedgerCloseMeta) error

@tsachiherman we are actually are storing the entire xdr.LedgerCloseMeta into the db. We don't store events in the db, we only keep events in memory. The reason for that is because I remember some discussion of eventually exposing an HTTP endpoint to serve ledgers in soroban-rpc and that this endpoint could be used by other services / clients instead of captive core.

2opremio · 2023-01-25T13:15:39Z

I think that mixing up the Ledger Entry and Events logic into the same files makes the code complicated. I would change the file hierarchy to decouple them and do the processing of each separately.

I would have a common module for the db stuff (like what you are suggesting in the but making sure we get the naming right, e.g. renaming LedgerEntry to something else when it's not used exclusively for ledger entries). It probably makes sense to separate this further into two files, one for the ledgerentry stuff and another for the meta stuff.
I would have two other modules for the events and ledger_entry stuff which can work as the Horizon processors (that requires some orchestrator for captive-core which passes the ledger information to each processor).

sreuland · 2023-01-25T19:45:04Z

cmd/soroban-rpc/internal/ledgerentry_storage/db.go

+}
+
+// GetLedger fetches a single ledger from the db.
+func (s *sqlDB) GetLedger(sequence uint32) (xdr.LedgerCloseMeta, bool, error) {


before sql gets more verbose, is it worth considering a type-safe compile time binding like go-jet.

Pull in helper functions from stellar/go#4750

paulbellamy · 2023-01-30T14:25:57Z

Is this closed in favor of #361, or still relevant?

tsachiherman · 2023-01-30T18:11:12Z

Is this closed in favor of #361, or still relevant?

yes; @tamirms created #361 in order to break this one into a smaller set of reviewable components.
So, he would need to rebase this one and keep working on completing this one.

tamirms · 2023-01-30T18:29:13Z

@paulbellamy @tsachiherman I will close this PR to minimize confusion and spin up new PRs for the remaining work

tsachiherman reviewed Jan 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Show resolved Hide resolved

tsachiherman reviewed Jan 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Outdated Show resolved Hide resolved

tsachiherman reviewed Jan 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Show resolved Hide resolved

tsachiherman reviewed Jan 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Outdated Show resolved Hide resolved

sreuland reviewed Jan 24, 2023

View reviewed changes

paulbellamy reviewed Jan 25, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Outdated Show resolved Hide resolved

2opremio reviewed Jan 25, 2023

View reviewed changes

cmd/soroban-rpc/internal/events/events.go Outdated Show resolved Hide resolved

2opremio reviewed Jan 25, 2023

View reviewed changes

sreuland reviewed Jan 25, 2023

View reviewed changes

tamirms mentioned this pull request Jan 26, 2023

ingest, xdr: Add helper functions for getting events and serializing ledgers stellar/go#4750

Merged

7 tasks

tamirms added 4 commits January 26, 2023 12:39

Add in-memory events storage

23544c1

improve Go doc strings

da39916

Run: go get github.com/stellar/go@soroban-xdr-next

c75cc30

Pull in helper functions from stellar/go#4750

Address feedback on events store and remove TODOs

37f3dbd

tamirms force-pushed the events-storage branch from 2413c3d to 37f3dbd Compare January 26, 2023 12:41

tamirms mentioned this pull request Jan 27, 2023

soroban-rpc: Add in-memory event store backed by circular buffer #361

Merged

tamirms closed this Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soroban-rpc: Add in-memory events storage #355

soroban-rpc: Add in-memory events storage #355

tamirms commented Jan 24, 2023 •

edited

Loading

tsachiherman Jan 24, 2023

tamirms Jan 25, 2023

tsachiherman Jan 26, 2023

tamirms Jan 26, 2023 •

edited

Loading

tsachiherman Jan 27, 2023

tsachiherman Jan 24, 2023

tsachiherman left a comment

sreuland Jan 24, 2023

2opremio Jan 25, 2023 •

edited

Loading

2opremio Jan 25, 2023

tsachiherman Jan 26, 2023

tamirms Jan 26, 2023

2opremio commented Jan 25, 2023 •

edited

Loading

sreuland Jan 25, 2023

paulbellamy commented Jan 30, 2023

tsachiherman commented Jan 30, 2023

tamirms commented Jan 30, 2023


		CREATE INDEX ledger_entries_key ON ledger_entries(key);

		CREATE TABLE ledger_close_meta (

		TrimLedgers(retentionWindow uint32) error
		InsertLedger(ledger xdr.LedgerCloseMeta) error

soroban-rpc: Add in-memory events storage #355

soroban-rpc: Add in-memory events storage #355

Conversation

tamirms commented Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirms Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio commented Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

paulbellamy commented Jan 30, 2023

tsachiherman commented Jan 30, 2023

tamirms commented Jan 30, 2023

tamirms commented Jan 24, 2023 •

edited

Loading

tamirms Jan 26, 2023 •

edited

Loading

2opremio Jan 25, 2023 •

edited

Loading

2opremio commented Jan 25, 2023 •

edited

Loading