-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: add unified recon block store implementation #245
Conversation
Add a unified store that can both store Recon values and expose them as IPFS blocks. This change also makes all the IPFS read only. Its not longer possible to store arbitrary blocks but instead all data must ingress via Recon. Note the Interests store remains unmodified.
.await | ||
.map_err(Error::Internal)? | ||
.ok_or(Error::NotFound)?) | ||
if self.store.has(&cid).await.map_err(Error::Internal)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter if we call has
-> get_size
instead of just calling get_size
and handle the error if the CID is absent? Same question for block_get
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can't tell the difference between an error reading from the database and an error that the block does not exist. We could update the iroh_bitswap Store trait to either return a Result<Option<>> but I didn't make that change with this PR.
store/src/tests.rs
Outdated
}); | ||
let mut root_bytes = Vec::new(); | ||
root.encode(DagCborCodec, &mut root_bytes).unwrap(); | ||
let root_cid = Cid::new_v1(0x71, MultihashDigest::digest(&Code::Sha2_256, &root_bytes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use IpldCodec::DagCbor.into()
in place of 0x71
.
Really like the new code organization! 🥳 |
I have run the perf tests locally and compared with main. There was no significant difference in performance between main and this change. This PR is now ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 🚀
(I reviewed most of it a couple of days ago)
Add a unified store that can both store Recon values and expose them as IPFS blocks.
This change also makes all the IPFS read only. Its not longer possible to store arbitrary blocks but instead all data must ingress via Recon.
Note the Interests store remains unmodified.
I have not performance tested this change yet.