Upgrade bitcoin/miniscript dependencies

[tcharding]

Upgrade:

• bitcoin to v0.31.0
• miniscript to v11.0.0

Draft still because requires currently unreleased:

• rust-hwi: master branch
• rust-esplora-clinet: https://github.com/bitcoindevkit/rust-esplora-client/pull/79

But I believe other than those releases this is good to go.

fixes #1196

[notmandatory]

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]

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]

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

[realeinherjar]

Closes #1191.

[tcharding]

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]

@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]

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

[tcharding]

The upgrade is currently stuck on the rust-electrum-client, PR looks read to go just needs review please: https://github.com/bitcoindevkit/rust-electrum-client/pull/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]

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]

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]

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]

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

https://github.com/bitcoindevkit/bdk/blob/c01983d02aac6ee49c1bb34ade833e5d72bc8738/crates/bdk/tests/wallet.rs#L2058-L2064

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();

[tcharding]

Looks like hwi is the final missing piece. The bitcoin upgrade is done there already.

[notmandatory]

Looks like hwi is the final missing piece. The bitcoin upgrade is done there already.

Thanks! didn't have hwi on my list but will work on getting it ready for a release.

[tcharding]

Thanks man, we are trying really hard to get to a place where you don't have to upgrade 300 crates constantly to get new versions of github.com/rust-bitcoin code.

[notmandatory]

The latest and greatest rust-hwi is published and ready to go: https://crates.io/crates/hwi

[tcharding]

FTR this is upgrade only with minimal changes, I did not attempt an improvements that may be possible with the upgrade eg., using Amount instead of u64 (I just converted everywhere using to_sat() and from_sat()) - thanks.

[tcharding]

ATM I don't know what the CI failure means:

• bug in this patch
• bug in the way weight and/or fee rate are used
• Or, most likely, a difference in the bitcoin::FeeRate from the original bdk::FeeRate.

Needs further investigation.

[notmandatory]

I did some digging into the broken test and found I could fix it by removing the line in Wallet::create_tx() where we always add the fee for 2 WU when calculating the new transaction fee_amount. All unit tests still pass with this line removed: https://github.com/tcharding/bdk/blob/df0a46a2731d1aebd734529537525cb2959de889/crates/bdk/src/wallet/mod.rs#L1511

My theory is your changes in https://github.com/rust-bitcoin/rust-bitcoin/pull/2076/commits/c34e3cc7cc03ec0dd69d679bfa560e32474ab0c3 to fix Transaction::weight() now already take into account the 2 WU for segwit. Does that make sense?

[tcharding]

Sounds plausible and likely. Thanks for investigating for me. I'll re-spin this with your fix.

[tcharding]

@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.

[ValuedMammal]

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]

@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]

@ValuedMammal two things:

1. 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.
2. 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.

[ValuedMammal]

@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.

https://github.com/bitcoindevkit/bdk/blob/53791eb6c5352d9bc7c8ed1cc3062e48f5a5116c/crates/bdk/src/wallet/signer.rs#L521-L542

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]

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]

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]

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

[tcharding]

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]

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.