Default 3rd argument of gettxout should be True

[AdamISZ]

Fixes #1294.
Before this commit, calls to query_utxo_set with default arguments
would ignore the mempool and thus return utxos which were spent in
unconfirmed transactions. Thus, takers would continue negotiation of
coinjoins with makers who sent them already-spent utxos, leading to
failure at broadcast time. This was not intended behaviour; we want
takers to reject utxos that are double spent in the mempool.
This commit changes that default argument to True so that utxo set
changes in the mempool are accounted for. All current calls to this
function use the default value of the includeunconf argument.

[kristapsk]

There are other places where query_utxo_set() is called.

Here I don't think unconfirmed should be included:

joinmarket-clientserver/jmclient/jmclient/commitment_utils.py

Line 71 in fb4644e

res = jm_single().bc_interface.query_utxo_set([u])

Also here definitely includeunconf must be set to False:

joinmarket-clientserver/jmclient/jmclient/maker.py

Lines 82 to 85 in 16fa85b

	#finally, check that the proffered utxo is real, old enough, large enough,
	#and corresponds to the pubkey
	res = jm_single().bc_interface.query_utxo_set([cr_dict['utxo']],
	includeconf=True)

Also here in taker code, as commitments must be confirmed (althrough age argument should filter them out anyway):

joinmarket-clientserver/jmclient/jmclient/taker.py

Lines 757 to 759 in e082c3c

	def filter_by_coin_age_amt(utxos, age, amt):
	results = jm_single().bc_interface.query_utxo_set(utxos,
	includeconf=True)

Concept ACK on changing argument default, but all places it is called should be carefully re-checked.

[AdamISZ]

I checked all of them - but it for sure doesn't hurt to check more!

In the PoDLE related ones (in maker and taker modules) the age is checked separately, as you noted.
In add-utxo we only validate the utxo is real, not its age (that'll be checked in real time at podle generation time).

There's a confusion that's easy to miss here: setting the 3rd argument of gettxout to False has an unfortunate effect: it means you return as 'existing', utxos which have been spent but the spending tx is not yet confirmed. In most cases, this not acceptable, and that includes these PoDLE generation cases, I think. So you have to use True, even if returns 0-conf new output. The calling code has to pay attention to the number of confs, if that matters.

The logic isn't that obvious (which is doubtless why I (assuming it was me) got it wrong in the first place .. at best a partial excuse, but there you go).

[AdamISZ]

On reflection we could reduce the confusion by renaming the argument include_mempool instead of includeunconf.

[PulpCattel]

Code change is straightforward, tests pass.

calls to query_utxo_set with default arguments
would ignore the mempool and thus return utxos which were spent in
unconfirmed transactions.

This indeed seems to be the case, it's easy to verify that given UTXO A and an unconfirmed transaction spending it, gettxout with default values does not return A as part of the UTXO set, while with 3rd argument False it does.
It is also easy to verify that this means returning 0 conf too.

Does this mean a taker will now accept 0 conf from makers? In _verify_ioauth_inputs we are not checking confs, right?

I checked all of them - but it for sure doesn't hurt to check more!

Git grep shows 9 instances, 3 of which are in test_wallets.py, and all of them as you say use default value for includeunconf.

[AdamISZ]

Does this mean a taker will now accept 0 conf from makers? In _verify_ioauth_inputs we are not checking confs, right?

Right, good catch. This logic is twisty. I checked that the maker is ensuring 1 conf with the includeconf argument, and the point behind this PR was to disallow, taker-side, spent-in-mempool utxos, but the point you raise is not addressed. I think takers should reject that (0-conf inputs), I think that was the original intent when using the includeunconf argument here, even though as we see it was incorrect functionally. (actually the more I think about it, there isn't any uncertainty, takers definitely don't want to accept these).

This should be done as mentioned above by using includeconf and checking for != 0. I'll add that to the patch but it'll still be very helpful having other people sanity check with concept here (as well as test).

[kristapsk]

If includeunconf is renamed to include_mempool, then why no rename includeconf to include_blockchain? :) The name blockchaininterface in our code is actually wrong, as it is used to access both Bitcoin blockchain and mempool data.

[AdamISZ]

If includeunconf is renamed to include_mempool, then why no rename includeconf to include_blockchain? :)

Well so 'includeconf' should better be called 'includeconfs' because it means 'include the number of confirmations'. Whereas the old name for the other variable 'includeunconf' was wildly misleading, instead of very slightly unclear; it suggested that if you set it to True, the only change that would happen would be that the returned result would include utxos that were currently unconfirmed, and as discussed here, that is wrong.

The name blockchaininterface in our code is actually wrong, as it is used to access both Bitcoin blockchain and mempool data.

You could say that for sure, but I think it's a bit pedantic and I don't think there is cause of concern here. My concern with the other things is that the names of those arguments were at least a part of why this bug occurred. So your complaint about includeconf, whether it was partially a joke or not, I kind of agree and think we should add 's'.

[AdamISZ]

Also in case you missed it, as per #1294 the variable name is directly taken from the Core RPC documentation here.

[kristapsk]

Well so 'includeconf' should better be called 'includeconfs' because it means 'include the number of confirmations'.

Ohh, sorry, somehow didn't read properly what this argument does, that it affects returned result not which UTXOs are included.

[AdamISZ]

Auditing usages before making another change.

These links are from current master in case that causes a confusion.

Usage 1:

joinmarket-clientserver/jmclient/jmclient/commitment_utils.py

Line 71 in b066097

res = jm_single().bc_interface.query_utxo_set([u])

There's no point in disallowing unconfirmed utxos here. It's only used in add-utxo and sendtomany; for the former, we're adding external commitments and they'll be checked later (as I mentioned earlier), for the latter spending unconf-ed is fine, or at least allowed.

Usage 2:

joinmarket-clientserver/jmclient/jmclient/blockchaininterface.py

Line 436 in b066097

res = self.query_utxo_set([(bintxid, i) for i in range(

This is only used by snicker code currently, and here unconfirmed should probably be better allowed.

Usage 3:

joinmarket-clientserver/jmclient/jmclient/taker.py

Line 561 in b066097

utxo_data = jm_single().bc_interface.query_utxo_set(utxo_list)

The original reason for this issue; here we don't want to accept unconfirmed utxos.

Usage 4:

This is a check at signature-insertion time of the utxo's validity. It should follow the same logic as above (since we already checked the utxo was confirmed, this really just checks that it hasn't been spent in the meantime; so the important part here is that include_mempool should be True, to avoid conflicted spends):

joinmarket-clientserver/jmclient/jmclient/taker.py

Lines 645 to 646 in b066097

	utxo_data = jm_single().bc_interface.query_utxo_set([x[
	1] for x in utxo.values()])

Usage 5:

The other usage in taker.py relate to PoDLE commitments, here the age is checked anyway so it doesn't really matter.

joinmarket-clientserver/jmclient/jmclient/taker.py

Lines 758 to 759 in b066097

	results = jm_single().bc_interface.query_utxo_set(utxos,
	includeconf=True)

Usage 6:

joinmarket-clientserver/jmclient/jmclient/wallet.py

Line 2567 in b066097

utxo_data = jm_single().bc_interface.query_utxo_set(utxo, includeconf=True)

As you can see from the lines below, the age was already being checked here and we don't want unconfirmed, so much as Usage 5 we can allow them, or not, they will be rejected anyway.

Usage 7:

joinmarket-clientserver/jmclient/jmclient/maker.py

Lines 84 to 85 in b066097

	res = jm_single().bc_interface.query_utxo_set([cr_dict['utxo']],
	includeconf=True)

This one is also a PoDLE check, so that the same reasoning applies: it doesn't really matter, as the age is checked anyway.

[kristapsk]

ACK 81d64d7

[AdamISZ]

2a9972f addresses the needs of usage 3 and as per @PulpCattel 's point, so we now reject both conflicted spends and unconfirmed utxos from makers.

As per the details above, I don't think any of the other usages require changes.

It is debatable whether we should have this as another argument to query_utxo_set or just, as here and the use in wallet.py for example, have the caller check the confirms field. I think the latter is a simpler patch.

[AdamISZ]

I'll squash and merge at some point fairly soon but more review would be very welcome; it's a rather important fix, or at the least, it's a sensitive one, as it could change the internal mechanics of coinjoin negotiation.

[AdamISZ]

So to summarize, logic is:

Maker only selects utxos that have minconfs 1 for offer size, and for actually converting an order into an input utxo set (see YieldGeneratorBasic.get_available_mixdepths and YieldGeneratorBasic._get_order_inputs. Note for the latter that the call to select_utxos specified minconfs=1 and thus overrides any user setting of listunspent_args in the config which chooses less than 1, i.e. 0, for listing available utxos in syncing process.

Taker uses include_mempool=True to reject currently confirmed input utxos with an unconfirmed conflicted spend.

Takers also, after this change, check for the confirmations field of the returned dict from gettxout to make sure that the maker has not erroneously sent an unconfirmed utxo.

[AdamISZ]

Squashed in a3e1ba3.

Now merging, as it has had a fair bit of review and is an important change that can affect several other fixes.