Fix an edge case in PrepareDenominate

[InhumanPerfection]

dash/src/privatesend-client.cpp

Lines 1181 to 1192 in 8e63646

	if (CPrivateSend::GetDenominations(vecTxOutRet) != nSessionDenom || (nSessionInputCount != 0 && nStep != nStepsMax)) {
	{
	// unlock used coins on failure
	LOCK(pwalletMain->cs_wallet);
	for (const auto& txin : vecTxDSInRet) {
	pwalletMain->UnlockCoin(txin.prevout);
	}
	}
	keyHolderStorage.ReturnAll();
	strErrorRet = "Can't make current denominated outputs";
	return false;
	}

This check (nSessionInputCount != 0 && nStep != nStepsMax) will fail if amount of selected inputs (from SelectCoinsByDenominations) for this round equal to nSessionInputCount

Mixing attempt will be failed:

• if nSessionInputCount equal to 9 (PRIVATESEND_ENTRY_MAX_SIZE)
• if overall amount of inputs for selected denomination (in all rounds) equal to nSessionInputCount

Here is the fix.

[PastaPastaPasta]

I don't see the problem, nor the fix here.

Can you extrapolate on this

Mixing attempt will be failed if nSessionInputCount equal to 9 (PRIVATESEND_ENTRY_MAX_SIZE) 
or if not enough of inputs (not more nSessionInputCount) in lower rounds.

How does your fix actually fix it?

When you say "This check (nSessionInputCount != 0 && nStep != nStepsMax) will fail" do you mean evaluate to true, or evaluate to false. Evaluating to true means that mixing failed, whereas false means mixing was (possibly) fine.

[InhumanPerfection]

Sorry for possible bad phrasing.

This check (nSessionInputCount != 0 && nStep != nStepsMax) will fail

means it will evaluate to true when it must evaluate to false, and then input selection for mixing will fail with error "Can't make current denominated outputs"

I don't see the problem, nor the fix here.

Ok... Because of bug nStep will never be incremented in last iteration of the loop (because of premature break) and will never be equal nStepsMax in such scenarios:

• if nSessionInputCount equal to 9 (PRIVATESEND_ENTRY_MAX_SIZE)

Very common situation if you'll search in the log after Spork 6 activation. SelectCoinsByDenominations (here) will never select more than 9 inputs, However StartNewQueue will produce such requests

• if overall amount of inputs for selected denomination (in all rounds) equal to nSessionInputCount

Say, if you have only 3 inputs of 0.0100001 then (because of bug) you can join only mixes with nSessionInputCount == 1 or 2. If you have only one input (for example 10.0001) then you can never mix it.

[UdjinM6]

Thanks for reporting! 👍 I agree that there is an edge case but IMO the condition itself is not quite logical and should be changed, pls see #2139. However I agree that fixing this part (ordering of break vs steps++) is also worth it, at least for the sake of the sanity of our code :)

Having said this, I don't want to fix one thing in 2 PRs, so if you would agree and cherry-pick cf9ec46 here, I'm fine closing #2139 in fav of this one. Or otherwise I'm going to close this and cherry-pick your commit there :)

[UdjinM6]

Perfect, thanks! 👍

utACK

EDIT: mixing seems to be working just fine in general (haven't tested the edge case).

[PastaPastaPasta]

Okay, so basically if you have n inputs to mix, you can only have rounds with max of n - 1

And this changes it to allow n inputs in one round.

Is my understanding correct?

[UdjinM6]

@paulied

When nInputCount (i.e. required number of denoms) matches the exact number of denoms in your wallet, you are going to end up with nValueLeft == 0 and exit the while loop before counting it as a step and thus the condition below will fail.

[PastaPastaPasta]

@UdjinM6 So in effect, currently you can not mix all of your inputs in one round?

I need to think on it more, but I am getting the vibe that changing this could result in data leakage, and possible de-anon of a round.

[InhumanPerfection]

@UdjinM6

I agree that there is an edge case ...

I think this is more broad - it will always fail if nSessionInputCount is 1, 2, 3, 4 or 9.

If wallet have only 1, 2, 3 or 4 inputs, according to this (it will always try to select all inputs, as random will produce values from 5 to 9), participant who starts mixing session will always fail to proceed in PrepareDenominate thereby breaking the session. Therefore nobody can't mix from 1 to 4 inputs in one round.

@paulied

So in effect, currently you can not mix all of your inputs in one round?

Just to be clear on how amount of inputs is selected for the one round (not in regard of the bug discussion):

• in case you start mixing session:
  • if you have 10+ inputs - no, you can't mix it all in one round, you can mix random amount (from 5 to 9) of inputs in one round
  • if you have 6-9 inputs (N) - you will mix random amount (from 5 to N) of inputs in one round
  • if you have 1-5 inputs - you will always mix all inputs in one round
• in case you join mixing session:
  • you have to provide amount of inputs equal to nSessionInputCount (the one who starts mixing session sets the value of nSessionInputCount)

[PastaPastaPasta]

@InhumanPerfection Is that the case in 12.2.3, or is this due to a change for 12.3? Because that doesn't seem consistent with my testing

[InhumanPerfection]

Only in 12.3 (#2075)

[PastaPastaPasta]

Looking at testnet, I've only seen mixes with exactly 24 in/outs. Meaning 8/participant. But based on what I've read shouldn't 5, 6 and 7 also be happening?

[UdjinM6]

@paulied they are happening, few examples

2018-06-21 17:02:33 CPrivateSendClient::SignFinalTransaction -- pushing sigs to the masternode, finalMutableTransaction=CMutableTransaction(hash=db57cf5954, ver=2, vin.size=15, vout.size=15, nLockTime=0)
...
2018-06-21 17:47:39 CPrivateSendClient::SignFinalTransaction -- pushing sigs to the masternode, finalMutableTransaction=CMutableTransaction(hash=5ea8c15ca4, ver=2, vin.size=18, vout.size=18, nLockTime=0)