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

nodeapi: Store raw oasis-core in serialized form #382

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

mitjat
Copy link
Collaborator

@mitjat mitjat commented Apr 13, 2023

Background: The indexer-internal nodeapi.Event type that represents an oasis-core consensus event has a Body field. It stores the raw body of the Event as received from oasis-core, but intentionally erases the type (by using the interface{} type) so that the rest of the indexer doesn't need to support all the variants of Event types from all the oasis-core versions. We just dump the raw body into the DB as json, for anybody who might want to inspect it later.

Problem: #360 is introducing a caching layer, as part of which all oasis-core responses (= nodeapi types) are (de)serialized to/from the disk. The interface{} type does not round-trip losslessly and throws errors when deserialzing back from on-disk JSON or CBOR.

Solution / This PR: This PR changes the nodeapi representation of the event to store a serialized version (JSON) of the oasis-core Event, not just a type-erased version (interface{}). Instead of JSON, we could choose CBOR; CBOR makes more sense in that it's used everywhere internally. But since the goal of this field is to store it in the DB (which offers native JSON columns) and then present it via HTTP API (again JSON), I'm going with JSON.

Note on runtimes: The runtime part of nodeapi has no such issue because the event types have been stable across all versions of the sdk, so the nodeapi Event representation does not erase the types and uses the current oasis-sdk types. At some point when types change enough, we might well need to introduce a similar solution there.

Testing: Indexed 100 blocks of consensus and compared DB table contents. No diffs.

@@ -482,7 +482,7 @@ func (m *Main) queueTxEventInserts(batch *storage.QueryBatch, data *storage.Cons
eventData := m.extractEventData(event)
txAccounts = append(txAccounts, eventData.relatedAddresses...)
accounts := extractUniqueAddresses(eventData.relatedAddresses)
body, err := json.Marshal(eventData.body)
body, err := json.Marshal(eventData.rawBodyJSON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should either (a) change rawBodyJSON to []byte and remove json.Marshal or (b) rename it to rawBody. otherwise marshaling something that's already labeled as "JSON" seems kind of weird

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well json.RawMessage is defined as []byte, so we kinda already had (a).
I went with (b) now.

common/types.go Outdated
func TryAsJSON(v interface{}) json.RawMessage {
encoded, err := json.Marshal(v)
if err != nil {
return json.RawMessage("null")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this what would have happened in the db if pgx failed to marshal the event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe not exactly; in the old setup, if pgx had failed, we would have failed the whole block analysis.
However

  • failure to serialize is very unlikely (we're just re-serializing plain structs that were previously cbor-serialized by the node)
  • a missing value here is low impact (people just won't be able to see the raw struct of an event)
  • all the verbose error handling would make the rest of the code harder to read

So I went with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dang I wish we had globally accessible logging

@mitjat mitjat force-pushed the mitjat/raw-event-body-as-json branch from 2321773 to 4002e02 Compare April 17, 2023 09:18
@mitjat mitjat requested a review from pro-wh April 17, 2023 10:23
Copy link
Collaborator

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

ya

Copy link
Collaborator

@Andrew7234 Andrew7234 left a comment

Choose a reason for hiding this comment

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

thanks!

@pro-wh pro-wh mentioned this pull request Apr 26, 2023
@pro-wh
Copy link
Collaborator

pro-wh commented Apr 26, 2023

💀 #360 merged part of this. rebase the second commit

@mitjat mitjat force-pushed the mitjat/raw-event-body-as-json branch from 4002e02 to 20de4d8 Compare April 29, 2023 04:16
@mitjat mitjat enabled auto-merge April 29, 2023 04:17
@mitjat mitjat merged commit 35fd167 into main Apr 29, 2023
5 checks passed
@mitjat mitjat deleted the mitjat/raw-event-body-as-json branch April 29, 2023 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants