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

fix: Run validateOrRevoke only if signer is changed #1695

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

adityapk00
Copy link
Contributor

@adityapk00 adityapk00 commented Feb 12, 2024

Motivation

The validateOrRevokeMessages job takes ~12 hours to run everyday. It is very inefficient because it checks every single message.

Change Summary

  • Check messages only if the signer has changed for an FID
  • Check the username proof messages irrespective of signers

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review


PR-Codex overview

This PR focuses on adding the validateOrRevokeState field to the HubState message and implementing the ValidateOrRevokeMessagesJobScheduler class.

Detailed summary

  • Added validateOrRevokeState field to HubState message in hub_state.proto and hub_state.ts
  • Implemented ValidateOrRevokeJobState message in hub_state.ts
  • Added createBaseValidateOrRevokeJobState function in hub_state.ts
  • Implemented encoding and decoding functions for ValidateOrRevokeJobState in hub_state.ts
  • Added doJobForFid and doJobs methods to ValidateOrRevokeMessagesJobScheduler in validateOrRevokeMessagesJob.ts
  • Added test cases in validateOrRevokeMessagesJob.test.ts

The following files were skipped due to too many changes: apps/hubble/src/storage/jobs/validateOrRevokeMessagesJob.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@adityapk00 adityapk00 added the t-perf Improve performance label Feb 12, 2024
Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: d5ed452

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@farcaster/core Patch
@farcaster/hubble Patch
@farcaster/hub-nodejs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hub-monorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 13, 2024 2:49pm

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (330ce83) 73.98% compared to head (d5ed452) 74.57%.
Report is 2 commits behind head on main.

Files Patch % Lines
...le/src/storage/jobs/validateOrRevokeMessagesJob.ts 79.72% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
+ Coverage   73.98%   74.57%   +0.59%     
==========================================
  Files          99       99              
  Lines        9263     9309      +46     
  Branches     2082     2090       +8     
==========================================
+ Hits         6853     6942      +89     
+ Misses       2289     2254      -35     
+ Partials      121      113       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

const lastJobTimestamp = hubState.validateOrRevokeState?.lastJobTimestamp ?? 0;
const lastFid = hubState.validateOrRevokeState?.lastFid ?? 0;

log.info({ lastJobTimestamp, lastFid, numFids }, "ValidateOrRevokeMessagesJob: starting");

for (let i = 0; i < numFids; i++) {
const fid = allFids[i] as number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to read all fids into memory?

}
const numChecked = await this.doJobForFid(lastJobTimestamp, fid);
// We'll alwats check for username proof messages
const numUsernamesChecked = await this.doUsernamesJobForFid(fid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Most users won't have this. Checking the storage cache message count for the UserNameProof messages will save db reads here.


do {
// First, find if any signers have changed since the last time the job ran
const signers = await this._engine.getOnChainSignersByFid(fid);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, too bad the on chain event has fid before the timestamp, otherwise we could scan all onchain events in one go after a particular block number.

But maybe it's still cheaper to fetch the onchain events by block number than iterate through all fids/onchain events?

Anyway, not a huge deal. This is still a significant improvment from what it used to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this in a new PR. But yeah, this new way is like much faster. It now takes only an hour or so (depending on signers changed) vs ~12 hours before.

@adityapk00 adityapk00 merged commit bf37ec7 into farcasterxyz:main Feb 13, 2024
10 checks passed
@adityapk00 adityapk00 deleted the validatejob branch February 13, 2024 15:27
dtbuchholz pushed a commit to dtbuchholz/hub-monorepo that referenced this pull request Feb 13, 2024
* fix: Run validateOrRevoke only if signer is changed

* cleanup

* timebug

* review
dtbuchholz pushed a commit to dtbuchholz/hub-monorepo that referenced this pull request Feb 13, 2024
* fix: Run validateOrRevoke only if signer is changed

* cleanup

* timebug

* review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-perf Improve performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants