refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight #15670

1. 
   ariard commented at 5:04 pm on March 26, 2019: member

   As suggested in #14711, pass height to CChain::FindEarliestAtLeast to simplify Chain interface by combining findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one
2. 
   DrahtBot commented at 8:27 pm on March 26, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

3. fanquake added the label Refactoring on Mar 26, 2019
4. RayHacker96 approved
5. in src/chain.cpp:66 in d43bff5e29 outdated

   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
6. in src/test/skiplist_tests.cpp:163 in d43bff5e29 outdated

   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.
7. MarcoFalke renamed this:
   refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTim…
   refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight
   on Mar 27, 2019
8. refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight

   As suggested in #14711, pass height to CChain::FindEarliestAtLeast to
   simplify Chain interface by combining findFirstBlockWithTime and
   findFirstBlockWithTimeAndHeight into one
   
   Extend findearliestatleast_edge_test in consequence

   765c0b364d
9. ariard force-pushed on Mar 27, 2019
10. ryanofsky approved
11. 
    ryanofsky commented at 4:27 pm on March 29, 2019: member

    utACK 765c0b364d41e9a251c3f88cbe203645854fd790. Looks good, thanks for implementing the suggestion! @jnewbery, you may be interested to review this since you first made the suggestion in #14711 (review)
12. 
    jnewbery commented at 9:29 pm on March 29, 2019: member

    utACK 765c0b364d41e9a251c3f88cbe203645854fd790. Nice work @ariard!
13. MarcoFalke merged this on Apr 19, 2019
14. MarcoFalke closed this on Apr 19, 2019

    ---

15. MarcoFalke referenced this in commit 56376f3365 on Apr 19, 2019
16. deadalnix referenced this in commit 6f4857cdbc on May 27, 2020
17. DrahtBot locked this on Dec 16, 2021

Contributors
ariard DrahtBot RayHacker96 ryanofsky jnewbery

Labels
Refactoring