diff --git a/arcanist/linter/CppVoidParameterLinter.php b/arcanist/linter/CppVoidParameterLinter.php index bcd0e347f..6750532ad 100644 --- a/arcanist/linter/CppVoidParameterLinter.php +++ b/arcanist/linter/CppVoidParameterLinter.php @@ -1,75 +1,75 @@ ArcanistLintSeverity::SEVERITY_AUTOFIX, ); } public function getLintNameMap() { return array( self::VOID_PARAMETER_FOUND => pht('Use C++ style void parameters.'), ); } private function isFalsePositive($line) { foreach(self::FALSE_POSITIVES as $falsePositive) { if (strpos($line, $falsePositive) !== false) { return true; } } return false; } public function lintPath($path) { $absPath = Filesystem::resolvePath($path, $this->getProjectRoot()); $fileContent = Filesystem::readFile($absPath); - if (preg_match_all('/[^\s{]+\s?\(void\)/', $fileContent, $voidParameters, + if (preg_match_all('/[^\s{(]+\s?\(void\)/', $fileContent, $voidParameters, PREG_OFFSET_CAPTURE)) { foreach ($voidParameters[0] as $voidParameter) { list($function, $offset) = $voidParameter; if ($this->isFalsePositive($function)) { continue; } $this->raiseLintAtOffset( $offset, self::VOID_PARAMETER_FOUND, pht('C++ style parameters should be used: () instead of (void).'), $function, str_replace('(void)', '()', $function) )->setBypassChangedLineFiltering(true); } } } } diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 631e62c8e..7452431d1 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1,1162 +1,1189 @@ Developer Notes =============== **Table of Contents** - [Developer Notes](#developer-notes) - [Coding Style (General)](#coding-style-general) - [Coding Style (C++)](#coding-style-c) - [Doxygen comments](#doxygen-comments) - [Coding Style (Python)](#coding-style-python) - [Development tips and tricks](#development-tips-and-tricks) - [Compiling for debugging](#compiling-for-debugging) - [Compiling for gprof profiling](#compiling-for-gprof-profiling) - [debug.log](#debuglog) - [Writing tests](#writing-tests) - [Writing script integration tests](#writing-script-integration-tests) - [Testnet and Regtest modes](#testnet-and-regtest-modes) - [DEBUG_LOCKORDER](#debug_lockorder) - [Valgrind suppressions file](#valgrind-suppressions-file) - [Compiling for test coverage](#compiling-for-test-coverage) - [Performance profiling with perf](#performance-profiling-with-perf) - [Sanitizers](#sanitizers) - [Locking/mutex usage notes](#lockingmutex-usage-notes) - [Threads](#threads) - [Ignoring IDE/editor files](#ignoring-ideeditor-files) - [Development guidelines](#development-guidelines) - [Wallet](#wallet) - [General C++](#general-c) - [C++ data structures](#c-data-structures) - [Strings and formatting](#strings-and-formatting) - [Variable names](#variable-names) - [Threads and synchronization](#threads-and-synchronization) - [Scripts](#scripts) - [Shebang](#shebang) - [Source code organization](#source-code-organization) - [GUI](#gui) - [Unit tests](#unit-tests) - [Third party libraries](#third-party-libraries) - [Git and GitHub tips](#git-and-github-tips) - [Release notes](#release-notes) - [RPC interface guidelines](#rpc-interface-guidelines) - [Internal interface guidelines](#internal-interface-guidelines) Coding Style (General) ---------------------- 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`. Coding Style (C++) ------------------ - Basic rules specified in [src/.clang-format](/src/.clang-format). - 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 `namespace`. - No extra spaces inside parenthesis; don't do ( this ) - No space after function names; one space after `if`, `for` and `while`. - Always add braces for block statements (e.g. `if`, `for`, `while`). - `++i` is preferred over `i++`. - `static_assert` is preferred over `assert` where possible. Generally; compile-time checking is preferred over run-time checking. - Use CamelCase for functions/methods, and lowerCamelCase for variables. - GLOBAL_CONSTANTS should use UPPER_SNAKE_CASE. - namespaces should use lower_snake_case. - Function names should generally start with an English command-form verb (e.g. `ValidateTransaction`, `AddTransactionToMempool`, `ConnectBlock`) - Variable names should generally be nouns or past/future tense verbs. (e.g. `canDoThing`, `signatureOperations`, `didThing`) - Avoid using globals, remove existing globals whenever possible. - Class member variable names should be prepended with `m_` - DO choose easily readable identifier names. - DO favor readability over brevity. - DO NOT use Hungarian notation. - DO NOT use abbreviations or contractions within identifiers. - WRONG: mempool - RIGHT: MemoryPool - WRONG: ChangeDir - RIGHT: ChangeDirectory - DO NOT use obscure acronyms, DO uppercase any acronyms. - FINALLY, do not migrate existing code unless refactoring. It makes forwarding-porting from Bitcoin Core more difficult. The naming convention roughly mirrors [Microsoft Naming Conventions](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions) C++ Coding Standards should strive to follow the [LLVM Coding Standards](https://llvm.org/docs/CodingStandards.html) Code style example: ```c++ // 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: ```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 ``` or ```cpp //! Description before the member int var; ``` 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. 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/ ``` Coding Style (Python) --------------------- Refer to [functional-tests.md#style-guidelines](functional-tests.md#style-guidelines). 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](unit-tests.md). For details on functional tests, see [Functional tests](functional-tests.md). ### 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 `/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. +### Assertions and Checks + +The util file `src/util/check.h` offers helpers to protect against coding and +internal logic bugs. They must never be used to validate user, network or any +other input. + +* `assert` or `Assert` should be used to document assumptions when any + violation would mean that it is not safe to continue program execution. The + code is always compiled with assertions enabled. + - For example, a nullptr dereference or any other logic bug in validation + code means the program code is faulty and must terminate immediately. +* `CHECK_NONFATAL` should be used for recoverable internal logic bugs. On + 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 + 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 + behaves like `Assert`/`assert` to notify developers and testers about + nonfatal errors. In production it doesn't warn or log anything, though the + expression is always evaluated. + - For example it can be assumed that a variable is only initialized once, + but a failed assumption does not result in a fatal bug. A failed + assumption may or may not result in a slightly degraded user experience, + but it is safe to continue program execution. + ### 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: ```shell $ 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: ```shell sudo apt install binutils-common g++ lcov python3 ``` To enable LCOV report generation during test runs: ```shell 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`](http://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. Perf installation is contingent on which kernel version you're running; see [this StackExchange thread](https://askubuntu.com/questions/50145/how-to-install-perf-monitoring-tool) for specific instructions. Certain kernel parameters may need to be set for perf to be able to inspect the running process' stack. ```sh $ sudo sysctl -w kernel.perf_event_paranoid=-1 $ sudo sysctl -w kernel.kptr_restrict=0 ``` Make sure you [understand the security trade-offs](https://lwn.net/Articles/420403/) of setting these kernel parameters. To profile a running bitcoind process for 60 seconds, you could use an invocation of `perf record` like this: ```sh $ perf record \ -g --call-graph dwarf --per-thread -F 140 \ -p `pgrep bitcoind` -- sleep 60 ``` You could then analyze the results by running ```sh perf report --stdio | c++filt | less ``` or using a graphical tool like [Hotspot](https://github.com/KDAB/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: ```bash # 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: ```bash 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): ```bash 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: ```bash mkdir build_ubsan cd build_ubsan cmake -GNinja .. \ -DCMAKE_BUILD_TYPE=Debug \ -DENABLE_SANITIZERS=undefined ninja check check-functional ``` Additional resources: * [AddressSanitizer](https://clang.llvm.org/docs/AddressSanitizer.html) * [LeakSanitizer](https://clang.llvm.org/docs/LeakSanitizer.html) * [MemorySanitizer](https://clang.llvm.org/docs/MemorySanitizer.html) * [ThreadSanitizer](https://clang.llvm.org/docs/ThreadSanitizer.html) * [UndefinedBehaviorSanitizer](https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html) * [GCC Instrumentation Options](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html) * [Google Sanitizers Wiki](https://github.com/google/sanitizers/wiki) * [Issue #12691: Enable -fsanitize flags in Travis](https://github.com/bitcoin/bitcoin/issues/12691) 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 ------- - [Main thread (`bitcoind`)](https://www.bitcoinabc.org/doc/dev/bitcoind_8cpp.html#a0ddf1224851353fc92bfbff6f499fa97) : Started from `main()` in `bitcoind.cpp`. Responsible for starting up and shutting down the application. - [ThreadImport (`b-loadblk`)](https://www.bitcoinabc.org/doc/dev/init_8cpp.html#ae9e290a0e829ec0198518de2eda579d1) : Loads blocks from `blk*.dat` files or `-loadblock=` on startup. - [ThreadScriptCheck (`b-scriptch.x`)](https://www.bitcoinabc.org/doc/dev/validation_8cpp.html#a925a33e7952a157922b0bbb8dab29a20) : Parallel script validation threads for transactions in blocks. - [ThreadHTTP (`b-http`)](https://www.bitcoinabc.org/doc/dev/httpserver_8cpp.html#abb9f6ea8819672bd9a62d3695070709c) : Libevent thread to listen for RPC and REST connections. - [HTTP worker threads(`b-httpworker.x`)](https://www.bitcoinabc.org/doc/dev/httpserver_8cpp.html#aa6a7bc27265043bc0193220c5ae3a55f) : Threads to service RPC and REST requests. - [Indexer threads (`b-txindex`, etc)](https://www.bitcoinabc.org/doc/dev/class_base_index.html#a96a7407421fbf877509248bbe64f8d87) : One thread per indexer. - [SchedulerThread (`b-scheduler`)](https://www.bitcoinabc.org/doc/dev/class_c_scheduler.html#a14d2800815da93577858ea078aed1fba) : Does asynchronous background tasks like dumping wallet contents, dumping addrman and running asynchronous validationinterface callbacks. - [TorControlThread (`b-torcontrol`)](https://www.bitcoinabc.org/doc/dev/torcontrol_8cpp.html#a4faed3692d57a0d7bdbecf3b37f72de0) : Libevent thread for tor connections. - Net threads: - [ThreadMessageHandler (`b-msghand`)](https://www.bitcoinabc.org/doc/dev/class_c_connman.html#aacdbb7148575a31bb33bc345e2bf22a9) : Application level message handling (sending and receiving). Almost all net_processing and validation logic runs on this thread. - [ThreadDNSAddressSeed (`b-dnsseed`)](https://www.bitcoinabc.org/doc/dev/class_c_connman.html#aa7c6970ed98a4a7bafbc071d24897d13) : Loads addresses of peers from the DNS. - [ThreadMapPort (`b-upnp`)](https://www.bitcoinabc.org/doc/dev/net_8cpp.html#a63f82a71c4169290c2db1651a9bbe249) : Universal plug-and-play startup/shutdown. - [ThreadSocketHandler (`b-net`)](https://www.bitcoinabc.org/doc/dev/class_c_connman.html#a765597cbfe99c083d8fa3d61bb464e34) : Sends/Receives data from peers on port 8333. - [ThreadOpenAddedConnections (`b-addcon`)](https://www.bitcoinabc.org/doc/dev/class_c_connman.html#a0b787caf95e52a346a2b31a580d60a62) : Opens network connections to added nodes. - [ThreadOpenConnections (`b-opencon`)](https://www.bitcoinabc.org/doc/dev/class_c_connman.html#a55e9feafc3bab78e5c9d408c207faa45) : Initiates new connections to peers. 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 ------- - 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 `test/functional/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 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 `unique_ptr` for allocations in a function. - *Rationale*: This avoids memory and resource leaks, and ensures exception safety - Use `std::make_unique()` to construct objects owned by `unique_ptr`s - *Rationale*: `std::make_unique` is concise and ensures exception safety in complex expressions. 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 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 - 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 - *Rationale*: Ensure determinism by avoiding accidental use of uninitialized values. Also, static analyzers balk about this. Initializing the members in the declaration makes it easy to spot uninitialized ones. ```cpp class A { uint32_t m_count{0}; } ``` - By default, declare single-argument constructors `explicit`. - *Rationale*: This is a precaution to avoid unintended conversions that might arise when single-argument constructors are used as implicit conversion functions. - 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 ------------------------ - 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`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing - *Rationale*: These functions do overflow checking, and avoid pesky locale issues 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: ```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. Please name variables so that their names do not shadow variables defined in the source code. 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 `-DCMAKE_BUILD_TYPE=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); { ... } ``` Scripts -------------------------- ### Shebang - Use `#!/usr/bin/env bash` instead of obsolete `#!/bin/bash`. - [*Rationale*](https://github.com/dylanaraps/pure-bash-bible#shebang): `#!/bin/bash` assumes it is always installed to /bin/ which can cause issues; `#!/usr/bin/env bash` searches the user's PATH to find the bash binary. OK: ```bash #!/usr/bin/env bash ``` Wrong: ```bash #!/bin/bash ``` 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 - Use only the lowercase alphanumerics (`a-z0-9`), underscore (`_`) and hyphen (`-`) in source code filenames. - *Rationale*: `grep`:ing and auto-completing filenames is easier when using a consistent naming pattern. Potential problems when building on case-insensitive filesystems are avoided when using only lowercase characters in source code filenames. - Don't import anything into the global namespace (`using namespace ...`). Use fully specified types such as `std::string`. - *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. ```c++ namespace mynamespace { ... } // namespace mynamespace namespace { ... } // namespace ``` - *Rationale*: Avoids confusion about the namespace context Header Inclusions ----------------- - Header inclusions should use angle brackets (`#include <>`). The include path should be relative to the `src` folder. e.g.: `#include ` - Native C++ headers should be preferred over C compatibility headers. e.g.: use `` instead of `` - In order to make the code consistent, header files should be included in the following order, with each section separated by a newline: 1. In a .cpp file, the associated .h is in first position. In a test source, this is the header file under test. 2. The project headers. 3. The test headers. 4. The 3rd party libraries headers. Different libraries should be in different sections. 5. The system libraries. All headers should be lexically ordered inside their block. - Use include guards to avoid the problem of double inclusion. The header file `foo/bar.h` should use the include guard identifier `BITCOIN_FOO_BAR_H`, e.g. ```c++ #ifndef BITCOIN_FOO_BAR_H #define BITCOIN_FOO_BAR_H ... #endif // BITCOIN_FOO_BAR_H ``` 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. - Avoid adding slow or blocking code in the GUI thread. In particular do not add new `interface::Node` and `interface::Wallet` method calls, even if they may be fast now, in case they are changed to lock or communicate across processes in the future. Prefer to offload work from the GUI thread to worker threads (see `RPCExecutor` in console code as an example) or take other steps (see ) to keep the GUI responsive. - *Rationale*: Blocking the GUI thread can increase latency, and lead to hangs and deadlocks. Unit Tests ----------- - 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. 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: - src/leveldb - Upstream at ; Maintained by Google. - **Note**: Follow the instructions in [Upgrading LevelDB](#upgrading-leveldb) when merging upstream changes to Bitcoin ABC. - src/secp256k1 - Upstream at ; actively maintained by Bitcoin Core contributors. Bitcoin ABC is using a modified version of libsecp256k1, some changes might be directly submitted to Bitcoin ABC. See the [secp256k1 README](../src/secp256k1/README.md) for details. - src/crypto/ctaes - Upstream at https://github.com/bitcoin-core/ctaes ; maintained by Bitcoin Core contributors. - src/univalue - Upstream at https://github.com/jgarzik/univalue ; maintained by Jeff Garzik. 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: ```bash $ 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 --------------------- - Github is not typically the source of truth for pull requests. See [CONTRIBUTING](../CONTRIBUTING.md) for instructions on setting up your repo correctly. - Similarly, your git remote origin should be set to: `ssh://vcs@reviews.bitcoinabc.org:2221/source/bitcoin-abc.git` instead of github.com. See [CONTRIBUTING](../CONTRIBUTING.md). For git and GitHub productivity tips, see [Productivity Notes](productivity.md). Release notes ------------- Release notes should be written for any PR that: - introduces a notable new feature - fixes a significant bug - changes an API or configuration model - makes any other visible change to the end-user experience. Release notes should be added to the [/doc/release-notes.md](/doc/release-notes.md) file, which is archived and cleared after each release. 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` - *Rationale*: Consistency with existing interface. - Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`) - *Rationale*: Consistency with 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. 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 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 `prioritisetransaction` and `getblocktemplate` because their interface is specified as-is in BIP22. - 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 `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. - *Rationale*: Avoids surprises when switching to name-based arguments. Missing name-based arguments are passed as 'null'. - Try not to overload methods on argument type. E.g. don't make `getblock(true)` and `getblock("hash")` do different things. - *Rationale*: This is impossible to use with `bitcoin-cli`, and can be surprising to users. - *Exception*: Some RPC calls can take both an `int` and `bool`, most notably when a bool was switched to a multi-value, or due to other historical reasons. **Always** have false map to 0 and true to 1 in this case. - 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. - Set okSafeMode in the RPC command table to a sensible value: safe mode is when the blockchain is regarded to be in a confused state, and the client deems it unsafe to do anything irreversible such as send. Anything that just queries should be permitted. - *Rationale*: Troubleshooting a node in safe mode is difficult if half the RPCs don't work. - Add every non-string RPC argument `(method, idx, name)` to the table `vRPCConvertParams` in `rpc/client.cpp`. - *Rationale*: `bitcoin-cli` and the GUI debug console use this table to determine how to convert a plaintext command line to JSON. If the types don't match, the method can be unusable from there. - A RPC method must either be a wallet method or a non-wallet method. Do not introduce new methods such as `signrawtransaction` that differ in behavior based on 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 separated to avoid introducing circular dependencies between code units. - 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 new data in the response is needed. - Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with `getblockchaininfo`'s state immediately prior to the call's execution. Wallet RPCs whose behavior does *not* depend on the current chainstate may omit this call. - *Rationale*: In previous versions of Bitcoin Core, the wallet was always in-sync with the chainstate (by virtue of them all being updated in the same cs_main lock). In order to maintain the behavior that wallet RPCs return results as of at least the highest best-known block an RPC client may be aware of prior to entering a wallet RPC call, we must block until the wallet is caught up to the chainstate as of the RPC call's entry. This also makes the API much easier for RPC clients to reason about. - Be aware of RPC method aliases and generally avoid registering the same callback function pointer for different RPCs. - *Rationale*: RPC methods registered with the same function pointer will be considered aliases and only the first method name will show up in the `help` RPC command list. - *Exception*: Using RPC method aliases may be appropriate in cases where a new RPC is replacing a deprecated RPC, to avoid both RPCs confusingly showing up in the command list. - Use the `UNIX_EPOCH_TIME` constant when describing UNIX epoch time or timestamps in the documentation. - *Rationale*: User-facing consistency. 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: - Interface classes should be abstract and have methods that are [pure virtual](https://en.cppreference.com/w/cpp/language/abstract_class). This allows multiple implementations to inherit from the same interface class, particularly so one implementation can execute functionality in the local process, and other implementations can forward calls to remote processes. - Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [`src/node/`](../src/node/) or [`src/wallet/`](../src/wallet/) and just exposed in [`src/interfaces/`](../src/interfaces/) instead of being implemented there, so it can be more modular and accessible to unit tests. - Interface method parameter and return types should either be serializable or be other interface classes. Interface methods shouldn't pass references to objects that can't be serialized or accessed from another process. Examples: ```c++ // Good: takes string argument and returns interface class pointer virtual unique_ptr loadWallet(std::string filename) = 0; // Bad: returns CWallet reference that can't be used from another process virtual CWallet& loadWallet(std::string filename) = 0; ``` ```c++ // Good: accepts and returns primitive types virtual bool findBlock(const uint256& hash, int& out_height, int64_t& out_time) = 0; // Bad: returns pointer to internal node in a linked list inaccessible to // other processes virtual const CBlockIndex* findBlock(const uint256& hash) = 0; ``` ```c++ // Good: takes plain callback type and returns interface pointer using TipChangedFn = std::function; virtual std::unique_ptr handleTipChanged(TipChangedFn fn) = 0; // Bad: returns boost connection specific to local process using TipChangedFn = std::function; 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. 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: Example: ```c++ // Good: method names are unique virtual bool disconnectByAddress(const CNetAddr& net_addr) = 0; virtual bool disconnectById(NodeId id) = 0; // Bad: methods are overloaded by type virtual bool disconnect(const CNetAddr& net_addr) = 0; 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 `UpperCamelCase`. Examples: ```c++ // Good: lowerCamelCase method name virtual void blockConnected(const CBlock& block, int height) = 0; // Bad: uppercase class method virtual void BlockConnected(const CBlock& block, int height) = 0; ``` ```c++ // Good: UpperCamelCase standalone function name std::unique_ptr MakeNode(LocalInit& init); // Bad: lowercase standalone function std::unique_ptr makeNode(LocalInit& init); ``` Note: This last convention isn't generally followed outside of [`src/interfaces/`](../src/interfaces/), though it did come up for discussion before in [#14635](https://github.com/bitcoin/bitcoin/pull/14635). diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 1f102528d..82c3ed26f 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,739 +1,738 @@ # Copyright (c) 2017 The Bitcoin developers project(bitcoind) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) # Default visibility is hidden on all targets. set(CMAKE_C_VISIBILITY_PRESET hidden) set(CMAKE_CXX_VISIBILITY_PRESET hidden) option(BUILD_BITCOIN_WALLET "Activate the wallet functionality" ON) option(BUILD_BITCOIN_ZMQ "Activate the ZeroMQ functionalities" ON) option(BUILD_BITCOIN_CLI "Build bitcoin-cli" ON) option(BUILD_BITCOIN_TX "Build bitcoin-tx" ON) option(BUILD_BITCOIN_QT "Build bitcoin-qt" ON) option(BUILD_BITCOIN_SEEDER "Build bitcoin-seeder" ON) option(BUILD_LIBBITCOINCONSENSUS "Build the bitcoinconsenus shared library" ON) option(ENABLE_BIP70 "Enable BIP70 (payment protocol) support in GUI" ON) option(ENABLE_HARDENING "Harden the executables" ON) option(ENABLE_REDUCE_EXPORTS "Reduce the amount of exported symbols" OFF) option(ENABLE_STATIC_LIBSTDCXX "Statically link libstdc++" OFF) option(ENABLE_GLIBC_BACK_COMPAT "Enable Glibc compatibility features" OFF) option(ENABLE_QRCODE "Enable QR code display" ON) option(ENABLE_UPNP "Enable UPnP support" ON) option(START_WITH_UPNP "Make UPnP the default to map ports" OFF) option(ENABLE_CLANG_TIDY "Enable clang-tidy checks for Bitcoin ABC" OFF) option(ENABLE_PROFILING "Select the profiling tool to use" OFF) # Linker option if(CMAKE_CROSSCOMPILING) set(DEFAULT_LINKER "") elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") set(DEFAULT_LINKER lld) elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") set(DEFAULT_LINKER gold) else() set(DEFAULT_LINKER "") endif() set(USE_LINKER "${DEFAULT_LINKER}" CACHE STRING "Linker to be used (default: ${DEFAULT_LINKER}). Set to empty string to use the system's default.") set(OS_WITH_JEMALLOC_AS_SYSTEM_DEFAULT "Android" "FreeBSD" "NetBSD" ) if(NOT CMAKE_SYSTEM_NAME IN_LIST OS_WITH_JEMALLOC_AS_SYSTEM_DEFAULT) set(USE_JEMALLOC_DEFAULT ON) endif() # FIXME: Building against jemalloc causes the software to segfault on OSX. # See https://github.com/Bitcoin-ABC/bitcoin-abc/issues/401 if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin" AND NOT CMAKE_CROSSCOMPILING) set(USE_JEMALLOC_DEFAULT OFF) endif() option(USE_JEMALLOC "Use jemalloc as an allocation library" ${USE_JEMALLOC_DEFAULT}) if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") set(DEFAULT_ENABLE_DBUS_NOTIFICATIONS ON) endif() option(ENABLE_DBUS_NOTIFICATIONS "Enable DBus desktop notifications. Linux only." ${DEFAULT_ENABLE_DBUS_NOTIFICATIONS}) # If ccache is available, then use it. find_program(CCACHE ccache) if(CCACHE) message(STATUS "Using ccache: ${CCACHE}") set(CMAKE_C_COMPILER_LAUNCHER ${CCACHE}) set(CMAKE_CXX_COMPILER_LAUNCHER ${CCACHE}) endif(CCACHE) # Disable what we do not need for the native build. include(NativeExecutable) native_add_cmake_flags( "-DBUILD_BITCOIN_WALLET=OFF" "-DBUILD_BITCOIN_QT=OFF" "-DBUILD_BITCOIN_ZMQ=OFF" "-DENABLE_QRCODE=OFF" "-DENABLE_UPNP=OFF" "-DUSE_JEMALLOC=OFF" "-DENABLE_CLANG_TIDY=OFF" "-DENABLE_BIP70=OFF" ) if(ENABLE_CLANG_TIDY) include(ClangTidy) endif() if(ENABLE_SANITIZERS) include(Sanitizers) enable_sanitizers(${ENABLE_SANITIZERS}) endif() include(AddCompilerFlags) if(USE_LINKER) set(LINKER_FLAG "-fuse-ld=${USE_LINKER}") check_linker_flag(IS_LINKER_SUPPORTED ${LINKER_FLAG}) if(NOT IS_LINKER_SUPPORTED) message(FATAL_ERROR "The ${USE_LINKER} linker is not supported, make sure ${USE_LINKER} is properly installed or use -DUSE_LINKER= to use the system's linker") endif() add_linker_flags(${LINKER_FLAG}) endif() # Prefer -g3, defaults to -g if unavailable foreach(LANGUAGE C CXX) set(COMPILER_DEBUG_LEVEL -g) check_compiler_flags(G3_IS_SUPPORTED ${LANGUAGE} -g3) if(${G3_IS_SUPPORTED}) set(COMPILER_DEBUG_LEVEL -g3) endif() add_compile_options_to_configuration_for_language(Debug ${LANGUAGE} ${COMPILER_DEBUG_LEVEL}) endforeach() -# Define the debugging symbols DEBUG and DEBUG_LOCKORDER when the Debug build -# type is selected. -add_compile_definitions_to_configuration(Debug DEBUG DEBUG_LOCKORDER) +# Define some debugging symbols when the Debug build type is selected. +add_compile_definitions_to_configuration(Debug DEBUG DEBUG_LOCKORDER ABORT_ON_FAILED_ASSUME) # Add -ftrapv when building in Debug add_compile_options_to_configuration(Debug -ftrapv) # All versions of gcc that we commonly use for building are subject to bug # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348. To work around that, set # -fstack-reuse=none for all gcc builds. (Only gcc understands this flag) if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") add_compiler_flags(-fstack-reuse=none) endif() if(${CMAKE_SYSTEM_NAME} MATCHES "Windows") # Ensure that WINDRES_PREPROC is enabled when using windres. list(APPEND CMAKE_RC_FLAGS "-DWINDRES_PREPROC") # Build all static so there is no dll file to distribute. add_linker_flags(-static) add_compile_definitions( # Windows 7 _WIN32_WINNT=0x0601 # Internet Explorer 5.01 (!) _WIN32_IE=0x0501 # Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, # RPC, Shell, and Windows Sockets. WIN32_LEAN_AND_MEAN ) endif() if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") add_compile_definitions(MAC_OSX OBJC_OLD_DISPATCH_PROTOTYPES=0) add_linker_flags(-Wl,-dead_strip_dylibs) endif() if(ENABLE_REDUCE_EXPORTS) # Default visibility is set by CMAKE__VISIBILITY_PRESET, but this # doesn't tell if the visibility set is effective. # Check if the flag -fvisibility=hidden is supported, as using the hidden # visibility is a requirement to reduce exports. check_compiler_flags(HAS_CXX_FVISIBILITY CXX -fvisibility=hidden) if(NOT HAS_CXX_FVISIBILITY) message(FATAL_ERROR "Cannot set default symbol visibility. Use -DENABLE_REDUCE_EXPORTS=OFF.") endif() # Also hide symbols from static libraries add_linker_flags(-Wl,--exclude-libs,libstdc++) endif() # Enable statically linking libstdc++ if(ENABLE_STATIC_LIBSTDCXX) add_linker_flags(-static-libstdc++) endif() set(CMAKE_POSITION_INDEPENDENT_CODE ON) if(ENABLE_HARDENING) # Enable stack protection add_cxx_compiler_flags(-fstack-protector-all -Wstack-protector) # Enable some buffer overflow checking, except in -O0 builds which # do not support them add_compiler_flags(-U_FORTIFY_SOURCE) add_compile_options($<$>:-D_FORTIFY_SOURCE=2>) # Enable ASLR (these flags are primarily targeting MinGw) add_linker_flags(-Wl,--dynamicbase -Wl,--nxcompat -Wl,--high-entropy-va) # Make the relocated sections read-only add_linker_flags(-Wl,-z,relro -Wl,-z,now) # CMake provides the POSITION_INDEPENDENT_CODE property to set PIC/PIE. cmake_policy(SET CMP0083 NEW) include(CheckPIESupported) check_pie_supported() if(${CMAKE_SYSTEM_NAME} MATCHES "Windows") # MinGw provides its own libssp for stack smashing protection link_libraries(ssp) endif() endif() if(ENABLE_PROFILING MATCHES "gprof") message(STATUS "Enable profiling with gprof") # -pg is incompatible with -pie. Since hardening and profiling together # doesn't make sense, we simply make them mutually exclusive here. # Additionally, hardened toolchains may force -pie by default, in which # case it needs to be turned off with -no-pie. if(ENABLE_HARDENING) message(FATAL_ERROR "Profiling with gprof requires disabling hardening with -DENABLE_HARDENING=OFF.") endif() add_linker_flags(-no-pie) add_compiler_flags(-pg) add_linker_flags(-pg) endif() # Enable warning add_c_compiler_flags(-Wnested-externs -Wstrict-prototypes) add_compiler_flags( -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized ) add_compiler_flag_group(-Wformat -Wformat-security) add_cxx_compiler_flags( -Wredundant-move ) if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") # GCC has no flag variant which is granular enough to avoid raising the clang # -Wshadow-uncaptured-local equivalent. This is causing a lot of warnings # on serialize.h which cannot be disabled locally, so drop the flag. add_compiler_flags( -Wshadow -Wshadow-field ) endif() option(EXTRA_WARNINGS "Enable extra warnings" OFF) if(EXTRA_WARNINGS) add_cxx_compiler_flags(-Wsuggest-override) else() add_compiler_flags(-Wno-unused-parameter) add_compiler_flags(-Wno-implicit-fallthrough) endif() # libtool style configure add_subdirectory(config) # Enable LFS (Large File Support) on targets that don't have it natively. # This should be defined before the libraries are included as leveldb need the # definition to be set. if(NOT HAVE_LARGE_FILE_SUPPORT) add_compile_definitions(_FILE_OFFSET_BITS=64) add_linker_flags(-Wl,--large-address-aware) endif() if(ENABLE_GLIBC_BACK_COMPAT) # Wrap some glibc functions with ours add_linker_flags(-Wl,--wrap=__divmoddi4) add_linker_flags(-Wl,--wrap=log2f) if(NOT HAVE_LARGE_FILE_SUPPORT) add_linker_flags(-Wl,--wrap=fcntl -Wl,--wrap=fcntl64) endif() endif() if(USE_JEMALLOC) # Most of the sanitizers require their instrumented allocation functions to # be fully functional. This is obviously the case for all the memory related # sanitizers (asan, lsan, msan) but not only. if(ENABLE_SANITIZERS) message(WARNING "Jemalloc is incompatible with the sanitizers and has been disabled.") else() find_package(Jemalloc 3.6.0 REQUIRED) link_libraries(Jemalloc::jemalloc) endif() endif() # Make sure that all the global compiler and linker flags are set BEFORE # including the libraries so they apply as needed. # libraries add_subdirectory(crypto) add_subdirectory(leveldb) add_subdirectory(secp256k1) add_subdirectory(univalue) # Find the git root, and returns the full path to the .git/logs/HEAD file if # it exists. function(find_git_head_logs_file RESULT) find_package(Git) if(GIT_FOUND) execute_process( COMMAND "${GIT_EXECUTABLE}" "rev-parse" "--show-toplevel" WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" OUTPUT_VARIABLE GIT_ROOT RESULT_VARIABLE GIT_RESULT OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET ) if(GIT_RESULT EQUAL 0) set(GIT_LOGS_DIR "${GIT_ROOT}/.git/logs") set(GIT_HEAD_LOGS_FILE "${GIT_LOGS_DIR}/HEAD") # If the .git/logs/HEAD does not exist, create it if(NOT EXISTS "${GIT_HEAD_LOGS_FILE}") file(MAKE_DIRECTORY "${GIT_LOGS_DIR}") file(TOUCH "${GIT_HEAD_LOGS_FILE}") endif() set(${RESULT} "${GIT_HEAD_LOGS_FILE}" PARENT_SCOPE) endif() endif() endfunction() find_git_head_logs_file(GIT_HEAD_LOGS_FILE) set(OBJ_DIR "${CMAKE_CURRENT_BINARY_DIR}/obj") file(MAKE_DIRECTORY "${OBJ_DIR}") set(BUILD_HEADER "${OBJ_DIR}/build.h") set(BUILD_HEADER_TMP "${BUILD_HEADER}.tmp") add_custom_command( DEPENDS "${GIT_HEAD_LOGS_FILE}" "${CMAKE_SOURCE_DIR}/share/genbuild.sh" OUTPUT "${BUILD_HEADER}" COMMAND "${CMAKE_SOURCE_DIR}/share/genbuild.sh" "${BUILD_HEADER_TMP}" "${CMAKE_SOURCE_DIR}" COMMAND ${CMAKE_COMMAND} -E copy_if_different "${BUILD_HEADER_TMP}" "${BUILD_HEADER}" COMMAND ${CMAKE_COMMAND} -E remove "${BUILD_HEADER_TMP}" ) # Because the Bitcoin ABc source code is disorganised, we # end up with a bunch of libraries without any apparent # cohesive structure. This is inherited from Bitcoin Core # and reflecting this. # TODO: Improve the structure once cmake is rocking. # Various completely unrelated features shared by all executables. add_library(util chainparamsbase.cpp clientversion.cpp compat/glibcxx_sanity.cpp compat/strnlen.cpp currencyunit.cpp fs.cpp interfaces/handler.cpp logging.cpp random.cpp randomenv.cpp rcu.cpp rpc/request.cpp blockdb.cpp support/cleanse.cpp support/lockedpool.cpp sync.cpp threadinterrupt.cpp uint256.cpp util/asmap.cpp util/bip32.cpp util/bytevectorhash.cpp util/error.cpp util/message.cpp util/moneystr.cpp util/settings.cpp util/spanparsing.cpp util/strencodings.cpp util/string.cpp util/system.cpp util/threadnames.cpp util/time.cpp util/url.cpp # obj/build.h "${BUILD_HEADER}" ) target_compile_definitions(util PUBLIC HAVE_CONFIG_H HAVE_BUILD_INFO) target_include_directories(util PUBLIC . # To access the config/ and obj/ directories ${CMAKE_CURRENT_BINARY_DIR} ) if(ENABLE_GLIBC_BACK_COMPAT) target_sources(util PRIVATE compat/glibc_compat.cpp) endif() # Target specific configs if(${CMAKE_SYSTEM_NAME} MATCHES "Windows") set(Boost_USE_STATIC_LIBS ON) set(Boost_USE_STATIC_RUNTIME ON) set(Boost_THREADAPI win32) find_package(SHLWAPI REQUIRED) target_link_libraries(util SHLWAPI::shlwapi) find_library(WS2_32_LIBRARY NAMES ws2_32) target_link_libraries(util ${WS2_32_LIBRARY}) target_compile_definitions(util PUBLIC BOOST_THREAD_USE_LIB) endif() target_link_libraries(util univalue crypto) macro(link_event TARGET) non_native_target_link_libraries(${TARGET} Event 2.0.22 ${ARGN}) endmacro() link_event(util event) macro(link_boost TARGET) non_native_target_link_libraries(${TARGET} Boost 1.59 ${ARGN}) endmacro() link_boost(util filesystem thread) # Make sure boost uses std::atomic (it doesn't before 1.63) target_compile_definitions(util PUBLIC BOOST_SP_USE_STD_ATOMIC BOOST_AC_USE_STD_ATOMIC) function(add_network_sources NETWORK_SOURCES) set(NETWORK_DIR abc) list(TRANSFORM ARGN PREPEND "networks/${NETWORK_DIR}/" OUTPUT_VARIABLE NETWORK_SOURCES ) set(NETWORK_SOURCES ${NETWORK_SOURCES} PARENT_SCOPE) endfunction() add_network_sources(NETWORK_SOURCES checkpoints.cpp network.cpp chainparamsconstants.cpp ) # More completely unrelated features shared by all executables. # Because nothing says this is different from util than "common" add_library(common amount.cpp base58.cpp bloom.cpp cashaddr.cpp cashaddrenc.cpp chainparams.cpp config.cpp consensus/merkle.cpp coins.cpp compressor.cpp eventloop.cpp feerate.cpp core_read.cpp core_write.cpp key.cpp key_io.cpp merkleblock.cpp net_permissions.cpp netaddress.cpp netbase.cpp outputtype.cpp policy/policy.cpp primitives/block.cpp protocol.cpp psbt.cpp rpc/rawtransaction_util.cpp rpc/util.cpp scheduler.cpp salteduint256hasher.cpp versionbitsinfo.cpp warnings.cpp ${NETWORK_SOURCES} ) target_link_libraries(common bitcoinconsensus util secp256k1 script) # script library add_library(script script/bitfield.cpp script/descriptor.cpp script/interpreter.cpp script/script.cpp script/script_error.cpp script/sigencoding.cpp script/sign.cpp script/signingprovider.cpp script/standard.cpp ) target_link_libraries(script common) # libbitcoinconsensus add_library(bitcoinconsensus arith_uint256.cpp hash.cpp primitives/transaction.cpp pubkey.cpp uint256.cpp util/strencodings.cpp consensus/tx_check.cpp ) target_link_libraries(bitcoinconsensus script) include(InstallationHelper) if(BUILD_LIBBITCOINCONSENSUS) target_compile_definitions(bitcoinconsensus PUBLIC BUILD_BITCOIN_INTERNAL HAVE_CONSENSUS_LIB ) install_shared_library(bitcoinconsensus script/bitcoinconsensus.cpp PUBLIC_HEADER script/bitcoinconsensus.h ) endif() # Bitcoin server facilities add_library(server addrdb.cpp addrman.cpp avalanche/avalanche.cpp avalanche/delegation.cpp avalanche/delegationbuilder.cpp avalanche/orphanproofpool.cpp avalanche/peermanager.cpp avalanche/processor.cpp avalanche/proof.cpp avalanche/proofid.cpp avalanche/proofbuilder.cpp banman.cpp blockencodings.cpp blockfilter.cpp blockindex.cpp chain.cpp checkpoints.cpp config.cpp consensus/activation.cpp consensus/tx_verify.cpp dbwrapper.cpp dnsseeds.cpp flatfile.cpp httprpc.cpp httpserver.cpp index/base.cpp index/blockfilterindex.cpp index/txindex.cpp init.cpp interfaces/chain.cpp interfaces/node.cpp invrequest.cpp miner.cpp minerfund.cpp net.cpp net_processing.cpp node/coin.cpp node/coinstats.cpp node/context.cpp node/psbt.cpp node/transaction.cpp node/ui_interface.cpp noui.cpp policy/fees.cpp policy/settings.cpp pow/aserti32d.cpp pow/daa.cpp pow/eda.cpp pow/grasberg.cpp pow/pow.cpp rest.cpp rpc/abc.cpp rpc/avalanche.cpp rpc/blockchain.cpp rpc/command.cpp rpc/mining.cpp rpc/misc.cpp rpc/net.cpp rpc/rawtransaction.cpp rpc/server.cpp script/scriptcache.cpp script/sigcache.cpp shutdown.cpp timedata.cpp torcontrol.cpp txdb.cpp txmempool.cpp validation.cpp validationinterface.cpp versionbits.cpp ) target_include_directories(server PRIVATE leveldb/helpers/memenv) target_link_libraries(server bitcoinconsensus leveldb memenv ) link_event(server event) if(NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows") link_event(server pthreads) endif() if(ENABLE_UPNP) find_package(MiniUPnPc 1.9 REQUIRED) target_link_libraries(server MiniUPnPc::miniupnpc) if(${CMAKE_SYSTEM_NAME} MATCHES "Windows") # TODO: check if we are really using a static library. Assume this is # the one from the depends for now since the native windows build is not # supported. target_compile_definitions(server PUBLIC -DSTATICLIB PUBLIC -DMINIUPNP_STATICLIB ) endif() endif() # Test suites. add_subdirectory(test) add_subdirectory(avalanche/test) add_subdirectory(pow/test) # Benchmark suite. add_subdirectory(bench) include(BinaryTest) include(WindowsVersionInfo) # Wallet if(BUILD_BITCOIN_WALLET) add_subdirectory(wallet) target_link_libraries(server wallet) # bitcoin-wallet add_executable(bitcoin-wallet bitcoin-wallet.cpp) generate_windows_version_info(bitcoin-wallet DESCRIPTION "CLI tool for ${PACKAGE_NAME} wallets" ) target_link_libraries(bitcoin-wallet wallet-tool common util) add_to_symbols_check(bitcoin-wallet) add_to_security_check(bitcoin-wallet) install_target(bitcoin-wallet) install_manpages(bitcoin-wallet) else() target_sources(server PRIVATE dummywallet.cpp) endif() # ZeroMQ if(BUILD_BITCOIN_ZMQ) add_subdirectory(zmq) target_link_libraries(server zmq) endif() # RPC client support add_library(rpcclient compat/stdin.cpp rpc/client.cpp ) target_link_libraries(rpcclient univalue util) # bitcoin-seeder if(BUILD_BITCOIN_SEEDER) add_subdirectory(seeder) endif() # bitcoin-cli if(BUILD_BITCOIN_CLI) add_executable(bitcoin-cli bitcoin-cli.cpp) generate_windows_version_info(bitcoin-cli DESCRIPTION "JSON-RPC client for ${PACKAGE_NAME}" ) target_link_libraries(bitcoin-cli common rpcclient) link_event(bitcoin-cli event) add_to_symbols_check(bitcoin-cli) add_to_security_check(bitcoin-cli) install_target(bitcoin-cli) install_manpages(bitcoin-cli) endif() # bitcoin-tx if(BUILD_BITCOIN_TX) add_executable(bitcoin-tx bitcoin-tx.cpp) generate_windows_version_info(bitcoin-tx DESCRIPTION "CLI Bitcoin transaction editor utility" ) target_link_libraries(bitcoin-tx bitcoinconsensus) add_to_symbols_check(bitcoin-tx) add_to_security_check(bitcoin-tx) install_target(bitcoin-tx) install_manpages(bitcoin-tx) endif() # bitcoind add_executable(bitcoind bitcoind.cpp) target_link_libraries(bitcoind server) generate_windows_version_info(bitcoind DESCRIPTION "Bitcoin node with a JSON-RPC server" ) add_to_symbols_check(bitcoind) add_to_security_check(bitcoind) install_target(bitcoind) install_manpages(bitcoind) # Bitcoin-qt if(BUILD_BITCOIN_QT) add_subdirectory(qt) endif() diff --git a/src/util/check.h b/src/util/check.h index 28045b994..5e7f1bd8c 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -1,64 +1,79 @@ // Copyright (c) 2019 The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef BITCOIN_UTIL_CHECK_H #define BITCOIN_UTIL_CHECK_H #if defined(HAVE_CONFIG_H) #include #endif #include #include class NonFatalCheckError : public std::runtime_error { using std::runtime_error::runtime_error; }; /** * Throw a NonFatalCheckError when the condition evaluates to false * * This should only be used * - where the condition is assumed to be true, not for error handling or * validating user input * - where a failure to fulfill the condition is recoverable and does not abort * the program * * For example in RPC code, where it is undesirable to crash the whole program, * this can be generally used to replace asserts or recoverable logic errors. A * NonFatalCheckError in RPC code is caught and passed as a string to the RPC * caller, which can then report the issue to the developers. */ #define CHECK_NONFATAL(condition) \ do { \ if (!(condition)) { \ throw NonFatalCheckError( \ strprintf("%s:%d (%s)\n" \ "Internal bug detected: '%s'\n" \ "You may report this issue here: %s\n", \ __FILE__, __LINE__, __func__, (#condition), \ PACKAGE_BUGREPORT)); \ } \ } while (false) #if defined(NDEBUG) #error "Cannot compile without assertions!" #endif -/** Helper for Assert(). TODO remove in C++14 and replace - * `decltype(get_pure_r_value(val))` with `T` (templated lambda) */ +/** Helper for Assert() */ template T get_pure_r_value(T &&val) { return std::forward(val); } /** Identity function. Abort if the value compares equal to zero */ #define Assert(val) \ - [&]() -> decltype(get_pure_r_value(val)) { \ + ([&]() -> decltype(get_pure_r_value(val)) { \ auto &&check = (val); \ assert(#val &&check); \ return std::forward(check); \ - }() + }()) + +/** + * Assume is the identity function. + * + * - Should be used to run non-fatal checks. In debug builds it behaves like + * Assert()/assert() to notify developers and testers about non-fatal errors. + * In production it doesn't warn or log anything. + * - For fatal errors, use Assert(). + * - For non-fatal errors in interactive sessions (e.g. RPC or command line + * interfaces), CHECK_NONFATAL() might be more appropriate. + */ +#ifdef ABORT_ON_FAILED_ASSUME +#define Assume(val) Assert(val) +#else +#define Assume(val) ((void)(val)) +#endif #endif // BITCOIN_UTIL_CHECK_H