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

User/Track update ipfs bug fix #1029

Merged
merged 11 commits into from Nov 5, 2020
Merged

Conversation

SidSethi
Copy link
Contributor

@SidSethi SidSethi commented Nov 3, 2020

Trello Card Link

https://trello.com/c/wQ26N0RE

Description

Replace ipfs validation with disk validation in user/track update flows, add ipfs + disk validation during image upload

Problem: currently, anytime someone updates user or artist data, CN performs validation on associated images. the function that performed this validation, getFileUUIDForImageCID() would call ipfs.cat() and ipfs.ls() on the image dir CIDs with no timeouts or try-catch. this is bad for many reasons, and likely the primary cause for repeated /audius_users calls timing out at 10min

Solution: On a state update, we should do minimal validation, and move the heavy-weight validation to first time upload to guarantee correctness before returning to user and writing CID to chain. image upload now performs a full byte-level validation of the files written to disk. user updates now perform a much lighter disk/db check.

Services

  • Discovery Provider
  • Creator Node
  • Identity Service
  • Libs
  • Contracts
  • Service Commands
  • Mad Dog

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

Delete an option.

  • 🚨 Yes, this touches cnode track + user update (not upload) and image upload

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.

  1. all existing CN unit and integration tests
  2. maddog test
  3. e2e manual testing via dapp: signup, upload, sync, failover, user update, track update
  4. perf test: measure route duration for /audius_users and /tracks, pull ensurePrimaryMiddleware duration from logs and subtract. on staging i'm repeatedly observing ~0.25s which is fantastic

Please list the unit test(s) you added to verify your changes.

…ws, add ipfs + disk validation during image upload
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

Some questions but the one I have some follow up q's on is what do we do in validateStateForImageDirCIDAndReturnFileUUID in the case of a non dir image? it seems like that flow is broken currently as well cause that would error?

creator-node/src/resizeImage.js Outdated Show resolved Hide resolved
creator-node/src/routes/files.js Show resolved Hide resolved
creator-node/src/utils.js Show resolved Hide resolved
creator-node/src/utils.js Show resolved Hide resolved
@SidSethi
Copy link
Contributor Author

SidSethi commented Nov 4, 2020

Fix ipfs image bug

@SidSethi SidSethi marked this pull request as ready for review November 4, 2020 04:25
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

Let's go, this looks great

@SidSethi SidSethi merged commit fd6eceb into master Nov 5, 2020
@SidSethi SidSethi deleted the ss-fix-update-route-image-checks branch November 5, 2020 22:01
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.

None yet

3 participants