Coin selection and fee warning

[dexX7]

To quote zathras-crypto#75:

---

int64_t minWarn = 3 * minRelayTxFee.GetFee(200) + CWallet::minTxFee.GetFee(2000); // based on 3x <200 byte outputs (change/reference/data) & total tx size of <2KB

Provides 2600 satoshi value for minWarn using default parameters (ie no relay or mining fee customization) on 0.0.10 (both testnet & mainnet). This is not sufficient however, as the cost to send a ~450 byte transaction was a touch over 6000 satoshi.

Results in no warning about low fees, but failure to send transaction.

Note to self, revisit how warning fee is calculated.

EDIT: for reminder - available inputs for mpZATHm5 were >2600 minWarn value but not sufficient to send a tx.

---

I further noticed, but please correct me, if I'm wrong, that the coin selection right now may select insufficient amounts.

In particular, selectCoins() appears to select coins, until n_max <= n_total is reached. n_total is returned in any case.

ClassAgnosticWalletTXBuilder() is used to construct transactions, and coins are selected as follows:

// Select the inputs
if (0 > selectCoins(senderAddress, coinControl, referenceAmount)) {
    return MP_INPUTS_INVALID;
}

As far as I can see, this is flawed:

1. The check can never fail, because selectCoins() returns at least 0.
2. If this check is refined, say to if (0 >= ..., then it seems as if this still not sufficient, because the coin selection may as well end, before the actual amount to select is reached.

[zathras-crypto]

The way selectCoins() is supposed to work is it will iterate over all inputs until either:

1. We run out of inputs
2. We reach a fee threshold (n_max) which means we'll then stop selecting more inputs (this is to avoid just selecting every possible input)

I further noticed, but please correct me, if I'm wrong, that the coin selection right now may select insufficient amounts.

Well, at the stage of calling selectCoins() there is not really such a thing as insufficient amounts, because at that stage we don't know enough about the transaction (most notably its size) to tell exactly how much is a sufficient amount.

To cut a long story short, selectCoins() doesn't select inputs to meet X amount, it selects all available inputs up until the threshold (n_max). This means it can still select inputs totalling below the threshold if it runs out of more inputs to select, since in many cases that's still enough to send a transaction.

The check can never fail, because selectCoins() returns at least 0.

That's a fair point, and should be changed to 0 >= because we should be erroring out in the case that we're not able to select any inputs.

If this check is refined, say to if (0 >= ..., then it seems as if this still not sufficient, because the coin selection may as well end, before the actual amount to select is reached.

This is probably the hard concept to grasp, but there is no actual amount to select. It's simply a case of "scoop up inputs until you run out or go over the max threshold" - then those inputs are used to try and create a transaction.

It's certainly less than ideal and I've had it on the todo for ages to give input selection a thorough going over to utilize heuristics such as count, size and age (since they directly affect mining priority) but it's not been high up the list sadly.

So looping back round to the original issue, the return value (the amount we selected for inputs) is accurate, it's the estimation of how much we'll need for the fees that seems not to be.

inputTotal: 2920  minwarn: 2600

Here inputTotal is accurate, it's the minWarn value that is too low, because the calculation:

int64_t minWarn = 3 * minRelayTxFee.GetFee(200) + CWallet::minTxFee.GetFee(2000); // based on 3x <200 byte outputs (change/reference/data) & total tx size of <2KB

provides a result of 2600, an insufficient value. Thus I believe it's the calculation that needs fixing. I've tried to get some more specifics but sadly even with src/qt/bitcoin-qt --testnet --omnidebug=all --debug=coindb --debug=selectcoins I'm still not seeing any verbose info here.

What we do know is that:

1. minRelayTxFee.GetFee(200) says we need a fee of 200 satoshis per output (which we multiply by 3)
2. minTxFee.GetFee(2000) says we need a fee of 2000 satoshis for a 2KB transaction
   And that's where our 2600 minimum comes from.

Now let's look at just sending a plain bitcoin transaction from that address using coin control and no custom values (payFee = 0), notice the fee required is almost 7000 satoshis and thus much higher than the available inputs?
http://i.imgur.com/0Lp5JDN.png
(Note - github is being stupid & frustrating me so linking to Imgur (repeatedly fails to attach an image /sigh)

Long story short I agree we need to make the selectCoins() return check 0>= instead of 0>, but for fixing this minimum fee warning, I think we need to get to the bottom of the actual minimums required.

Hope that makes sense bud !! :)

[dexX7]

Ah, thanks for the details!

My assumption was that selectCoins() can fail, and actively indicates failure, if the amount to select isn't available, but this explains it very well.

Regarding the fee issue: the formula is sort of sound, and I put some thoughts into it, when we adopted this one, see: zathras-crypto#17 (comment).

3 * relayFee + 2 * transactionFee should be okay, but right now we have a wrong understanding of what values relayFee and transactionFee have in practise.

[dexX7]

Slightly related:

The fee warning doesn't vanish, after BTC are sent to a "warned" address.

[zathras-crypto]

Ahh yes, thanks for the reminder - need to re-add wallet model for those bits...

[dexX7]

#99 ensures the warnings are properly cleared, and it added a static fee warning threshold of 10K satoshi, which isn't ideal, but it resolves this issue. Improvements may be submitted in the future, but tracked via a new issue. /close