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(storage): insure no-dupes on protocol race when pre-filling dupes #277

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

hannahhoward
Copy link
Collaborator

Goals

fix #276

Implementation

Basically, before we insert dups on dups=y, we need to make sure the CAR file we're verifying against has no duplicate puts due to protocol racing.

So, we simply don't add duplicate blocks to verification stream (that in turn inserts duplicates).

Honestly, this is all way too complicated. But it works.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Merging #277 (2b3656b) into main (4ed0ff7) will increase coverage by 1.88%.
The diff coverage is 93.61%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   71.18%   73.07%   +1.88%     
==========================================
  Files          69       70       +1     
  Lines        6043     6651     +608     
==========================================
+ Hits         4302     4860     +558     
- Misses       1496     1547      +51     
+ Partials      245      244       -1     
Impacted Files Coverage Δ
.../retriever/bitswaphelpers/preloadcachingstorage.go 80.00% <92.85%> (+2.82%) ⬆️
pkg/storage/duplicateaddercar.go 91.58% <100.00%> (+0.32%) ⬆️

... and 19 files with indirect coverage changes

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

I was trying to check this on bafybeiezxzqimpb3vdxljgd5pxlgc3kwlalbzm5am27d2ehjiguyclgf7e but it's not operating like it did yesterday, a bitswap peer has probably disappeared in the meantime so it just fails on bad CAR from eipfs

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

oh, wait, there's CI errors, malformed MIME header: missing colon: "0", are we hitting a problem with our chunked encoding error signal perhaps?

@hannahhoward hannahhoward force-pushed the fix/missing-block-multiple-protocols branch from 8c6cb9c to b171926 Compare June 1, 2023 01:36
@hannahhoward
Copy link
Collaborator Author

hannahhoward commented Jun 2, 2023

@rvagg ok, I think I've fixed, this, but I'll admit the fix was quite confusing, and I need your review.

Essentially, during an HTTP + bitswap traversal:

  1. I preload cid A in the preload caching storage module for Bitswap
  2. I load (normal) cid A in the preload caching storage module for Bitswap -- it blocks waiting for the preloader (since at this time, the block is simply not in preload or main storage)
  3. While it is preloading, cid A is written to the main store via HTTP
  4. When bitswap gets the block, the preloader writes it to the preload store. However, since it's already in the main store, it no ops meaning that it's not in the preload keys.
  5. Now, the main loader resumes and attempts to load the block from the preload store. However, the preload store has no fallback to the main store.
  6. It errors.

The solution is at step number 5, if it's not in the preload store, we try the main store as a backup.

@rvagg
Copy link
Member

rvagg commented Jun 2, 2023

I added another commit with more explanatory docs in the preloader to clear up confusion (hopefully) for how that tricky flow works in Loader(), which is the crux of the preloader. Up to you if you keep it, modify it, squash it etc. Your PR.

@hannahhoward hannahhoward merged commit d8193a2 into main Jun 2, 2023
@hannahhoward hannahhoward deleted the fix/missing-block-multiple-protocols branch June 2, 2023 15:59
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.

Bug: issue when using multiple protocols + dup = y
3 participants