fix: reviewing TODOes at v19 (#5303)

During reviewing TODO were found some TODOes that can be done now.

 - fix: follow-up dash#3467 - replaced commented code to disabled code
- follow-up bitcoin#16394 - uncommented code related to `watchonly`
feature
 - removed out-dated TODO in `rpc/masternode.cpp` (already done)
- fix: renamed name of clean up test_unittests: removed TODO and updated
name of variable TRAVIS
 - rewritten todo inside `.travis.yml`
 - fix: adds a missing description for result of rpc `mnsync`

Last commit (`mnsync`) is an only candidate for backport to v19, other
changes are non significant.

Run functional/unit tests

No breaking changes

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

.travis.yml

@@ -176,7 +176,7 @@ before_script:
     - python3 -c 'import os,sys,fcntl; flags = fcntl.fcntl(sys.stdout, fcntl.F_GETFL); fcntl.fcntl(sys.stdout, fcntl.F_SETFL, flags&~os.O_NONBLOCK);'
     # Build docker image only for develop branch of the main repo
     - if [ "$TRAVIS_REPO_SLUG" != "dashpay/dash" -o "$TRAVIS_BRANCH" != "develop" -o "$TRAVIS_PULL_REQUEST" != "false" ]; then export DOCKER_BUILD="false"; echo DOCKER_BUILD=$DOCKER_BUILD; fi
-    # TODO: Check keys and signed commits
+    # TODO(ignore if don't use travis): Check keys and signed commits
     #- if [ "$TRAVIS_REPO_SLUG" = "dashpay/dash" -a "$TRAVIS_PULL_REQUEST" = "false" ]; then while read LINE; do travis_retry gpg --keyserver hkp://subset.pool.sks-keyservers.net --recv-keys $LINE; done < contrib/verify-commits/trusted-keys; fi
     #- if [ "$TRAVIS_REPO_SLUG" = "dashpay/dash" -a "$TRAVIS_EVENT_TYPE" = "cron" ]; then travis_wait 30 contrib/verify-commits/verify-commits.py; fi
 after_script:

ci/dash/test_unittests.sh

@@ -16,8 +16,7 @@ if [ "$RUN_UNIT_TESTS" != "true" ]; then
   exit 0
 fi
 
-# TODO this is not Travis agnostic
-export BOOST_TEST_RANDOM=1$TRAVIS_BUILD_ID
+export BOOST_TEST_RANDOM=${BOOST_TEST_RANDOM:-1}
 export LD_LIBRARY_PATH=$BASE_BUILD_DIR/depends/$HOST/lib
 
 export WINEDEBUG=fixme-all

src/rpc/masternode.cpp

@@ -82,7 +82,6 @@ static UniValue masternode_connect(const JSONRPCRequest& request)
     if (!Lookup(strAddress, addr, 0, false))
         throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Incorrect masternode address %s", strAddress));
 
-    // TODO: Pass CConnman instance somehow and don't use global variable.
     NodeContext& node = EnsureNodeContext(request.context);
     node.connman->OpenMasternodeConnection(CAddress(addr, NODE_NETWORK));
     if (!node.connman->IsConnected(CAddress(addr, NODE_NETWORK), CConnman::AllNodes))

src/rpc/misc.cpp

@@ -79,8 +79,21 @@ static UniValue mnsync(const JSONRPCRequest& request)
         {
             {"mode", RPCArg::Type::STR, RPCArg::Optional::NO, "[status|next|reset]"},
         },
-        RPCResults{},/*TODO*/
-        RPCExamples{""}
+        {
+            RPCResult{"for mode = status",
+                RPCResult::Type::OBJ, "", "",
+                {
+                    {RPCResult::Type::NUM, "AssetID", "The asset ID"},
+                    {RPCResult::Type::STR, "AssetName", "The asset name"},
+                    {RPCResult::Type::NUM, "AssetStartTime", "The asset start time"},
+                    {RPCResult::Type::NUM, "Attempt", "The attempt"},
+                    {RPCResult::Type::BOOL, "IsBlockchainSynced", "true if the blockchain synced"},
+                    {RPCResult::Type::BOOL, "IsSynced", "true if synced"},
+                }},
+            RPCResult{"for mode = next|reset",
+                RPCResult::Type::STR, "", ""},
+        },
+        RPCExamples{""},
     }.Check(request);
 
     std::string strMode = request.params[0].get_str();

test/functional/feature_dbcrash.py

@@ -232,6 +232,8 @@ class ChainstateWriteCrashTest(BitcoinTestFramework):
         # Syncing the blocks could cause nodes to crash, so the test begins here.
         self.sync_node3blocks(block_hashes_to_sync)
 
+        starting_tip_height = self.nodes[3].getblockcount()
+
         # Main test loop:
         # each time through the loop, generate a bunch of transactions,
         # and then either mine a single new block on the tip, or some-sized reorg.
@@ -242,14 +244,15 @@ class ChainstateWriteCrashTest(BitcoinTestFramework):
             # Pick a random block between current tip, and starting tip
             current_height = self.nodes[3].getblockcount()
             # TODO: re-enable this when ReplayBlocks is fixed to support evodb and additional indexes
-            # random_height = random.randint(starting_tip_height, current_height)
-            # self.log.debug("At height %d, considering height %d", current_height, random_height)
-            # if random_height > starting_tip_height:
-            #     # Randomly reorg from this point with some probability (1/4 for
-            #     # tip, 1/5 for tip-1, ...)
-            #     if random.random() < 1.0 / (current_height + 4 - random_height):
-            #         self.log.debug("Invalidating block at height %d", random_height)
-            #         self.nodes[3].invalidateblock(self.nodes[3].getblockhash(random_height))
+            skip_this_test_ReplayBlocks = True
+            random_height = random.randint(starting_tip_height, current_height)
+            self.log.debug("At height %d, considering height %d", current_height, random_height)
+            if not skip_this_test_ReplayBlocks and random_height > starting_tip_height:
+                # Randomly reorg from this point with some probability (1/4 for
+                # tip, 1/5 for tip-1, ...)
+                if random.random() < 1.0 / (current_height + 4 - random_height):
+                    self.log.debug("Invalidating block at height %d", random_height)
+                    self.nodes[3].invalidateblock(self.nodes[3].getblockhash(random_height))
 
             # Now generate new blocks until we pass the old tip height
             self.log.debug("Mining longer tip")

test/functional/wallet_createwallet.py

@@ -123,12 +123,11 @@ class CreateWalletTest(BitcoinTestFramework):
         w7 = node.get_wallet_rpc('w7')
         assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 10)
 
-        # TODO: renable this when avoid reuse flag is added
-        # self.log.info('Test making a wallet with avoid reuse flag')
-        # self.nodes[0].createwallet('w8', False, False, '', True) # Use positional arguments to check for bug where avoid_reuse could not be set for wallets without needing them to be encrypted
-        # w8 = node.get_wallet_rpc('w8')
-        # assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 10)
-        # assert_equal(w8.getwalletinfo()["avoid_reuse"], True)
+        self.log.info('Test making a wallet with avoid reuse flag')
+        self.nodes[0].createwallet('w8', False, False, '', True) # Use positional arguments to check for bug where avoid_reuse could not be set for wallets without needing them to be encrypted
+        w8 = node.get_wallet_rpc('w8')
+        assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 10)
+        assert_equal(w8.getwalletinfo()["avoid_reuse"], True)
 
         self.log.info('Using a passphrase with private keys disabled returns error')
         assert_raises_rpc_error(-4, 'Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.', self.nodes[0].createwallet, wallet_name='w9', disable_private_keys=True, passphrase='thisisapassphrase')