Skip to content

Add more variance to coin selection in PS mixing #2261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UdjinM6 merged 5 commits into from
Sep 5, 2018
Merged

Add more variance to coin selection in PS mixing #2261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a8a127
Lean towards "from high to low" branch but still mix via "from low to…
UdjinM6 Aug 14, 2018
1dc7e9a
Lean towards edges but still mix starting from the middle sometimes
UdjinM6 Sep 3, 2018
8025de8
Try to mix non-duplicate txids only
UdjinM6 Aug 30, 2018
6eb1cc5
map -> set
UdjinM6 Sep 5, 2018
e0b0f4e
Refactor nRoundStart calculations
UdjinM6 Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions src/privatesend-client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,7 @@ bool CPrivateSendClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymize
CAmount nMaxAmount = nBalanceNeedsAnonymized;

// Try to match their denominations if possible, select exact number of denominations
if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, nMinAmount, nMaxAmount, vecTxDSInTmp, vCoinsTmp, nValueInTmp, 0, privateSendClient.nPrivateSendRounds)) {
if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, nMinAmount, nMaxAmount, vecTxDSInTmp, vCoinsTmp, nValueInTmp, 0, privateSendClient.nPrivateSendRounds, true)) {
LogPrintf("CPrivateSendClientSession::JoinExistingQueue -- Couldn't match %d denominations %d (%s)\n", vecBits.front(), dsq.nDenom, CPrivateSend::GetDenominationsToString(dsq.nDenom));
continue;
}
Expand Down Expand Up @@ -1180,25 +1180,45 @@ bool CPrivateSendClientSession::SubmitDenominate(CConnman& connman)
std::string strError;
std::vector<CTxDSIn> vecTxDSInRet;
std::vector<CTxOut> vecTxOutRet;
// lean towards "highest" branch but still mix via "lowest" one someties
bool fMixLowest = privateSendClient.nLiquidityProvider || (GetRandInt(4) == 0);
// lean towards edges but still mix starting from the middle someties
// Note: liqudity providers always start from 0
bool fScanFromTheMiddle = (privateSendClient.nLiquidityProvider == 0) && (GetRandInt(4) == 0);

int nRoundStart{0};
if (fScanFromTheMiddle) {
nRoundStart = privateSendClient.nPrivateSendRounds / 2;
} else if (!fMixLowest) {
nRoundStart = privateSendClient.nPrivateSendRounds;
}

