Altered min fee calculation function to match current Core behavior.

[lologarithm]

Specifically the number of KB is no longer rounded down to next integer.

Since this changes slightly the meaning of the 'minRelayTxFee' variable it has been renamed to 'relayTxFeeRate' and have deprecated the old config value associated with it and have added a new one.

See issue: #521

[jrick]

This uses the rate as the minimum allowed fee, but it should be calculated to be less than that when the transaction size is less than 1000 bytes. The serializedSize > 1000 check should be removed.

Also I'd like to see KB be switched to kB since k is the metric prefix for kilo. thoughts @davecgh @dajohi?

[davecgh]

This does not match the behavior. It should be:

minFee := (serializedSize * int64(minRelayTxFee)) / 1000
if minFee == 0 && minRelayTxFee > 0 {
    minFee = minRelayTxFee
}

For example, let's take some representative values. A serialized transaction size of 284 bytes and a relay rate of 5000.

The code in this PR as it is now would result in minFee = 5000. However it should be minFee = 1420.

In fact, I think it would be a good idea to add this specific test to TestCalcMinRequiredTxRelayFee.

[davecgh]

More examples of tx sizes, min relay fees, and expected min fees (I'd suggest adding these to the tests):

tx size: 1500 bytes
minRelayTxFee: 5000
minFee: 7500

tx size: 1500 bytes
minRelayTxFee: 3000
minFee: 4500

tx size: 782 bytes
minRelayTxFee: 5000
minFee: 3910

tx size: 782 bytes
minRelayTxFee: 3000
minFee: 2346

tx size: 782 bytes
minRelayTxFee: 2550
minFee: 1994

[jrick]

May want to rename minRelayTxfee to relayTxFeeRate or something else that doesn't imply it is a minimum for all transactions regardless of their size.

[lologarithm]

I will add all those unit tests. I like the idea of changing the variable name as it clearly confused me ;)

[davecgh]

@jrick: Yes, I'm fine with using kB to mean 1000 bytes since that matches the standard. Of course, I also prefer KiB to mean 1024 bytes since it is unambiguous.

[lologarithm]

I will make sure to use kB to signify 1000 instead of 1024.

[lologarithm]

I have reverted the change I made to make 'minRelayTxFee' the minimum. I have one outstanding question - what is the expected fee for a 0 byte transfer? With the new logic it becomes 0 but that seems bad. Should it default to 1? Or should it be 0?

[davecgh]

A 0 byte transfer would not require any fee. Since it's impossible to have a zero byte serialized transaction, the only way the result can be 0 is if the relay fee is set to 0 in which case, it should be 0.

[lologarithm]

Everything should be fixed now. I think if you want to change the minRelayTxfee variable name we could make a separate issue.

[davecgh]

I think it's probably a good idea to rename it, as it is a relic of the past when it actually was a minimum (due to the full kB boundary logic).

Speaking of which, since the logic is changing, I think the command line option minrelaytxfee probably needs to be updated in this same PR (the description).

I'm not a huge fan of changing CLI options since it can break existing setups, so I suspect we should add a new option to allow the rate to be configured and make the old CLI a deprecated alias for it (see getworkkey for an example).

@jrick Thoughts on the CLI option bit?

[jrick]

Deprecating and adding an alias sounds ok to me.

[davecgh]

There is a case missing here (I had it in my original comment):

if minFee == 0 && minRelayTxFee > 0 {
    minFee = int64(minRelayTxFee)
}

This is because there are 2 cases in which the minFee can be zero:

1. A zero serializedSize (which is actually impossible in practice)
2. When serializedSize * minRelayTxFee < 1000 (due to integer division)

The additional if statement covers the second case to ensure a combination of the serialized tx size and relay rates that would result in values too small will not result in a 0 fee.

[lologarithm]

New name for min fee will be relayTxFeeRate instead of minRelayTxFee.

I am also learning the config system now and am going to deprecate the old value and make a new one following the new name.

[davecgh]

Use the --relaytxfeerate option instead or simply Use --relaytxfeerate instead.

[davecgh]

After thinking about it some more, I'm not sure the new name really reflects the behavior. It's still a minimum tx fee rate and the new name doesn't convey that. Since it's in the unit of Satoshi/kB, tx sizes < 1000 scale it downwards just as sizes > 1000 scale it upwards.

I understand the confusion of thinking it's an absolute floor, however that can be resolved by comments.

Also, given the fact renaming it would cause churn to the CLI params (which as I previously stated, I'm not a huge fan of), I think I'd prefer not to rename it. @dajohi, @jrick thoughts?

[davecgh]

Rather than churning this PR, how about we just get the logic changes in without the rename, then if we really want to rename, do it in another PR?

[lologarithm]

I am ok either way. Let me know as soon as you decide and I will push this up.

[lologarithm]

Hadn't heard anything so I reverted the renaming changes. Another ticket for naming seems like a fairly easy thing to do once the new names and descriptions are chosen.

[davecgh]

Thanks. I was waiting for feedback from @jrick and @dajohi. However, I think we can safely worry about any potential renames in a different PR. This one should be ready. Please go ahead and rebase/squash it to a single commit (instructions in Managing a btcd Pull Request for Code Contibutions if you need them).

[dajohi]

OK on this diff, when squashed and rebased.

[jrick]

ok

[davecgh]

Would you please make the commit message following the Model Git Commit Message Format described in the Code Contribution Guidelines? Thanks!

[lologarithm]

Sure - sorry about that.

[davecgh]

OK