Upgrade bitcoin dependency to v0.29.0 #450

tcharding · 2022-07-25T06:11:39Z

This is going to need a pretty careful review by @sanket1729 and/or @apoelstra to double check all the sequence/lock_time stuff. Even with the new APIs this stuff is non-trivial. This patch changes the current behaviour around logic for these two types. I am unable to work out if current behaviour has bugs in it or if I just don't fully understand it.

Note

Please ignore the branch name, it stale now since this PR upgrades to the newly released bitcoin v0.29.1

Kixunil

I found a few mistakes. Disclaimer: didn't do super-deep review.

Kixunil · 2022-07-28T13:51:48Z

src/descriptor/tr.rs

@@ -257,6 +257,7 @@ impl<Pk: MiniscriptKey> Tr<Pk> {
                    TaprootBuilderError::EmptyTree => {
                        unreachable!("Taptree is a well formed tree with atleast 1 element")
                    }
+                    _ => unreachable!("non_exhaustive catchall"),


This is actually wrong, a new variant could be added for any reason. To correctly handle the error it needs to be returned.

If this is too much hassle maybe we should improve the API or perhaps commit to never changing TaprootBuilderError and remove #[non_exhaustive]

This whole error match is strange, every error variant has an unreachable!. To be honest I was mainly just interested in checking that rust-bitcoin worked so I just hacked this up to make it build.

They have explanation why it should be OK, so I think it's fine. But it makes me question the whole API. Maybe we should've used typestate instead?

I will make a cleanup PR to upstream to cleanup this API

Opened rust-bitcoin/rust-bitcoin#1151

This is fixed now, please check my use of a single unreachable statement @sanket1729

src/interpreter/error.rs

Kixunil · 2022-07-28T13:55:22Z

src/interpreter/mod.rs

    },
    ///Absolute Timelock for CLTV.
    AbsoluteTimelock {
        /// The value of Absolute timelock
-        time: u32,
+        n: LockTime,


time was definitely a better name than n.

Its a block height or a block time. n is from n OP_CLTV and nLockTime - that's why I used n.

n in nLockTime literally means number. It's meaningless. I do see that time is confusing, so lock_time would be better because most people already know it can be height.

Used lock_time for this field and similarly sequence for SatisfiedConstraint::RelativeTimelock field.

Kixunil · 2022-07-28T13:57:54Z

src/miniscript/hash256.rs

+    fn index(&self, index: I) -> &Self::Output {
+        &self.0[index]
+    }
+}


Doesn't Hash impl Deref? if it does this shouldn't be needed at all.

I added this because of the trait bounds on Hash trait (from bitcoin_consensus). Deref doesn't help with that, right? Or am I missing something?

I don't see any Hash trait in bitcoin_consensus, I guess you mean bitcoin_hashes. You're right that Deref doesn't help but it's a question if that bound should be there since it's quite burdensome. I'd have to look into it deeper. The first check is will anything break if we remove it?

I'll investigate this, cheers.

Please see: rust-bitcoin/bitcoin_hashes#168

Kixunil · 2022-07-28T14:05:36Z

src/miniscript/types/mod.rs

@@ -820,16 +821,16 @@ 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 == PackedLockTime::ZERO {


Did the code originally attempt to check if lock times are actually enabled or something?

Originally both sequnece and locktime (After and Older) where handled together. When I split them apart I kept the disable check even though it seemed wrong, finally I got the confidence to remove it :)

It's not just after vs older -- the original code would look at a u32 which might contain a value that didn't encode any locktime at all.

It's not clear to me that this check can be removed.

No worries, since this PR is not a merge candidate this bridge doesn't need crossing now. I imagine between @apoelstra and @sanket1729 this will have to be worked out when miniscript upgrades to use rust-bitcoin 0.29.0.

Flagging this thread, this will need resolving before we can consider upgrading to the newly release version of rust-bitcoin. Sorry to be a drag but I think this is kind of subtle and will need input by @sanket1729 and/or @apoelstra to work it out. I can code up the solution.

I feel confident that removing this check is correct. This was just checking something else entirely which was wrong.

This was checking a check on sequence values in script which has no constraints. The only constraint we have is on transaction's sequence, which is different from checking the number encoded in script.

@apoelstra are you convinced that this is resolved?

Let me re-read this PR.

