Bugfix: RPC/Mining: getblocktemplate: Delay updating nTransactionsUpdatedLast and time_start until after the new template is cached

[luke-jr]

This is needed to avoid parallel calls from incorrectly using the stale cached template or (after an OOM) dereferencing a nullptr pblocktemplate

Considering memory overcommit, the worst case scenario likely never occurs, but that possibility is the clear bugfix here.

Aside from that, I wonder if using the stale cache in this race scenario (which is what could happen if this isn't fixed) is better than having N threads all making a new block (with this fix).

A middle ground (and perhaps cleaner than a bunch of static variables) might be to take a lock and have racing requests all wait on the same block creation, but that is not implemented here.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

[cbergqvist]

A unique lock on cs_main is taken on line 605. It is temporarily released if (!lpval.isNull()), but re-locked again before exiting that if-block, well before the block with the static variables around line 740 and the rest of the function body. So only one thread should be executing that section at a time.

That would mean having a null pindexPrev should be enough for the null pblocktemplate to be recalculated after an OOM. But maybe something less obvious is happening here?

[luke-jr]

Indeed, forgot about cs_main