Do not store more than 200 timedata samples. #6545

1. 
   paveljanik commented at 2:14 pm on August 11, 2015: contributor

   As reported in the issue #6490, Bitcoin Core keeps and extends std::set of CNetAddr for all peers during the whole server run. Even for peers that do not affect the adjusted time of the Core at all.

   This change makes it clear (do nothing after 200 timedata samples), saves some CPU time and memory.

2. Do not store more than 200 timedata samples. 8be371db34
3. in src/timedata.cpp: in 8be371db34

   46 {
   47     LOCK(cs_nTimeOffset);
   48     // Ignore duplicates
   49     static set<CNetAddr> setKnown;
   50+    if (setKnown.size() == BITCOIN_TIMEDATA_MAX_SAMPLES)
   51+        return;
   

   ---

   ---

   

   jonasschnelli commented at 2:30 pm on August 11, 2015:

   I didn’t know the behavior in details: but does this mean if we have reached the BITCOIN_TIMEDATA_MAX_SAMPLES of 200 we run into a risk of not detecting a significant time offset anymore? Why not shift out the CNetAddr instance at index 0 if we have reached 200 (and still add the new one)?

   ---

   

   paveljanik commented at 3:45 pm on August 11, 2015:

   Please read the comment just after the change in the original code: https://github.com/bitcoin/bitcoin/blob/master/src/timedata.cpp#L56

   This PR does not change the behaviour described there at all. I do not even plan to change the current “use the timedata samples from the first 200 peers we have seen” to “use the timedata samples from the last 200 peers we have seen” because of the comments there…

   The purpose of this PR is to stop saving both CNetAddr for 200+th peers and rotating timedata samples because they will not be used at all anyway.

   ---

   

   theuni commented at 5:47 pm on August 11, 2015:

   Wow, that’s subtle. I thought the same as @jonasschnelli until re-reading the comments a few times.

   Concept ACK, but I’d prefer to see it implemented in a way such that the early return depends on the quirk, otherwise I’m afraid it could introduce new bugs down the road. At the very least it deserves its own comment.

   ---

   

   paveljanik commented at 5:52 pm on August 11, 2015:

   I had a comment there but have deleted it because it said something like

   0// Do not accept more timedata samples (see the comment below).
   

   But I have deleted it (it looks so clean with the comment below, that I thought it is better not comment it). The long comment below should probably just be moved up… What do you think?

   ---

   

   paveljanik commented at 5:54 pm on August 11, 2015:

   @theuni Can you please translate “the early return depends on the quirk” for me? I’m not native English speaker and this is not clear to me. How it should be implemented?

   ---

   

   theuni commented at 3:28 pm on August 13, 2015:

   @paveljanik Nevermind, it looks like there’s no good way to do that anyway.

   ---

   

   paveljanik commented at 9:34 pm on August 13, 2015:

   … without changing the way it works. Yes. I do not want to change it at all.

   If we agree on it, the next step could be getting rid of CMedianFilter, using plain std::set with 200 entries…

   ---

   

   laanwj commented at 3:11 pm on August 20, 2015:

   @paveljanik I wouldn’t spend time on this as-is unless it is broken (like this leak). This whole code needs to be replaced, @gmaxwell had some good ideas.
4. 
   laanwj commented at 7:26 am on August 12, 2015: member

   utACK. Nice catch!
5. laanwj added the label Bug on Aug 12, 2015
6. 
   btcdrak commented at 7:36 am on August 12, 2015: contributor

   utACK
7. laanwj merged this on Aug 20, 2015
8. laanwj closed this on Aug 20, 2015

   ---

9. laanwj referenced this in commit e128464bc5 on Aug 20, 2015
10. 
    laanwj commented at 3:14 pm on August 20, 2015: member

    backported to 0.11 as 649f5d9c1100bb3cbace724900f035939df5ea11
11. laanwj referenced this in commit 649f5d9c11 on Aug 20, 2015
12. paveljanik referenced this in commit c57fa5174d on Oct 10, 2015
13. paveljanik referenced this in commit 40bd04fac2 on Oct 10, 2015
14. random-zebra referenced this in commit 3337601431 on Oct 6, 2019
15. random-zebra referenced this in commit 03fe6df30a on Oct 7, 2019
16. random-zebra referenced this in commit 163e3bf658 on Oct 9, 2019
17. random-zebra referenced this in commit 0ebcbd6578 on Oct 12, 2019
18. random-zebra referenced this in commit 7db567488b on Oct 18, 2019
19. random-zebra referenced this in commit dece5a3258 on Oct 24, 2019
20. random-zebra referenced this in commit f5ec6a30c1 on Oct 27, 2019
21. random-zebra referenced this in commit 04b5c22891 on Oct 27, 2019
22. random-zebra referenced this in commit 97b32725e2 on Oct 28, 2019
23. random-zebra referenced this in commit cd4c65026f on Nov 8, 2019
24. random-zebra referenced this in commit 1133a74248 on Nov 14, 2019
25. random-zebra referenced this in commit b148421570 on Nov 14, 2019
26. random-zebra referenced this in commit c254df6e47 on Nov 20, 2019
27. CaveSpectre11 referenced this in commit 36acd7d5d7 on Dec 14, 2019
28. wqking referenced this in commit 90c2eba2b3 on May 28, 2020
29. wqking referenced this in commit 84da44e1b6 on Feb 10, 2021
30. DrahtBot locked this on Sep 8, 2021

Contributors
paveljanik jonasschnelli theuni laanwj btcdrak

Labels
Bug