Thiserror changeover

[yeastplume]

This is an update of the change to thiserror from the deprecated failure crate originally proposed by by @trevyn in #3633 (with many thanks for doing this).

As noted in that PR, we should be updating from the deprecated crate, and I'd like to make sure the error handling is updated and consistent between the node, wallet and gui code. Will be updating the wallet project similarly.

[phyro]

There are some occurrences in the lock files e.g. https://github.com/mimblewimble/grin/blob/master/core/fuzz/Cargo.lock#L312-L330 Should we do something about these?

[yeastplume]

There are some occurrences in the lock files e.g. https://github.com/mimblewimble/grin/blob/master/core/fuzz/Cargo.lock#L312-L330 Should we do something about these?

I've updated the fuzz tests (which I have never looked at) to latest versions of everything, this should remove those references, as well as clean up most of the dependencybot warnings we have against the code.

[phyro]

Successfully synced a node and tried a basic error. I noticed some RPC calls got a different return type e.g. get_header went from -> Result<BlockHeaderPrintable, ErrorKind>; to -> Result<BlockHeaderPrintable, Error>;. I tried a simple request for a header at height that didn't exist at the time on both master and this branch and they returned the same response

> curl -XPOST -s -w "%{http_code}" -ugrin:(cat ~/.grin/main/.foreign_api_secret) -d '{"jsonrpc": "2.0","method": "get_header","params": [1817338,null,null],"id": 1}' http://127.0.0.1:3413/v2/foreign
{
  "id": 1,
  "jsonrpc": "2.0",
  "result": {
    "Err": "NotFound"
  }
}200⏎ 

Do we care if all the error responses are exactly as they were before?

[yeastplume]

Successfully synced a node and tried a basic error. I noticed some RPC calls got a different return type e.g. get_header went from -> Result<BlockHeaderPrintable, ErrorKind>; to -> Result<BlockHeaderPrintable, Error>;. I tried a simple request for a header at height that didn't exist at the time on both master and this branch and they returned the same response

> curl -XPOST -s -w "%{http_code}" -ugrin:(cat ~/.grin/main/.foreign_api_secret) -d '{"jsonrpc": "2.0","method": "get_header","params": [1817338,null,null],"id": 1}' http://127.0.0.1:3413/v2/foreign
{
  "id": 1,
  "jsonrpc": "2.0",
  "result": {
    "Err": "NotFound"
  }
}200⏎ 

Do we care if all the error responses are exactly as they were before?

They can't be the same as before, because thiserror gets rid of ErrorKind. As you demonstrate there, the printed output is okay. Anyone linking directly may need to modify some error handling code, but it's straightforward (will include this in the release notes)

[yeastplume]

I left some minor comments/questions. Great job, I think this simplifies error handling quite a bit. It's a pretty big change and the error handling logic also touches consensus related errors. I'm not yet sure how well tested error cases are in our tests, but I do hope we have these (I've seen some changes here, but not that many iirc). Since we're not in a hurry to merge this (afaik?), I suggest someone else makes a pass as well when they find time.

Not in a hurry, but the budding GUI code is now dependent on these changes for error display, so would like to get this and the wallet version in soon.

[yeastplume]

Think I'm happy with the state of review now, so merging. We'll have a while to shake issues out on the master branch before release anyhow.