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.

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.

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

Closes #1191.

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.

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

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

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.

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.

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.

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

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

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

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

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

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

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
    };

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

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.

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

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

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.

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

Sweet, everything works then. Thanks

Closes #1381

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

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

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?