Move TestBlockValidity into a background thread #7167

GBT now appears to return before TBV is complete.

This doesn't crash immediately from the use after free with the CValidationstate escaping its scope?

Didn't crash for me....odd...tests passed on OSX(which is what I tested the change on), but not other OS's should it have failed?

Hmm, so it appeared to crash sometimes, hopefully this should fix it.

You still have a race on block.

Yeah, I think I'm missing a lock, which lock does TBV need to be under?

So what will catch the runtime_error that is thrown if it fails? Previously the RPC code caught that and returned an JSON-RPC error, but the node kept running.

@morcos I changed it to an assert so that it should shutdown instead

Please gate this on an option off by default. On Dec 3, 2015 6:31 PM, "jameshilliard" notifications@github.com wrote:

@morcos https://github.com/morcos I changed it to an assert so that it should shutdown instead

— Reply to this email directly or view it on GitHub #7167 (comment).

@pstratem If it's off by default a miner could keep mining on invalid templates without realizing anything is wrong.

I'm asking for background vs inline to be selectable.

So you always call TestBlockValidity, the question just being when. On Dec 3, 2015 8:04 PM, "jameshilliard" notifications@github.com wrote:

@pstratem https://github.com/pstratem If it's off by default a miner could keep mining on invalid templates without realizing anything is wrong.

— Reply to this email directly or view it on GitHub #7167 (comment).

Well I moved TBV to be inside of getblocktemplate, the only tests that seem to be failing now are the ones that expect TBV to be inside of CNB.

I'm thinking the easiest way to do this is to copy the block generated by CNB for the TBV thread and use a modified version of TBV that doesn't depend on the cs_main lock, is it possible to test the block well enough without the cs_main lock?

There is no way you can do TBV without cs_main, as it needs to access the UTXO set.

@sipa does it do the check based on the current UTXO set only or is it possible for it to do the check based off a historical UTXO set? ie so the thread can be run in the background with a trylock waiting for GBT to release its lock?

@jameshilliard the only way to make the test act truly in parallel would be to re-implement the utxo database as a proper mvcc database...

@pstratem it doesn't need to be parallel so much as not block CNB/GBT

And not block with other calls to ProcessNewBlock.

What types of failures are we worried about in regards to CNB? Would it be reasonably safe to skip checks that depend on the UTXO set?

If you skip checks based on the UTXO set, it cannot do anything. It can't do double-spend detection, script evaluation, fee/subsidy calculations, ...

The new CNB code is already intended to never ever produce an invalid block. TBV is a last-resort validation to be sure the code is working right. If you're going to skip checks, it is no longer useful; you could just as well not do any checks at all.

So, no, you need cs_main. And you need to make sure no ProcessBlock happened in between either.

@sipa Yeah, I wasn't sure if we are trying to protect against mempool errors or more just formatting errors.

Closing, as this is not a viable strategy.

I think there are alternatives possible (like not running TBV for every block, or running TBV while still holding cs_main, before after returning from the RPC), but maybe it's just much less needed with the much faster 0.12 CNB?