tcharding · 2022-07-28T21:50:50Z

Thanks for the review @Kixunil!

870ad59 Rename is_finalized to is_finalizable (sanket1729) aaadd25 Add breaking test that allowed incomplete builders to be created (sanket1729) 0b88051 Update TaprootBuilder::finalize (sanket1729) 5813ec7 Return EmptyTree instead of OverCompleteTree when there are no scripts to add (sanket1729) Pull request description: Found while reviewing rust-bitcoin/rust-miniscript#450 . There is also a BUG fix in the second commit that would have let users spendinfo from incomplete trees. The bug was introduced in #936 which I am responsible for ACKing ACKs for top commit: apoelstra: ACK 870ad59 Kixunil: ACK 870ad59 tcharding: ACK 870ad59 Tree-SHA512: 61442bd95df6912865cbecdc207f330b241e7fbb88b5e915243b2b48a756bea9eb29cb28d8c8249647a0a2ac514df4737bddab695f63075bd55284be5be228ff

110b5d8 Bump version to v0.29.0 (Tobin C. Harding) Pull request description: Add changelog notes and bump the version number to v0.29.0. ## Notes I attempted to go through all the PRs since last release, please sing out if you had a PR merged that is not mentioned and you would like it mentioned. The changelog notes can be changed or improved, please do not take me writing them to imply I know exactly what goes on round here - I just made an effort to save others having to do it. ## TODO - [x] merge all 'required' PRs - #1131 - #1137 - #1129 - #1151 - #1165 (add release notes still) - [x] Ensure all green from the CI run on: rust-bitcoin/rust-miniscript#450 - [ ] Carry out (and improve) the #1106 ACKs for top commit: tcharding: ACK 110b5d8 Kixunil: ACK 110b5d8 apoelstra: ACK 110b5d8 sanket1729: reACK 110b5d8 Tree-SHA512: d00c70534476794c01cd694ea9a23affef947c4f913b14344405272bc99cc00763f1ac755cc677e7afbd3dbef573d786251c9093d5dbafd76ee0cf86ca3b0ebd

sanket1729

Mostly ACK. did a round of careful review. Just waiting on reply for comparison FIXME comment

src/interpreter/mod.rs

sanket1729 · 2022-08-11T00:30:25Z

src/miniscript/astelem.rs

@@ -620,9 +621,11 @@ 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.0.into())


nit(here and elsewhere): slightly prefer to use t.to_u32() over t.0

I think I changed them all.

sanket1729 · 2022-08-11T00:55:01Z

src/miniscript/types/mod.rs

