Upgrade bitcoin/miniscript dependencies #1177

tcharding · 2023-10-13T01:16:37Z

Upgrade:

bitcoin to v0.31.0
miniscript to v11.0.0

notmandatory · 2023-10-16T14:48:05Z

Thanks for the headsup and trial patch. Any rough ETA for 0.31 ? Since this would be a breaking change we need to figure out if it could make it in to our 1.0-beta (this year 🤞) or safer to target for a 2.0 release next year.

tcharding · 2023-10-16T20:27:41Z

There isn't anything massive in this one, I wouldn't stress about getting it into 1.0-beta. You could even skip this one and jump straight to the next one if your 2.0 comes out after March/April.

tcharding · 2023-10-16T20:28:07Z

ETA for v0.31.0 would be in the next couple weeks I'd say.

realeinherjar · 2023-11-01T15:14:17Z

Closes #1191.

tcharding · 2023-11-01T20:53:48Z

rust-bitcoin release is out but we are working on miniscript still so not much help to you guys yet. Just mentioning in case you see it.

notmandatory · 2023-12-05T05:29:39Z

@tcharding I see that rust-miniscript 11.0 was released with rust-bitcoin/rust-miniscript#618, if the changes aren't too big can we get this into our 1.0-alpha.4 milestone?

tcharding · 2023-12-05T19:27:32Z

Deleted message, first notification of the morning and I totally misunderstood the comment. I'll fix this PR up.

tcharding · 2023-12-05T21:49:48Z

The upgrade is currently stuck on the rust-electrum-client, PR looks read to go just needs review please: bitcoindevkit/rust-electrum-client#121

I think I have all the other crate upgrades queued up. I can check them over and take of draft soon as rust-electrum-client is upgraded.

notmandatory · 2023-12-13T17:37:59Z

I'm moving this to up to the 1.0.0-alpha.4 release since rust-bitcoin 0.31.0 and rust-miniscript 11.0.0 are released. @tcharding has also created PRs to upgrade the other crates we depend on, we just need to get them merged and released.

tcharding · 2024-02-07T05:15:41Z

CI is expected to be red because I have paths sections in the manifest, to test locally you'll need to either use git wortkrees or update the paths.

tcharding · 2024-02-07T05:28:08Z

should be good to go

I rescind my claim :)

Has one test failure:

cargo test --all --all-features
...
failures:

---- test_bump_fee_add_input_change_dust stdout ----
thread 'test_bump_fee_add_input_change_dust' panicked at crates/bdk/tests/wallet.rs:2109:33:
called `Result::unwrap()` on an `Err` value: CoinSelection(InsufficientFunds { needed: 75159, available: 75000 })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_bump_fee_add_input_change_dust

ValuedMammal · 2024-03-06T22:46:49Z

cargo test --all --all-features
...
failures:

---- test_bump_fee_add_input_change_dust stdout ----
thread 'test_bump_fee_add_input_change_dust' panicked at crates/bdk/tests/wallet.rs:2109:33:
called `Result::unwrap()` on an `Err` value: CoinSelection(InsufficientFunds { needed: 75159, available: 75000 })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_bump_fee_add_input_change_dust

bdk/crates/bdk/tests/wallet.rs

Lines 2058 to 2064 in c01983d

    
           // We calculate the new weight as: 
        
           //   original weight 
        
           // + extra input weight: 160 WU = (32 (prevout) + 4 (vout) + 4 (nsequence)) * 4 
        
           // + input satisfaction weight: 112 WU = 106 (witness) + 2 (witness len) + (1 (script len)) * 4 
        
           // - change output weight: 124 WU = (8 (value) + 1 (script len) + 22 (script)) * 4 
        
           let new_tx_weight = 
        
               original_tx_weight + Weight::from_wu(160) + Weight::from_wu(112) - Weight::from_wu(124);

It appears the point of this test is to check that when bumping fee, a new input is added and one output is taken away when compared with the original tx.

We know what the expected fee is from the value of the ins/outs (50k + 25k - 45K), so why not just set the bumpfee using builder.fee_absolute() and leave the rest as is without bothering to play with weights. Any objections to this @tcharding ?

    let mut builder = wallet.build_fee_bump(txid).unwrap();
    // We set a fee high enough that during rbf we are forced to add
    // a new input and also that we have to remove the change
    // that we had previously
    // inputs: 50k + 25k
    // outputs: 45K
    let fee = 50_000 + 25_000 - 45_000;
    builder.fee_absolute(fee);
    let psbt = builder.finish().unwrap();

