update to bitcoin 0.30.0 by sanket1729 · Pull Request #537 · rust-bitcoin/rust-miniscript

sanket1729 · 2023-03-24T20:53:58Z

No description provided.

tcharding · 2023-03-27T22:26:28Z

PR title has typo mate, 10.0 should be 0.30.0, right?

tcharding · 2023-03-28T23:44:50Z

Hey @sanket1729, I pushed a branch of the same name to my tree with additional changes to get green in CI (I think). Feel free to grab them and squash it down into this. No need for attribution.

sanket1729 · 2023-03-28T23:50:45Z

Thanks, will take a look shortly. Waiting for bitcoind release before making any progress here.

apoelstra · 2023-04-03T15:57:54Z

@sanket1729 bitcoind is blocked on rust-bitcoincore-rpc which is blocked on Steven's availability. Can't we figure out some other solution?

sanket1729 · 2023-04-03T17:29:24Z

@apoelstra, is this not already updated here? rust-bitcoin/rust-bitcoincore-rpc@6746011

sanket1729 · 2023-04-03T17:29:57Z

Perhaps, you don't have publish priviledges?

apoelstra · 2023-04-03T17:33:05Z

@sanket1729 correct, I do not have publish privileges.

romanz · 2023-04-21T13:32:42Z

Thanks, will take a look shortly. Waiting for bitcoind release before making any progress here.

bitcoind 0.30 is now released :)

sanket1729 · 2023-05-02T20:26:54Z

This is also ready for review @apoelstra

apoelstra · 2023-05-02T22:52:01Z

Looks good. I think we need to revisit the assume_unchecked address crap in bitcoin. I'd also like to iterate a bit on the src/util.rs stuff (which has a FIXME and some allocations that shouldn't be necessary) but we can do that in a followup since this PR is so massive and these things are a small part of it.

apoelstra · 2023-05-02T23:07:22Z

There are also several clippy failures on this, but I am also willing to slip those to a followup PR.

tcharding

A few minor suggestions, I mainly mention them because I think people will look at miniscript to see how to best use rust-bitcoin so we should use it in the best/easiest manner.

Thanks!

tcharding · 2023-05-02T22:34:44Z

Cargo.toml

-bitcoin = { version = "0.29.1", default-features = false }
+bitcoin = { version = "0.30.0", default-features = false }
 hashbrown = { version = "0.11", optional = true }
+bitcoin-private = { version = "0.1.0", default-features = false }


In rust-bitcoin we found it more ergonomic to write use internals::write_err. To do so set the dependency as such

internals = { package = "bitcoin-private", version = "0.1.0" }

tcharding · 2023-05-02T22:38:24Z

bitcoind-tests/tests/test_cpp.rs

 use std::path::Path;

 use bitcoin::hashes::{sha256d, Hash};
+use bitcoin::psbt::PartiallySignedTransaction as Psbt;


Suggested change

use bitcoin::psbt::PartiallySignedTransaction as Psbt;

use bitcoin::psbt::Psbt;

tcharding · 2023-05-02T22:41:02Z

bitcoind-tests/tests/test_cpp.rs

-use bitcoin::{Amount, LockTime, OutPoint, Sequence, Transaction, TxIn, TxOut, Txid};
+use bitcoin::{psbt, Amount, OutPoint, Sequence, Transaction, TxIn, TxOut, Txid};
 use bitcoind::bitcoincore_rpc::{json, Client, RpcApi};
+use miniscript::bitcoin::absolute::LockTime;


Perhaps import absolute and use absolute::LockTime

Suggested change

use miniscript::bitcoin::absolute::LockTime;

use miniscript::bitcoin::absolute;

tcharding · 2023-05-02T22:41:42Z

bitcoind-tests/tests/test_desc.rs

 use std::{error, fmt};

 use actual_rand as rand;
+use bitcoin::absolute::LockTime;


Same comment here.

tcharding · 2023-05-02T22:41:54Z

bitcoind-tests/tests/test_desc.rs

-use bitcoin::util::sighash::SighashCache;
-use bitcoin::util::taproot::{LeafVersion, TapLeafHash};
-use bitcoin::util::{psbt, sighash};
+use bitcoin::psbt::PartiallySignedTransaction as Psbt;


Same comment here.

