make selection of ioauth input more clever

[undeath]

attempts to fix the issue outlined by @AdamISZ in #955 (comment)

Instead of blindly selecting the first selected utxo for ioauth purposes the new code makes sure there actually is a suitable utxo at that position, or aborts the coinjoin if this is not possible.

[AdamISZ]

This point may or may not be obvious but we should discuss it explicitly:

An essentially identical problem exists taker side: if they don't have a "native" type utxo, they will crash in trying to generate a PoDLE that uses a non-native key type. So, the current code just tries to select an in-transaction utxo for PoDLE generation, else in-mixdepth, we need to check how to rewrite that code so that it can hopefully use this same logic. Maybe some kind of janky select_utxos on a UTXOManager consisting of a fixed set. Not sure. But it would be good to reuse the logic since it's the same thing, basically.

[undeath]

Isn't the taker side a lot more complex? The code additionally checks if the utxo is "too_young" (although this is called "too_old" in the code) and "too_small". Besides those criteria, wouldn't randomly adding utxos to a taker cj mess up tumbler schedules?

[AdamISZ]

Besides those criteria, wouldn't randomly adding utxos to a taker cj mess up tumbler schedules?

Not sure what you mean by that part. I don't think we'd be adding random utxos to any specific joins or schedules? It's just filtering the choice of PoDLE utxo as per for the maker filtering the choice of authorizing utxo.
But I agree it's more complicated (and yes that too_old is a rather silly variable naming mistake that never got fixed).

[undeath]

I don't think we'd be adding random utxos to any specific joins or schedules?

ah, yes. The change would end up in the same mixdepth anyway. Had an error in my thinking here.

[AdamISZ]

Once I actually tried this in a manual test, I immediately realized that there is a big aspect left unconsidered: the maker has to sign for the timelocked input correctly (that part should be fine), but then has to transfer the signature to the taker - this part is non-standard, and I can see that right now it is borked and I'm not sure it can be fixed without upgrading taker code.

Apologies, I feel kinda stupid now that I didn't even think about it until I actually manually carried out the transaction (and then it was immediately obvious!).

I will keep looking at it today but I strongly doubt, now, that we will be able to implement spending timelock type outputs in coinjoins, at least for the time being.

(In case it isn't obvious what I'm talking about, look at how the !sig message, passed to the taker, is constructed in the maker:

joinmarket-clientserver/jmclient/jmclient/maker.py

Lines 157 to 171 in 7a50c76

	for index in our_inputs:
	# The second case here is kept for backwards compatibility.
	if self.wallet_service.get_txtype() == 'p2pkh':
	sigmsg = tx.vin[index].scriptSig
	elif self.wallet_service.get_txtype() == 'p2sh-p2wpkh':
	sig, pub = [a for a in iter(tx.wit.vtxinwit[index].scriptWitness)]
	scriptCode = btc.pubkey_to_p2wpkh_script(pub)
	sigmsg = btc.CScript([sig]) + btc.CScript(pub) + scriptCode
	elif self.wallet_service.get_txtype() == 'p2wpkh':
	sig, pub = [a for a in iter(tx.wit.vtxinwit[index].scriptWitness)]
	sigmsg = btc.CScript([sig]) + btc.CScript(pub)
	else:
	jlog.error("Taker has unknown wallet type")
	sys.exit(EXIT_FAILURE)
	sigs.append(base64.b64encode(sigmsg).decode('ascii'))

If you look at the taker's processing in on_sig(), you'll see why it ends up treating the message as a legacy p2pkh case, and sticks the data in the scriptSig, instead of the witness: it is very explicitly only allowing standard types. I defend this as being conservatism in the most security critical element of Joinmarket's code.

Edited to add: it may seem weird that the message even gets to the taker at all, but it's just because the witness for this p2wsh case consists of <signature> <scriptWitness> which happens to be the same length (2) as the witness for the p2wpkh case; but the second entry is not a pubkey and so it can't work, even if we didn't check in the taker, that the script pubkey is of a standard type (and we do, leading to the misinterpretation as legacy p2pkh).
).

The long and the short is: we just haven't written Joinmarket to support arbitrary scripts, yet, albeit we have the backend that could support it, in future.

[AdamISZ]

I should qualify the above: none of that prevents a taker from using a custom script, and I think that should work once we address the PoDLE issue. The maker's verify_unsigned_tx won't care and the maker doesn't need to know how the taker's input scripts are satisfied. It only checks that the maker's ins/outs function correctly as intended.

[AdamISZ]

Still nit on this comment: the priv/pub keypair itself is not intrinsically anything, only the scriptPubKey constructed from it, is.

[undeath]

fixed in 8c3ae11

[AdamISZ]

any particular reason not to wrap the line?

[undeath]

fixed in 8c3ae11

[AdamISZ]

2c143d1 test suite and flake passing locally.

[undeath]

squashed and rebased

[AdamISZ]

The error in test_payjoin is unrelated (and nondeterministic, I believe). Merging after my previous checks.