verify_tx_input is oblivious of actual input type

[undeath]

jmbitcoin's verify_tx_input function is oblivious of the actual input type in a transaction and opportunistically guesses the type from its arguments instead of checking the script. This leads to erroneously "good" signatures if a signature is, in itself, valid but does not match the actual script (p2pkh/p2sh-p2wpkh).

Since the resulting signed tx is invalid the implications are negligible but can lead to confusing log output.

I intend to make a PR for this eventually. This is mostly a reminder for myself.

[AdamISZ]

Technically, for segwit it doesn't pay attention to a claimed script type but insists on specifically p2sh/p2wpkh, see: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/c541b012c3a304711d6790f6489bff752df094e8/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L549-L550

But for non-segwit (which I can't see anyone using, but no matter), you're right, it accepts it as given, so I for sure agree we should check the type, so as to avoid mystery as to why the transaction turns out invalid. Thanks.

(Edited to fix link so it doesn't move)

[AdamISZ]

In the process of reviewing the handling of transaction types, I wanted to drop a note here to record what I learned and remembered about the details of this issue.

In maker, on receipt of transaction, we sign and have to send data in the !sig message which is sufficient for the other side to verify (and of course then construct the fully signed tx for broadcast). Now, the subtleties are around what this data consists of.

For p2pkh as in original JM, it's simple: we send (sig, pub); the receiver (taker) can take the scriptPubKey from the utxo set, and from that alone can construct the transaction template for sighashing, then do the ecdsa_verify operation. Technically we could I think drop the requirement to send the pubkey here and use pubkey recovery, but we didn't really want to get in the weeds there to save sending a single field.

For the current type, p2sh-p2wpkh, that data obviously isn't sufficient. The scriptPubkey contains the hash160 of the witness program as defined in BIP141/143; it can be thought of as basically the scriptPubKey for native segwit (0014... here). The hash of that isn't useful on its own. So we must send this witness program field. It's currently in the maker code referred to as wit or witness, which is quite confusing, as the witness and the witness program are two very different things. (Further, there is also confusion of a related nature in verify_tx_input about 'witness' but more on that below).

So far that isn't controversial, but what perhaps is: the thinking behind sending (sig, pub, witnessProgram) for the current p2sh-p2wpkh case was, we want to minimise what has to be transferred; so the remaining information necessary for constructing the transaction template for sighashing, namely the scriptCode, is considered implicit. That's why as you see above, the code currently in verify_tx_input assumes the nature of the scriptCode.

(Note how, in the case of p2sh-p2wsh (or indeed the native equivalent), the scriptCode cannot be implicit; it's basically a redeemScript, which can be any arbitrary script; in a future where Joinmarket transactions involved spending such utxos, it would be unavoidable to include this as a further (fourth) data field to pass across. We may be able to play the same game as before here: use the number of fields sent as a flag; but it can only provide backwards compatibility, not forwards (old code can't handle four data fields).)

However, to state the probably obvious, this is not the right way of going about things. Since jmbitcoin as a package is trying to provide bitcoin functionality, its verify_tx_input method should just verify any input, and any assumptions like the above about implicit scriptCode should be dealt with outside, specifically in the method Taker.on_sig.

For these reasons, while I'm currently adding generic signing support for all segwit types to jmbitcoin, I'm also (and as you can see it's taken some thinking!) changing verify_tx_input to be properly generic, and making slight changes to the Taker code also. I think also adding a get_script_code to the wallet (or more specifically the engine it encapsulates). But details obv can be reviewed later.

And to be more concrete, the current verify_tx_input has a confusion about the argument witness, which came out of me originally struggling to get these details clear; the field amount acts as an unambiguous flag as to whether the transaction input is to be signed segwit-style or not, so the field witness currently has no purpose. So I'm changing it this way: what's currently witness in that func signature, will change to scriptCode, and the script argument will contain the witnessProgram in cases where that is needed (note how for native segwit that already fits; the scriptPubKey is the witness program).

As a final note, I've removed the bug label from this issue. I think that should be reserved for cases where the functionality is actually incorrect or causes crashes. The functionality is not currently incorrect, even if it's got some pretty shitty/messy features as per above :) ; it's just that the code is not generic enough to handle other things we want to do in future.

[AdamISZ]

No longer applicable after #536 was merged.