Repository: bitcoin/bitcoin (language: cpp)

Issue:
validation: Review mutable static local variables in `CChainState` methods
Now that assumeutxo is well on the way, and multiple `CChainState` exist, we should review the mutable static local variables in `CChainState` methods. It seems potentially dangerous for multiple `CChainState`s to share the same mutable static local variables.

In `CChainState::FlushStateToDisk`:
https://github.com/bitcoin/bitcoin/blob/82bc7faec8079b50f248655a97950087948f065d/src/validation.cpp#L2154-L2155

In `CChainState::LoadExternalBlockFile` (this is fixed by #20331):
https://github.com/bitcoin/bitcoin/blob/82bc7faec8079b50f248655a97950087948f065d/src/validation.cpp#L4286

ping @jamesob 

-----

Now that I think about it... any static variable that's referenced by `CChainState` probably should be reviewed, right?


Discussion / hints:

Fix patch:
diff --git a/src/validation.cpp b/src/validation.cpp
index debdc2ae749e9..f4f05a48bd157 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2360,8 +2360,6 @@ bool Chainstate::FlushStateToDisk(
 {
     LOCK(cs_main);
     assert(this->CanFlushToDisk());
-    static std::chrono::microseconds nLastWrite{0};
-    static std::chrono::microseconds nLastFlush{0};
     std::set<int> setFilesToPrune;
     bool full_flush_completed = false;
 
@@ -2415,20 +2413,20 @@ bool Chainstate::FlushStateToDisk(
         }
         const auto nNow = GetTime<std::chrono::microseconds>();
         // Avoid writing/flushing immediately after startup.
-        if (nLastWrite.count() == 0) {
-            nLastWrite = nNow;
+        if (m_last_write.count() == 0) {
+            m_last_write = nNow;
         }
-        if (nLastFlush.count() == 0) {
-            nLastFlush = nNow;
+        if (m_last_flush.count() == 0) {
+            m_last_flush = nNow;
         }
         // The cache is large and we're within 10% and 10 MiB of the limit, but we have time now (not in the middle of a block processing).
         bool fCacheLarge = mode == FlushStateMode::PERIODIC && cache_state >= CoinsCacheSizeState::LARGE;
         // The cache is over the limit, we have to write now.
         bool fCacheCritical = mode == FlushStateMode::IF_NEEDED && cache_state >= CoinsCacheSizeState::CRITICAL;
         // It's been a while since we wrote the block index to disk. Do this frequently, so we don't need to redownload after a crash.
-        bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > nLastWrite + DATABASE_WRITE_INTERVAL;
+        bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > m_last_write + DATABASE_WRITE_INTERVAL;
         // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
-        bool fPeriodicFlush = mode == FlushStateMode::PERIODIC && nNow > nLastFlush + DATABASE_FLUSH_INTERVAL;
+        bool fPeriodicFlush = mode == FlushStateMode::PERIODIC && nNow > m_last_flush + DATABASE_FLUSH_INTERVAL;
         // Combine all conditions that result in a full cache flush.
         fDoFullFlush = (mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
         // Write blocks and block index to disk.
@@ -2458,7 +2456,7 @@ bool Chainstate::FlushStateToDisk(
 
                 UnlinkPrunedFiles(setFilesToPrune);
             }
-            nLastWrite = nNow;
+            m_last_write = nNow;
         }
         // Flush best chain related state. This can only be done if the blocks / block index write was also done.
         if (fDoFullFlush && !CoinsTip().GetBestBlock().IsNull()) {
@@ -2476,7 +2474,7 @@ bool Chainstate::FlushStateToDisk(
             // Flush the chainstate (which may refer to block index entries).
             if (!CoinsTip().Flush())
                 return AbortNode(state, "Failed to write to coin database");
-            nLastFlush = nNow;
+            m_last_flush = nNow;
             full_flush_completed = true;
             TRACE5(utxocache, flush,
                    (int64_t)(GetTimeMicros() - nNow.count()), // in microseconds (µs)
diff --git a/src/validation.h b/src/validation.h
index b8151dc1fc206..ccc884cb7a28a 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -743,6 +743,9 @@ class Chainstate
     void UpdateTip(const CBlockIndex* pindexNew)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    std::chrono::microseconds m_last_write{0};
+    std::chrono::microseconds m_last_flush{0};
+
     friend ChainstateManager;
 };
 


Test patch: