refactor: Scraper thread safety and code cleanup

[jamescowens]

This is a detailed PR that fully implements the appropriate keywords/qualifiers in the scraper system for the Clang thread safety static analysis. This allows advanced compilers to detect thread safety problems such as access to functions/variables without prior locks held. One thing the static analysis does NOT do is analyze for potential deadlocks due to lock order conflicts. In particular that caused a lot of headaches with this PR. The scraper is quite complex and uses a fine grained locking scheme to ensure that it can run relatively unimpeded with multiple threads accessing it. Unfortunately the by product of that is that extremely careful attention to locking order needs to be adhered to avoid deadlocks. The deadlock detection after many adjustments was done using manual code review of the call chains and the debug mode deadlock detection built into the wallet already in sync.h/cpp in debug mode. Note that this is difficult, because our old net code has an existing potential deadlock between cs_main and the cs_vnodes, which prevents running the wallet a long time to drain out all potential deadlocks.

Note: Once @div72 helped me get the Bitcoin sync.h/cpp port running the way I wanted where we could continue to run the wallet while logging potential deadlocks. I did a few more adjustments which are in the thread safety II commit. The remaining entries are all related to variations of the above mentioned potential deadlock and do not need to be fixed. We will cover those in the net code port.

I probably will squash some of these commits as this PR gets closer to completion, as they are too fine grained right now. [Done] Also, I am going to be ripping out the scraper lock logging, as that has proven not to be terribly useful, and eats up log file space and also stresses out the core to GUI signaling to update the scraper tab.

I have also taken the time to improve the documentation of the scraper, including replacing the // style comments with Doxygen compliant commenting where appropriate.

I removed the move(s) for the CScraperManifests, because once I converted those to be accessed via smart shared pointers earlier, "moving" the ownership of the pointer during a call is downright silly, given the control block of the shared pointer keeps track of the references and the reference of the calling function will fall away as soon as it is out of scope.

I have also more fully encapsulated the scraper appcache/extended appcache in anticipation of ripping the appcache out and replacing it with a native contract handler for the scraper section.

Note that this is built on top of #2292, so includes those commits at the moment. They will disappear once #2292 is merged and this PR is rebased. [Done]

Final note on testing: I have been running this on two different testnet scraper nodes, one of those in explorer mode, and it is running perfectly, with no problems and the two nodes participating in the convergences properly. I think this is ready to go.

There is one thread safety log entry remaining, and it is on PushMessages, but I believe it is a false positive. The PushMessages template appears to make copies of all of the Args... and I think that may be generating the false positive, because the custom serialization of the CScraperManifest appropriately does NOT copy the mutex.

A future improvement to the scraper would be to convert the custom reference system in the parts area to use smart shared pointers like the CScraperManifests, but that belongs in a separate PR, and the existing system is working perfectly, as demonstrated by running the scraperreport, which shows every object/reference fully accounted for.

Another future improvement would be to change the fallback rule to project level to be no more than 24 hours earlier than the present, as the existing algorithm will use the entire inventory of CScraperManifests to attempt a convergence at the manifest level before falling back to the project level if a project is not available. The effect of this the scraper to end up retaining the same statistics for two superblocks if a project has gone offline for < 48 hours. This change will correct that, but will have to be done at the next mandatory.

Finally, the scraper uses string fields rather than the newer native fields in the Fern+ code, and this is primarily because the scraper was written first, before the Fern code, before all of that refactoring was available. It would be good to retrofit the native fields and the more efficient storage/processing that goes along with it, but that is a significant amount of work for a dubious benefit at the current time.

[jamescowens]

It is the Bitcoin Include guard standard and what is required to clear the lint errors. We should be using the Bitcoin include guard standard rather than #pragma once.

…

Sent from my iPhone

On Sep 13, 2021, at 8:05 AM, div72 ***@***.***> wrote:  @div72 commented on this pull request. In src/gridcoin/scraper/fwd.h: > +#ifndef BITCOIN_GRIDCOIN_SCRAPER_FWD_H +#define BITCOIN_GRIDCOIN_SCRAPER_FWD_H What's the reasoning behind this change? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[div72]

I thought we were going with #pragma once for Gridcoin specific code? 7181af4 should've fixed the lint issue. Checking the logs, I see no complaint from the linter for scraper code.

[jamescowens]

That is because the include guards are correct now. It was complaining before.

[jamescowens]

I am ok with that as long as we can get the Linter to cooperate.

[jamescowens]

@div72 I rebased this now that #2336 is merged that implements the discussion on include guards above. Any further comments? If not, please sign off on it. I want to get it merged and then get a testnet build out.

[div72]

utACK, a few documentation changes lack punctuation and I personally would assign non-consensus critical scraper globals to locals instead of using lambdas but this is also fine. All the comments are essentially nits.

[jamescowens]

On the lambdas... they will be going away when I turn the scraper into a class, which is eventually going to happen.

[jamescowens]

I could have also used the WITH_LOCK's that came with the upgrading of sync.h/cpp I recently did with upstream, but that looks uglier than the repetitiveness of the lambdas in this case. What we really want is getters inside a class taking a lock and returning the class member variables.