Improve lock times - fix off-by-one bug - #4468
Conversation
github-actions
Bot
added
C-bitcoin
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
github-actions
Bot
added
the
API break
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
|
Nice. I believe this is correct. |
|
Mad, thanks. |
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
tcharding
changed the title
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
tcharding
changed the title
tcharding
changed the title
We recently improved the relative locktime function `is_satisfied_by` by adding mined at and chain tip. We can now do the same for the height/time satisfaction functions. Note I believe these functions should still be provided because a user may for some reason have either blocktime data or height data and not have the other. Requires some work to the errors, elect to just remove the original field that held the function argument. For now remove the examples in rustdocs, we can circle back to these once the dust settles.
Define 'is satisfied by' - this is a classic off-by-one problem, if a relative lock is satisfied does that mean it can go in this block or the next? Its most useful if it means 'it can go in the next' and this is how relative height and MTP are used in Core. Ramifications: - When checking a time based lock we check against the chain tip MTP, then when Core verifies a block with the output in it it uses the previous block (and this is still the chain tip). - When checking a height base lock we check against chain tip height + 1 because Core checks against height of the block being verified. Additionally we currently have a false negative in the satisfaction functions when the `crate` type (height or MTP) is to big to fit in a u16 - in this case we should return true not false because a value too big definitely is > the lock value. One final API paper cut - currently if the caller puts the args in the wrong order they get a false negative instead of an error. Fix all this by making the satisfaction functions return errors, update the docs to explicitly define 'satisfaction'. For now remove the examples in rustdocs, we can circle back to these once the dust settles. API test of Errors: Some of the errors are being 'API tested' tested in `primitives` but they should be being done in `units/tests/api.rs` - put all the new errors in the correct places.
862069e to
9c4466c
Compare
|
I've rolled #4486 into this PR, I think it makes more sense to review it all at once. |
tcharding
changed the title
When checking a locktime against block height we add 1 because when the
next block is being validated that is what the height will be and
`is_satisfied_by` is defined to return true if a transaction with this
locktime can be included in the next block.
As we have in `relative`, and for the same reasons, add an API to the
absolute `Height` and `MedianTimePast`: `is_satisfied_by`. Also add
`is_satisfied_by_{height, time}` variants to `absolute::LockTime`.
Call through to the new functions in `units`.
Use lock-by-time and lock-by-height instead of lock-by-blocktime and lock-by-blockheight respectively with no loss of clarity.
Use the new function and not the deprecated on in rustdcos and tests.
Move the `_by_{height,time}` functions to be underneath the
`is_satisfied_by` function.
Code move only, no logic change.
Slightly improve grammar and fix column width to 100.
`units::locktime::relative::Height` type is now deprecated, use the new name in rustdoc.
9c4466c to
4ccecf5
Compare
|
I just threw 5 extra patches on top of this. They are all clean ups and I'm working on the assumption that you will not review the diff as much as the final state of the locktime modules. These cleanups should help the effort. |
|
Setting locktime and fee rate work as top priority ATM. Locktimes are looking pretty good after this is merge IMO. |
|
Bump please. This and the fee rate stuff are the final remaining pieces before we can release the units 1.0-alpha. |
|
Nice. Will ACK and merge. I am torn between "user mixes up current and confirmed blocktime" being a panic or a result. I guess if it's a result the user has the choice to either do The business with internally adding +1s in places also feels weird but I think it's the right thing to do. |
2 participants
Make the APIs uniform in relative and absolute locktimes in relation to the
is_satisfied_byfunctions. In doing so improve the API and fix an off-by-one bug when checking satisfaction of locks by height.Done in three patches but maybe should be squashed? Probably easiest to review by looking at all the
is_satisfied_by*functions and convincing yourself we got it right.EDIT: Now has 5 cleanup patches also (mostly docs cleanups).