MemPool: Convert mapTx to boost::multi_index

ashleyholman commented at 8:49 am on June 24, 2015: contributor

This patch changes CTxMemPool::mapTx from a std::map to a boost::multi_index_container.

The container has three indexes:

Tx Hash
Fee Rate (fee-per-kb) - ordered from highest to lowest
Priority (at current chain height) - ordered from highest to lowest

The idea is that these indices can later be used for:

Creating blocks
Mempool eviction

The priority stuff is a bit ugly, so suggestions on alternate designs are welcome. I would also like to add another test case that tests the ConnectTip/DisconnectTip triggers, but I need to do some more work to figure out how to construct such a test.

ashleyholman force-pushed on Jun 24, 2015

TxMemPool: Change mapTx to a boost::multi_index_container

Indexes on:
- Tx Hash
- Fee Rate (fee-per-kb)
- Priority (at current chain height)

f27cd7ed6d

ashleyholman force-pushed on Jun 24, 2015

in src/test/mempool_tests.cpp: in f27cd7ed6d outdated

100@@ -101,4 +101,67 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
101     removed.clear();
102 }
103 
104+BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
105+{
106+  CTxMemPool pool(CFeeRate(0));

sipa commented at 3:42 pm on June 24, 2015:

Indentation style is 4 spaces per level.

ashleyholman commented at 12:00 pm on June 27, 2015:

Fixed in latest commit

sipa commented at 3:51 pm on June 24, 2015: member

Untested ACK. This is one of the few places where I think boost shines.

sipa commented at 3:51 pm on June 24, 2015: member

It would be nice to see how much the block creation code could be sped up and simplified with this, though.

in src/main.cpp: in f27cd7ed6d outdated

2216@@ -2216,6 +2217,7 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *
2217     // Remove conflicting transactions from the mempool.
2218     list<CTransaction> txConflicted;
2219     mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload());
2220+    mempool.recalcPriorities(pindexNew->nHeight);

sipa commented at 3:53 pm on June 24, 2015:

I think we want to know priorities aimed for the next block’s height, as that is where the transactions are expected to end up.

ashleyholman commented at 12:01 pm on June 27, 2015:

Fixed in latest commit. Also moved the recalcPriorities call to happen after UpdateTip(), because I didn’t think the priority recalc was important enough to delay updating the tip.

in src/txmempool.h: in f27cd7ed6d outdated

113+        boost::multi_index::ordered_non_unique<
114+            boost::multi_index::identity<CTxMemPoolEntry>,
115+            CompareTxMemPoolEntryByPriority
116+        >
117+    >
118+> ordered_transaction_set;

sipa commented at 3:56 pm on June 24, 2015:

Nit: I think that over time we will want this to be primarily a non-ordered set (because hash-based lookups are faster). Something like transaction_map or indexed_transaction_map would maybe be more appropriate? Maybe define it inside CTxMempool, so it is more clearly scoped.

ashleyholman commented at 6:51 pm on June 24, 2015:

For the unordered index I need a hash function for uint256 because the build fails when it defaults to boost::hash<uint256>. I think you mentioned to me before about some existing code that does this and prevents a dos case - where do I find that function?

sipa commented at 6:53 pm on June 24, 2015:

Don’t bother about that now.

I just mean that an “unordered transaction set” is not what this intends to function as. It’s just how the current implementation works.

As I said, a nit.

ashleyholman commented at 7:03 pm on June 24, 2015:

What about indexed_transaction_set since the indexes have a set-like interface?

sipa commented at 11:09 am on June 26, 2015:

Sounds good to me.

morcos commented at 5:58 pm on June 26, 2015:

@sipa, wouldn’t it be better to go ahead and make that change now? We’re looking up transactions by hash in the mempool all the time (for instance CCoinsViewMemPool::GetCoins). I’m not sure what the efficiency difference is, but given that its currently a hash lookup, its probably not something we should make slower.

morcos commented at 7:02 pm on June 26, 2015:

oops, silly me.. std::map is ordered

sipa commented at 7:13 pm on June 26, 2015:

It is nontrivial to make boost::multi_index_container work with salted hash functions (you want to avoid the ability for network players to produce transactions that collide in the hashtable). I’ve tried myself, so I think it’s reasonable to first get this in without. One of the advantages of boost::multi_index_container is that changing the indexes should be easy.

ashleyholman commented at 12:02 pm on June 27, 2015:

