Merge branch '1.14-branding'

1 files changed, 329 insertions, 18 deletions

diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index 30d365fb7..2cea300b8 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -1,17 +1,25 @@
-Coding
-====================
+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.
+gradually and you are encouraged to use the provided
+[clang-format-diff script](/contrib/devtools/README.md#clang-format-diffpy)
+to clean up the patch automatically before submitting a pull request.
+
+- 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 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.
+  - No space after function names; one space after `if`, `for` and `while`.
+  - 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
+    correctly indented on a new line.
+  - `++i` is preferred over `i++`.
 
 Block style example:
 ```c++
@@ -19,14 +27,18 @@ namespace foo
 {
 class Class
 {
-    bool Function(char* psz, int n)
+    bool Function(const std::string& s, int n)
     {
         // Comment summarising what this section of code does
-        for (int i = 0; i < n; i++) {
+        for (int i = 0; i < n; ++i) {
             // When something fails, return early
-            if (!Something())
-                return false;
+            if (!Something()) return false;
             ...
+            if (SomethingElse()) {
+                DoMore();
+            } else {
+                DoLess();
+            }
         }
 
         // Success return is usually at the end
@@ -57,7 +69,7 @@ As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this cas
 
 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()
@@ -71,6 +83,12 @@ To describe a member or variable use:
 int var; //!< Detailed description after the member
 ```
 
+or
+```cpp
+//! Description before the member
+int var;
+```
+
 Also OK:
 ```c++
 ///
@@ -102,7 +120,7 @@ CXXFLAGS="-g -ggdb -O0" or whatever debug flags you need.
 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 will turn
+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
@@ -172,15 +190,308 @@ Threads
 
 - Shutdown : Does an orderly shutdown of everything.
 
-Pull Request Terminology
+Ignoring IDE/editor files
+--------------------------
+
+In closed-source environments in which everyone uses the same IDE it is common
+to add temporary files it produces to the project-wide `.gitignore` file.
+
+However, in open source software such as Bitcoin Core, where everyone uses
+their own editors/IDE/tools, it is less common. Only you know what files your
+editor produces and this may change from version to version. The canonical way
+to do this is thus to create your local gitignore. Add this to `~/.gitconfig`:
+
+```
+[core]
+        excludesfile = /home/.../.gitignore_global
+```
+
+(alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global`
+on a terminal)
+
+Then put your favourite tool's temporary filenames in that file, e.g.
+```
+# NetBeans
+nbproject/
+```
+
+Another option is to create a per-repository excludes file `.git/info/exclude`.
+These are not committed but apply only to one repository.
+
+If a set of tools is used by the build system or scripts the repository (for
+example, lcov) it is perfectly acceptable to add its files to `.gitignore`
+and commit them.
+
+Development guidelines
+============================
+
+A few non-style-related recommendations for developers, as well as points to
+pay attention to for reviewers of Bitcoin Core code.
+
+General Bitcoin Core
+----------------------
+
+- New features should be exposed on RPC first, then can be made available in the GUI
+
+  - *Rationale*: RPC allows for better automatic testing. The test suite for
+    the GUI is very limited
+
+- Make sure pull requests pass Travis CI before merging
+
+  - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing
+     on the master branch. Otherwise all new pull requests will start failing the tests, resulting in
+     confusion and mayhem
+
+  - *Explanation*: If the test suite is to be updated for a change, this has to
+    be done first
+
+Wallet
+-------
+
+- Make sure that no crashes happen with run-time option `-disablewallet`.
+
+  - *Rationale*: In RPC code that conditionally uses the wallet (such as
+    `validateaddress`) it is easy to forget that global pointer `pwalletMain`
+    can be NULL. See `qa/rpc-tests/disablewallet.py` for functional tests
+    exercising the API with `-disablewallet`
+
+- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set
+
+  - *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB
+
+General C++
+-------------
+
+- Assertions should not have side-effects
+
+  - *Rationale*: Even though the source code is set to to refuse to compile
+    with assertions disabled, having side-effects in assertions is unexpected and
+    makes the code harder to understand
+
+- If you use the `.h`, you must link the `.cpp`
+
+  - *Rationale*: Include files define the interface for the code in implementation files. Including one but
+      not linking the other is confusing. Please avoid that. Moving functions from
+      the `.h` to the `.cpp` should not result in build errors
+
+- Use the RAII (Resource Acquisition Is Initialization) paradigm where possible. For example by using
+  `unique_ptr` 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 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
+
+- 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
 ------------------------
 
-Concept ACK - Agree with the idea and overall direction, but have neither reviewed nor tested the code changes.
+- 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`, `ParseUInt32`, `ParseUInt64`, `ParseDouble` from `utilstrencodings.h` for number parsing
+
+  - *Rationale*: These functions do overflow checking, and avoid pesky locale issues
+
+- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers
+
+  - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion
+
+Variable names
+--------------
+
+The shadowing warning (`-Wshadow`) is 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.
+
+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.
+
+
+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.
+
+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
+be sent upstream but bugfixes may also be prudent to PR against Bitcoin Core so that they can be integrated
+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.
+
+
+Git and GitHub tips
+---------------------
+
+- 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*.
 
-utACK (untested ACK) - Reviewed and agree with the code changes but haven't actually tested them.
+- 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.
 
-Tested ACK - Reviewed the code changes and have verified the functionality or bug fix.
+- When looking at other's pull requests, it may make sense to add the following section to your `.git/config`
+  file:
 
-ACK -  A loose ACK can be confusing. It's best to avoid them unless it's a documentation/comment only change in which case there is nothing to test/verify; therefore the tested/untested distinction is not there.
+        [remote "upstream-pull"]
+                fetch = +refs/pull/*:refs/remotes/upstream-pull/*
+                url = [email protected]:dogecoin/dogecoin.git
 
-NACK - Disagree with the code changes/concept. Should be accompanied by an explanation.
+  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.