Prune provably-unspendable outputs #2791
Conversation
|
Tested by running a -reindex on mainnet. |
|
What kind of effect does this have on the blockchain size? |
|
@Mazo None. The blockchain continues to store every single transaction, from 2009 through eternity. |
|
It looks good to me. Are there any unit tests for blocks and UTXO set changes? If so it'd be good to add a test for this. |
|
/me votes new tests be added to https://code.google.com/p/bitcoinj/source/browse/core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java |
|
How would you test it? There is no observable difference. You can try to create OP_RETURN outputs and try to spend them, and see that fails, but that's true before and after this PR (which doesn't mean it's a useless test). |
|
Well, yes, testing this properly probably cant be done out-of-process (maybe over rpc, though), but adding some OP_RETURN scripts to the block test-set (and some that look semi-unspendable, eg OP_RETURN in an IF) would be really nice. |
|
@sipa @TheBlueMatt I added tests for attempting to spend OP_RETURNS, including with IF's and similar, to the unit tests actually. I agree there should be tests to ensure they don't end up in the UTXO set though. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ec84e81e8383b3b1e1ef4a6dbcb088193d8de5d7 for binaries and test log. |
|
@petertodd @TheBlueMatt I agree we need tests to verify that such unspendable outputs don't end up in the UTXO set, but I disagree it should be part of pulltester. This does not affect network interaction, but is a client-side optimization. |
|
The point of adding it to pull-tester is that if it is done wrong, it is network interaction. Pieter Wuille notifications@github.com wrote:
|
|
@TheBlueMatt Just to be clear: I'm all for adding tests to pulltester intended to trigger edge cases related to handling of provably-unspendable outputs. I'm just not in favor of making pulltester use more than the P2P interface for testing (i.e., making it a whitebox rather than blackbox test) - tests for checking that unspendable outputs do not end up in the UTXO set are for unit tests, not for network interaction testing. |
|
Over the network how would you know if a peer wasn't doing OP_RETURN pruning properly anyway if their script implementation was correct? |
|
@petertodd That's my point - anyone in the network only cares whether your script/verification implementation is correct. Only you care about whether it's not using more storage than necessary. I prefer the block-acceptance tests to remain implementation-independent, so it can remain as generic as possible, and be used to find bugs that could lead to non-convergence - not only for bitcoind/bitcoin-qt. If another implementation chooses not to implement pruning, that's their choice. Sure, it would change the economics of the system as a whole, but it's not a network rule violation to do so. Of course, if a some form of committed-UTXO-set-in-coinbase is ever added, the actual UTXO set becomes observable to the network rules, and this changes of course. We need unit tests for verifying that provably-unspendable outputs do not end up in the UTXO set though. |
|
ACK |
|
So this will make the gettxoutsetinfo on existing nodes diverge until everyone does a -reindex. Likewise, gettxout will return different results. Is this a problem? One possibility would be to write a small piece of code that checks at startup to see if a particular OP_RETURN output is in the UTXO set, and if it is, traverses the set to remove all of them. At a minimum there should likely be a comment stuck someplace in the code to remind us if we ever make the utxo set normative that we need traverse it and prune these transactions first. |
|
@gmaxwell sounds like a good idea to me. The canary txout should be the first op_return in the chain of course. |
|
Oh, and don't forget test net... |
|
How about only enabling this when a "prune_op_return" flag is set in the database, which can only be set/changed at initial creation/reindex, and outputting this flag as part of gettxoutsetinfo? |
|
@sipa ACK. Definitely output the flag via gettxsetinfo. |
|
@sipa A flag sounds like less work, and few people would be affected, so go ahead and do it that way. The important thing is to give users an understanding of why two different UTXO hashes don't match. Maybe add a UTXO version number, and just increment it every time we change something? IMO it's fine to have a meta version -1 that gets set if you run some dev code that puts the UTXO database in a bad state or have a flag called "utxo-consistent" that is unset in that case. |
|
Either a version number or simply list of flags that might permute the output. |
|
Perfect is the enemy of good. I think the inconsistent values here can be resolved by anyone who wants to create UTXO tree hashes for proofs, and otherwise by reindexing. I think we should just take this as is. |
|
Let's get this merged. |
Prune provably-unspendable outputs
…in#2791) * Bump MAX_OUTBOUND_MASTERNODE_CONNECTIONS to 250 on masternodes Masternodes now need to connect to much more other MNs due to the intra-quorum communication. 250 is a very conservative value loosely based on the absolute worst-case number of outgoing connections required, assuming that a MN manages to become part of all 24 active LLMQs. * Fix infinite loop in CConnman::Interrupt * Move out conditional calc into it's own variable Co-Authored-By: codablock <ablock84@gmail.com>
Automatically prune outputs (at creation) which are provably unspendable.