Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Organization of Source Code #898

Open
scravy opened this issue Apr 4, 2019 · 2 comments
Open

Organization of Source Code #898

scravy opened this issue Apr 4, 2019 · 2 comments
Labels
build Build system technical debt Cleaning up code which is there for historical reasons
Milestone

Comments

@scravy
Copy link
Member

scravy commented Apr 4, 2019

bitcoin-core (and therefore unit-e) is divided into libraries:

  • libunivalue
  • libunite_server
  • libunite_common
  • libunite_consensus
  • libunite_util
  • libunite_crypto
  • libunite_zmq
  • libunite_usbdevice
  • libunite_wallet
  • and some more (libleveldb, libsecp256k1, ...)

The following executables exist:

  • unit-e
  • unit-e-cli
  • unit-e-tx
  • test_unite
  • bench_unite

The dependency graph between them should (it unfortunately does not) look roughly like this:

deps

Source for this graph:

digraph G {

  unit_e [shape=box, label="unit-e"];
  unit_e_cli [shape=box, label="unit-e-cli"];
  unit_e_tx [shape=box, label="unit-e-tx"];

  unit_e -> libunite_server;
  unit_e -> libunite_wallet;
  unit_e -> libunite_zmq;

  unit_e_cli -> libunivalue;
  unit_e_cli -> libunite_crypto;
  unit_e_cli -> libunite_util;

  unit_e_tx -> libunite_common;
  unit_e_tx -> libunite_consensus;

  libunite_zmq -> libunite_util;
  libunite_zmq -> libunite_server;
  libunite_server -> libunite_consensus;
  libunite_server -> libunite_memenv;
  libunite_server -> libleveldb;
  libunite_wallet -> libunite_usbdevice;
  libunite_consensus -> libunite_crypto;
  libunite_consensus -> libsecp256k1;
  libunite_consensus -> libunite_util;
  libunite_common -> libunite_util;
  libunite_util -> libunivalue;
  libunite_server -> libunite_common;
  libunite_wallet -> libunite_server;
  libunite_usbdevice -> libunite_util;

}

Unfortunately this is not the case. We frequently run into linking issues and even on the source level we have circuler dependencies (use circular-dependencies.py).

This begs the question of how we should go about resolving this situation. Bitcoin is separating modules more and more, while we have been adding circular dependencies rather carelessly. In fact one of the questions here is: Do we need the separation into these modules?

Circular dependencies are inherently bad. Some examples:

  • If A includes B and B includes A some compilation problems might only arise if your platform decides to load A first of B first. For instance: You might have missed a forward declaration in A to a symbol in B. By chance your platform (filesystem, compiler) processes B first. No problem. On somebody else's platform the compiler processes A first -> compilation error (this has happened to use with standard headers even like #include <functional> which broke the build on everybody's platform, except in travis or on mac)
  • Linking problems: While the linker in macOS throws all symbols into a big stew, stirrs, and everything is fine the linux linker does not. It builds a hierarchical order and even removes unused symbols. Depending on the order of object files to link (again might be platform dependent) you may or may not run into a problem. Just that you can not control it in source code as it depends on the resulting object files.

Some programming languages (Go, Haskell, ...) do not even support circular dependencies between modules, precisely because of the afore mentioned problems. Ciruclar dependencies are hard.

@scravy scravy added build Build system technical debt Cleaning up code which is there for historical reasons labels Apr 4, 2019
@dtr-org dtr-org deleted a comment from mergeable bot Apr 4, 2019
@scravy
Copy link
Member Author

scravy commented Apr 4, 2019

Circular dependencies in bitcoin master 79c345a0114c9a83fd40e01150519373c017b130 (excluding circular dependencies within gui code / everything that starts with qt/):

Circular dependency: chainparamsbase -> util/system -> chainparamsbase
Circular dependency: checkpoints -> validation -> checkpoints
Circular dependency: index/txindex -> validation -> index/txindex
Circular dependency: policy/fees -> txmempool -> policy/fees
Circular dependency: policy/policy -> validation -> policy/policy
Circular dependency: txmempool -> validation -> txmempool
Circular dependency: validation -> validationinterface -> validation
Circular dependency: wallet/coincontrol -> wallet/wallet -> wallet/coincontrol
Circular dependency: wallet/fees -> wallet/wallet -> wallet/fees
Circular dependency: wallet/wallet -> wallet/walletdb -> wallet/wallet
Circular dependency: policy/fees -> policy/policy -> validation -> policy/fees
Circular dependency: policy/rbf -> txmempool -> validation -> policy/rbf
Circular dependency: txmempool -> validation -> validationinterface -> txmempool

Circular dependencies in unit-e master ec33ab3:

