Merge pull request #4610 from vijaydasmp/bp196

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
+### Adding test cases
 
-The sources in this directory are unit test cases.  Boost includes a
+To add a new unit test file to our test suite you need
-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 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 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`
 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

@@ -2068,7 +2068,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.
@@ -3755,6 +3755,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
+The tests contain logging at five different levels (DEBUG, INFO, WARNING, ERROR
-default:
+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 @@ from test_framework.messages import (
     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 @@ class AssumeValidTest(BitcoinTestFramework):
         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 @@ class FullBlockTest(BitcoinTestFramework):
         #
         # 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 @@ import os
 
 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 @@ class NotificationsTest(BitcoinTestFramework):
             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 @@ class PruneTest(BitcoinTestFramework):
             ["-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()

test/functional/interface_zmq.py

@@ -12,11 +12,10 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import dashhash
 from test_framework.util import (
     assert_equal,
+    connect_nodes,
     hash256,
     )
 
-ADDRESS = "tcp://127.0.0.1:28332"
-
 def dashhash_helper(b):
     return encode(dashhash(b)[::-1], 'hex_codec').decode('ascii')
 
@@ -48,62 +47,59 @@ class ZMQTest (BitcoinTestFramework):
         self.skip_if_no_py3_zmq()
         self.skip_if_no_bitcoind_zmq()
 
-    def setup_nodes(self):
+    def run_test(self):
         import zmq
+        self.ctx = zmq.Context()
+        try:
+            self.test_basic()
+            self.test_reorg()
+        finally:
+            # Destroy the ZMQ context.
+            self.log.debug("Destroying ZMQ context")
+            self.ctx.destroy(linger=None)
 
-        # Initialize ZMQ context and socket.
+    def test_basic(self):
         # All messages are received in the same socket which means
         # that this test fails if the publishing order changes.
         # Note that the publishing order is not defined in the documentation and
         # is subject to change.
-        self.zmq_context = zmq.Context()
+        import zmq
-        socket = self.zmq_context.socket(zmq.SUB)
+        address = 'tcp://127.0.0.1:28332'
+        socket = self.ctx.socket(zmq.SUB)
         socket.set(zmq.RCVTIMEO, 60000)
-        socket.connect(ADDRESS)
+        socket.connect(address)
 
         # Subscribe to all available topics.
-        self.hashblock = ZMQSubscriber(socket, b"hashblock")
+        hashblock = ZMQSubscriber(socket, b"hashblock")
-        self.hashtx = ZMQSubscriber(socket, b"hashtx")
+        hashtx = ZMQSubscriber(socket, b"hashtx")
-        self.rawblock = ZMQSubscriber(socket, b"rawblock")
+        rawblock = ZMQSubscriber(socket, b"rawblock")
-        self.rawtx = ZMQSubscriber(socket, b"rawtx")
+        rawtx = ZMQSubscriber(socket, b"rawtx")
 
-        self.extra_args = [
-            ["-zmqpub%s=%s" % (sub.topic.decode(), ADDRESS) for sub in [self.hashblock, self.hashtx, self.rawblock, self.rawtx]],
-            []
-        ]
-        self.add_nodes(self.num_nodes, self.extra_args)
-        self.start_nodes()
 
-    def run_test(self):
+        self.restart_node(0, ["-zmqpub%s=%s" % (sub.topic.decode(), address) for sub in [hashblock, hashtx, rawblock, rawtx]])
-        try:
+        connect_nodes(self.nodes[0], 1)
-            self._zmq_test()
-        finally:
-            # Destroy the ZMQ context.
-            self.log.debug("Destroying ZMQ context")
-            self.zmq_context.destroy(linger=None)
-
-    def _zmq_test(self):
         num_blocks = 5
         self.log.info("Generate %(n)d blocks (and %(n)d coinbase txes)" % {"n": num_blocks})
         genhashes = self.nodes[0].generatetoaddress(num_blocks, ADDRESS_BCRT1_UNSPENDABLE)
+
         self.sync_all()
 
         for x in range(num_blocks):
             # Should receive the coinbase txid.
-            txid = self.hashtx.receive()
+            txid = hashtx.receive()
 
             # Should receive the coinbase raw transaction.
-            hex = self.rawtx.receive()
+            hex = rawtx.receive()
             assert_equal(hash256(hex), txid)
 
             # Should receive the generated block hash.
-            hash = self.hashblock.receive().hex()
+            hash = hashblock.receive().hex()
             assert_equal(genhashes[x], hash)
             # The block should only have the coinbase txid.
             assert_equal([txid.hex()], self.nodes[1].getblock(hash)["tx"])
 
             # Should receive the generated raw block.
-            block = self.rawblock.receive()
+            block = rawblock.receive()
             assert_equal(genhashes[x], dashhash_helper(block[:80]))
 
         if self.is_wallet_compiled():
@@ -112,23 +108,47 @@ class ZMQTest (BitcoinTestFramework):
             self.sync_all()
 
             # Should receive the broadcasted txid.
-            txid = self.hashtx.receive()
+            txid = hashtx.receive()
             assert_equal(payment_txid, txid.hex())
 
             # Should receive the broadcasted raw transaction.
-            hex = self.rawtx.receive()
+            hex = rawtx.receive()
             assert_equal(payment_txid, hash256(hex).hex())
 
 
         self.log.info("Test the getzmqnotifications RPC")
         assert_equal(self.nodes[0].getzmqnotifications(), [
-            {"type": "pubhashblock", "address": ADDRESS, "hwm": 1000},
+            {"type": "pubhashblock", "address": address, "hwm": 1000},
-            {"type": "pubhashtx", "address": ADDRESS, "hwm": 1000},
+            {"type": "pubhashtx", "address": address, "hwm": 1000},
-            {"type": "pubrawblock", "address": ADDRESS, "hwm": 1000},
+            {"type": "pubrawblock", "address": address, "hwm": 1000},
-            {"type": "pubrawtx", "address": ADDRESS, "hwm": 1000},
+            {"type": "pubrawtx", "address": address, "hwm": 1000},
         ])
 
         assert_equal(self.nodes[1].getzmqnotifications(), [])
 
+    def test_reorg(self):
+        import zmq
+        address = 'tcp://127.0.0.1:28333'
+        socket = self.ctx.socket(zmq.SUB)
+        socket.set(zmq.RCVTIMEO, 60000)
+        socket.connect(address)
+        hashblock = ZMQSubscriber(socket, b'hashblock')
+
+        # Should only notify the tip if a reorg occurs
+        self.restart_node(0, ['-zmqpub%s=%s' % (hashblock.topic.decode(), address)])
+
+        # Generate 1 block in nodes[0] and receive all notifications
+        self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
+        assert_equal(self.nodes[0].getbestblockhash(), hashblock.receive().hex())
+
+        # Generate 2 blocks in nodes[1]
+        self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_UNSPENDABLE)
+
+        # nodes[0] will reorg chain after connecting back nodes[1]
+        connect_nodes(self.nodes[0], 1)
+
+        # Should receive nodes[1] tip
+        assert_equal(self.nodes[1].getbestblockhash(), hashblock.receive().hex())
+
 if __name__ == '__main__':
     ZMQTest().main()

test/functional/mempool_accept.py

@@ -215,6 +215,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
             rawtxs=[tx.serialize().hex()],
         )
 
+        # The following two validations prevent overflow of the output amounts (see CVE-2010-5139).
         self.log.info('A transaction with too large output value')
         tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference)))
         tx.vout[0].nValue = 21000000 * COIN + 1

test/functional/p2p_connect_to_devnet.py

@@ -6,7 +6,7 @@
 
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, connect_nodes_bi
+from test_framework.util import assert_equal, connect_nodes
 
 class ConnectDevnetNodes(BitcoinTestFramework):
     def set_test_params(self):
@@ -18,13 +18,13 @@ class ConnectDevnetNodes(BitcoinTestFramework):
         self.add_nodes(self.num_nodes)
         self.start_node(0)
         self.start_node(1)
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         self.sync_all()
 
 
     def run_test(self):
         self.nodes[0].add_p2p_connection(P2PInterface())
-        assert_equal(self.nodes[0].getconnectioncount(), 3)  # 2 in/out dashd + 1 p2p
+        assert_equal(self.nodes[0].getconnectioncount(), 2)  # 1 out dashd + 1 p2p
 
 
 if __name__ == '__main__':

test/functional/p2p_disconnect_ban.py

@@ -8,7 +8,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
-    connect_nodes_bi,
+    connect_nodes,
     wait_until,
 )
 
@@ -17,6 +17,10 @@ class DisconnectBanTest(BitcoinTestFramework):
         self.num_nodes = 2
 
     def run_test(self):
+        self.log.info("Connect nodes both way")
+        connect_nodes(self.nodes[0], 1)
+        connect_nodes(self.nodes[1], 0)
+
         self.log.info("Test setban and listbanned RPCs")
 
         self.log.info("setban: successfully ban single IP address")
@@ -69,7 +73,9 @@ class DisconnectBanTest(BitcoinTestFramework):
 
         # Clear ban lists
         self.nodes[1].clearbanned()
-        connect_nodes_bi(self.nodes, 0, 1)
+        self.log.info("Connect nodes both way")
+        connect_nodes(self.nodes[0], 1)
+        connect_nodes(self.nodes[1], 0)
 
         self.log.info("Test disconnectnode RPCs")
 
@@ -88,7 +94,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         assert not [node for node in self.nodes[0].getpeerinfo() if node['addr'] == address1]
 
         self.log.info("disconnectnode: successfully reconnect node")
-        connect_nodes_bi(self.nodes, 0, 1)  # reconnect the node
+        connect_nodes(self.nodes[0], 1)  # reconnect the node
         assert_equal(len(self.nodes[0].getpeerinfo()), 2)
         assert [node for node in self.nodes[0].getpeerinfo() if node['addr'] == address1]
 

test/functional/p2p_invalid_block.py

@@ -54,10 +54,11 @@ class InvalidBlockRequestTest(BitcoinTestFramework):
         block_time = best_block["time"] + 1
 
         # Use merkle-root malleability to generate an invalid block with
-        # same blockheader.
+        # same blockheader (CVE-2012-2459).
         # Manufacture a block with 3 transactions (coinbase, spend of prior
         # coinbase, spend of that spend).  Duplicate the 3rd transaction to
         # leave merkle root and blockheader unchanged but invalidate the block.
+        # For more information on merkle-root malleability see src/consensus/merkle.cpp.
         self.log.info("Test merkle root malleability.")
 
         block2 = create_block(tip, create_coinbase(height), block_time)
@@ -81,15 +82,16 @@ class InvalidBlockRequestTest(BitcoinTestFramework):
         assert block2_orig.vtx != block2.vtx
 
         node.p2p.send_blocks_and_test([block2], node, success=False, request_block=False, reject_reason='bad-txns-duplicate')
-        # Check transactions for duplicate inputs
+        # Check transactions for duplicate inputs (CVE-2018-17144)
         self.log.info("Test duplicate input block.")
 
-        block2_orig.vtx[2].vin.append(block2_orig.vtx[2].vin[0])
+        block2_dup = copy.deepcopy(block2_orig)
-        block2_orig.vtx[2].rehash()
+        block2_dup.vtx[2].vin.append(block2_dup.vtx[2].vin[0])
-        block2_orig.hashMerkleRoot = block2_orig.calc_merkle_root()
+        block2_dup.vtx[2].rehash()
-        block2_orig.rehash()
+        block2_dup.hashMerkleRoot = block2_dup.calc_merkle_root()
-        block2_orig.solve()
+        block2_dup.rehash()
-        node.p2p.send_blocks_and_test([block2_orig], node, success=False, request_block=False, reject_reason='bad-txns-inputs-duplicate')
+        block2_dup.solve()
+        node.p2p.send_blocks_and_test([block2_dup], node, success=False, reject_reason='bad-txns-inputs-duplicate')
 
         self.log.info("Test very broken block.")
 
@@ -104,5 +106,31 @@ class InvalidBlockRequestTest(BitcoinTestFramework):
 
         node.p2p.send_blocks_and_test([block3], node, success=False, request_block=False, reject_reason='bad-cb-amount')
 
+        # Complete testing of CVE-2012-2459 by sending the original block.
+        # It should be accepted even though it has the same hash as the mutated one.
+
+        self.log.info("Test accepting original block after rejecting its mutated version.")
+        node.p2p.send_blocks_and_test([block2_orig], node, success=True, timeout=5)
+
+        # Update tip info
+        height += 1
+        block_time += 1
+        tip = int(block2_orig.hash, 16)
+
+        # Complete testing of CVE-2018-17144, by checking for the inflation bug.
+        # Create a block that spends the output of a tx in a previous block.
+        block4 = create_block(tip, create_coinbase(height), block_time)
+        tx3 = create_tx_with_script(tx2, 0, script_sig=b'\x51', amount=50 * COIN)
+
+        # Duplicates input
+        tx3.vin.append(tx3.vin[0])
+        tx3.rehash()
+        block4.vtx.append(tx3)
+        block4.hashMerkleRoot = block4.calc_merkle_root()
+        block4.rehash()
+        block4.solve()
+        self.log.info("Test inflation by duplicating input")
+        node.p2p.send_blocks_and_test([block4], node, success=False,  reject_reason='bad-txns-inputs-duplicate')
+
 if __name__ == '__main__':
     InvalidBlockRequestTest().main()

test/functional/p2p_invalid_messages.py

@@ -81,7 +81,7 @@ class InvalidMessagesTest(BitcoinTestFramework):
 
             # Peer 1, despite serving up a bunch of nonsense, should still be connected.
             self.log.info("Waiting for node to drop junk messages.")
-            node.p2p.sync_with_ping(timeout=30)
+            node.p2p.sync_with_ping(timeout=320)
             assert node.p2p.is_connected
 
         #

test/functional/p2p_node_network_limited.py

@@ -11,7 +11,7 @@ and that it responds to getdata requests for blocks correctly:
 from test_framework.messages import CInv, msg_getdata, NODE_BLOOM, NODE_NETWORK_LIMITED, msg_verack
 from test_framework.mininode import P2PInterface, mininode_lock
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, disconnect_nodes, connect_nodes_bi, sync_blocks, wait_until
+from test_framework.util import assert_equal, disconnect_nodes, connect_nodes, sync_blocks, wait_until
 
 class P2PIgnoreInv(P2PInterface):
     firstAddrnServices = 0
@@ -58,7 +58,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         assert_equal(int(self.nodes[0].getnetworkinfo()['localservices'], 16), expected_services)
 
         self.log.info("Mine enough blocks to reach the NODE_NETWORK_LIMITED range.")
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         blocks = self.nodes[1].generatetoaddress(292, self.nodes[1].get_deterministic_priv_key().address)
         sync_blocks([self.nodes[0], self.nodes[1]])
 
@@ -84,7 +84,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
 
         # connect unsynced node 2 with pruned NODE_NETWORK_LIMITED peer
         # because node 2 is in IBD and node 0 is a NODE_NETWORK_LIMITED peer, sync must not be possible
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
         try:
             sync_blocks([self.nodes[0], self.nodes[2]], timeout=5)
         except:
@@ -93,7 +93,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         assert_equal(self.nodes[2].getblockheader(self.nodes[2].getbestblockhash())['height'], 0)
 
         # now connect also to node 1 (non pruned)
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
 
         # sync must be possible
         sync_blocks(self.nodes)
@@ -105,7 +105,7 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         self.nodes[0].generatetoaddress(10, self.nodes[0].get_deterministic_priv_key().address)
 
         # connect node1 (non pruned) with node0 (pruned) and check if the can sync
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
 
         # sync must be possible, node 1 is no longer in IBD and should therefore connect to node 0 (NODE_NETWORK_LIMITED)
         sync_blocks([self.nodes[0], self.nodes[1]])

test/functional/rpc_fundrawtransaction.py

@@ -12,7 +12,7 @@ from test_framework.util import (
     assert_greater_than,
     assert_greater_than_or_equal,
     assert_raises_rpc_error,
-    connect_nodes_bi,
+    connect_nodes,
     count_bytes,
     find_vout_for_address,
 )
@@ -36,10 +36,10 @@ class RawTransactionsTest(BitcoinTestFramework):
     def setup_network(self):
         self.setup_nodes()
 
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
-        connect_nodes_bi(self.nodes, 0, 3)
+        connect_nodes(self.nodes[0], 3)
 
     def run_test(self):
         min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee']
@@ -475,10 +475,10 @@ class RawTransactionsTest(BitcoinTestFramework):
         for node in self.nodes:
             node.settxfee(min_relay_tx_fee)
 
-        connect_nodes_bi(self.nodes,0,1)
+        connect_nodes(self.nodes[0], 1)
-        connect_nodes_bi(self.nodes,1,2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes,0,2)
+        connect_nodes(self.nodes[0], 2)
-        connect_nodes_bi(self.nodes,0,3)
+        connect_nodes(self.nodes[0], 3)
         # Again lock the watchonly UTXO or nodes[0] may spend it, because
         # lockunspent is memory-only and thus lost on restart
         self.nodes[0].lockunspent(False, [{"txid": watchonly_txid, "vout": watchonly_vout}])

test/functional/rpc_fundrawtransaction_hd.py

@@ -7,7 +7,7 @@ from decimal import Decimal
 
 from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, assert_greater_than, connect_nodes_bi
+from test_framework.util import assert_equal, assert_greater_than, connect_nodes
 
 # Create one-input, one-output, no-fee transaction:
 class RawTransactionsTest(BitcoinTestFramework):
@@ -22,10 +22,10 @@ class RawTransactionsTest(BitcoinTestFramework):
 
     def setup_network(self):
         super().setup_network()
-        connect_nodes_bi(self.nodes,0,1)
+        connect_nodes(self.nodes[0],1)
-        connect_nodes_bi(self.nodes,1,2)
+        connect_nodes(self.nodes[1],2)
-        connect_nodes_bi(self.nodes,0,2)
+        connect_nodes(self.nodes[0],2)
-        connect_nodes_bi(self.nodes,0,3)
+        connect_nodes(self.nodes[0],3)
 
     def run_test(self):
         self.log.info("Mining blocks...")
@@ -451,10 +451,10 @@ class RawTransactionsTest(BitcoinTestFramework):
         for node in self.nodes:
             node.settxfee(min_relay_tx_fee)
 
-        connect_nodes_bi(self.nodes,0,1)
+        connect_nodes(self.nodes[0],1)
-        connect_nodes_bi(self.nodes,1,2)
+        connect_nodes(self.nodes[1],2)
-        connect_nodes_bi(self.nodes,0,2)
+        connect_nodes(self.nodes[0],2)
-        connect_nodes_bi(self.nodes,0,3)
+        connect_nodes(self.nodes[0],3)
         self.sync_all()
 
         # drain the keypool

test/functional/rpc_invalidateblock.py

@@ -7,7 +7,7 @@
 import time
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, connect_nodes, connect_nodes_bi, wait_until
+from test_framework.util import assert_equal, connect_nodes, wait_until
 
 class InvalidateTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -29,7 +29,7 @@ class InvalidateTest(BitcoinTestFramework):
         assert self.nodes[1].getblockcount() == 6
 
         self.log.info("Connect nodes to force a reorg")
-        connect_nodes_bi(self.nodes,0,1)
+        connect_nodes(self.nodes[0], 1)
         self.sync_blocks(self.nodes[0:2])
         assert self.nodes[0].getblockcount() == 6
         badhash = self.nodes[1].getblockhash(2)
@@ -42,7 +42,7 @@ class InvalidateTest(BitcoinTestFramework):
             raise AssertionError("Wrong tip for node0, hash %s, height %d"%(newhash,newheight))
 
         self.log.info("Make sure we won't reorg to a lower work chain:")
-        connect_nodes_bi(self.nodes,1,2)
+        connect_nodes(self.nodes[1], 2)
         self.log.info("Sync node 2 to node 1 so both have 6 blocks")
         self.sync_blocks(self.nodes[1:3])
         assert self.nodes[2].getblockcount() == 6
@@ -65,7 +65,7 @@ class InvalidateTest(BitcoinTestFramework):
         self.log.info("Make sure ResetBlockFailureFlags does the job correctly")
         self.restart_node(0, extra_args=["-checkblocks=5"])
         self.restart_node(1, extra_args=["-checkblocks=5"])
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         self.nodes[0].generate(10)
         self.sync_blocks(self.nodes[0:2])
         newheight = self.nodes[0].getblockcount()

test/functional/rpc_net.py

@@ -13,7 +13,7 @@ from test_framework.util import (
     assert_greater_than_or_equal,
     assert_greater_than,
     assert_raises_rpc_error,
-    connect_nodes_bi,
+    connect_nodes,
     p2p_port,
     wait_until,
 )
@@ -50,13 +50,16 @@ class NetTest(BitcoinTestFramework):
     def setup_network(self):
         self.disable_mocktime()
         self.setup_nodes()
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
 
     def run_test(self):
         # Wait for one ping/pong to finish so that we can be sure that there is no chatter between nodes for some time
         # Especially the exchange of messages like getheaders and friends causes test failures here
         self.nodes[0].ping()
         wait_until(lambda: all(['pingtime' in n for n in self.nodes[0].getpeerinfo()]))
+        self.log.info('Connect nodes both way')
+        connect_nodes(self.nodes[0], 1)
+        connect_nodes(self.nodes[1], 0)
 
         self._test_connection_count()
         self._test_getnettotals()
@@ -66,7 +69,7 @@ class NetTest(BitcoinTestFramework):
         self._test_getnodeaddresses()
 
     def _test_connection_count(self):
-        # connect_nodes_bi connects each node to the other
+        # connect_nodes connects each node to the other
         assert_equal(self.nodes[0].getconnectioncount(), 2)
 
     def _test_getnettotals(self):
@@ -110,7 +113,10 @@ class NetTest(BitcoinTestFramework):
         wait_until(lambda: self.nodes[1].getnetworkinfo()['connections'] == 0, timeout=3)
 
         self.nodes[0].setnetworkactive(state=True)
-        connect_nodes_bi(self.nodes, 0, 1)
+        self.log.info('Connect nodes both way')
+        connect_nodes(self.nodes[0], 1)
+        connect_nodes(self.nodes[1], 0)
+
         assert_equal(self.nodes[0].getnetworkinfo()['networkactive'], True)
         assert_equal(self.nodes[0].getnetworkinfo()['connections'], 2)
 

test/functional/rpc_preciousblock.py

@@ -7,7 +7,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes_bi,
+    connect_nodes,
     sync_blocks,
 )
 
@@ -61,7 +61,7 @@ class PreciousTest(BitcoinTestFramework):
         self.log.info("Connect nodes and check no reorg occurs")
         # Submit competing blocks via RPC so any reorg should occur before we proceed (no way to wait on inaction for p2p sync)
         node_sync_via_rpc(self.nodes[0:2])
-        connect_nodes_bi(self.nodes,0,1)
+        connect_nodes(self.nodes[0], 1)
         assert_equal(self.nodes[0].getbestblockhash(), hashC)
         assert_equal(self.nodes[1].getbestblockhash(), hashG)
         self.log.info("Make Node0 prefer block G")
@@ -98,8 +98,8 @@ class PreciousTest(BitcoinTestFramework):
         hashL = self.nodes[2].getbestblockhash()
         self.log.info("Connect nodes and check no reorg occurs")
         node_sync_via_rpc(self.nodes[1:3])
-        connect_nodes_bi(self.nodes,1,2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes,0,2)
+        connect_nodes(self.nodes[0], 2)
         assert_equal(self.nodes[0].getbestblockhash(), hashH)
         assert_equal(self.nodes[1].getbestblockhash(), hashH)
         assert_equal(self.nodes[2].getbestblockhash(), hashL)

test/functional/rpc_rawtransaction.py

@@ -17,7 +17,13 @@ from decimal import Decimal
 from io import BytesIO
 from test_framework.messages import CTransaction, ToHex
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, assert_raises_rpc_error, connect_nodes_bi, hex_str_to_bytes
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+    connect_nodes,
+    hex_str_to_bytes,
+)
+
 
 class multidict(dict):
     """Dictionary that allows duplicate keys.
@@ -53,7 +59,7 @@ class RawTransactionsTest(BitcoinTestFramework):
 
     def setup_network(self):
         super().setup_network()
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
 
     def run_test(self):
         self.log.info('prepare some coins for multiple *rawtransaction commands')

test/functional/test_framework/test_framework.py

@@ -37,7 +37,6 @@ from .util import (
     MAX_NODES,
     assert_equal,
     check_json_precision,
-    connect_nodes_bi,
     connect_nodes,
     copy_datadir,
     disconnect_nodes,
@@ -326,8 +325,18 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         # Connect the nodes as a "chain".  This allows us
         # to split the network between nodes 1 and 2 to get
         # two halves that can work on competing chains.
+        #
+        # Topology looks like this:
+        # node0 <-- node1 <-- node2 <-- node3
+        #
+        # If all nodes are in IBD (clean chain from genesis), node0 is assumed to be the source of blocks (miner). To
+        # ensure block propagation, all nodes will establish outgoing connections toward node0.
+        # See fPreferredDownload in net_processing.
+        #
+        # If further outbound connections are needed, they can be added at the beginning of the test with e.g.
+        # connect_nodes(self.nodes[1], 2)
         for i in range(self.num_nodes - 1):
-            connect_nodes_bi(self.nodes, i, i + 1)
+            connect_nodes(self.nodes[i + 1], i)
         self.sync_all()
 
     def setup_nodes(self):
@@ -459,7 +468,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         """
         Join the (previously split) network halves together.
         """
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
         self.sync_all()
 
     def sync_blocks(self, nodes=None, **kwargs):

test/functional/test_framework/util.py

@@ -9,6 +9,7 @@ from base64 import b64encode
 from binascii import unhexlify
 from decimal import Decimal, ROUND_DOWN
 import hashlib
+from subprocess import CalledProcessError
 import inspect
 import json
 import logging
@@ -16,7 +17,6 @@ import os
 import random
 import shutil
 import re
-from subprocess import CalledProcessError
 import time
 
 from . import coverage
@@ -37,6 +37,13 @@ def set_timeout_scale(_timeout_scale):
 # Assert functions
 ##################
 
+def assert_approx(v, vexp, vspan=0.00001):
+    """Assert that `v` is within `vspan` of `vexp`"""
+    if v < vexp - vspan:
+        raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
+    if v > vexp + vspan:
+        raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
+
 def assert_fee_amount(fee, tx_size, fee_per_kB):
     """Assert the fee was in range"""
     target_fee = round(tx_size * fee_per_kB / 1000, 8)
@@ -254,10 +261,11 @@ def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), sleep=
 # The maximum number of nodes a single test can spawn
 MAX_NODES = 15
 # Don't assign rpc or p2p ports lower than this
-PORT_MIN = 11000
+PORT_MIN = int(os.getenv('TEST_RUNNER_PORT_MIN', default=11000))
 # The number of ports to "reserve" for p2p and rpc, each
 PORT_RANGE = 5000
 
+
 class PortSeed:
     # Must be initialized with a unique integer for each process
     n = None
@@ -445,10 +453,6 @@ def connect_nodes(from_connection, node_num):
     wait_until(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
     wait_until(lambda: all(peer['bytesrecv_per_msg'].pop('verack', 0) == 24 for peer in from_connection.getpeerinfo()))
 
-def connect_nodes_bi(nodes, a, b):
-    connect_nodes(nodes[a], b)
-    connect_nodes(nodes[b], a)
-
 def isolate_node(node, timeout=5):
     node.setnetworkactive(False)
     st = time.time()

test/functional/tool_wallet.py

@@ -1,18 +1,25 @@
 #!/usr/bin/env python3
-# Copyright (c) 2018 The Bitcoin Core developers
+# Copyright (c) 2018-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.
 """Test dash-wallet."""
+
+import hashlib
+import os
+import stat
 import subprocess
 import textwrap
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
+BUFFER_SIZE = 16 * 1024
+
 class ToolWalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.setup_clean_chain = True
+        self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -32,23 +39,54 @@ class ToolWalletTest(BitcoinTestFramework):
     def assert_tool_output(self, output, *args):
         p = self.dash_wallet_process(*args)
         stdout, stderr = p.communicate()
-        assert_equal(p.poll(), 0)
         assert_equal(stderr, '')
         assert_equal(stdout, output)
+        assert_equal(p.poll(), 0)
 
-    def run_test(self):
+    def wallet_shasum(self):
+        h = hashlib.sha1()
+        mv = memoryview(bytearray(BUFFER_SIZE))
+        with open(self.wallet_path, 'rb', buffering=0) as f:
+            for n in iter(lambda : f.readinto(mv), 0):
+                h.update(mv[:n])
+        return h.hexdigest()
 
+    def wallet_timestamp(self):
+        return os.path.getmtime(self.wallet_path)
+
+    def wallet_permissions(self):
+        return oct(os.lstat(self.wallet_path).st_mode)[-3:]
+
+    def log_wallet_timestamp_comparison(self, old, new):
+        result = 'unchanged' if new == old else 'increased!'
+        self.log.debug('Wallet file timestamp {}'.format(result))
+
+    def test_invalid_tool_commands_and_args(self):
+        self.log.info('Testing that various invalid commands raise with specific error messages')
         self.assert_raises_tool_error('Invalid command: foo', 'foo')
-        # `dash-wallet help` is an error. Use `dash-wallet -help`
+        # `dash-wallet help` raises an error. Use `dash-wallet -help`.
         self.assert_raises_tool_error('Invalid command: help', 'help')
         self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create')
         self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo')
         self.assert_raises_tool_error('Error loading wallet.dat. Is wallet being used by other process?', '-wallet=wallet.dat', 'info')
         self.assert_raises_tool_error('Error: no wallet file at nonexistent.dat', '-wallet=nonexistent.dat', 'info')
 
-        # stop the node to close the wallet to call info command
+    def test_tool_wallet_info(self):
+        # Stop the node to close the wallet to call the info command.
         self.stop_node(0)
-
+        self.log.info('Calling wallet tool info, testing output')
+        #
+        # TODO: Wallet tool info should work with wallet file permissions set to
+        # read-only without raising:
+        # "Error loading wallet.dat. Is wallet being used by another process?"
+        # The following lines should be uncommented and the tests still succeed:
+        #
+        # self.log.debug('Setting wallet file permissions to 400 (read-only)')
+        # os.chmod(self.wallet_path, stat.S_IRUSR)
+        # assert(self.wallet_permissions() in ['400', '666']) # Sanity check. 666 because Appveyor.
+        # shasum_before = self.wallet_shasum()
+        timestamp_before = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
         out = textwrap.dedent('''\
             Wallet info
             ===========
@@ -74,12 +112,35 @@ class ToolWalletTest(BitcoinTestFramework):
             Address Book: 0
         ''')
         self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
+        timestamp_after = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
+        self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
+        self.log.debug('Setting wallet file permissions back to 600 (read/write)')
+        os.chmod(self.wallet_path, stat.S_IRUSR | stat.S_IWUSR)
+        assert(self.wallet_permissions() in ['600', '666']) # Sanity check. 666 because Appveyor.
+        #
+        # TODO: Wallet tool info should not write to the wallet file.
+        # The following lines should be uncommented and the tests still succeed:
+        #
+        # assert_equal(timestamp_before, timestamp_after)
+        # shasum_after = self.wallet_shasum()
+        # assert_equal(shasum_before, shasum_after)
+        # self.log.debug('Wallet file shasum unchanged\n')
 
-        # mutate the wallet to check the info command output changes accordingly
+    def test_tool_wallet_info_after_transaction(self):
+        """
+        Mutate the wallet with a transaction to verify that the info command
+        output changes accordingly.
+        """
         self.start_node(0)
+        self.log.info('Generating transaction to mutate wallet')
         self.nodes[0].generate(1)
         self.stop_node(0)
 
+        self.log.info('Calling wallet tool info after generating a transaction, testing output')
+        shasum_before = self.wallet_shasum()
+        timestamp_before = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
         out = textwrap.dedent('''\
             Wallet info
             ===========
@@ -90,7 +151,22 @@ class ToolWalletTest(BitcoinTestFramework):
             Address Book: 0
         ''')
         self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
+        shasum_after = self.wallet_shasum()
+        timestamp_after = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
+        self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
+        #
+        # TODO: Wallet tool info should not write to the wallet file.
+        # This assertion should be uncommented and succeed:
+        # assert_equal(timestamp_before, timestamp_after)
+        assert_equal(shasum_before, shasum_after)
+        self.log.debug('Wallet file shasum unchanged\n')
 
+    def test_tool_wallet_create_on_existing_wallet(self):
+        self.log.info('Calling wallet tool create on an existing wallet, testing output')
+        shasum_before = self.wallet_shasum()
+        timestamp_before = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp before calling create: {}'.format(timestamp_before))
         out = textwrap.dedent('''\
             Topping up keypool...
             Wallet info
@@ -102,13 +178,46 @@ class ToolWalletTest(BitcoinTestFramework):
             Address Book: 0
         ''')
         self.assert_tool_output(out, '-wallet=foo', 'create')
+        shasum_after = self.wallet_shasum()
+        timestamp_after = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp after calling create: {}'.format(timestamp_after))
+        self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
+        assert_equal(timestamp_before, timestamp_after)
+        assert_equal(shasum_before, shasum_after)
+        self.log.debug('Wallet file shasum unchanged\n')
 
+    def test_getwalletinfo_on_different_wallet(self):
+        self.log.info('Starting node with arg -wallet=foo')
         self.start_node(0, ['-wallet=foo'])
+
+        self.log.info('Calling getwalletinfo on a different wallet ("foo"), testing output')
+        shasum_before = self.wallet_shasum()
+        timestamp_before = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp before calling getwalletinfo: {}'.format(timestamp_before))
         out = self.nodes[0].getwalletinfo()
         self.stop_node(0)
 
+        shasum_after = self.wallet_shasum()
+        timestamp_after = self.wallet_timestamp()
+        self.log.debug('Wallet file timestamp after calling getwalletinfo: {}'.format(timestamp_after))
+
         assert_equal(0, out['txcount'])
         assert_equal(1000, out['keypoolsize'])
 
+        self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
+        assert_equal(timestamp_before, timestamp_after)
+        assert_equal(shasum_after, shasum_before)
+        self.log.debug('Wallet file shasum unchanged\n')
+
+    def run_test(self):
+        self.wallet_path = os.path.join(self.nodes[0].datadir, 'regtest', 'wallets', 'wallet.dat')
+        self.test_invalid_tool_commands_and_args()
+        # Warning: The following tests are order-dependent.
+        self.test_tool_wallet_info()
+        self.test_tool_wallet_info_after_transaction()
+        self.test_tool_wallet_create_on_existing_wallet()
+        self.test_getwalletinfo_on_different_wallet()
+
+
 if __name__ == '__main__':
     ToolWalletTest().main()

test/functional/wallet_backup.py

@@ -44,6 +44,7 @@ class WalletBackupTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         # nodes 1, 2,3 are spenders, let's give them a keypool=100
         self.extra_args = [["-keypool=100"], ["-keypool=100"], ["-keypool=100"], []]
+        self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()

test/functional/wallet_basic.py

@@ -12,7 +12,7 @@ from test_framework.util import (
     assert_equal,
     assert_fee_amount,
     assert_raises_rpc_error,
-    connect_nodes_bi,
+    connect_nodes,
     count_bytes,
     wait_until,
 )
@@ -32,9 +32,9 @@ class WalletTest(BitcoinTestFramework):
         self.start_node(0)
         self.start_node(1)
         self.start_node(2)
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
         self.sync_all(self.nodes[0:3])
 
     def check_fee_amount(self, curr_balance, balance_with_fee, fee_per_byte, tx_size):
@@ -214,7 +214,7 @@ class WalletTest(BitcoinTestFramework):
         node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('100'), fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex']))
 
         self.start_node(3)
-        connect_nodes_bi(self.nodes, 0, 3)
+        connect_nodes(self.nodes[0], 3)
         self.sync_all()
 
         # check if we can list zero value tx as available coins
@@ -249,9 +249,9 @@ class WalletTest(BitcoinTestFramework):
         self.start_node(0, ["-walletbroadcast=0"])
         self.start_node(1, ["-walletbroadcast=0"])
         self.start_node(2, ["-walletbroadcast=0"])
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
         self.sync_all(self.nodes[0:3])
 
         txid_not_broadcast = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2)
@@ -276,9 +276,9 @@ class WalletTest(BitcoinTestFramework):
         self.start_node(0)
         self.start_node(1)
         self.start_node(2)
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
-        connect_nodes_bi(self.nodes, 0, 2)
+        connect_nodes(self.nodes[0], 2)
         self.sync_blocks(self.nodes[0:3])
 
         self.nodes[0].generate(1)

test/functional/wallet_groups.py

@@ -7,15 +7,10 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import CTransaction, FromHex, ToHex
 from test_framework.util import (
+    assert_approx,
     assert_equal,
 )
 
-def assert_approx(v, vexp, vspan=0.00001):
-    if v < vexp - vspan:
-        raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
-    if v > vexp + vspan:
-        raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
-
 class WalletGroupTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True

test/functional/wallet_hd.py

@@ -10,7 +10,7 @@ import os
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes_bi,
+    connect_nodes,
 )
 
 class WalletHDTest(BitcoinTestFramework):
@@ -31,7 +31,7 @@ class WalletHDTest(BitcoinTestFramework):
         self.stop_node(1)
         self.nodes[1].assert_start_raises_init_error(['-usehd=0'], "Error: Error loading : You can't disable HD on an already existing HD wallet")
         self.start_node(1)
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
 
         # Make sure we use hd, keep chainid
         chainid = self.nodes[1].getwalletinfo()['hdchainid']
@@ -92,7 +92,7 @@ class WalletHDTest(BitcoinTestFramework):
             assert_equal(hd_info_2["hdkeypath"], "m/44'/1'/0'/0/"+str(i))
             assert_equal(hd_info_2["hdchainid"], chainid)
         assert_equal(hd_add, hd_add_2)
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         self.sync_all()
 
         # Needs rescan
@@ -108,7 +108,7 @@ class WalletHDTest(BitcoinTestFramework):
         shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "llmq"))
         shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, self.chain, "wallets", "wallet.dat"))
         self.start_node(1, extra_args=self.extra_args[1])
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         self.sync_all()
         # Wallet automatically scans blocks older than key on startup
         assert_equal(self.nodes[1].getbalance(), NUM_HD_ADDS + 1)

test/functional/wallet_keypool_topup.py

@@ -16,7 +16,7 @@ import shutil
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes_bi,
+    connect_nodes,
 )
 
 
@@ -38,7 +38,7 @@ class KeypoolRestoreTest(BitcoinTestFramework):
         self.stop_node(1)
         shutil.copyfile(wallet_path, wallet_backup_path)
         self.start_node(1, self.extra_args[1])
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
 
         self.log.info("Generate keys for wallet")
         for _ in range(90):
@@ -57,7 +57,7 @@ class KeypoolRestoreTest(BitcoinTestFramework):
         self.stop_node(1)
         shutil.copyfile(wallet_backup_path, wallet_path)
         self.start_node(1, self.extra_args[1])
-        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes(self.nodes[0], 1)
         self.sync_all()
 
         self.log.info("Verify keypool is restored and balance is correct")

test/functional/wallet_listsinceblock.py

@@ -5,7 +5,13 @@
 """Test the listsincelast RPC."""
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, assert_array_result, assert_raises_rpc_error
+from test_framework.util import (
+    assert_array_result,
+    assert_equal,
+    assert_raises_rpc_error,
+    connect_nodes,
+)
+
 
 class ListSinceBlockTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -16,6 +22,9 @@ class ListSinceBlockTest (BitcoinTestFramework):
         self.skip_if_no_wallet()
 
     def run_test(self):
+        # All nodes are in IBD from genesis, so they'll need the miner (node2) to be an outbound connection, or have
+        # only one connection. (See fPreferredDownload in net_processing)
+        connect_nodes(self.nodes[1], 2)
         self.nodes[2].generate(101)
         self.sync_all()
 

test/lint/lint-circular-dependencies.sh

@@ -67,6 +67,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
     "llmq/signing -> net_processing -> llmq/signing"
     "llmq/signing_shares -> net_processing -> llmq/signing_shares"
     "logging -> util/system -> logging"
+    "logging -> util/system -> random -> logging"
     "masternode/payments -> validation -> masternode/payments"
     "net -> netmessagemaker -> net"
     "net_processing -> spork -> net_processing"