diff options
Diffstat (limited to 'doc/developer-notes.md')
| -rw-r--r-- | doc/developer-notes.md | 297 |
1 files changed, 233 insertions, 64 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 583c50a763..23f975df34 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -12,10 +12,12 @@ Developer Notes - [Generating Documentation](#generating-documentation) - [Development tips and tricks](#development-tips-and-tricks) - [Compiling for debugging](#compiling-for-debugging) + - [Show sources in debugging](#show-sources-in-debugging) - [Compiling for gprof profiling](#compiling-for-gprof-profiling) - [`debug.log`](#debuglog) - [Signet, testnet, and regtest modes](#signet-testnet-and-regtest-modes) - [DEBUG_LOCKORDER](#debug_lockorder) + - [DEBUG_LOCKCONTENTION](#debug_lockcontention) - [Valgrind suppressions file](#valgrind-suppressions-file) - [Compiling for test coverage](#compiling-for-test-coverage) - [Performance profiling with perf](#performance-profiling-with-perf) @@ -30,6 +32,7 @@ Developer Notes - [C++ data structures](#c-data-structures) - [Strings and formatting](#strings-and-formatting) - [Shadowing](#shadowing) + - [Lifetimebound](#lifetimebound) - [Threads and synchronization](#threads-and-synchronization) - [Scripts](#scripts) - [Shebang](#shebang) @@ -89,8 +92,15 @@ code. - Class member variables have a `m_` prefix. - Global variables have a `g_` prefix. - Constant names are all uppercase, and use `_` to separate words. + - Enumerator constants may be `snake_case`, `PascalCase` or `ALL_CAPS`. + This is a more tolerant policy than the [C++ Core + Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps), + which recommend using `snake_case`. Please use what seems appropriate. - Class names, function names, and method names are UpperCamelCase - (PascalCase). Do not prefix class names with `C`. + (PascalCase). Do not prefix class names with `C`. See [Internal interface + naming style](#internal-interface-naming-style) for an exception to this + convention. + - Test suite naming convention: The Boost test suite in file `src/test/foo_tests.cpp` should be named `foo_tests`. Test suite names must be unique. @@ -100,6 +110,28 @@ code. - `nullptr` is preferred over `NULL` or `(void*)0`. - `static_assert` is preferred over `assert` where possible. Generally; compile-time checking is preferred over run-time checking. +For function calls a namespace should be specified explicitly, unless such functions have been declared within it. +Otherwise, [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl), also known as ADL, could be +triggered that makes code harder to maintain and reason about: +```c++ +#include <filesystem> + +namespace fs { +class path : public std::filesystem::path +{ +}; +// The intention is to disallow this function. +bool exists(const fs::path& p) = delete; +} // namespace fs + +int main() +{ + //fs::path p; // error + std::filesystem::path p; // compiled + exists(p); // ADL being used for unqualified name lookup +} +``` + Block style example: ```c++ int g_count = 0; @@ -132,11 +164,78 @@ public: } // namespace foo ``` +Coding Style (C++ functions and methods) +-------------------- + +- When ordering function parameters, place input parameters first, then any + in-out parameters, followed by any output parameters. + +- *Rationale*: API consistency. + +- Prefer returning values directly to using in-out or output parameters. Use + `std::optional` where helpful for returning values. + +- *Rationale*: Less error-prone (no need for assumptions about what the output + is initialized to on failure), easier to read, and often the same or better + performance. + +- Generally, use `std::optional` to represent optional by-value inputs (and + instead of a magic default value, if there is no real default). Non-optional + input parameters should usually be values or const references, while + non-optional in-out and output parameters should usually be references, as + they cannot be null. + +Coding Style (C++ named arguments) +------------------------------ + +When passing named arguments, use a format that clang-tidy understands. The +argument names can otherwise not be verified by clang-tidy. + +For example: + +```c++ +void function(Addrman& addrman, bool clear); + +int main() +{ + function(g_addrman, /*clear=*/false); +} +``` + +### Running clang-tidy + +To run clang-tidy on Ubuntu/Debian, install the dependencies: + +```sh +apt install clang-tidy bear clang +``` + +Then, pass clang as compiler to configure, and use bear to produce the `compile_commands.json`: + +```sh +./autogen.sh && ./configure CC=clang CXX=clang++ +make clean && bear make -j $(nproc) # For bear 2.x +make clean && bear -- make -j $(nproc) # For bear 3.x +``` + +To run clang-tidy on all source files: + +```sh +( cd ./src/ && run-clang-tidy -j $(nproc) ) +``` + +To run clang-tidy on the changed source lines: + +```sh +git diff | ( cd ./src/ && clang-tidy-diff -p2 -j $(nproc) ) +``` + Coding Style (Python) --------------------- Refer to [/test/functional/README.md#style-guidelines](/test/functional/README.md#style-guidelines). + Coding Style (Doxygen-compatible comments) ------------------------------------------ @@ -249,6 +348,35 @@ Development tips and tricks Run configure with `--enable-debug` to add additional compiler flags that produce better debugging builds. +### Show sources in debugging + +If you have ccache enabled, absolute paths are stripped from debug information +with the -fdebug-prefix-map and -fmacro-prefix-map options (if supported by the +compiler). This might break source file detection in case you move binaries +after compilation, debug from the directory other than the project root or use +an IDE that only supports absolute paths for debugging. + +There are a few possible fixes: + +1. Configure source file mapping. + +For `gdb` create or append to `.gdbinit` file: +``` +set substitute-path ./src /path/to/project/root/src +``` + +For `lldb` create or append to `.lldbinit` file: +``` +settings set target.source-map ./src /path/to/project/root/src +``` + +2. Add a symlink to the `./src` directory: +``` +ln -s /path/to/project/root/src src +``` + +3. Use `debugedit` to modify debug information in the binary. + ### Compiling for gprof profiling Run configure with the `--enable-gprof` option, then make. @@ -282,6 +410,21 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts run-time checks to keep track of which locks are held and adds warnings to the `debug.log` file if inconsistencies are detected. +### DEBUG_LOCKCONTENTION + +Defining `DEBUG_LOCKCONTENTION` adds a "lock" logging category to the logging +RPC that, when enabled, logs the location and duration of each lock contention +to the `debug.log` file. + +The `--enable-debug` configure option adds `-DDEBUG_LOCKCONTENTION` to the +compiler flags. You may also enable it manually for a non-debug build by running +configure with `-DDEBUG_LOCKCONTENTION` added to your CPPFLAGS, +i.e. `CPPFLAGS="-DDEBUG_LOCKCONTENTION"`, then build and run bitcoind. + +You can then use the `-debug=lock` configuration option at bitcoind startup or +`bitcoin-cli logging '["lock"]'` at runtime to turn on lock contention logging. +It can be toggled off again with `bitcoin-cli logging [] '["lock"]'`. + ### Assertions and Checks The util file `src/util/check.h` offers helpers to protect against coding and @@ -297,7 +440,7 @@ other input. failure, it will throw an exception, which can be caught to recover from the error. - For example, a nullptr dereference or any other logic bug in RPC code - means that the RPC code is faulty and can not be executed. However, the + means that the RPC code is faulty and cannot be executed. However, the logic bug can be shown to the user and the program can continue to run. * `Assume` should be used to document assumptions when program execution can safely continue even if the assumption is violated. In debug builds it @@ -345,7 +488,7 @@ make cov Profiling is a good way to get a precise idea of where time is being spent in code. One tool for doing profiling on Linux platforms is called -[`perf`](http://www.brendangregg.com/perf.html), and has been integrated into +[`perf`](https://www.brendangregg.com/perf.html), and has been integrated into the functional test framework. Perf can observe a running process and sample (at some frequency) where its execution is. @@ -563,10 +706,6 @@ Wallet - Make sure that no crashes happen with run-time option `-disablewallet`. -- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set. - - - *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB. - General C++ ------------- @@ -669,19 +808,19 @@ Foo(vec); ```cpp enum class Tabs { - INFO, - CONSOLE, - GRAPH, - PEERS + info, + console, + network_graph, + peers }; int GetInt(Tabs tab) { switch (tab) { - case Tabs::INFO: return 0; - case Tabs::CONSOLE: return 1; - case Tabs::GRAPH: return 2; - case Tabs::PEERS: return 3; + case Tabs::info: return 0; + case Tabs::console: return 1; + case Tabs::network_graph: return 2; + case Tabs::peers: return 3; } // no default case, so the compiler can warn about missing cases assert(false); } @@ -692,11 +831,6 @@ int GetInt(Tabs tab) Strings and formatting ------------------------ -- Be careful of `LogPrint` versus `LogPrintf`. `LogPrint` takes a `category` argument, `LogPrintf` does not. - - - *Rationale*: Confusion of these can result in runtime exceptions due to - formatting mismatch, and it is easy to get wrong because of subtly similar naming. - - Use `std::string`, avoid C string manipulation functions. - *Rationale*: C++ string handling is marginally safer, less scope for @@ -769,22 +903,37 @@ from using a different variable with the same name), please name variables so that their names do not shadow variables defined in the source code. When using nested cycles, do not name the inner cycle variable the same as in -the upper cycle, etc. +the outer cycle, etc. + +Lifetimebound +-------------- + +The [Clang `lifetimebound` +attribute](https://clang.llvm.org/docs/AttributeReference.html#lifetimebound) +can be used to tell the compiler that a lifetime is bound to an object and +potentially see a compile-time warning if the object has a shorter lifetime from +the invalid use of a temporary. You can use the attribute by adding a `LIFETIMEBOUND` +annotation defined in `src/attributes.h`; please grep the codebase for examples. Threads and synchronization ---------------------------- -- Prefer `Mutex` type to `RecursiveMutex` one +- Prefer `Mutex` type to `RecursiveMutex` one. - Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to - get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with - run-time asserts in function definitions: + get compile-time warnings about potential race conditions or deadlocks in code. - In functions that are declared separately from where they are defined, the thread safety annotations should be added exclusively to the function declaration. Annotations on the definition could lead to false positives (lack of compile failure) at call sites between the two. + - Prefer locks that are in a class rather than global, and that are + internal to a class (private or protected) rather than public. + + - Combine annotations in function declarations with run-time asserts in + function definitions: + ```C++ // txmempool.h class CTxMemPool @@ -808,21 +957,37 @@ void CTxMemPool::UpdateTransactionsFromBlock(...) ```C++ // validation.h -class ChainstateManager +class CChainState { +protected: + ... + Mutex m_chainstate_mutex; + ... public: ... - bool ProcessNewBlock(...) LOCKS_EXCLUDED(::cs_main); + bool ActivateBestChain( + BlockValidationState& state, + std::shared_ptr<const CBlock> pblock = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + ... + bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); ... } // validation.cpp -bool ChainstateManager::ProcessNewBlock(...) +bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) { + AssertLockNotHeld(m_chainstate_mutex); AssertLockNotHeld(::cs_main); - ... - LOCK(::cs_main); - ... + { + LOCK(cs_main); + ... + } + + return ActivateBestChain(state, std::shared_ptr<const CBlock>()); } ``` @@ -853,6 +1018,8 @@ TRY_LOCK(cs_vNodes, lockNodes); Scripts -------------------------- +Write scripts in Python rather than bash, when possible. + ### Shebang - Use `#!/usr/bin/env bash` instead of obsolete `#!/bin/bash`. @@ -959,37 +1126,40 @@ Subtrees Several parts of the repository are subtrees of software maintained elsewhere. -Some of these are maintained by active developers of Bitcoin Core, in which case changes should probably go -directly upstream without being PRed directly against the project. They will be merged back in the next -subtree merge. +Some of these are maintained by active developers of Bitcoin Core, in which case +changes should go directly upstream without being PRed directly against the project. +They will be merged back in the next subtree merge. -Others are external projects without a tight relationship with our project. Changes to these should also -be sent upstream, but bugfixes may also be prudent to PR against Bitcoin Core so that they can be integrated -quickly. Cosmetic changes should be purely taken upstream. +Others are external projects without a tight relationship with our project. Changes +to these should also be sent upstream, but bugfixes may also be prudent to PR against +a Bitcoin Core subtree, so that they can be integrated quickly. Cosmetic changes +should be taken upstream. -There is a tool in `test/lint/git-subtree-check.sh` ([instructions](../test/lint#git-subtree-checksh)) to check a subtree directory for consistency with -its upstream repository. +There is a tool in `test/lint/git-subtree-check.sh` ([instructions](../test/lint#git-subtree-checksh)) +to check a subtree directory for consistency with its upstream repository. Current subtrees include: - src/leveldb - - Upstream at https://github.com/google/leveldb ; Maintained by Google, but - open important PRs to Core to avoid delay. + - Subtree at https://github.com/bitcoin-core/leveldb-subtree ; maintained by Core contributors. + - Upstream at https://github.com/google/leveldb ; maintained by Google. Open + important PRs to the subtree to avoid delay. - **Note**: Follow the instructions in [Upgrading LevelDB](#upgrading-leveldb) when merging upstream changes to the LevelDB subtree. - src/crc32c - Used by leveldb for hardware acceleration of CRC32C checksums for data integrity. - - Upstream at https://github.com/google/crc32c ; Maintained by Google. + - Subtree at https://github.com/bitcoin-core/crc32c-subtree ; maintained by Core contributors. + - Upstream at https://github.com/google/crc32c ; maintained by Google. - src/secp256k1 - - Upstream at https://github.com/bitcoin-core/secp256k1/ ; actively maintained by Core contributors. + - Upstream at https://github.com/bitcoin-core/secp256k1/ ; maintained by Core contributors. - src/crypto/ctaes - - Upstream at https://github.com/bitcoin-core/ctaes ; actively maintained by Core contributors. + - Upstream at https://github.com/bitcoin-core/ctaes ; maintained by Core contributors. -- src/univalue - - Upstream at https://github.com/bitcoin-core/univalue ; actively maintained by Core contributors, deviates from upstream https://github.com/jgarzik/univalue +- src/minisketch + - Upstream at https://github.com/sipa/minisketch ; maintained by Core contributors. Upgrading LevelDB --------------------- @@ -1119,7 +1289,10 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Rationale*: Consistency with the existing interface. -- Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`) +- Argument and field naming: please consider whether there is already a naming + style or spelling convention in the API for the type of object in question + (`blockhash`, for example), and if so, try to use that. If not, use snake case + `fee_delta` (and not, e.g. `feedelta` or camel case `feeDelta`). - *Rationale*: Consistency with the existing interface. @@ -1158,7 +1331,7 @@ A few guidelines for introducing and reviewing new RPC interfaces: - Don't forget to fill in the argument names correctly in the RPC command table. - - *Rationale*: If not, the call can not be used with name-based arguments. + - *Rationale*: If not, the call cannot be used with name-based arguments. - Add every non-string RPC argument `(method, idx, name)` to the table `vRPCConvertParams` in `rpc/client.cpp`. @@ -1213,6 +1386,12 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Rationale*: User-facing consistency. +- Use `fs::path::u8string()` and `fs::u8path()` functions when converting path + to JSON strings, not `fs::PathToString` and `fs::PathFromString` + + - *Rationale*: JSON strings are Unicode strings, not byte strings, and + RFC8259 requires JSON to be encoded as UTF-8. + Internal interface guidelines ----------------------------- @@ -1279,22 +1458,9 @@ communication: virtual boost::signals2::scoped_connection connectTipChanged(TipChangedFn fn) = 0; ``` -- For consistency and friendliness to code generation tools, interface method - input and inout parameters should be ordered first and output parameters - should come last. +- Interface methods should not be overloaded. - Example: - - ```c++ - // Good: error output param is last - virtual bool broadcastTransaction(const CTransactionRef& tx, CAmount max_fee, std::string& error) = 0; - - // Bad: error output param is between input params - virtual bool broadcastTransaction(const CTransactionRef& tx, std::string& error, CAmount max_fee) = 0; - ``` - -- For friendliness to code generation tools, interface methods should not be - overloaded: + *Rationale*: consistency and friendliness to code generation tools. Example: @@ -1308,10 +1474,13 @@ communication: virtual bool disconnect(NodeId id) = 0; ``` -- For consistency and friendliness to code generation tools, interface method - names should be `lowerCamelCase` and standalone function names should be +### Internal interface naming style + +- Interface method names should be `lowerCamelCase` and standalone function names should be `UpperCamelCase`. + *Rationale*: consistency and friendliness to code generation tools. + Examples: ```c++ |