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

bug(hubble): Fetch fname transfers when merge fails due to "username not registered" #2001

Open
sanjayprabhu opened this issue May 15, 2024 · 6 comments
Labels
help wanted Well specified and ready to be worked on s-triage Needs to be reviewed, designed and prioritized t-bug A fix for a bug with the current system

Comments

@sanjayprabhu
Copy link
Contributor

What is the bug?
Hubble only polls the fname server for new names every 5 seconds. This means, if a user registers an fid and an fid and submits UserDataAdd messages within 5 seconds, the username message will be rejected since the hub has not picked it up yet.

We should detect this failure, and check the transfer history for that fid and retry the message automatically. We should also put in some protections so this cannot be used to ddos the fname server (simple check like only x calls/min to the fname server should do, user registration is not high traffic)

How can it be reproduced? (optional)
Include steps, code samples, replits, screenshots and anything else that would be helpful to reproduce the problem.

Additional context (optional)
Add any other context about the problem here.

@sanjayprabhu sanjayprabhu added t-bug A fix for a bug with the current system help wanted Well specified and ready to be worked on labels May 15, 2024
@github-actions github-actions bot added the s-triage Needs to be reviewed, designed and prioritized label May 15, 2024
@manan19
Copy link
Contributor

manan19 commented May 22, 2024

@sanjayprabhu can we bump on the priority on this? We're receiving a lot of missing fname reports.

@henryphaver
Copy link

Good finding @sanjayprabhu – We are also getting multiple reports on this from both developers and users. – Because of this issue no username is shown, but instead !123456 which renders the UX useless in almost all cases. Hope this gets prioritized.

@0xAegir
Copy link

0xAegir commented May 22, 2024

+1 This is biting us as well.

@sanjayprabhu
Copy link
Contributor Author

sanjayprabhu commented May 22, 2024

Are these fnames eventually fixed or are they still missing? Could you provide some examples? Trying to determine if this is related to the issue above or a separate issue. The above issue happens only for new registrations and should fix itself within a few minutes when our backend retries.

@andrewhong5297
Copy link

+10 this is making the query experience on Dune a bit buggy, a few users have brought this up

@henryphaver
Copy link

henryphaver commented May 22, 2024

@sanjayprabhu Actually now 2/3 of the sample size fixed already by themselves. - Will check if the third one is pending refresh on test system. - If not then been stuck for longer. Will also get more samples. Will report back.

sanjayprabhu pushed a commit that referenced this issue May 31, 2024
## Motivation

Potential solution for
#2001. Adds a
mechanism to re-fetch fname transfers for a given name when merging a
`UserDataAdd` that fails due to missing proof.

## Change Summary

Refetches fname transfers when merging a `UserDataAdd` that errors out
due to a missing proof. Also moves some code around for testing
purposes.

## Merge Checklist

_Choose all relevant options below by adding an `x` now or at any time
before submitting for review_

- [x] PR title adheres to the [conventional
commits](https://www.conventionalcommits.org/en/v1.0.0/) standard
- [x] PR has a
[changeset](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#35-adding-changesets)
- [ ] PR has been tagged with a change label(s) (i.e. documentation,
feature, bugfix, or chore)
- [ ] PR includes
[documentation](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#32-writing-docs)
if necessary.
- [x] All [commits have been
signed](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#22-signing-commits)

## Additional Context

If this is a relatively large or complex change, provide more details
here that will help reviewers

<!-- start pr-codex -->

---

## PR-Codex overview
The focus of this PR is to enhance the `@farcaster/hubble` module by
improving transfer retry logic and adding a new
`FNameRegistryClientInterface` with related functionalities.

### Detailed summary
- Added retry logic for fetching transfers on failed merge
- Introduced `FNameRegistryClientInterface` with methods for managing
transfers
- Updated `MockFnameRegistryClient` with transfer handling functionality

> The following files were skipped due to too many changes:
`apps/hubble/src/storage/engine/index.test.ts`

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

<!-- end pr-codex -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Well specified and ready to be worked on s-triage Needs to be reviewed, designed and prioritized t-bug A fix for a bug with the current system
Projects
None yet
Development

No branches or pull requests

5 participants