Prompted by the libbitcoinkernel issue [#24303](/bitcoin-bitcoin/24303/) and PRs, I started looking at existing libraries and what their dependencies are and wrote this document to describe them and where libbitcoinkernel fits in.
Readable link is: https://github.com/ryanofsky/bitcoin/blob/pr/libs/doc/design/libraries.md
Feedback is welcome
Concept ACK.
70 | * Discuss on the [BitcoinTalk](https://bitcointalk.org/) forums, in the [Development & Technical Discussion board](https://bitcointalk.org/index.php?board=6.0).
71 | * Discuss project-specific development on #bitcoin-core-dev on Libera Chat. If you don't have an IRC client, you can use [web.libera.chat](https://web.libera.chat/#bitcoin-core-dev).
72 |
73 | ### Miscellaneous
74 | - [Assets Attribution](assets-attribution.md)
75 | -- [Assumeutxo design](assumeutxo.md)
Maybe add a doc/design/README.md (although the above description doesn't add much to just a directory view)
re: #24352 (review)
Maybe add a
doc/design/README.md(although the above description doesn't add much to just a directory view)
I think I'd hold off on adding another readme for now. It seems like it would be pretty bare and even if there were things to say, they might be more helpful and visible in the main readme. Could rethink in the future as more things are added here
30 | +## Dependencies 31 | + 32 | +- Libraries should generally be careful about what other libraries they depend on, and only reference symbols following the arrows shown in the dependency graph below: 33 | + 34 | +```mermaid 35 | +graph TD;
This looks a bit scary with all these curved lines. It's probably better to use (a reasonably chosen set of) fixed positions and straight lines.
Yeah this graph is atrocious. I thought mermaid might do a better job with it. I think the markdown source code is more undertandable than the graphic, but I will look for mermaid options (straight lines!) which might make this more readable.
FWIW, I figured out how to make the lines straight. I thinks it cuts down some of the chaos and helps a little. I also added a caption to the graph to call out more important aspects.
Flow chart still looks confusing. I tried creating some flowcharts using mermaid live editor based on syntax shared here. I was able to simplify bitcoind, bitcoin-qt and bitcoin-wallet but not other things (used different color for binaries):
flowchart TB
subgraph 1
d[bitcoind]-->libbitcoin_wallet
end
subgraph 2
qt[bitcoin-qt]-->libbitcoin_node
qt[bitcoin-qt]-->libbitcoinqt
qt[bitcoin-qt]-->libbitcoin_wallet
end
subgraph 3
w[bitcoin-wallet]-->libbitcoin_wallet
w[bitcoin-wallet]-->libbitcoin_wallet_tool
end
classDef teal fill:#033E3E,stroke:#AFDCEC,stroke-width:1px;
class d,qt,w teal
flowchart TB
subgraph 1
d[bitcoind]-->libbitcoin_wallet
end
subgraph 2
qt[bitcoin-qt]-->libbitcoin_node
qt[bitcoin-qt]-->libbitcoinqt
qt[bitcoin-qt]-->libbitcoin_wallet
end
subgraph 3
w[bitcoin-wallet]-->libbitcoin_wallet
w[bitcoin-wallet]-->libbitcoin_wallet_tool
end
classDef teal fill:#033E3E,stroke:#AFDCEC,stroke-width:1px;
class d,qt,w teal
re: #24352 (review)
Thanks! I applied some of the styling here to highlight the exe outputs, though I avoided black text on teal background, because that was not readable on my PC. I also experimented with subgraphs, but trying to apply them to complete graph always seemed to blow up the graph. Probably there is room to do fancier things here in the future, but for now I'm happy if graph just shows what symbol dependencies between libraries are allowed.
though I avoided black text on teal background, because that was not readable on my PC
Just realized it looks different in github (light theme). Text is white in github (dark theme)
Probably there is room to do fancier things here in the future, but for now I'm happy if graph just shows what symbol dependencies between libraries are allowed.
Makes sense.
I had a conversation with Ryan about this and agree to the broad strokes of things. We decided that it would be good to move:
common that kernel depends on to utilutil that kernel doesn't depend on to commonI need to make that move and see where things fall to make an educated assessment of the hierarchy described here (mostly the part about libbitcoinkernel not depending on common and only depending on util).
We decided that it would be good to move:
- Everything in
commonthatkerneldepends on toutil- Everything in
utilthatkerneldoesn't depend on tocommon
Implemented this here: https://github.com/dongcarl/bitcoin/commits/2022-03-libbitcoinkernel-utilcommon-restructure
Seems to me like a nice dividing line between util and common. Would love more Concept ACKs
Concept ACK (on adding docs for this) - will look at the actual content.
0 | @@ -0,0 +1,90 @@ 1 | +# Libraries 2 | + 3 | +| Name | Description | 4 | +|------------------------|-------------| 5 | +| libbitcoin_cli | RPC client functionality used by `bitcoin-cli` executable | 6 | +| libbitcoin_common | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_util`, but higher-level (see [Dependencies](#dependencies). |
Missing a closing paren on this line
8 | +| libbitcoinconsensus | Shared library build of static `libbitcoin_consensus` library | 9 | +| libbitcoin_kernel | Consensus "engine" and support library used for validation by `libbitcoin_node` and also exposed as a [shared library](../doc/shared-libraries.md). | 10 | +| libbitcoinqt | GUI functionality used by `bitcoin-qt` and `bitcoin-gui` executables | 11 | +| libbitcoin_ipc | IPC functionality used by `bitcoin-node`, `bitcoin-wallet`, `bitcoin-gui` executables to communicate when [`--enable-multiprocess`](multiprocess.md) is used. | 12 | +| libbitcoin_node | P2P and RPC server functionality used by `bitcoind` and `bitcoin-qt` executables. | 13 | +| libbitcoin_util | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_common`, but lower-level (see [Dependencies](#dependencies). |
Another missing paren
ACK
Not sure how I missed this when it was filed, but I think this is really valuable documentation - thanks for writing it up! I also don't think the mermaid graph looks too bad, and I think it's certainly preferably to maintaining some kind of in-repo image. I say we keep it.
I think the constraints you outline later in the document (module X should never depend on module Y etc.) is very helpful information - can it be enforced somehow at the build system level? Obviously we'd know if there were ever violations because it'd be clear (I think) from Makefile changes, but wondering if there's some extra step we could take.
Updated 4131c5ff8e8905cdc23410d52c23979aa881e3b7 -> d076c18de5d9debc4b3304727712da59524a7ee9 (pr/libs.1 -> pr/libs.2, compare) with various edits and some clarifications about kernel and util and common libs
Updated d076c18de5d9debc4b3304727712da59524a7ee9 -> 93fd8d9e98feb4e0e060b3de8a8cb1671a693e79 (pr/libs.2 -> pr/libs.3, compare) with minor edits
re: #24352#pullrequestreview-983783293
I think the constraints you outline later in the document (module X should never depend on module Y etc.) is very helpful information - can it be enforced somehow at the build system level?
Probably it would not be too hard to write a linter to check this. It could run nm src/libbitcoin_util.a etc, look at all the symbols each library defines (T), and all the symbols each library uses (U), and make sure libraries only uses symbols from libraries they are allowed to depend on.
ACK 93fd8d9e98feb4e0e060b3de8a8cb1671a693e79
Looks reasonable to me!
82 | + 83 | +- The graph shows what _symbols_ (functions and variables) from each library other libraries can call and reference directly, but it is not a call graph. For example, there is no arrow connecting *libbitcoin_wallet* and *libbitcoin_node* libraries, because these libraries are intended to be modular and not depend on each other's internal implementation details. But wallet code still is still able to call node code indirectly through the `interfaces::Chain` abstract class in [`interfaces/chain.h`](../../src/interfaces/chain.h) and node code calls wallet code through the `interfaces::ChainClient` and `interfaces::Chain::Notifications` abstract classes in the same file. In general, defining abstract classes in [`src/interfaces/`](../../src/interfaces/) can be a convenient way of avoiding unwanted direct dependencies or circular dependencies between libraries. 84 | + 85 | +- *libbitcoin_consensus* should be a standalone dependency that any library can depend on, and it should not depend on any other libraries itself. 86 | + 87 | +- *libbitcoin_util* should also be a standalone dependency that any library can depend on, and it should not depend on other internal libraries. (It does use currently use boost and univalue external libraries.)
- *libbitcoin_util* should also be a standalone dependency that any library can depend on, and it should not depend on other internal libraries.
re: #24352 (review)
Thanks. I'm a little unclear what the original reason was for this suggestion, but dropped this sentence because there's no need to mention external libraries here, so probably better to be shorter.
31 | + 32 | +- Libraries should minimize what other libraries they depend on, and only reference symbols following the arrows shown in the dependency graph below: 33 | + 34 | +<table><tr><td> 35 | + 36 | +```mermaid
Huh TIL, had no idea github could do this. Cool.
39 | + 40 | +graph TD; 41 | + 42 | +bitcoin-cli-->libbitcoin_cli; 43 | + 44 | +bitcoind-->libbitcoin_node;
I think it would be useful to make the "binary" boxes a different color or style from the library ones, if possible.
re: #24352 (review)
I think it would be useful to make the "binary" boxes a different color or style from the library ones, if possible.
Thanks, used style suggestion from 1440000bytes above
ACK, this matches with my understanding of the dependencies and library uses. At some point we could add something about the exported APIs of the shared libraries, but no need to do that here.
ACK dc1e7ad7a5713d885f70ccc6c93e7a4c07e76559
Approach ACK dc1e7ad7a5713d885f70ccc6c93e7a4c07e76559, using this doc as a guide in https://github.com/hebasto/bitcoin/pull/3 :)