merge bitcoin #16953 #16918 #16917 #16898 #14696 #16888 #16737 #16404 #15687 #16294 : Backport

.cirrus.yml

@@ -9,6 +9,7 @@ task:
     MAKEJOBS: "-j9"
     CONFIGURE_OPTS: "--disable-dependency-tracking"
     GOAL: "install"
+    TEST_RUNNER_PORT_MIN: "14000"  # Must be larger than 12321, which is used for the http cache. See https://cirrus-ci.org/guide/writing-tasks/#http-cache
     CCACHE_SIZE: "200M"
     CCACHE_COMPRESS: 1
     CCACHE_DIR: "/tmp/ccache_dir"

doc/zmq.md

@@ -154,7 +154,9 @@ using other means such as firewalling.
 
 Note that when the block chain tip changes, a reorganisation may occur
 and just the tip will be notified. It is up to the subscriber to
-retrieve the chain from the last known block to the new tip.
+retrieve the chain from the last known block to the new tip. Also note
+that no notification occurs if the tip was in the active chain - this
+is the case after calling invalidateblock RPC.
 
 There are several possibilities that ZMQ notification can get lost
 during transmission depending on the communication type you are

src/consensus/tx_check.cpp

@@ -28,7 +28,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state)
     if (tx.vExtraPayload.size() > MAX_TX_EXTRA_PAYLOAD)
         return state.DoS(100, false, REJECT_INVALID, "bad-txns-payload-oversize");
 
-    // Check for negative or overflow output values
+    // Check for negative or overflow output values (see CVE-2010-5139)
     CAmount nValueOut = 0;
     for (const auto& txout : tx.vout) {
         if (txout.nValue < 0)

src/net_processing.cpp

@@ -3220,7 +3220,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 }
                 AddOrphanTx(ptx, pfrom->GetId());
 
-                // DoS prevention: do not allow mapOrphanTransactions to grow unbounded
+                // DoS prevention: do not allow mapOrphanTransactions to grow unbounded (see CVE-2012-3789)
                 unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000;
                 unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTxSize);
                 if (nEvicted > 0) {

src/qt/test/apptests.cpp

@@ -12,6 +12,8 @@
 #include <qt/networkstyle.h>
 #include <qt/rpcconsole.h>
 #include <shutdown.h>
+#include <test/setup_common.h>
+#include <univalue.h>
 #include <validation.h>
 
 #if defined(HAVE_CONFIG_H)
@@ -31,8 +33,6 @@
 #include <QtGlobal>
 #include <QtTest/QtTestWidgets>
 #include <QtTest/QtTestGui>
-#include <string>
-#include <univalue.h>
 
 namespace {
 //! Call getblockchaininfo RPC and check first field of JSON output.
@@ -67,6 +67,7 @@ void AppTests::appTests()
     }
 #endif
 
+    BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
     ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
     LogInstance().DisconnectTestLogger();
 

src/qt/test/paymentservertests.cpp

@@ -13,7 +13,7 @@
 #include <random.h>
 #include <script/script.h>
 #include <script/standard.h>
-#include <util/system.h>
+#include <test/setup_common.h>
 #include <util/strencodings.h>
 
 #include <openssl/x509.h>
@@ -66,7 +66,7 @@ static SendCoinsRecipient handleRequest(PaymentServer* server, std::vector<unsig
 
 void PaymentServerTests::paymentServerTests()
 {
-    SelectParams(CBaseChainParams::MAIN);
+    BasicTestingSetup testing_setup(CBaseChainParams::MAIN);
     auto node = interfaces::MakeNode();
     OptionsModel optionsModel(*node);
     PaymentServer* server = new PaymentServer(nullptr, false);

src/qt/test/rpcnestedtests.cpp

@@ -40,9 +40,6 @@ void RPCNestedTests::rpcNestedTests()
     tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]);
     //mempool.setSanityCheck(1.0);
 
-    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
-    LogInstance().DisconnectTestLogger();
-
     TestingSetup test;
 
     if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();

src/qt/test/test_main.cpp

@@ -44,12 +44,18 @@ Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin);
 #endif
 #endif
 
-extern void noui_connect();
-
 // This is all you need to run all the tests
 int main(int argc, char *argv[])
 {
-    BasicTestingSetup test{CBaseChainParams::REGTEST};
+    // Initialize persistent globals with the testing setup state for sanity.
+    // E.g. -datadir in gArgs is set to a temp directory dummy value (instead
+    // of defaulting to the default datadir), or globalChainParams is set to
+    // regtest params.
+    //
+    // All tests must use their own testing setup (if needed).
+    {
+        BasicTestingSetup dummy{CBaseChainParams::REGTEST};
+    }
 
     auto node = interfaces::MakeNode();
 

src/script/interpreter.cpp

@@ -338,7 +338,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                 opcode == OP_MUL ||
                 opcode == OP_LSHIFT ||
                 opcode == OP_RSHIFT)
