Improve lock time errors #2417

Kixunil · 2024-01-29T22:44:42Z

The errors returned from various lock time functions had several issues. Among the obvious - Error being returned from all operations even when some of its variants were unreachable, there were subtle issues around error messages:

ParseIntError didn't contain information whether the parsed object is Height or Time.
Logically overflow and out-of-bounds should be the same thing but produced different error messages.
Mentioning integers is too technical for a user, talking about upper and lower bound is easier to understand.
When minus sign is present std reports it as invalid digit which is less helpful than saying negative numbers are not allowed.

It is also possible that ParseIntError will need to be removed from public API during crate smashing or stabilization, so avoiding it may be better.

This commit significantly refactors the errors. It adds separate types for parsing Height and Time. Notice that we don't compose them from ParseIntError and ConversionError - that's not helpful because they carry information that wouldn't be used when displaying which is wasteful. Keeping errors small can be important.

It's also worth noting that exposing the inner representation could cause confusion since the same thing: out of bounds can be represented as an overflow or as a conversion error. So for now we conservatively hide the details and even pretend there's no source in case of overflow. This can be expanded in the future if needed.

The returned errors are now minimal. LockTime parsing errors are currentlly unchanged.

I can add LockTime changes in the same commit or separate within this PR if you want. Just wanted to push something for review before I go to sleep.

coveralls · 2024-01-29T22:45:49Z

Pull Request Test Coverage Report for Build 7727384831

-52 of 74 (29.73%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 84.148%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/parse.rs	0	4	0.0%
bitcoin/src/blockdata/locktime/absolute.rs	22	70	31.43%

Files with Coverage Reduction	New Missed Lines	%
bitcoin/src/blockdata/locktime/absolute.rs	3	64.27%

Totals
Change from base Build 7704305879:	-0.2%
Covered Lines:	19328
Relevant Lines:	22969

💛 - Coveralls

apoelstra · 2024-01-30T16:41:45Z

bitcoin/src/blockdata/locktime/absolute.rs

+        Self::from_consensus(n).map_err(Into::into)
+    }
+    */
+}


In 670c16b:

Did you mean to leave this commented out code here?

Definitely not, thanks!

apoelstra · 2024-01-30T16:44:03Z

bitcoin/src/blockdata/locktime/absolute.rs

+        let n = u32::try_from(n).map_err(|_| ParseError::Conversion(n))?;
+        Self::from_consensus(n).map_err(Into::into)
+    }
+}


In 670c16b:

There are two error paths here which both wind up returning the ParseError::Conversion variant. Is there a way this function could return ConversionError instead?

Oh, this whole method is not needed, I messed up hex parsing for Time.

But to give a bit of context, it was originally intentional since it's a private method that is only used as parsing primitive and it helps to return the internal type because in theory it makes constructing the other errors simpler. This worked well in my other projects so I just reused the pattern however it turns out it's not really that useful in this code.

tcharding

A few more questions please:

With this applied we have three functions in this file called from_hex_str_no_prefix each with a different implementation, its not obvious why they are different?
Why does parse_hex exist to be only called once? (Related to point above.)
We are parsing hex, I don't understand why all the signed stuff? Are you trying to improve the error if someone attempts to parse in negative hex as a lock time?

tcharding · 2024-01-31T00:51:52Z

bitcoin/src/blockdata/locktime/absolute.rs

+#[cfg(feature = "std")]
+impl std::error::Error for ParseHeightError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.0.source()


Why is it written like this instead of Some(&self.0)? Won't this loose one layer of nesting because source is supposed to return the error source witch is ParseError not ParseError's source?

I intentionally want to hide ParseError because it's internal and in addition as you can see that method is intentionally not std::error::Error method because that would require Display but displaying requires additional context. And the method is needed because I want to filter-out overflow errors.

I think it's worth a comment here because the code looks weird.

OK, I've copied the comment from the other source fn.

tcharding · 2024-01-31T00:55:12Z

bitcoin/src/blockdata/locktime/absolute.rs

