ipfs block rm doesn't notify of a non-existing key

[schomatis]

This is the case even if turning off the force flag (which I think is off by default so I am definitely missing something here).

HASH=$(echo "Hello world!" | ipfs block put)
ipfs block rm $HASH
# removed QmP8Ag9AnxQuExj6TaZGbFuKTHYjKUuPhnnxmpURzFGoPD
ipfs block rm $HASH
# removed QmP8Ag9AnxQuExj6TaZGbFuKTHYjKUuPhnnxmpURzFGoPD
ipfs block rm --force=false --quiet=false $HASH
# removed QmP8Ag9AnxQuExj6TaZGbFuKTHYjKUuPhnnxmpURzFGoPD

[kevina]

@schomatis this could be related to the datastore. What datastore are you using?

[schomatis]

Thanks @kevina, this is indeed related to the datastore, in fact I was making some tests for Badger when I realize this command wasn't behaving as expected (the flatfs actually reports the missing block with blockstore: block not found). Why is that?

[kevina]

Probably because Badger doesn't return an error when you try to remove a non-existent key...

[schomatis]

Yes, a delete in Badger is just another entry marking the key as deleted (so future Get operations will know that the key doesn't exist), it doesn't matter if the key existed before or not. I'm thinking that this might break the API (of what the user might be expecting from a block rm operation). Ideally an explicit Has could be ordered before the Delete call but this will have a non-negligible performance impact (especially in a repo GC and especially in Badger where the get/read operations are more expensive than the put/write ones). We should at least document this.

[Stebalien]

Related to: ipfs/go-ds-flatfs#30

If we really want to avoid breaking the API, we can add a has check to the block rm command.

[schomatis]

If we really want to avoid breaking the API, we can add a has check to the block rm command.

Yes, as I mentioned before, I had some concerns about performance.

Ideally an explicit Has could be ordered before the Delete call but this will have a non-negligible performance impact (especially in a repo GC and especially in Badger where the get/read operations are more expensive than the put/write ones).

@Stebalien WDYT?

[Stebalien]

If I could start over, I would have made block rm idempotent from the beginning. However, given that we already have the force flag, I'd:

1. Make delete idempotent at the datastore level.
2. Have the block rm command (well, the RmBlocks utility function) check Has iff force is false.

This won't slow down GC as GC doesn't use RmBlocks. It technically changes the datastore interface however, I'm pretty sure this change is more likely to fix bugs than it is to cause them.

If a user really needs to rapidly delete blocks via the block rm command, they can specify --force.

[Stebalien]

Delete is now idempotent by default. We've fixed ipfs block rm by checking has first.

(slightly race but not the end of the world).