Move LoadAddrman from init to addrdb #22754

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2108-initAddrDb changing 11 files +34 −19

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)
refactor: Fix includes (add missing ones, remove unneeded) fa3999a27e
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
vasild commented at 11:11 AM on August 20, 2021: member

Concept ACK

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

    const int64_t nStart{GetTimeMillis()};

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

Done

jonatack commented at 11:28 AM on August 20, 2021: member

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

Move LoadAddrman from init to addrdb

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

faffb098fd

MarcoFalke force-pushed on Aug 20, 2021
DrahtBot added the label GUI on Aug 20, 2021
DrahtBot added the label P2P on Aug 20, 2021
MarcoFalke removed the label GUI on Aug 20, 2021
MarcoFalke removed the label P2P on Aug 20, 2021
MarcoFalke added the label Refactoring on Aug 20, 2021
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.
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.
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.
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.
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.
MarcoFalke closed this on Aug 21, 2021
MarcoFalke deleted the branch on Aug 21, 2021
DrahtBot locked this on Aug 21, 2022

Contributors

Labels

Linked (view graph)

#22697 addrman: Remove CAddrMan::Clear() function

Move LoadAddrman from init to addrdb #22754

Conflicts