Avoid repeated lookups in mapOrphanTransactions and mapOrphanTransactionsByPrev #4873

As it says on the tin.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4873_89d91f6aa74da5e3fb1bbd08ce20ab3b9d593abd/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Tested ACK.

Untested ACK

ACK.

Bah: did some more testing today with MAX_ORPHAN_TRANSACTIONS = 4 and got a core dump.

I'm running with this fix; so far so good:

diff --git a/src/main.cpp b/src/main.cpp
index 2e24eb9..10f9ba9 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -498,6 +498,8 @@ void static EraseOrphanTx(uint256 hash)
     BOOST_FOREACH(const CTxIn& txin, it->second.vin)
     {
         map<uint256, set<uint256> >::iterator itPrev = mapOrphanTransactionsByPrev.find(txin.prevout.hash);
+        if (itPrev == mapOrphanTransactionsByPrev.end())
+            continue;
         itPrev->second.erase(hash);
         if (itPrev->second.empty())
             mapOrphanTransactionsByPrev.erase(itPrev);

@gavinandresen Ugh, that's not good. That means the orphan pool is inconsistent.

Good sanity check to add, but indeed in principle it shouldn't happen.

If we don't understand why the orphan pool is getting inconsistent, I think we should revert instead of patching it up (an assert would be fine, though).

We have no reason to think that this is caused by this change. Before, this problem would not have been caught (or have resulted in a crash) because the record was created every time.

mapOrphanTransactionsByPrev[txin.prevout.hash].erase(hash);

So all that @gavinandresen's patch would do is restore the old behavior. But indeed, that would ignore the inconsistency.

Looking at this more closely, I'm confident my proposed fix is correct.

Scenario 1 that tickles the bug: Orphan transaction that spends two inputs of the same transaction. I think two orphan transactions that spend the same output of a parent transaction would also cause the crash.

The memory pool is consistent-- mapOrphanTransactions/mapOrphanTransactionsByPrev is just a big bag of transactions that can be inconsistent (may contain double-spends or invalid transactions), and that is OK, transactions are checked before moving from mapOrphanTransactions into the memory pool.

By inconsistent I just meant a discrepance between mapOrphanTransactions and mapOrphanTransactionsByPrev. But indeed, as they're identified by txid and not txid:index, a transaction spending two outputs from the same transactions does explain it. ACK on the change.