notmandatory · 2024-03-21T19:55:44Z

crates/esplora/Cargo.toml

@@ -13,16 +13,16 @@ readme = "README.md"

 [dependencies]
 bdk_chain = { path = "../chain", version = "0.9.0", default-features = false }
-esplora-client = { version = "0.6.0", default-features = false }
+esplora-client = { version = "0.7.0", default-features = false, path = "../../../../rust-esplora-client/02-07-upgrade-bitcoin-0.31" }


The rust-esplora-client 0.7.0 is now published on crates.io so this can be fixed.

Thanks @notmandatory, will do.

Upgrade: - bitcoin to v0.31.0 - miniscript to v11.0.0 Note: The bitcoin upgrade includes improvements to the `Transaction::weight()` function, it appears those guys did good, we no longer need to add the 2 additional weight units "just in case".

ValuedMammal

ACK 984c758

ValuedMammal · 2024-04-08T13:33:09Z

crates/bdk/src/wallet/signer.rs

-            )?,
-            sighash,
-        ))
+        Ok((sighash, sighash_type))


This looks better, thank you.

ValuedMammal · 2024-04-08T14:35:01Z

The only thing I'm confused about (on my end) is why I see this when trying to build locally

error[E0277]: the trait bound `bitcoin::bitcoin_hashes::sha256d::Hash: ThirtyTwoByteHash` is not satisfied
   --> crates/bdk/src/wallet/signer.rs:537:13
    |
