Merge pull request #2490 from jamescowens/fix_blockfinder

rpc, util: Remove caching from BlockFinder

Author: jamescowens

src/gridcoin/contract/contract.cpp

@@ -418,16 +418,14 @@ Contract GRC::MakeLegacyContract(
 
 void GRC::ReplayContracts(CBlockIndex* pindex_end, CBlockIndex* pindex_start)
 {
-    static BlockFinder blockFinder;
-
     CBlockIndex*& pindex = pindex_start;
 
     // If there is no pindex_start (i.e. default value of nullptr), then set standard lookback. A Non-standard lookback
     // where there is a specific pindex_start argument supplied, is only used in the GRC InitializeContracts call for
     // when the beacon database in LevelDB has not already been populated.
     if (!pindex)
     {
-        pindex = blockFinder.FindByMinTime(pindexBest->nTime - Beacon::MAX_AGE);
+        pindex = GRC::BlockFinder::FindByMinTime(pindexBest->nTime - Beacon::MAX_AGE);
     }
 
     if (pindex->nHeight < (fTestNet ? 1 : 164618)) {

src/gridcoin/gridcoin.cpp

@@ -120,7 +120,7 @@ bool InitializeResearchRewardAccounting(CBlockIndex* pindexBest)
 
     const int64_t start_height = Params().GetConsensus().ResearchAgeHeight;
 
-    if (!Tally::Initialize(BlockFinder().FindByHeight(start_height))) {
+    if (!Tally::Initialize(BlockFinder::FindByHeight(start_height))) {
         return error("Failed to initialize tally.");
     }
 
@@ -169,9 +169,7 @@ void InitializeContracts(CBlockIndex* pindexBest)
     LogPrintf("Gridcoin: replaying contracts...");
     uiInterface.InitMessage(_("Replaying contracts..."));
 
-    static BlockFinder blockFinder;
-
-    CBlockIndex* pindex_start = blockFinder.FindByMinTime(pindexBest->nTime - Beacon::MAX_AGE);
+    CBlockIndex* pindex_start = GRC::BlockFinder::FindByMinTime(pindexBest->nTime - Beacon::MAX_AGE);
 
     const int& V11_height = Params().GetConsensus().BlockV11Height;
     const int& lookback_window_low_height = pindex_start->nHeight;

src/gridcoin/scraper/scraper.cpp

@@ -571,11 +571,9 @@ const CBlockIndex* GetBeaconConsensusHeight() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     AssertLockHeld(cs_main);
 
-    static BlockFinder block_finder;
-
     // Use 4 times the BLOCK_GRANULARITY which moves the consensus block every hour.
     // TODO: Make the mod a function of SCRAPER_CMANIFEST_RETENTION_TIME in scraper.h.
-    return block_finder.FindByHeight(
+    return BlockFinder::FindByHeight(
         (nBestHeight - CONSENSUS_LOOKBACK)
             - (nBestHeight - CONSENSUS_LOOKBACK) % (BLOCK_GRANULARITY * 4));
 }

src/gridcoin/support/block_finder.cpp

@@ -5,14 +5,8 @@
 #include "main.h"
 #include "gridcoin/support/block_finder.h"
 
-#include <cstdlib>
-
 using namespace GRC;
 
-BlockFinder::BlockFinder()
-    : cache(nullptr)
-{}
-
 CBlockIndex* BlockFinder::FindByHeight(int height)
 {
     // If the height is at the bottom half of the chain, start searching from
@@ -23,11 +17,6 @@ CBlockIndex* BlockFinder::FindByHeight(int height)
 
     if(index != nullptr)
     {
-        // Use the cache if it's closer to the target than the current
-        // start block.
-        if (cache && abs(height - index->nHeight) > std::abs(height - cache->nHeight))
-            index = cache;
-
         // Traverse towards the tail.
         while (index && index->pprev && index->nHeight > height)
             index = index->pprev;
@@ -37,7 +26,6 @@ CBlockIndex* BlockFinder::FindByHeight(int height)
             index = index->pnext;
     }
 
-    cache = index;
     return index;
 }
 
@@ -52,11 +40,6 @@ CBlockIndex* BlockFinder::FindByMinTime(int64_t time)
 
     if(index != nullptr)
     {
-        // If we have a cache that's closer to target than our current index,
-        // use it.
-        if(cache && abs(time - index->nTime) > abs(time - int64_t(cache->nTime)))
-            index = cache;
-
         // Move back until the previous block is no longer younger than "time".
         while(index && index->pprev && index->pprev->nTime > time)
             index = index->pprev;
@@ -66,11 +49,5 @@ CBlockIndex* BlockFinder::FindByMinTime(int64_t time)
             index = index->pnext;
     }
 
-    cache = index;
     return index;
 }
-
-void BlockFinder::Reset()
-{
-    cache = nullptr;
-}

src/gridcoin/support/block_finder.h

@@ -14,22 +14,17 @@ namespace GRC {
 class BlockFinder
 {
 public:
-    //!
-    //! \brief Constructor.
-    //!
-    BlockFinder();
-
     //!
     //! \brief Find a block with a specific height.
     //!
     //! Traverses the chain from head or tail, depending on what's closest to
-    //! find the block that matches \p height. This is a caching operation
+    //! find the block that matches \p height.
     //!
     //! \param nHeight Block height to find.
     //! \return The block with the height closest to \p nHeight if found, otherwise
     //! \a nullptr is returned.
     //!
-    CBlockIndex* FindByHeight(int height);
+    static CBlockIndex* FindByHeight(int height);
 
     //!
     //! \brief Find block by time.
@@ -42,18 +37,7 @@ class BlockFinder
     //! \return The youngest block which is not older than \p time, or the
     //! head of the chain if it is older than \p time.
     //!
-    CBlockIndex* FindByMinTime(int64_t time);
-
-    //!
-    //! \brief Reset finder cache.
-    //!
-    //! Clears the block finder cache. This should be used when blocks are removed
-    //! from the chain to avoid accessing deleted memory.
-    //!
-    void Reset();
-
-private:
-    CBlockIndex* cache;
+    static CBlockIndex* FindByMinTime(int64_t time);
 };
 } // namespace GRC
 

src/gridcoin/voting/registry.cpp

@@ -337,10 +337,8 @@ CBlockIndex* PollReference::GetEndingBlockIndexPtr() const
 {
     // Has poll ended?
     if (Expired(GetAdjustedTime())) {
-        GRC::BlockFinder blockfinder;
-
         // Find and return the last block that contains valid votes for the poll.
-        return blockfinder.FindByMinTime(Expiration());
+        return GRC::BlockFinder::FindByMinTime(Expiration());
     }
 
     return nullptr;

src/rpc/blockchain.cpp

@@ -32,8 +32,6 @@ double CoinToDouble(double surrogate);
 extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);
 UniValue ContractToJson(const GRC::Contract& contract);
 
-GRC::BlockFinder RPCBlockFinder;
-
 UniValue MRCToJson(const GRC::MRC& mrc) {
     UniValue json(UniValue::VOBJ);
 
@@ -471,7 +469,7 @@ UniValue showblock(const UniValue& params, bool fHelp)
 
     LOCK(cs_main);
 
-    CBlockIndex* pblockindex = RPCBlockFinder.FindByHeight(nHeight);
+    CBlockIndex* pblockindex = GRC::BlockFinder::FindByHeight(nHeight);
 
     if (pblockindex == nullptr)
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
@@ -587,7 +585,7 @@ UniValue getblockhash(const UniValue& params, bool fHelp)
 
     LOCK(cs_main);
 
-    CBlockIndex* RPCpblockindex = RPCBlockFinder.FindByHeight(nHeight);
+    CBlockIndex* RPCpblockindex = GRC::BlockFinder::FindByHeight(nHeight);
 
     return RPCpblockindex->phashBlock->GetHex();
 }
@@ -634,9 +632,8 @@ UniValue getblockbynumber(const UniValue& params, bool fHelp)
     LOCK(cs_main);
 
     CBlock block;
-    static GRC::BlockFinder block_finder;
 
-    CBlockIndex* pblockindex = block_finder.FindByHeight(nHeight);
+    CBlockIndex* pblockindex = GRC::BlockFinder::FindByHeight(nHeight);
     ReadBlockFromDisk(block, pblockindex, Params().GetConsensus());
 
     return blockToJSON(block, pblockindex, params.size() > 1 ? params[1].get_bool() : false);
@@ -660,9 +657,8 @@ UniValue getblockbymintime(const UniValue& params, bool fHelp)
     LOCK(cs_main);
 
     CBlock block;
-    static GRC::BlockFinder block_finder;
 
-    CBlockIndex* pblockindex = block_finder.FindByMinTime(nTimestamp);
+    CBlockIndex* pblockindex = GRC::BlockFinder::FindByMinTime(nTimestamp);
     ReadBlockFromDisk(block, pblockindex, Params().GetConsensus());
 
     return blockToJSON(block, pblockindex, params.size() > 1 ? params[1].get_bool() : false);

src/rpc/dataacq.cpp

@@ -25,8 +25,6 @@
 using namespace GRC;
 using namespace std;
 
-extern GRC::BlockFinder RPCBlockFinder;
-
 // Brod
 static bool compare_second(const pair<std::string, int64_t>  &p1, const pair<std::string, int64_t> &p2)
 {

src/rpc/rawtransaction.cpp

@@ -1075,9 +1075,7 @@ UniValue consolidatemsunspent(const UniValue& params, bool fHelp)
         if (nMaxInputs == 0 || nMaxInputs > nEqMaxInputs)
             nMaxInputs = nEqMaxInputs;
 
-        GRC::BlockFinder blockfinder;
-
-        CBlockIndex* pblkindex = blockfinder.FindByHeight((nBlockStart - 1));
+        CBlockIndex* pblkindex = GRC::BlockFinder::FindByHeight((nBlockStart - 1));
 
         if (!pblkindex)
             throw JSONRPCError(RPC_PARSE_ERROR, "Block not found");
@@ -1279,9 +1277,7 @@ UniValue scanforunspent(const UniValue& params, bool fHelp)
     {
         LOCK(cs_main);
 
-        GRC::BlockFinder blockfinder;
-
-        CBlockIndex* pblkindex = blockfinder.FindByHeight((nBlockStart - 1));
+        CBlockIndex* pblkindex = GRC::BlockFinder::FindByHeight((nBlockStart - 1));
 
         if (!pblkindex)
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");

src/test/gridcoin/block_finder_tests.cpp

@@ -46,46 +46,41 @@ BOOST_AUTO_TEST_SUITE(block_finder_tests);
 BOOST_AUTO_TEST_CASE(FindBlockInNormalChainShouldWork)
 {
     BlockChain<100> chain;
-    GRC::BlockFinder finder;
     for(auto& block : chain.blocks)
-        BOOST_CHECK_EQUAL(&block, finder.FindByHeight(block.nHeight));
+        BOOST_CHECK_EQUAL(&block, GRC::BlockFinder::FindByHeight(block.nHeight));
 }
 
 BOOST_AUTO_TEST_CASE(FindBlockAboveHighestHeightShouldReturnHighestBlock)
 {
     BlockChain<100> chain;
-    GRC::BlockFinder finder;
     CBlockIndex& last = chain.blocks.back();
-    BOOST_CHECK_EQUAL(&last, finder.FindByHeight(101));
+    BOOST_CHECK_EQUAL(&last, GRC::BlockFinder::FindByHeight(101));
 }
 
 BOOST_AUTO_TEST_CASE(FindBlockByHeightShouldWorkOnChainsWithJustOneBlock)
 {
     BlockChain<1> chain;
-    GRC::BlockFinder finder;
-    BOOST_CHECK_EQUAL(&chain.blocks.front(), finder.FindByHeight(0));
-    BOOST_CHECK_EQUAL(&chain.blocks.front(), finder.FindByHeight(1));
-    BOOST_CHECK_EQUAL(&chain.blocks.front(), finder.FindByHeight(-1));
+    BOOST_CHECK_EQUAL(&chain.blocks.front(), GRC::BlockFinder::FindByHeight(0));
+    BOOST_CHECK_EQUAL(&chain.blocks.front(), GRC::BlockFinder::FindByHeight(1));
+    BOOST_CHECK_EQUAL(&chain.blocks.front(), GRC::BlockFinder::FindByHeight(-1));
 }
 
 BOOST_AUTO_TEST_CASE(FindBlockByTimeShouldReturnNextYoungestBlock)
 {
     // Chain with block times 0, 10, 20, 30, 40 etc.
     BlockChain<10> chain;
-    GRC::BlockFinder finder;
 
     // Finding the block older than time 10 should return block #2
     // which has time 20.
-    BOOST_CHECK_EQUAL(&chain.blocks[2], finder.FindByMinTime(11));
-    BOOST_CHECK_EQUAL(&chain.blocks[1], finder.FindByMinTime(10));
-    BOOST_CHECK_EQUAL(&chain.blocks[1], finder.FindByMinTime(9));
+    BOOST_CHECK_EQUAL(&chain.blocks[2], GRC::BlockFinder::FindByMinTime(11));
+    BOOST_CHECK_EQUAL(&chain.blocks[1], GRC::BlockFinder::FindByMinTime(10));
+    BOOST_CHECK_EQUAL(&chain.blocks[1], GRC::BlockFinder::FindByMinTime(9));
 }
 
 BOOST_AUTO_TEST_CASE(FindBlockByTimeShouldReturnLastBlockIfOlderThanTime)
 {
     BlockChain<10> chain;
-    GRC::BlockFinder finder;
-    BOOST_CHECK_EQUAL(&chain.blocks.back(), finder.FindByMinTime(999999));
+    BOOST_CHECK_EQUAL(&chain.blocks.back(), GRC::BlockFinder::FindByMinTime(999999));
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/wallet/wallet.cpp

@@ -2860,7 +2860,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const EXC
             mapKeyBirth[it->first] = it->second.nCreateTime;
 
     // map in which we'll infer heights of other keys
-    CBlockIndex *pindexMax = GRC::BlockFinder().FindByHeight(std::max(0, nBestHeight - 144)); // the tip can be reorganised; use a 144-block safety margin
+    CBlockIndex *pindexMax = GRC::BlockFinder::FindByHeight(nBestHeight);
     std::map<CKeyID, CBlockIndex*> mapKeyFirstBlock;
     std::set<CKeyID> setKeys;
     GetKeys(setKeys);