Circular dependency: blockchain/blockchain_genesis -> blockchain/blockchain_parameters -> blockchain/blockchain_genesis
Circular dependency: chain -> pow -> chain
Circular dependency: chainparamsbase -> util -> chainparamsbase
Circular dependency: coins -> snapshot/iterator -> coins
Circular dependency: coins -> snapshot/messages -> coins
Circular dependency: consensus/merkle -> primitives/block -> consensus/merkle
Circular dependency: esperanza/checks -> esperanza/finalizationstate -> esperanza/checks
Circular dependency: esperanza/checks -> txmempool -> esperanza/checks
Circular dependency: esperanza/checks -> validation -> esperanza/checks
Circular dependency: esperanza/finalizationstate -> validation -> esperanza/finalizationstate
Circular dependency: esperanza/walletextension -> wallet/wallet -> esperanza/walletextension
Circular dependency: finalization/state_processor -> finalization/state_repository -> finalization/state_processor
Circular dependency: httpserver -> init -> httpserver
Circular dependency: init -> net_processing -> init
Circular dependency: init -> rpc/server -> init
Circular dependency: init -> txdb -> init
Circular dependency: init -> validation -> init
Circular dependency: init -> validationinterface -> init
Circular dependency: injector -> validation -> injector
Circular dependency: keystore -> script/ismine -> keystore
Circular dependency: p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> p2p/finalizer_commits_handler
Circular dependency: policy/fees -> txmempool -> policy/fees
Circular dependency: policy/policy -> validation -> policy/policy
Circular dependency: random -> util -> random
Circular dependency: snapshot/snapshot_index -> txdb -> snapshot/snapshot_index
Circular dependency: staking/validation_error -> staking/validation_result -> staking/validation_error
Circular dependency: sync -> util -> sync
Circular dependency: txmempool -> validation -> txmempool
Circular dependency: usbdevice/debugdevice -> usbdevice/usbdevice -> usbdevice/debugdevice
Circular dependency: usbdevice/ledgerdevice -> usbdevice/usbdevice -> usbdevice/ledgerdevice
Circular dependency: usbdevice/usbdevice -> wallet/wallet -> usbdevice/usbdevice
Circular dependency: validation -> validationinterface -> validation
Circular dependency: wallet/coincontrol -> wallet/wallet -> wallet/coincontrol
Circular dependency: wallet/fees -> wallet/wallet -> wallet/fees
Circular dependency: wallet/init -> wallet/wallet -> wallet/init
Circular dependency: wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet
Circular dependency: wallet/wallet -> wallet/walletdb -> wallet/wallet
Circular dependency: blockchain/blockchain_behavior -> blockchain/blockchain_custom_parameters -> blockchain/blockchain_genesis -> blockchain/blockchain_behavior
Circular dependency: blockchain/blockchain_rpc -> rpc/util -> injector -> blockchain/blockchain_rpc
Circular dependency: consensus/tx_verify -> esperanza/finalizationstate -> validation -> consensus/tx_verify
Circular dependency: esperanza/vote -> keystore -> script/script -> esperanza/vote
Circular dependency: httprpc -> httpserver -> init -> httprpc
Circular dependency: init -> injector -> settings -> init
Circular dependency: init -> wallet/init -> wallet/rpcwallet -> init
Circular dependency: injector -> staking/transactionpicker -> miner -> injector
Circular dependency: policy/fees -> policy/policy -> validation -> policy/fees
Circular dependency: policy/rbf -> txmempool -> validation -> policy/rbf
Circular dependency: txmempool -> validation -> validationinterface -> txmempool
Circular dependency: base58 -> blockchain/blockchain_behavior -> blockchain/blockchain_parameters -> settings -> base58
Circular dependency: finalization/state_processor -> snapshot/creator -> snapshot/p2p_processing -> injector -> finalization/state_processor
Circular dependency: finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> finalization/vote_recorder
Circular dependency: httpserver -> init -> wallet/init -> wallet/rpcwallet -> httpserver
Circular dependency: injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> net_processing -> injector
Circular dependency: injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> snapshot/p2p_processing -> injector
Circular dependency: injector -> staking/transactionpicker -> miner -> txmempool -> injector
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> consensus/tx_verify
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> consensus/tx_verify
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> esperanza/checks
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> esperanza/checks
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> esperanza/checks
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> txmempool -> consensus/tx_verify
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> wallet/walletdb -> consensus/tx_verify

This has already bitten us while working on merging 0.17.

@thothd thothd added this to the 1.0 milestone Apr 5, 2019
@kostyantyn kostyantyn reopened this Apr 5, 2019
@scravy
Copy link
Member Author

scravy commented Apr 10, 2019

Here is the corresponding ticket in bitcoin: bitcoin/bitcoin#15732

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system technical debt Cleaning up code which is there for historical reasons
Projects
None yet
Development

No branches or pull requests

3 participants