-                return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes.
+                return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes (CVE-2010-5137).
 
             // With SCRIPT_VERIFY_CONST_SCRIPTCODE, OP_CODESEPARATOR is rejected even in an unexecuted branch
             if (opcode == OP_CODESEPARATOR && sigversion == SigVersion::BASE && (flags & SCRIPT_VERIFY_CONST_SCRIPTCODE))
@@ -1599,6 +1599,8 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne
         return set_error(serror, SCRIPT_ERR_SIG_PUSHONLY);
     }
 
+    // scriptSig and scriptPubKey must be evaluated sequentially on the same stack
+    // rather than being simply concatenated (see CVE-2010-5141)
     std::vector<std::vector<unsigned char> > stack, stackCopy;
     if (!EvalScript(stack, scriptSig, flags, checker, SigVersion::BASE, serror))
         // serror is set

src/test/README.md

@@ -1,3 +1,15 @@
+# Unit tests
+
+The sources in this directory are unit test cases. Boost includes a
+unit testing framework, and since Dash Core already uses Boost, it makes
+sense to simply use this framework rather than require developers to
+configure some other framework (we want as few impediments to creating
+unit tests as possible).
+
+The build system is set up to compile an executable called `test_dash`
+that runs all of the unit tests. The main source file is called
+`setup_common.cpp`.
+
 ### Compiling/running unit tests
 
 Unit tests will be automatically compiled if dependencies were met in `./configure`
@@ -12,7 +24,7 @@ to run the dashd tests.
 
 To add more dashd tests, add `BOOST_AUTO_TEST_CASE` functions to the existing
 .cpp files in the `test/` directory or add new .cpp files that
-implement new BOOST_AUTO_TEST_SUITE sections.
+implement new `BOOST_AUTO_TEST_SUITE` sections.
 
 To run the dash-qt tests manually, launch `src/qt/test/test_dash-qt`
 
@@ -32,20 +44,24 @@ example, to run just the getarg_tests verbosely:
 
 Run `test_dash --help` for the full list.
 
-### Note on adding test cases
-
-The sources in this directory are unit test cases.  Boost includes a
-unit testing framework, and since Dash Core already uses boost, it makes
-sense to simply use this framework rather than require developers to
-configure some other framework (we want as few impediments to creating
-unit tests as possible).
+### Adding test cases
 
-The build system is setup to compile an executable called `test_dash`
-that runs all of the unit tests.  The main source file is called
-setup_common.cpp. To add a new unit test file to our test suite you need
+To add a new unit test file to our test suite you need
 to add the file to `src/Makefile.test.include`. The pattern is to create
 one test file for each class or source file for which you want to create
-unit tests.  The file naming convention is `<source_filename>_tests.cpp`
+unit tests. The file naming convention is `<source_filename>_tests.cpp`
 and such files should wrap their tests in a test suite
 called `<source_filename>_tests`. For an example of this pattern,
-examine `uint256_tests.cpp`.
+see `uint256_tests.cpp`.
+
+### Logging and debugging in unit tests
+
+To write to logs from unit tests you need to use specific message methods
+provided by Boost. The simplest is `BOOST_TEST_MESSAGE`.
+
+For debugging you can launch the test_dash executable with `gdb`or `lldb` and
+start debugging, just like you would with dashd:
+
+```bash
+gdb src/test/test_dash
+```

src/test/data/script_tests.json

@@ -827,6 +827,9 @@
 ["NOP", "2SWAP 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"],
 ["1", "2 3 2SWAP 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"],
 
+["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"],
+
+["TEST DISABLED OP CODES (CVE-2010-5137)"],
 ["'a' 'bc'", "CAT", "P2SH,STRICTENC", "DISABLED_OPCODE"],
 ["'abc' 'a' 'bc'", "CAT EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK"],
 ["'' '' ''", "CAT EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK"],
@@ -909,9 +912,6 @@
 ["0x05 0x0100800080", "BIN2NUM -8388609 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"],
 ["0x05 0x01000f0000", "BIN2NUM 983041 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"],
 ["0x05 0x01000f0080", "BIN2NUM -983041 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"],
