Move TestBlockValidity into a background thread

[jameshilliard]

GBT now appears to return before TBV is complete.

[gmaxwell]

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

[jameshilliard]

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?

[jameshilliard]

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

[luke-jr]

You still have a race on block.

[jameshilliard]

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

[morcos]

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.

[jameshilliard]

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

[pstratem]

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).

[jameshilliard]

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

[pstratem]

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).

[jameshilliard]

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.

[jameshilliard]

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?

[sipa]

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

[jameshilliard]

@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?

[pstratem]

@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...

[jameshilliard]

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

[sipa]

And not block with other calls to ProcessNewBlock.

[jameshilliard]

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?

[sipa]

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, ...

[sipa]

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.

[jameshilliard]

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

[sipa]

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?