bitcoindevkit / bdk-cli

A CLI wallet library and REPL tool to demo and test the BDK library
Other
108 stars 64 forks source link

Broadcast should tell user to finalize PSBTs #97

Closed icota closed 1 year ago

icota commented 2 years ago

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

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

notmandatory commented 2 years ago

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.

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
icota commented 2 years ago

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

notmandatory commented 2 years ago

Do the changes in https://github.com/bitcoindevkit/bdk/pull/621 solve your issue if we bring that into bdk-cli?

icota commented 2 years ago

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?

notmandatory commented 2 years ago

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.

icota commented 2 years ago

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.

w2k31984 commented 2 years ago

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)\"}")))

waterst0ne commented 2 years ago

@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 usingbdk-cli looks something like this...

One final note: The instance when I got the error message seen below was when I did not sign the PSBT

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)"}")))

rajarshimaitra commented 2 years ago

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

rajarshimaitra commented 1 year ago

This one is up for grab btw.. If we don't get anyone on this in few weeks I will take it up..

rajarshimaitra commented 1 year ago

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