Push & Pull protocol unit test #24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
appcypher
reviewed
Jul 27, 2023
Co-authored-by: James Walker <[email protected]>
Also: - Put `CarFile` bytes into a newtype - Abstract out push&pull protocol parts into `common` - Abstract out test utilities
appcypher
reviewed
Aug 24, 2023
There was a problem hiding this comment.
First off, huge kudos to you for diving into the intricacies of the car-mirror protocol and coming up with a great unit test! 🚀 I can see the effort and attention to detail that went into addressing the push & pull mechanisms, especially considering the nuances of state management.
That said, I have some questions and some nitpicks.
|
Oh snap! Merged it by mistake. |
|
All good, wanted to merge it anyways |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements a unit test that pushes a random DAG from one
MemoryBlockStoreto another via CAR files.Closes #16
Closes #17
Closes #18
It doesn't really do #19, because in this version, CAR mirror is treated as mostly stateless.
The only state that needs to be kept is client-side, and it's the most recent response that it got from the server (so it knows what else remains to be sent or whether the protocol is done).
And (in this version) the server side is always completely stateless.
Should this become a performance issue, then we can easily add a bunch of memoization caches.
Some ideas for such caches:
Cid -> Vec<Cid>, instead of having to fetch the block everytime & recompute links.Cid -> Vec<Cid>, but this time with all transitive references. Storing the whole transitive closure is going to be faster, but also requires a quadratic amount of memory in the depth of the DAG, if done for every block.Cid -> Bloom. This one is neater, but also needs more thought, since it's likely the bloom will be the wrong size. Before we do this, we should probably do other versions first and see how memory-hungry they are.