@@ -437,16 +438,16 @@ pub trait Property: Sized {
                // 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 == PackedLockTime::ZERO {


Just making sure that this was a bug that was fixed here

I believe so but that belief is based on the meaningless-ness of comparing a LockTime to the disable bit of sequence.

I'm still at a loss as to exactly what the comment means. It should be removed now though, right?

sanket1729 · 2022-08-11T01:04:08Z

src/miniscript/types/mod.rs

@@ -820,16 +821,16 @@ 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 == PackedLockTime::ZERO {


I feel confident that removing this check is correct. This was just checking something else entirely which was wrong.

This was checking a check on sequence values in script which has no constraints. The only constraint we have is on transaction's sequence, which is different from checking the number encoded in script.

sanket1729 · 2022-08-11T01:10:44Z

src/policy/semantic.rs

        self = match self {
            Policy::After(t) => {
-                if !timelock::absolute_timelocks_are_same_unit(t, n) {
+                // FIXME: If the point of the LockTime type is to make it hard to make mistakes then
+                // this code shows it fails at that since we _must_ check is_same_unit before comparing.


Indeed, sorry I was absent during the review of the initial code. I get it that there was some hesitance in implementing PartialOrd, but why not have a method that returns Optioncmp::Ordering?

We should really not need to check that they are same units here.

PartialOrd gives us a method that returns Option<cmp::Ordering>. It's Ord that we couldn't implement, because there is no natural total ordering on timelocks, but which we wanted to implement so that you could use Vec::sort and derive Ord on structures that contain timelocks.

I think @sanket1729 means that PartialOrd still allows < and > operators which are foot guns. Maybe this should be a clippy lint.

src/psbt/mod.rs

tcharding · 2022-08-11T19:31:07Z

Thanks for the review @sanket1729, I'll get back to this soonish, no rush because AFAIU there are a few other crates to upgrade first. Feel free to take this work and use it if you don't want to wait for me.

LLFourn · 2022-08-14T07:52:05Z

src/miniscript/satisfy.rs

+        if self.is_height_locked() && n.is_height_locked()
+            || self.is_time_locked() && n.is_time_locked()
+        {
+            n.to_consensus_u32() <= self.to_consensus_u32()


The previous implementation masked the sequence before doing the comparison. How can we justify not doing this here? What if one has the RBF disabled and the other doesn't for example (disabling RBF will always be the highest value unless you mask out that bit).

PS methods on Sequence like is_satisfied_by_height and is_satisfied_by_time would be super useful here.

How can we justify not doing this here?

Thanks man, you are 100% correct, this code is not the same as the code it replaces - that's my bug.

PS methods on Sequence like is_satisfied_by_height and is_satisfied_by_time would be super useful here.

A relative lock time type is meant to be in the works (not sure if anyone is actively working on it yet) and that type would have these methods you suggest. When Sequence was added relative lock time methods were explicitly out of scope.

I've reverted these changes, so we now have

// We need a relative lock time type in rust-bitcoin to clean this up. /* If nSequence encodes a relative lock-time, this mask is * applied to extract that lock-time from the sequence field. */ const SEQUENCE_LOCKTIME_MASK: u32 = 0x0000ffff; const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 0x00400000; let mask = SEQUENCE_LOCKTIME_MASK | SEQUENCE_LOCKTIME_TYPE_FLAG; let masked_n = n.to_consensus_u32() & mask; let masked_seq = self.to_consensus_u32() & mask; if masked_n < SEQUENCE_LOCKTIME_TYPE_FLAG && masked_seq >= SEQUENCE_LOCKTIME_TYPE_FLAG { false } else { masked_n <= masked_seq }

Please see #455 which implements this using a new relative::LockTime API.

LLFourn · 2022-08-16T01:06:22Z

src/miniscript/satisfy.rs

@@ -109,36 +106,35 @@ pub trait Satisfier<Pk: MiniscriptKey + ToPublicKey> {
    }

    /// Assert whether an relative locktime is satisfied
-    fn check_older(&self, _: u32) -> bool {
+    fn check_older(&self, _: Sequence) -> bool {


If you introduce a relative locktime type then it should probably be passed in here instead of Sequence.

Thanks man, I've done that now in the demo PR.

sanket1729 · 2022-09-01T00:54:40Z

I don't think we should wait for relative lock time PR in bitcoin 0.30.0. We would need a version of rust-miniscript that supports bitcoin 0.29.0

sanket1729 · 2022-09-01T22:28:12Z

src/policy/semantic.rs

                    Policy::Unsatisfiable
-                } else if t > n {
+                } else if t.to_consensus_u32() > n.to_consensus_u32() {


This is okay, but I would have expected an API like

let cmp_res = t.is_greater_than(n) ; // Any other name. match(cmp_res) { Some(true), Some(false), None, }

Rust miniscript/or any other downstream project should need to know that they should check !t.is_same_unit() before comparing.

Perhaps, something worth looking at 0.30.0

We could add a method is_satisfied_by_lock in the same manner as relative::LockTime.

Which would give us the much nicer:

Policy::After(t) => { let t = LockTime::from(t); } else if !t.is_satisfied_by_lock(n) { Policy::Unsatisfiable } else { Policy::After(t.into()) } }

rust-bitcoin/rust-bitcoin#1258

Face palm, we actually provide a patter for this in the rustdocs (double face palm, I even wrote the rustdocs and still forgot to use the correct pattern)

I've updated the PR to be

Policy::After(t) => { let t = LockTime::from(t); let is_satisfied_by = match (t, n) { (Blocks(t), Blocks(n)) => t <= n, (Seconds(t), Seconds(n)) => t <= n, _ => false }; if !is_satisfied_by { Policy::Unsatisfiable } else { Policy::After(t.into()) } }

tcharding · 2022-09-11T22:47:31Z

Hey @sanket1729, seems @josediegorobles is pretty keen to use the upcoming release, is there anything else that needs doing before we can consider this PR and get a release out?

Cargo already uses an env var to configure the toolchain `RUSTUP_TOOLCHAIN`, there is no need for our custom `TOOLCHAIN` environment variable.

A recent release of `form_urlencoded` breaks our dev build when using 1.41.1 toolchain. Pin the `url` and `form_urlencoded` dependencies when on 1.41.1 toolchain.

Depend on the newly released bitcoin 0.29.0

sanket1729 · 2022-09-14T09:06:12Z

Hi @tcharding, there is nothing that needs to be done in particular here. I have a few cleanups in mind, but those can wait for 8.1.0 . I will look into preparing a new release this week.

josediegorobles · 2022-09-14T09:15:46Z

Hi, @sanket1729 by suggestion of @tcharding i started a PR for the release: tcharding#1

What version must be, 7.1.0 or 8.0.0? And what other things must be on the changelog?

sanket1729

ACK 24a2498. Reviewed by range-diff from c51595b .

I am waiting on @apoelstra to respond to the comment issue he raised. #450 (comment)

apoelstra · 2022-09-19T21:53:40Z

src/policy/semantic.rs

            }),
            ("older", 1) => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x).map(Policy::Older)
+                expression::parse_num(x).map(|x| Policy::older(x))


nit: there was no need for the |x| here

apoelstra

ACK 24a2498

apoelstra · 2022-09-20T13:34:55Z

@sanket1729 in future before merging can you edit PR descriptions to remove the @ signs before people's names? Otherwise github creates spurious notifications for them.

sanket1729 · 2022-09-20T14:07:39Z

@apoelstra, will take care from next time

tcharding · 2022-09-20T22:27:15Z

Should I just not put mentions in the PR description? I thought it was only in the commit log (on the actual patch) that mattered?

apoelstra · 2022-09-21T00:02:06Z

@tcharding no, it matters both on the commits and in the PR description. (Though the PR description is really easy to edit, for anyone with merge permissions, while the commits are a pain.)

tcharding · 2022-09-21T05:39:45Z

My bad, duly schooled :)

Kixunil · 2022-09-21T06:55:33Z

@apoelstra are you sure? I was never spuriously notified from PRs. Creation doesn't count as spurious - I consider mentioning explicitly wanting to notify someone.

apoelstra · 2022-09-21T12:50:01Z

@Kixunil yes, though I believe the reason is that our merge script copies the entire PR description into the merge commit message.

Kixunil · 2022-09-21T15:23:12Z

Ah, good point! Could the merge script be modified to replace ([:whitespace:])@([a-zA-Z0-9_-]+[:whitespace]]) with \1\2 in PR description?

apoelstra · 2022-09-21T16:52:49Z

I don't know -- could you open an issue on https://github.com/bitcoin-core/bitcoin-maintainer-tools/ ?

Note that it does currently display an angry warning if there are @s in the PR description, which I always respond to by just editing the PR description and trying again ... maybe they don't want to modify PRs by default but it'd be nice if there was at least a gitconfig flag or something to do it.

3f451db Release 8.0.0 (sanket1729) Pull request description: Draft release PR. Still blocked on reviews of 1) #418 2) #450 3) #461 ACKs for top commit: apoelstra: ACK 3f451db Tree-SHA512: aa455fd57a894b1edb49b6273126a05c9d0897ca50ee572fa8a59b4d2a96e2013f2ed9ec4b96cd5e449ba5b5ee6bd32aa57da110df69bd35b8a970ccb1440002

…022_10 cddc22d Merge rust-bitcoin/rust-miniscript#450: Upgrade bitcoin dependency to v0.29.0 (Change satisfier locktime)

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from a373def to 084bb97 Compare July 27, 2022 01:10

tcharding mentioned this pull request Jul 27, 2022

Release tracking PR - v0.29.0 rust-bitcoin/rust-bitcoin#1109

Merged

3 tasks

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from 084bb97 to c525f53 Compare July 27, 2022 23:15

tcharding changed the title ~~Depend on "possible" bitcoin 0.29.0~~ Depend on bitcoin 0.29.0 release tracking PR Jul 27, 2022

Kixunil suggested changes Jul 28, 2022

View reviewed changes

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from c525f53 to 06ae8b7 Compare July 28, 2022 21:50

sanket1729 mentioned this pull request Jul 29, 2022

Update finalize API rust-bitcoin/rust-bitcoin#1151

Merged

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from 06ae8b7 to d73f5e3 Compare August 1, 2022 05:01

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from d73f5e3 to 05c5637 Compare August 9, 2022 00:17

tcharding changed the title ~~Depend on bitcoin 0.29.0 release tracking PR~~ Upgrade bitcoin dependency to v0.29.0 Aug 9, 2022

tcharding mentioned this pull request Aug 9, 2022

Remove Index trait bound from Hash rust-bitcoin/bitcoin_hashes#168

Closed

sanket1729 reviewed Aug 11, 2022

View reviewed changes

LLFourn reviewed Aug 14, 2022

View reviewed changes

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from 05c5637 to 4a0a87b Compare August 16, 2022 00:11

LLFourn reviewed Aug 16, 2022

View reviewed changes

notmandatory mentioned this pull request Aug 26, 2022

Problem at use bdk 0.19 with wasm bitcoindevkit/bdk#646

Closed

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch 2 times, most recently from be5a7bc to 478ff7f Compare August 29, 2022 06:06

dpc mentioned this pull request Aug 29, 2022

Bump rand to 0.8 (and everything that touches it) fedimint/fedimint#389

Merged

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch 2 times, most recently from fd48ef3 to c51595b Compare September 1, 2022 02:12

tcharding marked this pull request as ready for review September 1, 2022 02:56

sanket1729 reviewed Sep 1, 2022

View reviewed changes

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from c51595b to 882d791 Compare September 11, 2022 22:44

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from 882d791 to 363b438 Compare September 11, 2022 23:30

tcharding added 3 commits September 12, 2022 10:28

Remove TOOLCHAIN

15b57cd

Cargo already uses an env var to configure the toolchain `RUSTUP_TOOLCHAIN`, there is no need for our custom `TOOLCHAIN` environment variable.

Pin dependencies when using MSRV

c6d89cd

A recent release of `form_urlencoded` breaks our dev build when using 1.41.1 toolchain. Pin the `url` and `form_urlencoded` dependencies when on 1.41.1 toolchain.

Depend on bitcoin 0.29.0

24a2498

Depend on the newly released bitcoin 0.29.0

tcharding force-pushed the pre-release-bitcoin-0.29.0 branch from 363b438 to 24a2498 Compare September 12, 2022 00:38

josediegorobles mentioned this pull request Sep 12, 2022

New release with rust-bitcoin 0.29 #452

Closed

josediegorobles mentioned this pull request Sep 15, 2022

Release 7.1.0 tcharding/rust-miniscript#1

Open

sanket1729 approved these changes Sep 19, 2022

View reviewed changes

sanket1729 added this to the 8.0.0 milestone Sep 19, 2022

sanket1729 mentioned this pull request Sep 19, 2022

Release 8.0.0 #462

Merged

apoelstra reviewed Sep 19, 2022

View reviewed changes

apoelstra approved these changes Sep 19, 2022

View reviewed changes

sanket1729 merged commit cddc22d into rust-bitcoin:master Sep 20, 2022

NicolaLS mentioned this pull request Sep 20, 2022

Dependency version tracking issue fedimint/fedimint#577

Closed

Kixunil mentioned this pull request Sep 22, 2022

Automatically remove @ symbols? bitcoin-core/bitcoin-maintainer-tools#139

Open

notmandatory mentioned this pull request Oct 19, 2022

Support for sortedmulti_a #320

Open

Upgrade bitcoin dependency to v0.29.0 #450

Upgrade bitcoin dependency to v0.29.0 #450

Conversation

tcharding commented Jul 25, 2022 • edited Loading

Note

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jul 28, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Sep 11, 2022

sanket1729 commented Sep 14, 2022

josediegorobles commented Sep 14, 2022

sanket1729 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 20, 2022

sanket1729 commented Sep 20, 2022

tcharding commented Sep 20, 2022

apoelstra commented Sep 21, 2022

tcharding commented Sep 21, 2022

Kixunil commented Sep 21, 2022

apoelstra commented Sep 21, 2022

Kixunil commented Sep 21, 2022

apoelstra commented Sep 21, 2022

tcharding commented Jul 25, 2022 •

edited

Loading

sanket1729 Jul 29, 2022 •

edited

Loading

tcharding Aug 29, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading