Developer Notes

Table of Contents

Coding Style

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 and a handful of linters will help you to clean up your patches before submitting them for review. These linters are run automatically when using arc diff but can also be explicitly called with arc lint.

The naming convention roughly mirrors Microsoft Naming Conventions

C++ Coding Standards should strive to follow the LLVM Coding Standards

Code style example:

// namespaces should be lower_snake_case
namespace foo_bar_bob {

/**
 * Class is used for doing classy things.  All classes should
 * have a doxygen comment describing their PURPOSE.  That is to say,
 * why they exist.  Functional details can be determined from the code.
 * @see PerformTask()
 */
class Class {
private:
    //! memberVariable's name should be lowerCamelCase, and be a noun.
    int m_memberVariable;

public:
    /**
    * The documentation before a function or class method should follow Doxygen
    * spec. The name of the function should start with an english verb which
    * indicates the intended purpose of this code.
    *
    * The  function name should be should be CamelCase.
    *
    * @param[in] s    A description
    * @param[in] n    Another argument description
    * @pre Precondition for function...
    */
    bool PerformTask(const std::string& s, int n) {
        // Use lowerChamelCase for local variables.
        bool didMore = false;

        // Comment summarizing the intended purpose of this section of code
        for (int i = 0; i < n; ++i) {
            if (!DidSomethingFail()) {
              return false;
            }
            ...
            if (IsSomethingElse()) {
                DoMore();
                didMore = true;
            } else {
                DoLess();
            }
        }

        return didMore;
    }
}
} // namespace foo

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:

/**
 * ... 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:

/**
 * 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:

int var; //!< Detailed description after the member

or

//! Description before the member
int var;

Also OK:

///
/// ... text ...
///
bool function2(int arg1, const char *arg2)

Not OK (used plenty in the current source, but not picked up):

//
// ... 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.

To build doxygen locally to test changes to the Doxyfile or visualize your comments before landing changes:

ninja doc-doxygen
# output goes to doc/doxygen/html/

Development tips and tricks

Compiling for debugging

Run cmake with -DCMAKE_BUILD_TYPE=Debug to add additional compiler flags that produce better debugging builds.

Compiling for gprof profiling

  cmake -GNinja .. -DENABLE_HARDENING=OFF -DENABLE_PROFIILING=gprof

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.

Writing tests

For details on unit tests, see Compiling/running unit tests.

For details on functional tests, see Functional tests.

Writing script integration tests

Script integration tests are built using src/test/script_tests.cpp:

  1. Uncomment the line with #define UPDATE_JSON_TESTS
  2. Add a new TestBuilder to the script_build test to cover your test case.
  3. ninja check-bitcoin-script_tests
  4. Copy your newly generated test JSON from <build-dir>/src/script_tests.json.gen to src/test/data/script_tests.json.

Please commit your TestBuilder along with your generated test JSON and cleanup the uncommented #define before code review.

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 test/functional/ (/test/functional) for tests that run in -regtest mode.

DEBUG_LOCKORDER

Bitcoin ABC is a multi-threaded application, and deadlocks or other multi-threading bugs can be very difficult to track down. The -DCMAKE_BUILD_TYPE=Debug cmake 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.

Valgrind suppressions file

Valgrind is a programming tool for memory debugging, memory leak detection, and profiling. The repo contains a Valgrind suppressions file (valgrind.supp (/contrib/valgrind.supp)) which includes known Valgrind warnings in our dependencies that cannot be fixed in-tree. Example use:

$ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin
$ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \
      --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite
$ valgrind -v --leak-check=full src/bitcoind -printtoconsole

Compiling for test coverage

LCOV can be used to generate a test coverage report based upon some test targets execution. Some packages are required to generate the coverage report: c++filt, gcov, genhtml, lcov and python3.

To install these dependencies on Debian 10:

sudo apt install binutils-common g++ lcov python3

To enable LCOV report generation during test runs:

cmake -GNinja .. -DENABLE_COVERAGE=ON
ninja coverage-check-all

A coverage report will now be accessible at ./check-all.coverage/index.html.

To include branch coverage, you can add the -DENABLE_BRANCH_COVERAGE=ON option to the cmake command line.

Performance profiling with perf

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, and has been integrated into the functional test framework. Perf can observe a running process and sample (at some frequency) where its execution is.

Perf installation is contingent on which kernel version you’re running; see this StackExchange thread for specific instructions.

Certain kernel parameters may need to be set for perf to be able to inspect the running process’ stack.

$ sudo sysctl -w kernel.perf_event_paranoid=-1
$ sudo sysctl -w kernel.kptr_restrict=0

Make sure you understand the security trade-offs of setting these kernel parameters.

To profile a running bitcoind process for 60 seconds, you could use an invocation of perf record like this:

$ perf record \
    -g --call-graph dwarf --per-thread -F 140 \
    -p `pgrep bitcoind` -- sleep 60

You could then analyze the results by running

perf report --stdio | c++filt | less

or using a graphical tool like Hotspot.

See the functional test documentation for how to invoke perf within tests.

Sanitizers

Bitcoin ABC can be compiled with various “sanitizers” enabled, which add instrumentation for issues regarding things like memory safety, thread race conditions, or undefined behavior. This is controlled with the -DENABLE_SANITIZERS cmake flag, which should be a semicolon separated list of sanitizers to enable. The sanitizer list should correspond to supported -fsanitize= options in your compiler. These sanitizers have runtime overhead, so they are most useful when testing changes or producing debugging builds.

Some examples:

# Enable both the address sanitizer and the undefined behavior sanitizer
cmake -GNinja .. -DENABLE_SANITIZERS="address;undefined"

# Enable the thread sanitizer
cmake -GNinja .. -DENABLE_SANITIZERS=thread

If you are compiling with GCC you will typically need to install corresponding “san” libraries to actually compile with these flags, e.g. libasan for the address sanitizer, libtsan for the thread sanitizer, and libubsan for the undefined sanitizer. If you are missing required libraries, the cmake script will fail with an error when testing the sanitizer flags.

Note that the sanitizers will give a better output if they are run with a Debug build configuration.

There are a number of known problems for which suppressions files are provided under test/sanitizer_suppressions. These files are intended to be used with the suppressions option from the sanitizers. If you are using the check-* targets to run the tests, the suppression options are automatically set. Otherwise they need to be set manually using environment variables; refer to your compiler manual for the correct syntax.

The address sanitizer is known to fail in sha256_sse4::Transform (/src/crypto/sha256_sse4.cpp) which makes it unusable unless you also use -DCRYPTO_USE_ASM=OFF when running cmake. We would like to fix sanitizer issues, so please send pull requests if you can fix any errors found by the address sanitizer (or any other sanitizer).

Not all sanitizer options can be enabled at the same time, e.g. trying to build with `-DENABLE_SANITIZERS==”address;thread” will fail in the cmake script as these sanitizers are mutually incompatible. Refer to your compiler manual to learn more about these options and which sanitizers are supported by your compiler.

Examples:

Build and run the test suite with the address sanitizer enabled:

mkdir build_asan
cd build_asan

cmake -GNinja .. \
  -DCMAKE_BUILD_TYPE=Debug \
  -DENABLE_SANITIZERS=address \
  -DCRYPTO_USE_ASM=OFF

ninja check check-functional

Build and run the test suite with the thread sanitizer enabled (it can take a very long time to complete):

mkdir build_tsan
cd build_tsan

cmake -GNinja .. \
  -DCMAKE_BUILD_TYPE=Debug \
  -DENABLE_SANITIZERS=thread

ninja check check-functional

Build and run the test suite with the undefined sanitizer enabled:

mkdir build_ubsan
cd build_ubsan

cmake -GNinja .. \
  -DCMAKE_BUILD_TYPE=Debug \
  -DENABLE_SANITIZERS=undefined

ninja check check-functional

Additional resources:

Locking/mutex usage notes

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 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 (or use -DCMAKE_BUILD_TYPE=Debug) 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 FillableSigningProvider class and its cs_KeyStore lock for example).

Threads

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 ABC, 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 favorite 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 ABC code.

Wallet

General C++

C++ data structures

class A
{
    uint32_t m_count{0};
}

Strings and formatting

Variable names

The shadowing warning (-Wshadow) is enabled by default. It prevents issues rising from using a different variable with the same name.

E.g. in member initializers, prepend _ to the argument name shadowing the member name:

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.

Please name variables so that their names do not shadow variables defined in the source code.

Threads and synchronization

{
    TRY_LOCK(cs_vNodes, lockNodes);
    ...
}

Wrong:

TRY_LOCK(cs_vNodes, lockNodes);
{
    ...
}

Scripts

Shebang

Source code organization

namespace mynamespace {
    ...
} // namespace mynamespace

namespace {
    ...
} // namespace

Header Inclusions

All headers should be lexically ordered inside their block.

#ifndef BITCOIN_FOO_BAR_H
#define BITCOIN_FOO_BAR_H
...
#endif // BITCOIN_FOO_BAR_H

GUI

Unit Tests

Third party libraries

Several parts of the repository are software maintained elsewhere.

Changes to these should preferably be sent upstream but bugfixes may also be submitted to Bitcoin ABC so that they can be integrated quickly. Cosmetic changes should be purely taken upstream.

Current third party libraries include:

Upgrading LevelDB

Extra care must be taken when upgrading LevelDB. This section explains issues you must be aware of.

File Descriptor Counts

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 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 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.

In addition to reviewing the upstream changes in env_posix.cc, you can use lsof to check this. For example, on Linux this command will show open .ldb file counts:

$ lsof -p $(pidof bitcoind) |\
    awk 'BEGIN { fd=0; mem=0; } /ldb$/ { if ($4 == "mem") mem++; else fd++ } END { printf "mem = %s, fd = %s\n", mem, fd}'
mem = 119, fd = 0

The mem value shows how many files are mmap’ed, and the fd value shows you many file descriptors these files are using. You should check that fd is a small number (usually 0 on 64-bit hosts).

See the notes in the SetMaxOpenFiles() function in dbwrapper.cc for more details.

Consensus Compatibility

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 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 would be to revert the upstream fix before applying the updates to Bitcoin ABC’s copy of LevelDB. In general you should be wary of any upstream changes affecting what data is returned from LevelDB queries.

Git and GitHub tips

RPC interface guidelines

A few guidelines for introducing and reviewing new RPC interfaces:

Internal interface guidelines

Internal interfaces between parts of the codebase that are meant to be independent (node, wallet, GUI), are defined in src/interfaces/ (../src/interfaces/). The main interface classes defined there are interfaces::Chain (../src/interfaces/chain.h), used by wallet to access the node’s latest chain state, interfaces::Node (../src/interfaces/node.h), used by the GUI to control the node, and interfaces::Wallet (../src/interfaces/wallet.h), used by the GUI to control an individual wallet. There are also more specialized interface types like interfaces::Handler (../src/interfaces/handler.h) interfaces::ChainClient (../src/interfaces/chain.h) passed to and from various interface methods.

Interface classes are written in a particular style so node, wallet, and GUI code doesn’t need to run in the same process, and so the class declarations work more easily with tools and libraries supporting interprocess communication: