refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight #15670

  64 |  {
  65 | -    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime,
  66 | -        [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTimeMax() < time; });
  67 | +    std::pair<int64_t, int> blockparams = std::make_pair(nTime, height);
  68 | +    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), blockparams,
  69 | +        [](CBlockIndex* pBlock, const std::pair<int64_t, int>& blockparams) -> bool { return pBlock->GetBlockTimeMax() < blockparams.first && pBlock->nHeight < blockparams.second; });

ryanofsky commented at 8:06 PM on March 27, 2019:

It seems like this should say || not &&. If either the time is to low or the height is too small, the block is too early?

ariard commented at 9:33 PM on March 27, 2019:

Yes, already corrected locally and tests seem ok, wasn't sure of std:lower_bound behavior at first

 174 | -    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::max()));
 175 | -    BOOST_CHECK(!chain.FindEarliestAtLeast(std::numeric_limits<unsigned int>::max()));
 176 | -    BOOST_CHECK(!chain.FindEarliestAtLeast(int64_t(std::numeric_limits<unsigned int>::max()) + 1));
 177 | +    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(50, 0)->nHeight, 0);
 178 | +    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(100, 0)->nHeight, 0);
 179 | +    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(150, 3)->nHeight, 3);

ryanofsky commented at 8:08 PM on March 27, 2019:

This decreases test coverage for bisecting based on time.

It would be better if this PR just added a few new tests for nonzero heights, and left the existing tests alone (by passing 0 minimum heights).

ariard commented at 10:32 PM on March 27, 2019:

Done, also few tests covering both block attributes.