Merge #15670: refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight

MarcoFalke · MarcoFalke · commit 56376f336548 · 2019-04-19T12:03:12.000-04:00
765c0b3 refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight (Antoine Riard) Pull request description: As suggested in #14711, pass height to CChain::FindEarliestAtLeast to simplify Chain interface by combining findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one ACKs for commit 765c0b: jnewbery: utACK 765c0b3. Nice work @ariard! ryanofsky: utACK 765c0b3. Looks good, thanks for implementing the suggestion! Tree-SHA512: 63f98252a93da95f08c0b6325ea98f717aa9ae4036d17eaa6edbec68e5ddd65672d66a6af267b80c36311fffa9b415a47308e95ea7718b300b685e23d4e9e6ec
diff --git a/src/chain.cpp b/src/chain.cpp
@@ -59,10 +59,11 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
     return pindex;
 }
 
-CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime) const
+CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime, int height) const
 {
-    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime,
-        [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTimeMax() < time; });
+    std::pair<int64_t, int> blockparams = std::make_pair(nTime, height);
+    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), blockparams,
+        [](CBlockIndex* pBlock, const std::pair<int64_t, int>& blockparams) -> bool { return pBlock->GetBlockTimeMax() < blockparams.first || pBlock->nHeight < blockparams.second; });
     return (lower == vChain.end() ? nullptr : *lower);
 }
 
diff --git a/src/chain.h b/src/chain.h
@@ -465,8 +465,8 @@ class CChain {
     /** Find the last common block between this chain and a block index entry. */
     const CBlockIndex *FindFork(const CBlockIndex *pindex) const;
 
-    /** Find the earliest block with timestamp equal or greater than the given. */
-    CBlockIndex* FindEarliestAtLeast(int64_t nTime) const;
+    /** Find the earliest block with timestamp equal or greater than the given time and height equal or greater than the given height. */
+    CBlockIndex* FindEarliestAtLeast(int64_t nTime, int height) const;
 };
 
 #endif // BITCOIN_CHAIN_H
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
@@ -84,29 +84,15 @@ class LockImpl : public Chain::Lock
         CBlockIndex* block = ::chainActive[height];
         return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
     }
-    Optional<int> findFirstBlockWithTime(int64_t time, uint256* hash) override
+    Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
     {
-        CBlockIndex* block = ::chainActive.FindEarliestAtLeast(time);
+        CBlockIndex* block = ::chainActive.FindEarliestAtLeast(time, height);
         if (block) {
             if (hash) *hash = block->GetBlockHash();
             return block->nHeight;
         }
         return nullopt;
     }
-    Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height) override
-    {
-        // TODO: Could update CChain::FindEarliestAtLeast() to take a height
-        // parameter and use it with std::lower_bound() to make this
-        // implementation more efficient and allow combining
-        // findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one
-        // method.
-        for (CBlockIndex* block = ::chainActive[height]; block; block = ::chainActive.Next(block)) {
-            if (block->GetBlockTime() >= time) {
-                return block->nHeight;
-            }
-        }
-        return nullopt;
-    }
     Optional<int> findPruned(int start_height, Optional<int> stop_height) override
     {
         if (::fPruneMode) {
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -104,21 +104,12 @@ class Chain
         //! pruned), and contains transactions.
         virtual bool haveBlockOnDisk(int height) = 0;
 
-        //! Return height of the first block in the chain with timestamp equal
-        //! or greater than the given time, or nullopt if there is no block with
-        //! a high enough timestamp. Also return the block hash as an optional
-        //! output parameter (to avoid the cost of a second lookup in case this
-        //! information is needed.)
-        virtual Optional<int> findFirstBlockWithTime(int64_t time, uint256* hash) = 0;
-
         //! Return height of the first block in the chain with timestamp equal
         //! or greater than the given time and height equal or greater than the
-        //! given height, or nullopt if there is no such block.
-        //!
-        //! Calling this with height 0 is equivalent to calling
-        //! findFirstBlockWithTime, but less efficient because it requires a
-        //! linear instead of a binary search.
-        virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height) = 0;
+        //! given height, or nullopt if there is no block with a high enough
+        //! timestamp and height. Also return the block hash as an optional output parameter
+        //! (to avoid the cost of a second lookup in case this information is needed.)
+        virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;
 
         //! Return height of last block in the specified range which is pruned, or
         //! nullopt if no block in the range is pruned. Range is inclusive.
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -1012,7 +1012,7 @@ static UniValue pruneblockchain(const JSONRPCRequest& request)
     // too low to be a block time (corresponds to timestamp from Sep 2001).
     if (heightParam > 1000000000) {
         // Add a 2 hour buffer to include blocks which might have had old timestamps
-        CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW);
+        CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0);
         if (!pindex) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Could not find block with at least the specified timestamp.");
         }
