consensus: fix timing overhead, race condition, and unnecessary computation (gridcoin-community#2880)

- Replace MilliTimer with lightweight GetTimeMillis() deltas in
  GridcoinConnectBlock, guarded by IsShadowValidationEnabled() so no
  timing overhead is incurred on the hot validation path when shadow
  mode is off
- Fix race between LogShadowResult and ShutdownShadowValidator by
  moving is_open() check inside the mutex and guarding close() with
  the same mutex
- Move mandatory/local sidestake spec computation inside the
  fEnableSideStaking guard so stake-splitting-only runs skip the
  registry queries and spec computation entirely

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: jamescowens

src/gridcoin/consensus/shadow_validator.cpp

@@ -70,30 +70,32 @@ void LogShadowResult(
     }
 
     // Write to structured log file if open.
-    if (g_shadow_log_file.is_open()) {
+    {
         std::lock_guard<std::mutex> lock(g_shadow_log_mutex);
 
-        g_shadow_log_file
-            << "{\"height\":" << height
-            << ",\"block_hash\":\"" << block_hash.ToString() << "\""
-            << ",\"component\":\"" << component << "\""
-            << ",\"auth_impl\":\"" << g_consensus_impl << "\""
-            << ",\"auth_pass\":" << (auth_pass ? "true" : "false")
-            << ",\"shadow_pass\":" << (shadow_pass ? "true" : "false");
-
-        if (!auth_error.empty()) {
-            g_shadow_log_file << ",\"auth_error\":\"" << JsonEscape(auth_error) << "\"";
-        }
-        if (!shadow_error.empty()) {
-            g_shadow_log_file << ",\"shadow_error\":\"" << JsonEscape(shadow_error) << "\"";
-        }
+        if (g_shadow_log_file.is_open()) {
+            g_shadow_log_file
+                << "{\"height\":" << height
+                << ",\"block_hash\":\"" << block_hash.ToString() << "\""
+                << ",\"component\":\"" << component << "\""
+                << ",\"auth_impl\":\"" << g_consensus_impl << "\""
+                << ",\"auth_pass\":" << (auth_pass ? "true" : "false")
+                << ",\"shadow_pass\":" << (shadow_pass ? "true" : "false");
+
+            if (!auth_error.empty()) {
+                g_shadow_log_file << ",\"auth_error\":\"" << JsonEscape(auth_error) << "\"";
+            }
+            if (!shadow_error.empty()) {
+                g_shadow_log_file << ",\"shadow_error\":\"" << JsonEscape(shadow_error) << "\"";
+            }
 
-        g_shadow_log_file
-            << ",\"auth_ms\":" << auth_ms
-            << ",\"shadow_ms\":" << shadow_ms
-            << "}\n";
+            g_shadow_log_file
+                << ",\"auth_ms\":" << auth_ms
+                << ",\"shadow_ms\":" << shadow_ms
+                << "}\n";
 
-        g_shadow_log_file.flush();
+            g_shadow_log_file.flush();
+        }
     }
 }
 
@@ -168,6 +170,8 @@ void GRC::LogShadowComparison(
 
 void GRC::ShutdownShadowValidator()
 {
+    std::lock_guard<std::mutex> lock(g_shadow_log_mutex);
+
     if (g_shadow_log_file.is_open()) {
         g_shadow_log_file.close();
         LogPrintf("ShadowValidator: closed shadow log file");

src/miner.cpp

@@ -906,50 +906,53 @@ void SplitCoinStakeOutput(CBlock &blocknew, int64_t &nReward, bool &fEnableStake
     // Initialize nOutputUsed at 1, because one is already used for the empty coinstake flag output.
     unsigned int nOutputsUsed = 1;
 
-    // Compute the mandatory sidestake spec using BlockRewardRules — the shared invariant computation that
-    // both miner and validator consume. This replaces the separate ActiveSideStakeEntries() call and manual
-    // dust/address filtering that previously lived only here, eliminating the class of drift bugs where the
-    // miner and validator made different eligibility decisions (see #2848).
-    CTxDestination coinstake_dest;
-    bool have_coinstake_dest = ExtractDestination(CoinStakeScriptPubKey, coinstake_dest);
-    if (!have_coinstake_dest) {
-        LogPrintf("WARN: SplitCoinStakeOutput: could not extract coinstake destination, "
-                  "skipping mandatory sidestake spec computation.");
-    }
-
-    GRC::BlockRewardRules rules(pindexBest, blocknew.nVersion, blocknew.nTime);
-
+    // Mandatory and local sidestake computation — only needed when sidestaking is active.
     std::vector<GRC::BlockRewardRules::MandatorySidestakeSpec> mandatory_specs;
+    SideStakeAlloc local_sidestakes;
+    unsigned int nMandatoryOutputLimit = GetMandatorySideStakeOutputLimit(blocknew.nVersion);
 
-    if (have_coinstake_dest) {
-        mandatory_specs = GRC::BlockRewardRules::FilterEligible(
-            rules.ComputeEligibleMandatorySidestakes(coinstake_dest, nReward));
-    }
+    if (fEnableSideStaking) {
+        // Compute the mandatory sidestake spec using BlockRewardRules — the shared invariant computation that
+        // both miner and validator consume. This replaces the separate ActiveSideStakeEntries() call and manual
+        // dust/address filtering that previously lived only here, eliminating the class of drift bugs where the
+        // miner and validator made different eligibility decisions (see #2848).
+        CTxDestination coinstake_dest;
+        bool have_coinstake_dest = ExtractDestination(CoinStakeScriptPubKey, coinstake_dest);
+        if (!have_coinstake_dest) {
+            LogPrintf("WARN: SplitCoinStakeOutput: could not extract coinstake destination, "
+                      "skipping mandatory sidestake spec computation.");
+        }
 
-    unsigned int nMandatoryOutputLimit = GetMandatorySideStakeOutputLimit(blocknew.nVersion);
+        if (have_coinstake_dest) {
+            GRC::BlockRewardRules rules(pindexBest, blocknew.nVersion, blocknew.nTime);
 
-    // Shuffle when over the limit — non-deterministic selection. The validator cannot reproduce the shuffle
-    // and doesn't need to; it matches against the eligible set regardless of order.
-    if (mandatory_specs.size() > nMandatoryOutputLimit) {
-        Shuffle(mandatory_specs.begin(), mandatory_specs.end(), FastRandomContext());
-    }
+            mandatory_specs = GRC::BlockRewardRules::FilterEligible(
+                rules.ComputeEligibleMandatorySidestakes(coinstake_dest, nReward));
+        }
 
-    // If the number of voluntary sidestaking allocation entries exceeds the remaining output capacity, then
-    // shuffle the list to support sidestaking with more than the available number of entries. This is a super
-    // simple solution but has some disadvantages. If the person made a mistake and has the entries in the config
-    // file add up to more than 100%, then those entries resulting in a cumulative total over 100% will always be
-    // excluded, not just randomly excluded, because the cumulative check is done in the order of the entries in
-    // the config file. This is not regarded as a big issue, because all of the entries are supposed to add up to
-    // less than or equal to 100%. Also when there are more than the available number of entries, the residual
-    // returned to the coinstake will vary when the entries are shuffled, because the total percentage of the
-    // selected entries will be randomized. No attempt to renormalize the percentages is done.
-    SideStakeAlloc local_sidestakes
-        = GRC::GetSideStakeRegistry().ActiveSideStakeEntries(GRC::SideStake::FilterFlag::LOCAL, false);
-
-    if (local_sidestakes.size() > nMaxSideStakeOutputs
-                                      - std::min<unsigned int>(nMandatoryOutputLimit,
-                                                               mandatory_specs.size())) {
-        Shuffle(local_sidestakes.begin(), local_sidestakes.end(), FastRandomContext());
+        // Shuffle when over the limit — non-deterministic selection. The validator cannot reproduce the shuffle
+        // and doesn't need to; it matches against the eligible set regardless of order.
+        if (mandatory_specs.size() > nMandatoryOutputLimit) {
+            Shuffle(mandatory_specs.begin(), mandatory_specs.end(), FastRandomContext());
+        }
+
+        // If the number of voluntary sidestaking allocation entries exceeds the remaining output capacity, then
+        // shuffle the list to support sidestaking with more than the available number of entries. This is a super
+        // simple solution but has some disadvantages. If the person made a mistake and has the entries in the config
+        // file add up to more than 100%, then those entries resulting in a cumulative total over 100% will always be
+        // excluded, not just randomly excluded, because the cumulative check is done in the order of the entries in
+        // the config file. This is not regarded as a big issue, because all of the entries are supposed to add up to
+        // less than or equal to 100%. Also when there are more than the available number of entries, the residual
+        // returned to the coinstake will vary when the entries are shuffled, because the total percentage of the
+        // selected entries will be randomized. No attempt to renormalize the percentages is done.
+        local_sidestakes
+            = GRC::GetSideStakeRegistry().ActiveSideStakeEntries(GRC::SideStake::FilterFlag::LOCAL, false);
+
+        if (local_sidestakes.size() > nMaxSideStakeOutputs
+                                          - std::min<unsigned int>(nMandatoryOutputLimit,
+                                                                   mandatory_specs.size())) {
+            Shuffle(local_sidestakes.begin(), local_sidestakes.end(), FastRandomContext());
+        }
     }
 
     // Initialize remaining stake output value to the total value of output for stake, which also includes

src/validation.cpp

@@ -1561,46 +1561,49 @@ bool GridcoinConnectBlock(
             std::string shadow_error;
             int64_t shadow_ms = 0;
 
+            const bool shadow_enabled = GRC::IsShadowValidationEnabled();
+
             if (GRC::IsNewImplAuthoritative()) {
                 // BlockRewardRules is authoritative.
-                MilliTimer auth_timer;
+                int64_t t_start = shadow_enabled ? GetTimeMillis() : 0;
 
                 GRC::BlockRewardRules rules(block, pindex, stake_value_in, total_claimed, fees, out_coin_age);
                 auth_pass = rules.Check(auth_error);
-                auth_ms = auth_timer.GetTimes("auth_check").elapsed_time;
+
+                if (shadow_enabled) auth_ms = GetTimeMillis() - t_start;
 
                 if (!auth_pass) {
                     block.DoS(10, error("%s: BlockRewardRules::Check FAILED at height %d: %s",
                                         __func__, pindex->nHeight, auth_error));
                 }
 
                 // Shadow: run old ClaimValidator for comparison.
-                if (GRC::IsShadowValidationEnabled()) {
-                    MilliTimer shadow_timer;
+                if (shadow_enabled) {
+                    int64_t t_shadow = GetTimeMillis();
                     shadow_pass = ClaimValidator(block, pindex, stake_value_in, total_claimed, fees, out_coin_age).Check();
-                    shadow_ms = shadow_timer.GetTimes("shadow_check").elapsed_time;
+                    shadow_ms = GetTimeMillis() - t_shadow;
 
                     if (!shadow_pass) {
                         shadow_error = "ClaimValidator::Check() failed (see debug log for details)";
                     }
                 }
             } else {
                 // ClaimValidator is authoritative (default).
-                MilliTimer auth_timer;
+                int64_t t_start = shadow_enabled ? GetTimeMillis() : 0;
                 auth_pass = ClaimValidator(block, pindex, stake_value_in, total_claimed, fees, out_coin_age).Check();
-                auth_ms = auth_timer.GetTimes("auth_check").elapsed_time;
+                if (shadow_enabled) auth_ms = GetTimeMillis() - t_start;
 
                 // Shadow: run BlockRewardRules for comparison.
-                if (GRC::IsShadowValidationEnabled()) {
-                    MilliTimer shadow_timer;
+                if (shadow_enabled) {
+                    int64_t t_shadow = GetTimeMillis();
 
                     GRC::BlockRewardRules rules(block, pindex, stake_value_in, total_claimed, fees, out_coin_age);
                     shadow_pass = rules.Check(shadow_error);
-                    shadow_ms = shadow_timer.GetTimes("shadow_check").elapsed_time;
+                    shadow_ms = GetTimeMillis() - t_shadow;
                 }
             }
 
-            if (GRC::IsShadowValidationEnabled()) {
+            if (shadow_enabled) {
                 GRC::LogShadowComparison(pindex->nHeight, pindex->GetBlockHash(),
                                          auth_pass, shadow_pass,
                                          auth_error, shadow_error,