Thiserror changeover #3728

yeastplume · 2022-07-04T12:16:58Z

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 · 2022-07-05T08:37:04Z

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 · 2022-07-05T09:24:59Z

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 · 2022-07-05T13:54:37Z

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 · 2022-07-06T09:48:47Z

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)

phyro

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.

phyro · 2022-07-11T18:16:01Z

api/src/handlers/chain_api.rs

-		Ok(())
+		w(&self.chain)?
+			.compact()
+			.map_err(|_| Error::Internal("chain error".to_owned()))


is this error mapping intended? The previous code uses the ? and doesn't map the error from what I can tell

all fine, but adding slightly modified version to PR (has to do with From Trait implementation of chain::Error to rest::Error vs missing trait implemention of Result((), Error)... not worth getting into)

phyro · 2022-07-11T18:43:34Z

api/src/rest.rs

-			self.private_key
-		)))?;
+		let keyfile = File::open(&self.private_key).map_err(|e| {
+			Error::Internal(format!("failed to open file {} {}", self.private_key, e))


I guess that's a more general question regarding the difference in how errors were done before and are done now. We now also print e to it. Could this produce some kind of information leaking to logs that we may not want? If the answer is "we don't know", that's ok because I understand we don't have people to ask right now.

It could.. answer is 'I don't know'. In this case it's a private key read from a file stored in a string, so I'm going to revert the code that logs it. (the code wouldn't have been able to read it anyhow given this was an error reading the underlying file, but seems obvious it's not something we need to log)

phyro · 2022-07-11T18:52:03Z

chain/src/pipe.rs

-	block
-		.validate(&prev.total_kernel_offset)
-		.map_err(ErrorKind::InvalidBlockProof)?;
+	block.validate(&prev.total_kernel_offset)?;


did we forget to map error to InvalidBlockProof like it was before?

no, it's just implicitly done here now https://github.com/yeastplume/grin/blob/ba7eb9739987fb2687c8b72c3049de0eee3ab183/chain/src/error.rs#L54

phyro · 2022-07-11T19:06:31Z

core/fuzz/fuzz_targets/block_read_v2.rs


 fuzz_target!(|data: &[u8]| {
 	let mut d = data.clone();
-	let _t: Result<UntrustedBlock, ser::Error> = ser::deserialize(&mut d, ser::ProtocolVersion(2));
+	let _t: Result<UntrustedBlock, ser::Error> =
+		ser::deserialize(&mut d, ser::ProtocolVersion(2), DeserializationMode::Full);


ah, we missed these in one of the previous refactorings didn't we? Good this was found and fixed 👍
Do you know if we run any fuzz testing when making a release or on CI?

No, fuzz testing is very manual and I don't think anyone's looked at it in years. I updated all of it because they're small and only took a few minutes to update, so someone can have a look later if they like. If it had been a non-trivial task I would have just removed or disabled them.

yeastplume · 2022-07-12T10:20:28Z

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 · 2022-07-14T10:07:50Z

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.

trevyn and others added 6 commits April 10, 2021 10:44

WIP remove failure from all Cargo.toml

b9f69f7

WIP remove extern crate failure_derive

de44674

Use thiserror to fix all errors

77035bc

StoreErr is still a tuple

9487ecb

Remove another set of unnecessary .into()s

531f802

update and merge of thiserror swapover into master

a0a97b0

yeastplume requested review from phyro and quentinlesceller July 4, 2022 12:16

update fuzz tests

d2b21e4

update pool/fuzz dependencies in cargo.lock

ba7eb97

yeastplume mentioned this pull request Jul 6, 2022

Replace failure with thiserror mimblewimble/grin-wallet#654

Merged

phyro reviewed Jul 11, 2022

View reviewed changes

small changes based on feedback

ff8c744

yeastplume merged commit a14a8e3 into mimblewimble:master Jul 14, 2022

bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 21, 2024

grin v5.3 (0107) Thiserror changeover (mimblewimble#3728)

ad292d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thiserror changeover #3728

Thiserror changeover #3728

yeastplume commented Jul 4, 2022

phyro commented Jul 5, 2022 •

edited

Loading

yeastplume commented Jul 5, 2022

phyro commented Jul 5, 2022

yeastplume commented Jul 6, 2022

phyro left a comment

phyro Jul 11, 2022

yeastplume Jul 12, 2022

phyro Jul 11, 2022

yeastplume Jul 12, 2022

phyro Jul 11, 2022

yeastplume Jul 12, 2022

phyro Jul 11, 2022

yeastplume Jul 12, 2022

yeastplume commented Jul 12, 2022

yeastplume commented Jul 14, 2022

Thiserror changeover #3728

Thiserror changeover #3728

Conversation

yeastplume commented Jul 4, 2022

phyro commented Jul 5, 2022 • edited Loading

yeastplume commented Jul 5, 2022

phyro commented Jul 5, 2022

yeastplume commented Jul 6, 2022

phyro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeastplume commented Jul 12, 2022

yeastplume commented Jul 14, 2022

phyro commented Jul 5, 2022 •

edited

Loading