Repository: bitcoindevkit/bdk-cli (language: rust)

Issue:
Broadcast should tell user to finalize PSBTs
When trying to `broadcast` a PSBT such as
```
cHNidP8BAHEBAAAAAQU2MK4mbnsx/zjbmKwHGUAMVT1zXRFTPBArkMACRZjxAQAAAAD9////AhAnAAAAAAAAFgAU/52lZ+YvMOqGVPodX71HvvjjvhP7FQEAAAAAABYAFKqBwKFeT43famR0JJGaAhoMZKrXAAAAAAABAN4BAAAAAAEBArVvNyXe4TvWObt2vPpIv4IJPeFG1hRK8RtTgzVx3MEAAAAAAP3///8CECcAAAAAAAAWABT/naVn5i8w6oZU+h1fvUe++OO+E+ZAAQAAAAAAFgAUvv5IaH57cUVjoZ35bLxinO1BwkwCRzBEAiABTG7xfKRyZF2ezFCByBLSMGTA2FXYpMN881YXQZB/TgIgbU/OqbuWbe4KhWnjKeglVbnkko70H6glFe/zoo069fUBIQIeb6icGFSB9miQOCV9IMVmJdbEkXG8Hi86LaEyJRa5swAAAAABAR/mQAEAAAAAABYAFL7+SGh+e3FFY6Gd+Wy8YpztQcJMIgICZNLhaTjDm6k30vM/U2+d1TKQ02pk3MFxixFp4EQN1G5IMEUCIQCYRpRlO8YiUXZHLRYmS7fxhgCCDtYRVJ7Tb1rVOKqsHAIgcAeg6Dh8l2N9cixRUyCKwe0jWC9Xc7lNMrxJnDnlmN8BAQMEAQAAACIGAmTS4Wk4w5upN9LzP1NvndUykNNqZNzBcYsRaeBEDdRuGCrNFF1UAACAAQAAgAAAAIABAAAAAQAAAAAAIgIDRcB4bLvY45WvIPqto3nRP4nAQm1FHeIBWcqo2UiIHYoYKs0UXVQAAIABAACAAAAAgAEAAAACAAAAAA==
```
the transaction gets (rightfully) rejected by the network
```
[2022-05-26T13:05:29Z ERROR bdk_cli] Electrum(Protocol(String("sendrawtransaction RPC error: {\"code\":-26,\"message\":\"non-mandatory-script-verify-flag (Witness program hash mismatch)\"}")))
```
due to not being `finalize`d.

To avoid user confusion I think the user should be either instructed to finalize the PSBT or have this be done automatically (which would be in line with what other wallets are doing).


Discussion / hints:
Good idea, rather than complicating the `broadcast` command with the extra options `finalize` has I'd prefer to give a nicer error if someone tries to broadcast a non-finalized psbt. 

```shell
bdk-cli wallet help finalize_psbt
bdk-cli-wallet-finalize_psbt 0.4.0
Finalizes a PSBT

USAGE:
    bdk-cli wallet finalize_psbt [OPTIONS] --psbt <BASE64_PSBT>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
        --psbt <BASE64_PSBT>              Sets the PSBT to finalize
        --assume_height <HEIGHT>          Assume the blockchain has reached a specific height
        --trust_witness_utxo <WITNESS>    Whether the signer should trust the witness_utxo, if the non_witness_utxo hasn’t been provided
```
@notmandatory TBH before I opened this issue I didn't even realise there is a `finalize_psbt` (with the options) in the CLI. In that case it's obviously better to improve the error - I've changed the title to reflect that.
Do the changes in https://github.com/bitcoindevkit/bdk/pull/621 solve your issue if we bring that into bdk-cli?
I don't see how? My issue came about from trying to broadcast an unfinalized transaction and getting confused by the error message.

