Remove UTXO cache entries when the tx they were added for is removed/does not enter mempool #6872
Conversation
| void CCoinsViewCache::Uncache(const uint256& hash) | ||
| { | ||
| CCoinsMap::iterator it = cacheCoins.find(hash); | ||
| if (it != cacheCoins.end() && it->second.flags == 0) |
There was a problem hiding this comment.
You can do slightly better:
if (it != cacheCoins.end() && (((it->second.flags & CCoinsCacheEntry::DIRTY) == 0) || ((it->second.flags & CCoinsCacheEntry::FRESH) && it->second.coins.IsPruned())))
This will also allow removing entries that were created and spent entirely within a cache.
There was a problem hiding this comment.
Such an entry should probably be removed without a call to Uncache, no?
There was a problem hiding this comment.
Uh, right, and it already is... never mind!
|
Do you feel like pulling in sipa@4186ac9 ? |
|
sipa@4186ac9 seems pretty nasty to me....completely clearing the cache when we add a transaction that pushes our cache over 90% usage seems like a really bad idea. |
|
@TheBlueMatt it's a last resort. If the cache size can grow outside of its
set bounds, we have to do something or there is no limit. Furthermore, it's
the same as what will happen anyway after the next block.
I considered it unacceptable on its own, as it would makr transaction
spamming result in more or less synchronized wiping of the cache across the
network, which could interfere with relay. Hopefully the rest of your patch
should reduce the risk of that.
|
|
Wiping the cache sounds like a dirty hack to me. I'd rather evict random elements until the cache is reasonably-sized than that. |
|
Also, we should fix it to not dump the entire cache after each new block. |
|
That, or some for of LRU or priority tracking in the coin cache would be a
significant improvement for sure.
But we need a means to limit memory usage at all costs IMHO, and this
solution is simple and doesn't worsen things on average, I think, as the
cache would be flushed anyway at the next block - potentially even in
between blocks in a reorg if the usage passed 100% by that time.
|
|
Sure, making the cache smarter would be nice, but lets not rush to fix it with a dirty hack when we're not even gonna ship the fix for a few months anyway. |
|
It's orthogonal. We need to enforce memory usage limits after each
transaction, so let's call the function to do that at that time.
How that function works is a different matter, and can be arbitrarily
improved later.
I don't understand your resistance here. I think it's the only solution.
|
|
Have you guys looked into how expensive all this work is? @sipa Once we've made it so that you can't bloat memory usage with txs that aren't accepted (as accomplished by this pull) then it seems to me the right approach is to just set your maxmempool size low enough that the unknown and variable multiple of that that is the size of your cache is also small enough for you. IMHO, the default dbcache size is too small. I'm with @TheBlueMatt that it seems bad to randomly wipe it unless we think that'll be very rare. I'd actually argue we might not even need to remove cache entries that are due to evicted tx's, because there is a relay cost that is paid to create those. |
| @@ -830,13 +842,17 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa | |||
| view.SetBackend(viewMemPool); | |||
|
|
|||
| // do we already have it? | |||
| if (view.HaveCoins(hash)) | |||
| if (view.HaveCoins(hash)) { | |||
| vHashTxnToUncache.push_back(hash); | |||
There was a problem hiding this comment.
If I understand correctly, then we might remove this transaction from the cache even though there may be in-mempool spends of it, right? I think we want some kind of check like you have in TrimToSize, where you only uncache if there's nothing in the mempool that spends it.
(Alternatively, if HaveCoins somehow reported whether any new caching happened, that might be helpful, but that seems like a harder change to make.)
|
Sure, if the eventual solution is to tweak FlushStateToDisk to be On 10/26/15 05:17, Pieter Wuille wrote:
|
|
I don't think it's worse than what we have. The cache will already be flushed on the next block. |
|
Depends on which you find worse, memory usage or DB I/O. Because its easy to optimize for loading lots of crap into the cache, an attacker can obviously tickle the cache to make it do more DB I/O with sipa@4186ac9, but can cause an equivalent increase in memory usage without. |
|
I think we should move to a model of thinking where exceeding some reasonable but fixed memory limit is equivalent in badness to crashing. People need to be able to rely on the memory usage to remain bounded. So if it's a choice between more I/O and that, I think the first is far better. |
|
I think thats a fine idea...except that we're so far away from it, it seems a bit premature to be optimizing for it now. |
|
concept ACK |
|
Observation: So when ATMT rejects due to rate limiting, the coincache entries pulled in don't seem to be removed. |
|
Observation: |
|
My observations are likely explained by this patch not updating coinsCachedUsage, so the pcoinsTip->DynamicMemoryUsage() going off the charts. |
|
New observation: |
|
Tested ACK. |
|
I'm a bit hesitant to merge this instead of a cleaner way of smart flushing and I'd still like to see some performance analysis. |
|
Without this, my mining node with an elevated relay fee (and running mempool limiting) and default dbcache is at 4GB RES. We do need to do more to control the size of the coins cache, if not precisely this change. |
|
I did some benchmarking on this pull. I built it on top of all the other improvements in #6976 except I tried it with and without the hot cache. It caused a significant performance boost to ConnectBlock. Unfortunately on my test days, 10/6 and 10/7 and using 100 M mempool and 200 M dbcache, the total cpu time shot up by over 10%. This seems to be entirely due to the 15kb very low fee transaction spam and increased time to reject these txs. I did not break down where the time was spent, but my guess is that it is not all in the uncaching code and that part of the problem is that these large txs often spent from the same prev hashes. So by uncaching them we cause more time to be spent just calculating the fee of new txs before we can reject them. Here is a summary of the results. I haven't fully reviewed the code yet, but I think this might be a performance hit we could tolerate.
|
|
Still needs rebase |
533ac3a to
85f0a08
Compare
|
Rebased, squashed, pulled in sipa@4186ac9 for 0.12. |
85f0a08 to
fab63f2
Compare
|
@TheBlueMatt Needs rebase. (Travis fail is unrelated: #6540) |
fab63f2 to
dd5862c
Compare
|
Rebased. Again. |
dd5862c Flush coins cache also after transaction processing (Pieter Wuille) bde953e Uncache input txn in utxo cache if a tx is not accepted to mempool (Matt Corallo) 97bf377 Add CCoinsViewCache::HaveCoinsInCache to check if a tx is cached (Matt Corallo) 677aa3d Discard txn cache entries that were loaded for removed mempool txn (Matt Corallo) b2e74bd Get the set of now-uncacheable-txn from CTxMemPool::TrimToSize (Matt Corallo) 74d0f90 Add method to remove a tx from CCoinsViewCache if it is unchanged (Matt Corallo)
|
Merged this because I want to branch off 0.12 soon, |
|
code review and tested for performance only ACK (I think most of us tested this before #5967 was merged, but I can't think why that should cause any problems) |
|
What was the justification for pulling in the flush commit? It seemed from the conversation that there was agreement of @TheBlueMatt and @morcos that it was not a good idea. |
|
It's important to limit cache at *some point* during normal operation, see gmaxwell's comment.
…On December 27, 2017 8:24:06 AM GMT+01:00, Ariel ***@***.***> wrote:
What was the justification for pulling in the flush commit? It seemed
from the conversation that there was agreement of @TheBlueMatt and
@morcos that it was not a good idea.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#6872 (comment)
|
…dded for is removed 2bf0de2 Flush coins cache also after transaction processing (Pieter Wuille) 48baa51 Uncache input txn in utxo cache if a tx is not accepted to mempool (random-zebra) c7070de Add CCoinsViewCache::HaveCoinsInCache to check if a tx is cached (Matt Corallo) 7d1836d Discard txn cache entries that were loaded for removed mempool txn (random-zebra) db35bd8 Get the set of now-uncacheable-txn from CTxMemPool::TrimToSize (Matt Corallo) 15e0321 Add method to remove a tx from CCoinsViewCache if it is unchanged (Matt Corallo) Pull request description: backporting bitcoin#6872 ACKs for top commit: furszy: Code review ACK 2bf0de2 . furszy: Tested ACK 2bf0de2. Fuzzbawls: Code ACK 2bf0de2 Tree-SHA512: 06145fcb5eba7b81fc1264ac3ff1fc22a7281c71c7ddf844a4a331055fb58bc8be384961951df68d46b259077f0a7aaf223ab11bab0f546665f6293f7ee7fb6d
…emaining Only return confirmed (not from mempool) outpoints in `pvNoSpendsRemaining`, as is the intended behaviour. With the previous code, removed chains of tx would have all internally-spent outpoints included in this vector, which is not the intended result. It seems to have been incorrectly coded from the very start (bitcoin#6872) but the bug is benign as this result is only used to uncache coins. As a bonus, no copying of CTransaction is needed now. An existing test case is modified to test this behaviour (the modified test fails under prior behaviour).
…emaining Only return confirmed (not from mempool) outpoints in `pvNoSpendsRemaining`, as is the intended behaviour. With the previous code, removed chains of tx would have all internally-spent outpoints included in this vector, which is not the intended result. It seems to have been incorrectly coded from the very start (bitcoin#6872) but the bug is benign as this result is only used to uncache coins. As a bonus, no copying of CTransaction is needed now. An existing test case is modified to test this behaviour (the modified test fails under prior behaviour).
…aining Only return confirmed outpoints in `pvNoSpendsRemaining`, as is the intended behaviour. With the current code, removed chains of tx will have all internally-spent outpoints included in this vector, which is not the intended result. It seems to have been incorrectly coded from the very start (bitcoin/bitcoin#6872) but the bug is benign as this result is only used to uncache coins. As a bonus, no copying of CTransaction is needed. An existing test case is modified to test this behaviour (the modified test will fail under prior behaviour).
No description provided.