Better intervals between feelers #19869

1. 
   naumenkogs commented at 11:02 am on September 4, 2020: member

   ~Draft pending on #19958~

   1. If an attempt to connect to a feeler wasn’t actually made (an entry from AddrMan we considered was invalid, or we made too many tries, or it didn’t have the right flag) — wait less until we try the next feeler.
   2. Make those intervals mockable
   3. Remove unnecessary extra random noise between feelers

   (1) would make transitioning from “new” to “tried” better in the presence of those events. An adversary here fills our “new” table with garbage to prevent us from trying honest peers from “new”. This probably won’t help much against a very powerful adversary but will help against a moderate adversary. It also may help in natural non-malicious cases.

   Moving from “new” to “tried” table is important for p2p security.

2. fanquake added the label P2P on Sep 4, 2020
3. 
   DrahtBot commented at 12:17 pm on September 4, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19884 (p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty by dhruv)
   • #19858 (Periodically make block-relay connections and sync headers by sdaftuar)
   • #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. naumenkogs marked this as a draft on Sep 15, 2020
5. naumenkogs force-pushed on Oct 5, 2020
6. naumenkogs force-pushed on Oct 5, 2020
7. naumenkogs marked this as ready for review on Oct 5, 2020
8. Wait less if last feeler candidate wasn't tried

   We should distinguish the case when we selected a feeler
   and attempted connecting to it from the case when we didn't
   even try: maybe the record was invalid, or too many attempts
   were made. In the latter case, make next feeler attempt sooner.

   3a1c487e60
9. Make feeler intervals mockable 69ed426924
10. Drop unnecessary random noise in feeler intervals

    Feeler intervals are already poisson-randomized, no need
    to add extra random noise.

    9feba975c1
11. naumenkogs force-pushed on Oct 5, 2020
12. DrahtBot added the label Needs rebase on Oct 15, 2020
13. 
    DrahtBot commented at 7:34 pm on October 15, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

14. 
    sdaftuar commented at 2:26 pm on October 20, 2020: member

    Concept ACK improvements along these lines. However, I’m not sure about the more complicated interval logic around the feeler connections – it seems to me like that logic is already a bit hard to follow, so I wonder if a better approach might be to just check for invalidity in the while (!interruptNet) loop, and be willing to iterate in that loop many more times if fFeeler is set?
15. 
    naumenkogs commented at 1:16 pm on October 22, 2020: member

    @sdaftuar I agree with the high-level idea. Even though I don’t think I’m making it significantly harder to follow, simplifying is a good idea.

    But let me check, are you suggesting just to carefully use continue instead of break inside the loop?

16. 
    naumenkogs commented at 11:28 am on September 15, 2021: member

    Closing this:

    1. Mockable time is already done.
    2. Smarter intervals probably better be done after a refactoring (gonna do that in another PR first)
    3. Dropping extra noise probably ain’t worth it on its own
17. naumenkogs closed this on Sep 15, 2021

    ---

18. DrahtBot locked this on Oct 30, 2022

Contributors
naumenkogs DrahtBot sdaftuar

Labels
P2P Needs rebase