Are you saying changes in https://github.com/bitcoindevkit/bdk/pull/621 would make it less likely for the user to forget to finalize?
Yes my thinking is for the `sign` command to always `finalize` (if possible), and then add a `--no-finalize` for those who don't want to finalize for some reason. The result of sign already lets the user know if the PSBT is in fact finalized. The problem with doing an automatic `finalize` on `broadcast` (and then throwing an error if that fails) is the user would have to repeat the signing options they may have already used when signing the PSBT. 
My flow was from outside (hardware device) coming in (to `bdk-cli`). 

But also I agree with what you say @notmandatory, in all cases `broadcast` shouldn't `finalize`. That's why I changed the title to merely suggest to the user to finalize.

```
non-mandatory-script-verify-flag (Witness program hash mismatch)
```
was way too cryptic for me. 

Would be great to say something like "WARNING: trying to broadcast a non-finalized transaction" beforehand.

When using the command broadcast --psbt I get the following error, I don't know how to solve it, any idea. 
[2022-07-20T15:04:04Z ERROR bdk_cli] Electrum(Protocol(String("sendrawtransaction RPC error: {\"code\":-26,\"message\":\"non-mandatory-script-verify-flag (Witness program hash mismatch)\"}")))
> 
@w2k31984 
It seems you might of forgot to **sign** the psbt. 
 You need to sign the PSBT using the `sign --psbt $UNSIGNED_PSBT` command.

Creating a  transaction using` bdk-cli` looks something like this...
- First you **create** a transaction`create_tx --to addressgoeshere:amountSats`
- Second you **sign** the transaction `sign --psbt $UNSIGNED_PSBT`
- Third and final step  is to **brodcast** the transaction using `brodcast --psbt $SIGNED`
-  A full preview of the command --> `bdk-cli wallet -w mywalletName --descriptor "wpkh($XPRV_GoesHere/84'/0'/0'/0/*)" broadcast --psbt $SIGNED_PSBT ` .

One final note: The instance when I got the error message seen below was when I did not sign the **PSBT**
- ` ERROR bdk_cli] Electrum(Protocol(String("sendrawtransaction RPC error: {\"code\":-26,\"message\":\"non-mandatory-script-verify-flag (Witness program hash mismatch)\"}")))` 


> When using the command broadcast --psbt I get the following error, I don't know how to solve it, any idea. [2022-07-20T15:04:04Z ERROR bdk_cli] Electrum(Protocol(String("sendrawtransaction RPC error: {"code":-26,"message":"non-mandatory-script-verify-flag (Witness program hash mismatch)"}")))


Hmm.. This seems like a possible improvement.. The "Witness program hash moissmatch" is not really enlightening.. Its should say something more friendly than that..

Putting this down in my todos.. 
This one is up for grab btw.. If we don't get anyone on this in few weeks I will take it up.. 
@notmandatory I am adding back the good first issue for this one in case someone wanna try this on. This doesn't seem like a pressing need but surely a nice thing to have.. 

Fix patch:
diff --git a/src/handlers.rs b/src/handlers.rs
index 422657e..ca35231 100644
--- a/src/handlers.rs
+++ b/src/handlers.rs
@@ -331,6 +331,7 @@ where
                 (Some(psbt), None) => {
                     let psbt = base64::decode(&psbt).map_err(|e| Error::Generic(e.to_string()))?;
                     let psbt: PartiallySignedTransaction = deserialize(&psbt)?;
+                    is_final(&psbt)?;
                     psbt.extract_tx()
                 }
                 (None, Some(tx)) => deserialize(&Vec::<u8>::from_hex(&tx)?)?,
@@ -388,6 +389,37 @@ where
     }
 }
 
