fix: address review suggestions

- remove dead spent-index declarations from validation paths
- harden async index disconnect rewind handling for stale-branch notifications
- make rewind failures trigger fatal index shutdown handling
- refresh getaddressbalance maturity height after address index queries
- apply address index rewind DB updates atomically
- clean up format-string and logging lint handling around FatalError and wrapped LogPrintf calls
- make timestamp index GetSerializeSize helpers static
- fix a typo in test/util/data/non-backported.txt

Co-authored-by: OpenAI Codex <codex@openai.com>

Author: UdjinM6

src/index/addressindex.cpp

@@ -10,6 +10,7 @@
 #include <index/addressindex_util.h>
 #include <logging.h>
 #include <node/blockstorage.h>
+#include <tinyformat.h>
 #include <undo.h>
 #include <util/system.h>
 
@@ -138,6 +139,26 @@ bool AddressIndex::DB::UpdateAddressUnspentIndex(const std::vector<CAddressUnspe
     return CDBWrapper::WriteBatch(batch);
 }
 
+bool AddressIndex::DB::RewindBatch(const std::vector<CAddressIndexEntry>& address_entries,
+                                   const std::vector<CAddressUnspentIndexEntry>& unspent_entries)
+{
+    CDBBatch batch(*this);
+
+    for (const auto& [key, _] : address_entries) {
+        batch.Erase(std::make_pair(DB_ADDRESSINDEX, key));
+    }
+
+    for (const auto& [key, value] : unspent_entries) {
+        if (value.IsNull()) {
+            batch.Erase(std::make_pair(DB_ADDRESSUNSPENTINDEX, key));
+        } else {
+            batch.Write(std::make_pair(DB_ADDRESSUNSPENTINDEX, key), value);
+        }
+    }
+
+    return CDBWrapper::WriteBatch(batch);
+}
+
 AddressIndex::AddressIndex(size_t n_cache_size, bool f_memory, bool f_wipe) :
     m_db(std::make_unique<AddressIndex::DB>(n_cache_size, f_memory, f_wipe))
 {
@@ -349,13 +370,9 @@ bool AddressIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new
             }
         }
 
-        // Erase address history entries and update unspent index
-        if (!m_db->EraseAddressIndex(addressIndex)) {
-            return error("%s: Failed to erase address index during rewind", __func__);
-        }
-
-        if (!m_db->UpdateAddressUnspentIndex(addressUnspentIndex)) {
-            return error("%s: Failed to update address unspent index during rewind", __func__);
+        // Apply both rewind updates in a single batch to avoid leaving the index half-rewound.
+        if (!m_db->RewindBatch(addressIndex, addressUnspentIndex)) {
+            return error("%s: Failed to apply address index rewind batch", __func__);
         }
     }
 
@@ -369,10 +386,16 @@ void AddressIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block,
     // to remove this block's data
     const CBlockIndex* best_block_index = CurrentIndex();
 
-    // Only rewind if we have this block indexed
+    // Ignore stale-branch disconnect notifications that do not connect to the indexed chain.
     if (best_block_index && best_block_index->nHeight >= pindex->nHeight && pindex->pprev) {
+        if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) {
+            LogPrintf("%s: WARNING: Block %s does not disconnect from an ancestor of " /* Continued */
+                      "known best chain (tip=%s); not updating index\n",
+                      __func__, pindex->GetBlockHash().ToString(), best_block_index->GetBlockHash().ToString());
+            return;
+        }
         if (!Rewind(best_block_index, pindex->pprev)) {
-            error("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
+            FatalError("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
         }
     }
 }

src/index/addressindex.h

@@ -52,6 +52,10 @@ class AddressIndex final : public BaseIndex
 
         /// Update address unspent index (handles both adds and deletes)
         bool UpdateAddressUnspentIndex(const std::vector<CAddressUnspentIndexEntry>& entries);
+
+        /// Atomically erase address history entries and update unspent outputs during rewind.
+        bool RewindBatch(const std::vector<CAddressIndexEntry>& address_entries,
+                         const std::vector<CAddressUnspentIndexEntry>& unspent_entries);
     };
 
     /// Override to return false - we need undo data

src/index/base.cpp

@@ -23,12 +23,10 @@ constexpr uint8_t DB_BEST_BLOCK{'B'};
 constexpr auto SYNC_LOG_INTERVAL{30s};
 constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
 
-template <typename... Args>
-static void FatalError(const char* fmt, const Args&... args)
+void BaseIndex::FatalErrorImpl(const std::string& message)
 {
-    std::string strMessage = tfm::format(fmt, args...);
-    SetMiscWarning(Untranslated(strMessage));
-    LogPrintf("*** %s\n", strMessage);
+    SetMiscWarning(Untranslated(message));
+    LogPrintf("*** %s\n", message);
     AbortError(_("A fatal internal error occurred, see debug.log for details"));
     StartShutdown();
 }

src/index/base.h

@@ -6,6 +6,7 @@
 #define BITCOIN_INDEX_BASE_H
 
 #include <dbwrapper.h>
+#include <tinyformat.h>
 #include <util/threadinterrupt.h>
 #include <validationinterface.h>
 
@@ -112,6 +113,15 @@ class BaseIndex : public CValidationInterface
     /// Get the name of the index for display in logs.
     virtual const char* GetName() const = 0;
 
+    /// Trigger a fatal index error and initiate shutdown.
+    static void FatalErrorImpl(const std::string& message);
+
+    template <typename... Args>
+    void FatalError(const char* fmt, const Args&... args) const
+    {
+        FatalErrorImpl(tfm::format(fmt, args...));
+    }
+
     /// Update the internal best block index as well as the prune lock.
     void SetBestBlockIndex(const CBlockIndex* block);
 

src/index/spentindex.cpp

