Add API method `absolute::LockTime::is_satisfied_by_lock` by tcharding · Pull Request #1258 · rust-bitcoin/rust-bitcoin

tcharding · 2022-09-11T22:40:00Z

Patch 1 is a docs improvement.

Patch 2 commit log:

When implementing the absolute lock time API we decided on not
supporting checking lock satisfaction with another lock, instead we
provided a pattern in the docs for doing so. Fast forward a months and
I, the primary author, then forgot to use the correct pattern when using
the API in rust-miniscript - this is a sure sign that the API is too
hard to use. In this time we worked on the relative lock API and came up
with a is_satisfied_by_lock method - this is identical to the required
use case in the absolute lock time module.

Add a method on absolute::LockTime for checking a lock against another
lock, add rustdoc comment explaining the methods function in filtering
prospective lock time values (how we use it in rust-miniscript).

apoelstra

ACK c25e93d2a7efc96f896ffc8d2215a5ebb88f6bb5

Currently in one of the rustdoc examples showing lock satisfaction we use two locks with the same value, this obfuscates which lock is doing the satisfying and which lock is being satisfied. Increment the value in one of the locks so it is obvious which lock is which.

tcharding · 2022-09-14T00:24:45Z

Rebase only, no other changes.

apoelstra

ACK 999a3029ece778c952073db6a277dcca64d63491

Kixunil

I was thinking that locks don't satisfy other locks, only block height and time can satisfy other locks, so the function is weird. And then it hit me.

The correct name for the method is is_implied_by_lock (or you could reverse the arguments and call it implies_lock). This is also the exact semantics used in miniscript if my understanding is correct. Once the name is not confusing the return values are suddenly natural - of course a height doesn't imply a time or vice versa.

This is so much clearer I request to change it. :)

Kixunil · 2022-09-14T10:01:04Z

bitcoin/src/blockdata/locktime/absolute.rs

Please, use more meaningful names for n and m. I suggest this and other

Sure thing, will fix. Thanks

Kixunil · 2022-09-14T10:05:57Z

bitcoin/src/blockdata/locktime/absolute.rs

Shouldn't this be filtering out locks that are guaranteed to be satisfied? Creating conflicting locks is impossible in Bitcoin.

I believe the existing text is correct, but it is possible that we're interpreting "filtering out" differently..

If a locktime can't be satisfied then you want to eliminate any part of your script that depends on it, since it's unsatisfiable.

How it's possible to create unsatisfiable lock time in Bitcoin? I've seen people explaining that Bitcoin doesn't allow that because it'd greatly complicate reorgs.

Within a complex script you can have locktime checks that simply aren't executed and wouldn't have been valid if they were. (e.g. by using BOOLOR)

AFAIR all lock time checking opcodes have verify semantics so you can't put FALSE on stack if the lock time is not satisfied.

What I mean is you could have a situation where there are two checks: whether height is above x and x + n. x + n implies x and thus x is not needed because it's guaranteed to be satisfied.

<B> OP_CHECKSIG OP_NOTIF <C> OP_CHECKSIG OP_NOTIF <e803> OP_CHECKSEQUENCEVERIFY OP_VERIFY OP_ENDIF OP_ENDIF <A> OP_CHECKSIG

This is the default example on https://bitcoin.sipa.be/miniscript/ -- you can simplify it a bit and click "Compile" but if you make it too simple it won't bother wrapping the CSV/CLTV in IF/ENDIF.

I don't understand, it has only one OP_CHECKSEQUENCEVERIFY so there is no other to compare to.

When you are analyzing the transaction you would use the sequence number from the transaction (or from a hypothetical transaction you are thinking of creating) to compare it.

Ah, that makes sense. Given how long it took me to get it, I think we should have a clearer comment. Maybe something like this:

Can be used to check if OP_CHECKSEQUENCEVERIFY allows specific sequence. Example:

txin.sequence.is_implied_by_lock(csv_param)

Or remove the smaller value of two OP_CHECKSEQUENCEVERIFY operations within one branch of the script.

The example given is not quite right, we have to convert sequence to a relative lock time before calling is_implied_by. I've updated the comments in both absolute and relative modules. Done as a separate patch since the docs are similar.

tcharding · 2022-09-14T21:40:51Z

I was thinking that locks don't satisfy other locks, only block height and time can satisfy other locks, so the function is weird.

I agree that it is weird if a lock satisfied another lock, I also think that this 'lock time' concept in Bitcoin is just plain hard to think about. That being said, we are checking if the lock time, that represents an absolute lock (self) is satisfied by a 'lock time value' - a 'lock time value' encodes height and time. I don't think introducing the term 'implies' helps, I looked up synonyms of 'imply' and also I don't think any of them are better, better enough to warrant another term. 'satisfies' is already a term in use, in our code and and in Bitcoin in general (I believe).

tcharding · 2022-09-14T21:43:08Z

Used this and other in pattern match as suggested.

Kixunil · 2022-09-14T22:43:18Z

