Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Taproot PSBT finalization fails often when signing with internal key #1142

Closed
yukibtc opened this issue Oct 2, 2023 · 9 comments · Fixed by #1200
Closed

Taproot PSBT finalization fails often when signing with internal key #1142

yukibtc opened this issue Oct 2, 2023 · 9 comments · Fixed by #1200
Assignees
Labels
bug Something isn't working
Milestone

Comments

@yukibtc
Copy link
Contributor

yukibtc commented Oct 2, 2023

Describe the bug

Hi, I'm experiencing an issue during the finalization of a taproot PSBT in a 1 of N multisig.
When I try to sign and finalize the PSBT with BDK everything goes well until the broadcast: the node says the schnorr signature it's invalid. If I try to finalize it with the rust-bitcoin crate (psbt.finalize_mut()) it rise the following error:

InputError(Interpreter(PkEvaluationError(XOnlyKey(XOnlyPublicKey(d44e6a54b589447a3ddb929211c8e6a645d19d1e18d6bfed8aa56b409a833d3152ba840362ad8602ad9b6635b9afa3b49069be77207b9d484d5a4a45b3600911)))), 1)

This error is rised often when the internal key it's used. Using a NON-internal key, everything works well.

To Reproduce

Descriptor: tr([5cb492a5/86'/1'/784923']tpubDD56LAR1MR7X5EeZYMpvivk2Lh3HMo4vdDNQ8jAv4oBjLPEddQwxaxNypvrHbMk2qTxAj44YLzqHrzwy5LDNmVyYZBesm6aShhmhYrA8veT/0/*,{pk([76fdbca2/86'/1'/784923']tpubDCDepsNyAPWySAgXx1Por6sHpSWzxsTB9XJp5erEN7NumgdZMhhmycJGMQ1cHZwx66KyZr6psjttDDQ7mV4uJGV2DvB9Mri1nTVmpquvTDR/0/*),pk([3b8ae29b/86'/1'/784923']tpubDDpkQsJQTpHi2bH5Cg7L1pThUxeEStcn9ZsQ53XHkW8Fs81h71XobqpwYf2Jb8ECmW1mUUJxQhZstmwFUg5wQ6EVzH5HmF3cpHcyxjvF1Ep/0/*)})#yxpuntg3

Mnemonic (internal key): message scissors typical gravity patrol lunch about bacon person focus cry uncover

PSBT: cHNidP8BAKkBAAAAAsoKb5SPcP0PpTjvnGo3b4ANmCiirV/br9mIOgkb6DqyAAAAAAD9////Sq+kHldYHiLZhBePzo2YhIQzK7VF80/HJJZqIk50+/EAAAAAAP3///8C0AcAAAAAAAAZdqkUWcraUDFMgp4Z9ad4b47g1Jh/Qp2IrFEGAAAAAAAAIlEgmXTDG3J0JlZ5VMtvwY5SMzULRwNydG2ygklHanaSQlyigSYAAAEBK3MBAAAAAAAAIlEgG1/+NmZkL1JsGZNosSxtg/9Kkbe/HAUcK32jpI5QbrFCFcAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeHbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmIyDygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdOKzAQhXALvqqp1yCAIMIHggU5SfyXsrGZvweJyXfY4GyHL71w3iFfQngXX5orCqnTyd5FX5DQxoiwCPnGNHfdsePh7btxiMgFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV6swCEWFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV45AXbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmdv28olYAAIABAACAG/oLgAAAAAAAAAAAIRYu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeBkAXLSSpVYAAIABAACAG/oLgAAAAAAAAAAAIRbygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdODkBhX0J4F1+aKwqp08neRV+Q0MaIsAj5xjR33bHj4e27cY7iuKbVgAAgAEAAIAb+guAAAAAAAAAAAABFyAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeAEYIJ+wkpRCDWMuD6tNf1GqVxY6CKaJmgmcgC+PYTOQZfUyAAEBK6wNAAAAAAAAIlEgMT2DmkBrpYrtv9YYHp3RRabmyBGSkts9ekSJtVRqTtRCFcAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+RBDin7tGE2aR+kQoKJyxpF0DZHJsT2V1VdaFoOsXsk2IyApsM0myknnBxj19PqhGgwh2Ghhie5zpCHmxWITzm8BEKzAQhXAI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vmraKimoW2w0V14cQ7x2yrHf3+02CAXJKvfU3aloRTY+yMgQixWhD3FgiFyBWG6XMj442r0/t9K7yyPVK0ihnCVwPGswCEWI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vkZAFy0kqVWAACAAQAAgBv6C4AAAAAAAwAAACEWKbDNJspJ5wcY9fT6oRoMIdhoYYnuc6Qh5sViE85vARA5AatoqKahbbDRXXhxDvHbKsd/f7TYIBckq99TdqWhFNj7dv28olYAAIABAACAG/oLgAAAAAADAAAAIRZCLFaEPcWCIXIFYbpcyPjjavT+30rvLI9UrSKGcJXA8TkBEEOKfu0YTZpH6RCgonLGkXQNkcmxPZXVV1oWg6xeyTY7iuKbVgAAgAEAAIAb+guAAAAAAAMAAAABFyAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+QEYIBVtNWu2F3juLK2NU1s/NbnQ97FQF+ua/Q9ZeFfiZ+G5AAABBSBYBYrEW0Bu/cWXXpZnrr2RUcX2rCyE7v9hu1WWvWcHowEGSgHAIiBUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KqwBwCIgZKOX97OhpiSB3gSJ5F036Hr0DFle15w8QhmWFLXdbFysIQdUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KjkBB6FOPb7JMsJhK6qLWwBpzD3rjkesybS+bwmOFlisHh07iuKbVgAAgAEAAIAb+guAAAAAAAQAAAAhB1gFisRbQG79xZdelmeuvZFRxfasLITu/2G7VZa9ZwejGQBctJKlVgAAgAEAAIAb+guAAAAAAAQAAAAhB2Sjl/ezoaYkgd4EieRdN+h69AxZXtecPEIZlhS13WxcOQFanOfF7GTH3HfDxnTeS5RLxV7ovOatHNKUHW1IAKnf2nb9vKJWAACAAQAAgBv6C4AAAAAABAAAAAA=

Expected behavior

Successfully finalize the PSBT.

Build environment

  • BDK tag/commit: 417963f
  • Rust/Cargo version: 1.72.0
@yukibtc yukibtc added the bug Something isn't working label Oct 2, 2023
@notmandatory
Copy link
Member

Thanks for the detailed issue. I've added it to our next milestone to investigate. If you discover any new details please add them here.

@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Oct 3, 2023
@yukibtc
Copy link
Contributor Author

yukibtc commented Oct 3, 2023

Ah, I forgot to say that it fails also depending on the UTXOs used (only with the internal key, as I written above using a NON-internal key everything always works).

For example, in my unit test I set 3 UTXOs:

  • 371 sat
  • 3500 sat
  • 4149 sat

When BDK use the UTXO of 3500 sat, the signing and finalization with the internal key works well.
But often BDK select the other 2 UTXOs (371 + 4149): it's in this case that finalization fails when using the internal key.

Funded wallet: https://github.com/smartvaults/smartvaults/blob/60b67229ac1e14c6ef2e0f0f5915b43ed23a4d6e/crates/smartvaults-core/src/lib.rs#L60

Unit test: https://github.com/smartvaults/smartvaults/blob/60b67229ac1e14c6ef2e0f0f5915b43ed23a4d6e/crates/smartvaults-core/src/lib.rs#L261

@mgravitt
Copy link

mgravitt commented Nov 2, 2023

Hi @notmandatory any update on this? We are waiting on it to go live in prod because we're afraid it could impact all 1-of-n policies on smart vaults

@danielabrozzoni
Copy link
Member

Hey folks, I'm taking this issue up, I'll let you know if I find the culprit!

@danielabrozzoni danielabrozzoni self-assigned this Nov 7, 2023
@danielabrozzoni
Copy link
Member

In the issue description you're saying that "in bdk everything goes well until i try to broadcast", while in your comment you say that finalization fails when using 2 utxos; can you clarify if you have a failure at finalization time, or if the finalization works well, and then you get an error broadcasting?

Also, would you be able to provide a small, contained example that uses bdk only and reproduces the issue? It would be much easier for me to debug if I could just run a small example. I tried with this:

const DB_MAGIC: &str = "bdk_wallet_esplora_example";
const SEND_AMOUNT: u64 = 1000;
const STOP_GAP: usize = 5;
const PARALLEL_REQUESTS: usize = 1;

use std::{io::Write, str::FromStr};

use bdk::{
    bitcoin::{Address, Network, psbt::Psbt},
    wallet::{AddressIndex, Update},
    SignOptions, Wallet,
};
use bdk_esplora::{esplora_client, EsploraExt};
use bdk_file_store::Store;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let db_path = std::env::temp_dir().join("bdk-esplora-example");
    let db = Store::<bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;
    let external_descriptor = "tr([5cb492a5/86'/1'/784923']tprv8gP4BkNmD3RrBmcmeiALKX5umfXMCTt23umcrD8ceXPLVtys128NQTm7emhSGEEnHVLjxi4SKsRdibF4d8wRr17atRjmMV9eVaZUwzNg3ZD/0/*,{pk([76fdbca2/86'/1'/784923']tpubDCDepsNyAPWySAgXx1Por6sHpSWzxsTB9XJp5erEN7NumgdZMhhmycJGMQ1cHZwx66KyZr6psjttDDQ7mV4uJGV2DvB9Mri1nTVmpquvTDR/0/*),pk([3b8ae29b/86'/1'/784923']tpubDDpkQsJQTpHi2bH5Cg7L1pThUxeEStcn9ZsQ53XHkW8Fs81h71XobqpwYf2Jb8ECmW1mUUJxQhZstmwFUg5wQ6EVzH5HmF3cpHcyxjvF1Ep/0/*)})";

    let mut wallet = Wallet::new(
        external_descriptor,
        None,
        db,
        Network::Testnet,
    )?;

    let mut psbt = Psbt::from_str("cHNidP8BAKkBAAAAAsoKb5SPcP0PpTjvnGo3b4ANmCiirV/br9mIOgkb6DqyAAAAAAD9////Sq+kHldYHiLZhBePzo2YhIQzK7VF80/HJJZqIk50+/EAAAAAAP3///8C0AcAAAAAAAAZdqkUWcraUDFMgp4Z9ad4b47g1Jh/Qp2IrFEGAAAAAAAAIlEgmXTDG3J0JlZ5VMtvwY5SMzULRwNydG2ygklHanaSQlyigSYAAAEBK3MBAAAAAAAAIlEgG1/+NmZkL1JsGZNosSxtg/9Kkbe/HAUcK32jpI5QbrFCFcAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeHbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmIyDygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdOKzAQhXALvqqp1yCAIMIHggU5SfyXsrGZvweJyXfY4GyHL71w3iFfQngXX5orCqnTyd5FX5DQxoiwCPnGNHfdsePh7btxiMgFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV6swCEWFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV45AXbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmdv28olYAAIABAACAG/oLgAAAAAAAAAAAIRYu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeBkAXLSSpVYAAIABAACAG/oLgAAAAAAAAAAAIRbygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdODkBhX0J4F1+aKwqp08neRV+Q0MaIsAj5xjR33bHj4e27cY7iuKbVgAAgAEAAIAb+guAAAAAAAAAAAABFyAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeAEYIJ+wkpRCDWMuD6tNf1GqVxY6CKaJmgmcgC+PYTOQZfUyAAEBK6wNAAAAAAAAIlEgMT2DmkBrpYrtv9YYHp3RRabmyBGSkts9ekSJtVRqTtRCFcAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+RBDin7tGE2aR+kQoKJyxpF0DZHJsT2V1VdaFoOsXsk2IyApsM0myknnBxj19PqhGgwh2Ghhie5zpCHmxWITzm8BEKzAQhXAI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vmraKimoW2w0V14cQ7x2yrHf3+02CAXJKvfU3aloRTY+yMgQixWhD3FgiFyBWG6XMj442r0/t9K7yyPVK0ihnCVwPGswCEWI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vkZAFy0kqVWAACAAQAAgBv6C4AAAAAAAwAAACEWKbDNJspJ5wcY9fT6oRoMIdhoYYnuc6Qh5sViE85vARA5AatoqKahbbDRXXhxDvHbKsd/f7TYIBckq99TdqWhFNj7dv28olYAAIABAACAG/oLgAAAAAADAAAAIRZCLFaEPcWCIXIFYbpcyPjjavT+30rvLI9UrSKGcJXA8TkBEEOKfu0YTZpH6RCgonLGkXQNkcmxPZXVV1oWg6xeyTY7iuKbVgAAgAEAAIAb+guAAAAAAAMAAAABFyAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+QEYIBVtNWu2F3juLK2NU1s/NbnQ97FQF+ua/Q9ZeFfiZ+G5AAABBSBYBYrEW0Bu/cWXXpZnrr2RUcX2rCyE7v9hu1WWvWcHowEGSgHAIiBUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KqwBwCIgZKOX97OhpiSB3gSJ5F036Hr0DFle15w8QhmWFLXdbFysIQdUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KjkBB6FOPb7JMsJhK6qLWwBpzD3rjkesybS+bwmOFlisHh07iuKbVgAAgAEAAIAb+guAAAAAAAQAAAAhB1gFisRbQG79xZdelmeuvZFRxfasLITu/2G7VZa9ZwejGQBctJKlVgAAgAEAAIAb+guAAAAAAAQAAAAhB2Sjl/ezoaYkgd4EieRdN+h69AxZXtecPEIZlhS13WxcOQFanOfF7GTH3HfDxnTeS5RLxV7ovOatHNKUHW1IAKnf2nb9vKJWAACAAQAAgBv6C4AAAAAABAAAAAA=").unwrap();
    dbg!(&psbt);
    let finalized = wallet.sign(&mut psbt, SignOptions::default())?;
    assert!(finalized);

    let tx = psbt.extract_tx();

    Ok(())
}

but finalization worked well, and with this:

const DB_MAGIC: &str = "bdk_wallet_esplora_example";
const SEND_AMOUNT: u64 = 1000;
const STOP_GAP: usize = 5;
const PARALLEL_REQUESTS: usize = 1;

use std::{io::Write, str::FromStr};

use bdk::{
    bitcoin::{consensus::serialize, psbt::Psbt, Address, Network},
    wallet::{AddressIndex, Update},
    SignOptions, Wallet,
};
use bitcoin_internals::hex::display::DisplayHex;

use bdk_esplora::{esplora_client, EsploraExt};
use bdk_file_store::Store;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let db_path = std::env::temp_dir().join("bdk-esplora-example");
    let db = Store::<bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;
    let external_descriptor = "tr([5cb492a5/86'/1'/784923']tprv8gP4BkNmD3RrBmcmeiALKX5umfXMCTt23umcrD8ceXPLVtys128NQTm7emhSGEEnHVLjxi4SKsRdibF4d8wRr17atRjmMV9eVaZUwzNg3ZD/0/*,{pk([76fdbca2/86'/1'/784923']tpubDCDepsNyAPWySAgXx1Por6sHpSWzxsTB9XJp5erEN7NumgdZMhhmycJGMQ1cHZwx66KyZr6psjttDDQ7mV4uJGV2DvB9Mri1nTVmpquvTDR/0/*),pk([3b8ae29b/86'/1'/784923']tpubDDpkQsJQTpHi2bH5Cg7L1pThUxeEStcn9ZsQ53XHkW8Fs81h71XobqpwYf2Jb8ECmW1mUUJxQhZstmwFUg5wQ6EVzH5HmF3cpHcyxjvF1Ep/0/*)})";
    let client =
        esplora_client::Builder::new("https://blockstream.info/testnet/api").build_blocking()?;

    let mut wallet = Wallet::new(external_descriptor, None, db, Network::Testnet)?;

    let prev_tip = wallet.latest_checkpoint();
    let keychain_spks = wallet
        .spks_of_all_keychains()
        .into_iter()
        .map(|(k, k_spks)| {
            let mut once = Some(());
            let mut stdout = std::io::stdout();
            let k_spks = k_spks
                .inspect(move |(spk_i, _)| match once.take() {
                    Some(_) => print!("\nScanning keychain [{:?}]", k),
                    None => print!(" {:<3}", spk_i),
                })
                .inspect(move |_| stdout.flush().expect("must flush"));
            (k, k_spks)
        })
        .collect();

    let (update_graph, last_active_indices) =
        client.scan_txs_with_keychains(keychain_spks, None, None, STOP_GAP, PARALLEL_REQUESTS)?;
    let missing_heights = update_graph.missing_heights(wallet.local_chain());
    let chain_update = client.update_local_chain(prev_tip, missing_heights)?;
    let update = Update {
        last_active_indices,
        graph: update_graph,
        chain: Some(chain_update),
    };

    wallet.apply_update(update)?;
    wallet.commit()?;
    println!();

    let balance = wallet.get_balance();
    println!("Wallet balance after syncing: {} sats", balance.total());

    let address = wallet.get_address(AddressIndex::New);
    println!("Generated Address: {}", address);

    dbg!(wallet.list_unspent().collect::<Vec<_>>());

    let mut tx_builder = wallet.build_tx();
    tx_builder
        .add_recipient(address.script_pubkey(), 5000)
        .enable_rbf();

    let mut psbt = tx_builder.finish()?;
    let finalized = wallet.sign(&mut psbt, SignOptions::default())?;
    assert!(finalized);

    let tx = psbt.extract_tx();

    dbg!(&tx);
    dbg!(&serialize(&tx).to_lower_hex_string());

    client.broadcast(&tx).unwrap();

    Ok(())
}

and both finalizing and broadcasting worked well. Notice that in the first case I used your PSBT, but in the second one I had to re-create a new one as your PSBT"s inputs were spent already. I made sure that the new psbt used two inputs as you described.

Also, my examples use a descriptor with a tprv for brevity, I generated it using your seed and the derivation path of your tpub, and checked that it matched your tpub.

@yukibtc
Copy link
Contributor Author

yukibtc commented Nov 8, 2023

Using BDK the PSBT it's always finalized but calling psbt.finalize_mut as sanity check, if the UTXOs are the 371 + 4149, it rise an error (only with the internal key).

I tested your code and it works (also calling the finalize_mut), but using the descriptor with only tpubs not.

const DB_MAGIC: &str = "bdk_wallet_esplora_example";
const NETWORK: Network = Network::Testnet;

use std::{str::FromStr, sync::Arc};

use bdk::keys::bip39::Mnemonic;
use bdk::{
    bitcoin::{bip32::ExtendedPrivKey, psbt::Psbt, secp256k1::Secp256k1, Network, PrivateKey},
    miniscript::psbt::PsbtExt,
    signer::{SignerContext, SignerOrdering, SignerWrapper},
    KeychainKind, SignOptions, Wallet,
};
use bdk_file_store::Store;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let secp = Secp256k1::new();

    // Descriptor
    let external_descriptor = "tr([5cb492a5/86'/1'/784923']tpubDD56LAR1MR7X5EeZYMpvivk2Lh3HMo4vdDNQ8jAv4oBjLPEddQwxaxNypvrHbMk2qTxAj44YLzqHrzwy5LDNmVyYZBesm6aShhmhYrA8veT/0/*,{pk([76fdbca2/86'/1'/784923']tpubDCDepsNyAPWySAgXx1Por6sHpSWzxsTB9XJp5erEN7NumgdZMhhmycJGMQ1cHZwx66KyZr6psjttDDQ7mV4uJGV2DvB9Mri1nTVmpquvTDR/0/*),pk([3b8ae29b/86'/1'/784923']tpubDDpkQsJQTpHi2bH5Cg7L1pThUxeEStcn9ZsQ53XHkW8Fs81h71XobqpwYf2Jb8ECmW1mUUJxQhZstmwFUg5wQ6EVzH5HmF3cpHcyxjvF1Ep/0/*)})#yxpuntg3";

    // Compose BDK Wallet
    let db_path = std::env::temp_dir().join("bdk-esplora-example");
    let db = Store::<bdk::wallet::ChangeSet>::new_from_path(DB_MAGIC.as_bytes(), db_path)?;
    let mut wallet = Wallet::new(external_descriptor, None, db, NETWORK)?;

    // Mnemonic to BIP32 root key
    let mnemonic = Mnemonic::from_str(
        "message scissors typical gravity patrol lunch about bacon person focus cry uncover",
    )?;
    let seed: [u8; 64] = mnemonic.to_seed("");
    let bip32_root_key = ExtendedPrivKey::new_master(NETWORK, &seed).unwrap();
    let root_fingerprint = bip32_root_key.fingerprint(&secp);

    // Deserialize PSBT
    let mut psbt = Psbt::from_str("cHNidP8BAKkBAAAAAsoKb5SPcP0PpTjvnGo3b4ANmCiirV/br9mIOgkb6DqyAAAAAAD9////Sq+kHldYHiLZhBePzo2YhIQzK7VF80/HJJZqIk50+/EAAAAAAP3///8C0AcAAAAAAAAZdqkUWcraUDFMgp4Z9ad4b47g1Jh/Qp2IrFEGAAAAAAAAIlEgmXTDG3J0JlZ5VMtvwY5SMzULRwNydG2ygklHanaSQlyigSYAAAEBK3MBAAAAAAAAIlEgG1/+NmZkL1JsGZNosSxtg/9Kkbe/HAUcK32jpI5QbrFCFcAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeHbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmIyDygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdOKzAQhXALvqqp1yCAIMIHggU5SfyXsrGZvweJyXfY4GyHL71w3iFfQngXX5orCqnTyd5FX5DQxoiwCPnGNHfdsePh7btxiMgFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV6swCEWFtIMTX0uKt5YZlCIes8oMhGHEckuB4zFQk+7pxoEcV45AXbbPh9W8OonWaQmZlOtCBpPQgWr3u+7xL2VYl0usbBmdv28olYAAIABAACAG/oLgAAAAAAAAAAAIRYu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeBkAXLSSpVYAAIABAACAG/oLgAAAAAAAAAAAIRbygCgrMtGbrZpIZm8llbWf0D1DyyQkfUTNeG84gLWdODkBhX0J4F1+aKwqp08neRV+Q0MaIsAj5xjR33bHj4e27cY7iuKbVgAAgAEAAIAb+guAAAAAAAAAAAABFyAu+qqnXIIAgwgeCBTlJ/JeysZm/B4nJd9jgbIcvvXDeAEYIJ+wkpRCDWMuD6tNf1GqVxY6CKaJmgmcgC+PYTOQZfUyAAEBK6wNAAAAAAAAIlEgMT2DmkBrpYrtv9YYHp3RRabmyBGSkts9ekSJtVRqTtRCFcAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+RBDin7tGE2aR+kQoKJyxpF0DZHJsT2V1VdaFoOsXsk2IyApsM0myknnBxj19PqhGgwh2Ghhie5zpCHmxWITzm8BEKzAQhXAI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vmraKimoW2w0V14cQ7x2yrHf3+02CAXJKvfU3aloRTY+yMgQixWhD3FgiFyBWG6XMj442r0/t9K7yyPVK0ihnCVwPGswCEWI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vkZAFy0kqVWAACAAQAAgBv6C4AAAAAAAwAAACEWKbDNJspJ5wcY9fT6oRoMIdhoYYnuc6Qh5sViE85vARA5AatoqKahbbDRXXhxDvHbKsd/f7TYIBckq99TdqWhFNj7dv28olYAAIABAACAG/oLgAAAAAADAAAAIRZCLFaEPcWCIXIFYbpcyPjjavT+30rvLI9UrSKGcJXA8TkBEEOKfu0YTZpH6RCgonLGkXQNkcmxPZXVV1oWg6xeyTY7iuKbVgAAgAEAAIAb+guAAAAAAAMAAAABFyAj3pBoGesdJs4f23N6vnEdW0yxvOHh889PIee20pj6+QEYIBVtNWu2F3juLK2NU1s/NbnQ97FQF+ua/Q9ZeFfiZ+G5AAABBSBYBYrEW0Bu/cWXXpZnrr2RUcX2rCyE7v9hu1WWvWcHowEGSgHAIiBUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KqwBwCIgZKOX97OhpiSB3gSJ5F036Hr0DFle15w8QhmWFLXdbFysIQdUI9MMMGixrfXy3h7cF0LaRUxovhO315u0pNMtxag5KjkBB6FOPb7JMsJhK6qLWwBpzD3rjkesybS+bwmOFlisHh07iuKbVgAAgAEAAIAb+guAAAAAAAQAAAAhB1gFisRbQG79xZdelmeuvZFRxfasLITu/2G7VZa9ZwejGQBctJKlVgAAgAEAAIAb+guAAAAAAAQAAAAhB2Sjl/ezoaYkgd4EieRdN+h69AxZXtecPEIZlhS13WxcOQFanOfF7GTH3HfDxnTeS5RLxV7ovOatHNKUHW1IAKnf2nb9vKJWAACAAQAAgBv6C4AAAAAABAAAAAA=").unwrap();
    
    // Extract inputs that mnemonic is able to sign
    let mut paths = Vec::new();
    for input in psbt.inputs.iter() {
        for (fingerprint, path) in input.bip32_derivation.values() {
            if fingerprint.eq(&root_fingerprint) {
                paths.push(path);
            }
        }

        for (_, (fingerprint, path)) in input.tap_key_origins.values() {
            if fingerprint.eq(&root_fingerprint) {
                paths.push(path);
            }
        }
    }

    // Derive subkeys to sign the inputs and compose signers
    let mut counter: usize = 0;
    for path in paths.into_iter() {
        // Derive subkey
        let child_priv: ExtendedPrivKey = bip32_root_key.derive_priv(&secp, path)?;
        let private_key: PrivateKey = PrivateKey::new(child_priv.private_key, NETWORK);

        // Compose signer wrapper
        let signer_ctx: SignerContext = SignerContext::Tap {
            is_internal_key: true,
        };
        let signer: SignerWrapper<PrivateKey> = SignerWrapper::new(private_key, signer_ctx);
        wallet.add_signer(
            KeychainKind::External,
            SignerOrdering(counter),
            Arc::new(signer),
        );
        counter += 1;
    }

    //dbg!(&psbt);
    let finalized = wallet.sign(&mut psbt, SignOptions::default())?;
    assert!(finalized);

    // Performs a sanity interpreter check on the finalized psbt which involves checking the signatures/preimages/timelocks
    psbt.finalize_mut(&secp).unwrap();

    Ok(())
}

The above code panic when it's performed the satiny check with finalize_mut. When finalize_mut method rise an error, the transaction it's not accepted by the nodes.

If you try to replace the PSBT in my code with the following one, everything works well (use only the UTXO of 3500 sat):
cHNidP8BAIABAAAAAV99U31xYmIep1eqgtcrfuJIPHXRiBb1IMuX60hvNJy2AAAAAAD9////AtAHAAAAAAAAGXapFFnK2lAxTIKeGfWneG+O4NSYf0KdiKwxBQAAAAAAACJRIDE9g5pAa6WK7b/WGB6d0UWm5sgRkpLbPXpEibVUak7UgnUmAAABASusDQAAAAAAACJRICK656hMN3zHJuk41jFs0WBQqdlgQ/s52uwFKYBeagmXQhXAoACiV/jMFp+5qEHyj6dKGhBc6EafJBIRflxcaOg0qnscpeCx4QADGCRE8cxyP5HcxLzHJ0MHZ2s30d9tqOVQ2SMg8qdWUHEN+X0aiCaXdIBdjXqe3LqXRr4IXLPJj5gVTcKswEIVwKAAolf4zBafuahB8o+nShoQXOhGnyQSEX5cXGjoNKp7ICjlYEcXIlhI/QV4YGkPK4gpLau7Xh3Yq1khzP2Ua3wjIGXP3zZdbF12HUHTp03M1NOgWN0BllPEUjt9fgKLGVC1rMAhFmXP3zZdbF12HUHTp03M1NOgWN0BllPEUjt9fgKLGVC1OQEcpeCx4QADGCRE8cxyP5HcxLzHJ0MHZ2s30d9tqOVQ2TuK4ptWAACAAQAAgBv6C4AAAAAAAQAAACEWoACiV/jMFp+5qEHyj6dKGhBc6EafJBIRflxcaOg0qnsZAFy0kqVWAACAAQAAgBv6C4AAAAAAAQAAACEW8qdWUHEN+X0aiCaXdIBdjXqe3LqXRr4IXLPJj5gVTcI5ASAo5WBHFyJYSP0FeGBpDyuIKS2ru14d2KtZIcz9lGt8dv28olYAAIABAACAG/oLgAAAAAABAAAAARcgoACiV/jMFp+5qEHyj6dKGhBc6EafJBIRflxcaOg0qnsBGCBZXR37ccEb/NtmcktzgQNn2tAegCzWdjDKwg82j7h+twAAAQUgI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vkBBkoBwCIgQixWhD3FgiFyBWG6XMj442r0/t9K7yyPVK0ihnCVwPGsAcAiICmwzSbKSecHGPX0+qEaDCHYaGGJ7nOkIebFYhPObwEQrCEHI96QaBnrHSbOH9tzer5xHVtMsbzh4fPPTyHnttKY+vkZAFy0kqVWAACAAQAAgBv6C4AAAAAAAwAAACEHKbDNJspJ5wcY9fT6oRoMIdhoYYnuc6Qh5sViE85vARA5AatoqKahbbDRXXhxDvHbKsd/f7TYIBckq99TdqWhFNj7dv28olYAAIABAACAG/oLgAAAAAADAAAAIQdCLFaEPcWCIXIFYbpcyPjjavT+30rvLI9UrSKGcJXA8TkBEEOKfu0YTZpH6RCgonLGkXQNkcmxPZXVV1oWg6xeyTY7iuKbVgAAgAEAAIAb+guAAAAAAAMAAAAA

@danielabrozzoni
Copy link
Member

danielabrozzoni commented Nov 8, 2023

The problem lies in the fact that you're adding single keys as signers manually, and at the moment we don't check if the private key corresponds to the input before signing (I suppose you can't even check that, but I'll doublecheck with the team). So what's happening is, when you have multiple inputs, you add multiple single keys, and then bdk uses one key for signing all the inputs.
In case we can't even check that, it means that users must not manually add private keys as signers.
Would it be possible for you to get rid of the code deriving the keys, and just add the private descriptor as a signer?

Edit: oh, I think I have an easy fix on our side :) Will open PR soon

danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Nov 8, 2023
...internal key before signing

Fixes bitcoindevkit#1142

We would previously sign with whatever x_only_pubkey we had in hand,
without first checking if it was the right key or not. This effectively
meant that adding multiple taproot PrivateKey signers would produce
unbroadcastable transactions.
@danielabrozzoni
Copy link
Member

Can you check if #1200 solves the issue?

@yukibtc
Copy link
Contributor Author

yukibtc commented Nov 8, 2023

Can you check if #1200 solves the issue?

Works! Thank you very much!

danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Nov 10, 2023
...internal key before signing

Fixes bitcoindevkit#1142

We would previously sign with whatever x_only_pubkey we had in hand,
without first checking if it was the right key or not. This effectively
meant that adding multiple taproot PrivateKey signers would produce
unbroadcastable transactions.
danielabrozzoni added a commit that referenced this issue Nov 13, 2023
…before signing

e553231 fix(bdk): Check if we're using the correct... ...internal key before signing (Daniela Brozzoni)

Pull request description:

  ### Description

  Fixes #1142

  We would previously sign with whatever x_only_pubkey we had in hand, without first checking if it was the right key or not. This effectively meant that adding multiple taproot PrivateKey signers would produce unbroadcastable transactions.

  ### Changelog notice

  - Fix a bug related to taproot signing with internal keys. We would previously sign with the first private key we had, without checking if it was the correct internal key or not.

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

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK e553231

Tree-SHA512: c4abbcd27935b8ce80a70b6e0843507866e3d075939f0b01504c090929ed96b4b9c6fee599f701e69960a6c86175682cc6d7f8cc4c3fb1d08a74b7563f8ca145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants