Skip to content

Gradually bump fee when trying to select coins for PS spending tx #2361

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

Closed
UdjinM6 wants to merge 1 commit into from
Closed

Gradually bump fee when trying to select coins for PS spending tx #2361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 26 additions & 16 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2861,26 +2861,36 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
}

//if we're doing only denominated, we need to round up to the nearest smallest denomination
if(nCoinType == ONLY_DENOMINATED) {
if (nCoinType == ONLY_DENOMINATED) {

@PastaPastaPasta PastaPastaPasta Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mixing formatting with code changes? Disappointed... 😛

@UdjinM6 UdjinM6 Oct 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point, shame on me :)

std::vector<CAmount> vecPrivateSendDenominations = CPrivateSend::GetStandardDenominations();
CAmount nSmallestDenom = vecPrivateSendDenominations.back();
// Make outputs by looping through denominations, from large to small
for (const auto& nDenom : vecPrivateSendDenominations)
{
for (const auto& out : vCoins)
{
//make sure it's the denom we're looking for, round the amount up to smallest denom
if(out.tx->tx->vout[out.i].nValue == nDenom && nValueRet + nDenom < nTargetValue + nSmallestDenom) {
COutPoint outpoint = COutPoint(out.tx->GetHash(),out.i);
int nRounds = GetCappedOutpointPrivateSendRounds(outpoint);
// make sure it's actually anonymized
if(nRounds < privateSendClient.nPrivateSendRounds) continue;
nValueRet += nDenom;
setCoinsRet.insert(std::make_pair(out.tx, out.i));
std::vector<CAmount> vecDenominationsAsAFee = vecPrivateSendDenominations;
std::reverse(vecDenominationsAsAFee.begin(), vecDenominationsAsAFee.end());

// Try to fit into fee that matches one of denominations, from small to large
for (const auto& nDenomAsAFee : vecDenominationsAsAFee) {
CAmount nMaxPSFee = std::min(nDenomAsAFee, maxTxFee);

@PastaPastaPasta PastaPastaPasta Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does validation.h#L61 need to be changed (maxTxFee)?
current

//! -maxtxfee default
static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.2 * COIN; // "smallest denom" + X * "denom tails"

@UdjinM6 UdjinM6 Oct 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hmmm.. we should've probably reverted #737 (i.e. changed this to 0.1 * COIN which is bitcoin's default) the last time we introduced smaller denom becasue the logic in original PR was no longer relevant even at that moment 🙈This probably deserves its own PR however, since it's not directly connected to changes introduced in this specific PR imo.

@PastaPastaPasta PastaPastaPasta Oct 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see #2362

// Make outputs by looping through denominations, from large to small
for (const auto& nDenom : vecPrivateSendDenominations) {
for (const auto& out : vCoins) {
// Make sure it's the denom we're looking for, round the amount up to current max fee
if (out.tx->tx->vout[out.i].nValue == nDenom && nValueRet + nDenom < nTargetValue + nMaxPSFee) {
COutPoint outpoint = COutPoint(out.tx->GetHash(),out.i);
int nRounds = GetRealOutpointPrivateSendRounds(outpoint);
// Make sure it's actually anonymized
if (nRounds < privateSendClient.nPrivateSendRounds) continue;
nValueRet += nDenom;
setCoinsRet.insert(std::make_pair(out.tx, out.i));
if (nValueRet >= nTargetValue) return true; // Done, no need to look any further

@PastaPastaPasta PastaPastaPasta Oct 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm worried that as we are adding inputs it doesn't appear we are increasing the nTargetValue nor accounting for the fees of the added inputs... Could this fail in a way that the amount of the denoms is enough to cover the amount being sent but not enough to cover the amount + the fee resulting in a tx failing?

@UdjinM6 UdjinM6 Oct 23, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was thinking about this and no, technically, this should be good. But then I realised that even in this case we are probably doing it wrong (it's not the job of SelectCoins to guess fees/pick the best set). So, it was a very good question actually :) Pls see #2371 for an alternative solution.

}
}
}
// No luck, try next denom as current max fee
setCoinsRet.clear();
// but only if current denom doesn't exceed the global max fee already
if (nDenomAsAFee >= maxTxFee) return false;
}
return (nValueRet >= nTargetValue);
// should never get here, just in case denom vector is empty for some reason
return false;
}
// calculate value from preset inputs and store them
std::set<std::pair<const CWalletTx*, uint32_t> > setPresetCoins;
Expand Down