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

Create a seperate table for storing logs. #102

Merged
merged 2 commits into from
Aug 27, 2021
Merged

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented Aug 5, 2021

No description provided.

@arijitAD arijitAD marked this pull request as draft August 5, 2021 05:32
@arijitAD arijitAD changed the base branch from v1.10.6-statediff to v1.10.5-statediff August 5, 2021 05:32
@arijitAD arijitAD changed the base branch from v1.10.5-statediff to v1.10.5-statediff-0.0.25 August 5, 2021 05:33
@arijitAD arijitAD changed the base branch from v1.10.5-statediff-0.0.25 to v1.10.4-statediff August 5, 2021 05:33
@arijitAD arijitAD force-pushed the log-table branch 2 times, most recently from 4c43c21 to fdedb5f Compare August 9, 2021 11:53
@arijitAD arijitAD changed the base branch from v1.10.4-statediff to v1.10.6-statediff August 9, 2021 11:55
Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

Need to make some changes to the db schema, which will necessitate updating the go code some too, but otherwise everything looks great (and would work for the current schema).

core/types/receipt.go Outdated Show resolved Hide resolved
statediff/indexer/ipfs/ipld/eth_receipt.go Outdated Show resolved Hide resolved
statediff/indexer/ipfs/ipld/eth_receipt.go Show resolved Hide resolved
statediff/indexer/ipfs/ipld/eth_receipt.go Show resolved Hide resolved
statediff/indexer/ipfs/ipld/shared.go Outdated Show resolved Hide resolved
topic0s VARCHAR(66)[],
topic1s VARCHAR(66)[],
topic2s VARCHAR(66)[],
topic3s VARCHAR(66)[],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need another column for data?

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't see this til after commenting: #102 (comment)

If you think it is useful from a users/consumers perspective let's do it @ashwinphatak

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's useful if we implement the GetLogs GQL API using the log index table in cerc-io/ipld-eth-server#92, so we don't have to read it from elsewhere?

We haven't done that yet in the linked PR in ipld-eth-server. We should return the log CID and ipld block there, maybe also the receipt CID in a new field.

Copy link
Collaborator

@ashwinphatak ashwinphatak Aug 20, 2021

Choose a reason for hiding this comment

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

@i-norden While doing this, would it make sense to filter out "failed" logs (based on the receipt status) here: https://github.com/vulcanize/ipld-eth-server/blob/master/pkg/graphql/graphql.go#L1031 (or in the table join, as long as the log index turns out correct)

@arijitAD arijitAD marked this pull request as ready for review August 16, 2021 12:58
Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

LGTM, just one or two minor tweaks and then there are a few things still open to discussion. I'll bring these things up in slack and we can figure out what the best approach is.

core/types/receipt.go Show resolved Hide resolved
statediff/indexer/indexer.go Outdated Show resolved Hide resolved
statediff/indexer/ipfs/ipld/eth_receipt.go Show resolved Hide resolved
statediff/indexer/ipfs/ipld/eth_receipt.go Outdated Show resolved Hide resolved
statediff/indexer/writer.go Show resolved Hide resolved
@i-norden i-norden self-requested a review August 20, 2021 01:47
log_data BYTEA,
mh_key TEXT NOT NULL REFERENCES public.blocks (key) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
Copy link
Collaborator

@i-norden i-norden Aug 20, 2021

Choose a reason for hiding this comment

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

So we still want a mh_key and cid that reference an IPLD block in this table, but instead of referencing a Log IPLD it should reference a LogTrie leaf node IPLD (the leaf node that contains the log this table was previously directly referencing). Does that make sense? It will be a bit involved/tricky to generate this cid/mh_key, you'll probably need to iterate over the trie we generate in processLogs while noting the path of each leaf node (the path is the RLP encoding of the log's index in the Receipt.Logs list, so we can tell which log is in which leaf based on the leaf node's path and the known index of the log).

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

Thanks Arijit, this is looking good. A few comments, nothing major. But I'm curious if we have proper unit test coverage for the indexing of the logs table- tests that include the new mh_key and cid rows? I don't see it in this diff, might have missed it.

}
logMhKey[idx] = mhKey
logCID[idx] = id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to reallocate the log CIDs to this new logCID slice, can't we use the slice in args.logLeafNodeCIDs?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -1,6 +1,8 @@
-- +goose Up
CREATE TABLE eth.log_cids (
id SERIAL PRIMARY KEY,
cid TEXT NOT NULL,
Copy link
Collaborator

@i-norden i-norden Aug 24, 2021

Choose a reason for hiding this comment

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

I think we should rename this to leaf_cid, to make it clear our CID no longer references a log directly but rather the leaf node that contains it. What are your thoughts on that @ashwinphatak?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -1,6 +1,8 @@
-- +goose Up
CREATE TABLE eth.log_cids (
id SERIAL PRIMARY KEY,
cid TEXT NOT NULL,
mh_key TEXT NOT NULL REFERENCES public.blocks (key) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
Copy link
Collaborator

@i-norden i-norden Aug 24, 2021

Choose a reason for hiding this comment

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

And same here, we could rename this to leaf_mh_key to make an explicit distinction.

Copy link
Author

Choose a reason for hiding this comment

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

Done

mappedContracts[l.Address.String()] = true
logDataSet[idx] = &models.LogsModel{
ID: 0,
Address: l.Address.String(),
Index: int64(l.Index),
Data: l.Data,
CID: logCID[idx],
Copy link
Collaborator

@i-norden i-norden Aug 24, 2021

Choose a reason for hiding this comment

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

Related to #102 (comment), couldn't this be args.logLeafNodeCIDs[i][idx]?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@i-norden i-norden self-requested a review August 24, 2021 02:18
@arijitAD
Copy link
Author

Will merge this PR once we have a major release #107

@arijitAD arijitAD changed the base branch from v1.10.6-statediff to v1.10.8-statediff August 24, 2021 12:33
@arijitAD arijitAD force-pushed the log-table branch 2 times, most recently from 09e4dae to 921242a Compare August 26, 2021 16:32
@arijitAD arijitAD merged commit 639af8b into v1.10.8-statediff Aug 27, 2021
@github-cerc-io github-cerc-io deleted the log-table branch October 2, 2023 20:57
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.

3 participants