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

cache receipts #2103

Merged
merged 6 commits into from
Oct 22, 2024
Merged

cache receipts #2103

merged 6 commits into from
Oct 22, 2024

Conversation

tudor-malene
Copy link
Collaborator

@tudor-malene tudor-malene commented Oct 21, 2024

Why this change is needed

Retrieving tx receipts is expensive because the SQL query is complex.
The most common use case is that users will request receipts for recent transactions.

What changes were made as part of this PR

  • introduce a cache of the most recent transactions
  • implement the event visibility logic in-memory to filter out the logs from the cached receipts
  • unrelated - there was an issue in the caching service where the 'cost' of some cache entries was wrong
  • unrelated - remove unnecessary tx.content column from the receipt SQL query

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@tudor-malene tudor-malene changed the title WIP cache receipts cache receipts Oct 22, 2024
Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

Few comments but approach seems reasonable to me


logs := rec.Receipt.Logs
// filter out the logs that the sender can't read
// doesn't apply to value transfers (when to=nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Value transfers have a To address I think, it's only contract creation where To=nil isn't it? Is it ok that this returns error for value transfers?

// event visibility logic
canView := eventType.IsPublic() ||
(eventType.AutoPublic != nil && *eventType.AutoPublic) ||
(eventType.SenderCanView != nil && *eventType.SenderCanView) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the method signature it looks like this checks for a generic requester but actually you can only call this method when you've verified sender == requester. Worth clarifying somewhere with a comment or in the method name I think.

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 actually had a comment like that and removed it

}

func (cs *CacheService) ReadConvertedHeader(ctx context.Context, batchHash common.L2BatchHash, onCacheMiss func(any) (*types.Header, error)) (*types.Header, error) {
return getCachedValue(ctx, cs.convertedGethHeaderCache, cs.logger, batchHash, blockHeaderCost, onCacheMiss)
return getCachedValue(ctx, cs.convertedGethHeaderCache, cs.logger, batchHash, blockHeaderCost, onCacheMiss, false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth a comment to say it's not added to the cache in this case and why I think, because unusual behaviour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was a typo actually. should have been true

Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

lgtm

@tudor-malene tudor-malene merged commit 684a115 into main Oct 22, 2024
1 of 2 checks passed
@tudor-malene tudor-malene deleted the tudor/cache_tx_receipts branch October 22, 2024 09:24
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.

2 participants