tcharding · 2023-05-02T23:01:07Z

src/lib.rs

+}
+
+/// An absolute locktime that implements `Ord`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]


You might want to use

#[allow(clippy::derive_ord_xor_partial_ord)]

Perhaps with a comment (currently missing from rust-bitcoin) that its ok to derive eq and implement ord because the enum variants hold the same u32 as returned by to_u32() so the impls match as required.

Have we added clippy in CI in rust-bitcoin? Perhaps, that gives a green light to add it here too

Yep sure have, its in contrib/test.sh. Done for bitcoin, hashes, secp256k1, internals, and bech32.

Note, you have to lint the examples explicitly (see rust-bitcon/bitcoin/contrib/test.sh)

tcharding · 2023-05-02T23:43:23Z

src/util.rs

+            // FIXME: There has to be a better way than this.
+            let push = PushBytesBuf::try_from(wit.clone()).expect("FIXME: Handle error");
+            b = b.push_slice(&push)


Hey @Kixunil can you take a look at this please. I cannot work out how to get something that implements AsRef<PushBytes> from a &Vec<u8> to pass to push_slice? Only using a reference and checking that the vec is a valid length before passing it to push_slice is exactly what PushBytes is for right?

Firstly, I don't understand why this function even exists - witnesses and scripts are different.

The only way is to return Option or Result from the function or document that it panics if a witness with too large element is passed in.

Also to avoid allocation:

let push: &PushBytes = wit.try_into()?; // or expect b = b.push_slice(&push);

This is an internal to how rust-miniscript works. We have a function that constructs a Vec<Vec<u8>> type for all types of scripts. If the script is legacy p2sh or bare script pubkey, we need to convert this to ScriptBuf converting each Vec<u8> into a scriptPush.

The only way is to return Option or Result from the function or document that it panics if a witness with too large element is passed in.

This is one of the selling points of miniscript. Miniscripts are statically analyzed to make sure that all valid spend paths constructed at spend time are within bounds and spendable on bitcoin network. In other words, this function is only called on satisfactions generated from miniscript satisfier which guarantees the resource bounds enforced by bitcoin consensus rules.

It does context based checks that total witness length cannot be more than 3600 for segwitv0 and so on.

What KixUnil is saying is correct.

The allocation should be avoided. I don't remember how I did this :( . This was a month gap when I relooked at this PR. I only made sure the compiler did not complain and opened the PR for review.

Would it make sense to pass in something like &[WitnessElement] where WitnessElement is a type that guarantees length at most 3600? You could then safely implement AsRef<PushBytes> for it. Or if it's not that important for miniscript to carry the precise bound just use &[PushBytesBuf].

Firstly, I don't understand why this function even exists - witnesses and scripts are different.

Prior to segwit they are not.

Prior to segwit witness doesn't exist.

We could use &[PushBytesBuf], since all witnesses we push are either signatures, hash preimages, or constants like 1 or 0.

But for the final push, we have to push the script itself. Which is almost 3600 in segwit and implicit block size bound in transaction.

apoelstra

ACK 60943aa

sanket1729 · 2023-05-03T18:00:26Z

Re-reviewed the PR again. And force pushed removing all PushBytesBuf related allocations.

apoelstra

ACK d5a564c

Kixunil · 2023-05-03T19:24:09Z

src/util.rs

+
+    /// All pushes in miniscript script construction will never exceed 2**32 bytes.
+    /// We only push slice of hashes outputs (32 bytes), and keys (33/65 bytes).
+    fn push_ms_slice(self, data: &[u8]) -> Self;


I don't see this being called anywhere.

Good catch. This is a remanent of previous design where I thought I could hide all unwraps\expects for miniscript library. But later on decided to have explicit unwraps at call site to avoid future errors.

tcharding

ACK d5a564c

Can re-ack after push_ms_slice is removed but feel free to merge without me :)

apoelstra · 2023-05-03T22:42:30Z

I think we should remove push_ms_slice. I'm also happy to re-ack quickly.

apoelstra · 2023-05-04T13:14:50Z

On further thought let's just merge this and delete the funciton in a followup.

sanket1729 · 2023-05-04T14:43:06Z

Force pushed. I wonder why it did not show unused warning?

sanket1729 · 2023-05-04T14:43:28Z

