Use flex-error for tendermint-rs errors #923

soareschen · 2021-07-01T09:35:39Z

Closes #669

This is the second part for informalsystems/hermes#988

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

codecov-commenter · 2021-07-14T14:59:08Z

Codecov Report

Merging #923 (afd7185) into master (e824bfc) will increase coverage by 1.2%.
The diff coverage is 54.9%.

@@           Coverage Diff            @@
##           master    #923     +/-   ##
========================================
+ Coverage    70.6%   71.9%   +1.2%     
========================================
  Files         205     202      -3     
  Lines       16439   16183    -256     
========================================
+ Hits        11620   11643     +23     
+ Misses       4819    4540    -279

Impacted Files	Coverage Δ
abci/src/lib.rs	`100.0% <ø> (ø)`
light-client/examples/light_client.rs	`0.0% <0.0%> (ø)`
light-client/src/builder/error.rs	`0.0% <0.0%> (ø)`
light-client/src/builder/light_client.rs	`0.0% <0.0%> (ø)`
light-client/src/builder/supervisor.rs	`0.0% <0.0%> (ø)`
light-client/src/components/io.rs	`0.0% <0.0%> (-1.4%)`	⬇️
light-client/src/evidence.rs	`50.0% <ø> (ø)`
light-client/src/fork_detector.rs	`78.6% <0.0%> (-1.4%)`	⬇️
light-client/src/lib.rs	`100.0% <ø> (ø)`
light-client/src/utils/block_on.rs	`0.0% <0.0%> (ø)`
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e824bfc...afd7185. Read the comment docs.

thanethomson · 2021-07-21T19:19:46Z

This all looks pretty great so far! Is it ready for review yet?

soareschen · 2021-07-21T20:34:13Z

It should be reviewable soon. Currently I am refactoring the coding convention from error::constructor_error() to Error::constructor() in informalsystems/hermes#988. I will also do the same here and also update the documentation of flex-error to reflect the changes.

Signed-off-by: Thane Thomson <[email protected]>