+/// Determine if PSBT has final script sigs or witnesses for all unsigned tx inputs.
+#[cfg(any(
+    feature = "electrum",
+    feature = "esplora",
+    feature = "compact_filters",
+    feature = "rpc"
+))]
+pub(crate) fn is_final(psbt: &PartiallySignedTransaction) -> Result<(), Error> {
+    let unsigned_tx_inputs = psbt.unsigned_tx.input.len();
+    let psbt_inputs = psbt.inputs.len();
+    if unsigned_tx_inputs != psbt_inputs {
+        return Err(Error::Generic(format!(
+            "Malformed PSBT, {} unsigned tx inputs and {} psbt inputs.",
+            unsigned_tx_inputs, psbt_inputs
+        )));
+    }
+    let sig_count = psbt.inputs.iter().fold(0, |count, input| {
+        if input.final_script_sig.is_some() || input.final_script_witness.is_some() {
+            count + 1
+        } else {
+            count
+        }
+    });
+    if unsigned_tx_inputs > sig_count {
+        return Err(Error::Generic(
+            "The PSBT is not finalized, inputs are are not fully signed.".to_string(),
+        ));
+    }
+    Ok(())
+}
+
 /// Handle a key sub-command
 ///
 /// Key sub-commands are described in [`KeySubCommand`].
