From 997a98a674df70a2192e8d8b91c631e5c241509d Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sun, 8 Jan 2017 04:05:14 +0000 Subject: [PATCH 1/2] Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. In spite of the name FindLatestBefore used std::lower_bound to try to find the earliest block with a nTime greater or equal to the the requested value. But lower_bound uses bisection and requires the input to be ordered with respect to the comparison operation. Block times are not well ordered. I don't know what lower_bound is permitted to do when the data is not sufficiently ordered, but it's probably not good. (I could construct an implementation which would infinite loop...) To resolve the issue this commit introduces a maximum-so-far to the block indexes and searches that. For clarity the function is renamed to reflect what it actually does. An issue that remains is that there is no grace period in importmulti: If a address is created at time T and a send is immediately broadcast and included by a miner with a slow clock there may not yet have been any block with at least time T. The normal rescan has a grace period of 7200 seconds, but importmulti does not. --- src/chain.cpp | 4 ++-- src/chain.h | 13 +++++++++++-- src/rpc/blockchain.cpp | 4 ++-- src/validation.cpp | 2 ++ src/wallet/rpcdump.cpp | 4 ++-- 5 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/chain.cpp b/src/chain.cpp index 3cd7b3392..0f4d422b9 100644 --- a/src/chain.cpp +++ b/src/chain.cpp @@ -61,10 +61,10 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const { return pindex; } -CBlockIndex* CChain::FindLatestBefore(int64_t nTime) const +CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime) const { std::vector::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime, - [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTime() < time; }); + [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTimeMax() < time; }); return (lower == vChain.end() ? NULL : *lower); } diff --git a/src/chain.h b/src/chain.h index 989a71958..acb29b667 100644 --- a/src/chain.h +++ b/src/chain.h @@ -202,6 +202,9 @@ public: //! (memory only) Sequential id assigned to distinguish order in which blocks are received. int32_t nSequenceId; + //! (memory only) Maximum nTime in the chain upto and including this block. + unsigned int nTimeMax; + void SetNull() { phashBlock = NULL; @@ -216,6 +219,7 @@ public: nChainTx = 0; nStatus = 0; nSequenceId = 0; + nTimeMax = 0; nVersion = 0; hashMerkleRoot = uint256(); @@ -281,6 +285,11 @@ public: return (int64_t)nTime; } + int64_t GetBlockTimeMax() const + { + return (int64_t)nTimeMax; + } + enum { nMedianTimeSpan=11 }; int64_t GetMedianTimePast() const @@ -461,8 +470,8 @@ public: /** Find the last common block between this chain and a block index entry. */ const CBlockIndex *FindFork(const CBlockIndex *pindex) const; - /** Find the most recent block with timestamp lower than the given. */ - CBlockIndex* FindLatestBefore(int64_t nTime) const; + /** Find the earliest block with timestamp equal or greater than the given. */ + CBlockIndex* FindEarliestAtLeast(int64_t nTime) const; }; #endif // BITCOIN_CHAIN_H diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index bbcbde71c..368654bfa 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -839,9 +839,9 @@ UniValue pruneblockchain(const JSONRPCRequest& request) // Height value more than a billion is too high to be a block height, and // too low to be a block time (corresponds to timestamp from Sep 2001). if (heightParam > 1000000000) { - CBlockIndex* pindex = chainActive.FindLatestBefore(heightParam); + CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam); if (!pindex) { - throw JSONRPCError(RPC_INTERNAL_ERROR, "Could not find block before specified timestamp."); + throw JSONRPCError(RPC_INTERNAL_ERROR, "Could not find block with at least the specified timestamp."); } heightParam = pindex->nHeight; } diff --git a/src/validation.cpp b/src/validation.cpp index 3faa1bf00..625307b6a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2606,6 +2606,7 @@ CBlockIndex* AddToBlockIndex(const CBlockHeader& block) pindexNew->nHeight = pindexNew->pprev->nHeight + 1; pindexNew->BuildSkip(); } + pindexNew->nTimeMax = (pindexNew->pprev ? std::max(pindexNew->pprev->nTimeMax, pindexNew->nTime) : pindexNew->nTime); pindexNew->nChainWork = (pindexNew->pprev ? pindexNew->pprev->nChainWork : 0) + GetBlockProof(*pindexNew); pindexNew->RaiseValidity(BLOCK_VALID_TREE); if (pindexBestHeader == NULL || pindexBestHeader->nChainWork < pindexNew->nChainWork) @@ -3416,6 +3417,7 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) { CBlockIndex* pindex = item.second; pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex); + pindex->nTimeMax = (pindex->pprev ? std::max(pindex->pprev->nTimeMax, pindex->nTime) : pindex->nTime); // We can link the chain of blocks for which we've received transactions at some point. // Pruned nodes may have deleted the block. if (pindex->nTx > 0) { diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index a1912a78e..7d4ed70ed 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1048,8 +1048,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) } } - if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTime()) { - CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindLatestBefore(nLowestTimestamp) : chainActive.Genesis(); + if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) { + CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis(); if (pindex) { pwalletMain->ScanForWalletTransactions(pindex, true); From 4b06e41c309123c4eefce0d3578c01efe1ae10df Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 11 Jan 2017 21:31:00 -0500 Subject: [PATCH 2/2] Add unit test for FindEarliestAtLeast --- src/test/skiplist_tests.cpp | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/test/skiplist_tests.cpp b/src/test/skiplist_tests.cpp index 5b4ef3fe7..0b2fe0ef9 100644 --- a/src/test/skiplist_tests.cpp +++ b/src/test/skiplist_tests.cpp @@ -100,4 +100,47 @@ BOOST_AUTO_TEST_CASE(getlocator_test) } } +BOOST_AUTO_TEST_CASE(findearliestatleast_test) +{ + std::vector vHashMain(100000); + std::vector vBlocksMain(100000); + for (unsigned int i=0; inTimeMax >= test_time); + BOOST_CHECK((ret->pprev==NULL) || ret->pprev->nTimeMax < test_time); + BOOST_CHECK(vBlocksMain[r].GetAncestor(ret->nHeight) == ret); + } +} BOOST_AUTO_TEST_SUITE_END()