Repository: ipfs/boxo (language: go)

Issue:
blockstore: properly document the interface
It has docs, but they're pretty short. Some notable examples:

* Is a blockstore implementation meant to be safe for concurrent use? (I think so)
* Should a blockstore identify blocks by just a multihash? (I think we're converging on this, including AllKeysChan flattening to Codec==Raw) }  – see also https://github.com/ipfs/boxo/issues/361
* Should a blockstore deduplicate Puts? What about AllKeysChan?

A particular implementation can differ on any of these points, but only when clearly documented as such.

We've talked about this before, but it's not on any issue or PR, so I'm filing this so we don't forget.

cc @aschmahmann @Stebalien @willscott 


Discussion / hints:
Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within **two business days**. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  - "Priority" labels will show how urgent this is for the team.
  - "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  - "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

Something about expected behavior of readonly blockstores could be useful as well. (is the only way to try and fail to `put`?) (does the error on write imply the blockstore is invalid and needs to be re-opened?)
Independently of the design of a v2 blockstore interface, I think v1 could also start defining a ReadBlockstore with a subset of the methods, so that those only needing reads can start using the narrower interface.

Implementers like carv2 might never be able to only implement ReadBlockstore, because it might take months or years for the whole ecosystem of modules to shift to the interface subset, but at least we could make the docs clearer and the API contracts more precise.
And of course I forgot two important CCs: @ribasushi @masih 
If you want a real world use case: we use a read only (S3 based) blockstore in production in Peergos. The writes happen outside ipfs with our own blockstore implementation. 
Another piece of lacking docs: what guarantees does AllKeysChan give about the list of keys it provides?

* If a blockstore supports duplicate CIDs, should AllKeysChan also return duplicate CIDs?
* Should AllKeysChan return CIDs in a deterministic order? (probably yes, for everyone's sanity)
* Should AllKeysChan return CIDs in a specific order, such as "order of insertion" or "increasing order by CID"?
>  such as "order of insertion" 

It is incredibly difficult to preserve that in the case of large datasets: avoid.
My take is:

> AllKeysChan treats the underlying blockstore as a set, and returns that set in full. The only guarantee is that the consumer of AKC will encounter every CID in the underlying set, at least once. If the underlying blockstore supports duplicate CIDs it is up to the implementation to elect to return such duplicates or not. Similarly no guarantees are made regarding CID ordering.

Fix patch:
diff --git a/blockstore/blockstore.go b/blockstore/blockstore.go
index ba964c7ab..6a606f1e1 100644
--- a/blockstore/blockstore.go
+++ b/blockstore/blockstore.go
@@ -47,6 +47,16 @@ type Blockstore interface {
 	// AllKeysChan returns a channel from which
 	// the CIDs in the Blockstore can be read. It should respect
 	// the given context, closing the channel if it becomes Done.
+	//
+	// AllKeysChan treats the underlying blockstore as a set, and returns that
+	// set in full. The only guarantee is that the consumer of AKC will
+	// encounter every CID in the underlying set, at least once. If the
+	// underlying blockstore supports duplicate CIDs it is up to the
+	// implementation to elect to return such duplicates or not. Similarly no
+	// guarantees are made regarding CID ordering.
+	//
+	// When underlying blockstore is operating on Multihash and codec information
+	// is not preserved, returned CIDs will use Raw (0x55) codec.
 	AllKeysChan(ctx context.Context) (<-chan cid.Cid, error)
 
 	// HashOnRead specifies if every read block should be


Test patch: