InputPair's lack of a constructor seems to cause mistakes

[DanGould]

@kumulynja mentioned to me that

[in] the last step of broadcasting the payjoin transaction I get the following error:

BdkError.electrum(field0: Electrum server error: "sendrawtransaction RPC error -26: mandatory-script-verify-flag-failed (Signature must be zero for failed CHECK(MULTI)SIG operation)")

The proposal psbt is finalized when signing with the sender before broadcasting, and I think I am using the same signing options as in the current version, so not sure what to change there.

Turned out @kumulynja was passing empty lists to the psbt::Input's redeemScript and witnessScript fields, but should have been leaving them None to spend his P2WPKH Input. See the commented code below:

I believe I recall @konstantinullrich having a similar problem in implementation.

I wonder if AddressType-specific constructors (e.g. InputPair::new_p2tr, InputPair::new_p2wpkh etc.) would have prevented this implementation error. It was tricky to hunt down what the problem even was since it only failed script verification.

To be explicit, I think their flutter (dart) implementation:

Future<InputPair> _inputPairFromUtxo(bdk.LocalUtxo utxo, bool isTestnet) async {
  final psbtin = PsbtInput(
    witnessUtxo: TxOut(
      value: utxo.txout.value,
      scriptPubkey: utxo.txout.scriptPubkey.bytes,
    ),
  );
  final txin = TxIn(
    previousOutput:
        OutPoint(txid: utxo.outpoint.txid, vout: utxo.outpoint.vout),
    scriptSig: await Script.newInstance(rawOutputScript: []),
    sequence: 0xFFFFFFFF,
    witness: [],
  );
  return InputPair.newInstance(txin, psbtin);
}

May prevent misuse if an imagined InputPair constructor were instead applied as

InputPair _inputPairFromUtxo(bdk.LocalUtxo utxo, bool isTestnet) {
  return InputPair.newP2wpkh(witnessUtxo: utxo.txout, previousOutput: utxo.outpoint);
}

and provided the other fields as defaults (perhaps with optional sequence), or further specified the necessary parameters to create a p2wpkh InputPair.

[Johnosezele]

Hi @DanGould Let me pick this up

[DanGould]

Thanks for stepping up. go for it @Johnosezele

[Johnosezele]

https://github.com/payjoin/rust-payjoin/pull/641

Addressing this in PR @DanGould

[spacebear21]

#712 added constructors for:

• p2wpkh
• p2wsh
• p2tr

still to-do (pulled from #712 (review)):

• p2pkh
• p2wpkh-in-p2sh
• Update callsites to use the new constructors
• Update docstrings
• Rename new constructor to new_unchecked
• Test constructors in conjunction with expected_input_weight
• Move stuff to new input_pair module

[spacebear21]

Upon further digging by @arturgontijo it seems like the constructors for the script-spend types (p2sh, p2wsh, and p2tr in script-spend path) may be unviable.

One issue is that it's not sufficient to provide witness_script since that only covers the UTXO locking script weight, but the unlocking script (i.e. signature) could be any arbitrary script of unpredictable weight and can't be known in advance of signing. Since wallets should have the information required to predict the max size of the eventual signature, we can have them provide an "max expected weight" or similar parameter when constructing InputPairs.

Another issue is that there are potentially fields on the PSBT that must be present for the wallet to sign, but that are omitted by the new constructors. For example, Liana wallet cannot sign if bip32_derivation is not present. Since we can't predict every use case by every wallet, it may be necessary to leave a flexible constructor like new in which callers can provide custom Psbt::Input and TxIn pairs (in addition to specifying a custom expected weight).

It was suggested that we make expected_weight a required field on InputPair and calculate it and store it upfront for the known/predicatble address types (instead of calculating on the fly with the existing method), and allow this field to be specified manually for more advanced wallets like Liana.

[shinghim]

The "max expected weight" makes sense for the scripts - these would just apply to the p2sh and p2wsh constructors, right?

Since we can't predict every use case by every wallet, it may be necessary to leave a flexible constructor like new in which callers can provide custom Psbt::Input and TxIn pairs (in addition to specifying a custom expected weight).

Does it make sense to rename this to new_unchecked or something similar, just so the user is aware that they should be validating the psbt::Inputs and TxIns before passing them in? I think that would help address the original concern of this issue