Tracking: Improving ErrNotFound

[hsanjuan]

@Stebalien this is meant to hold a small async discussion about continuing ErrNotFound approach, for lack of a better place (would affect multiple repos).

We have done: ipfs/go-ipld-format#55

And now, roughly:

go-datastore has an ErrNotFound.

go-ipfs-blockstore sees those and turns them into it's own ErrNotFounds

go-blockservice sees those and turns them into it's own ErrNotFounds

go-merkledag sees those and turns them into the ipld.ErrNotFound that we fixed.

Two possible approaches:

• A: Remove package-specific ErrNotFound for blockstore, blockservice and merkledag and use and bubble ipld.ErrNotFound{CID} from the blockstore. This reduces lines of code. At the top level (go-ipfs), check with errors.Is(error, ipld.ErrNotFound{}), where needed.

• B:

  • Modify all ErrNotFound types in blockstore, blockservice, merkledag (and potentially datastore), to be able to include CID information and NotFound() method
  • Bubble errors directly without re-converting them on every module.
  • At the top-level, where it matters to verify something is a NotFound, check with errors.As() to see if what we got implements the NotFound() interface.
  • This is more righteous at the expense of some more, duplicated code.

For practical reasons, I lean towards A, given that Blockstore, blockservice, merkledag, ipld-format are almost like internal modules in the go-ipfs codebase, and then it would not be too crazy to consolidate on a single error type for this.

(of course I can bypass all this and just make go-merkledag work with the new ipld.ErrNotFound, but I thought I'd ask as this would all be quick to fix in one go, either way).

Thoughts?

[bonedaddy]

A sounds like best approach. I was updating some dependencies and found that the IPLD format update broke merkledag so I tried implementing the patch in a fork to push upstream and it's not as intuitive as before when it comes to handling the error.

For example https://github.com/RTradeLtd/go-merkledag/blob/fix%2Fipldformat/dagutils/utils.go#L100 this doesn't take a cid.Cid type so you have to do cid.Decode to get the CID to use in error checking, but the decode process also returns an error.

Bubbling this up, and not needing to do cid type conversion in top level could would be extremely helpful

[hsanjuan]

Bubbling this up, and not needing to do cid type conversion in top level could would be extremely helpful

you can leave the cid empty too...

As a side note, don't be overly aggressive updating dependencies (patch version bumps are fine), but other than that I'd keep go-ipfs's go.mod as reference.

[bonedaddy]

you can leave the cid empty too...

Didnt know that

As a side note, don't be overly aggressive updating dependencies (patch version bumps are fine), but other than that I'd keep go-ipfs's go.mod as reference.

Normally I dont but dependabot picked this the ipld format change in a PR and it failed so I was curious what it would take to get resolved.

[hsanjuan]

Discussed with @Stebalien , we're taking approach A. We will merge go-ipfs-block-format with go-ipld-format in the future (keeping aliases for backwards compat), for added consistency.

[hsanjuan]

Ok, I am going to hijack this issue to track this effort.

• This can only happen post 0.5.0, when we move to raw-multihashes because we have already merged/updated go-blockstore for that and I don't want to undo that just for this.
• We have reverted changes to go-ipld-format as they were breaking and blocking, so that revert needs to be reverted and tagged.
• Use ipld.ErrNotFound go-ipfs-blockstore#48 needs to be merged to start using ipld.ErrNotFound{cid} + tag.
• Use ipld.ErrNotFound go-bitswap#339 needs merging to use the new blockstore + tag
• Use ipld.ErrNotFound instead of ErrNotFound go-blockservice#55 needs merging with the new blockstore and bitswap + tag
• Improve NotFound error description go-ipfs-exchange-offline#27 needs merging to improve the offline error message with the new ipld error + tag
• Improve ErrNotFound go-merkledag#54 needs merging with the new blockservice, offline exchange, blockstore + tag
• Use ipld.ErrNotFound go-filestore#31 needs merging with the new merkledag and ipld errors
• Update tests to use ipld.IsNotFound to check for notfound errors interface-go-ipfs-core#82 needs merging with new merkledag and ipld errors plus fixing tests.
• go-ipfs needs to bump all those dependencies and adapt to the new ipld.ErrNotFound

[hsanjuan]

@Stebalien I request inclusion in 0.6.0 milestone.

[hsanjuan]

Behold, for we have finally moved away from raw multihashes and this has been unblocked!

[Stebalien]

🐢 🏁

[BigLep]

@hsanjuan @Jorropo : I assume we're going to need to do a bunch of releases on so this can bubble up to go-ipfs.

I suggest we use the versioning suggestions/tooling here: https://github.com/protocol/.github/blob/master/VERSIONING.md

If help is needed with the versioning workflow, please discuss in IPFS Discord / #ip-productivity.

[hsanjuan]

A small follow up for the tagging was on #8757 . This is now finally closed.