-
-["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"],
-
 ["Disabled opcodes"],
 ["'abc'", "IF INVERT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "INVERT disabled"],
 ["2 0 IF 2MUL ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "2MUL disabled"],

src/validation.cpp

@@ -2052,7 +2052,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
     // If such overwrites are allowed, coinbases and transactions depending upon those
     // can be duplicated to remove the ability to spend the first instance -- even after
     // being sent to another address.
-    // See BIP30 and http://r6.ca/blog/20120206T005236Z.html for more information.
+    // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information.
     // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool
     // already refuses previously-known transaction ids entirely.
     // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC.
@@ -3739,6 +3739,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
             return state.DoS(100, false, REJECT_INVALID, "bad-cb-multiple", false, "more than one coinbase");
 
     // Check transactions
+    // Must check for duplicate inputs (see CVE-2018-17144)
     for (const auto& tx : block.vtx)
         if (!CheckTransaction(*tx, state))
             return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),

test/README.md

@@ -141,8 +141,10 @@ killall dashd
 
 ##### Test logging
 
-The tests contain logging at different levels (debug, info, warning, etc). By
-default:
+The tests contain logging at five different levels (DEBUG, INFO, WARNING, ERROR
+and CRITICAL). From within your functional tests you can log to these different
+levels using the logger included in the test_framework, e.g.
+`self.log.debug(object)`. By default:
 
 - when run through the test_runner harness, *all* logs are written to
   `test_framework.log` and no logs are output to the console.
@@ -187,18 +189,32 @@ call methods that interact with the dashd nodes-under-test.
 If further introspection of the dashd instances themselves becomes
 necessary, this can be accomplished by first setting a pdb breakpoint
 at an appropriate location, running the test to that point, then using
-`gdb` to attach to the process and debug.
+`gdb` (or `lldb` on macOS) to attach to the process and debug.
 
-For instance, to attach to `self.node[1]` during a run:
+For instance, to attach to `self.node[1]` during a run you can get
+the pid of the node within `pdb`.
+
+```
+(pdb) self.node[1].process.pid
+```
+
+Alternatively, you can find the pid by inspecting the temp folder for the specific test
+you are running. The path to that folder is printed at the beginning of every
+test run:
 
 ```bash
 2017-06-27 14:13:56.686000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testo9vsdjo3
 ```
 
-use the directory path to get the pid from the pid file:
+Use the path to find the pid file in the temp folder:
 
 ```bash
 cat /tmp/user/1000/testo9vsdjo3/node1/regtest/dashd.pid
+```
+
+Then you can use the pid to start `gdb`:
+
+```bash
 gdb /home/example/dashd <pid>
 ```
 

test/functional/feature_assumevalid.py

@@ -40,7 +40,7 @@
     CTxIn,
     CTxOut,
     msg_block,
-    msg_headers
+    msg_headers,
 )
 from test_framework.mininode import P2PInterface
 from test_framework.script import (CScript, OP_TRUE)
@@ -190,7 +190,7 @@ def run_test(self):
         for i in range(200):
             p2p1.send_message(msg_block(self.blocks[i]))
         # Syncing so many blocks can take a while on slow systems. Give it plenty of time to sync.
-        p2p1.sync_with_ping(300)
+        p2p1.sync_with_ping(960)
         assert_equal(self.nodes[1].getblock(self.nodes[1].getbestblockhash())['height'], 200)
 
         # Send blocks to node2. Block 102 will be rejected.

test/functional/feature_block.py

@@ -765,7 +765,7 @@ def run_test(self):
         #
         # Blocks are not allowed to contain a transaction whose id matches that of an earlier,
         # not-fully-spent transaction in the same chain. To test, make identical coinbases;
-        # the second one should be rejected.
+        # the second one should be rejected. See also CVE-2012-1909.
         #
         self.log.info("Reject a block with a transaction with a duplicate hash of a previous transaction (BIP30)")
         self.move_tip(60)

test/functional/feature_dbcrash.py

@@ -50,7 +50,7 @@ class ChainstateWriteCrashTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 4
         self.setup_clean_chain = False
-        self.rpc_timeout = 180
+        self.rpc_timeout = 480
 
         # Set -maxmempool=0 to turn off mempool memory sharing with dbcache
         # Set -rpcservertimeout=900 to reduce socket disconnects in this

test/functional/feature_notifications.py

@@ -7,7 +7,11 @@
 
 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, wait_until, connect_nodes_bi
+from test_framework.util import (
+    assert_equal,
+    wait_until,
+    connect_nodes,
+)
 
 
 class NotificationsTest(BitcoinTestFramework):
@@ -59,7 +63,7 @@ def run_test(self):
             self.log.info("test -walletnotify after rescan")
             # restart node to rescan to force wallet notifications
             self.start_node(1)
-            connect_nodes_bi(self.nodes, 0, 1)
+            connect_nodes(self.nodes[0], 1)
 
             wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 

test/functional/feature_pruning.py

@@ -85,6 +85,7 @@ def set_test_params(self):
             ["-dip3params=2000:2000", "-dip8params=2000", "-disablegovernance","-txindex=0","-maxreceivebuffer=20000","-blockmaxsize=999000"],
             ["-dip3params=2000:2000", "-dip8params=2000", "-disablegovernance","-txindex=0","-prune=550"],
         ]
+        self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()