[p2p] Pimpl AddrMan to abstract implementation details

[amitiuttarwar]

Introduce the pimpl pattern for AddrMan to separate the implementation details from the externally used object representation. This reduces compile-time dependencies and conceptually clarifies AddrMan's interface from the implementation specifics.

Since the unit & fuzz tests currently rely on accessing AddrMan internals, this PR introduces addrman_impl.h, which is exclusively imported by addrman.cpp and test files.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23140 (Make CAddrman::Select_ select buckets, not positions, first by sipa)
• #23035 (p2p, rpc, test: expose tried and refcount in getnodeaddresses, update/improve tests by jonatack)
• #22937 (refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky)
• #22872 (log: improve checkaddrman logging with duration in milliseconds by jonatack)
• #22839 (log: improve addrman logging by mzumsande)
• #22508 (fuzz: replace every fuzzer-controlled while loop with a macro by apoelstra)
• #20295 (rpc: getblockfrompeer by Sjors)

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.

[amitiuttarwar]

Fixed the lint failure that was caused by the circular dependencies check with 95a066e. The check treated file_name.h and file_name.cpp as the same module, so the import pattern here of addrman.h <- addrman_impl.h <- addrman.cpp was perceived as a circular dependency. I fixed this by telling it to also treat file_name_impl.h as part of the same module.

More info about updated fix here: #22950 (comment)

[jnewbery]

Concept ACK. I'm a big fan of compilation firewall patterns. From Herb Sutter:

One big advantage of this idiom is that it breaks compile-time dependencies. First, system builds run faster because using a Pimpl can eliminate extra #includes. I have worked on projects where converting just a few widely-visible classes to use Pimpls has halved the system’s build time. Second, it localizes the build impact of code changes because the parts of a class that reside in the Pimpl can be freely changed – that is, members can be freely added or removed – without recompiling client code. Because it’s so good at eliminating compilation cascades due to changes in only the now-hidden members, it’s often dubbed a “compilation firewall.”

(https://herbsutter.com/gotw/_100/)

Both are relevant here. addrman.h is eventually included by just about everything, so minimizing its compilation time is potentially a big win. There's also a plan to rework its implementation. If we first make addrman into a pimpl, that could theoretically be done without any impact on the rest of the codebase.

[jonatack]

Concept ACK on improved separation and shorter build times. Debug build clean. Just read through pages 147-156 of Scott Meyers' "Effective Modern C++" about the (unique) pointer to implementation idiom and started looking at the commit organisation, particularly the choices in the second ("Introduce CAddrMan::Impl to encapsulate addrman implementation") and third ("Remove external dependencies on CAddrInfo objects") commits where the action appears to be.

Edit, noting for myself, also:

• Scott Meyers, "Effective C++", item 31
• Herb Sutter, "Exceptional C++", items 26-30

[laanwj]

Concept ACK

Both are relevant here. addrman.h is eventually included by just about everything, so minimizing its compilation time is potentially a big win.

Yes, at some point there was discussion about doing this for txmempool.h too, as it's included a fair bit and includes all these boost multi_index structures, which i'm sure slow down compilation quite a bit.

[amitiuttarwar]

adopted a different fix to the detected circular dependency- I made the impl a stand-alone class, so it's now AddrManImpl instead of AddrMan::Impl. this means addrman_impl.h does not need to import addrman.h, and the linter script does not need to be updated :)

[JeremyRubin]

concept ack!

[amitiuttarwar]

Thank you for the thoughtful reviews @mzumsande, @martinus, @jnewbery & @theuni !! I believe I've incorporated all the review comments.

A note on the latest push:

Mostly I rebased & addressed comments, but in doing so I added another commit- f2e5f38. I generally attempted to get the grouping / ordering of functions to be consistent, between the declarations & definitions in AddrMan & AddrManImpl. Since (for now) the functions are repeated a few times, having predictable ordering seems valuable for making the file easier for human parsing.

Thoughts on future work:

1. Reduce the redundancy of having all interface functions defined 3 times.

@mzumsande, @jnewbery, @theuni

With the recent changes to Check() allowing much more flexibility when and how often to call the checks, maybe it is not necessary anymore and the Check() calls could be merged into the Function_() ?

Do you think it would be overkill to have a variadic template CheckExecuteCheck() function that implements the pattern of locking cs, running Check(), calling the internal function, then running Check() again?

Agree with @mzumsande about cleaning up the internal functions as a next step,

Hehe, I've been meaning to play out the variadic template and take a look, it seems very appealing to reduce the boilerplate code. That said, I think incorporating Check() into Function_() would also be an improvement. One of these improvements should come next.

2. Improving the interface of Select & SelectTriedCollision

@theuni

It would be nice if we had a time type so it'd be obvious what the second member of the pair was in the return values from Select and friends

I agree, it's unfortunate that this PR made things slightly worse because addrman's interface now exposes an int64_t to represent time. I opted for this solution over making it a chrono in an attempt to keep the changes of this PR more mechanical.

Both of these functions are exclusively used by CConnman::ThreadOpenConnections, and I think the best solution would be to simplify the interfaces. When connman identifies that it is time to open a feeler, I think it should simply ask addrman for the top priority address. The concept of prioritizing tried collisions over a random connection should be internal to addrman.

Making the code change is not difficult, but we'd have to make the change with care because it would logically change the handling of repeated failures here. So, this is a long winded way of saying, I hope returning a pair is a temporary thing.

3. Improving doxygen comments of AddrMan functions - there are a few AddrMan interface functions that could benefit from improved comments.

[jnewbery]

ACK 021f869

The incremental commits make this really easy to review. Thank you!

[rajarshimaitra]

Concept + Code Review ACK 021f869

To check the effect of compilation firewall, I added a private integer member in CAddrMan of master and AddrManImpl of #22950.

Verified that the addition causes recompilation of many tests and and other object files in master, where as in case of #22950, only the compilation of relevant test files (that includes addrman_impl header) are triggered..

Overall the commit changes are very clear and easy to review. Compilation time reduction is significant and very much observable.

Also thanks for such an elaborate review club description. Helped me to learn a ton about compiler firewall.

Below is one naming question for my own understanding.

[GeneFerneau]

utACK 021f869

[mzumsande]

ACK 021f869

Reviewed the code and did some sanity checks running this for a while on mainnet with -checkaddrman=1, but no testing beyond that.

[theuni]

ACK 021f869

I had a last-minute panic that some of the unsuspicious moves from CAddrInfo to CAddress could quietly change the behavior of calls like addr.IsValid() if isValid had previously been overridden by CAddrInfo, but fortunately no such overrides existed.

[theuni]

Tiny nit for one of the refactor PRs: addresses.reserve(std::min(vRandom.size(), nNodes));

[theStack]

Post-merge concept and code-review ACK 021f869