Fix handling of all-txouts-unspendable transactions #3143

pull petertodd wants to merge 1 commits into bitcoin:master from petertodd:fix-all-unspendable-txs changing 1 files +29 −18

petertodd commented at 1:09 PM on October 24, 2013: contributor

Previously DisconnectBlock() would fail when undoing such transactions because it assumed all transactions will add at least one output to the UTXO set.

Be warned that I haven't tested this very well; I may be missing something.
sipa commented at 1:44 PM on October 24, 2013: member

I believe the issue and the solution is correct, but this patch seems over-complicated. I'll look at it later today.

As we do not have any releases with the pruning code enabled, this is not urgent.
gmaxwell commented at 2:19 PM on October 25, 2013: contributor

I think it's slightly urgent as my analysis was that this issue is fork forming, and people do mine on git code. (The conclusion I reached while investigating this was that it would cause reorgs to fail, though I haven't attempted a test)

Fix handling of all-txouts-unspendable transactions

Previously DisconnectBlock() would fail when undoing such transactions
because it assumed all transactions will add at least one output to the
UTXO set.

cbea442e6d

in src/main.cpp:None in 74f3320520 outdated
```
1755 | -            fClean = fClean && error("DisconnectBlock() : outputs still spent? database corrupted");
1756 | -            view.SetCoins(hash, CCoins());
1757 | +        // Remember that if every output of a transaction is unspendable the
1758 | +        // correct state for the UTXO set to be in is always to not have the
1759 | +        // transaction indexed at all.
1760 | +        if (!tx.AllOutputsUnspendable()) {
```
sipa commented at 6:57 PM on October 25, 2013:

Can you replace this by tx.IsPruned(), and see if things still work?

sipa commented at 7:00 PM on October 25, 2013:

Ugh, this is a CTransaction, not a CCoins of course. Never mind.

petertodd commented at 7:14 PM on October 25, 2013:

FWIW I dunno I really need to add that function to CTransaction itself; this seems like the only case where it would ever be used.

in src/main.cpp:None in 74f3320520 outdated

1756 | -            view.SetCoins(hash, CCoins());
1757 | +        // Remember that if every output of a transaction is unspendable the
1758 | +        // correct state for the UTXO set to be in is always to not have the
1759 | +        // transaction indexed at all.
1760 | +        if (!tx.AllOutputsUnspendable()) {
1761 | +            if (!view.HaveCoins(hash) & !tx.AllOutputsUnspendable()) {

sipa commented at 7:00 PM on October 25, 2013:

No need to call AllOutputsUnspendable twice, I think? (also, use && instead of &).

petertodd commented at 7:12 PM on October 25, 2013:

Ooops!

Yeah, that patch was a bit rushed...

petertodd commented at 10:25 AM on October 26, 2013: contributor

Fixed nits from @sipa
sipa commented at 10:33 AM on October 26, 2013: member

Alternative with less code changes: move outsBlock construction to before the test, and then use outsBlock.IsPruned() instead of the loop to check whether all outputs are unspendable.
petertodd commented at 10:36 AM on October 26, 2013: contributor

@sipa Note though I haven't actually changed much at all - all the lines changed are just due to an indent.
BitcoinPullTester commented at 10:39 AM on October 26, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cbea442e6d8fb2b447e14fd8a527d670f76b1e68 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.
sipa commented at 11:49 AM on October 26, 2013: member

@petertodd Sure, but it avoids adding the extra loop for detecting the all-prunable case (and the duplicate work that is associated with it)
Diapolo commented at 1:03 PM on October 26, 2013: none

Seems this is urgent, I had to reindex now 2 times on my mainnet wallet using current master and 5 times my testnet wallet.
sipa commented at 7:32 PM on October 26, 2013: member

@petertodd I sent a simpler pullreq (#3163) that should accomplish the same. Care to test it?
sipa commented at 7:37 PM on October 26, 2013: member

@Diapolo A reindex does not fix this (but that will let you temporarily avoid it); using -checklevel=2 is an easier workaround.
gmaxwell commented at 8:11 PM on October 26, 2013: contributor

A checklevel 2 is not really a workaround here, since we still can't disconnect the block if it ends up orphaned.
sipa commented at 8:14 PM on October 26, 2013: member

@gmaxwell An alternative to reindexing.

In case we need to reorganize away from such a block, there is indeed a problem. Though both this and #3163 should fix it.
petertodd commented at 4:34 AM on October 28, 2013: contributor

FWIW looks like #3163 is the better solution here.

Will close once that gets merged if nothing else comes up.
petertodd closed this on Oct 28, 2013
Bushstar referenced this in commit c182c6ca14 on Apr 8, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

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