@@ -12,6 +12,7 @@
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
+#include <tinyformat.h>
 #include <undo.h>
 #include <util/system.h>
 
@@ -151,10 +152,16 @@ void SpentIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block, c
     // to remove this block's data
     const CBlockIndex* best_block_index = CurrentIndex();
 
-    // Only rewind if we have this block indexed
+    // Ignore stale-branch disconnect notifications that do not connect to the indexed chain.
     if (best_block_index && best_block_index->nHeight >= pindex->nHeight && pindex->pprev) {
+        if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) {
+            LogPrintf("%s: WARNING: Block %s does not disconnect from an ancestor of " /* Continued */
+                      "known best chain (tip=%s); not updating index\n",
+                      __func__, pindex->GetBlockHash().ToString(), best_block_index->GetBlockHash().ToString());
+            return;
+        }
         if (!Rewind(best_block_index, pindex->pprev)) {
-            error("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
+            FatalError("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
         }
     }
 }

src/index/timestampindex.cpp

@@ -6,6 +6,7 @@
 
 #include <chain.h>
 #include <logging.h>
+#include <tinyformat.h>
 #include <util/system.h>
 
 constexpr uint8_t DB_TIMESTAMPINDEX{'s'};
@@ -93,10 +94,16 @@ void TimestampIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& bloc
     // to remove this block's data
     const CBlockIndex* best_block_index = CurrentIndex();
 
-    // Only rewind if we have this block indexed and it's not the genesis block
+    // Ignore stale-branch disconnect notifications that do not connect to the indexed chain.
     if (best_block_index && best_block_index->nHeight >= pindex->nHeight && pindex->pprev) {
+        if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) {
+            LogPrintf("%s: WARNING: Block %s does not disconnect from an ancestor of " /* Continued */
+                      "known best chain (tip=%s); not updating index\n",
+                      __func__, pindex->GetBlockHash().ToString(), best_block_index->GetBlockHash().ToString());
+            return;
+        }
         if (!Rewind(best_block_index, pindex->pprev)) {
-            error("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
+            FatalError("%s: Failed to rewind %s to previous block after disconnect", __func__, GetName());
         }
     }
 }

src/index/timestampindex_types.h

@@ -22,7 +22,7 @@ struct CTimestampIndexIteratorKey {
 
     void SetNull() { m_time = 0; }
 
-    size_t GetSerializeSize(int nType, int nVersion) const { return 4; }
+    static size_t GetSerializeSize(int nType, int nVersion) { return 4; }
 
     template <typename Stream>
     void Serialize(Stream& s) const
@@ -54,7 +54,7 @@ struct CTimestampIndexKey {
         m_block_hash.SetNull();
     }
 
-    size_t GetSerializeSize(int nType, int nVersion) const { return 36; }
+    static size_t GetSerializeSize(int nType, int nVersion) { return 36; }
 
     template <typename Stream>
     void Serialize(Stream& s) const

src/rpc/node.cpp

@@ -666,13 +666,11 @@ static RPCHelpMan getaddressbalance()
         throw JSONRPCError(RPC_MISC_ERROR, "Address index is not enabled. Start with -addressindex to enable.");
     }
 
-    // Check sync status first to return a clear error message
-    // Use the index's best block height instead of chain tip to avoid inconsistency
+    // Check sync status first to return a clear error message.
     const IndexSummary summary = g_addressindex->GetSummary();
     if (!summary.synced) {
         throw JSONRPCError(RPC_MISC_ERROR, strprintf("Address index is syncing. Current height: %d", summary.best_block_height));
     }
-    int nHeight = summary.best_block_height;
 
     for (const auto& address : addresses) {
         if (!g_addressindex->GetAddressIndex(address.first, address.second, addressIndex,
@@ -681,6 +679,10 @@ static RPCHelpMan getaddressbalance()
         }
     }
 
+    // Refresh the indexed height after the query so maturity checks are based on the same
+    // index state that produced the returned entries.
+    const int nHeight = g_addressindex->GetSummary().best_block_height;
+
 
     CAmount balance = 0;
     CAmount balance_spendable = 0;

src/validation.cpp

@@ -2008,8 +2008,6 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
         return DISCONNECT_FAILED;
     }
 
-    std::vector<CSpentIndexEntry> spentIndex;
-
     std::optional<MNListUpdates> mnlist_updates_opt{std::nullopt};
     if (!m_chain_helper->special_tx->UndoSpecialTxsInBlock(block, pindex, mnlist_updates_opt)) {
         error("DisconnectBlock(): UndoSpecialTxsInBlock failed");
@@ -2354,8 +2352,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
     int nInputs = 0;
     unsigned int nSigOps = 0;
     blockundo.vtxundo.reserve(block.vtx.size() - 1);
-    std::vector<CSpentIndexEntry> spentIndex;
-
     bool fDIP0001Active_context = DeploymentActiveAt(*pindex, m_params.GetConsensus(), Consensus::DEPLOYMENT_DIP0001);
 
     // MUST process special txes before updating UTXO to ensure consistency between mempool and block processing
@@ -4582,8 +4578,6 @@ bool CChainState::RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& i
             pindex->GetBlockHash().ToString(), state.ToString());
     }
 
-    std::vector<CSpentIndexEntry> spentIndex;
-
     for (size_t i = 0; i < block.vtx.size(); i++) {
         const CTransactionRef& tx = block.vtx[i];
 

src/validation.h

@@ -1069,8 +1069,6 @@ class ChainstateManager
 
     //! Load the block tree and coins database from disk, initializing state if we're running with -reindex
     bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-    //! Initialize additional indexes and store their flags to disk
-    void InitAdditionalIndexes() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     //! Check to see if caches are out of balance and if so, call
     //! ResizeCoinsCaches() as needed.