/bitcoin

Make check to distinguish between orphan txs and old txs more efficient. #10557

Merged
merged 1 commit into from Jul 14, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+8 −12
Split
View
20 src/validation.cpp
@@ -491,24 +491,20 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
CCoinsViewMemPool viewMemPool(pcoinsTip, pool);
view.SetBackend(viewMemPool);
- // do we already have it?
- for (size_t out = 0; out < tx.vout.size(); out++) {
- COutPoint outpoint(hash, out);
- bool had_coin_in_cache = pcoinsTip->HaveCoinInCache(outpoint);
- if (view.HaveCoin(outpoint)) {
- if (!had_coin_in_cache) {
- coins_to_uncache.push_back(outpoint);
- }
- return state.Invalid(false, REJECT_DUPLICATE, "txn-already-known");
- }
- }
-
// do all inputs exist?
for (const CTxIn txin : tx.vin) {
if (!pcoinsTip->HaveCoinInCache(txin.prevout)) {
coins_to_uncache.push_back(txin.prevout);
}
if (!view.HaveCoin(txin.prevout)) {
+ // Are inputs missing because we already have the tx?
+ for (size_t out = 0; out < tx.vout.size(); out++) {
+ // Optimistically just do efficient check of cache for outputs
+ if (pcoinsTip->HaveCoinInCache(COutPoint(hash, out))) {
@sipa

sipa Jun 8, 2017 • edited

Owner

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.

@sdaftuar

sdaftuar Jun 8, 2017 • edited

Contributor

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.

@morcos

morcos Jun 8, 2017

Contributor

@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.

@sipa

sipa Jun 8, 2017

Owner

@morcos Do you think it's worth the complexity? I like @sdaftuar's suggestion.

@morcos

morcos Jun 8, 2017

Contributor

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?

@sipa

sipa Jun 8, 2017

Owner

Ok.
+ return state.Invalid(false, REJECT_DUPLICATE, "txn-already-known");
+ }
+ }
+ // Otherwise assume this might be an orphan tx for which we just haven't seen parents yet
if (pfMissingInputs) {
*pfMissingInputs = true;
}