Fix fee priority for block fill

[xnbya]

This changes the priority of TXs when considering them for inclusion in a block template, prioritising TXs with the highest fee per KB. It also properly takes the transaction fees into account when considering whether to include them in the block, and prevents the overall block reward falling below the reward of an empty block.

[vtnerd]

Instead of this, can we use the default comparison for the set? It will sort correctly, and be less comparisons.

[vtnerd]

And to be more clear I am suggesting that this be removed from the code entirely. By default std::less is used which is just a single comparison, and std::pair::operator< does whats needed in just a handle of checks too.

[iamsmooth]

@vtnerd it is better to invert the division and avoid the possibility of divide by zero. The descending sort is still needed.

[iamsmooth]

although it would be possible to use -(fee/byte) as the sort key and the use the default comparison.

[vtnerd]

The function dividing by fee checks that the transaction fee has met the minimum requirements before adding it to the pool. The function check only allows a fee of zero if the byte count is 0 so it is currently not possible to do an insert of size 0. An explicit check in this function would also be nice for clarity (the cost is small).

If this is the preferred way, that's fine. I think it is strange to do a operator> followed by an operator< when its easy to do both and keep clarity (via "default" usage of std::set). Either way I am attempting to put together some unit tests, because its already long overdo.

[vtnerd]

Or the case where someone modifies the file on disk so that it contains an incorrect fee value is possible too I suppose. Wondering if that deserialization function has an incorporated checksum, that would be nice.

[iamsmooth]

@vtnerd 0 fee is allowed by the protocol, and there are some blocks containing 0 fee transactions. They normally don't enter the pool but I believe still enter the pool during processing of blocks and can stay there if there is a reorg. Given that it is not entirely disallowed, it is better to use fee/bytes rather than bytes/fee. Despite all that, divide by zero would probably "work okay" but is poor style.

As for the "extra" conditionals, that seems to be the preferred way to do a descending sort in C++, which is inherently a less intuitive operation given the way compare functions are defined in only one direction. Ascending could still be used here without introducing the complication of divide-by-zero by using -(fee/bytes) as a I noted in my previous comment.

[vtnerd]

I was not stating that zero fee transactions are impossible, just that this data structure will not allow them. A division by zero can only occur with state file corruption (still possible), or when blockchain::check_fee returns true when the fee is zero. A glance at check_fee suggested that this is only possible if the dynamic fee calculation returns 0 XMR per kb, which would be a bug AFAIK. I quick glance at that fee calculation code indicates that you need ~6x10^12 byte median block size during the 0.6 XMR tail emission phase to reach the 0 dynamic per kb fee (which probably isn't intended, is a result of precision loss, and is an insane block size right now). Perhaps I am mistaken on this portion of the code.

As for the number of conditional checks, I apologize for skipping a step in my description. The crypto::hash does not need to be a part of the comparison checks, it adds no value, and therefore I was removing any associated checks when evaluating the comparison functions. A std::multimap<std::pair<double, std::time>, crypto::hash> does everything necessary. Including crypto::hash in the comparisons is a waste because it adds nothing when selecting a transaction for generating a new block, and does not actually filter out duplicates because operator!= is used instead of a proper sort function (also, duplicates with std::time should be very unlikely already). m_transactions is the data structure used for filtering out duplicate transactions.

[vtnerd]

Two follow-ups:

• It is not my intention to block progress on this patch. After I write some tests I will try to convince everyone to remove the crypto::hash from the sort function. Standard (or at least consistent) sort behavior would be nice for expectation purposes, but tests should help with expectations if dividing by fee is truly a non-starter.
• The number of comparisons has nothing to do with sort direction, it has to do with equivalence. If a secondary field is used for sorting, a check for equivalence on the primary field must be done first. Therefore, moving crypto::hash from key to value means the last two if statements in the custom comparison function can be removed. Again, this just sort of happens naturally if the standard behavior is used because it does exactly this.

[vtnerd]

Sorry about the zero fee noise @iamsmooth , I made an ass(out of myself)umption how about how/when this function was called. A miner can add zero fee transactions to a block, and every transaction in a block is forcibly inserted into tx_pool when received. The latter was surprising; it makes the add_tx a bit more confusing since the kept_by_block flag becomes necessary. tx_pool isn't doing the real double-spend check, and ... whatever another awkward but working feature.

The one positive is that I can update the comments to explain the check on line 164.

[fluffypony]

Reviewed