I’ve now renamed it to CTxMemPool::indexed_transaction_set in my latest commit.

Code review fixups:

- Rename ordered_transaction_set to indexed_transaction_set and move typedef
  inside CTxMemPool
- Use chain height + 1 for mempool priority calculation (also moved the
  calculation to happen after updating the tip, so it doesn't delay the block
  being added to the active chain)
- Fix indentation of test case

362bf769ed

ashleyholman commented at 12:06 pm on June 27, 2015: contributor

If this gets merged, the next thing I would look at is changing CreateNewBlock to use the indexes.

But first I need to fix the bug from #6292 because the CreateNewBlock code currently calculates priorities correctly, whereas these indexes use CTxMemPoolEntry::GetPriority which gives incorrect results.

luke-jr commented at 5:46 pm on June 27, 2015: member

@ashleyholman One thing to consider is that these things are policy, so need to be easily modified by the end user - especially for miners. Obviously we’re far from that with the current code, but I wouldn’t want to make the situation worse (I’m not saying this PR does, but it’s something to keep in mind).

laanwj added the label Mempool on Jul 2, 2015

dgenr8 commented at 6:11 pm on July 8, 2015: contributor

Multiple indexes are great but, the building-block for eviction policy will probably take the form of an enhanced “priority” calc that depends on fees, and anything else that’s required.

Separate indexes leave unsolved the problem of how to combine them to find the next tx to evict.

sipa commented at 6:15 pm on July 8, 2015: member

It’s one step towards it.

dgenr8 commented at 6:25 pm on July 8, 2015: contributor

@sipa Completely agree. At least 2 indexes are needed - txid and “priority”.

EDIT: @morcos by quoted “priority” I mean the same thing as “effective fee” or fees with fee deltas applied. Another name for it is an objective function (that which is to be maximized).

morcos commented at 7:20 pm on July 8, 2015: member

Continuing to support transaction priority (as defined by coin age) seems more trouble that its worth. However, by combining this with #6357, it’ll be easy and efficient to keep the existing policies in place. I did some timing tests on the first 7 days of July and there was some small increase in the average of 250ms to connect a block. Calling recalcPriorities took about 2ms. In the busy spam period of July 7th, recalcPriorities was averaging about 8ms out of 500ms to connect a block. Those seem like acceptable performance hits to me. #6357 did not seem to adversely affect that performance and causes the priorities to actually be correct.

Where the default policies will break is when we start adding mempool eviction. We’d need to maintain two separate maps of transactions, one for those destined to be included in the fee section and one for those to be included in the priority section of blocks and evict from them independently. I’d argue that instead, we should accept that high priority but low fee transactions may get evicted from the mempool if we hit mempool saturation. Alternatively we could implement @petertodd’s exponentially increasing minRelayTxFee and not actually evict things. Either way those are next steps and shouldn’t hold up this getting merged.

utACK at this point, I’m going to keep testing.

dgenr8 commented at 9:38 pm on July 8, 2015: contributor

@mikehearn insight: mempool DoS attack may be mitigated through some reliance on coin age priority, as coin age is not something that can be trivially bought on the spot, unlike fee priority.

ashleyholman commented at 10:50 am on July 9, 2015: contributor

@morcos thanks for measuring the recalcPriorities performance

jgarzik commented at 2:24 pm on July 10, 2015: contributor

Drop the priority index, IMO

sipa commented at 3:32 pm on July 10, 2015: member

@wumpus @jgarzik @pstratem @gmaxwell @morcos @ashleyholman @jtimon @luke-jr Ping to get attention of the people involved here.

There are a bunch of related but not quite compatible changes being made here (better indexes, limited mempool, removing priority, accounting memory usage). I think we need to figure out what to do here at a higher level.

Remain compatible with the existing priorities system. This means the recomputations of priority as implemented in this PR, and retaining the relatively complex multi-faceted block creation code. This is likely the least controversial change, but implementing correct mempool limiting becomes pretty hard.
Simplify the priority mechanism to become single-faceted. This would mean defining a (potentially policy-dependent) single scoring function for transactions that can give a benefit to specific transactions. Something similar to priority could be implemented by mapping the “zero-fee” transactions onto a negative number -1/priority. A single ordered index would suffice, and mempool limiting and block creation would be simplified significantly, but re-evaluating the ordering would be necessary after every block.
The same as (2), but only compute the policy-dependent scoring once only - when entering the mempool. You can approximate something similar to priority by counting old/large inputs as a virtual small increase in fee. It would not get updated for new blocks, simplifying code for replacement/limiting further.
Just have a single feerate index, essentially limiting ourselves to a single mempool policy. This is arguably the most economic thing to do for miners, and has the best predictability for fee estimation. It’s perhaps the most controversial change.

