Repository: ipfs/boxo

Issue:
blockstore: remove HashOnRead
Filling issue based on poc code in https://github.com/ipfs/go-ipfs-blockstore/pull/51:
 
> Instead, provide a simple `ValidatingBlockstore` wrapper. This:
> 
> 1. Isn't racy (fixes https://github.com/ipfs/go-ipfs-blockstore/issues/49).
> 2. Keeps the base blockstore simple.
> 

> :point_right:  Really, "HashOnRead" should never have been a part of the blockstore API in the
> first place.

Discussion:

Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c8a99668..958dc143 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -28,6 +28,7 @@ The following emojis are used to highlight certain changes:
     - `ipfs_http_gw_responses_total{code}`: Counter for all HTTP responses by status code
     - `ipfs_http_gw_retrieval_timeouts_total{code,truncated}`: Counter for retrieval timeout events with details on truncation
 - `namesys/IPNSPublisher`: option to `PublishOptions` that allows for setting a custom sequence number for the IPNS record with proper validation to prevent unintentional replay attacks. [#962](https://github.com/ipfs/boxo/pull/962)
+- `blockstore`: Added `ValidatingBlockstore` wrapper. This replaces the `HashOnRead` blockstore API.
 
 ### Changed
 - `bitswap/network`: The connection event manager now has a `SetListeners` method. Both `bsnet` and `httpnet` now have options to provide the `ConnectionEventManager` during `New(...)`. This allows sharing the connection event manager when using both. The connection manager SHOULD be shared when using both networks with the `network.Router` utility.
@@ -42,6 +43,9 @@ The following emojis are used to highlight certain changes:
 
 ### Removed
 
+- `blockstore`: Removed HashOnRead API. This is a potentially BREAKING CHANGE for any users of the HashOnRead API. Use the `ValidatingBlocksore` instead.
+  - The `HashOnRead` function was also removed from `Filestore`. To use this functionality, provide a `ValidatingBlockstore` when creating a new `Filestore`.
+
 ### Fixed
 
 ### Security
diff --git a/blockstore/blockstore.go b/blockstore/blockstore.go
index 8230b05e..a46750a2 100644
--- a/blockstore/blockstore.go
+++ b/blockstore/blockstore.go
@@ -61,10 +61,6 @@ type Blockstore interface {
 	// 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
-	// rehashed to make sure it matches its CID.
-	HashOnRead(enabled bool)
 }
 
 // Viewer can be implemented by blockstores that offer zero-copy access to
@@ -183,16 +179,11 @@ func NewBlockstoreNoPrefix(d ds.Batching) Blockstore {
 type blockstore struct {
 	datastore ds.Batching
 
-	rehash       atomic.Bool
 	writeThrough bool
 	noPrefix     bool
 	provider     routing.ContentProviding
 }
 
-func (bs *blockstore) HashOnRead(enabled bool) {
-	bs.rehash.Store(enabled)
-}
-
 func (bs *blockstore) Get(ctx context.Context, k cid.Cid) (blocks.Block, error) {
 	if !k.Defined() {
 		logger.Error("undefined cid in blockstore")
@@ -205,18 +196,6 @@ func (bs *blockstore) Get(ctx context.Context, k cid.Cid) (blocks.Block, error)
 	if err != nil {
 		return nil, err
 	}
-	if bs.rehash.Load() {
-		rbcid, err := k.Prefix().Sum(bdata)
-		if err != nil {
-			return nil, err
-		}
-
-		if !rbcid.Equals(k) {
-			return nil, ErrHashMismatch
-		}
-
-		return blocks.NewBlockWithCid(bdata, rbcid)
-	}
 	return blocks.NewBlockWithCid(bdata, k)
 }
 
diff --git a/blockstore/bloom_cache.go b/blockstore/bloom_cache.go
index a0a1b9ad..8561cfd0 100644
--- a/blockstore/bloom_cache.go
+++ b/blockstore/bloom_cache.go
@@ -215,10 +215,6 @@ func (b *bloomcache) PutMany(ctx context.Context, bs []blocks.Block) error {
 	return nil
 }
 
-func (b *bloomcache) HashOnRead(enabled bool) {
-	b.blockstore.HashOnRead(enabled)
-}
-
 func (b *bloomcache) AllKeysChan(ctx context.Context) (<-chan cid.Cid, error) {
 	return b.blockstore.AllKeysChan(ctx)
 }
diff --git a/blockstore/idstore.go b/blockstore/idstore.go
index 340b8b18..875d6732 100644
--- a/blockstore/idstore.go
+++ b/blockstore/idstore.go
@@ -109,10 +109,6 @@ func (b *idstore) PutMany(ctx context.Context, bs []blocks.Block) error {
 	return b.bs.PutMany(ctx, toPut)
 }
 
-func (b *idstore) HashOnRead(enabled bool) {
-	b.bs.HashOnRead(enabled)
-}
-
 func (b *idstore) AllKeysChan(ctx context.Context) (<-chan cid.Cid, error) {
 	return b.bs.AllKeysChan(ctx)
 }
diff --git a/blockstore/twoqueue_cache.go b/blockstore/twoqueue_cache.go
index 2b03cb97..e0a35d41 100644
--- a/blockstore/twoqueue_cache.go
+++ b/blockstore/twoqueue_cache.go
@@ -355,10 +355,6 @@ func (b *tqcache) PutMany(ctx context.Context, bs []blocks.Block) error {
 	return nil
 }
 
-func (b *tqcache) HashOnRead(enabled bool) {
-	b.blockstore.HashOnRead(enabled)
-}
-
 func (b *tqcache) cacheHave(key string, have bool) {
 	b.cache.Add(key, cacheHave(have))
 }
diff --git a/blockstore/validating_blockstore.go b/blockstore/validating_blockstore.go
new file mode 100644
index 00000000..cc013c9e
--- /dev/null
+++ b/blockstore/validating_blockstore.go
@@ -0,0 +1,28 @@
+package blockstore
+
+import (
+	"context"
+
+	blocks "github.com/ipfs/go-block-format"
+	cid "github.com/ipfs/go-cid"
+)
+
+// ValidatingBlockstore validates blocks on get.
+type ValidatingBlockstore struct {
+	Blockstore
+}
+
+func (bs *ValidatingBlockstore) Get(ctx context.Context, c cid.Cid) (blocks.Block, error) {
+	block, err := bs.Blockstore.Get(ctx, c)
+	if err != nil {
+		return nil, err
+	}
+	rbcid, err := c.Prefix().Sum(block.RawData())
+	if err != nil {
+		return nil, err
+	}
+	if !rbcid.Equals(c) {
+		return nil, ErrHashMismatch
+	}
+	return block, nil
+}
diff --git a/filestore/filestore.go b/filestore/filestore.go
index 90a61549..0d45ca5c 100644
--- a/filestore/filestore.go
+++ b/filestore/filestore.go
@@ -239,9 +239,4 @@ func (f *Filestore) PutMany(ctx context.Context, bs []blocks.Block) error {
 	return nil
 }
 
-// HashOnRead calls blockstore.HashOnRead.
-func (f *Filestore) HashOnRead(enabled bool) {
-	f.bs.HashOnRead(enabled)
-}
-
 var _ blockstore.Blockstore = (*Filestore)(nil)



Test patch:
diff --git a/blockstore/blockstore_test.go b/blockstore/blockstore_test.go
index 713009ef..c5ffee53 100644
--- a/blockstore/blockstore_test.go
+++ b/blockstore/blockstore_test.go
@@ -137,14 +137,14 @@ func TestPutUsesHas(t *testing.T) {
 	}
 }
 
-func TestHashOnRead(t *testing.T) {
+func TestValidatingBlockstore(t *testing.T) {
 	orginalDebug := u.Debug
 	defer (func() {
 		u.Debug = orginalDebug
 	})()
 	u.Debug = false
 
-	bs := NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
+	bs := ValidatingBlockstore{NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))}
 	bl := blocks.NewBlock([]byte("some data"))
 	blBad, err := blocks.NewBlockWithCid([]byte("some other data"), bl.Cid())
 	if err != nil {
@@ -153,7 +153,6 @@ func TestHashOnRead(t *testing.T) {
 	bl2 := blocks.NewBlock([]byte("some other data"))
 	bs.Put(bg, blBad)
 	bs.Put(bg, bl2)
-	bs.HashOnRead(true)
 
 	if _, err := bs.Get(bg, bl.Cid()); err != ErrHashMismatch {
 		t.Fatalf("expected '%v' got '%v'\n", ErrHashMismatch, err)