RFC: Make ErrNotFound a struct and include IsNotFound helper

[kevina]

The IsNotFound helper allows the error to be wrapped by using pkg/errors.Cause(...).

Next Steps:

1. Have the blockstore use this error instead of its own internal error when a Cid is not found. This will simplfy the merkeldag code.
2. Fix go-ipfs code which includes (1) the simplification of the merkeldag code and (2) using IsNotFound instead of testing for the error directly.

DO NOT MERGE until steps 1 and 2 are done in case of problems.

Closes ipfs/kubo#4468
Also See: ipfs/kubo#4099

[whyrusleeping]

@kevina each package should be able to have its own NotFoundError. We should be checking them based on the "Assert errors for behaviour, not type" section of this: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully

[kevina]

@whyrusleeping what is the benefit of that? No other error gets this special treatment and it adds a lot of (in by view) unnecessary boilerplate code to the merkedag package (currently still is go-ipfs) that checks if it is IsNotFound from the blockstore and then converts it to its own.

The IsNotFound is basically asserting based on behavior.

[kevina]

For example instead of:

b, err := n.Blocks.GetBlock(ctx, c)
if err != nil {
	if err == bserv.ErrNotFound {
		return nil, ipld.ErrNotFound
	}
	return nil, fmt.Errorf("Failed to get block for %s: %v", c, err)
}

I think we should just use some think like this:

import "https://github.com/pkg/errors"
...
b, err := n.Blocks.GetBlock(ctx, c)
if err != nil {
	return nil, errors.Wrap(err, "merkledag: get failed")
}

[whyrusleeping]

@kevina aside from the fact that there is value in being able to distinguish where exactly there error is from. With this method, you get silent failures if packages change. (if package hashes change, the equality of the errors fails)

[kevina]

@whyrusleeping I am not following the second argument.

If you don't like where you think I am going with this then please outline what you would like to see.

I have given you what I think will improve at least the errors when a CID is not found in a by retuning the CID with the error and allowing it to be wrapped with additional information, including a backtrace with the use of pkg/errors. The additional information can be used to say determine if a node is offline or not and solve the concern in ipfs/kubo#4099.

I am fine with a blockstore having its own ErrNotFound assuming that it will also contain the CID. But if we do I do not see the point in the merkledag having its own ErrNotFound method as all ErrNotFound errors come from the blockstore.

I can also just leave the blockstore alone and only change the ErrNotFound error from the merkledag package (in go-ipfs), basically scratch (1) from the list and just do (2) and do something like:

// Get retrieves a node from the dagService, fetching the block in the BlockService
func (n *dagService) Get(ctx context.Context, c *cid.Cid) (ipld.Node, error) {
	if n == nil {
		return nil, fmt.Errorf("dagService is nil")
	}

	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	b, err := n.Blocks.GetBlock(ctx, c)
	if err != nil {
		if err == bserv.ErrNotFound {
			return nil, ipld.ErrNotFound{c}
		}
		return nil, fmt.Errorf("Failed to get block for %s: %v", c, err)
	}

	return ipld.Decode(b)
}