shardtree: make construction of fully-empty Parent nodes an error #131
Conversation
Ideally the errors here would all be panics, as construction of such a node represents a programming error; however, it is preferable to return extra contextual information about the circumstance that led to this error where possible, so we model this as an error where possible without altering the public API.
nuttycom
force-pushed
the
checked_prunable_parent_construction
branch
from
463fec6 to
1d7cd42
Compare
| @@ -286,7 +291,7 @@ fn combine_with_empty<H: Hashable + Clone + PartialEq>( | |||
| incomplete: &mut Vec<IncompleteAt>, | |||
| contains_marked: bool, | |||
| prune_below: Level, | |||
| ) -> LocatedPrunableTree<H> { | |||
| ) -> Result<LocatedPrunableTree<H>, Address> { | |||
There was a problem hiding this comment.
Using an Address directly as an error type is odd.
There was a problem hiding this comment.
It's the address at which inconsistencies were found; anyway, Result is just a badly-named Either.
There was a problem hiding this comment.
We've discussed this before I think. This is not an idiomatic use of Result: it normally takes an error type on the right. The error type can just be a wrapper around Address if you want, but by convention it implements Debug, fmt::Display, and std::error::Error if appropriate, something like this:
/// Error indicating an inconsistency at a given address.
#[derive(Debug)]
pub struct InconsistencyError(pub Address);
impl fmt::Display for InconsistencyError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Inconsistency found at address {}", self.0)
}
}
#[cfg(feature = "std")]
impl std::error::Error for InconsistencyError {}There was a problem hiding this comment.
Part of the problem is that this fundamentally shouldn't be an error; I intend to remove these errors in the future in favor of it being a panic, once I can figure out where the actual programming error is.
I guess the alternative is to make it a panic right now? But my worry is that doing so could further brick wallets. That being said, those wallets can't currently send if the note in the tree with the inconsistency is implicated, so maybe that's no worse.
The more I think about it the more I'm inclined to defer this change and not block on it for this release. It should be a panic, the bug should be fixed, and zcash/librustzcash#1709 should provide the remediation for issue.
| // construct the replacement node bottom-up | ||
| let mut node = self; | ||
| let mut incomplete = vec![]; | ||
| while node.root_addr.level() < level { |
There was a problem hiding this comment.
This function cannot guarantee that the returned tree's root is at level, because self.root_addr.level() might already be greater than level. Is this intended (or would it be an error by the caller)? Either way it should be documented.
There was a problem hiding this comment.
Ah, yeah, factoring this out as a standalone function instead of a closure means that the invariants that were enforced by the enclosing scope are no longer checked. I don't think that's a blocker though; this is just used in the one place for now.
Ideally the errors here would all be panics, as construction of such a node represents a programming error; however, it is preferable to return extra contextual information about the circumstance that led to this error where possible, so we model this as an error where possible without altering the public API.