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.

Sign up for GitHub

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

Jump to bottom

Rework cache #2224

Merged
merged 13 commits into from
Dec 30, 2024
Merged

Rework cache #2224

merged 13 commits into from
Dec 30, 2024

Conversation

tudor-malene
Copy link
Collaborator

@tudor-malene tudor-malene commented Dec 23, 2024 •
edited
Loading

Why this change is needed

  • We were using ristretto v1. This PR migrates to v2 which has generics.
  • The receipts and batches cache should be FIFO
  • use the cache for "GetTransaction" RPC calls
  • better handle "not authorised" "getReceipt" and "getTx" calls

Under normal operation, this change should avoid most DB access when users poll for transactions they submitted recently

What changes were made as part of this PR

  • remove the unnecessary caching wrapper.
  • cache receipts and batches using a different library
  • remove the custom "cost" constants, which were useful when we were using a single cache for all types
  • when saving an executed batch, cache the receipts after the db transaction was committed
  • cache the knowledge that a receipt was not found

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

StefanIliev545
StefanIliev545 reviewed Dec 23, 2024
View reviewed changes
go/enclave/rpc/GetTransactionReceipt.go Outdated
@@ -114,6 +114,12 @@ func fetchFromCache(ctx context.Context, storage storage.Storage, cacheService *
}
}
r := marshalReceipt(rec.Receipt, logs, rec.From, rec.To)

// after the receipt was requested by a user remove it from the cache
err := cacheService.DelReceipt(ctx, txHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because it means that the user doesn't need it any more. Basically, polling will stop when a receipt is returned

StefanIliev545
StefanIliev545 approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@StefanIliev545 StefanIliev545 left a comment

Choose a reason for hiding this comment

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

LGTM, tho i don't understand where the issue with the perf is

@tudor-malene
Copy link
Collaborator Author

LGTM, tho i don't understand where the issue with the perf is

During the perf test, some receipts are not fetched from the cache, even though everything should be cached.

StefanIliev545
StefanIliev545 reviewed Dec 28, 2024
View reviewed changes
go/enclave/rpc/GetTransaction.go Outdated
if rec != nil {
rpc.logger.Debug("Cache hit for tx", log.TxKey, txHash)
// authorise - only the signer can request the transaction
if rec.From.Hex() != requester.Hex() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This hex comparison feels a bit weird

StefanIliev545
StefanIliev545 approved these changes Dec 28, 2024
View reviewed changes
Copy link
Contributor

@StefanIliev545 StefanIliev545 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 e0b7ed7 into main Dec 30, 2024
23 checks passed
@tudor-malene tudor-malene deleted the tudor/rework_cache branch December 30, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanIliev545 StefanIliev545 StefanIliev545 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tudor-malene @StefanIliev545 @zkokelj @BedrockSquirrel