533 |         sign_psbt_ecdsa(
    |         --------------- required by a bound introduced by this call
...
537 |             hash,
    |             ^^^^ the trait `ThirtyTwoByteHash` is not implemented for `bitcoin::bitcoin_hashes::sha256d::Hash`
    |
    = help: the following other types implement trait `ThirtyTwoByteHash`:
              bitcoin::secp256k1::bitcoin_hashes::sha256t::Hash<T>
              bitcoin::secp256k1::bitcoin_hashes::sha256::Hash
              bitcoin::secp256k1::bitcoin_hashes::sha256d::Hash
              bitcoin::LegacySighash
              bitcoin::SegwitV0Sighash
              bitcoin::TapSighash
note: required by a bound in `sign_psbt_ecdsa`

notmandatory · 2024-04-08T17:48:00Z

@notmandatory you mean the bug was hiding in the line of code with 9 lines of comments describing why it was there, I probably could have looked a bit harder huh. Thanks for finding it man. I included it in the same patch, if you think its needs pulling out separately I can re-do it.

Same patch is fine, thanks!

apoelstra · 2024-04-08T19:19:02Z

@ValuedMammal two things:

We are moving away from the ThirtyTwoByteHash in favor of just Into<Message>, since having a dedicated trait that crossed both rust-secp256k1 and bitcoin-hashes was making our upgrades more fragile.
We specifically dropped the ThirtyTwoByteHash bound on sha256d::Hash because as far as we were aware nobody was signing bare hashes like this. How are you obtaining this hash? The rust-bitcoin sighashing methods all return one of the bitcoin::*Sighash types.

notmandatory

ACK 984c758

Thanks for doing the update, this was a lot of work!

ValuedMammal · 2024-04-08T20:22:44Z

@apoelstra Thanks for adding the extra context.

BDK has a trait ComputeSighash where the associated type Sighash looks to be correct for each script context Legacy, Segwitv0, and Tap. I think part of the problem in this case is we're trying to call sign_psbt_ecdsa for both legacy and segwitv0 sighashes by first calling to_raw_hash.

bdk/crates/bdk/src/wallet/signer.rs

Lines 521 to 542 in 53791eb

    
           let (hash, hash_ty) = match self.ctx { 
        
               SignerContext::Segwitv0 => { 
        
                   let (h, t) = Segwitv0::sighash(psbt, input_index, ())?; 
        
                   let h = h.to_raw_hash(); 
        
                   (h, t) 
        
               } 
        
               SignerContext::Legacy => { 
        
                   let (h, t) = Legacy::sighash(psbt, input_index, ())?; 
        
                   let h = h.to_raw_hash(); 
        
                   (h, t) 
        
               } 
        
               _ => return Ok(()), // handled above 
        
           }; 
        
           sign_psbt_ecdsa( 
        
               &self.inner, 
        
               pubkey, 
        
               &mut psbt.inputs[input_index], 
        
               hash, 
        
               hash_ty, 
        
               secp, 
        
               sign_options.allow_grinding, 
        
           );

Instead of being fancy, I think if we just call sign_psbt_ecdsa individually for each case, then this won't be an issue.

    match self.ctx {
        SignerContext::Segwitv0 => {
            let (hash, hash_ty) = Segwitv0::sighash(psbt, input_index, ())?;
            sign_psbt_ecdsa(
                &self.inner,
                pubkey,
                &mut psbt.inputs[input_index],
                hash,
                hash_ty,
                secp,
                sign_options.allow_grinding,
            );
        }
        SignerContext::Legacy => {
            let (hash, hash_ty) = Legacy::sighash(psbt, input_index, ())?;
            sign_psbt_ecdsa(
                &self.inner,
                pubkey,
                &mut psbt.inputs[input_index],
                hash,
                hash_ty,
                secp,
                sign_options.allow_grinding,
            );
        }
        _ => return Ok(()), // handled above
    };

apoelstra · 2024-04-08T20:56:27Z

Instead of being fancy, I think if we just call sign_psbt_ecdsa individually for each case, then this won't be an issue.

Gotcha! Yep, this makes sense -- and I suspect that in future we may try to split out the two sign_psbt_ecdsas into separate methods. (Though I'm not sure; it may be that we let the pre-Taproot API be somewhat flexible/poorly defined and just focus on Taproot.)

tcharding · 2024-04-08T21:00:19Z

Note @apoelstra this is the 0.31 upgrade not the 0.32 upgrade (using secp 0.28 so ThirtyTwoByteHash should still be implemented on hashes types, we only just removed it in secp 0.29). I'm working on the 0.32-rc1 upgrade as well, its not done yet though.

apoelstra · 2024-04-08T22:30:36Z

@tcharding ah, then I suspect the cause of the error is actually rust-bitcoin/rust-secp256k1#673 ... which is why we are removing ThirtyTwoByteHash :)

tcharding · 2024-04-09T00:25:13Z

Instead of being fancy, I think if we just call sign_psbt_ecdsa individually for each case

The bdk signing is not using the PSBT signing API from rust-bitcoin, you guys are signing manually using secp (and at least as it is now you can't because rust-bitcoins PSBT signing API does not support grinding).

The only thing I'm confused about (on my end) is why I see this when trying to build locally

Can you check your lock file and see what version of hashes, secp, and bitcoin you are pulling in please - just to make sure its not broken at our end.

Thanks

tcharding · 2024-04-09T00:28:00Z

Just for the record, this PR does not attempt to use any of the improvements in the new rust-bitcoin version (assuming you see them as improvements). It just makes the code build. E.g., keeps standard integer types instead of using new ADT - Weight::to_wu and Weight::from_wu and uses sats Amount::to_sat and Amount::from_sat instead of Amount.

ValuedMammal · 2024-04-09T01:35:12Z

Can you check your lock file and see what version of hashes, secp, and bitcoin you are pulling in please - just to make sure its not broken at our end.

I'm glad you brought this up. While troubleshooting I did a cargo clean but never thought to remove the lock file, oops. That seems to have fixed the compile error.

Before resetting Cargo.lock

bitcoin 0.31.2, hashes 0.13
secp 0.28.2, hashes 0.12

After

bitcoin 0.31.2, hashes 0.13
secp 0.28.2, hashes 0.13

tcharding · 2024-04-09T02:21:36Z

Sweet, everything works then. Thanks

oleonardolima

ACK 984c758

oleonardolima · 2024-04-10T15:09:25Z

Closes #1381

evanlinjin · 2024-04-12T04:34:54Z

Unfortunately this does not compile when rebased on top of master. Should be an easy fix though.

error[E0277]: the trait bound `bitcoin::bitcoin_hashes::sha256d::Hash: ThirtyTwoByteHash` is not satisfied
   --> crates/bdk/src/wallet/signer.rs:537:13
    |
533 |         sign_psbt_ecdsa(
    |         --------------- required by a bound introduced by this call
...
537 |             hash,
    |             ^^^^ the trait `ThirtyTwoByteHash` is not implemented for `bitcoin::bitcoin_hashes::sha256d::Hash`
    |
    = help: the following other types implement trait `ThirtyTwoByteHash`:
              bitcoin::secp256k1::bitcoin_hashes::sha256t::Hash<T>
              bitcoin::secp256k1::bitcoin_hashes::sha256::Hash
              bitcoin::secp256k1::bitcoin_hashes::sha256d::Hash
              bitcoin::LegacySighash
              bitcoin::SegwitV0Sighash
              bitcoin::TapSighash
note: required by a bound in `sign_psbt_ecdsa`
   --> crates/bdk/src/wallet/signer.rs:551:40
    |
547 | fn sign_psbt_ecdsa(
    |    --------------- required by a bound in this function
...
551 |     hash: impl bitcoin::hashes::Hash + bitcoin::secp256k1::ThirtyTwoByteHash,
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `sign_ps
bt_ecdsa`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `bdk` (lib) due to previous error

notmandatory · 2024-04-12T05:32:48Z

The corresponding LDK issue is lightningdevkit/rust-lightning#2745.

notmandatory · 2024-04-12T15:46:34Z

Unfortunately this does not compile when rebased on top of master. Should be an easy fix though.

I just did a test-rebase (https://github.com/bitcoindevkit/bdk/pull/1405/checks) and CI builds fine for me. Maybe when you tried it you didn't do a cargo update?

storopoli

ACK 984c758

5f238d8 Bump bdk version to 1.0.0-alpha.9 (Steve Myers) Pull request description: ### Description Bump bdk version to 1.0.0-alpha.9 bdk_chain to 0.12.0 bdk_bitcoind_rpc to 0.8.0 bdk_electrum to 0.11.0 bdk_esplora to 0.11.0 bdk_file_store to 0.9.0 bdk_testenv to 0.2.0 fixes #1399 ### Notes to the reviewers I also removed unneeded version for bdk_testenv in dev-dependencies, see #1177 (comment). ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: evanlinjin: ACK 5f238d8 Tree-SHA512: 7ede94a6476a6b8c49a16b0aad147030eab23e26b9c4cadb1dd319fb8562ae325640f506f2d6dab0e85b0ee7f6955ac298ec1f70264704dc7f9a9492f8794f38

tcharding force-pushed the 10-13-upgrade-bitcoin branch from e67c6e8 to 52d19be Compare October 16, 2023 08:53

notmandatory assigned tcharding Oct 16, 2023

nondiremanuel mentioned this pull request Oct 22, 2023

W43 Lib Team Call bitcoindevkit/.github#83

Closed

33 tasks

nondiremanuel added this to the 2.0.0-alpha.0 milestone Oct 24, 2023

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

notmandatory mentioned this pull request Nov 27, 2023

Bump rust-bitcoin to version 0.31 lightningdevkit/rust-lightning#2745

Closed

notmandatory modified the milestones: 2.0.0-alpha.0, 1.0.0-alpha.4 Dec 13, 2023

nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024

notmandatory added the api A breaking API change label Jan 16, 2024

tcharding force-pushed the 10-13-upgrade-bitcoin branch from 52d19be to 6d4ea1c Compare February 7, 2024 05:13

tcharding force-pushed the 10-13-upgrade-bitcoin branch from 6d4ea1c to 6233210 Compare February 7, 2024 05:26

notmandatory added module-wallet dependencies Pull requests that update a dependency file labels Mar 16, 2024

notmandatory mentioned this pull request Mar 21, 2024

Update rust-esplora-client to 0.7.0 #1381

Closed

notmandatory reviewed Mar 21, 2024

View reviewed changes

tcharding force-pushed the 10-13-upgrade-bitcoin branch 4 times, most recently from a9199e4 to 1efe247 Compare April 8, 2024 05:12

Upgrade miniscript/bitcoin dependency

984c758

Upgrade: - bitcoin to v0.31.0 - miniscript to v11.0.0 Note: The bitcoin upgrade includes improvements to the `Transaction::weight()` function, it appears those guys did good, we no longer need to add the 2 additional weight units "just in case".

tcharding force-pushed the 10-13-upgrade-bitcoin branch from 1efe247 to 984c758 Compare April 8, 2024 05:16

ValuedMammal approved these changes Apr 8, 2024

View reviewed changes

crates/bdk/src/wallet/signer.rs

)?,

sighash,

))

Ok((sighash, sighash_type))

Copy link

Contributor

ValuedMammal Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better, thank you.

notmandatory approved these changes Apr 8, 2024

View reviewed changes

oleonardolima approved these changes Apr 10, 2024

View reviewed changes

notmandatory mentioned this pull request Apr 12, 2024

Bump bdk version to 1.0.0-alpha.9 #1404

Merged

3 tasks

storopoli approved these changes Apr 12, 2024

View reviewed changes

notmandatory merged commit 358e842 into bitcoindevkit:master Apr 12, 2024
12 checks passed

notmandatory mentioned this pull request Apr 12, 2024

Release 1.0.0-alpha.9 #1399

Closed

25 tasks

ValuedMammal mentioned this pull request May 3, 2024

refactor(wallet): Use Psbt::sighash_ecdsa for computing sighashes #1424

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade bitcoin/miniscript dependencies #1177

Upgrade bitcoin/miniscript dependencies #1177

tcharding commented Oct 13, 2023 •

edited

Loading

notmandatory commented Oct 16, 2023

tcharding commented Oct 16, 2023

tcharding commented Oct 16, 2023 •

edited

Loading

realeinherjar commented Nov 1, 2023

tcharding commented Nov 1, 2023

notmandatory commented Dec 5, 2023

tcharding commented Dec 5, 2023 •

edited

Loading

tcharding commented Dec 5, 2023 •

edited

Loading

notmandatory commented Dec 13, 2023 •

edited

Loading

tcharding commented Feb 7, 2024 •

edited

Loading

tcharding commented Feb 7, 2024 •

edited

Loading

ValuedMammal commented Mar 6, 2024 •

edited

Loading

notmandatory Mar 21, 2024

tcharding Mar 21, 2024

ValuedMammal left a comment

ValuedMammal Apr 8, 2024

ValuedMammal commented Apr 8, 2024

notmandatory commented Apr 8, 2024

apoelstra commented Apr 8, 2024

notmandatory left a comment

ValuedMammal commented Apr 8, 2024

apoelstra commented Apr 8, 2024

tcharding commented Apr 8, 2024

apoelstra commented Apr 8, 2024

tcharding commented Apr 9, 2024

tcharding commented Apr 9, 2024 •

edited

Loading

ValuedMammal commented Apr 9, 2024

tcharding commented Apr 9, 2024

oleonardolima left a comment

oleonardolima commented Apr 10, 2024

evanlinjin commented Apr 12, 2024 •

edited

Loading

notmandatory commented Apr 12, 2024

notmandatory commented Apr 12, 2024

storopoli left a comment

Upgrade bitcoin/miniscript dependencies #1177

Upgrade bitcoin/miniscript dependencies #1177

Conversation

tcharding commented Oct 13, 2023 • edited Loading

notmandatory commented Oct 16, 2023

tcharding commented Oct 16, 2023

tcharding commented Oct 16, 2023 • edited Loading

realeinherjar commented Nov 1, 2023

tcharding commented Nov 1, 2023

notmandatory commented Dec 5, 2023

tcharding commented Dec 5, 2023 • edited Loading

tcharding commented Dec 5, 2023 • edited Loading

notmandatory commented Dec 13, 2023 • edited Loading

tcharding commented Feb 7, 2024 • edited Loading

tcharding commented Feb 7, 2024 • edited Loading

ValuedMammal commented Mar 6, 2024 • edited Loading

notmandatory Mar 21, 2024

Choose a reason for hiding this comment

tcharding Mar 21, 2024

Choose a reason for hiding this comment

ValuedMammal left a comment

Choose a reason for hiding this comment

ValuedMammal Apr 8, 2024

Choose a reason for hiding this comment

ValuedMammal commented Apr 8, 2024

notmandatory commented Apr 8, 2024

apoelstra commented Apr 8, 2024

notmandatory left a comment

Choose a reason for hiding this comment

ValuedMammal commented Apr 8, 2024

apoelstra commented Apr 8, 2024

tcharding commented Apr 8, 2024

apoelstra commented Apr 8, 2024

tcharding commented Apr 9, 2024

tcharding commented Apr 9, 2024 • edited Loading

ValuedMammal commented Apr 9, 2024

tcharding commented Apr 9, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima commented Apr 10, 2024

evanlinjin commented Apr 12, 2024 • edited Loading

notmandatory commented Apr 12, 2024

notmandatory commented Apr 12, 2024

storopoli left a comment

Choose a reason for hiding this comment

tcharding commented Oct 13, 2023 •

edited

Loading

tcharding commented Oct 16, 2023 •

edited

Loading

tcharding commented Dec 5, 2023 •

edited

Loading

tcharding commented Dec 5, 2023 •

edited

Loading

notmandatory commented Dec 13, 2023 •

edited

Loading

tcharding commented Feb 7, 2024 •

edited

Loading

tcharding commented Feb 7, 2024 •

edited

Loading

ValuedMammal commented Mar 6, 2024 •

edited

Loading

tcharding commented Apr 9, 2024 •

edited

Loading

evanlinjin commented Apr 12, 2024 •

edited

Loading