diff --git a/src/test/skiplist_tests.cpp b/src/test/skiplist_tests.cpp
@@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(findearliestatleast_test)
         // Pick a random element in vBlocksMain.
         int r = InsecureRandRange(vBlocksMain.size());
         int64_t test_time = vBlocksMain[r].nTime;
-        CBlockIndex *ret = chain.FindEarliestAtLeast(test_time);
+        CBlockIndex* ret = chain.FindEarliestAtLeast(test_time, 0);
         BOOST_CHECK(ret->nTimeMax >= test_time);
         BOOST_CHECK((ret->pprev==nullptr) || ret->pprev->nTimeMax < test_time);
         BOOST_CHECK(vBlocksMain[r].GetAncestor(ret->nHeight) == ret);
@@ -158,22 +158,34 @@ BOOST_AUTO_TEST_CASE(findearliestatleast_edge_test)
     CChain chain;
     chain.SetTip(&blocks.back());
 
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(50)->nHeight, 0);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(100)->nHeight, 0);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(150)->nHeight, 3);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(200)->nHeight, 3);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(250)->nHeight, 6);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(300)->nHeight, 6);
-    BOOST_CHECK(!chain.FindEarliestAtLeast(350));
-
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0)->nHeight, 0);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(-1)->nHeight, 0);
-
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::min())->nHeight, 0);
-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(-int64_t(std::numeric_limits<unsigned int>::max()) - 1)->nHeight, 0);
-    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::max()));
-    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<unsigned int>::max()));
-    BOOST_CHECK(!chain.FindEarliestAtLeast(int64_t(std::numeric_limits<unsigned int>::max()) + 1));
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(50, 0)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(100, 0)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(150, 0)->nHeight, 3);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(200, 0)->nHeight, 3);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(250, 0)->nHeight, 6);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(300, 0)->nHeight, 6);
+    BOOST_CHECK(!chain.FindEarliestAtLeast(350, 0));
+
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0, 0)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(-1, 0)->nHeight, 0);
+
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::min(), 0)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(-int64_t(std::numeric_limits<unsigned int>::max()) - 1, 0)->nHeight, 0);
+    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::max(), 0));
+    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<unsigned int>::max(), 0));
+    BOOST_CHECK(!chain.FindEarliestAtLeast(int64_t(std::numeric_limits<unsigned int>::max()) + 1, 0));
+
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0, -1)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0, 0)->nHeight, 0);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0, 3)->nHeight, 3);
+    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(0, 8)->nHeight, 8);
+    BOOST_CHECK(!chain.FindEarliestAtLeast(0, 9));
+
+    CBlockIndex* ret1 = chain.FindEarliestAtLeast(100, 2);
+    BOOST_CHECK(ret1->nTimeMax >= 100 && ret1->nHeight == 2);
+    BOOST_CHECK(!chain.FindEarliestAtLeast(300, 9));
+    CBlockIndex* ret2 = chain.FindEarliestAtLeast(200, 4);
+    BOOST_CHECK(ret2->nTimeMax >= 200 && ret2->nHeight == 4);
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -1728,7 +1728,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
     uint256 start_block;
     {
         auto locked_chain = chain().lock();
-        const Optional<int> start_height = locked_chain->findFirstBlockWithTime(startTime - TIMESTAMP_WINDOW, &start_block);
+        const Optional<int> start_height = locked_chain->findFirstBlockWithTimeAndHeight(startTime - TIMESTAMP_WINDOW, 0, &start_block);
         const Optional<int> tip_height = locked_chain->getHeight();
         WalletLogPrintf("%s: Rescanning last %i blocks\n", __func__, tip_height && start_height ? *tip_height - *start_height + 1 : 0);
     }
