Skip to content

Fix up mixup of age/height #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jun 10, 2022
Merged

Fix up mixup of age/height #424

merged 5 commits into from
Jun 10, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jun 3, 2022 •
edited
Loading

We currently use age/height as identifiers for values related to CLTV and CSV. However we get them mixed up in the function call to from_txdata. Do a few improvements to fix the mixup and improve the overall situation.

  • Patch 2 is a breaking change, changes a public method name.
  • Patch 1 an 4 are bug fixes.

(This description and PR are totally re-written, the comment directly below is now stale.)

@tcharding tcharding mentioned this pull request Jun 3, 2022
Closed
@apoelstra
Copy link
Member

Are we actually using age and height to classify things? I would expect two classifications - height vs time and relative vs absolute ... where I would expect age to be a synonym for relative.

@tcharding tcharding force-pushed the 06-03-rename-age-height branch from 4466a8b to 06d7c53 Compare June 6, 2022 00:37
@tcharding tcharding changed the title Use lock_time instead of age Fix up mixup of age/height Jun 6, 2022
@tcharding tcharding marked this pull request as ready for review June 6, 2022 00:39
tcharding added 4 commits June 6, 2022 11:15
@tcharding
Use correct parameter call order
bb091eb 
We are passing `csv` and `cltv` variables to `from_txdata` in the wrong
order. While we are at it, add code comments linking age/csv and
height/cltv.
@tcharding
Re-name function at_height -> at_lock_time
bfd6991 
The function takes as a parameter that represents the argument from
`OP_CLTV`, this is a height _or_ time. Improve the function name to
reflect this.
@tcharding
Re-name function parameter to 'age'
b9c9d5b 
The parameter to `to_age` represents either a number of blocks _or_ a
time (medium time passed). The term 'age' is associated with relative
time locks in general.

Use 'age' as the parameter name instead of 'time'.
@tcharding
Pass correct fields to evaluate_[after|older]
3d76d02 
We are passing the wrong fields into `evaluate_after` and
`evaluate_older`. 'after' is associated with _absolute_ timelocks, this
means 'height'. 'older' is associated with _relative_ timelocks, this
means 'height'.
@tcharding tcharding force-pushed the 06-03-rename-age-height branch from 8fa8cdc to 3d76d02 Compare June 6, 2022 01:16
@apoelstra
Copy link
Member

Done reviewing 3d76d02.

I'm still confused by the logic in the last commit ... it seems like we are using "height" vs "age", which as I've mentioned in my first comment, seems like a wrong way to classify. But maybe this should be fixed in a later PR? This PR is definitely an improvement.

@tcharding
Copy link
Member Author

I'm trying to detangle things here in rust-miniscript while simultaneously creating the new 'timelocks' API in rust-bitcoin. I'm held back by the fact that I keep getting confused, for the last two weeks I thought 'age' went with CLTV, only after your original comment on this PR did I realize the mix ups we had.

I'm still confused by the logic in the last commit ... it seems like we are using "height" vs "age"

I thought I was in line with your original comment, the last commit fixes the fact that we pass the wrong value to evaluate_older and evaluate_after. It also removes the usage of 'age' in relation to CLTV but keeps the usage of 'age' for CSV.

I'm taking baby steps, mainly focusing on CLTV. The weakness in this approach is I'm not 100% across all the intricacies of CSV right at this minute so this might mean some churn when I move onto that part of the timelocks.

@apoelstra
Copy link
Member

evaluate_after should take an absolute timelock while evaluate_older should take a relative one. Height versus time is a different classification.

@tcharding
Copy link
Member Author

tcharding commented Jun 7, 2022 •
edited
Loading

evaluate_after should take an absolute timelock while evaluate_older should take a relative one. Height versus time is a different classification.

I think we are on the same page, I believe the PR is correct, and believe that patch 1 and 4 are rectifying real bugs.

Oooooh, I finally get your comments, you mean to say that you would expect this PR to include a change to the field names of Interpreter so we are not using height/age. I'm going to get rid of height altogether ...

@tcharding
Copy link
Member Author

I pushed an additional patch that removes height in favour of lock_time. I left it separate because I think its better to see each tiny change as a separate patch even though they are all interdependent. Can squash the whole thing into a single patch once you are convinced the changes are correct also.

@tcharding tcharding force-pushed the 06-03-rename-age-height branch from c7483cc to 454db6e Compare June 7, 2022 23:51
@tcharding
Use lock_time instead of height
fec8014 
Currently we are using the identifier `height` for the CLTV vaule. This
is incorrect because the value can be either a height or time. There is
also a risk of mixing up the value with the CSV `age` value.

Use `lock_time` to clearly disambiguate the CLTV value.
@tcharding tcharding force-pushed the 06-03-rename-age-height branch from 454db6e to fec8014 Compare June 8, 2022 00:08
apoelstra
apoelstra approved these changes Jun 8, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fec8014

sanket1729
sanket1729 approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fec8014

@sanket1729 sanket1729 merged commit c80aa7c into rust-bitcoin:master Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcharding @apoelstra @sanket1729