Improve error handling in the merkle_tree::block module
#1390
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In preparation for moving `MerkleBlock` into the `merkle_tree` module; create a new directory for the module and move `merkle_tree.rs` to `merkle_tree/mod.rs`.
Move the `merkleblock` module into the `merkle_tree` module in a submodule called `block`. In order to do the minimum amount of changes in this patch DO NOT rename types to improved naming and reduce stutter. Note: - block module is private - the three types are re-exported from `merkle_block` This patch purposefully does the minimum amount of changes because there a whole bunch of improvements to the old "merkleblock" module that are coming next.
We have a test directory for holding test data, use it to for the hex used for testing the `MerkleBlock`.
The macro has unusual indentation, fix to to be more regular.
Reading the code is arguably easier if we have seen the call site before seeing the function, saves having to think what the function does.
Refactor import statements by doing: - Put all the crate imports together - Use `self` to import the error variants to make it more explicit
Code is arguably easier to read if the important stuff comes first. Rearrange the `merkle_tree::block` module with this in mind, put the `MerkbleBlock` struct at the top of the file, then the `PartialMerkleTree`, and then the error at the bottom. Refactor only, no logic changes.
Add a unit test for a `PartialMerkleTree` with node counts of 1-7
Add a private method to the `PartialMerkleTree` to calculate the height. Enables removal of duplicate code.
By convention we name errors `Error` and use `module::Error` to disambiguate them when needed. Rename the `MerkleBlockError` type to `Error`.
Instead of a `String` nested in `BadFormat` add an error variant for each type of error. Note that the `Error` is marked with `non_exhaustive` so future variants can still be added.
We have a public method that panics if the input is invalid, this is unnecessary we should return an error. Add a new error variant `ArrayLengthsNotEqual`, use it and the existing `NoTransactions` for the error paths instead of panicing. While we are changing the function signature, take ownership of the `header` parameter because we use it to build the returned data structure - taking ownership makes more sense than borrowing and copying.
52156ea to
dbed0c7
Compare
Kixunil
reviewed
Nov 15, 2022
There was a problem hiding this comment.
I'm not really sure about changing panics. In general invalid parameters do panic if the API would be too annoying otherwise and the caller is responsible for valid inputs.
As a rule of thumb, if an input is expected to come from an external source it's better to return error than panic. In this case the code assumes valid Block but the Block type has no validity invariants (maybe it should?). I think there's a good chance Block will come from an external source.
At the same time the API looks quite unfortunate. It forces allocations where I think they could be avoided. Things like supplying an empty slice or unequal lengths do look like programmer errors but again if they are likely to come from an external source maybe they aren't.
|
Closing due to uncertainty about benefits of the PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR 4 in the
merkle_tree::blockseries, builds on top of #1388 and #1389. This PR is the last 3 patches.Improve error handling in the
merkle_tree::blockmodule by doingMerkleBlockError->Error. Please note I did this before moving the merkleblock module, now with the new location and the current reexports I'm not sure its exactly correct. I think its still a step in the right direction though.I'm still not 100% sure I understand where we are going with error handling, please feel free to school me if this is not going in the correct direction.