@@ -4257,7 +4257,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         // No need to read and scan block if block was created before
         // our wallet birthday (as adjusted for block time variability)
         if (walletInstance->nTimeFirstKey) {
-            if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height)) {
+            if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
                 rescan_height = *first_block;
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -59,10 +59,11 @@ const CBlockIndex CChain::FindFork(const CBlockIndex pindex) const {`
`59`	`59`	`return pindex;`
`60`	`60`	`}`
`61`	`61`
`62`		`-CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime) const`
	`62`	`+CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime, int height) const`
`63`	`63`	`{`
`64`		`- std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime,`
`65`		`- [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTimeMax() < time; });`
	`64`	`+ std::pair<int64_t, int> blockparams = std::make_pair(nTime, height);`
	`65`	`+ std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), blockparams,`
	`66`	`+ [](CBlockIndex* pBlock, const std::pair<int64_t, int>& blockparams) -> bool { return pBlock->GetBlockTimeMax() < blockparams.first \|\| pBlock->nHeight < blockparams.second; });`
`66`	`67`	`return (lower == vChain.end() ? nullptr : *lower);`
`67`	`68`	`}`
`68`	`69`
Original file line number	Diff line number	Diff line change
`@@ -1012,7 +1012,7 @@ static UniValue pruneblockchain(const JSONRPCRequest& request)`
`1012`	`1012`	`// too low to be a block time (corresponds to timestamp from Sep 2001).`
`1013`	`1013`	`if (heightParam > 1000000000) {`
`1014`	`1014`	`// Add a 2 hour buffer to include blocks which might have had old timestamps`
`1015`		`- CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW);`
	`1015`	`+ CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0);`
`1016`	`1016`	`if (!pindex) {`
`1017`	`1017`	`throw JSONRPCError(RPC_INVALID_PARAMETER, "Could not find block with at least the specified timestamp.");`
`1018`	`1018`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1728,7 +1728,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r`
`1728`	`1728`	`uint256 start_block;`
`1729`	`1729`	`{`
`1730`	`1730`	`auto locked_chain = chain().lock();`
`1731`		`- const Optional<int> start_height = locked_chain->findFirstBlockWithTime(startTime - TIMESTAMP_WINDOW, &start_block);`
	`1731`	`+ const Optional<int> start_height = locked_chain->findFirstBlockWithTimeAndHeight(startTime - TIMESTAMP_WINDOW, 0, &start_block);`
`1732`	`1732`	`const Optional<int> tip_height = locked_chain->getHeight();`
`1733`	`1733`	`WalletLogPrintf("%s: Rescanning last %i blocks\n", __func__, tip_height && start_height ? tip_height - start_height + 1 : 0);`
`1734`	`1734`	`}`
`@@ -4257,7 +4257,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,`
`4257`	`4257`	`// No need to read and scan block if block was created before`
`4258`	`4258`	`// our wallet birthday (as adjusted for block time variability)`
`4259`	`4259`	`if (walletInstance->nTimeFirstKey) {`
`4260`		`- if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height)) {`
	`4260`	`+ if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {`
`4261`	`4261`	`rescan_height = *first_block;`
`4262`	`4262`	`}`
`4263`	`4263`	`}`