refactor: Nuke policy/fees->mempool circular dependencies #17786

This PR:

• gets rid of the policy/fees -> txmempool -> policy/fees circular dependency
• is an alternative to #13949, which nukes only one circular dependency

Related discussion:

• #15638 (review) by @ryanofsky
• #13949 (comment) by @MarcoFalke

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
• #25361 (BIP324: Cipher suite by dhruv)
• #25325 (Add pool based memory resource by martinus)
• #24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
• #23962 (Use int32_t type for transaction size/weight consistently by hebasto)
• #23561 (BIP324: Handshake prerequisites by dhruv)
• #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

Concept ACK.

ACK fbf6b7524eac8a7668e49b7c340d06447a9d248d.

I'd squash 1st and 2nd commits. 3rd commit is unrelated but is a nice cleanup.

Concept ACK: thanks for nuking circular dependencies

@promag

I'd squash 1st and 2nd commits. 3rd commit is unrelated but is a nice cleanup.

Done.

~I stumble on UB sanitizer. How could a move-only change cause an error?~

~Is this a side effect of circular dependencies resolving?~ nm

Travis is happy now ;)

Generally agree with this change, I've thought about doing it myself a few times myself. It also seems to be a good alternative to the observer pattern mentioned as we change far fewer things to get the same circular dependency break. So a slight code review ACK that this seems correct.

Slightly short of concept ack for a few reasons:

1. Needs a simple benchmark to show that we haven't made functions calling the CTxMemPoolEntry's non-header functions slower because they are no longer within the same compilation unit
2. Some other changes I've been working on lately w.r.t. epoch mempool introduce some new classes & move code between the CTxMemPool and CTxMemPoolEntry classes; while those can be rebased, it's not clear where some of the shared code should live (perhaps in a third additional header, or a new circular dependency). We already have some things which are inherent circular dependencies (that won't get picked up by the linter, maps of MemPoolChildren/MemPoolParents) that will deliver a big performance win to get rid of (I think it will at least), so I'd rather kill those circular dependencies first so that it doesn't become harder to do later -- I'm happy to open up a PR for that once the first step of my #17268 Project, #17925, goes through.

@JeremyRubin

... to show that we haven't made functions calling the CTxMemPoolEntry's non-header functions slower because they are no longer within the same compilation unit

How is it possible?

It's annoying, but because these functions were previously in the same translation unit then it's possible they can get completely inlined, but now they're in separate units they will definitely trigger function calls.

Because these updater functions get called in hot loops you have possibly replaced something optimized previously with something slow.

This matters a bit because we may expect these loops/regions to get even hotter if we increase mempool limits.

One way of addressing this would be to make CTxMemPoolEntry header only...

@JeremyRubin

It's annoying, but because these functions were previously in the same translation unit then it's possible they can get completely inlined, but now they're in separate units they will definitely trigger function calls.

IIUC, making function inline depends on compiler and its optimization settings. The bitcoind compiled with GCC 7.4.0 was checked with nm tool, and CTxMemPoolEntry member function symbols are present, therefore those functions are not inlined (except those which are defined in class declaration, as expected).

It seems runtime performance issues and circular dependency resolving are orthogonal. And "yes", we could

... make CTxMemPoolEntry header only...

Correct; this is compiler dependent behavior. But we happen to compile with optimizations enabled for release builds -- not sure if we do different flags for non release builds (I think it's the same?).

Presence of the symbols doesn't prove that they aren't also inlined. An inline function is a hint to inline, not a mandate, and the function can be either called or or inlined depending on context, you would need to check the actual call sites.

I agree it's not a large issue; but specifically code organization does affect emitted code (this is what LTO is intended, in a sense, to get around, but I don't think we do that other than experimentally)

Presence of the symbols doesn't prove that they aren't also inlined. An inline function is a hint to inline, not a mandate, and the function can be either called or or inlined depending on context, you would need to check the actual call sites.

I mean if a function is inlined in all call sites, there is no dedicated assembly code and no symbol. Could a compiler inline a function in some call sites and make a standard function call of the same function in other call sites?

Unless the symbol isn't exported there will be a separate copy of it included in the object for external callers. That extra copy will persist in the final binary so long as there are any non-inlined callers.

For a quick check, just look at the size of all functions in dump of the binary. If they're all the same size then it's very likely that all you did was move things around.

In C++, inline is more than a hint, and actually has a meaning: it's a function that may be defined (and exported from) multiple translation units without conflicting with itself (it's only well defined if all definitions are identical, though). This is what allows having code in .h files. In practice it means the linker will merge all the definitions into one symbol.

