Generalize the remove-outputs check for fully-prunable transactions. #3163

Instead of explicitly testing for the presence of any output, and dealing with this case specially, just interpret it as an empty CCoins.

The case previously caught using the HaveCoins check, is now handled by the generic outs != outsBlock test.

This fixes #3156 and replaces #3143.

Tested it myself. I can reproduce the failed disconnect at startup in master, but no problem appears with this patch.

I can also confirm that it makes the startup problem go away. Perhaps we should add a couple of test cases with these in a reorg in pulltester before we call it cured? (E.g. four transactions: 1 op_return, 2 op_return, 1 op_return and one regular, in both orders. Mine them, reorg them out, put them back in later.)

@gmaxwell PullTester doesn't seem to be running the comparison tool anymore.

Odd, I tried to reproduce the problem in git master with ./bitcoind -checkblocks=10500, which is more than enough to check the failing block, but I'm not seeing the error. Tried default checklevel, and checklevel=4 as well.

@petertodd not odd at all, the reorg test is memory limited and only goes as far back as the dbcache size allows. Effectively the checklevel is lower for blocks further back than that.

Ah. With -dbcache set appropriately I can reproduce the bug on master, and this pull-req fixes it.

I think this patch needs some comments explaining what's going on re: the immediately pruned case, but otherwise looks good. Maybe something like the following:

Check that all outputs are available and match the outputs in the block itself
exactly. Note that transactions with only provably unspendable outputs won't
have outputs available even in the block itself, so we handle that case
specially with outsEmpty.

Finally it'd be ideal if GetCoins() had an assert checking that the database never had an empty CCoins structure in it, although you'd need to either do this three times: CCoinsViewCache::GetCoins(), CCoinsViewDB::GetCoins() and CCoinsViewMemPool::GetCoins() or make a wrapper function in CCoinsView()

ACK... let's get this merged.

This fixes testnet for me.

@petertodd An empty CCoins cannot even be serialized - there is no storage format that corresponds to. Enough as a guarantee that it doesn't occur in the database? :)

Updated with peter's suggestions.

@sipa That's a pretty good guarantee!

ACK

Currently integrating this in a local build to also see if it helps :).

retested and re-ACK w/ updated equality check

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

ACK