Merge pull request #4217 from PastaPastaPasta/backports-0.18-triv-pr2

backport: 'trivial' pr2

configure.ac

@@ -95,8 +95,8 @@ AC_PATH_TOOL(RANLIB, ranlib)
 AC_PATH_TOOL(STRIP, strip)
 AC_PATH_TOOL(GCOV, gcov)
 AC_PATH_PROG(LCOV, lcov)
-dnl Python 3.x is supported from 3.4 on (see https://github.com/bitcoin/bitcoin/issues/7893)
+dnl Python 3.4 is specified in .python-version and should be used if available, see doc/dependencies.md
-AC_PATH_PROGS([PYTHON], [python3.7 python3.6 python3.5 python3.4 python3 python])
+AC_PATH_PROGS([PYTHON], [python3.4 python3.5 python3.6 python3.7 python3 python])
 AC_PATH_PROG(GENHTML, genhtml)
 AC_PATH_PROG([GIT], [git])
 AC_PATH_PROG(CCACHE,ccache)

doc/README.md

@@ -47,6 +47,7 @@ Development
 The Dash Core repo's [root README](/README.md) contains relevant information on the development process and automated testing.
 
 - [Developer Notes](developer-notes.md)
+- [Productivity Notes](productivity.md)
 - [Release Notes](release-notes.md)
 - [Release Process](release-process.md)
 - Source Code Documentation ***TODO***

doc/developer-notes.md

@@ -33,6 +33,7 @@ Developer Notes
     - [Subtrees](#subtrees)
     - [Git and GitHub tips](#git-and-github-tips)
     - [Scripted diffs](#scripted-diffs)
+    - [Release notes](#release-notes)
     - [RPC interface guidelines](#rpc-interface-guidelines)
 
 <!-- markdown-toc end -->
@@ -790,54 +791,6 @@ would be to revert the upstream fix before applying the updates to Bitcoin'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
----------------------
-
-- 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.
-
 Scripted diffs
 --------------
 
@@ -858,6 +811,21 @@ The scripted-diff is verified by the tool `test/lint/commit-script-check.sh`
 
 Commit [`bb81e173`](https://github.com/bitcoin/bitcoin/commit/bb81e173) is an example of a scripted-diff.
 
+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 a PR-specific release note file at
+`/doc/release-notes-<PR number>.md` to avoid conflicts between multiple PRs.
+All `release-notes*` files are merged into a single
+[/doc/release-notes.md](/doc/release-notes.md) file prior to the release.
+
 RPC interface guidelines
 --------------------------
 

doc/productivity.md

@@ -0,0 +1,161 @@
+Productivity Notes
+==================
+
+Table of Contents
+-----------------
+
+* [General](#general)
+   * [Cache compilations with `ccache`](#cache-compilations-with-ccache)
+   * [Disable features with `./configure`](#disable-features-with-configure)
+   * [Make use of your threads with `make -j`](#make-use-of-your-threads-with-make--j)
+   * [Multiple working directories with `git worktrees`](#multiple-working-directories-with-git-worktrees)
+* [Writing code](#writing-code)
+   * [Format C/C++/Protobuf diffs with `clang-format-diff.py`](#format-ccprotobuf-diffs-with-clang-format-diffpy)
+   * [Format Python diffs with `yapf-diff.py`](#format-python-diffs-with-yapf-diffpy)
+* [Rebasing/Merging code](#rebasingmerging-code)
+   * [More conflict context with `merge.conflictstyle diff3`](#more-conflict-context-with-mergeconflictstyle-diff3)
+* [Reviewing code](#reviewing-code)
+   * [Reduce mental load with `git diff` options](#reduce-mental-load-with-git-diff-options)
+   * [Reference PRs easily with `refspec`s](#reference-prs-easily-with-refspecs)
+   * [Diff the diffs with `git range-diff`](#diff-the-diffs-with-git-range-diff)
+
+General
+------
+
+### Cache compilations with `ccache`
+
+The easiest way to faster compile times is to cache compiles. `ccache` is a way to do so, from its description at the time of writing:
+
+> ccache is a compiler cache. It speeds up recompilation by caching the result of previous compilations and detecting when the same compilation is being done again. Supported languages are C, C++, Objective-C and Objective-C++.
+
+Install `ccache` through your distribution's package manager, and run `./configure` with your normal flags to pick it up.
+
+To use ccache for all your C/C++ projects, follow the symlinks method [here](https://ccache.samba.org/manual/latest.html#_run_modes) to set it up.
+
+### Disable features with `./configure`
+
+After running `./autogen.sh`, which generates the `./configure` file, use `./configure --help` to identify features that you can disable to save on compilation time. A few common flags:
+
+```sh
+--without-miniupnpc
+--disable-bench
+--disable-wallet
+--without-gui
+```
+
+### Make use of your threads with `make -j`
+
+If you have multiple threads on your machine, you can tell `make` to utilize all of them with:
+
+```sh
+make -j"$(($(nproc)+1))"
+```
+
+### Multiple working directories with `git worktrees`
+
+If you work with multiple branches or multiple copies of the repository, you should try `git worktrees`.
+
+To create a new branch that lives under a new working directory without disrupting your current working directory (useful for creating pull requests):
+```sh
+git worktree add -b my-shiny-new-branch ../living-at-my-new-working-directory based-on-my-crufty-old-commit-ish
+```
+
+To simply check out a commit-ish under a new working directory without disrupting your current working directory (useful for reviewing pull requests):
+```sh
+git worktree add --checkout ../where-my-checkout-commit-ish-will-live my-checkout-commit-ish
+```
+
+-----
+
+This synergizes well with [`ccache`](#cache-compilations-with-ccache) as objects resulting from unchanged code will most likely hit the cache and won't need to be recompiled.
+
+You can also set up [upstream refspecs](#reference-prs-easily-with-refspecs) to refer to pull requests easier in the above `git worktree` commands.
+
+Writing code
+------------
+
+### Format C/C++/Protobuf diffs with `clang-format-diff.py`
+
+See [contrib/devtools/README.md](contrib/devtools/README.md#clang-format-diff.py).
+
+### Format Python diffs with `yapf-diff.py`
+
+Usage is exactly the same as [`clang-format-diff.py`](#format-ccprotobuf-diffs-with-clang-format-diffpy). You can get it [here](https://github.com/MarcoFalke/yapf-diff).
+
+Rebasing/Merging code
+-------------
+
+### More conflict context with `merge.conflictstyle diff3`
+
+For resolving merge/rebase conflicts, it can be useful to enable diff3 style using `git config merge.conflictstyle diff3`. Instead of
+
+```diff
+<<<
+yours
+===
+theirs
+>>>
+```
+
+  you will see
+
+```diff
+<<<
+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).
+
+Reviewing code
+--------------
+
+### Reduce mental load with `git diff` options
+
+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. `--color-moved=dimmed-zebra` will also dim the coloring of moved hunks in the diff on compatible terminals.
+
+### Reference PRs easily with `refspec`s
+
+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.
+
+### Diff the diffs with `git range-diff`
+
+It is very common for contributors to rebase their pull requests, or make changes to commits (perhaps in response to review) that are not at the head of their branch. This poses a problem for reviewers as when the contributor force pushes, the reviewer is no longer sure that his previous reviews of commits are still valid (as the commit hashes can now be different even though the diff is semantically the same). `git range-diff` can help solve this problem by diffing the diffs.
+
+For example, to identify the differences between your previously reviewed diffs P1-5, and the new diffs P1-2,N3-4 as illustrated below:
+```
+       P1--P2--P3--P4--P5   <-- previously-reviewed-head
+      /
+...--m   <-- master
+      \
+       P1--P2--N3--N4--N5   <-- new-head (with P3 slightly modified)
+```
+
+You can do:
+```sh
+git range-diff master previously-reviewed-head new-head
+```
+
+Note that `git range-diff` also work for rebases.
+
+-----
+
+`git range-diff` also accepts normal `git diff` options, see [Reduce mental load with `git diff` options](#reduce-mental-load-with-git-diff-options) for useful `git diff` options.
+
+You can also set up [upstream refspecs](#reference-prs-easily-with-refspecs) to refer to pull requests easier in the above `git range-diff` commands.

doc/translation_strings_policy.md

@@ -33,25 +33,25 @@ General recommendations
 
 Try not to burden translators with translating messages that are e.g. slight variations of other messages.
 In the GUI, avoid the use of text where an icon or symbol will do.
-Make sure that placeholder texts in forms don't end up in the list of strings to be translated (use `<string notr="true">`).
+Make sure that placeholder texts in forms do not end up in the list of strings to be translated (use `<string notr="true">`).
 
 ### Make translated strings understandable
 
-Try to write translation strings in an understandable way, for both the user and the translator. Avoid overly technical or detailed messages
+Try to write translation strings in an understandable way, for both the user and the translator. Avoid overly technical or detailed messages.
 
 ### Do not translate internal errors
 
-Do not translate internal errors, or log messages, or messages that appear on the RPC interface. If an error is to be shown to the user,
+Do not translate internal errors, log messages, or messages that appear on the RPC interface. If an error is to be shown to the user,
-use a translatable generic message, then log the detailed message to the log. E.g. "A fatal internal error occurred, see debug.log for details".
+use a translatable generic message, then log the detailed message to the log. E.g., "A fatal internal error occurred, see debug.log for details".
 This helps troubleshooting; if the error is the same for everyone, the likelihood is increased that it can be found using a search engine.
 
 ### Avoid fragments
 
-Avoid dividing up a message into fragments. Translators see every string separately, so may misunderstand the context if the messages are not self-contained.
+Avoid dividing up a message into fragments. Translators see every string separately, so they may misunderstand the context if the messages are not self-contained.
 
 ### Avoid HTML in translation strings
 
-There have been difficulties with use of HTML in translation strings; translators should not be able to accidentally affect the formatting of messages.
+There have been difficulties with the use of HTML in translation strings; translators should not be able to accidentally affect the formatting of messages.
 This may sometimes be at conflict with the recommendation in the previous section.
 
 ### Plurals
@@ -66,7 +66,7 @@ Plurals can be complex in some languages. A quote from the gettext documentation
     25-31 pliko'w
     and so on
 
-In Qt code use tr's third argument for optional plurality. For example:
+In Qt code, use tr's third argument for optional plurality. For example:
 
     tr("%n hour(s)","",secs/HOUR_IN_SECONDS);
     tr("%n day(s)","",secs/DAY_IN_SECONDS);
@@ -82,7 +82,7 @@ This adds `<numerusform>`s to the respective `.ts` file, which can be translated
         </translation>
     </message>
 
-Where it is possible try to avoid embedding numbers into the flow of the string at all. e.g.
+Where possible, try to avoid embedding numbers into the flow of the string at all. E.g.,
 
     WARNING: check your network connection, %d blocks received in the last %d hours (%d expected)
 

src/Makefile.am

@@ -142,6 +142,7 @@ BITCOIN_CORE_H = \
   coinjoin/coinjoin-util.h \
   coins.h \
   compat.h \
+  compat/assumptions.h \
   compat/byteswap.h \
   compat/endian.h \
   compat/sanity.h \

src/compat/assumptions.h

@@ -0,0 +1,49 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-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.
+
+// Compile-time verification of assumptions we make.
+
+#ifndef BITCOIN_COMPAT_ASSUMPTIONS_H
+#define BITCOIN_COMPAT_ASSUMPTIONS_H
+
+#include <limits>
+
+// Assumption: We assume that the macro NDEBUG is not defined.
+// Example(s): We use assert(...) extensively with the assumption of it never
+//             being a noop at runtime.
+#if defined(NDEBUG)
+# error "Dash Core cannot be compiled without assertions."
+#endif
+
+// Assumption: We assume the floating-point types to fulfill the requirements of
+//             IEC 559 (IEEE 754) standard.
+// Example(s): Floating-point division by zero in ConnectBlock, CreateTransaction
+//             and EstimateMedianVal.
+static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed");
+static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
+
+// Assumption: We assume eight bits per byte (obviously, but remember: don't
+//             trust -- verify!).
+// Example(s): Everywhere :-)
+static_assert(std::numeric_limits<unsigned char>::digits == 8, "8-bit byte assumed");
+
+// Assumption: We assume floating-point widths.
+// Example(s): Type punning in serialization code (ser_{float,double}_to_uint{32,64}).
+static_assert(sizeof(float) == 4, "32-bit float assumed");
+static_assert(sizeof(double) == 8, "64-bit double assumed");
+
+// Assumption: We assume integer widths.
+// Example(s): GetSizeOfCompactSize and WriteCompactSize in the serialization
+//             code.
+static_assert(sizeof(short) == 2, "16-bit short assumed");
+static_assert(sizeof(int) == 4, "32-bit int assumed");
+
+// Some important things we are NOT assuming (non-exhaustive list):
+// * We are NOT assuming a specific value for sizeof(std::size_t).
+// * We are NOT assuming a specific value for std::endian::native.
+// * We are NOT assuming a specific value for std::locale("").name().
+// * We are NOT assuming a specific value for std::numeric_limits<char>::is_signed.
+
+#endif // BITCOIN_COMPAT_ASSUMPTIONS_H

src/key.cpp

@@ -226,7 +226,7 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
     secp256k1_ecdsa_recoverable_signature sig;
     int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, nullptr);
     assert(ret);
-    secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &sig);
+    ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &sig);
     assert(ret);
     assert(rec != -1);
     vchSig[0] = 27 + rec + (fCompressed ? 4 : 0);

src/qt/transactionview.cpp

@@ -216,8 +216,8 @@ void TransactionView::setModel(WalletModel *_model)
         transactionView->setAlternatingRowColors(true);
         transactionView->setSelectionBehavior(QAbstractItemView::SelectRows);
         transactionView->setSelectionMode(QAbstractItemView::ExtendedSelection);
+        transactionView->horizontalHeader()->setSortIndicator(TransactionTableModel::Date, Qt::DescendingOrder);
         transactionView->setSortingEnabled(true);
-        transactionView->sortByColumn(TransactionTableModel::Date, Qt::DescendingOrder);
         transactionView->verticalHeader()->hide();
 
         transactionView->setColumnWidth(TransactionTableModel::Status, STATUS_COLUMN_WIDTH);

src/scheduler.h

@@ -45,13 +45,13 @@ public:
     // Call func at/after time t
     void schedule(Function f, boost::chrono::system_clock::time_point t=boost::chrono::system_clock::now());
 
-    // Convenience method: call f once deltaSeconds from now
+    // Convenience method: call f once deltaMilliSeconds from now
     void scheduleFromNow(Function f, int64_t deltaMilliSeconds);
 
     // Another convenience method: call f approximately
-    // every deltaSeconds forever, starting deltaSeconds from now.
+    // every deltaMilliSeconds forever, starting deltaMilliSeconds from now.
     // To be more precise: every time f is finished, it
-    // is rescheduled to run deltaSeconds later. If you
+    // is rescheduled to run deltaMilliSeconds later. If you
     // need more accurate scheduling, don't use this method.
     void scheduleEvery(Function f, int64_t deltaMilliSeconds);
 

src/test/coins_tests.cpp

@@ -278,6 +278,8 @@ UtxoData::iterator FindRandomFrom(const std::set<COutPoint> &utxoSet) {
 // has the expected effect (the other duplicate is overwritten at all cache levels)
 BOOST_AUTO_TEST_CASE(updatecoins_simulation_test)
 {
+    SeedInsecureRand(/* deterministic */ true);
+
     bool spent_a_duplicate_coinbase = false;
     // A simple map to track what we expect the cache stack to represent.
     std::map<COutPoint, Coin> result;

src/util/system.h

@@ -17,6 +17,7 @@
 
 #include <attributes.h>
 #include <compat.h>
+#include <compat/assumptions.h>
 #include <fs.h>
 #include <logging.h>
 #include <sync.h>

test/README.md

@@ -179,7 +179,8 @@ cat /tmp/user/1000/testo9vsdjo3/node1/regtest/dashd.pid
 gdb /home/example/dashd <pid>
 ```
 
-Note: gdb attach step may require `sudo`
+Note: gdb attach step may require ptrace_scope to be modified, or `sudo` preceding the `gdb`.
+See this link for considerations: https://www.kernel.org/doc/Documentation/security/Yama.txt
 
 ### Util tests
 

test/functional/p2p_sendheaders.py

@@ -494,7 +494,7 @@ class SendHeadersTest(BitcoinTestFramework):
 
         # Now announce a header that forks the last two blocks
         tip = blocks[0].sha256
-        height -= 1
+        height -= 2
         blocks = []
 
         # Create extra blocks for later

test/functional/wallet_basic.py

@@ -325,12 +325,38 @@ class WalletTest(BitcoinTestFramework):
         tx_obj = self.nodes[0].gettransaction(txid)
         assert_equal(tx_obj['amount'], Decimal('-0.0001'))
 
+        # General checks for errors from incorrect inputs
         # This will raise an exception because the amount type is wrong
         assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "1f-4")
 
         # This will raise an exception since generate does not accept a string
         assert_raises_rpc_error(-1, "not an integer", self.nodes[0].generate, "2")
 
+        # This will raise an exception for the invalid private key format
+        assert_raises_rpc_error(-5, "Invalid private key encoding", self.nodes[0].importprivkey, "invalid")
+
+        # This will raise an exception for importing an address with the PS2H flag
+        temp_address = self.nodes[1].getnewaddress()
+        assert_raises_rpc_error(-5, "Cannot use the p2sh flag with an address - use a script instead", self.nodes[0].importaddress, temp_address, "label", False, True)
+
+        # This will raise an exception for attempting to dump the private key of an address you do not own
+        assert_raises_rpc_error(-4, "Private key for address %s is not known" % temp_address, self.nodes[0].dumpprivkey, temp_address)
+
+        # This will raise an exception for attempting to get the private key of an invalid Dash address
+        assert_raises_rpc_error(-5, "Invalid Dash address", self.nodes[0].dumpprivkey, "invalid")
+
+        # This will raise an exception for attempting to set a label for an invalid Dash address
+        assert_raises_rpc_error(-5, "Invalid Dash address", self.nodes[0].setlabel, "invalid address", "label")
+
+        # This will raise an exception for importing an invalid address
+        assert_raises_rpc_error(-5, "Invalid Dash address or script", self.nodes[0].importaddress, "invalid")
+
+        # This will raise an exception for attempting to import a pubkey that isn't in hex
+        assert_raises_rpc_error(-5, "Pubkey must be a hex string", self.nodes[0].importpubkey, "not hex")
+
+        # This will raise an exception for importing an invalid pubkey
+        assert_raises_rpc_error(-5, "Pubkey is not a valid public key", self.nodes[0].importpubkey, "5361746f736869204e616b616d6f746f")
+
         # Import address and private key to check correct behavior of spendable unspents
         # 1. Send some coins to generate new UTXO
         address_to_import = self.nodes[2].getnewaddress()