v. 1.0.0.0
This commit is contained in:
NeoDashh
377
doc/developer-notes.md
Normal file
377
doc/developer-notes.md
Normal file
@@ -0,0 +1,377 @@
|
||||
Developer Notes
|
||||
===============
|
||||
|
||||
Various coding styles have been used during the history of the codebase,
|
||||
and the result is not very consistent. However, we're now trying to converge to
|
||||
a single style, so please use it in new code. Old code will be converted
|
||||
gradually.
|
||||
- Basic rules specified in src/.clang-format. Use a recent clang-format-3.5 to format automatically.
|
||||
- Braces on new lines for namespaces, classes, functions, methods.
|
||||
- Braces on the same line for everything else.
|
||||
- 4 space indentation (no tabs) for every block except namespaces.
|
||||
- No indentation for public/protected/private or for namespaces.
|
||||
- No extra spaces inside parenthesis; don't do ( this )
|
||||
- No space after function names; one space after if, for and while.
|
||||
|
||||
Block style example:
|
||||
```c++
|
||||
namespace foo
|
||||
{
|
||||
class Class
|
||||
{
|
||||
bool Function(char* psz, int n)
|
||||
{
|
||||
// Comment summarising what this section of code does
|
||||
for (int i = 0; i < n; i++) {
|
||||
// When something fails, return early
|
||||
if (!Something())
|
||||
return false;
|
||||
...
|
||||
}
|
||||
|
||||
// Success return is usually at the end
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Doxygen comments
|
||||
-----------------
|
||||
|
||||
To facilitate the generation of documentation, use doxygen-compatible comment blocks for functions, methods and fields.
|
||||
|
||||
For example, to describe a function use:
|
||||
```c++
|
||||
/**
|
||||
* ... text ...
|
||||
* @param[in] arg1 A description
|
||||
* @param[in] arg2 Another argument description
|
||||
* @pre Precondition for function...
|
||||
*/
|
||||
bool function(int arg1, const char *arg2)
|
||||
```
|
||||
A complete list of `@xxx` commands can be found at http://www.stack.nl/~dimitri/doxygen/manual/commands.html.
|
||||
As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this case), you don't
|
||||
*need* to provide any commands for a comment to be valid; just a description text is fine.
|
||||
|
||||
To describe a class use the same construct above the class definition:
|
||||
```c++
|
||||
/**
|
||||
* Alerts are for notifying old versions if they become too obsolete and
|
||||
* need to upgrade. The message is displayed in the status bar.
|
||||
* @see GetWarnings()
|
||||
*/
|
||||
class CAlert
|
||||
{
|
||||
```
|
||||
|
||||
To describe a member or variable use:
|
||||
```c++
|
||||
int var; //!< Detailed description after the member
|
||||
```
|
||||
|
||||
Also OK:
|
||||
```c++
|
||||
///
|
||||
/// ... text ...
|
||||
///
|
||||
bool function2(int arg1, const char *arg2)
|
||||
```
|
||||
|
||||
Not OK (used plenty in the current source, but not picked up):
|
||||
```c++
|
||||
//
|
||||
// ... text ...
|
||||
//
|
||||
```
|
||||
|
||||
A full list of comment syntaxes picked up by doxygen can be found at http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html,
|
||||
but if possible use one of the above styles.
|
||||
|
||||
Development tips and tricks
|
||||
---------------------------
|
||||
|
||||
**compiling for debugging**
|
||||
|
||||
Run configure with the --enable-debug option, then make. Or run configure with
|
||||
CXXFLAGS="-g -ggdb -O0" or whatever debug flags you need.
|
||||
|
||||
**debug.log**
|
||||
|
||||
If the code is behaving strangely, take a look in the debug.log file in the data directory;
|
||||
error and debugging messages are written there.
|
||||
|
||||
The -debug=... command-line option controls debugging; running with just -debug or -debug=1 will turn
|
||||
on all categories (and give you a very large debug.log file).
|
||||
|
||||
The Qt code routes qDebug() output to debug.log under category "qt": run with -debug=qt
|
||||
to see it.
|
||||
|
||||
**testnet and regtest modes**
|
||||
|
||||
Run with the -testnet option to run with "play bitcoins" on the test network, if you
|
||||
are testing multi-machine code that needs to operate across the internet.
|
||||
|
||||
If you are testing something that can run on one machine, run with the -regtest option.
|
||||
In regression test mode, blocks can be created on-demand; see qa/rpc-tests/ for tests
|
||||
that run in -regtest mode.
|
||||
|
||||
**DEBUG_LOCKORDER**
|
||||
|
||||
Neodash Core is a multithreaded application, and deadlocks or other multithreading bugs
|
||||
can be very difficult to track down. Compiling with -DDEBUG_LOCKORDER (configure
|
||||
CXXFLAGS="-DDEBUG_LOCKORDER -g") inserts run-time checks to keep track of which locks
|
||||
are held, and adds warnings to the debug.log file if inconsistencies are detected.
|
||||
|
||||
Locking/mutex usage notes
|
||||
-------------------------
|
||||
|
||||
The code is multi-threaded, and uses mutexes and the
|
||||
LOCK/TRY_LOCK macros to protect data structures.
|
||||
|
||||
Deadlocks due to inconsistent lock ordering (thread 1 locks cs_main
|
||||
and then cs_wallet, while thread 2 locks them in the opposite order:
|
||||
result, deadlock as each waits for the other to release its lock) are
|
||||
a problem. Compile with -DDEBUG_LOCKORDER to get lock order
|
||||
inconsistencies reported in the debug.log file.
|
||||
|
||||
Re-architecting the core code so there are better-defined interfaces
|
||||
between the various components is a goal, with any necessary locking
|
||||
done by the components (e.g. see the self-contained CKeyStore class
|
||||
and its cs_KeyStore lock for example).
|
||||
|
||||
Threads
|
||||
-------
|
||||
|
||||
- ThreadScriptCheck : Verifies block scripts.
|
||||
|
||||
- ThreadImport : Loads blocks from blk*.dat files or bootstrap.dat.
|
||||
|
||||
- StartNode : Starts other threads.
|
||||
|
||||
- ThreadDNSAddressSeed : Loads addresses of peers from the DNS.
|
||||
|
||||
- ThreadMapPort : Universal plug-and-play startup/shutdown
|
||||
|
||||
- ThreadSocketHandler : Sends/Receives data from peers on port 8333.
|
||||
|
||||
- ThreadOpenAddedConnections : Opens network connections to added nodes.
|
||||
|
||||
- ThreadOpenConnections : Initiates new connections to peers.
|
||||
|
||||
- ThreadMessageHandler : Higher-level message handling (sending and receiving).
|
||||
|
||||
- DumpAddresses : Dumps IP addresses of nodes to peers.dat.
|
||||
|
||||
- ThreadFlushWalletDB : Close the wallet.dat file if it hasn't been used in 500ms.
|
||||
|
||||
- ThreadRPCServer : Remote procedure call handler, listens on port 9998 for connections and services them.
|
||||
|
||||
- BitcoinMiner : Generates bitcoins (if wallet is enabled).
|
||||
|
||||
- ThreadCheckDarkSendPool : Runs masternode list and sync data update loops
|
||||
|
||||
- Shutdown : Does an orderly shutdown of everything.
|
||||
|
||||
Ignoring IDE/editor files
|
||||
--------------------------
|
||||
|
||||
In closed-source environments in which everyone uses the same IDE it is common
|
||||
to add temporary files it produces to the project-wide `.gitignore` file.
|
||||
|
||||
However, in open source software such as Neodash Core, where everyone uses
|
||||
their own editors/IDE/tools, it is less common. Only you know what files your
|
||||
editor produces and this may change from version to version. The canonical way
|
||||
to do this is thus to create your local gitignore. Add this to `~/.gitconfig`:
|
||||
|
||||
```
|
||||
[core]
|
||||
excludesfile = /home/.../.gitignore_global
|
||||
```
|
||||
|
||||
(alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global`
|
||||
on a terminal)
|
||||
|
||||
Then put your favourite tool's temporary filenames in that file, e.g.
|
||||
```
|
||||
# NetBeans
|
||||
nbproject/
|
||||
```
|
||||
|
||||
Another option is to create a per-repository excludes file `.git/info/exclude`.
|
||||
These are not committed but apply only to one repository.
|
||||
|
||||
If a set of tools is used by the build system or scripts the repository (for
|
||||
example, lcov) it is perfectly acceptable to add its files to `.gitignore`
|
||||
and commit them.
|
||||
|
||||
Development guidelines
|
||||
============================
|
||||
|
||||
A few non-style-related recommendations for developers, as well as points to
|
||||
pay attention to for reviewers of Neodash Core code.
|
||||
|
||||
General Neodash Core
|
||||
----------------------
|
||||
|
||||
- New features should be exposed on RPC first, then can be made available in the GUI
|
||||
|
||||
- *Rationale*: RPC allows for better automatic testing. The test suite for
|
||||
the GUI is very limited
|
||||
|
||||
- Make sure pull requests pass Travis CI before merging
|
||||
|
||||
- *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing
|
||||
on the master branch. Otherwise all new pull requests will start failing the tests, resulting in
|
||||
confusion and mayhem
|
||||
|
||||
- *Explanation*: If the test suite is to be updated for a change, this has to
|
||||
be done first
|
||||
|
||||
Wallet
|
||||
-------
|
||||
|
||||
- Make sure that no crashes happen with run-time option `-disablewallet`.
|
||||
|
||||
- *Rationale*: In RPC code that conditionally uses the wallet (such as
|
||||
`validateaddress`) it is easy to forget that global pointer `pwalletMain`
|
||||
can be NULL. See `qa/rpc-tests/disablewallet.py` for functional tests
|
||||
exercising the API with `-disablewallet`
|
||||
|
||||
- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set
|
||||
|
||||
- *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB
|
||||
|
||||
General C++
|
||||
-------------
|
||||
|
||||
- Assertions should not have side-effects
|
||||
|
||||
- *Rationale*: Even though the source code is set to to refuse to compile
|
||||
with assertions disabled, having side-effects in assertions is unexpected and
|
||||
makes the code harder to understand
|
||||
|
||||
- If you use the `.h`, you must link the `.cpp`
|
||||
|
||||
- *Rationale*: Include files define the interface for the code in implementation files. Including one but
|
||||
not linking the other is confusing. Please avoid that. Moving functions from
|
||||
the `.h` to the `.cpp` should not result in build errors
|
||||
|
||||
- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example by using
|
||||
`scoped_pointer` for allocations in a function.
|
||||
|
||||
- *Rationale*: This avoids memory and resource leaks, and ensures exception safety
|
||||
|
||||
C++ data structures
|
||||
--------------------
|
||||
|
||||
- Never use the `std::map []` syntax when reading from a map, but instead use `.find()`
|
||||
|
||||
- *Rationale*: `[]` does an insert (of the default element) if the item doesn't
|
||||
exist in the map yet. This has resulted in memory leaks in the past, as well as
|
||||
race conditions (expecting read-read behavior). Using `[]` is fine for *writing* to a map
|
||||
|
||||
- Do not compare an iterator from one data structure with an iterator of
|
||||
another data structure (even if of the same type)
|
||||
|
||||
- *Rationale*: Behavior is undefined. In C++ parlor this means "may reformat
|
||||
the universe", in practice this has resulted in at least one hard-to-debug crash bug
|
||||
|
||||
- Watch out for vector out-of-bounds exceptions. `&vch[0]` is illegal for an
|
||||
empty vector, `&vch[vch.size()]` is always illegal. Use `begin_ptr(vch)` and
|
||||
`end_ptr(vch)` to get the begin and end pointer instead (defined in
|
||||
`serialize.h`)
|
||||
|
||||
- Vector bounds checking is only enabled in debug mode. Do not rely on it
|
||||
|
||||
- Make sure that constructors initialize all fields. If this is skipped for a
|
||||
good reason (i.e., optimization on the critical path), add an explicit
|
||||
comment about this
|
||||
|
||||
- *Rationale*: Ensure determinism by avoiding accidental use of uninitialized
|
||||
values. Also, static analyzers balk about this.
|
||||
|
||||
- Use explicitly signed or unsigned `char`s, or even better `uint8_t` and
|
||||
`int8_t`. Do not use bare `char` unless it is to pass to a third-party API.
|
||||
This type can be signed or unsigned depending on the architecture, which can
|
||||
lead to interoperability problems or dangerous conditions such as
|
||||
out-of-bounds array accesses
|
||||
|
||||
- Prefer explicit constructions over implicit ones that rely on 'magical' C++ behavior
|
||||
|
||||
- *Rationale*: Easier to understand what is happening, thus easier to spot mistakes, even for those
|
||||
that are not language lawyers
|
||||
|
||||
Strings and formatting
|
||||
------------------------
|
||||
|
||||
- Be careful of `LogPrint` versus `LogPrintf`. `LogPrint` takes a `category` argument, `LogPrintf` does not.
|
||||
|
||||
- *Rationale*: Confusion of these can result in runtime exceptions due to
|
||||
formatting mismatch, and it is easy to get wrong because of subtly similar naming
|
||||
|
||||
- Use `std::string`, avoid C string manipulation functions
|
||||
|
||||
- *Rationale*: C++ string handling is marginally safer, less scope for
|
||||
buffer overflows and surprises with `\0` characters. Also some C string manipulations
|
||||
tend to act differently depending on platform, or even the user locale
|
||||
|
||||
- Use `ParseInt32`, `ParseInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing
|
||||
|
||||
- *Rationale*: These functions do overflow checking, and avoid pesky locale issues
|
||||
|
||||
- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers
|
||||
|
||||
- *Rationale*: Neodash Core uses tinyformat, which is type safe. Leave them out to avoid confusion
|
||||
|
||||
Threads and synchronization
|
||||
----------------------------
|
||||
|
||||
- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
|
||||
deadlocks are introduced. As of 0.12, this is defined by default when
|
||||
configuring with `--enable-debug`
|
||||
|
||||
- When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of
|
||||
the current scope, so surround the statement and the code that needs the lock
|
||||
with braces
|
||||
|
||||
OK:
|
||||
|
||||
```c++
|
||||
{
|
||||
TRY_LOCK(cs_vNodes, lockNodes);
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
Wrong:
|
||||
|
||||
```c++
|
||||
TRY_LOCK(cs_vNodes, lockNodes);
|
||||
{
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
Source code organization
|
||||
--------------------------
|
||||
|
||||
- Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or
|
||||
when performance due to inlining is critical
|
||||
|
||||
- *Rationale*: Shorter and simpler header files are easier to read, and reduce compile time
|
||||
|
||||
- Don't import anything into the global namespace (`using namespace ...`). Use
|
||||
fully specified types such as `std::string`.
|
||||
|
||||
- *Rationale*: Avoids symbol conflicts
|
||||
|
||||
GUI
|
||||
-----
|
||||
|
||||
- Do not display or manipulate dialogs in model code (classes `*Model`)
|
||||
|
||||
- *Rationale*: Model classes pass through events and data from the core, they
|
||||
should not interact with the user. That's where View classes come in. The converse also
|
||||
holds: try to not directly access core data structures from Views.
|
||||
Reference in New Issue
Block a user