Gradually bump fee when trying to select coins for PS spending tx #2361
Conversation
|
|
||
| //if we're doing only denominated, we need to round up to the nearest smallest denomination | ||
| if(nCoinType == ONLY_DENOMINATED) { | ||
| if (nCoinType == ONLY_DENOMINATED) { |
There was a problem hiding this comment.
Mixing formatting with code changes? Disappointed... 😛
|
|
||
| // 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); |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
closing in fav of #2371 |
A better way of handling PS fees in cases when no smallest denoms left.
Replaces #1372