Add CMutableTransaction and make CTransaction immutable. #4309

This is an alternative to #4283 and #4292.

It introduces a CMutableTransaction (which is just a struct, and only has GetHash() as utility method (which does not cache), next to CTransaction (which becomes immutable with a cached hash).

To keep the diff small, there are implicit conversions between CMutableTransaction and CTransaction. That may lead to unexpected low performance perhaps, but CMutableTransaction is only ever used in places that don't require performance (block building, wallet, tests); in particular, note that there is no CMutableTransaction anywhere in main.

Ping @pstratem.

This effectively reduces duplicate calls to CTransaction::GetHash() to zero.

There are a few duplicates but I believe those to be the coinbase transactions which are actually identical.

I have not reviewed for correctness.

Actually those appear to be something to do with the genesis block.

Either way it's a trivial number of duplicate calls.

Added a commit that removes all function arguments that were hashes of a CTransaction that is already passed, resulting in simpler code in many places.

Very nice!

Spiffy! I like it! Untested ACK

@pstratem What were your benchmark results? I remember something like 17% faster reindex to block 180k or so?

@sipa benchmarks show a ~15% faster reindex to block 150k

I've been testing this on my nodes for ~2 weeks with no problems.

ACK

Code reviewed and some testing.

ACK.

@gavinandresen Can you make some space for pulltester?

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4309_d38da59bf68fbb37535e2579bfb7355a16baed0e/ 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.

This change seems unnecessary? As a grep of all CMutableTransaction uses helpfully highlights, there is never a case where a TX is accessed in parallel or stored in a parallel-access container while also being mutated.

As such, you may apply the common "I'm done mutating this data structure, update cached data" pattern without needing a new class.

ETA: I'm posting this on a closed issue because I'm updating the bitcoin-tx code for this change :)

 - CTransaction tx;
 - DecodeHexTx(tx, strHexTx);
 + CTransaction txDecodeTmp;
 + DecodeHexTx(txDecodeTmp, strHexTx);
 + CMutableTransaction tx(txDecodeTmp);