We have:

Unlike:

This needs to be fixed in both v0 and v1 of the blockstore.

@hsanjuan just pushed a release tag: v1.2.0.
Please manually verify validity (using gorelease), and update version.json to reflect the manually released version, if necessary.
In the future, please use the automated process.

The "has" cache uses a ton of memory. Go reports ~40MiB but that likely doesn't include allocator overhead. There are two things we may be able to do to help with this:

1. Only record that we don't have something. If we do have something, we'll have to load it anyways so caching the fact that we have it doesn't help much.
2. Use a better ARC cache. While the ARC cache was using ~40MiB, we only had ~7.5MiB worth of CIDs. Given (1), we may be able to make this more efficient with an ARC set.

Ideally, we'd use the bloom filter. Even for a 1% failure rate, at 1M items an optimal bloom filter would only be about a megabyte (requiring 7 lookups per check). However, loading the bloom filter is a bit of a pain.

While working on some tests to detect race conditions with a blockstore wrapper I'm working on, I create multiple goroutines to execute the various blockstore functions. I noticed that when I had each goroutine call Blockstore::HashOnRead(num%2==0) where num is the goroutine number, that a race condition is triggered.

Subsequently I looked into the blockstore and found this:

type blockstore struct {
	datastore ds.Batching

	rehash bool
}

func (bs *blockstore) HashOnRead(enabled bool) {
	bs.rehash = enabled
}

This seems a little unsafe, since there's no documentation explicitly saying that this shouldn't be called by multiple goroutines. It should either be guarded by a mutex, or, alternatively use an atomic boolean

edit:

Doing some more digging, this race condition will not only effect HashOnRead, but also Get, GetSize

@hsanjuan just pushed a release tag: v0.2.0.
Please manually verify validity (using gorelease), and update version.json to reflect the manually released version, if necessary.
In the future, please use the automated process.

Moved from: ipfs/kubo#3208

--- FAIL: TestHasIsBloomCached (0.03s)
    bloom_cache_test.go:100: Bloom filter has cache miss rate of more than 5%
FAIL
FAIL    github.com/ipfs/go-ipfs/blocks/blockstore   0.053s

They point to non existent commits...

$ go get github.com/ipfs/go-ipfs-blockstore@master
go: github.com/ipfs/[email protected] requires
	github.com/ipfs/[email protected]: invalid version: unknown revision 28a3721c2e66
$ go get github.com/ipfs/[email protected]
go: github.com/ipfs/[email protected] requires
	github.com/ipfs/[email protected]: invalid version: unknown revision 28a3721c2e66

Steps:

1. Call Put & Delete on the same block at the same time.
2. After finishing step 1, call Put on the block.

After step 1, the block may or may not be in the blockstore depending on how the operations serialize. However, after step 2, the block must be in the datastore.

Unfortunately, there's a race:

1. Put 1: put the block.
2. Delete 1: Delete the block.
3. Delete 1: Cache that we don't have the block.
4. Put 1: Cache that we have the block.
5. Put 2: Check the cache, see that we should have the block, and walk away.

There's also a similar race between get & delete and get & put.

Fix: add striped (by CID) locking.
Impact:

1. Put & Delete: data loss (in go-ipfs, requires GC)
2. Get & Delete: data loss (in go-ipfs, requires GC)
3. Get & Put: Inability to retrieve a block from the datastore (until restarting).

@hsanjuan just pushed a release tag: v0.2.0.
Please manually verify validity (using gorelease), and update version.json to reflect the manually released version, if necessary.
In the future, please use the automated process.