Make check to distinguish between orphan txs and old txs more efficient. #10557
Conversation
src/validation.cpp
Outdated
There was a problem hiding this comment.
I believe this risks resulting in true for transactions that are in fact not yet confirmed and not in the mempool, but were just disconnected in a reorg. Is that something we care about? If not, perhaps add a comment to explain.
There was a problem hiding this comment.
Can we just change HaveCoinInCache() to have the same semantics as HaveCoin(), and only return true if the coin is unspent? Seems confusing that their semantics aren't the same.
For many of the existing usages of HaveCoinInCache(), we store the result to potentially Uncache() later -- but I don't think it's possible to ever have a spent coin in your cache that is able to be uncached (ie not marked DIRTY)?
This issue affects the existing usage of HaveCoinInCache() in AlreadyHave(), I think.
There was a problem hiding this comment.
@sipa I actually think that's likely a better outcome, but it may be too confusing. If a tx was just disconnected in a reorg, and no longer has one of its inputs available, it means it or its parent was probably double spent and so it shouldn't be in the orphan map anyway.
There was a problem hiding this comment.
Yes I like his suggestion as well.
I think this PR is "correct" as is, and I will make that change in a separate PR as a fix to HaveCoinsInCache?
|
utACK 301566d1cb49e7ef78d91e0eb86783deee1885db |
|
Needs rebase. |
Checking for the existence in the CCoinsViewCache of the outputs of a new tx will result in a disk hit for every output since they will not be found. On the other hand if those outputs exist already, then the inputs must also have been missing, so we can move this check inside the input existence check so in the common case of a new tx it doesn't need to run. The purpose of the check is to avoid spamming the orphanMap with slightly old txs which we have already seen in a block, but it is already only optimistic (depending on the outputs not being spent), so make it even more efficient by only checking the cache and not the entire pcoinsTip.
|
Simple rebase due to #10503 |
|
utACK 18bacec |
|
Please tag for 0.15 |
gmaxwell
left a comment
There was a problem hiding this comment.
utACK. "Duh"-- it should have always done this.
|
This will spam the orphan map somewhat after flushing, unfortunately-- but we can fix this by making flushing less hard in the future... and the orphan map is much less important than it used to be already. |
|
I still think we should take this for 15, but another thing to consider is to add a rolling bloom filter in net_processing ala TheBlueMatt@316516c (which is based on a big pile of changes, but could be pulled out separately), on top of this change. |
…s more efficient. 18bacec Make check to distinguish between orphan txs and old txs more efficient. (Alex Morcos) Tree-SHA512: b6b4bad89aa561975dce7b68b2fdad5623af5ebcb9c38fd6a72b5f6d0544ed441df4865591ac018f7ae0df9b5c60820cb4d9e55664f5667c9268458df70fd554
… old txs more efficient. 18bacec Make check to distinguish between orphan txs and old txs more efficient. (Alex Morcos) Tree-SHA512: b6b4bad89aa561975dce7b68b2fdad5623af5ebcb9c38fd6a72b5f6d0544ed441df4865591ac018f7ae0df9b5c60820cb4d9e55664f5667c9268458df70fd554
Checking for the existence in the CCoinsViewCache of the outputs of a new tx
will result in a disk hit for every output since they will not be found. On the
other hand if those outputs exist already, then the inputs must also have been
missing, so we can move this check inside the input existence check so in the
common case of a new tx it doesn't need to run.
The purpose of the check is to avoid spamming the orphanMap with slightly old
txs which we have already seen in a block, but it is already only optimistic
(depending on the outputs not being spent), so make it even more efficient by
only checking the cache and not the entire pcoinsTip.
@sipa this is my biggest feedback to #10195
For a single valid tx with 500 outputs in regtest on a machine with an SSD, the time to ATMP dropped from 4 ms to 2.4 ms.