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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26292 (util: move threadinterrupt into util/ by fanquake)
• #26251 (refactor: add cs_main.h by fanquake)
• #26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
• #25729 (wallet: Check max transaction weight in CoinSelection by aureleoules)
• #25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)
• #24897 ([Draft / POC] Silent Payments by w0xlt)
• #24539 (Add a "tx output spender" index by sstone)
• #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
• #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
• #15294 (refactor: Extract RipeMd160 by Empact)
• #10102 (Multiprocess bitcoin by ryanofsky)

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.

I think something should be done about the inconsistency of having libbitcoin_common.a source files outside of the src/common/ and this is one possible option. Other possible options are:

1. Move src/interface/ files to a libbitcoin_interfaces.a library
2. Delete src/interface/*.cpp files and make src/interfaces/ directory only contain headers

I think I like these options a little better, because they would keep source structure flat and avoid too much nesting. They would also avoid namespace inconsistencies or the need to have a nested common::interfaces:: namespace that would make function declarations in headers more verbose.

I want look into option 2 a little more since it seems like the 3 .cpp files in the interfaces directory are very minimal and don't contain much real code.

@ryanofsky

As you mentioned, I agree that we should go for a less nested option when possible.

Now considering the alternatives you mentioned, Option 2 seems like a pretty interesting idea. I thought it to be a preferable option because it seemed a cleaner way to achieve our goal.
However, on second thought, what if we need to expand src/interfaces in the future for some reason, and going with a header-only route for the expansion might be unreasonable? So we might have to move to an option similar to your first suggestion.

This might be an infeasible scenario, and there might exist other options than the two you mentioned to achieve this goal, which I haven't considered. But I just wanted to explain why I think the second option, though cleaner, might not be feasible in the long run.

I implemented option 2 above in #26298. It changes fewer files than this PR and avoids the nested directory structure.

However, on second thought, what if we need to expand src/interfaces in the future for some reason, and going with a header-only route for the expansion might be unreasonable?

I don't see this happening, just because the point of src/interfaces/ directory is just to define abstract classes with virtual methods. Implementation of the interfaces belongs in the node, wallet, or common libraries, or in the GUI. If implementation code sneaks into the interfaces directory it's probably a mistake. Or if it is really intentional, it could logically go into a libbitcoin_interfaces.a library that without requiring all the interfaces to move somewhere.

because they would keep source structure flat and avoid too much nesting.

Is this something we are actively trying to avoid? i.e in #26250 (comment) we seem to be going the other direction.

In any case, closing this in favour of #26298.

because they would keep source structure flat and avoid too much nesting.

Is this something we are actively trying to avoid? i.e in #26250 (comment) we seem to be going the other direction.

I think both changes make sense. In that case, the file mempool_utils.cpp is renamed util/mempool.cpp, so it's really just swapping out a _ separator for the / separator, not actually adding a deeper level of nesting. I think that change is an improvement because it groups mempool utils with other utils instead of grouping them with mempool tests. It should make mempool utils more discoverable so they can be used by tests using mempool indirectly, not just tests directly testing the mempool.

In this case, this PR isn't just changing a _ separator to a / separator or :: separator, but really adding an extra level to the hierarchy, renaming "interfaces" to "common interfaces" where I think the word "common" isn't helpful descriptively, and isn't great organizationally since it groups wallet and node interface definitions in with libbitcoin_common library code that shouldn't know anything about nodes or wallets. It could also make headers more verbose with identifiers like common::interfaces::Wallet instead of interfaces::Wallet, or introducing an inconsistency between the directory structure and namespace structure.

The idea for src/interfaces/ is that it defines abstract interfaces that libraries can use to call each other without depending on each other at link time. Since #20494, the libbitcoin_node library provides Node and Chain implementations, the libbitcoin_wallet library provides Wallet and WalletLoader implementations. #26298 just extends #20494 so the libbitcoin_common provides now provides Handler implementations.

Hope that makes sense.