Fix races for strMiscWarning and fLargeWork*Found, make QT runawayException use GetWarnings #9236

gmaxwell commented at 9:52 AM on November 29, 2016: contributor

Eliminate data races for strMiscWarning and fLargeWork*Found.

This moves all access to these datastructures through accessor functions and protects them with a lock.

and

Make QT runawayException call GetWarnings("gui") instead of directly access strMiscWarning.

gmaxwell commented at 9:52 AM on November 29, 2016: contributor

I have not yet tested the QT part of this change.

fanquake added the label Refactoring on Nov 29, 2016

jonasschnelli commented at 10:48 AM on November 29, 2016: contributor

General Concept ACK but I would prefer moving the warning function in a new file (seems to be a misuse of utils.h/c).

MarcoFalke commented at 11:34 AM on November 29, 2016: member

Maybe ui_interface.* for GetWarnings()?

gmaxwell commented at 6:16 AM on November 30, 2016: contributor

@MarcoFalke Can you walk me through your reason for adding a third warning context there?

jonasschnelli commented at 7:39 AM on November 30, 2016: contributor

utACK 6ed742dcbeeb16e7e5fcc42b292bc0c43f5e219c

in src/warnings.cpp:None in 6ed742dcbe outdated

  52 | +
  53 | +    LOCK(cs_warnings);
  54 | +
  55 | +    if (!CLIENT_VERSION_IS_RELEASE) {
  56 | +        strStatusBar = "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications";
  57 | +        strGUI = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");

paveljanik commented at 3:57 PM on November 30, 2016:

No RPC info about pre-release build? (I know you only copied this from prev. location.)

gmaxwell commented at 2:47 PM on December 2, 2016:

strStatusBar does that, RPC is what you get in safemode errors, effectively.

MarcoFalke commented at 10:01 PM on November 30, 2016: member

My initial thinking was to put all such ui related code into ui_interface, but I don't think this makes sense in the long run as the original scope of ui_interface was different. So I am fine with putting it in the new file, though I don't see why the new header needs to go into the consensus lib... I think common or server should be enough?

in src/main.cpp:None in 6ed742dcbe outdated

2719 | @@ -2721,9 +2720,10 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
2720 |              ThresholdState state = checker.GetStateFor(pindex, chainParams.GetConsensus(), warningcache[bit]);
2721 |              if (state == THRESHOLD_ACTIVE || state == THRESHOLD_LOCKED_IN) {
2722 |                  if (state == THRESHOLD_ACTIVE) {
2723 | -                    strMiscWarning = strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit);
2724 | +                    string strWarning = strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit);

MarcoFalke commented at 10:02 PM on November 30, 2016:

Maybe std::string to preempt silent merge conflicts?

gmaxwell commented at 2:47 PM on December 2, 2016:

done

sipa commented at 12:12 AM on December 1, 2016: member

Needs rebase.

sipa commented at 12:13 AM on December 1, 2016: member

Needs rebase.

gmaxwell commented at 2:48 PM on December 2, 2016: contributor

I believe I addressed the comments from @MarcoFalke

MarcoFalke commented at 3:15 PM on December 2, 2016: member

utACK 0b5a146cbc04949a8eb2a3f07e53c768fb0ca7dd

Make QT runawayException call GetWarnings instead of directly access strMiscWarning.

This is a first step in avoiding racy accesses to strMiscWarning.

The change required moving GetWarnings and related globals to util.

c63198f1c7

Eliminate data races for strMiscWarning and fLargeWork*Found.

This moves all access to these datastructures through accessor functions
 and protects them with a lock.

e3ba0ef956

Move GetWarnings() into its own file. 749be013f5

gmaxwell commented at 9:11 AM on December 3, 2016: contributor

Rebased for the main murder mania.

laanwj commented at 11:38 AM on December 19, 2016: member

Makes sense. utACK 749be01

laanwj merged this on Dec 19, 2016

laanwj closed this on Dec 19, 2016

laanwj referenced this in commit 7f72568e6b on Dec 19, 2016

codablock referenced this in commit dcc9dffae9 on Jan 18, 2018

andvgal referenced this in commit 8686389426 on Jan 6, 2019

CryptoCentric referenced this in commit 79fc6d7a79 on Feb 26, 2019

zkbot referenced this in commit 69e0e7ee62 on Feb 21, 2020

denverbdr referenced this in commit 2c8e5c086f on Mar 6, 2020

random-zebra referenced this in commit 84ca60a42f on Dec 17, 2020

MarcoFalke locked this on Sep 8, 2021