Lock cs_main for TestBlockValidity

[jmjatlanta]

Fixes issue #483

The BitcoinMiner() did not properly lock cs_main before using chainActive or calling TestBlockValidity.

I have verified that all other calls to TestBlockValidity maintain the lock on cs_main before the call.

[dimxy]

I agree with this fix.
Actually I have already added this lock in my experimental branches. I did load tests with and without it and got -25 mempool errors when the lock omitted

[DeckerSU]

Actually CChain::LastTip() is kind of a hack introduced in our codebase few years ago (in 2018), all Bitcoin-based coins using CChain::Tip() to get the index entry for the last tip of chain. So, my offer is to follow the Bitcoin codebase:

• get rid of this CChain::LastTip() and change all LastTip() calls on Tip().
• don't try to introduce new methods, like GetLastTipWithLock(), but just use LOCK(cs_main) instead where needed.

It will be more clear and make the codebase following bitcoin upstream at the same time. Actually we have a lot of things which are implemented in Bitcoin, but absent in Komodo ... if you'll look on ZCash codebase - they trying to merge Bitcoin upstream step-by-step ... my opinion - we should do the same first: try to take the best from Bitcoin upstream and minimize introducing
fixes like GetLastTipWithLock().

p.s. All of above is just my personal opinion.

[dimxy]

maybe let's now replace LastTip everywhere (it is really confusing)
not use GetLastTipWithLock and run more tests with DEBUGLOCKORDER on later
but add now LOCK(cs_main) for TestBlockValidity: I believe running it without a lock is dangerous for critical code

[dimxy]

maybe this function GetLastTipWithLock() is not needed as we agreed not to use it

[dimxy]

the conflict needs to be resolved

[jmjatlanta]

Conflict resolved.

[dimxy]

I am running my NN on this code, seems this code is okay.
I have also code in succession for this PR with more refactoring of mining code: use LOCK2 in CreateNewBlock, block connecting code moved out from validBlock(), elimination of extra call to TestLockValidity - still in testing.
But for now this PR#486 code could be used on dev NNs to fix the deadlock situation (#554)

[Alrighttt]

LGTM, but I did not personally test as I no longer have access to a notary.

[DeckerSU]

LGTM generallу.

p.s. May be we also need to include this changes zcash#5853 in this PR. Good to have protection if chain tip is nullptr somehow. I added this change to Komodo-Qt codebase.
p.p.s. Also github web ui shows that this PR changes marmara.cpp which is absent at the moment. As I remember we get rid of it earlier, or I did it only in Komodo-Qt codebase.

[dimxy]

p.s. May be we also need to include this changes zcash#5853 in this PR. Good to have protection if chain tip is nullptr somehow.

Currently this code

komodo/src/init.cpp

Line 2016 in d456be3

while (!fRequestShutdown && chainActive.Tip() == NULL)

in AppInit2() ensures that the genesis block is imported (and chainActive becomes not empty) before mining starts
But there is a bad thing that if fShutdown is set chainActive still may be empty. So I think first we should add a return false if fShutdown is true. And we could also add zcash#5853 for better safety

[dimxy]

p.p.s. Also github web ui shows that this PR changes marmara.cpp which is absent at the moment. As I remember we get rid of it earlier, or I did it only in Komodo-Qt codebase.

I remember jmjatlanta intended to remove marmara.cpp but now I have all his PRs merged in my branch and marmara.cpp is still here. Maybe he did not make a PR with this removal

[tonymorony]

these changes implemented in combined PR: #559