[Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction #7062

Merged
merged 4 commits into from Dec 21, 2015

Conversation

Projects
None yet
5 participants
@sdaftuar
Member

sdaftuar commented Nov 19, 2015

This fixes the behavior of the mempool limiting to respect any fee deltas applied via PrioritiseTransaction.

(Note that the first 5 commits here are introduced by @morcos in #6898.)

It also includes a couple other fixes relating to PrioritiseTransaction:

  • Updates RBF logic
  • Updates mempool acceptance logic

@morcos morcos referenced this pull request Nov 19, 2015

Merged

Rewrite CreateNewBlock #6898

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 19, 2015

Member

This will create merge conflicts for #6871 -- let's try to get that reviewed and merged first, and I can resolve the merge conflicts in this PR afterward.

Member

sdaftuar commented Nov 19, 2015

This will create merge conflicts for #6871 -- let's try to get that reviewed and merged first, and I can resolve the merge conflicts in this PR afterward.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2015

Contributor

Note that this leaks information about what transactions you've prioritised to the outside world even more so than before. Probably unavoidable right now, but in the long run it'd be better if we had a separate mempool for mining that kept priority deltas. Transactions themselves can be shared across both mempools and reference counted.

Contributor

petertodd commented Nov 20, 2015

Note that this leaks information about what transactions you've prioritised to the outside world even more so than before. Probably unavoidable right now, but in the long run it'd be better if we had a separate mempool for mining that kept priority deltas. Transactions themselves can be shared across both mempools and reference counted.

@petertodd

View changes

src/rpcblockchain.cpp
" \"time\" : n, (numeric) local time transaction entered pool in seconds since 1 Jan 1970 GMT\n"
" \"height\" : n, (numeric) block height when transaction entered pool\n"
" \"startingpriority\" : n, (numeric) priority when transaction entered pool\n"
" \"currentpriority\" : n, (numeric) transaction priority now\n"
" \"descendantcount\" : n, (numeric) number of in-mempool descendant transactions (including this one)\n"
" \"descendantsize\" : n, (numeric) size of in-mempool descendants (including this one)\n"
- " \"descendantfees\" : n, (numeric) fees of in-mempool descendants (including this one)\n"
+ " \"descendantfees\" : n, (numeric) modified fees of in-mempool descendants (including this one)\n"

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

The term "modified fees" won't mean anything to the reader. How about "fees of in-mempool descendants (including this one) with priority deltas applied"

@petertodd

petertodd Nov 20, 2015

Contributor

The term "modified fees" won't mean anything to the reader. How about "fees of in-mempool descendants (including this one) with priority deltas applied"

This comment has been minimized.

@sdaftuar

sdaftuar Nov 24, 2015

Member

I was trying to make a reference to the term "modifedfee" which I added above without having to redefine the concept, what if I just do: "modified fees (see above) of in-mempool descendants (including this one)"

@sdaftuar

sdaftuar Nov 24, 2015

Member

I was trying to make a reference to the term "modifedfee" which I added above without having to redefine the concept, what if I just do: "modified fees (see above) of in-mempool descendants (including this one)"

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2015

Contributor

Otherwise, utACK, though will want to recheck rebased version once #6871 is merged of course.

Contributor

petertodd commented Nov 20, 2015

Otherwise, utACK, though will want to recheck rebased version once #6871 is merged of course.

@sdaftuar sdaftuar changed the title from [Mempool] Fix mempool limiting for PrioritiseTransaction to [Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction Nov 30, 2015

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 30, 2015

Member

Rebased.

@petertodd This pull has now been updated to use fee deltas for the replace-by-fee calculation, and the replace-by-fee rpc test has been updated to exercise that logic. (I cherry-picked #7137, so if that gets merged first I'll rebase this one, or I can close that pull if that's easier.)

Member

sdaftuar commented Nov 30, 2015

Rebased.

@petertodd This pull has now been updated to use fee deltas for the replace-by-fee calculation, and the replace-by-fee rpc test has been updated to exercise that logic. (I cherry-picked #7137, so if that gets merged first I'll rebase this one, or I can close that pull if that's easier.)

@petertodd

View changes

qa/rpc-tests/replace-by-fee.py
+ # the transactions we needed mined were mined, and
+ # don't worry about what is left.
+ break
+ mempool_size = new_size

This comment has been minimized.

@petertodd

petertodd Dec 1, 2015

Contributor

FYI, I've found bugs before by erroring out if any txs are stuck; not sure this is a good idea.

@petertodd

petertodd Dec 1, 2015

Contributor

FYI, I've found bugs before by erroring out if any txs are stuck; not sure this is a good idea.

This comment has been minimized.

@sdaftuar

sdaftuar Dec 1, 2015

Member

Fair enough -- I ran into an issue where the really large transaction created in one of the tests was too big to mined, but I should be able to rework that test and make this check error out instead.

@sdaftuar

sdaftuar Dec 1, 2015

Member

Fair enough -- I ran into an issue where the really large transaction created in one of the tests was too big to mined, but I should be able to rework that test and make this check error out instead.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 1, 2015

Member

Rebased and changed rpc test to error out if the mempool ends up with stuck transactions.

Member

sdaftuar commented Dec 1, 2015

Rebased and changed rpc test to error out if the mempool ends up with stuck transactions.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 1, 2015

Member

Rebased now that #6898 is merged

Member

sdaftuar commented Dec 1, 2015

Rebased now that #6898 is merged

@laanwj laanwj added this to the 0.12.0 milestone Dec 2, 2015

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 2, 2015

Member

Updated so that now mempool acceptance is also determined by including a transaction's fee delta from PrioritiseTransaction.

In doing so, I realized that GetMinRelayFee is not very useful, so I scrapped it. With the DEFAULT_BLOCK_PRIORITY_SIZE set to 0, this function was stopping all free transactions from making it in (unless they were prioritised using PrioritiseTransaction). Scrapping it means that there is a behavior change from before, where now we will allow in transactions between 50kb and 100kb that are free as long as they have sufficient priority. Given that when the mempool is full we don't allow free transactions at all, and given that we expire things after 3 days, I think this should be okay.

EDIT: Actually, there's a bug in GetMinRelayFee so that the comparison with DEFAULT_BLOCK_PRIORITY_SIZE was incorrect; the comparison is comparing two things as unsigned int's, but the right hand side is negative... So this test isn't preventing anything from getting into the mempool now that the DEFAULT_BLOCK_PRIORITY_SIZE is 0.

if (nBytes < (DEFAULT_BLOCK_PRIORITY_SIZE - 1000))      
        nMinFee = 0;
Member

sdaftuar commented Dec 2, 2015

Updated so that now mempool acceptance is also determined by including a transaction's fee delta from PrioritiseTransaction.

In doing so, I realized that GetMinRelayFee is not very useful, so I scrapped it. With the DEFAULT_BLOCK_PRIORITY_SIZE set to 0, this function was stopping all free transactions from making it in (unless they were prioritised using PrioritiseTransaction). Scrapping it means that there is a behavior change from before, where now we will allow in transactions between 50kb and 100kb that are free as long as they have sufficient priority. Given that when the mempool is full we don't allow free transactions at all, and given that we expire things after 3 days, I think this should be okay.

EDIT: Actually, there's a bug in GetMinRelayFee so that the comparison with DEFAULT_BLOCK_PRIORITY_SIZE was incorrect; the comparison is comparing two things as unsigned int's, but the right hand side is negative... So this test isn't preventing anything from getting into the mempool now that the DEFAULT_BLOCK_PRIORITY_SIZE is 0.

if (nBytes < (DEFAULT_BLOCK_PRIORITY_SIZE - 1000))      
        nMinFee = 0;
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 2, 2015

Member

utACK (modulo comment above)

Member

morcos commented Dec 2, 2015

utACK (modulo comment above)

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 2, 2015

Member

Addressed @morcos' nit (and squashed into first commit).

Member

sdaftuar commented Dec 2, 2015

Addressed @morcos' nit (and squashed into first commit).

sdaftuar added some commits Nov 19, 2015

Fix mempool limiting for PrioritiseTransaction
Redo the feerate index to be based on mining score, rather than fee.

Update mempool_packages.py to test prioritisetransaction's effect on
package scores.
Remove GetMinRelayFee
One test in AcceptToMemoryPool was to compare a transaction's fee
agains the value returned by GetMinRelayFee. This value was zero for
all small transactions.  For larger transactions (between
DEFAULT_BLOCK_PRIORITY_SIZE and MAX_STANDARD_TX_SIZE), this function
was preventing low fee transactions from ever being accepted.

With this function removed, we will now allow transactions in that range
with fees (including modifications via PrioritiseTransaction) below
the minRelayTxFee, provided that they have sufficient priority.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 21, 2015

Member

utACK

Member

laanwj commented Dec 21, 2015

utACK

@laanwj laanwj merged commit 901b01d into bitcoin:master Dec 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 21, 2015

Merge pull request #7062
901b01d Remove GetMinRelayFee (Suhas Daftuar)
27fae34 Use fee deltas for determining mempool acceptance (Suhas Daftuar)
9ef2a25 Update replace-by-fee logic to use fee deltas (Suhas Daftuar)
eb30666 Fix mempool limiting for PrioritiseTransaction (Suhas Daftuar)

laanwj added a commit that referenced this pull request Dec 21, 2015

[Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTrans…
…action

1) Fix mempool limiting for PrioritiseTransaction

Redo the feerate index to be based on mining score, rather than fee.

Update mempool_packages.py to test prioritisetransaction's effect on
package scores.

2) Update replace-by-fee logic to use fee deltas

3) Use fee deltas for determining mempool acceptance

4) Remove GetMinRelayFee

One test in AcceptToMemoryPool was to compare a transaction's fee
agains the value returned by GetMinRelayFee. This value was zero for
all small transactions.  For larger transactions (between
DEFAULT_BLOCK_PRIORITY_SIZE and MAX_STANDARD_TX_SIZE), this function
was preventing low fee transactions from ever being accepted.

With this function removed, we will now allow transactions in that range
with fees (including modifications via PrioritiseTransaction) below
the minRelayTxFee, provided that they have sufficient priority.

Github-Pull: #7062
Rebased-From: eb30666 9ef2a25 27fae34 901b01d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment