diff options
Diffstat (limited to 'doc/developer-notes.md')
| -rw-r--r-- | doc/developer-notes.md | 375 |
1 files changed, 375 insertions, 0 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md new file mode 100644 index 000000000..358792251 --- /dev/null +++ b/doc/developer-notes.md @@ -0,0 +1,375 @@ +Developer Notes +=============== + +Various coding styles have been used during the history of the codebase, +and the result is not very consistent. However, we're now trying to converge to +a single style, so please use it in new code. Old code will be converted +gradually. +- Basic rules specified in src/.clang-format. Use a recent clang-format-3.5 to format automatically. + - Braces on new lines for namespaces, classes, functions, methods. + - 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 namespaces. + - No extra spaces inside parenthesis; don't do ( this ) + - No space after function names; one space after if, for and while. + +Block style example: +```c++ +namespace foo +{ +class Class +{ + bool Function(char* psz, int n) + { + // Comment summarising what this section of code does + for (int i = 0; i < n; i++) { + // When something fails, return early + if (!Something()) + return false; + ... + } + + // Success return is usually at the end + return true; + } +} +} +``` + +Doxygen comments +----------------- + +To facilitate the generation of documentation, use doxygen-compatible comment blocks for functions, methods and fields. + +For example, to describe a function use: +```c++ +/** + * ... text ... + * @param[in] arg1 A description + * @param[in] arg2 Another argument description + * @pre Precondition for function... + */ +bool function(int arg1, const char *arg2) +``` +A complete list of `@xxx` commands can be found at http://www.stack.nl/~dimitri/doxygen/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: +```c++ +/** + * Alerts are for notifying old versions if they become too obsolete and + * need to upgrade. The message is displayed in the status bar. + * @see GetWarnings() + */ +class CAlert +{ +``` + +To describe a member or variable use: +```c++ +int var; //!< Detailed description after the member +``` + +Also OK: +```c++ +/// +/// ... text ... +/// +bool function2(int arg1, const char *arg2) +``` + +Not OK (used plenty in the current source, but not picked up): +```c++ +// +// ... text ... +// +``` + +A full list of comment syntaxes picked up by doxygen can be found at http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html, +but if possible use one of the above styles. + +Development tips and tricks +--------------------------- + +**compiling for debugging** + +Run configure with the --enable-debug option, then make. Or run configure with +CXXFLAGS="-g -ggdb -O0" or whatever debug flags you need. + +**debug.log** + +If the code is behaving strangely, take a look in the debug.log file in the data directory; +error and debugging messages are written there. + +The -debug=... command-line option controls debugging; running with just -debug or -debug=1 will turn +on all categories (and give you a very large debug.log file). + +The Qt code routes qDebug() output to debug.log under category "qt": run with -debug=qt +to see it. + +**testnet and regtest modes** + +Run with the -testnet option to run with "play bitcoins" on the test network, if you +are testing multi-machine code that needs to operate across the internet. + +If you are testing something that can run on one machine, run with the -regtest option. +In regression test mode, blocks can be created on-demand; see qa/rpc-tests/ for tests +that run in -regtest mode. + +**DEBUG_LOCKORDER** + +Bitcoin Core is a multithreaded application, and deadlocks or other multithreading bugs +can be very difficult to track down. Compiling with -DDEBUG_LOCKORDER (configure +CXXFLAGS="-DDEBUG_LOCKORDER -g") inserts run-time checks to keep track of which locks +are held, and adds warnings to the debug.log file if inconsistencies are detected. + +Locking/mutex usage notes +------------------------- + +The code is multi-threaded, and uses mutexes and the +LOCK/TRY_LOCK macros to protect data structures. + +Deadlocks due to inconsistent lock ordering (thread 1 locks cs_main +and then cs_wallet, while thread 2 locks them in the opposite order: +result, deadlock as each waits for the other to release its lock) are +a problem. Compile with -DDEBUG_LOCKORDER to get lock order +inconsistencies 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 CKeyStore class +and its cs_KeyStore lock for example). + +Threads +------- + +- ThreadScriptCheck : Verifies block scripts. + +- ThreadImport : Loads blocks from blk*.dat files or bootstrap.dat. + +- StartNode : Starts other threads. + +- ThreadDNSAddressSeed : Loads addresses of peers from the DNS. + +- ThreadMapPort : Universal plug-and-play startup/shutdown + +- ThreadSocketHandler : Sends/Receives data from peers on port 8333. + +- ThreadOpenAddedConnections : Opens network connections to added nodes. + +- ThreadOpenConnections : Initiates new connections to peers. + +- ThreadMessageHandler : Higher-level message handling (sending and receiving). + +- DumpAddresses : Dumps IP addresses of nodes to peers.dat. + +- ThreadFlushWalletDB : Close the wallet.dat file if it hasn't been used in 500ms. + +- ThreadRPCServer : Remote procedure call handler, listens on port 8332 for connections and services them. + +- BitcoinMiner : Generates bitcoins (if wallet is enabled). + +- Shutdown : Does an orderly shutdown of everything. + +Ignoring IDE/editor files +-------------------------- + +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 +their own editors/IDE/tools, it is less common. Only you know what files your +editor produces and this may change from version to version. The canonical way +to do this is thus to create your local gitignore. Add this to `~/.gitconfig`: + +``` +[core] + excludesfile = /home/.../.gitignore_global +``` + +(alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global` +on a terminal) + +Then put your favourite tool's temporary filenames in that file, e.g. +``` +# NetBeans +nbproject/ +``` + +Another option is to create a per-repository excludes file `.git/info/exclude`. +These are not committed but apply only to one repository. + +If a set of tools is used by the build system or scripts the repository (for +example, lcov) it is perfectly acceptable to add its files to `.gitignore` +and commit them. + +Development guidelines +============================ + +A few non-style-related recommendations for developers, as well as points to +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 + + - *Rationale*: RPC allows for better automatic testing. The test suite for + the GUI is very limited + +- 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 + + - *Explanation*: If the test suite is to be updated for a change, this has to + be done first + +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` + can be NULL. See `qa/rpc-tests/disablewallet.py` for functional tests + exercising the API with `-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++ +------------- + +- Assertions should not have side-effects + + - *Rationale*: Even though the source code is set to to refuse to compile + with assertions disabled, having side-effects in assertions is unexpected and + makes the code harder to understand + +- 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 + +- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example by using + `scoped_pointer` for allocations in a function. + + - *Rationale*: This avoids memory and resource leaks, and ensures exception safety + +C++ data structures +-------------------- + +- 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 + +- Do not compare an iterator from one data structure with an iterator of + 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 + +- Watch out for vector out-of-bounds exceptions. `&vch[0]` is illegal for an + empty vector, `&vch[vch.size()]` is always illegal. Use `begin_ptr(vch)` and + `end_ptr(vch)` to get the begin and end pointer instead (defined in + `serialize.h`) + +- Vector bounds checking is only enabled in debug mode. Do not rely on it + +- Make sure that constructors initialize all fields. If this is skipped for a + good reason (i.e., optimization on the critical path), add an explicit + comment about this + + - *Rationale*: Ensure determinism by avoiding accidental use of uninitialized + values. Also, static analyzers balk about this. + +- Use explicitly signed or unsigned `char`s, or even better `uint8_t` and + `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 + +- 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 + +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 + 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`, `ParseDouble` from `utilstrencodings.h` for number parsing + + - *Rationale*: These functions do overflow checking, and avoid pesky locale issues + +- 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 + +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` + +- 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 + + OK: + +```c++ +{ + TRY_LOCK(cs_vNodes, lockNodes); + ... +} +``` + + Wrong: + +```c++ +TRY_LOCK(cs_vNodes, lockNodes); +{ + ... +} +``` + +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 + + - *Rationale*: Shorter and simpler header files are easier to read, and reduce compile time + +- Don't import anything into the global namespace (`using namespace ...`). Use + fully specified types such as `std::string`. + + - *Rationale*: Avoids symbol conflicts + +GUI +----- + +- 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. |