Require an explicit sighash or default to SIGHASH_ALL

[ulrichard]

I fail to get a transaction signed for a multisig P2WSH-P2SH wallet.
To illustrate what I am trying to do, I created a test at the bottom of:
https://github.com/ulrichard/bdk/blob/sign_multi_p2wsh_p2sh/src/wallet/mod.rs
When I debug into the sign function, it fails in ComputeSighash for Legacy with MissingSighash.
Am I doing something wrong, or is this a bug in bdk?
If something is missing in bdk, I am willing to give it a try implementing it. But I would need some guidance for that.

[afilini]

That's not really a bug, it's the expected behavior (even though there's probably room to discuss whether this is the best behavior or not).

Since bdk supports signing with any sighash, it requires the sighash_type field of a PSBT input to always have a value, which is normally SigHashType::All. If you create the transaction using bdk itself it should automatically set the field to All, unless it's overridden by TxBuilder::sighash, but if you create it with a different wallet they might not set it.

Another option would be to just default to All if nothing is set, which is probably what most other wallets do.

I'll rename this issue so that we can use it to discuss whether or not we should make this change.

[ulrichard]

I'm all for defaulting to All, and in fact I tried setting it manually to All before. But then it just fails a few lines below with MissingNonWitnessUtxo. This is actually another problem. I think it should be in Segwitv0 instead of Legacy. But how can I create such a PartiallySignedTransaction when using from_unsigned_tx. Should there be a second function, or a parameter to construct a segwit tx?

[afilini]

Currently bdk decides whether it should sign with Segwitv0 or Legacy based on the presence of witness_utxo in your PSBT input. If it's present, it uses segwit, otherwise it defaults to legacy.

If you just create a psbt using from_unsigned_tx you will generally be missing a few other important pieces, like the hd_keypaths and the witness_utxo/non_witness_utxo. Unfortunately those must be provided by whoever created the raw transaction, since that's pretty much the whole point of the PSBT standard: giving somebody else a single unsigned transaction is not enough to sign, and the standard describes how those extra metadata can be somewhat attached to a transaction.

In your case you have a fixed WIF key, so you don't actually need the hd_keypaths, but you still need to know the witness_utxo since it contains the previous script (which theoretically you could also recreate independently since you have the descriptor), but most importantly it contains the value of your output, which is required to sign under bip143 (segwit signatures).

For instance, have a look at the Wallet::complete_transaction() method in bdk, which starts by creating the psbt using from_unsigned_tx and then adds all the extra metadata. You are basically missing all that part in your example.

bdk/src/wallet/mod.rs

Lines 970 to 1042 in 64b4cfe

	fn complete_transaction<Cs: coin_selection::CoinSelectionAlgorithm>(
	&self,
	tx: Transaction,
	prev_script_pubkeys: HashMap<OutPoint, Script>,
	builder: TxBuilder<Cs>,
	) -> Result<PSBT, Error> {
	let mut psbt = PSBT::from_unsigned_tx(tx)?;
	// add metadata for the inputs
	for (psbt_input, input) in psbt
	.inputs
	.iter_mut()
	.zip(psbt.global.unsigned_tx.input.iter())
	{
	let prev_script = match prev_script_pubkeys.get(&input.previous_output) {
	Some(prev_script) => prev_script,
	None => continue,
	};
	// Add sighash, default is obviously "ALL"
	psbt_input.sighash_type = builder.sighash.or(Some(SigHashType::All));
	// Try to find the prev_script in our db to figure out if this is internal or external,
	// and the derivation index
	let (script_type, child) = match self
	.database
	.borrow()
	.get_path_from_script_pubkey(&prev_script)?
	{
	Some(x) => x,
	None => continue,
	};
	let (desc, _) = self.get_descriptor_for_script_type(script_type);
	psbt_input.hd_keypaths = desc.get_hd_keypaths(child)?;
	let derived_descriptor = desc.derive(ChildNumber::from_normal_idx(child)?);
	psbt_input.redeem_script = derived_descriptor.psbt_redeem_script();
	psbt_input.witness_script = derived_descriptor.psbt_witness_script();
	let prev_output = input.previous_output;
	if let Some(prev_tx) = self.database.borrow().get_raw_tx(&prev_output.txid)? {
	if derived_descriptor.is_witness() {
	psbt_input.witness_utxo =
	Some(prev_tx.output[prev_output.vout as usize].clone());
	}
	if !derived_descriptor.is_witness() || builder.force_non_witness_utxo {
	psbt_input.non_witness_utxo = Some(prev_tx);
	}
	}
	}
	// probably redundant but it doesn't hurt...
	self.add_input_hd_keypaths(&mut psbt)?;
	// add metadata for the outputs
	for (psbt_output, tx_output) in psbt
	.outputs
	.iter_mut()
	.zip(psbt.global.unsigned_tx.output.iter())
	{
	if let Some((script_type, child)) = self
	.database
	.borrow()
	.get_path_from_script_pubkey(&tx_output.script_pubkey)?
	{
	let (desc, _) = self.get_descriptor_for_script_type(script_type);
	psbt_output.hd_keypaths = desc.get_hd_keypaths(child)?;
	}
	}
	Ok(psbt)
	}

[ulrichard]

Cool, thanks for the hints.
I cannot call complete_transaction from outside because it is private. So I tried to replicate some things. But there I also ran into the same problem, that a lot of stuff is private. For example DescriptorScripts.
Does that mean that transactions have to be created with BDK in order to be signed with BDK?
And that the full PartiallySignedTransaction has to be serialized and transported between co-signers?
Isn't there a way to reconstruct the additional information on the co-signer site?

[afilini]

Yeah most of those stuff are private because they are not really polished and they'll probably change as we move forward with the development. I wasn't trying to suggest to use directly complete_transaction or any other of the methods that are currently private, I just wanted to show you how the transactions are created currently in BDK to give you an idea of what was missing.

That said, I think we could look into exposing those functionalities as a wallet call that takes a raw transaction or a very basic PSBT and tries to add all the metadata it can. I'm pretty sure Bitcoin Core has such a call, and that's definitely something that would be useful.

Keep in mind tough that even having that option today wouldn't probably make your test work, since your wallet is an offline wallet, and thus doesn't have the UTXOs that are being spent in its database. This would be something that another online and properly synced co-signer could do, but that's not the case in your example. That's why, generally, those metadata are added directly by the creator of the PSBT: if a wallet picks a UTXO to spend it in a transaction, it means that it also has enough information to add that UTXO as a witness_utxo or non_witness_utxo.

Does that mean that transactions have to be created with BDK in order to be signed with BDK?

Not at all, but BDK needs a full PSBT to sign, not just a raw transaction. Every wallet that can export a PSBT today would work with BDK, or at most it would fail with the MissingSighash error because not many wallet fill in that field, but that's exactly why we have this issue to discuss having a default for when that happens.

And that the full PartiallySignedTransaction has to be serialized and transported between co-signers?

Yes, exactly. Usually PSBTs are encoded in base64, or at least that's what Bitcoin Core and HWI do, which makes it very easy to copy/paste them. So you could take a PSBT, serialize it to a binary buffer like Vec<u8> and then serialize that buffer using base64. On the other side you would do the opposite to decode it. There are also other formats being developed to move large PSBTs over animated QR codes.

[notmandatory]

Defaulting to SigHashType::All if nothing is set sound like the right approach if that's what core and other wallets do.

Is the worst case for this approach that if the signer meant to not sign for all outputs then their signatures would be invalid? In this case it seems reasonable that the PSBT creator should be required to specify the SigHashType when it is not ALL.

[afilini]

Yeah apparently that's not only what other wallets do, but also part of the spec. From BIP174, "Data Signers Check For":

If a sighash type is not provided, the signer should sign using SIGHASH_ALL, but may use any sighash type they wish.

[ulrichard]

Thanks for your help and explanations. I could get my example working by using a deserialized psbt from electrum.
It is a bit strange that I still have to set the SigHashType. Did electrum not set that correctly, or was it not de-serialized correctly?

[afilini]

That's pretty much what this issue is all about: BDK requires having an explicit sighash but other wallets don't usually set that field. The spec also says that a wallet should default to SIGHASH_ALL if it's not provided, so we'll update BDK accordingly.

In the end you'll just be able to sign PSBTs directly from wallets like Electrum, Bitcoin Core, etc without having to modify them before.