diff options
Diffstat (limited to 'doc/developer-notes.md')
| -rw-r--r-- | doc/developer-notes.md | 178 |
1 files changed, 80 insertions, 98 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 45e55b7c40..561f623cd5 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -27,14 +27,13 @@ Developer Notes - [General C++](#general-c) - [C++ data structures](#c-data-structures) - [Strings and formatting](#strings-and-formatting) - - [Variable names](#variable-names) + - [Shadowing](#shadowing) - [Threads and synchronization](#threads-and-synchronization) - [Scripts](#scripts) - [Shebang](#shebang) - [Source code organization](#source-code-organization) - [GUI](#gui) - [Subtrees](#subtrees) - - [Git and GitHub tips](#git-and-github-tips) - [Scripted diffs](#scripted-diffs) - [Release notes](#release-notes) - [RPC interface guidelines](#rpc-interface-guidelines) @@ -63,7 +62,7 @@ tool to clean up patches automatically before submission. - Braces on the same line for everything else. - 4 space indentation (no tabs) for every block except namespaces. - No indentation for `public`/`protected`/`private` or for `namespace`. - - No extra spaces inside parenthesis; don't do ( this ) + - No extra spaces inside parenthesis; don't do ( this ). - No space after function names; one space after `if`, `for` and `while`. - If an `if` only has a single-statement `then`-clause, it can appear on the same line as the `if`, without braces. In every other case, @@ -73,12 +72,12 @@ tool to clean up patches automatically before submission. - **Symbol naming conventions**. These are preferred in new code, but are not required when doing so would need changes to significant pieces of existing code. - - Variable (including function arguments) and namespace names are all lowercase, and may use `_` to + - Variable (including function arguments) and namespace names are all lowercase and may use `_` to separate words (snake_case). - Class member variables have a `m_` prefix. - Global variables have a `g_` prefix. - Constant names are all uppercase, and use `_` to separate words. - - Class names, function names and method names are UpperCamelCase + - Class names, function names, and method names are UpperCamelCase (PascalCase). Do not prefix class names with `C`. - Test suite naming convention: The Boost test suite in file `src/test/foo_tests.cpp` should be named `foo_tests`. Test suite names @@ -135,6 +134,7 @@ Bitcoin Core uses [Doxygen](http://www.doxygen.nl/) to generate its official doc Use Doxygen-compatible comment blocks for functions, methods, and fields. For example, to describe a function use: + ```c++ /** * ... text ... @@ -144,11 +144,12 @@ For example, to describe a function use: */ bool function(int arg1, const char *arg2) ``` + A complete list of `@xxx` commands can be found at http://www.doxygen.nl/manual/commands.html. As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this case), you don't *need* to provide any commands for a comment to be valid; just a description text is fine. -To describe a class use the same construct above the class definition: +To describe a class, use the same construct above the class definition: ```c++ /** * Alerts are for notifying old versions if they become too obsolete and @@ -190,7 +191,7 @@ but the above styles are favored. Documentation can be generated with `make docs` and cleaned up with `make clean-docs`. The resulting files are located in `doc/doxygen/html`; open `index.html` to view the homepage. -Before running `make docs`, you will need to install dependencies `doxygen` and `dot`. For example, on MacOS via Homebrew: +Before running `make docs`, you will need to install dependencies `doxygen` and `dot`. For example, on macOS via Homebrew: ``` brew install doxygen --with-graphviz ``` @@ -232,7 +233,7 @@ that run in `-regtest` mode. Bitcoin Core is a multi-threaded application, and deadlocks or other multi-threading bugs can be very difficult to track down. The `--enable-debug` 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 +run-time checks to keep track of which locks are held and adds warnings to the debug.log file if inconsistencies are detected. ### Valgrind suppressions file @@ -280,7 +281,7 @@ thread](https://askubuntu.com/questions/50145/how-to-install-perf-monitoring-too for specific instructions. Certain kernel parameters may need to be set for perf to be able to inspect the -running process' stack. +running process's stack. ```sh $ sudo sysctl -w kernel.perf_event_paranoid=-1 @@ -300,7 +301,7 @@ $ perf record \ -p `pgrep bitcoind` -- sleep 60 ``` -You could then analyze the results by running +You could then analyze the results by running: ```sh perf report --stdio | c++filt | less @@ -365,7 +366,7 @@ Additional resources: Locking/mutex usage notes ------------------------- -The code is multi-threaded, and uses mutexes and the +The code is multi-threaded and uses mutexes and the `LOCK` and `TRY_LOCK` macros to protect data structures. Deadlocks due to inconsistent lock ordering (thread 1 locks `cs_main` and then @@ -376,7 +377,7 @@ reported in the debug.log file. Re-architecting the core code so there are better-defined interfaces between the various components is a goal, with any necessary locking -done by the components (e.g. see the self-contained `CBasicKeyStore` class +done by the components (e.g. see the self-contained `FillableSigningProvider` class and its `cs_KeyStore` lock for example). Threads @@ -390,7 +391,7 @@ Threads - ThreadDNSAddressSeed : Loads addresses of peers from the DNS. -- ThreadMapPort : Universal plug-and-play startup/shutdown +- ThreadMapPort : Universal plug-and-play startup/shutdown. - ThreadSocketHandler : Sends/Receives data from peers on port 8333. @@ -409,7 +410,7 @@ Threads Ignoring IDE/editor files -------------------------- -In closed-source environments in which everyone uses the same IDE it is common +In closed-source environments in which everyone uses the same IDE, it is common to add temporary files it produces to the project-wide `.gitignore` file. However, in open source software such as Bitcoin Core, where everyone uses @@ -447,19 +448,19 @@ pay attention to for reviewers of Bitcoin Core code. General Bitcoin Core ---------------------- -- New features should be exposed on RPC first, then can be made available in the GUI +- New features should be exposed on RPC first, then can be made available in the GUI. - *Rationale*: RPC allows for better automatic testing. The test suite for - the GUI is very limited + the GUI is very limited. -- Make sure pull requests pass Travis CI before merging +- Make sure pull requests pass Travis CI before merging. - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing - on the master branch. Otherwise all new pull requests will start failing the tests, resulting in - confusion and mayhem + on the master branch. Otherwise, all new pull requests will start failing the tests, resulting in + confusion and mayhem. - *Explanation*: If the test suite is to be updated for a change, this has to - be done first + be done first. Wallet ------- @@ -467,13 +468,13 @@ Wallet - Make sure that no crashes happen with run-time option `-disablewallet`. - *Rationale*: In RPC code that conditionally uses the wallet (such as - `validateaddress`) it is easy to forget that global pointer `pwalletMain` + `validateaddress`), it is easy to forget that global pointer `pwalletMain` can be nullptr. See `test/functional/disablewallet.py` for functional tests - exercising the API with `-disablewallet` + exercising the API with `-disablewallet`. -- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set +- 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 + - *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB. General C++ ------------- @@ -484,26 +485,26 @@ Guidelines](https://isocpp.github.io/CppCoreGuidelines/). Common misconceptions are clarified in those sections: - Passing (non-)fundamental types in the [C++ Core - Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional) + Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional). -- Assertions should not have side-effects +- Assertions should not have side-effects. - *Rationale*: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and - makes the code harder to understand + makes the code harder to understand. -- If you use the `.h`, you must link the `.cpp` +- If you use the `.h`, you must link the `.cpp`. - *Rationale*: Include files define the interface for the code in implementation files. Including one but not linking the other is confusing. Please avoid that. Moving functions from - the `.h` to the `.cpp` should not result in build errors + the `.h` to the `.cpp` should not result in build errors. -- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example by using +- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example, by using `unique_ptr` for allocations in a function. - - *Rationale*: This avoids memory and resource leaks, and ensures exception safety + - *Rationale*: This avoids memory and resource leaks, and ensures exception safety. -- Use `MakeUnique()` to construct objects owned by `unique_ptr`s +- Use `MakeUnique()` to construct objects owned by `unique_ptr`s. - *Rationale*: `MakeUnique` is concise and ensures exception safety in complex expressions. `MakeUnique` is a temporary project local implementation of `std::make_unique` (C++14). @@ -511,27 +512,27 @@ Common misconceptions are clarified in those sections: C++ data structures -------------------- -- Never use the `std::map []` syntax when reading from a map, but instead use `.find()` +- Never use the `std::map []` syntax when reading from a map, but instead use `.find()`. - *Rationale*: `[]` does an insert (of the default element) if the item doesn't exist in the map yet. This has resulted in memory leaks in the past, as well as - race conditions (expecting read-read behavior). Using `[]` is fine for *writing* to a map + race conditions (expecting read-read behavior). Using `[]` is fine for *writing* to a map. - Do not compare an iterator from one data structure with an iterator of - another data structure (even if of the same type) + another data structure (even if of the same type). - *Rationale*: Behavior is undefined. In C++ parlor this means "may reformat - the universe", in practice this has resulted in at least one hard-to-debug crash bug + the universe", in practice this has resulted in at least one hard-to-debug crash bug. - Watch out for out-of-bounds vector access. `&vch[vch.size()]` is illegal, including `&vch[0]` for an empty vector. Use `vch.data()` and `vch.data() + vch.size()` instead. -- Vector bounds checking is only enabled in debug mode. Do not rely on it +- Vector bounds checking is only enabled in debug mode. Do not rely on it. - Initialize all non-static class members where they are defined. If this is skipped for a good reason (i.e., optimization on the critical - path), add an explicit comment about this + path), add an explicit comment about this. - *Rationale*: Ensure determinism by avoiding accidental use of uninitialized values. Also, static analyzers balk about this. @@ -555,12 +556,12 @@ class A `int8_t`. Do not use bare `char` unless it is to pass to a third-party API. This type can be signed or unsigned depending on the architecture, which can lead to interoperability problems or dangerous conditions such as - out-of-bounds array accesses + out-of-bounds array accesses. -- Prefer explicit constructions over implicit ones that rely on 'magical' C++ behavior +- Prefer explicit constructions over implicit ones that rely on 'magical' C++ behavior. - *Rationale*: Easier to understand what is happening, thus easier to spot mistakes, even for those - that are not language lawyers + that are not language lawyers. Strings and formatting ------------------------ @@ -568,17 +569,17 @@ 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 + formatting mismatch, and it is easy to get wrong because of subtly similar naming. -- Use `std::string`, avoid C string manipulation functions +- Use `std::string`, avoid C string manipulation functions. - *Rationale*: C++ string handling is marginally safer, less scope for - buffer overflows and surprises with `\0` characters. Also some C string manipulations - tend to act differently depending on platform, or even the user locale + buffer overflows, and surprises with `\0` characters. Also, some C string manipulations + tend to act differently depending on platform, or even the user locale. -- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing +- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing. - - *Rationale*: These functions do overflow checking, and avoid pesky locale issues. + - *Rationale*: These functions do overflow checking and avoid pesky locale issues. - Avoid using locale dependent functions if possible. You can use the provided [`lint-locale-dependence.sh`](/test/lint/lint-locale-dependence.sh) @@ -608,48 +609,32 @@ Strings and formatting `wcstoll`, `wcstombs`, `wcstoul`, `wcstoull`, `wcstoumax`, `wcswidth`, `wcsxfrm`, `wctob`, `wctomb`, `wctrans`, `wctype`, `wcwidth`, `wprintf` -- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers +- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers. - - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion + - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion. -Variable names +Shadowing -------------- -Although the shadowing warning (`-Wshadow`) is not enabled by default (it prevents issues rising +Although the shadowing warning (`-Wshadow`) is not enabled by default (it prevents issues arising 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. -E.g. in member initializers, prepend `_` to the argument name shadowing the -member name: - -```c++ -class AddressBookPage -{ - Mode m_mode; -} - -AddressBookPage::AddressBookPage(Mode _mode) - : m_mode(_mode) -... -``` - When using nested cycles, do not name the inner cycle variable the same as in -upper cycle etc. - +the upper cycle, etc. Threads and synchronization ---------------------------- - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. As of 0.12, this is defined by default when - configuring with `--enable-debug` + configuring with `--enable-debug`. - When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of the current scope, so surround the statement and the code that needs the lock - with braces + with braces. OK: - ```c++ { TRY_LOCK(cs_vNodes, lockNodes); @@ -658,7 +643,6 @@ Threads and synchronization ``` Wrong: - ```c++ TRY_LOCK(cs_vNodes, lockNodes); { @@ -680,13 +664,11 @@ Scripts `#!/usr/bin/env bash` searches the user's PATH to find the bash binary. OK: - ```bash #!/usr/bin/env bash ``` Wrong: - ```bash #!/bin/bash ``` @@ -695,9 +677,9 @@ Source code organization -------------------------- - Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or - when performance due to inlining is critical + when performance due to inlining is critical. - - *Rationale*: Shorter and simpler header files are easier to read, and reduce compile time + - *Rationale*: Shorter and simpler header files are easier to read and reduce compile time. - Use only the lowercase alphanumerics (`a-z0-9`), underscore (`_`) and hyphen (`-`) in source code filenames. @@ -715,7 +697,7 @@ Source code organization - Don't import anything into the global namespace (`using namespace ...`). Use fully specified types such as `std::string`. - - *Rationale*: Avoids symbol conflicts + - *Rationale*: Avoids symbol conflicts. - Terminate namespaces with a comment (`// namespace mynamespace`). The comment should be placed on the same line as the brace closing the namespace, e.g. @@ -730,7 +712,7 @@ namespace { } // namespace ``` - - *Rationale*: Avoids confusion about the namespace context + - *Rationale*: Avoids confusion about the namespace context. - Use `#include <primitives/transaction.h>` bracket syntax instead of `#include "primitives/transactions.h"` quote syntax. @@ -753,13 +735,13 @@ namespace { GUI ----- -- Do not display or manipulate dialogs in model code (classes `*Model`) +- Do not display or manipulate dialogs in model code (classes `*Model`). - *Rationale*: Model classes pass through events and data from the core, they should not interact with the user. That's where View classes come in. The converse also holds: try to not directly access core data structures from Views. -- Avoid adding slow or blocking code in the GUI thread. In particular do not +- Avoid adding slow or blocking code in the GUI thread. In particular, do not add new `interfaces::Node` and `interfaces::Wallet` method calls, even if they may be fast now, in case they are changed to lock or communicate across processes in the future. @@ -778,12 +760,12 @@ 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 +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 Bitcoin Core so that they can be integrated +quickly. Cosmetic changes should be purely taken upstream. There is a tool in `test/lint/git-subtree-check.sh` to check a subtree directory for consistency with its upstream repository. @@ -813,11 +795,11 @@ you must be aware of. ### File Descriptor Counts -In most configurations we use the default LevelDB value for `max_open_files`, +In most configurations, we use the default LevelDB value for `max_open_files`, which is 1000 at the time of this writing. If LevelDB actually uses this many -file descriptors it will cause problems with Bitcoin's `select()` loop, because +file descriptors, it will cause problems with Bitcoin's `select()` loop, because it may cause new sockets to be created where the fd value is >= 1024. For this -reason, on 64-bit Unix systems we rely on an internal LevelDB optimization that +reason, on 64-bit Unix systems, we rely on an internal LevelDB optimization that uses `mmap()` + `close()` to open table files without actually retaining references to the table file descriptors. If you are upgrading LevelDB, you must sanity check the changes to make sure that this assumption remains valid. @@ -842,14 +824,14 @@ details. It is possible for LevelDB changes to inadvertently change consensus compatibility between nodes. This happened in Bitcoin 0.8 (when LevelDB was -first introduced). When upgrading LevelDB you should review the upstream changes +first introduced). When upgrading LevelDB, you should review the upstream changes to check for issues affecting consensus compatibility. For example, if LevelDB had a bug that accidentally prevented a key from being returned in an edge case, and that bug was fixed upstream, the bug "fix" would -be an incompatible consensus change. In this situation the correct behavior +be an incompatible consensus change. In this situation, the correct behavior would be to revert the upstream fix before applying the updates to Bitcoin's -copy of LevelDB. In general you should be wary of any upstream changes affecting +copy of LevelDB. In general, you should be wary of any upstream changes affecting what data is returned from LevelDB queries. Scripted diffs @@ -868,7 +850,7 @@ To create a scripted-diff: - `-BEGIN VERIFY SCRIPT-` - `-END VERIFY SCRIPT-` -The scripted-diff is verified by the tool `test/lint/commit-script-check.sh`. The tool's default behavior when supplied +The scripted-diff is verified by the tool `test/lint/commit-script-check.sh`. The tool's default behavior, when supplied with a commit is to verify all scripted-diffs from the beginning of time up to said commit. Internally, the tool passes the first supplied argument to `git rev-list --reverse` to determine which commits to verify script-diffs for, ignoring commits that don't conform to the commit message format described above. @@ -901,23 +883,23 @@ RPC interface guidelines A few guidelines for introducing and reviewing new RPC interfaces: -- Method naming: use consecutive lower-case names such as `getrawtransaction` and `submitblock` +- Method naming: use consecutive lower-case names such as `getrawtransaction` and `submitblock`. - - *Rationale*: Consistency with existing interface. + - *Rationale*: Consistency with the existing interface. - Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`) - - *Rationale*: Consistency with existing interface. + - *Rationale*: Consistency with the existing interface. - Use the JSON parser for parsing, don't manually parse integers or strings from arguments unless absolutely necessary. - *Rationale*: Introduces hand-rolled string manipulation code at both the caller and callee sites, - which is error prone, and it is easy to get things such as escaping wrong. + which is error-prone, and it is easy to get things such as escaping wrong. JSON already supports nested data structures, no need to re-invent the wheel. - *Exception*: AmountFromValue can parse amounts as string. This was introduced because many JSON - parsers and formatters hard-code handling decimal numbers as floating point + parsers and formatters hard-code handling decimal numbers as floating-point values, resulting in potential loss of precision. This is unacceptable for monetary values. **Always** use `AmountFromValue` and `ValueFromAmount` when inputting or outputting monetary values. The only exceptions to this are @@ -926,7 +908,7 @@ A few guidelines for introducing and reviewing new RPC interfaces: - Missing arguments and 'null' should be treated the same: as default values. If there is no default value, both cases should fail in the same way. The easiest way to follow this - guideline is detect unspecified arguments with `params[x].isNull()` instead of + guideline is to detect unspecified arguments with `params[x].isNull()` instead of `params.size() <= x`. The former returns true if the argument is either null or missing, while the latter returns true if is missing, and false if it is null. @@ -960,7 +942,7 @@ A few guidelines for introducing and reviewing new RPC interfaces: from there. - A RPC method must either be a wallet method or a non-wallet method. Do not - introduce new methods that differ in behavior based on presence of a wallet. + introduce new methods that differ in behavior based on the presence of a wallet. - *Rationale*: as well as complicating the implementation and interfering with the introduction of multi-wallet, wallet and non-wallet code should be @@ -968,7 +950,7 @@ A few guidelines for introducing and reviewing new RPC interfaces: - Try to make the RPC response a JSON object. - - *Rationale*: If a RPC response is not a JSON object then it is harder to avoid API breakage if + - *Rationale*: If a RPC response is not a JSON object, then it is harder to avoid API breakage if new data in the response is needed. - Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with |