-
Notifications
You must be signed in to change notification settings - Fork 36.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit mempool by throwing away the cheapest txn and setting min relay fee to it #6722
Conversation
mutable int64_t lastRollingFeeUpdate; | ||
mutable bool blockSinceLastRollingFeeBump; | ||
mutable double rollingMinimumFeeRate; //! minimum fee to get into the pool, decreases exponentially | ||
static const double ROLLING_FEE_HALFLIFE = 60 * 60 * 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
travis complains about a missing mutable bool blockSinceLastRollingFeeUpdate;
here.
7ef6af9
to
152dcb6
Compare
One thing that I think is maybe not great about the behavior of this, is let's say we have: TXs: If A and B are the min in the set, submitting C should kick them out. Now, let's say B wanted to increase their fee, they would need to go above 21 to get in. As implemented, it doesn't seem to me that two TX's could both raise by 1 to, combined, provide more fee (because it seems tx's get added one at a time?) Perhaps a better compromise between these two behaviors would be to have a two part mempool, the inclusion set and the to-be ousted set and trigger a "GC" with some frequency. The to be ousted-set can be RBF'd or something. Lastly justification on who might take advantage of such a behavior, perhaps a major exchange with a bunch of settlements out at once would want to make sure they all go through expediently and can coordinate increasing them all a hair. |
I think that my earlier comment is not fully needed, because mempool is a large multiple of block size, currently. Perhaps a more future proof implementation would allow setting:
|
|
||
if (expsize <= sizelimit) { | ||
BOOST_FOREACH(const txiter& it, stage) | ||
removeUnchecked(it); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't call this by itself anymore. Use removeStaged
@JeremyRubin No, you're right, this breaks relaying of child-pays-for-parent when mempool grows large (assuming the package is not already present). The easy solution is to allow fee calulation of packages together when processing orphans, and then you send your package in reverse-dependancy order. |
8cf2d4c
to
0ae4669
Compare
@TheBlueMatt re: my comment on high fee txs. I see now, you aren't doing the overall fee check in order to boot a package. I just assumed the StageTrimToSize logic was the same. So how do you think about free relay then? Could you write up a quick intro describing the algorithm as it would help to know how you think about it. Is the idea that all even though the tx causing the eviction hasn't covered the fees to pay for the evicted packages relay, by boosting the minRelayRate you're essentially forcing all future transactions to do so? It's an interesting idea, one question is how big a sweet spot there is between having the half-life too long and worrying about the "cram relayFee high all of a sudden" attack vs having it too low and perhaps having some vague concern about free relay. Why does your increased relay fee only apply to low priority transactions? I think it has to apply to all. |
@morcos see the description of the main commit: The minimum -maxmempool size is 10*-limitdescendantsize, as it is Note that this effectively disables high-priority transaction relay As for your specific questions: Yes, the idea is that you can relay some cheap crap for a bit, driving up the min relay fee by the default min relay fee each time (which was always meant as a "this is what it costs to send a transaction around the network" constant, though it hasn't always done a good job of being accurate there). The increased relay fee will effectively apply to low priority transactions, as they will be the package selected by the final TrimToSize call. Thus, priority-based relay will effectively remain enabled until people's mempools fill up. |
while (!todo.empty()) { | ||
const txiter& itnow = todo.front(); | ||
if (now.count(itnow)) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to pop_front() before continuing, otherwise its an infinite loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, oops...
But in particular the increased relay fee does NOT apply to high priority txs? That's what I don't understand. It seems you could use the same stable of high priority inputs over and over to gain free relay. |
777fea6
to
22d846f
Compare
Hmm, indeed, there is an attack there where you can cause lots of relay for free there. You cant really get much into the mempool (only up to the max package size) and you do have to increase the feerate each time, but only by one satoshi per kb... |
break; | ||
} | ||
txiter rootit = mapTx.project<0>(it.base()); | ||
rootit--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bug. rootit is an iterator by txid hash, so decrementing it puts you at a completely random transaction.
the base iterator needs to be decremented before projecting.
@sdaftuar and i didn't like this oddness, so the first commit in #6557 reverses the feerate sort. there was no reason to do it the other way in the first place. maybe you should just grab that?
What's wrong with XT's method of discarding a random transaction so that you can't predictably manipulate the mempool? |
5106010
to
ea64dfa
Compare
@NanoAkron It makes it trivial to DoS the network, among many other issues. |
7baa440
to
104c63d
Compare
|
||
if (expsize <= sizelimit) { | ||
RemoveStaged(stage); | ||
trackRemovedOrAddFailed(bestFeeRateRemoved); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions will be called every time through even if the mempool wasn't full to start with
I think the
From the point of view of the bandwidth attack: As soon as the Since the decay is exponential, you'll actually take a lot longer than 53 mins to repeat the attack if the prevailing fee rate multiple X is considerably less than 20. However as the prevailing fee rate climbs the attack could be considered a bigger concern. This should be addressed by having a default minimum relay rate that is higher. It seams reasonable that over the long term the default minimum relay rate will not be much less than 1/20th of the prevailing fee rate at the bottom of mempools. From the point of view of stuffing the mempool: In this case access to the network will be blocked for all txs less than 100k feerate for 5 hours while those transactions are mined anyway. The additional gain the Since the attacker could have stopped anything under 50K feerate anyway for 10 hours by just issuing 60MB worth of transactions at that fee rate. This attack is not significantly worse. So I think 12 hours strikes about the right balance. |
BOOST_FOREACH(const CTxIn& in, toadd.GetTx().vin) | ||
protect.insert(in.prevout.hash); | ||
|
||
size_t expsize = DynamicMemoryUsage() + toadd.DynamicMemoryUsage(); // Track the expected resulting memory usage of the mempool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about how much this is likely to matter but I don't think this is the best way to guess the expected size of the resulting mempool -- it misses the extra overhead from mapLinks, mapNextTx, and the multi_index pointers itself.
I think this code here is almost correct:
https://github.com/sdaftuar/bitcoin/blob/7008233767bd5e03521d96cde414394975e940d7/src/txmempool.cpp#L797
[There is an error though; the value of "9" that is used in the multi_index memory estimator should actually be a "6" I think in both DynamicMemoryUsage and GuessDynamicMemoryUsage.]
nTransactionsUpdated(0) | ||
{ | ||
clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change produces a crash on osx.
jonasschnelli$ ./src/bitcoind --regtest
libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::lock_error> >: boost: mutex lock failed in pthread_mutex_lock: Invalid argument
Stacktrace goes back to cxx_global_var_initXX()
.
I think calling LOCK()
from global var init (CTxMemPool mempool(::minRelayTxFee);
) through this new clear()
(which LOCKS mempool) is problematic.
this is going to be in v0.12.0? |
Yes, this should be added to the release-notes. |
2105947 Implement helper class for CTxMemPoolEntry constructor (Alex Morcos) 1cef905 Make -checkmempool=1 not fail through int32 overflow (Pieter Wuille) 0f72ff2 Support -checkmempool=N, which runs checks on average once every N transactions (Pieter Wuille) 89483d0 [Bug] Make operator() a const function in CompareTxMemPoolEntryByX (random-zebra) a50ad77 Lower default policy limits (random-zebra) 03f7152 fix locking issue with new mempool limiting (random-zebra) 1598961 Fix stale comment in CTxMemPool::TrimToSize. (random-zebra) 98d0d68 Undo GetMinFee-requires-extra-call-to-hit-0 (random-zebra) 6ad6ee6 Add reasonable test case for mempool trimming (random-zebra) 8dcbb7e Only call TrimToSize once per reorg/blocks disconnect (random-zebra) c20cd38 Implement on-the-fly mempool size limitation. (random-zebra) aee2e17 Print mempool size in KB when adding txn (random-zebra) f7c85fd Add CFeeRate += operator (random-zebra) 5bd2a00 Track (and define) ::minRelayTxFee in CTxMemPool (random-zebra) 0b50f6c Add Mempool Expire function to remove old transactions (random-zebra) d26f5e0 Fix calling mempool directly, instead of pool, in ATMP (random-zebra) fc5eddb Reverse the sort on the mempool's feerate index (random-zebra) 0ce1df0 [BUG] Fix CTxMemPool::check excluding zerocoins from children checks (random-zebra) 1f7bd52 Track transaction packages in CTxMemPoolEntry (random-zebra) 1fd406b TxMemPool: Change mapTx to a boost::multi_index_container (random-zebra) Pull request description: built on top of - [x] #1645 This PR pulls some updates from upstream in the mempool area, adding the required adjustments for legacy zerocoin txes and updating the functional test suite. Specifically, here we: - track mempool descendants (in-mempool transactions that depend on other mempool transactions) - turn `mapTx` into a `boost::multi_index_container` that sorts the mempool on 3 criteria: - transaction hash - fee rate - time in the mempool - Add a max size for the mempool (throwing away the cheapest txs and bumping the min relay fee, when full) - Implement on-the-fly mempool size limit with the flag `-maxmempool` - Implement `-checkmempool=N` to customize the frequency of the mempool check - Implement helper for `CTxMemPoolEntry` for the unit tests. Backports: - bitcoin#6654 - bitcoin#6722 [`*`] - bitcoin#6889 - bitcoin#6771 - bitcoin#6776 - bitcoin#6896 - bitcoin#7020 [`*`] excluding bitcoin@9e93640 as our default minimum tx fee rate of 10k satoshis is only 0,00003 USD at the time of writing. ACKs for top commit: Fuzzbawls: utACK 2105947 furszy: Re utACK 2105947 and merging this nice upgrade :) . Tree-SHA512: 51a7d75bd52f7646d461252c78f0dd9d7e8b5c1c66c22944120bfe293b28f5d48135de339ebf3d8a5b4c61ca5452383ed1b10c417be06dc4a335ac645842ea14
return CFeeRate(0); | ||
} | ||
} | ||
return std::max(CFeeRate(rollingMinimumFeeRate), minReasonableRelayFee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it limited to minReasonableRelayFee here, but on line 869, it isn't?
Did this end up being implemented? |
Current master crashes on OSX with an exception: "boost: mutex lock failed in pthread_mutex_lock: Invalid argument" (cherry picked from commit 0d699fc) Zcash: Also adds the `clear` call that this was fixing. Upstream added it in bitcoin/bitcoin#6722 which we never backported (instead implementing our own mempool limiting logic).
ZIP 239 preparations 2 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6722 - Only the ancillary commits, not the mempool limiting commits (we have our own). - bitcoin/bitcoin#6898 - Only the first three commits (we'll cherry-pick the main content later). - bitcoin/bitcoin#7840
ZIP 239 preparations 2 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6722 - Only the ancillary commits, not the mempool limiting commits (we have our own). - bitcoin/bitcoin#6898 - Only the first three commits (we'll cherry-pick the main content later). - bitcoin/bitcoin#7840
Tests forthcoming, but I felt bad I still hadnt pushed this.
See commitmsg for more details.