Move LoadAddrman from init to addrdb #22754

1. 
   MarcoFalke commented at 10:56 am on August 20, 2021: member

   Init should only concern itself with the initialization order, not the detailed initialization logic of every module.

   Discussed in #22697 (review)

2. refactor: Fix includes (add missing ones, remove unneeded) fa3999a27e
3. in src/addrman.cpp:742 in fac39ac43e outdated

   735@@ -733,3 +736,22 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
   736     }
   737     return bits;
   738 }
   739+
   740+void LoadAddrman(std::unique_ptr<CAddrMan>& addrman, const ArgsManager& args)
   741+{
   742+    assert(!addrman);
   

   ---

   ---

   

   laanwj commented at 11:05 am on August 20, 2021:

   Maybe return std::unique_ptr<CAddrMan>? (or an optional, if it can fail, but doesn’t seem so). Passing it in then asserting seems unnecessary.

   ---

   

   MarcoFalke commented at 11:37 am on August 20, 2021:

   Done
4. 
   vasild commented at 11:11 am on August 20, 2021: member

   Concept ACK
5. in src/addrman.cpp:747 in fac39ac43e outdated

   742+    assert(!addrman);
   743+    auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
   744+    addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
   745+
   746+    // Load addresses from peers.dat
   747+    int64_t nStart = GetTimeMillis();
   

   ---

   ---

   

   jonatack commented at 11:28 am on August 20, 2021:

   nit, if you retouch

   0    const int64_t nStart{GetTimeMillis()};
   

   ---

   

   MarcoFalke commented at 11:38 am on August 20, 2021:

   Done
6. 
   jonatack commented at 11:28 am on August 20, 2021: member

   Concept ACK
7. MarcoFalke force-pushed on Aug 20, 2021
8. MarcoFalke renamed this:
   Move LoadAddrman from init to addrman
   Move LoadAddrman from init to addrdb
   on Aug 20, 2021
9. Move LoadAddrman from init to addrdb

   Init should only concern itself with the initialization order, not the
   detailed initialization logic of every module.

   faffb098fd
10. MarcoFalke force-pushed on Aug 20, 2021
11. DrahtBot added the label GUI on Aug 20, 2021
12. DrahtBot added the label P2P on Aug 20, 2021
13. MarcoFalke removed the label GUI on Aug 20, 2021
14. MarcoFalke removed the label P2P on Aug 20, 2021
15. MarcoFalke added the label Refactoring on Aug 20, 2021
16. 
    jnewbery commented at 1:46 pm on August 20, 2021: member

    I’m ~0 on this change. I don’t think there’s any real benefit in moving this initialization code from init.cpp to a free function in addrdb.cpp that gets called by init. That doesn’t seem to me to be reducing the complexity of what AppInitMain() is doing, rather than just hiding it behind a function call.
17. 
    MarcoFalke commented at 2:04 pm on August 20, 2021: member

    init.cpp is almost 2000 lines of code. I don’t see why init.cpp needs to include all existing headers to learn about the implementation details of each module to initialize them. Just like wallet initialization and block importing was moved out, I think the address database details can be moved out as well.
18. 
    DrahtBot commented at 4:08 pm on August 20, 2021: 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:

    • #20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)

    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.

19. 
    jnewbery commented at 4:41 pm on August 20, 2021: member

    I don’t see why init.cpp needs to include all existing headers to learn about the implementation details of each module to initialize them.

    This PR doesn’t reduce the number of headers that init.cpp includes (and I also don’t think it’s a problem for init to include lots of headers - after all, it’s init’s job to start everything up so it needs to include the headers for all the submodules).

    Just like wallet initialization and block importing was moved out, I think the address database details can be moved out as well.

    The difference with wallet initialization and block importing is that those are internal to the wallet and validation modules. Addrman initialization from peers.dat is something that’s being done to a CAddrMan externally, even after this PR.

    To be clear, I don’t object to this and I’m certainly not NACKing, I just don’t think it brings much benefit.

20. 
    MarcoFalke commented at 12:33 pm on August 21, 2021: member

    As this is required for some other changes, I am just going to bundle with them.
21. MarcoFalke closed this on Aug 21, 2021

    ---

22. MarcoFalke deleted the branch on Aug 21, 2021
23. DrahtBot locked this on Aug 21, 2022

Contributors
MarcoFalke laanwj vasild jonatack jnewbery DrahtBot

Labels
Refactoring