Make FindLatestBefore use GetMedianTimePast instead of GetBlockTime. #9489

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:fix_find_latest_before changing 1 files +1 −1
  1. gmaxwell commented at 4:19 AM on January 8, 2017: contributor

    Importmulti uses FindLatestBefore to find the height to rescan from, FindLatestBefore is using the block time. This results in a couple issues:

    One issue is that miners are able to put times in the past on blocks: You could make an address at time T, send to it at T+1 and have that confirmed in a block with a timestamp T-1.

    A bigger issue is that FindLatestBefore uses std::lower_bound which bisects to find the location, which requires that the iterator be ordered with respect to the comparison function. Block times are not ordered! I don't know what lower_bound is permitted to do when your data is not sufficiently ordered, but it's probably not good. (I could construct a plausible implementation which would infinite loop.)

    Instead, we change to use GetMedianTimePast. Block times are ordered with respect to the mediantimepast. MTP advances with the consent of multiple miners so it is more likely to be sufficient for our purpose, and it is offset back a bit so it is more conservative.

    The result might rescan a couple extra blocks, but that is no big deal. If the mediantime calculation is ever a performance concern we could cache it in the future.

  2. Make FindLatestBefore use GetMedianTimePast instead of GetBlockTime.
    Importmulti uses FindLatestBefore to find the height to rescan from,
     FindLatestBefore is using the block time.  This results in a couple
     issues:
    
    One issue is that miners are able to put times in the past on blocks:
     You could make an address at time T, send to it at T+1 and
     have that confirmed in a block with a timestamp T-1.
    
    A bigger issue is that FindLatestBefore uses std::lower_bound which
     bisects to find the location, which requires that the iterator be
     ordered with respect to the comparison function. Block times are
     not ordered!  I don't know what lower_bound is permitted to do when
     your data is not sufficiently ordered, but it's probably not good.
     (I could construct a plausible implementation which would infinite
      loop.)
    
    Instead, we change to use GetMedianTimePast. Block times are ordered
     with respect to the mediantimepast. MTP advances with the consent
     of multiple miners so it is more likely to be sufficient for our
     purpose, and it is offset back a bit so it is more conservative.
    
    The result might rescan a couple extra blocks, but that is no big
     deal.  If the mediantime calculation is ever a performance concern
     we could cache it in the future.
    32b2b65942
  3. gmaxwell commented at 4:19 AM on January 8, 2017: contributor

    Please tag for 0.14.

  4. fanquake added this to the milestone 0.14.0 on Jan 8, 2017
  5. gmaxwell commented at 4:54 AM on January 8, 2017: contributor

    darn wrong fix, MTP is conservative in the wrong direction. :-/

  6. gmaxwell closed this on Jan 8, 2017

  7. MarcoFalke locked this on Sep 8, 2021
Contributors

Milestone
0.14.0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-18 21:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me