+impl From<ParseError> for ParseHeightError {
+    fn from(value: ParseError) -> Self {
+        Self(value)


Suggested change

impl From<ParseError> for ParseHeightError {

fn from(value: ParseError) -> Self {

Self(value)

impl From<ParseError> for ParseHeightError {

fn from(e: ParseError) -> Self { Self(e) }

This is an extremely trivial review suggestion but I get the feeling that having all the error code super uniform is going to pay dividends in the future. For the rest of time (hyperbolic I know) devs will want to flick to error code and quickly parse it. Any differences cause one to double take. I rekon we should only have differences when they are meaningful, to help catch peoples eye.

I'll try to run fmt and see if it makes diff too crazy or not.

This whole not being able to format PRs because of one devs requirement to not use nightly, which is now required for clippy anyways is really a PITA.

clippy warnings aren't too common and are easy to fix in the reasonably-rare case they show up in CI. Vs formatting issues which show up in probably 98% of all PRs unless the user is running a format tool locally. So the clippy nightly requirement isn't really a burden on individual developers.

The real issue is the Rust ecosystem's total lack of a stable working formatting tool. And even the unstable one is unable to format diffs. I hardly blame individual developers who are uncomfortable frequently downloading binary blobs from the developers of that mess.

tcharding · 2024-01-31T01:08:05Z

bitcoin/src/blockdata/locktime/absolute.rs

+        let height = i64::from_str_radix(s.as_ref(), 16)
+            .map_err(ParseError::invalid_int(s))?;


I don't understand this change, why call from_str_radix on a signed type? I'm guessing you are trying to improve the error reporting coming from std but I don't see how?

I see // we use i64 to have nicer messages for negative values but I don't see where we get negative values from?

From parsing, it's in the PR description. For -42 std says "invalid digit" which is confusing, I want to just say that the value is below the limit which is 0. And for that it's easiest to parse i64 because if there actually is another invalid digit (-42blocks) then the error makes sense and i64 losslessly holds u32. (Technically i33 would be enough...)

Should probably elaborate the comment here as well.

There is a comment on the variant. I can copy it.

When minus sign is present std reports it as invalid digit which is less helpful than saying negative numbers are not allowed.

Ha! Guess I did not read the PR description close enough. A wee code comment on the i64 would be great please.

Oh you've pushed already - excuse the noise, I was mindlessly reading the thread.

Kixunil · 2024-01-31T09:32:57Z

Why does parse_hex exist to be only called once?

Because I messed up, it's supposed to be used twice and the two of the methods should be same. I've explained it above.

Are you trying to improve the error if someone attempts to parse in negative hex as a lock time

Yes, though it's a side effect of improving decimal parsing. I think that error message about -42 having invalid digit is weird. (And yes, I realize my change makes parsing a bit slower in the error case, I think it's fine.)

The errors returned from various lock time functions had several issues. Among the obvious - `Error` being returned from all operations even when some of its variants were unreachable, there were subtle issues around error messages: * `ParseIntError` didn't contain information whether the parsed object is `Height` or `Time`. * Logically overflow and out-of-bounds should be the same thing but produced different error messages. * Mentioning integers is too technical for a user, talking about upper and lower bound is easier to understand. * When minus sign is present `std` reports it as invalid digit which is less helpful than saying negative numbers are not allowed. It is also possible that `ParseIntError` will need to be removed from public API during crate smashing or stabilization, so avoiding it may be better. This commit significantly refactors the errors. It adds separate types for parsing `Height` and `Time`. Notice that we don't compose them from `ParseIntError` and `ConversionError` - that's not helpful because they carry information that wouldn't be used when displaying which is wasteful. Keeping errors small can be important. It's also worth noting that exposing the inner representation could cause confusion since the same thing: out of bounds can be represented as an overflow or as a conversion error. So for now we conservatively hide the details and even pretend there's no `source` in case of overflow. This can be expanded in the future if needed. The returned errors are now minimal. `LockTime` parsing errors are currentlly unchanged.

apoelstra

ACK 93dba89

tcharding

ACK 93dba89

tcharding · 2024-01-31T20:20:34Z

Thanks for formatting!

Kixunil added the API break This PR requires a version bump for the next release label Jan 29, 2024

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Jan 29, 2024

Kixunil force-pushed the locktime-errors branch 2 times, most recently from 94e612f to 670c16b Compare January 30, 2024 09:16

apoelstra reviewed Jan 30, 2024

View reviewed changes

tcharding reviewed Jan 31, 2024

View reviewed changes

Kixunil force-pushed the locktime-errors branch from 670c16b to 93dba89 Compare January 31, 2024 14:14

apoelstra approved these changes Jan 31, 2024

View reviewed changes

tcharding approved these changes Jan 31, 2024

View reviewed changes

apoelstra merged commit 8efaf4a into rust-bitcoin:master Jan 31, 2024
31 checks passed

Kixunil deleted the locktime-errors branch January 31, 2024 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve lock time errors #2417

Improve lock time errors #2417

Kixunil commented Jan 29, 2024

coveralls commented Jan 29, 2024 •

edited

apoelstra Jan 30, 2024

Kixunil Jan 30, 2024

apoelstra Jan 30, 2024

Kixunil Jan 30, 2024

tcharding left a comment

tcharding Jan 31, 2024

Kixunil Jan 31, 2024

apoelstra Jan 31, 2024

Kixunil Jan 31, 2024

tcharding Jan 31, 2024

Kixunil Jan 31, 2024

tcharding Jan 31, 2024

apoelstra Jan 31, 2024

tcharding Jan 31, 2024

tcharding Jan 31, 2024

Kixunil Jan 31, 2024

apoelstra Jan 31, 2024

Kixunil Jan 31, 2024

tcharding Jan 31, 2024

tcharding Jan 31, 2024

Kixunil commented Jan 31, 2024

apoelstra left a comment

tcharding left a comment

tcharding commented Jan 31, 2024

		let height = i64::from_str_radix(s.as_ref(), 16)
		.map_err(ParseError::invalid_int(s))?;

Improve lock time errors #2417

Improve lock time errors #2417

Conversation

Kixunil commented Jan 29, 2024

coveralls commented Jan 29, 2024 • edited

Pull Request Test Coverage Report for Build 7727384831

💛 - Coveralls

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

tcharding 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

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

Kixunil commented Jan 31, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Jan 31, 2024

coveralls commented Jan 29, 2024 •

edited