* Implement full-duplex secret connection (#938) * Implement thread-safe cloning of a secret connection Signed-off-by: Thane Thomson <[email protected]> * Expand documentation for SecretConnection on threading considerations Signed-off-by: Thane Thomson <[email protected]> * Extract peer construction into its own method Signed-off-by: Thane Thomson <[email protected]> * Add test for cloned SecretConnection This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <[email protected]> * Add more messages to test Signed-off-by: Thane Thomson <[email protected]> * Expand comment for clarity Signed-off-by: Thane Thomson <[email protected]> * Add .changelog entry Signed-off-by: Thane Thomson <[email protected]> * Restore half-duplex operations Signed-off-by: Thane Thomson <[email protected]> * Extract encrypt/decrypt fns as independent methods Signed-off-by: Thane Thomson <[email protected]> * Remove unnecessary trait bounds Signed-off-by: Thane Thomson <[email protected]> * Extract send/receive state Signed-off-by: Thane Thomson <[email protected]> * Extract read/write functionality as standalone methods Signed-off-by: Thane Thomson <[email protected]> * Add logic to facilitate splitting SecretConnection into its sending and receiving halves Signed-off-by: Thane Thomson <[email protected]> * Restore split SecretConnection test using new semantics Signed-off-by: Thane Thomson <[email protected]> * Update changelog entry Signed-off-by: Thane Thomson <[email protected]> * Update docs for `SecretConnection` Signed-off-by: Thane Thomson <[email protected]> * Condense error reporting Signed-off-by: Thane Thomson <[email protected]> * Extract TryClone trait into its own crate As per the discussion at #938 (comment), this extracts the `TryClone` trait into a new crate called `tendermint-std-ext` in the `std-ext` directory. This new crate is intended to contain any code that we need that extends the Rust standard library. Signed-off-by: Thane Thomson <[email protected]> * Reorder imports Signed-off-by: Thane Thomson <[email protected]> * Assert validation regardless of debug build This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <[email protected]> * Remove remote_pubkey optionality from sender/receiver halves Signed-off-by: Thane Thomson <[email protected]> * Update SecretConnection docs with comment content Signed-off-by: Thane Thomson <[email protected]> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <[email protected]> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <[email protected]> * Add docs on SecretConnection failures and connection integrity Signed-off-by: Thane Thomson <[email protected]> * Synchronize sending/receiving failures to comply with crypto algorithm constraints Signed-off-by: Thane Thomson <[email protected]> * Rename try_split method to split for SecretConnection Signed-off-by: Thane Thomson <[email protected]> * Remove redundant field name prefixes Signed-off-by: Thane Thomson <[email protected]> * Fix broken link in docs Signed-off-by: Thane Thomson <[email protected]> * Fix recent clippy errors on `master` (#941) * Fix needless borrows in codebase Signed-off-by: Thane Thomson <[email protected]> * Ignore needless collect warning (we do actually seem to need it) Signed-off-by: Thane Thomson <[email protected]> * Remove trailing semicolon in macro to fix docs compiling Signed-off-by: Thane Thomson <[email protected]>

Signed-off-by: Thane Thomson <[email protected]>

thanethomson

Thanks for all this work @soareschen! It's really excellent.

I fixed a couple of minor things (e.g. some error messages during Light Client testing) and I've fixed the merge conflicts with master, so I'm going to go ahead and merge this now. Everything compiles, clippy's happy and all tests pass.

If we encounter any issues we can always fix them in follow-up PRs.

thanethomson · 2021-08-07T20:05:38Z

p2p/src/error.rs

+impl From<std::io::Error> for Error {
+    fn from(e: std::io::Error) -> Self {
+        Self::io(e)
+    }
 }


This is an example of what I'm talking about in #946.

thanethomson · 2021-08-07T20:32:05Z

rpc/src/error.rs

+    }
+}
+
+impl Clone for Error {


Is this not perhaps something that flex-error should do automatically in the define_error macro?

We can't use #[derive(Clone)] here because eyre::Report doesn't implement Clone. This is a general problem for deriving on the main error type, because error tracers like eyre::Report do not implement many common traits, not even std::error::Error. So I made flex-error leave that to the user to decide how to deal with the tracer part.

thanethomson · 2021-08-07T20:36:21Z

rpc/src/response_error.rs

+use std::fmt::Display;
+
+#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
+pub struct ResponseError {


Nice refactor! Thanks 🙏

soareschen added 17 commits June 30, 2021 11:11

Use flex-error for tendermint

8bd987c

Use flex-error for p2p

0df6c59

Use flex-error for light-client

f9edb66

Use flex-error for light_client::predicates

d042c04

Fix lint

c21edce

Use flex-error for builder and io errors

90a26cd

Use flex-error for rpc errors

c4a41df

Use flex-error for protobuf errors

668beb3

Use flex-error for abci

98a161a

Fix test_bisection_no_witness_left

266e0cc

Fix build errors in all-features

a9bb713

Fix failing tests

45f95ad

Fix more failures

f79236f

Merge branch 'master' into soares/flex-error

cdee9e1

Fix tungstenite error under wasm target

67e275b

Merge remote-tracking branch 'origin/master' into soares/flex-error

5682f66

Fix incoming_fixtures test

5763a28

soareschen added 2 commits July 20, 2021 09:30

Merge branch 'master' into soares/flex-error

08115e0

Fix conflict

17506fa

soareschen added 6 commits July 23, 2021 15:48

Update flex-error to v0.4.0

515f31e

Merge remote-tracking branch 'origin/master' into soares/flex-error

2095a70

set std feature in flex-error instead of individual crates

bef6287

Add flex-error patch to tools/Cargo.toml

da2ed80

Use published version of flex-error v0.4.1

bb30f30

Enable flex-error/eyre_tracer feature by default

90a0df2

soareschen marked this pull request as ready for review July 27, 2021 08:19

thanethomson mentioned this pull request Aug 3, 2021

Add changelog entry for flex-error integration work #940

Merged

5 tasks

Add .changelog entry (#940)

2ea6da4

Signed-off-by: Thane Thomson <[email protected]>

thanethomson mentioned this pull request Aug 7, 2021

flex-error: resolve conflicts with master #945

Merged

5 tasks

thanethomson and others added 2 commits August 7, 2021 15:13

Fix master merge

c699ba1

Signed-off-by: Thane Thomson <[email protected]>

thanethomson mentioned this pull request Aug 7, 2021

Follow-up on flex-error integration #946

Open

thanethomson added 2 commits August 7, 2021 15:44

Remove unnecessary macros

ab1aae4

Signed-off-by: Thane Thomson <[email protected]>

Correct error messages

afd7185

Signed-off-by: Thane Thomson <[email protected]>

thanethomson approved these changes Aug 7, 2021

View reviewed changes

thanethomson merged commit 877e408 into master Aug 7, 2021

thanethomson deleted the soares/flex-error branch August 7, 2021 21:11

thanethomson added this to the Q3 2021 milestone Aug 7, 2021

hdevalence mentioned this pull request Sep 9, 2021

abci: Add domain types #862

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use flex-error for tendermint-rs errors #923

Use flex-error for tendermint-rs errors #923

soareschen commented Jul 1, 2021 •

edited by thanethomson

Loading

codecov-commenter commented Jul 14, 2021 •

edited

Loading

thanethomson commented Jul 21, 2021

soareschen commented Jul 21, 2021

thanethomson left a comment

thanethomson Aug 7, 2021

thanethomson Aug 7, 2021

soareschen Aug 9, 2021

thanethomson Aug 7, 2021

Use flex-error for tendermint-rs errors #923

Use flex-error for tendermint-rs errors #923

Conversation

soareschen commented Jul 1, 2021 • edited by thanethomson Loading

codecov-commenter commented Jul 14, 2021 • edited Loading

Codecov Report

thanethomson commented Jul 21, 2021

soareschen commented Jul 21, 2021

thanethomson left a comment

Choose a reason for hiding this comment

thanethomson Aug 7, 2021

Choose a reason for hiding this comment

thanethomson Aug 7, 2021

Choose a reason for hiding this comment

soareschen Aug 9, 2021

Choose a reason for hiding this comment

thanethomson Aug 7, 2021

Choose a reason for hiding this comment

soareschen commented Jul 1, 2021 •

edited by thanethomson

Loading

codecov-commenter commented Jul 14, 2021 •

edited

Loading