The trait is pub(crate).

apoelstra

ACK 1c0c78e

Kixunil · 2023-05-04T18:04:48Z

I wonder why it did not show unused warning?

I think the analysis is harder for traits. Especially if it's used as dyn anywhere.

1c0c78e update to bitcoin 0.30.0 (Tobin C. Harding) Pull request description: ACKs for top commit: apoelstra: ACK 1c0c78e Tree-SHA512: 4ab6d41200a030526107cc9a5c1c7451483b1e0d9bcc813cb19f8336a5289ba6197dfc049dc0c3426afd3ddbf85dc055469213c329a3fc71a30238cab909fe96

1c0c78e94563fbd12845547e71d438b8834f288d update to bitcoin 0.30.0 (Tobin C. Harding) Pull request description: ACKs for top commit: apoelstra: ACK 1c0c78e94563fbd12845547e71d438b8834f288d Tree-SHA512: 4ab6d41200a030526107cc9a5c1c7451483b1e0d9bcc813cb19f8336a5289ba6197dfc049dc0c3426afd3ddbf85dc055469213c329a3fc71a30238cab909fe96

sanket1729 changed the title ~~update to bitcoin 10.0~~ update to bitcoin 0.30.0 Mar 28, 2023

apoelstra mentioned this pull request May 2, 2023

9.0.1 does not seem to work with bitcoin 0.30 #542

Closed

sanket1729 force-pushed the release_10.0.0 branch 2 times, most recently from 956d332 to ca305c8 Compare May 2, 2023 19:47

sanket1729 marked this pull request as ready for review May 2, 2023 19:48

sanket1729 force-pushed the release_10.0.0 branch from ca305c8 to 60943aa Compare May 2, 2023 20:08

tcharding reviewed May 2, 2023

View reviewed changes

apoelstra previously approved these changes May 2, 2023

View reviewed changes

sanket1729 dismissed apoelstra’s stale review via a700935 May 3, 2023 17:26

sanket1729 force-pushed the release_10.0.0 branch 2 times, most recently from a700935 to d5a564c Compare May 3, 2023 17:57

sanket1729 mentioned this pull request May 3, 2023

update TranslatePk again #493

Merged

apoelstra previously approved these changes May 3, 2023

View reviewed changes

Kixunil reviewed May 3, 2023

View reviewed changes

tcharding previously approved these changes May 3, 2023

View reviewed changes

update to bitcoin 0.30.0

1c0c78e

sanket1729 dismissed stale reviews from tcharding and apoelstra via 1c0c78e May 4, 2023 14:42

sanket1729 force-pushed the release_10.0.0 branch from d5a564c to 1c0c78e Compare May 4, 2023 14:42

apoelstra approved these changes May 4, 2023

View reviewed changes

sanket1729 merged commit 7c28bd3 into rust-bitcoin:master May 4, 2023

dr-orlovsky mentioned this pull request May 23, 2023

Release v10 with rust-bitcoin v0.30 support #550

Closed

	use bitcoin::psbt::PartiallySignedTransaction as Psbt;
	use bitcoin::psbt::Psbt;

	use miniscript::bitcoin::absolute::LockTime;
	use miniscript::bitcoin::absolute;

Conversation

sanket1729 commented Mar 24, 2023

Uh oh!

tcharding commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Mar 28, 2023

Uh oh!

apoelstra commented Apr 3, 2023

Uh oh!

sanket1729 commented Apr 3, 2023

Uh oh!

sanket1729 commented Apr 3, 2023

Uh oh!

apoelstra commented Apr 3, 2023

Uh oh!

romanz commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented May 2, 2023

Uh oh!

apoelstra commented May 2, 2023

Uh oh!

apoelstra commented May 2, 2023

Uh oh!

tcharding 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!

sanket1729 May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sanket1729 May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented May 3, 2023

Uh oh!

apoelstra 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!

tcharding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tcharding commented Mar 27, 2023 •

edited

Loading

tcharding commented Mar 28, 2023 •

edited

Loading

romanz commented Apr 21, 2023 •

edited

Loading

sanket1729 May 3, 2023 •

edited

Loading

sanket1729 May 3, 2023 •

edited

Loading

tcharding left a comment •

edited

Loading