I would prefer 2 or 3.

jtimon commented at 4:00 pm on July 10, 2015: contributor

I think my preference is a mix between 3 and 4. I would prefer a single heuristic function that doesn’t need to be recalculated with every block. My preference: priority(tx, utxo) = fee(tx) / (size(tx) + delta_utxo_size(tx, utxo)) Note that delta_utxo_size(tx, utxo) can be negative but size(tx) + delta_utxo_size(tx, utxo) shouldn’t. In fact, we could make a consensus_size(tx, utxo) = size(tx) + delta_utxo_size(tx, utxo) a consensus rule, but I guess that’s a topic for another PR.

essentially limiting ourselves to a single mempool policy

Note that this limitation disappears when we have a CPolicy interface class and a -policy= command line option.

sipa commented at 4:04 pm on July 10, 2015: member

@jtimon I wasn’t even talking about specific policy we would like to see implemented, but rather about what type the “framework” should support. (3) and (4) cannot do the same thing as policy now. (4) doesn’t have any policy in the mix at all for prioritization of transactions.

morcos commented at 4:04 pm on July 10, 2015: member

I think I’d vote for 1. Not because I really care about keeping priority, but because I don’t want the controversy of removing it to delay the other needed improvements. I can see two downsides to 1. There is some additional memory footprint and complication by maintaining correct priorities, but I think the efficiency hit is tolerable. And the issue about defining “correct” mempool limiting. However, I think that issue exists regardless of whether we maintain priority. @jgarzik’s comments about booting oldest first make a lot of sense for relay nodes which are incapable of perfectly guessing the composite miner transaction inclusion policies, but don’t make sense for miners who should boot based on their own inclusion policies. Also if we combine a floating relay minimum with these other things, we might be able to reach a situation where limiting the mempool is an edge case safety measure.

sipa commented at 4:11 pm on July 10, 2015: member

@morcos My largest objection to 1 is that it makes implementing limiting the mempool harder, and (2) can be made to do the (almost?) same thing, but needs a larger code change.

I don’t think we can make mempool limiting an edge case. Adjusting the relayfee works preventive, while mempool limiting works on-the-fly.

jtimon commented at 4:13 pm on July 10, 2015: contributor

@jtimon I wasn’t even talking about specific policy we would like to see implemented, but rather about what type the “framework” should support.

Ok, then I’d like a unified Priority(const CTransaction& tx, const CCoinsViewCache& mapInputs) function and a single unified space: no separated space for low fee transactions. If I understood you correctly, that’s the same as 3.

(3) and (4) cannot do the same thing as policy now.

I don’t think I understand this. Why can’t Priority(const CTransaction& tx, const CCoinsViewCache& mapInputs) be implemented in policy/policy or policy/fees.

sipa commented at 4:15 pm on July 10, 2015: member

@jtimon (3) and (4) cannot update the score after acceptance into the mempool. Priority is time-dependent (needs to be recomputed after every block).

jtimon commented at 4:21 pm on July 10, 2015: contributor

@jtimon (3) and (4) cannot update the score after acceptance into the mempool.

Yes, that’s why I said “single heuristic function that doesn’t need to be recalculated”.

Priority is time-dependent (needs to be recomputed after every block).

Oh, I’m talking about redefining what priority means: to a single unified heuristic for ordering transactions that takes size, fees and the change in size to the utxo (but not time) into account. Otherwise I would have said priority(tx, utxo, time) instead of priority(tx, utxo). Is this option 5 then? I thought this was 3.

morcos commented at 4:22 pm on July 10, 2015: member

@sipa I’m trying to understand how it makes limiting the mempool harder? Do you mean that high priority transactions might accidentally be booted? That’s the problem that I’m saying exists regardless. If you boot by fee, then you might end up with high fee (but for some other reason unlikely to be mined) tx’s sticking around in your mempool forever. If you boot by time, then you might accidentally boot a high fee transaction. So I feel like we already need to put more thought into how we boot.

