2015-09-26 18:25:54 +02:00
|
|
|
Developer Notes
|
|
|
|
===============
|
2013-05-20 06:30:00 +02:00
|
|
|
|
2014-07-09 18:17:23 +02:00
|
|
|
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
|
2017-05-31 16:59:33 +02:00
|
|
|
a single style, which is specified below. When writing patches, favor the new
|
|
|
|
style over attempting to mimick the surrounding style, except for move-only
|
|
|
|
commits.
|
|
|
|
|
|
|
|
Do not submit patches solely to modify the style of existing code.
|
2017-01-30 13:49:34 +01:00
|
|
|
|
2017-05-31 16:59:33 +02:00
|
|
|
- **Indentation and whitespace rules** as specified in
|
|
|
|
[src/.clang-format](/src/.clang-format). You can use the provided
|
|
|
|
[clang-format-diff script](/contrib/devtools/README.md#clang-format-diffpy)
|
|
|
|
tool to clean up patches automatically before submission.
|
2014-07-09 18:17:23 +02:00
|
|
|
- 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.
|
2017-01-11 13:52:06 +01:00
|
|
|
- No indentation for `public`/`protected`/`private` or for `namespace`.
|
2014-07-09 18:17:23 +02:00
|
|
|
- No extra spaces inside parenthesis; don't do ( this )
|
2017-01-11 13:52:06 +01:00
|
|
|
- No space after function names; one space after `if`, `for` and `while`.
|
2017-05-31 16:59:33 +02:00
|
|
|
- If an `if` only has a single-statement `then`-clause, it can appear
|
|
|
|
on the same line as the `if`, without braces. In every other case,
|
|
|
|
braces are required, and the `then` and `else` clauses must appear
|
2017-01-11 13:52:06 +01:00
|
|
|
correctly indented on a new line.
|
2017-05-31 16:59:33 +02:00
|
|
|
|
|
|
|
- **Symbol naming conventions**. These are preferred in new code, but are not
|
|
|
|
required when doing so would need changes to significant pieces of existing
|
|
|
|
code.
|
|
|
|
- Variable and namespace names are all lowercase, and may use `_` to
|
|
|
|
separate words.
|
|
|
|
- Class member variables have a `m_` prefix.
|
|
|
|
- Global variables have a `g_` prefix.
|
|
|
|
- Constant names are all uppercase, and use `_` to separate words.
|
|
|
|
- Class names, function names and method names are CamelCase. Do not prefix
|
|
|
|
class names with `C`.
|
|
|
|
|
|
|
|
- **Miscellaneous**
|
2016-08-26 15:54:25 +02:00
|
|
|
- `++i` is preferred over `i++`.
|
2017-12-14 01:33:08 +01:00
|
|
|
- Align pointers and references to the left i.e. use `type& var` and not `type &var`.
|
2014-06-26 11:49:51 +02:00
|
|
|
|
2014-07-09 18:17:23 +02:00
|
|
|
Block style example:
|
|
|
|
```c++
|
2017-05-31 16:59:33 +02:00
|
|
|
int g_count = 0;
|
|
|
|
|
2014-07-09 18:17:23 +02:00
|
|
|
namespace foo
|
|
|
|
{
|
|
|
|
class Class
|
|
|
|
{
|
2017-05-31 16:59:33 +02:00
|
|
|
std::string m_name;
|
|
|
|
|
|
|
|
public:
|
2017-01-11 13:52:06 +01:00
|
|
|
bool Function(const std::string& s, int n)
|
2014-07-09 18:17:23 +02:00
|
|
|
{
|
|
|
|
// Comment summarising what this section of code does
|
2016-08-26 15:54:25 +02:00
|
|
|
for (int i = 0; i < n; ++i) {
|
2017-05-31 16:59:33 +02:00
|
|
|
int total_sum = 0;
|
2014-07-09 18:17:23 +02:00
|
|
|
// When something fails, return early
|
2017-01-11 13:52:06 +01:00
|
|
|
if (!Something()) return false;
|
2014-07-09 18:17:23 +02:00
|
|
|
...
|
2017-05-31 16:59:33 +02:00
|
|
|
if (SomethingElse(i)) {
|
|
|
|
total_sum += ComputeSomething(g_count);
|
2017-01-11 13:52:06 +01:00
|
|
|
} else {
|
2017-05-31 16:59:33 +02:00
|
|
|
DoSomething(m_name, total_sum);
|
2017-01-11 13:52:06 +01:00
|
|
|
}
|
2014-07-09 18:17:23 +02:00
|
|
|
}
|
2013-05-20 06:30:00 +02:00
|
|
|
|
2014-07-09 18:17:23 +02:00
|
|
|
// Success return is usually at the end
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
}
|
2017-06-26 13:37:42 +02:00
|
|
|
} // namespace foo
|
2014-06-26 11:49:51 +02:00
|
|
|
```
|
2013-05-20 06:30:00 +02:00
|
|
|
|
2014-04-07 08:39:31 +02:00
|
|
|
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
|
2015-04-28 16:48:28 +02:00
|
|
|
*need* to provide any commands for a comment to be valid; just a description text is fine.
|
2014-04-07 08:39:31 +02:00
|
|
|
|
|
|
|
To describe a class use the same construct above the class definition:
|
|
|
|
```c++
|
2015-10-17 12:10:45 +02:00
|
|
|
/**
|
2014-04-07 08:39:31 +02:00
|
|
|
* 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
|
|
|
|
```
|
|
|
|
|
2016-04-05 17:49:42 +02:00
|
|
|
or
|
|
|
|
```cpp
|
|
|
|
//! Description before the member
|
|
|
|
int var;
|
|
|
|
```
|
|
|
|
|
2014-04-07 08:39:31 +02:00
|
|
|
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.
|
|
|
|
|
2014-12-13 05:35:39 +01:00
|
|
|
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.
|
|
|
|
|
2015-05-20 06:29:59 +02:00
|
|
|
The -debug=... command-line option controls debugging; running with just -debug or -debug=1 will turn
|
2014-12-13 05:35:39 +01:00
|
|
|
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**
|
|
|
|
|
2017-02-19 22:02:33 +01:00
|
|
|
Run with the -testnet option to run with "play coins" on the test network, if you
|
2014-12-13 05:35:39 +01:00
|
|
|
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.
|
2019-05-19 22:20:34 +02:00
|
|
|
In regression test mode, blocks can be created on-demand; see test/functional/ for tests
|
2014-12-13 05:35:39 +01:00
|
|
|
that run in -regtest mode.
|
|
|
|
|
|
|
|
**DEBUG_LOCKORDER**
|
|
|
|
|
2016-03-06 16:26:01 +01:00
|
|
|
Dash Core is a multithreaded application, and deadlocks or other multithreading bugs
|
2014-12-13 05:35:39 +01:00
|
|
|
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.
|
|
|
|
|
2013-05-20 06:30:00 +02:00
|
|
|
Locking/mutex usage notes
|
2014-04-07 08:39:31 +02:00
|
|
|
-------------------------
|
2013-05-20 06:30:00 +02:00
|
|
|
|
|
|
|
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
|
2014-04-07 08:39:31 +02:00
|
|
|
-------
|
2013-05-20 06:30:00 +02:00
|
|
|
|
2014-03-05 21:23:39 +01:00
|
|
|
- ThreadScriptCheck : Verifies block scripts.
|
|
|
|
|
|
|
|
- ThreadImport : Loads blocks from blk*.dat files or bootstrap.dat.
|
|
|
|
|
2013-05-20 06:30:00 +02:00
|
|
|
- StartNode : Starts other threads.
|
|
|
|
|
2014-03-05 21:23:39 +01:00
|
|
|
- ThreadDNSAddressSeed : Loads addresses of peers from the DNS.
|
|
|
|
|
|
|
|
- ThreadMapPort : Universal plug-and-play startup/shutdown
|
2013-05-20 06:30:00 +02:00
|
|
|
|
2017-02-19 22:02:33 +01:00
|
|
|
- ThreadSocketHandler : Sends/Receives data from peers on port 9999.
|
2014-03-05 21:23:39 +01:00
|
|
|
|
|
|
|
- ThreadOpenAddedConnections : Opens network connections to added nodes.
|
|
|
|
|
2013-05-20 06:30:00 +02:00
|
|
|
- ThreadOpenConnections : Initiates new connections to peers.
|
|
|
|
|
2019-01-22 14:33:20 +01:00
|
|
|
- ThreadOpenMasternodeConnections : Opens network connections to masternodes.
|
|
|
|
|
2014-03-05 21:23:39 +01:00
|
|
|
- ThreadMessageHandler : Higher-level message handling (sending and receiving).
|
|
|
|
|
|
|
|
- DumpAddresses : Dumps IP addresses of nodes to peers.dat.
|
2013-05-20 06:30:00 +02:00
|
|
|
|
|
|
|
- ThreadFlushWalletDB : Close the wallet.dat file if it hasn't been used in 500ms.
|
2014-03-05 21:23:39 +01:00
|
|
|
|
2016-03-06 16:26:01 +01:00
|
|
|
- ThreadRPCServer : Remote procedure call handler, listens on port 9998 for connections and services them.
|
2014-03-05 21:23:39 +01:00
|
|
|
|
|
|
|
- Shutdown : Does an orderly shutdown of everything.
|
2015-10-23 12:32:40 +02:00
|
|
|
|
2019-08-28 09:54:19 +02:00
|
|
|
- CSigSharesManager::WorkThreadMain : Processes pending BLS signature shares.
|
|
|
|
|
|
|
|
- CInstantSendManager::WorkThreadMain : Processes pending InstantSend locks.
|
|
|
|
|
|
|
|
Thread pools
|
|
|
|
------------
|
|
|
|
|
|
|
|
- CBLSWorker : A highly parallelized worker/helper for BLS/DKG calculations.
|
|
|
|
|
|
|
|
- CDKGSessionManager : A thread pool for processing LLMQ messages.
|
|
|
|
|
2015-10-23 12:32:40 +02:00
|
|
|
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.
|
|
|
|
|
2016-03-06 16:26:01 +01:00
|
|
|
However, in open source software such as Dash Core, where everyone uses
|
2015-10-23 12:32:40 +02:00
|
|
|
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.
|
|
|
|
|
2015-11-13 11:49:12 +01:00
|
|
|
Development guidelines
|
|
|
|
============================
|
|
|
|
|
|
|
|
A few non-style-related recommendations for developers, as well as points to
|
2016-03-06 16:26:01 +01:00
|
|
|
pay attention to for reviewers of Dash Core code.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
2016-03-06 16:26:01 +01:00
|
|
|
General Dash Core
|
2015-11-13 11:49:12 +01:00
|
|
|
----------------------
|
|
|
|
|
|
|
|
- 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
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- Make sure pull requests pass Travis CI before merging
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- *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
|
2016-08-26 15:54:25 +02:00
|
|
|
|
2015-11-13 11:49:12 +01:00
|
|
|
- *Explanation*: If the test suite is to be updated for a change, this has to
|
2016-08-26 15:54:25 +02:00
|
|
|
be done first
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
Wallet
|
|
|
|
-------
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- Make sure that no crashes happen with run-time option `-disablewallet`.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- *Rationale*: In RPC code that conditionally uses the wallet (such as
|
2015-11-13 11:49:12 +01:00
|
|
|
`validateaddress`) it is easy to forget that global pointer `pwalletMain`
|
2019-05-19 22:20:34 +02:00
|
|
|
can be NULL. See `test/functional/disablewallet.py` for functional tests
|
2015-11-13 11:49:12 +01:00
|
|
|
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
|
|
|
|
|
2017-06-22 20:36:16 +02:00
|
|
|
- *Rationale*: Even though the source code is set to refuse to compile
|
2015-11-13 11:49:12 +01:00
|
|
|
with assertions disabled, having side-effects in assertions is unexpected and
|
|
|
|
makes the code harder to understand
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- If you use the `.h`, you must link the `.cpp`
|
2015-11-13 11:49:12 +01:00
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- *Rationale*: Include files define the interface for the code in implementation files. Including one but
|
2015-11-13 11:49:12 +01:00
|
|
|
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
|
2016-06-10 11:29:17 +02:00
|
|
|
`unique_ptr` for allocations in a function.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- *Rationale*: This avoids memory and resource leaks, and ensures exception safety
|
|
|
|
|
|
|
|
C++ data structures
|
|
|
|
--------------------
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- Never use the `std::map []` syntax when reading from a map, but instead use `.find()`
|
2015-11-13 11:49:12 +01:00
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- *Rationale*: `[]` does an insert (of the default element) if the item doesn't
|
2015-11-13 11:49:12 +01:00
|
|
|
exist in the map yet. This has resulted in memory leaks in the past, as well as
|
2015-11-23 21:11:53 +01:00
|
|
|
race conditions (expecting read-read behavior). Using `[]` is fine for *writing* to a map
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- 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
|
|
|
|
|
2016-06-10 11:29:17 +02:00
|
|
|
- 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.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- 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
|
|
|
|
------------------------
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- Be careful of `LogPrint` versus `LogPrintf`. `LogPrint` takes a `category` argument, `LogPrintf` does not.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- *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
|
|
|
|
|
2015-11-23 21:11:53 +01:00
|
|
|
- Use `std::string`, avoid C string manipulation functions
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- *Rationale*: C++ string handling is marginally safer, less scope for
|
2015-11-23 21:11:53 +01:00
|
|
|
buffer overflows and surprises with `\0` characters. Also some C string manipulations
|
2015-11-13 11:49:12 +01:00
|
|
|
tend to act differently depending on platform, or even the user locale
|
|
|
|
|
2016-06-09 07:37:01 +02:00
|
|
|
- Use `ParseInt32`, `ParseInt64`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- *Rationale*: These functions do overflow checking, and avoid pesky locale issues
|
|
|
|
|
|
|
|
- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers
|
|
|
|
|
2016-03-06 16:26:01 +01:00
|
|
|
- *Rationale*: Dash Core uses tinyformat, which is type safe. Leave them out to avoid confusion
|
2015-11-13 11:49:12 +01:00
|
|
|
|
2016-11-09 14:12:07 +01:00
|
|
|
Variable names
|
|
|
|
--------------
|
|
|
|
|
2017-05-11 07:17:10 +02:00
|
|
|
Although the shadowing warning (`-Wshadow`) is not enabled by default (it prevents issues rising
|
|
|
|
from using a different variable with the same name),
|
|
|
|
please name variables so that their names do not shadow variables defined in the source code.
|
2016-11-09 14:12:07 +01:00
|
|
|
|
|
|
|
E.g. in member initializers, prepend `_` to the argument name shadowing the
|
|
|
|
member name:
|
|
|
|
|
|
|
|
```c++
|
|
|
|
class AddressBookPage
|
|
|
|
{
|
|
|
|
Mode mode;
|
|
|
|
}
|
|
|
|
|
|
|
|
AddressBookPage::AddressBookPage(Mode _mode) :
|
|
|
|
mode(_mode)
|
|
|
|
...
|
|
|
|
```
|
|
|
|
|
|
|
|
When using nested cycles, do not name the inner cycle variable the same as in
|
|
|
|
upper cycle etc.
|
|
|
|
|
|
|
|
|
2015-11-13 11:49:12 +01:00
|
|
|
Threads and synchronization
|
|
|
|
----------------------------
|
|
|
|
|
|
|
|
- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
|
2016-09-12 09:40:23 +02:00
|
|
|
deadlocks are introduced.
|
2015-11-13 11:49:12 +01:00
|
|
|
|
|
|
|
- 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
|
|
|
|
|
2017-06-13 19:09:59 +02:00
|
|
|
- Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other
|
|
|
|
definitions from, even if those headers are already included indirectly through other headers. One exception
|
|
|
|
is that a `.cpp` file does not need to re-include the includes already included in its corresponding `.h` file.
|
|
|
|
|
|
|
|
- *Rationale*: Excluding headers because they are already indirectly included results in compilation
|
|
|
|
failures when those indirect dependencies change. Furthermore, it obscures what the real code
|
|
|
|
dependencies are.
|
|
|
|
|
2015-11-13 11:49:12 +01:00
|
|
|
- Don't import anything into the global namespace (`using namespace ...`). Use
|
|
|
|
fully specified types such as `std::string`.
|
|
|
|
|
|
|
|
- *Rationale*: Avoids symbol conflicts
|
|
|
|
|
2017-06-26 13:37:42 +02:00
|
|
|
- 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
|
|
|
|
|
2015-11-13 11:49:12 +01:00
|
|
|
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.
|
2016-06-09 16:42:16 +02:00
|
|
|
|
2016-12-02 16:20:17 +01:00
|
|
|
Subtrees
|
|
|
|
----------
|
|
|
|
|
|
|
|
Several parts of the repository are subtrees of software maintained elsewhere.
|
|
|
|
|
|
|
|
Some of these are maintained by active developers of Bitcoin Core, in which case changes should probably go
|
|
|
|
directly upstream without being PRed directly against the project. They will be merged back in the next
|
|
|
|
subtree merge.
|
|
|
|
|
|
|
|
Others are external projects without a tight relationship with our project. Changes to these should also
|
2018-01-16 13:13:39 +01:00
|
|
|
be sent upstream but bugfixes may also be prudent to PR against Dash Core so that they can be integrated
|
2016-12-02 16:20:17 +01:00
|
|
|
quickly. Cosmetic changes should be purely taken upstream.
|
|
|
|
|
|
|
|
There is a tool in contrib/devtools/git-subtree-check.sh to check a subtree directory for consistency with
|
|
|
|
its upstream repository.
|
|
|
|
|
|
|
|
Current subtrees include:
|
|
|
|
|
|
|
|
- src/leveldb
|
|
|
|
- Upstream at https://github.com/google/leveldb ; Maintained by Google, but open important PRs to Core to avoid delay
|
|
|
|
|
|
|
|
- src/libsecp256k1
|
|
|
|
- Upstream at https://github.com/bitcoin-core/secp256k1/ ; actively maintaned by Core contributors.
|
|
|
|
|
|
|
|
- src/crypto/ctaes
|
|
|
|
- Upstream at https://github.com/bitcoin-core/ctaes ; actively maintained by Core contributors.
|
|
|
|
|
|
|
|
- src/univalue
|
|
|
|
- Upstream at https://github.com/jgarzik/univalue ; report important PRs to Core to avoid delay.
|
|
|
|
|
|
|
|
|
2017-02-16 15:05:46 +01:00
|
|
|
Git and GitHub tips
|
2016-06-09 16:42:16 +02:00
|
|
|
---------------------
|
|
|
|
|
|
|
|
- For resolving merge/rebase conflicts, it can be useful to enable diff3 style using
|
|
|
|
`git config merge.conflictstyle diff3`. Instead of
|
|
|
|
|
|
|
|
<<<
|
|
|
|
yours
|
|
|
|
===
|
|
|
|
theirs
|
|
|
|
>>>
|
|
|
|
|
|
|
|
you will see
|
|
|
|
|
|
|
|
<<<
|
|
|
|
yours
|
|
|
|
|||
|
|
|
|
original
|
|
|
|
===
|
|
|
|
theirs
|
|
|
|
>>>
|
|
|
|
|
|
|
|
This may make it much clearer what caused the conflict. In this style, you can often just look
|
|
|
|
at what changed between *original* and *theirs*, and mechanically apply that to *yours* (or the other way around).
|
|
|
|
|
|
|
|
- When reviewing patches which change indentation in C++ files, use `git diff -w` and `git show -w`. This makes
|
|
|
|
the diff algorithm ignore whitespace changes. This feature is also available on github.com, by adding `?w=1`
|
|
|
|
at the end of any URL which shows a diff.
|
|
|
|
|
|
|
|
- When reviewing patches that change symbol names in many places, use `git diff --word-diff`. This will instead
|
|
|
|
of showing the patch as deleted/added *lines*, show deleted/added *words*.
|
|
|
|
|
|
|
|
- When reviewing patches that move code around, try using
|
|
|
|
`git diff --patience commit~:old/file.cpp commit:new/file/name.cpp`, and ignoring everything except the
|
|
|
|
moved body of code which should show up as neither `+` or `-` lines. In case it was not a pure move, this may
|
|
|
|
even work when combined with the `-w` or `--word-diff` options described above.
|
|
|
|
|
|
|
|
- When looking at other's pull requests, it may make sense to add the following section to your `.git/config`
|
|
|
|
file:
|
|
|
|
|
|
|
|
[remote "upstream-pull"]
|
|
|
|
fetch = +refs/pull/*:refs/remotes/upstream-pull/*
|
|
|
|
url = git@github.com:bitcoin/bitcoin.git
|
|
|
|
|
|
|
|
This will add an `upstream-pull` remote to your git repository, which can be fetched using `git fetch --all`
|
|
|
|
or `git fetch upstream-pull`. Afterwards, you can use `upstream-pull/NUMBER/head` in arguments to `git show`,
|
|
|
|
`git checkout` and anywhere a commit id would be acceptable to see the changes from pull request NUMBER.
|
2017-05-02 07:52:34 +02:00
|
|
|
|
|
|
|
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.
|
|
|
|
|
2017-08-08 11:27:15 +02:00
|
|
|
- *Exception*: AmountFromValue can parse amounts as string. This was introduced because many JSON
|
2017-05-02 07:52:34 +02:00
|
|
|
parsers and formatters hard-code handling decimal numbers as floating point
|
|
|
|
values, resulting in potential loss of precision. This is unacceptable for
|
2017-08-08 11:27:15 +02:00
|
|
|
monetary values. **Always** use `AmountFromValue` and `ValueFromAmount` when
|
2017-05-02 07:52:34 +02:00
|
|
|
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.
|
|
|
|
|
|
|
|
- *Rationale*: Avoids surprises when switching to name-based arguments. Missing name-based arguments
|
|
|
|
are passed as 'null'.
|
|
|
|
|
|
|
|
- *Exception*: Many legacy exceptions to this exist, one of the worst ones is
|
|
|
|
`getbalance` which follows a completely different code path based on the
|
|
|
|
number of arguments. We are still in the process of cleaning these up. Do not introduce
|
|
|
|
new ones.
|
|
|
|
|
|
|
|
- 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 `getinfo` and `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.
|