Lock time can only be satisfied by n blocks being mined or n seconds passing. If you have two lock times with same unit then larger lock time being satisfied implies (in mathematical sense) the smaller one being satisfied.

I was thinking of an alternative satisfied_implies_satisfaction_of but seemed too long. If it's actually more clear then I'd still prefer it over satisfied.

tcharding · 2022-09-14T22:57:21Z

oooh, I get what you mean by implies now, thanks for clarifying.

When filtering it is necessary to check two lock times against each other, we currently provide a patter for doing so in the docs but we can do better. It was observed that satisfaction of a lock time 'implies' satisfaction of another lock time if the lock times are the same unit and one locks value is less than the others - this is exactly the code pattern we suggest for filtering. Add a method on `absolute::LockTime` for checking a lock against another lock, add rustdoc comment explaining the methods function in filtering prospective lock time values (how we use it in `rust-miniscript`).

tcharding · 2022-09-14T23:23:26Z

I use is_implied_by and also added a patch to do the same for relative::LockTime.

sanket1729

ACK 11d80ad4d00850a57edd26f35c5d859624d5ae7f

sanket1729 · 2022-09-16T10:26:17Z

bitcoin/src/blockdata/locktime/relative.rs

Really a niche application. Perhaps unnecessary to write this. I can't think of any setting where someone would use two timelocks in the same branch.

Isn't miniscript supposed to do optimizations like this?

I agree with Sanket. This seems really niche and took me a long time to think about what situations it would be done. (As near as I can tell, only if you had a conjunction of two locktimes for some reason, but didn't want this.)

@Kixunil miniscript does not do optimizations like this, though I suppose that it could ... but even if you were eliminating locktimes it's not obvious that you'd always remove the smaller one.

Kixunil · 2022-09-19T08:16:04Z

bitcoin/src/blockdata/locktime/relative.rs

The function was not available in a published release, so no point of deprecating - just remove it.

Yes, you are right - thanks.

As we just did for `absolute::LockTime` add a method `is_implied_by` and deprecate `is_satisfied_by_lock`. Reasoning: it is odd to think of a lock satisfying another lock but it is clear to see that satisfaction of one lock can imply satisfaction of another.

The lock time methods are a source of endless confusion; make an attempt at improving further the documentation on the two `is_implied_by` methods (one on absolute lock time and one on relative).

tcharding · 2022-09-19T22:42:11Z

Removed deprecation of is_satisfied_by_lock, no other changes.

Kixunil

ACK 8aa94bd

apoelstra

ACK 8aa94bd

tcharding mentioned this pull request Sep 11, 2022

Upgrade bitcoin dependency to v0.29.0 rust-bitcoin/rust-miniscript#450

Merged

tcharding force-pushed the 09-12-is-satisfied-by-lock branch from 69c4748 to c25e93d Compare September 11, 2022 23:06

tcharding added this to the 0.30.0 milestone Sep 11, 2022

tcharding added minor API Change This PR should get a minor version bump release notes mention labels Sep 11, 2022

apoelstra previously approved these changes Sep 12, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 999a302 September 14, 2022 00:24

tcharding force-pushed the 09-12-is-satisfied-by-lock branch from c25e93d to 999a302 Compare September 14, 2022 00:24

apoelstra previously approved these changes Sep 14, 2022

View reviewed changes

Kixunil requested changes Sep 14, 2022

View reviewed changes

Kixunil mentioned this pull request Sep 14, 2022

Rename is_satisfied_by_lock to is_implied_by_lock (or similar) #1271

Closed

tcharding dismissed apoelstra’s stale review via 6d49b1b September 14, 2022 21:42

tcharding force-pushed the 09-12-is-satisfied-by-lock branch from 999a302 to 6d49b1b Compare September 14, 2022 21:42

tcharding force-pushed the 09-12-is-satisfied-by-lock branch from 6d49b1b to a838092 Compare September 14, 2022 23:23

sanket1729 previously approved these changes Sep 16, 2022

View reviewed changes

tcharding requested a review from Kixunil September 18, 2022 22:27

Kixunil requested changes Sep 19, 2022

View reviewed changes

tcharding added 2 commits September 20, 2022 08:40

Improve docs on is_implied_by

8aa94bd

The lock time methods are a source of endless confusion; make an attempt at improving further the documentation on the two `is_implied_by` methods (one on absolute lock time and one on relative).

tcharding dismissed sanket1729’s stale review via 8aa94bd September 19, 2022 22:41

tcharding force-pushed the 09-12-is-satisfied-by-lock branch from 11d80ad to 8aa94bd Compare September 19, 2022 22:41

tcharding requested a review from Kixunil September 19, 2022 22:42

Kixunil approved these changes Sep 20, 2022

View reviewed changes

apoelstra approved these changes Sep 20, 2022

View reviewed changes

apoelstra merged commit a0899eb into rust-bitcoin:master Sep 20, 2022

tcharding deleted the 09-12-is-satisfied-by-lock branch September 21, 2022 00:19

Conversation

tcharding commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 19, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Sep 11, 2022 •

edited

Loading