By adjusting the relayfee, I meant on the fly. Such as in this idea I got from @petertodd. The downside of that is that you might mine low fee transactions which made it in your mempool earlier, but reject higher fee transactions that come later. But I think it might be easier to reason about mempools that have a changing relay cutoff, than mempools that are booting things.

jgarzik commented at 4:40 pm on July 10, 2015: contributor

As I noted on IRC: I think the current multi-index patch is fine for the current codebase. It is most aligned with current code (fee, priority)

Thus concept ACK for as-is implementation.

sipa commented at 4:48 pm on July 10, 2015: member

@jtimon No, that’s just 3. I guess it was just a semantics discussion.

Put another way: (a) Do we need a separate fee area and priority area (1), or is a unified score sufficient (continue)? (b) Does the unified score need to be height dependent (2), or can it be computed only at mempool entrance (continue) (c) Does the only-once-computed score need to be policy dependent at all (3) or does only feerate suffice (4) ?

jtimon commented at 5:08 pm on July 10, 2015: contributor

Ok, so I think the best is 3: a unified score is sufficient and it can be computed only at mempool entrance, but it needs to be a function so that we can support different policies with different scores in the future (so nack 4).

sipa commented at 5:23 pm on July 10, 2015: member

Ok, suggested deployment:

Merge this (#6331) without the priority-based index (@ashleyholman, if you agree, can you remove the priority code here?)
Implement block size (as in #6281) limiting using this index, using actual usage stats (#6410).
Generalize the feerate here to a unified policy-dependent score (effectively removing the priority as it exists today, as in #6405).
Implement block creation using this score-based index.

This results at every step in a usable system without degradation.

morcos commented at 5:43 pm on July 10, 2015: member

@sipa ACK for suggested deployment plan.

jtimon commented at 6:14 pm on July 10, 2015: contributor

@sipa the overall plan doesn’t sound bad, but some points: For #6281 would CTxMemPool::removeOldTransactions work better if we introduce a time index here like morcos proposes?

Also, I think your third point “Generalize the feerate here to a unified policy-dependent score (effectively removing the priority as it exists today, as in #6405).” Can be divided in 2:

Replace the pure feerate index with the new unified index.
Stop using feerates and priorities and just use the new unified heuristics (as in #6405)

1 can be done directly in this PR. The new index is first unused, then used for the cap, then for almost anything else, then for block creation too. So CompareTxMemPoolEntryByFee would need to be replaced with something more generic, ideally, compatible with multiple policies. Instead of using CTxMemPoolEntry::GetFeeRate(), we could directly use something like GetTxPriority(const CTransaction&) that can be implemented in policy/policy.cpp [to be later replaced with the method CPolicy::GetTxPriority(const CTransaction&)].

I still don’t know how the comparator will get a const reference to a Policy without using the global directly though. But that will only be a problem when we want to turn the function into a method.

Merge remote-tracking branch 'BITCOIN/master' into mempool_multiindex 55bb08e5f8

ashleyholman force-pushed on Jul 11, 2015

Remove priority index. fad00fb74f

ashleyholman commented at 10:34 am on July 11, 2015: contributor

@sipa I have removed the priority index. The fee-rate index previously used priority as a secondary ordering, so I have now changed that to use nTime instead. Ie. when the fee-rate is equal, the index will favour older transactions. @jtimon I can add an index on time, but first I would like to understand the reasoning for CTxMemPool::removeOldTransactions. I’ve posted a comment in #6281 to clarify.

jtimon commented at 12:40 pm on July 11, 2015: contributor

@ashleyholman sure, that it’s something that could be always added later anyway (for example, in #6281 ).

What about replacing CompareTxMemPoolEntryByFee with something that calls an independent function instead of calling CTxMemPoolEntry::GetFeeRate() ? It can be functionally identical as what you have (ie: the independent function also looks returns the feerate), but it will make it easier to support multiple policies with different heuristic functions later (specially if you define the function in policy/policy.cpp). The new priority function can return just an integer instead of a double, and it should take const CTransaction& as parameter instead of a CTxMemPoolEntry like CTxMemPoolEntry::GetFeeRate() [as this “parameter”] IMO.

sipa commented at 12:45 pm on July 11, 2015: member

@jtimon You might as well just replace the feerate field in CTxMemPoolEntry with a score field, and call a policy function to compute the score when creating the entry, and then simply compare the scores. Alternatively, you definitely have to pass the CTxMemPoolEntry and not CTransaction, as you can’t compute the feerate from a CTransaction.

petertodd commented at 1:16 pm on July 11, 2015: contributor

Concept ACK @sipa’s unified priority ideas.

Done right this should make implementing CPFP reasonably easy, as the child can have a high priority, meaning “mine this tx, and whatever txs it depends on” Block creation then needs to be taught to to keep a list of already included txs; if it has a list of already spent txouts we can easily implement CPFP RBF scorched-earth by simply not removing double-spent txs immediately.

I did some work on that idea awhile back; didn’t have time/funding to finish it, but it seemed like a reasonable approach at least.

jtimon commented at 2:05 pm on July 11, 2015: contributor

@ashleyholman What I’m saying above, corrected and in code (on top of fad00fb7 ): https://github.com/bitcoin/bitcoin/commit/f86244ea960cb0a9fabf7c89886061034c46774b Also, I think https://github.com/bitcoin/bitcoin/pull/6331/files#diff-8304b3e94624036c3673f31eeb7e9de0R83 removes the need for the time index. @sipa Yes, there were some wrong things in what I said. I decided that it would be faster for me to communicate with code. Please, let me know what you think about that. Most of the arguments are not used, but they could be useful for some other policies. I believe this is the most policy-flexible thing we can do without disturbing anything.

sipa commented at 2:09 pm on July 11, 2015: member

@jtimon I fully agree that the sorting order should be something controlled by policy. I think there are more urgent issues than that now, though.

sipa commented at 2:10 pm on July 11, 2015: member

@ashleyholman See rebased version of this PR on top of #6410 inside #6421.

jtimon commented at 2:17 pm on July 11, 2015: contributor

@sipa The point is that my proposal is functionally equivalent and almost diff-free (compared to the total PR diff) if it is done in this PR. If it is not done in this PR, it can be done later with a less clean history and more work as additional costs. In terms of git history bike-shedding, this is the perfect time and place to do this, precisely because it’s almost free to do so (nothing is depending on the new index yet). Note that my 19+7- (compared to the PR, not as a delta(PR, PR+jtimon_nits) which is even smaller) proposal doesn’t have any dependencies besides this PR in itself. It is intended to be squashed in this PR or left for later. What is the disadvantage of squashing my suggestion?

sipa commented at 2:18 pm on July 11, 2015: member

@jtimon I’m sorry, but I have no time to deal with that right now.

ashleyholman commented at 2:43 pm on July 11, 2015: contributor

@jtimon looks good to me. The only thing I wonder about is the nHeight ordering which is still inside the comparator. I wonder if there’s a nice way to move that inside the policy as well. Maybe the comparator function has to be provided by the policy class?

BTW I’m not sure how to fetch your commit down into my local repository, because github isn’t telling me which repo/branch it’s from.

jtimon commented at 2:52 pm on July 11, 2015: contributor

Since @sipa is worried that introducing policy may slow this down, here’s an even simpler fixup commit ( https://github.com/bitcoin/bitcoin/commit/1bae6755ade6a4158477fcb19ef6af70f9704fd7 ) that would solve my concerns, which I will try to summarize. I’m fine with using the feeRate alone as the heuristics/score for the new index, but let’s not get married to it, we don’t know what the score will depend on in the future: all we know it’s that we want it to be calculated once and the information we have available in CTxMemPoolEntry’s constructor. Please, let’s calculate the feeRate score in an independent function that doesn’t make any additional assumptions. I don’t think I’m asking for that much (specially when it’s almost free total-diff-wise).

petertodd commented at 3:01 pm on July 11, 2015: contributor

Keep in mind that some of @sipa’s fixes may wind up getting backported to v0.10.x as a emergency hot fix, so it pays to keep them as minimal as possible.

On 11 July 2015 10:53:14 GMT-04:00, “Jorge Timón” notifications@github.com wrote:

Since @sipa is worried that introducing policy may slow this down, here’s an even simpler fixup commit ( https://github.com/bitcoin/bitcoin/commit/1bae6755ade6a4158477fcb19ef6af70f9704fd7 ) that would solve my concerns, which I will try to summarize. I’m fine with using the feeRate alone as the heuristics/score for the new index, but let’s not get married to it, we don’t know what the score will depend on in the future: all we know it’s that we want it to be calculated once and the information we have available in CTxMemPoolEntry’s constructor. Please, let’s calculate the feeRate score in an independent function that doesn’t make any additional assumptions. I don’t think I’m asking for that much (specially when it’s almost free total-diff-wise).

Reply to this email directly or view it on GitHub: #6331 (comment)

jtimon commented at 3:05 pm on July 11, 2015: contributor

@petertodd I believe my current suggestion is very minimal and portable (no less than the current PR, since it only adds a single-line function that can be easily pasted in previous versions). My suggestion isn’t actually not functionally equivalent but superior: the PR as it stands is ~~dividing~~multiplying the score by 1000 for no good reason.

petertodd commented at 3:07 pm on July 11, 2015: contributor

Sure, so make it a separate pull-req to make that easy to evaluate; easier to cherry pick if its separate.

On 11 July 2015 11:05:50 GMT-04:00, “Jorge Timón” notifications@github.com wrote:

@petertodd I believe my current suggestion is very minimal and portable (no less than the current PR, since it only adds a single-line function that can be easily pasted in previous versions). My suggestion isn’t actually not functionally equivalent but superior: the PR as it stands is dividing the score by 1000 for no good reason.

Reply to this email directly or view it on GitHub: #6331 (comment)

jtimon commented at 3:08 pm on July 11, 2015: contributor

@petertodd No, it is better to cherry pick this one if ashleyholman squashes my commit.

sipa commented at 3:09 pm on July 11, 2015: member

@jtimon The code in that commit looks perfectly fine to me, but I don’t think there’s a benefit to including it now.

jtimon commented at 3:14 pm on July 11, 2015: contributor

@sipa the only benefit is that it won’t have to be introduced in the future (when it won’t be free total-diff-of-this-PR-wise [well, it’s actually +4 for the new function but “just now” is as cheap as it can get diff-wise]). What is the disadvantage? That’s totally what I’m missing.

jtimon commented at 3:31 pm on July 11, 2015: contributor

IMO the question is why would CTxMemPoolEntry::feeRate ever need to be replaced by CTxMemPoolEntry::nScore if CTxMemPoolEntry::feeRate hasn’t been introduced yet, forget about the independent function if you want:

0    nScore = _nFee / nTxSize;

or even (to be fully-funcationally equivalent to the current PR)

0    nScore = _nFee * 1000 / nTxSize;

is enough for me: fully diff-free if included in this PR.

EDIT: @ashleyholman My latest proposal is ~~https://github.com/bitcoin/bitcoin/commit/a596abe48dd070385316c1cf711a63fa012dc66f~~ https://github.com/ashleyholman/bitcoin/compare/mempool_multiindex...jtimon:pr-6331-0.11.99 +7-7 with respect to your PR and +0-0 total-diff-wise. You tell me if what I’m asking is that crazy.

jtimon commented at 4:12 pm on July 11, 2015: contributor

Nits commit rebased.

ashleyholman commented at 9:25 pm on July 11, 2015: contributor

@jtimon I see your point, but I think that keeping each patch understandable and isolated is just as important as minimizing the lines changed.

This patch is more readable if it is just introducing an index on fee rate without trying to genericize the policy at the same time. A future patch could externalize the policy while introducing the score variable(s).

jtimon commented at 9:38 pm on July 11, 2015: contributor

@ashleyholman The code (that doesn’t contain anything related to policy) is here https://github.com/jtimon/bitcoin/commits/pr-6331-0.11.99 @sipa whatever call it nRevenueScore instead of nScore or even feeRate, but there’s not reason to use CFeeRate instead of int64_t. What is the disadvantage of using int64_t instead of CFeeRate. Or, what is the advantage of doing the opposite?

The main advantage of not using CFeeRate right now is not having to fix it later.

mikehearn commented at 12:23 pm on July 14, 2015: contributor

Priority cannot be removed without breaking things for current users and programs that rely on it. In particular, priority is the main reason that a transaction that undershoots on fee will still confirm eventually, which lends significant robustness. And priority is probably very useful for ensuring that at least some legitimate transactions confirm even if someone is burning fee money to flood the network with DoS transactions. Please do not remove it.

ashleyholman commented at 1:12 pm on July 14, 2015: contributor

This work has been rolled into #6421 so I’ll close this off now.

ashleyholman closed this on Jul 14, 2015

MarcoFalke locked this on Sep 8, 2021

MemPool: Convert mapTx to boost::multi_index_container #6331