// Submit transaction to the pool if we get here
if (privateSendClient.nLiquidityProvider) {
// Try to use only inputs with the same number of rounds starting from the lowest number of rounds possible
for(int i = 0; i< privateSendClient.nPrivateSendRounds; i++) {
if(PrepareDenominate(i, i + 1, strError, vecTxDSInRet, vecTxOutRet)) {
LogPrintf("CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, success\n", i);
return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
if (fMixLowest) {
// Try to use only inputs with the same number of rounds, from low to high
while (true) {
for(int i = nRoundStart; i < privateSendClient.nPrivateSendRounds; i++) {
if(PrepareDenominate(i, i + 1, strError, vecTxDSInRet, vecTxOutRet)) {
LogPrintf("CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, success\n", i);
return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
}
LogPrint("privatesend", "CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, error: %s\n", i, strError);
}
LogPrint("privatesend", "CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, error: %s\n", i, strError);
if (nRoundStart == 0) break;
nRoundStart = 0;
}
} else {
// Try to use only inputs with the same number of rounds starting from the highest number of rounds possible
for(int i = privateSendClient.nPrivateSendRounds; i > 0; i--) {
if(PrepareDenominate(i - 1, i, strError, vecTxDSInRet, vecTxOutRet)) {
LogPrintf("CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, success\n", i);
return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
// Try to use only inputs with the same number of rounds, from high to low
while (true) {
for(int i = nRoundStart; i > 0; i--) {
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for( -> for (

if(PrepareDenominate(i - 1, i, strError, vecTxDSInRet, vecTxOutRet)) {
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if( -> if (

LogPrintf("CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, success\n", i);
return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
}
LogPrint("privatesend", "CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, error: %s\n", i, strError);
}
LogPrint("privatesend", "CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, error: %s\n", i, strError);
if (nRoundStart == privateSendClient.nPrivateSendRounds) break;
nRoundStart = privateSendClient.nPrivateSendRounds;
}
}

Expand Down Expand Up @@ -1251,7 +1271,7 @@ bool CPrivateSendClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds
return false;
}
std::vector<CAmount> vecStandardDenoms = CPrivateSend::GetStandardDenominations();
bool fSelected = pwalletMain->SelectCoinsByDenominations(nSessionDenom, vecStandardDenoms[vecBits.front()], CPrivateSend::GetMaxPoolAmount(), vecTxDSIn, vCoins, nValueIn, nMinRounds, nMaxRounds);
bool fSelected = pwalletMain->SelectCoinsByDenominations(nSessionDenom, vecStandardDenoms[vecBits.front()], CPrivateSend::GetMaxPoolAmount(), vecTxDSIn, vCoins, nValueIn, nMinRounds, nMaxRounds, true);
if (nMinRounds >= 0 && !fSelected) {
strErrorRet = "Can't select current denominated inputs";
return false;
Expand Down
9 changes: 8 additions & 1 deletion src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3007,8 +3007,10 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov
return true;
}

bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, std::vector<COutput>& vCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax)
bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, std::vector<COutput>& vCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax, bool fNoDuplicateTxIds)
{
std::set<uint256> setRecentTxIds;

vecTxDSInRet.clear();
vCoinsRet.clear();
nValueRet = 0;
Expand Down Expand Up @@ -3037,6 +3039,7 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
{
// masternode-like input should not be selected by AvailableCoins now anyway
//if(out.tx->vout[out.i].nValue == 1000*COIN) continue;
if(fNoDuplicateTxIds && setRecentTxIds.find(out.tx->GetHash()) != setRecentTxIds.end()) continue;
Copy link
Copy Markdown

@InhumanPerfection InhumanPerfection Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But will not this lead to the situation that most mixing transactions will contain only 1 input from each participant?

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't. Even if it does that's fine for privacy.

Copy link
Copy Markdown

@InhumanPerfection InhumanPerfection Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's very fine for privacy. But mixing time will increase significantly. And mixing will cost much more.

Copy link
Copy Markdown
Author

@UdjinM6 UdjinM6 Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a valid concern. I'll have a look.

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fees would go up, but I see no situation where a participant would only be able to submit one denom, maybe the first mix when they are all fresh out of create denoms

Copy link
Copy Markdown

@InhumanPerfection InhumanPerfection Sep 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PastaPastaPasta not only after create denoms.

What we have: in most cases (nRoundStart = privateSendClient.nPrivateSendRounds) mixing function will select inputs with highest round originating only from different Txs.

After "create denoms" the participant will be able to provide from 1 to several inputs (depending on amount he is trying to mix - can be several "create denoms" Txs).

And after every mixing round where participant will be able to provide more than 1 input, on the next round in most cases he will not be able to provide more than 1 input because all inputs with highest round will be from the same Tx.

The number of inputs in this situation will always tends to one.

if(nValueRet + out.tx->tx->vout[out.i].nValue <= nValueMax){

CTxIn txin = CTxIn(out.tx->GetHash(), out.i);
Expand All @@ -3051,11 +3054,15 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
vecTxDSInRet.push_back(CTxDSIn(txin, out.tx->tx->vout[out.i].scriptPubKey));
vCoinsRet.push_back(out);
nDenomResult |= 1 << nBit;
setRecentTxIds.emplace(out.tx->GetHash());
LogPrint("privatesend", "CWallet::SelectCoinsByDenominations -- hash %s nValue %d\n", out.tx->GetHash().ToString(), out.tx->tx->vout[out.i].nValue);
}
}
}
}

LogPrintf("CWallet::SelectCoinsByDenominations -- setRecentTxIds.size() %d\n", setRecentTxIds.size());

return nValueRet >= nValueMin && nDenom == nDenomResult;
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, bool fUseInstantSend = false) const;

// Coin selection
bool SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, std::vector<COutput>& vCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax);
bool SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, std::vector<COutput>& vCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax, bool fNoDuplicateTxIds);
bool GetCollateralTxDSIn(CTxDSIn& txdsinRet, CAmount& nValueRet) const;
bool SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<CTxIn>& vecTxInRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax) const;

Expand Down