I think what @gmaxwell says is correct with my question. @sipa that sounds correct. These weren't previously marked inline either, so I guess what I meant to capture is that not marking inline is a hint not to inline but not a mandate to not inline. E.g., if some function F is called by G, F may or may not be inlined depending on the compiler if they are in the same translation unit, but certainly won't be if F is in a different translation unit, is non inline, and LTO is not enabled.

I think if you were to just put all the mempool entry functions into the header I wouldn't have any concerns. It doesn't seem to me like there's a really strong reason for them to be separate, and any time you change the initializers you've likely also changed the class definition so it doesn't seem like it would trigger recompilation explosions.

It also (maybe I'm wrong here) looks like you have some unneeded dependencies in the cpp (e.g., memusage), so that can be pruned out to keep the header small.

@JeremyRubin Thank you for your review.

I think if you were to just put all the mempool entry functions into the header I wouldn't have any concerns.

Done.

It also (maybe I'm wrong here) looks like you have some unneeded dependencies in the cpp (e.g., memusage), so that can be pruned out to keep the header small.

All #includes are checked again. They all are needed, e.g., core_memusage.h is needed for RecursiveDynamicUsage.

Rebased after #17261 has been merged.

Looking at the (currently) 9 conflicts, maybe it makes sense to postpone this changeset until after some of the other user-facing/features got merged to the mempool code?

Rebased after #17925 has been merged.

ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/19f6bf826d890cc78bd4a19f10dfbf59dd58c10d

A few possible refinements: https://github.com/hebasto/bitcoin/compare/20191221-mempool-circ-dep...Empact:2020-02-mempool-circ-dep

Updated 19f6bf826d890cc78bd4a19f10dfbf59dd58c10d -> 287b930a18921716bdf6961350959804042b46b0 (pr17786.07 -> pr17786.08, diff):

• implemented @Empact's suggestions

Re-ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/287b930a18921716bdf6961350959804042b46b0 - reviewed diff particularly changed lines as identified by git. Didn't audit the includes.

Rebased 287b930a18921716bdf6961350959804042b46b0 -> 219324ca4fe0d466c9007c30543c88444dec3569 (pr17786.08 -> pr17786.09) due to the conflict with #19331.

Rebased 219324ca4fe0d466c9007c30543c88444dec3569 -> 9782a8ee773a59667424353de047fd6bb9c3cb5d (pr17786.09 -> pr17786.10) due to the conflict with #19478.

Looks correct / refactor only.

CR-Ack.

@promag @empact Mind re-reviewing?

Code Review ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/9782a8ee773a59667424353de047fd6bb9c3cb5d

Why the close?

@MarcoFalke

Why the close?

Reopened and rebased 9782a8ee773a59667424353de047fd6bb9c3cb5d -> 77438e18478ca0a06708f3e988fdb31567bb76e0 (pr17786.10 -> pr17786.11) on top of the merged #23054 and the recent CI changes.

Rebased 77438e18478ca0a06708f3e988fdb31567bb76e0 -> 785968d88833c1fb4018050849f88b6014a4e8eb (pr17786.11 -> pr17786.12) on top of the recent conflicting changes.

Rebased 785968d88833c1fb4018050849f88b6014a4e8eb -> 85a7dcb799e8f0d10f8b58d23a32aeb14ea872ef (pr17786.12 -> pr17786.13) on top of the recent conflicting changes.

Rebased 85a7dcb799e8f0d10f8b58d23a32aeb14ea872ef -> b0225a6f26876873265cc76e2ad8f68e35401bdf (pr17786.13 -> pr17786.14) due to the conflict with #23795.

Concept ACK to removing this circular dependency

But perhaps a better approach would be to remove minerPolicyEstimator from txmempool, make it a client of CValidationInterface instead (will need to return more information to clients in the interface), and remove policy/fees as a dependency of txmempool. In fact, it seems like background fee estimator has been planned for a while (https://github.com/bitcoin/bitcoin/pull/10199#discussion_r115530273) and implemented but unmerged at some point (#11775).

Rebased b0225a6f26876873265cc76e2ad8f68e35401bdf -> 122db971c4e1283d8e500b03edbc0b026ad23d5a (pr17786.14 -> pr17786.15) due to the conflict with #21464.

Rebased 122db971c4e1283d8e500b03edbc0b026ad23d5a -> f7297485cf2388352bd36bd2d4dea6ba009bba21 (pr17786.15 -> pr17786.16) due to the conflict with #24218.

Rebased f7297485cf2388352bd36bd2d4dea6ba009bba21 -> 8675c9ae35aa37428732ed91c04dae94183e0705 (pr17786.16 -> pr17786.17).

Rebased 8675c9ae35aa37428732ed91c04dae94183e0705 -> 083b8166e032dc6ea20eed4d13d3353e90a0a633 (pr17786.17 -> pr17786.18) due to the conflict with #24915.

Rebased 083b8166e032dc6ea20eed4d13d3353e90a0a633 -> 4a107ebe5892272cfb4b74085b2ab4bb06500b72 (pr17786.18 -> pr17786.19) due to the conflict with #25254.

Rebased 4a107ebe5892272cfb4b74085b2ab4bb06500b72 -> 3a430a4700780173cf4ea98ec9b4dbc2fc7b1984 (pr17786.19 -> pr17786.20).

Rebased 3a430a4700780173cf4ea98ec9b4dbc2fc7b1984 -> c4c311ea83338709a7b66c49a2364a65ae430c50 (pr17786.20 -> pr17786.21) due to the conflict with #25648.

Rebased c4c311ea83338709a7b66c49a2364a65ae430c50 -> 11d885bc3dd9a44af4c7007a95b4fbb6076e4428 (pr17786.21 -> pr17786.22) due to the conflict with #26250.

Rebased 11d885bc3dd9a44af4c7007a95b4fbb6076e4428 -> f140f9468bfa505a34112d625128f3fcff18ddba (pr17786.22 -> pr17786.23) due to the conflict with #25667.

Code review ACK f140f9468bfa505a34112d625128f3fcff18ddba. This seems like an improvement over status quo to remove a circular dependency and improve code organization.

I agree with Gloria in #17786 (comment) other improvements could also be made which remove the circular dependency, but they seem complimentary and could build on this.

I think all the other discussion about code inlining and runtime performance and build performance makes sense, but I don't think those issues are a lot more pertinent to this PR than a lot of other PRs. So it just seems unlucky for this PR that they were brought up here. It would be good if we could think of a standard approach to avoiding performance regressions that could apply more fairly.

One other suggestion to make the PR more reviewable might be to split it into two commits: a move-only commit that just moved the CTxMemPoolEntry class definition without changing it, followed by a commit inlining its various methods. (This PR has been open a while and I probably would have reviewed it earlier if the bigger moveonly change didn't include other changes and was easier to verify.)

@ryanofsky

Thank you for your review!

One other suggestion to make the PR more reviewable might be to split it into two commits: a move-only commit that just moved the CTxMemPoolEntry class definition without changing it, followed by a commit inlining its various methods.

Done.

Code review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review

Code-review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770

Nice to see another circular dependency gone 💯

utACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770, agree these changes are an improvement.

the reason it was brought up historically is that circa 2019/early 2020 I thought we might be able to increase mempool limits, and these functions end up being relatively hot, especially with larger workloads, so it made sense to be careful with them. @jamesob would probably be the guy to ask about tracking perf regressions....