@@ -714,3 +746,28 @@ pub(crate) fn handle_command(cli_opts: CliOpts) -> Result<String, Error> {
     };
     result.map_err(|e| e.into())
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use bdk::bitcoin::psbt::PartiallySignedTransaction;
+    use std::str::FromStr;
+
+    #[cfg(any(
+        feature = "electrum",
+        feature = "esplora",
+        feature = "compact_filters",
+        feature = "rpc"
+    ))]
+    #[test]
+    fn test_psbt_is_final() {
+        let unsigned_psbt = PartiallySignedTransaction::from_str("cHNidP8BAIkBAAAAASWJHzxzyVORV/C3lAynKHVVL7+Rw7/Jj8U9fuvD24olAAAAAAD+////AiBOAAAAAAAAIgAgLzY9yE4jzTFJnHtTjkc+rFAtJ9NB7ENFQ1xLYoKsI1cfqgKVAAAAACIAIFsbWgDeLGU8EA+RGwBDIbcv4gaGG0tbEIhDvwXXa/E7LwEAAAABALUCAAAAAAEBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////BALLAAD/////AgD5ApUAAAAAIgAgWxtaAN4sZTwQD5EbAEMhty/iBoYbS1sQiEO/Bddr8TsAAAAAAAAAACZqJKohqe3i9hw/cdHe/T+pmd+jaVN1XGkGiXmZYrSL69g2l06M+QEgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQErAPkClQAAAAAiACBbG1oA3ixlPBAPkRsAQyG3L+IGhhtLWxCIQ78F12vxOwEFR1IhA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDIQLKhV/gEZYmlsQXnsL5/Uqv5Y8O31tmWW1LQqIBkiqzCVKuIgYCyoVf4BGWJpbEF57C+f1Kr+WPDt9bZlltS0KiAZIqswkEboH3lCIGA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDBDS6ZSEAACICAsqFX+ARliaWxBeewvn9Sq/ljw7fW2ZZbUtCogGSKrMJBG6B95QiAgPyVdlP9KV1voj+PUHLGIpRL5GRHeKYZgzPJ1fMAvjHgwQ0umUhAA==").unwrap();
+        assert!(is_final(&unsigned_psbt).is_err());
+
+        let part_signed_psbt = PartiallySignedTransaction::from_str("cHNidP8BAIkBAAAAASWJHzxzyVORV/C3lAynKHVVL7+Rw7/Jj8U9fuvD24olAAAAAAD+////AiBOAAAAAAAAIgAgLzY9yE4jzTFJnHtTjkc+rFAtJ9NB7ENFQ1xLYoKsI1cfqgKVAAAAACIAIFsbWgDeLGU8EA+RGwBDIbcv4gaGG0tbEIhDvwXXa/E7LwEAAAABALUCAAAAAAEBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////BALLAAD/////AgD5ApUAAAAAIgAgWxtaAN4sZTwQD5EbAEMhty/iBoYbS1sQiEO/Bddr8TsAAAAAAAAAACZqJKohqe3i9hw/cdHe/T+pmd+jaVN1XGkGiXmZYrSL69g2l06M+QEgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQErAPkClQAAAAAiACBbG1oA3ixlPBAPkRsAQyG3L+IGhhtLWxCIQ78F12vxOyICA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDSDBFAiEAnNPpu6wNX2HXYz8s2q5nXug4cWfvCGD3SSH2CNKm+yECIEQO7/URhUPsGoknMTE+GrYJf9Wxqn9QsuN9FGj32cQpAQEFR1IhA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDIQLKhV/gEZYmlsQXnsL5/Uqv5Y8O31tmWW1LQqIBkiqzCVKuIgYCyoVf4BGWJpbEF57C+f1Kr+WPDt9bZlltS0KiAZIqswkEboH3lCIGA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDBDS6ZSEAACICAsqFX+ARliaWxBeewvn9Sq/ljw7fW2ZZbUtCogGSKrMJBG6B95QiAgPyVdlP9KV1voj+PUHLGIpRL5GRHeKYZgzPJ1fMAvjHgwQ0umUhAA==").unwrap();
+        assert!(is_final(&part_signed_psbt).is_err());
+
+        let full_signed_psbt = PartiallySignedTransaction::from_str("cHNidP8BAIkBAAAAASWJHzxzyVORV/C3lAynKHVVL7+Rw7/Jj8U9fuvD24olAAAAAAD+////AiBOAAAAAAAAIgAgLzY9yE4jzTFJnHtTjkc+rFAtJ9NB7ENFQ1xLYoKsI1cfqgKVAAAAACIAIFsbWgDeLGU8EA+RGwBDIbcv4gaGG0tbEIhDvwXXa/E7LwEAAAABALUCAAAAAAEBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////BALLAAD/////AgD5ApUAAAAAIgAgWxtaAN4sZTwQD5EbAEMhty/iBoYbS1sQiEO/Bddr8TsAAAAAAAAAACZqJKohqe3i9hw/cdHe/T+pmd+jaVN1XGkGiXmZYrSL69g2l06M+QEgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQErAPkClQAAAAAiACBbG1oA3ixlPBAPkRsAQyG3L+IGhhtLWxCIQ78F12vxOwEFR1IhA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDIQLKhV/gEZYmlsQXnsL5/Uqv5Y8O31tmWW1LQqIBkiqzCVKuIgYCyoVf4BGWJpbEF57C+f1Kr+WPDt9bZlltS0KiAZIqswkEboH3lCIGA/JV2U/0pXW+iP49QcsYilEvkZEd4phmDM8nV8wC+MeDBDS6ZSEBBwABCNsEAEgwRQIhAJzT6busDV9h12M/LNquZ17oOHFn7whg90kh9gjSpvshAiBEDu/1EYVD7BqJJzExPhq2CX/Vsap/ULLjfRRo99nEKQFHMEQCIGoFCvJ2zPB7PCpznh4+1jsY03kMie49KPoPDdr7/T9TAiB3jV7wzR9BH11FSbi+8U8gSX95PrBlnp1lOBgTUIUw3QFHUiED8lXZT/Sldb6I/j1ByxiKUS+RkR3imGYMzydXzAL4x4MhAsqFX+ARliaWxBeewvn9Sq/ljw7fW2ZZbUtCogGSKrMJUq4AACICAsqFX+ARliaWxBeewvn9Sq/ljw7fW2ZZbUtCogGSKrMJBG6B95QiAgPyVdlP9KV1voj+PUHLGIpRL5GRHeKYZgzPJ1fMAvjHgwQ0umUhAA==").unwrap();
+        assert!(is_final(&full_signed_psbt).is_ok());
+    }
+}


Test patch: