Avoid duplicate transaction hashes during validation #4283

pull sipa wants to merge 2 commits into bitcoin:master from sipa:nodoublehash changing 7 files +51 −42

sipa commented at 10:49 PM on June 3, 2014: member

No description provided.
jgarzik commented at 10:53 PM on June 3, 2014: contributor

Untested ACK.

Style nit: lack of spaces in 'for' loop declarations
pstratem commented at 10:53 PM on June 3, 2014: contributor

I haven't reviewed this patch, but I can confirm this is likely a significant issue in the reindexing time.

Apparently approximately 80% of calls to CHashWriter and 90% of calls to Hash, are for data that has already been hashed.
sipa commented at 10:58 PM on June 3, 2014: member

@jgarzik Fixed.
sipa commented at 11:20 PM on June 3, 2014: member

Bugfix + removed some duplicate hashes in wallet too.
sipa commented at 12:00 AM on June 4, 2014: member

Fixed tests.
Avoid duplicate transaction hashes during validation 14055d8a3f
Remove some duplicate transaction hashes in wallet 93655f2e2b
BitcoinPullTester commented at 4:53 AM on June 4, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/93655f2e2b8122e11b86f5c63ab130e536fb4e56 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.
in src/main.cpp:None in 93655f2e2b
```
1980 | @@ -1980,21 +1981,22 @@ bool static DisconnectTip(CValidationState &state) {
1981 |      if (!WriteChainState(state))
1982 |          return false;
1983 |      // Resurrect mempool transactions from the disconnected block.
1984 | -    BOOST_FOREACH(const CTransaction &tx, block.vtx) {
1985 | +    for (unsigned int i = 0; i < block.vtx.size(); i++) {
```
laanwj commented at 6:28 AM on June 4, 2014:

I like the BOOST_FOREACH better, especially as it can be changed to the new for constucts when we introduce C++11 support. Going to int-based instead of iterator-based iteration seems a step back instead of forward.

sipa commented at 7:34 AM on June 4, 2014:

I like the BOOST_FOREACH better, especially as it can be changed to the new for constucts when we introduce C++11 support. Going to int-based instead of iterator-based iteration seems a step back instead of forward.

Sure, where possible I also prefer iterator based approaches. But in order to use block.GetTxHash(int) we need to know the index of the transaction we're accessing.

laanwj commented at 7:35 AM on June 4, 2014:

OK, very good point. I was afraid for a moment that you were going to change all of them :)
laanwj commented at 7:38 AM on June 4, 2014: member

ACK on the idea

I still think it'd make sense to cache the hash in the transaction structure. This would make GetHash() virtually free, and would allow similar speedups without as much code changes.
sipa commented at 9:27 AM on June 9, 2014: member

Closing in favor of #4309.
sipa closed this on Jun 9, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4292 Cache hashes in CTransaction and CBlock #4309 Add CMutableTransaction and make CTransaction immutable.