Split relative locktime error up

[tcharding]

The relative module has a single general error type, we are moving away from this style to specific error types.

Split the relative::Error up into three error structs.

I forget the policy on public inner fields.

[tcharding]

I went to move the relative::Height and Time types to units and discovered the general error type. I'm not sure exactly what you are working on @Kixunil, hope I didn't overlap with you.

[coveralls]

Pull Request Test Coverage Report for Build 8217718201

Details

• 7 of 20 (35.0%) changed or added relevant lines in 2 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.04%) to 84.002%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/blockdata/transaction.rs	1	4	25.0%
bitcoin/src/blockdata/locktime/relative.rs	6	16	37.5%

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

Totals
Change from base Build 8214829480:	0.04%
Covered Lines:	19517
Relevant Lines:	23234

---

💛 - Coveralls

[Kixunil]

I was trying to pull out absolute before since you said it's hard, I assumed you're not going to do that soon. But got tangled in other things lately. Anyway, this PR doesn't conflict (yet?).

[tcharding]

I hadn't tried to move any code out of the locktime modules, I said (at least I meant) that pow was hard. Moving Height and Time from both absolute and relative shouldn't be a problem once the errors are improved.

[tcharding]

Only change in force push is improvement to the error message printed for two of the new errors.

[tcharding]

Can this go in? We need this if we want to move the locktime types to units which is required to move ParseIntError yada yada yada

[apoelstra]

ACK 09ea073

[tcharding]

Changes in force push:

• Made the error fields public and used name fields instead of tuple struct
• Did not include non_exhaustive - I believe these errors, and the functions in relative module, are stable and won't need changing.
• rebased to pick up Fix CI new build warnings #2488

[Kixunil]

Well, there's one weird property - one can create the error with funny values like seconds: 0 which is never actually wrong and thus the error is kinda invalid.

So I don't like it much but it's probably OK.

[apoelstra]

I think we'll have to live with it because Rust doesn't have ranged types (and it is a real PITA to create and maintain your own).

[Kixunil]

We can just hide the representation leaving the option to add it later. (And have a getter today.)

[tcharding]

We can do this but it seems excessive for an error type:

/// Input time in seconds was too large to be encoded to a 16 bit 512 second interval.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TimeOverflowError {
    /// Time value in seconds that overflowed.
    seconds: u32                // We maintain an invariant that this value does indeed overflow.
}

impl TimeOverflowError {
    pub fn new(seconds: u32) -> Self {
        let overflows = u16::try_from((seconds + 511) / 512).is_ok();
        assert!(overflows);
        TimeOverflowError { seconds }
    }
}

I personally think the error is fine as it is. We are not here to protect every programmer from every single programming mistake they might make.

[apoelstra]

@tcharding the cool thing is that we don't need new because users should never be directly constructing our error types.

And internally of course we can just directly construct them.

[Kixunil]

@tcharding I don't think so. I still see the field as pub, you didn't address @apoelstra's comment and I agree with his comment.

[tcharding]

Oh, damn - I removed the non_exhaustive thinking the error as stable inadvertently allowing construction, thereby forgetting this discussion thread.

[tcharding]

I made the field pub(crate) and added a comment so it doesn't get un-privated.

[tcharding]

Note also that the constructor has been removed - I believe this thread is resolved.

[apoelstra]

I am going to just disable the "resolve conversation" rule.

[apoelstra]

ACK 2213cdd except for the pub/accessor thing, which would be OK as a followup PR that defined having accessors as part of our error policy

[tcharding]

Please note the new error types in this PR are not hider errors, are returned from functions that are stable, and therefore have public fields and do not use non_exhaustive.

[Kixunil]

@tcharding I don't think so. I still see the field as pub, you didn't address @apoelstra's comment and I agree with his comment.

[Kixunil]

FTR, if we want to stabilize sooner maybe we should just hide all errors entirely and we can go back and uncover (parts of) them later.

[tcharding]

FTR, if we want to stabilize sooner maybe we should just hide all errors entirely and we can go back and uncover (parts of) them later.

I made both incompatible error types non_exhaustive

This should be Height to statically represent that the lock was height.

I"m not sure about this. If we do this (and similar change for Time) then then the errors become the same shape, opening up the debate about using same errors in different functions. Secondly the input is a LockTime so I think the error should be a LockTime so debuggers are looking at exactly the same thing. I don't feel strongly about either argument though.

Your comments did uncover that I was using the errors incorrectly (for how I had meant it to be used) though which is interesting. Now is_satisfied_by_time returns IncompatibleTimeError and vice versa where as before I had them backwards.

[Kixunil]

I made both incompatible error types non_exhaustive

That's not always sufficient.

If we do this (and similar change for Time) then then the errors become the same shape

Firstly, not really, they are mirrored (and we would need a third field to distinguish them). Secondly I think that being imprecise just to avoid the discussion is just lying to ourselves. If the discussion is not worth slowing down stabilization just make them opaque.

Secondly the input is a LockTime so I think the error should be a LockTime so debuggers are looking at exactly the same thing.

The concrete types imply the outer one, so IMO it still is the same thing.

Your comments did uncover that I was using the errors incorrectly

Cool!

[sanket1729]

ACK 7c0cc02

[Kixunil]

ACK 7c0cc02

[tcharding]

Both unresolved comments are yours @Kixunil, I believe they are both resolved.

[tcharding]

Rebase only, no other changes.

[sanket1729]

utACK 3c8edae

[apoelstra]

Note that a hidden discussion links to issue #2553 about renaming local variables inside some method.

[apoelstra]

ACK 3c8edae