Connection eviction logic (AttemptToEvictConnection) is not covered by our test suite #16660

issue practicalswift openend this issue on August 19, 2019
  1. practicalswift commented at 8:14 pm on August 19, 2019: contributor

    From what I can tell the connection eviction logic is not covered by our test suite (functional and unit tests).

    More specifically the following is never executed with nInbound >= nMaxInbound:

    https://github.com/bitcoin/bitcoin/blob/ed9a2a37c1878ba6b75c39b7cb263dc16c52a295/src/net.cpp#L962-L970

    Context:

    https://github.com/bitcoin/bitcoin/blob/ed9a2a37c1878ba6b75c39b7cb263dc16c52a295/src/net.cpp#L800-L808

    A test would be nice here :-)

    Due to AttemptToEvictConnection not being tested the following net.cpp functions – which are only called directly or indirectly via AttemptToEvictConnection – are currently untested:

    0static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
1static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
2static bool CompareNetGroupKeyed(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
3static bool CompareNodeBlockTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
4static bool CompareNodeTXTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
5static void EraseLastKElements(std::vector<T>& elements, Comparator comparator, size_t k)
  2. fanquake added the label Tests on Aug 20, 2019
  3. fanquake added the label P2P on Aug 20, 2019
  4. MarcoFalke commented at 2:54 pm on August 21, 2019: member
    Closing as duplicate of #14210
  5. MarcoFalke closed this on Aug 21, 2019
  6. practicalswift commented at 3:21 pm on August 21, 2019: contributor

    @MarcoFalke

    Do you know if we have any tests for the eviction logic? Perhaps my measurements are incorrect.

    AFAICT the eviction logic is by far the most critical code path not covered at all by any test. I mean “at all” in the most literal sense: the code path it isn’t even entered (if my measurements are correct) when running the full test suite :-)

    I’m not super convinced that this very specific observation/issue is a duplicate of the general observation/issue “we could do better network testing in general” (#14210). Fixing the former can be done quickly - fixing the latter will take quite some time. Don’t you think? :-)

  7. MarcoFalke commented at 4:00 pm on August 21, 2019: member
    Fixing #14210 is a requirement for fixing this issue. This issue is also mentioned as motivating example in #14210.
  8. practicalswift commented at 4:15 pm on August 21, 2019: contributor

    I can see the dependency on #14210 for functional testing, but unit tests could easily be written to cover these functions AFAICT:

    0static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
1static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
2static bool CompareNetGroupKeyed(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
3static bool CompareNodeBlockTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
4static bool CompareNodeTXTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
5static void EraseLastKElements(std::vector<T>& elements, Comparator comparator, size_t k)

    That would be good even if #14210 takes time to resolve :-)

  9. mzumsande commented at 1:06 am on August 23, 2019: member

    I think that parts of the eviction logic could be tested in a functional test also prior to #14210 being solved.

    The eviction logic applies to inbound peers and ignores outbound peers - we can simulate inbound connections with P2PInterface, and don’t need outbound peers to actually exist.

    I don’t see how the logic based on address/netgroup could be tested, but other parts can. There is a draft version of a functional test that partially covers the logic of CConnman::AttemptToEvictConnection in mzumsande@56d38d05c922ec37cae7ca957ad230a1dfbc4e96

    I intend to open a PR based on this in the next days, but for that PR I will also add a commit with unit tests (unless @practicalswift you are working on that?).

  10. practicalswift commented at 4:36 am on August 23, 2019: contributor

    @mzumsande Oh, that’s excellent news! Very glad to hear that you’re working on this! Thanks!

    […] (unless @practicalswift you are working on that?).

    No, I’m just sharing my observation: I was quite surprised to see that the code eviction code path wasn’t exercised at all when running the full test suite given how important it is that we get this logic correct. An error in this code could lead to attacker triggered network partitioning. @MarcoFalke Would you mind re-opening this ticket?

  11. MarcoFalke reopened this on Aug 26, 2019
  12. practicalswift closed this on May 19, 2020
  13. practicalswift commented at 11:28 am on May 19, 2020: contributor
    Closing due to lack of progress/interest :)
  14. jnewbery commented at 8:17 pm on May 31, 2020: member

    Closing due to lack of progress/interest :)

    #16756 has received some good review over the last two weeks, and we’re covering it in review club next week (https://bitcoincore.reviews/16756.html)

    I don’t think we need to re-open this issue, but it’s certainly not true that there isn’t progress/interest in this :)

  15. practicalswift commented at 8:18 pm on May 31, 2020: contributor
    Great news! :)
  16. practicalswift reopened this on May 31, 2020
  17. MarcoFalke referenced this in commit 5af16a4db7 on Jun 12, 2020
  18. MarcoFalke closed this on Jun 12, 2020
  19. sidhujag referenced this in commit 0fa1d20796 on Jun 13, 2020
  20. DrahtBot locked this on Feb 15, 2022
  21. PastaPastaPasta referenced this in commit c70bb67c0e on Mar 5, 2022
  22. vijaydasmp referenced this in commit 0ad2a86d8e on Dec 28, 2022
  23. vijaydasmp referenced this in commit 0e11ffcd01 on Dec 29, 2022
  24. vijaydasmp referenced this in commit 7220a8bd77 on Dec 30, 2022
  25. vijaydasmp referenced this in commit eb52fc9c09 on Dec 30, 2022
  26. vijaydasmp referenced this in commit 42a9c5cf8b on Dec 30, 2022
  27. vijaydasmp referenced this in commit 9276609261 on Dec 30, 2022
  28. vijaydasmp referenced this in commit 42df3431b7 on Jan 3, 2023
  29. vijaydasmp referenced this in commit eb0d1d8ca0 on Jan 4, 2023
  30. vijaydasmp referenced this in commit 7f1ba8a5b1 on Jan 4, 2023
  31. vijaydasmp referenced this in commit 5460d64684 on Jan 4, 2023
  32. vijaydasmp referenced this in commit d97bbba3ec on Jan 4, 2023
  33. vijaydasmp referenced this in commit e316987eba on Jan 6, 2023
  34. vijaydasmp referenced this in commit 5ad9c1a06b on Jan 17, 2023
  35. vijaydasmp referenced this in commit d95e9080df on Jan 17, 2023
  36. vijaydasmp referenced this in commit 9df469a180 on Jan 24, 2023
  37. vijaydasmp referenced this in commit b0ceda77b0 on Jan 24, 2023
  38. vijaydasmp referenced this in commit 10af13670b on Feb 14, 2023
  39. vijaydasmp referenced this in commit 65d761dbdb on Feb 15, 2023
  40. vijaydasmp referenced this in commit 71a33951ec on Feb 15, 2023
  41. vijaydasmp referenced this in commit 4bd7335f7d on Feb 18, 2023
  42. UdjinM6 referenced this in commit 0de5f7018a on Feb 20, 2023
  43. vijaydasmp referenced this in commit ca0475d0d9 on Feb 20, 2023
  44. vijaydasmp referenced this in commit ae0f0a0f7c on Feb 21, 2023
  45. PastaPastaPasta referenced this in commit 434420f120 on Feb 28, 2023


practicalswift MarcoFalke mzumsande jnewbery

Labels
Tests P2P

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: 2024-11-17 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me