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.
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
Demonstrate use of new LockTime
type to replace u32 in After
.
#408
Conversation
Nice! concept ACK. |
27e72b0
to
779a715
Compare
This new timelock module could actually go in rust-bitcoin, there is nothing specific to miniscript in it? I've been learning quite a bit as I go along, the timelock module API is now kind of bloated. Any review that nacks methods or suggests re-names if my understanding is incorrect would be super appreciated please. |
Oh, yeah, nice idea. I'd be in favor of bringing timelocks into rust-bitcoin. Not sure how the other maintainers would feel. Then we could add accessor functions to |
779a715
to
3b5cf23
Compare
Please review patch 2 of rust-bitcoin/rust-bitcoin#994 to see if I got everything you wanted. |
4d2d119
to
726b086
Compare
This looks great! I did not review it in detail. |
726b086
to
59f8593
Compare
b084010 Use terse functional programming terms (Tobin C. Harding) 6f3303d Improve docs on TimelockInfo (Tobin C. Harding) b2f6ef0 Add unit test for combine_threshold (Tobin C. Harding) a36f608 Be uniform in spelling of timelock (Tobin C. Harding) 51de643 Rename comibine methods (Tobin C. Harding) ef6803f Use > 1 instead of >= 2 (Tobin C. Harding) d1fdbaa Use LOCKTIME_THRESHOLD same as bitcoin core (Tobin C. Harding) Pull request description: Done while working on a [timelock module](rust-bitcoin/rust-bitcoin#994). This is all the initial patches (except one) from #408 (which is a PR displaying usage of the new timelock module). Note, does _not_ do the 'make `TimelockInfo` fields pub crate' change - I was unsure if this was correct. Top commit has no ACKs. Tree-SHA512: aa54e2d884f7cb1fb5dcb2d828ada29830ac4a7a09b04797e6e2fb448df476cbb31345841735e6cf4d5b7b1f6783781859778805fffef708f259fe780c6ba677
59f8593
to
e5c5e2f
Compare
LockTime
type to replace u32 in After
.
2f1ae91
to
2b4a71e
Compare
0bd872c
to
20e7463
Compare
019d0ff
to
93eca56
Compare
src/miniscript/astelem.rs
Outdated
@@ -622,7 +623,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> { | |||
.push_slice(&Pk::hash_to_hash160(hash)[..]) | |||
.push_opcode(opcodes::all::OP_EQUALVERIFY), | |||
Terminal::After(t) => builder | |||
.push_int(t as i64) | |||
.push_int(t.to_consensus_u32() as i64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
into()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of having long-ass (thus informative) to_consensus_u32
and then being able to just .into()
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i64::from()
is also a possibility. My point is to avoid as
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is maybe we should we should get rid of that Into
/From
if we don't want users to willy-nilly convert to raw integer types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're talking completely different types. Conversion from u32
to i64
is provided by core
and it's a completely obvious conversion preserving the value as the same number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get it now. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add, thanks.
@@ -757,7 +758,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> { | |||
match *self { | |||
Terminal::PkK(ref pk) => Ctx::pk_len(pk), | |||
Terminal::PkH(..) | Terminal::RawPkH(..) => 24, | |||
Terminal::After(n) => script_num_size(n as usize) + 1, | |||
Terminal::After(n) => script_num_size(n.to_consensus_u32() as usize) + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crate should have compile_error
for platforms that have pointers smaller than 32 bits, didn't check if that's the case. And even better have an internal utility function for the cast to improve safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/miniscript/types/mod.rs
Outdated
@@ -820,7 +822,7 @@ impl Property for Type { | |||
// Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The | |||
// number on the stack would be a 5 bytes signed integer but Miniscript's B type | |||
// only consumes 4 bytes from the stack. | |||
if t == 0 || (t & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 { | |||
if t.to_consensus_u32() == 0 || (t.to_consensus_u32() & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a reason to have is_zero()
method in LockTime
? Anyway, this looks nicer to me: t == LockTime::ZERO
Also WTF is sequence mixed with lock time here?! Looks very wrong to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this looks nicer to me:
t == LockTime::ZERO
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Its locktime::ZERO
not LockTime::ZERO
because associated consts don't exist for enums.)
Doing this led to adding pub use crate::blockdata::locktime::{self, LockTime};
in rust-bitcoin/src/lib.rs
to make imports in miniscript a bit more ergonomic use bitocin::{locktime, LockTime}
. Flagging that here because we do not export any of the other modules within blockdata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecause associated consts don't exist for enums
Did you miss my comment before demonstrating it's not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now includes LockTime::ZERO
:)
Policy::After(n) => { | ||
if n.to_consensus_u32() == 0 { | ||
Err(PolicyError::ZeroTime) | ||
} else if n.to_consensus_u32() > 2u32.pow(31) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look like 2u32.pow(32)
should be a constant. (MAX_TIME: LockTime
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong @sanket1729 or @apoelstra but I think this is a miniscript restriction not a locktime restriction. If that's the case I don't think it should be part of rust-bitcoin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be a constant in miniscript
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct @tcharding.
I would suggest putting the constant in rust-bitcoin as MAX_SCRIPTNUM_VALUE
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to rust-bitcoin/src/blockdata/constants.rs
.
// different as well as for any a/b that are the same except After/After. | ||
match (a, b) { | ||
(After(a), After(b)) => a.to_consensus_u32().cmp(&b.to_consensus_u32()), | ||
(a, b) => a.partial_cmp(b).unwrap_or(Ordering::Equal), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, per comment above mentioning canonical order it seems to make sense. I'd probably call the method canonically_sorted
to make it more visible.
However unwrap_or
doesn't seem right. None
means these two can't be ordered
. It'd be better to just list all variants and call cmp
on them. Yes it's annoying but will statically prevent problems. A macro can help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I thought the same originally but I don't believe we can do so, e.g.
(Trivial, Unsatisfiable) => a.cmp(&b),
cmp
is not implemented for Policy
(that's the whole thing we are trying to get around). Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the exact thing derive does: return Ordering::Greater
.
Thanks for the review @Kixunil, I believe you may now start to see some of the difficulty in doing this work :) Its not trivial to code up the |
93eca56
to
8e58db9
Compare
8e58db9
to
c98de73
Compare
0ed78e5 Add lock time types (Tobin C. Harding) 1390ee1 Add a max scriptnum constant (Tobin C. Harding) Pull request description: Implement a `LockTime` type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY. For example usage in `rust-miniscript` please see rust-bitcoin/rust-miniscript#408. ### Notes: I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad. This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them _both_ timelocks. If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :) Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too. ACKs for top commit: Kixunil: ACK 0ed78e5 apoelstra: ACK 0ed78e5 Tree-SHA512: 486fcce859b38fa1e8e6b11cd2f494462d6d7d1d9030d096ce6b260f6c9d0342b8952afe35152bdf3afe323a234a8165ac3d6c946304afcc13406d7a0489d75a
0ed78e5 Add lock time types (Tobin C. Harding) 1390ee1 Add a max scriptnum constant (Tobin C. Harding) Pull request description: Implement a `LockTime` type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY. For example usage in `rust-miniscript` please see rust-bitcoin/rust-miniscript#408. ### Notes: I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad. This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them _both_ timelocks. If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :) Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too. ACKs for top commit: Kixunil: ACK 0ed78e5 apoelstra: ACK 0ed78e5 Tree-SHA512: 486fcce859b38fa1e8e6b11cd2f494462d6d7d1d9030d096ce6b260f6c9d0342b8952afe35152bdf3afe323a234a8165ac3d6c946304afcc13406d7a0489d75a
|
This PR shows how we might use the new
LockTime
type fromrust-bitcoin
. (rust-bitcoin/rust-bitcoin#994)This PR is not intended to be a merge candidate.
Please note, this PR has changed a lot, comments below are mostly stale.
Currently all test pass when run locally, the bitcoin dependency has to be set in all the other dependencies (bitcoind, rust-bitcoincore-rpc). I only did this locally - you'll just have to believe me :)